BIND 10 #1451: Create DDNS process module

[BIND 10 Development]

#1451: Create DDNS process module
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  major  |             Milestone:
                  Component:  DDNS   |  Sprint-20111220
                   Keywords:         |            Resolution:
            Defect Severity:  N/A    |             Sensitive:  0
Feature Depending on Ticket:  DDNS   |           Sub-Project:  DNS
        Add Hours to Ticket:  0      |  Estimated Difficulty:  6
                  Internal?:  0      |           Total Hours:  6
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Thanks for the review

 Replying to [comment:6 jinmei]:
 > '''ddns.py.in'''
 >
 > - general: I think we try to provide docstring for all
 >   classes/functions/methods by default (some existing python
 >   apps/modules are bad examples in this sense as many of them were
 >   implemented at an earlier stage of development and often skipped
 >   docs/tests with an excuse of lack of time)
 >

 of course. my bad.

 > - general: personally, I'm not convinced the use of threads for our
 >   Python servers is a good design.  In particular, xfrout results in a
 >   very complicated mixture of (nest of!) threads and asynchronous I/O,
 >   and kills a major advantage of thread-style: straightforward code
 >   logic.  The current snippet of ddns makes me afraid about it going
 >   to be a second version of xfrout spaghetti.  Since DNS transactions
 >   are generally quite simple in terms of communication style
 >   (basically a transaction consists of a sequence of one request and
 >   one response, and especially in the case of UDP there shouldn't be
 >   any blocking operation other than for receiving requests), my gut
 >   feeling is that a simple form of event-style design with an
 >   asynchronous I/O primitive is sufficient.  Maybe I'm wrong, but at
 >   least I'd like to leave this point open until we really implement
 >   this part.
 >

 Ok, I confess that I just grabbed this from xfrout, since that one had the
 most in common with ddns in terms of interaction with the rest of the
 system.

 > - general: While looking at the ddns code, I had a feeling that we are
 >   now duplicating the same code logic for several Python programs in
 >   initialization and main command/configuration loop, and wondered
 >   whether we might extract the common pattern (maybe with some
 >   generalization) into a shared module.  This may be beyond the scope
 >   of this ticket, and we may rather complete the ddns work faster with
 >   possibly duplicate code, but I thought it's at least worth
 >   considering.
 >

 Agree (but not right now ;)) (also goes for some of the testing stuff I
 guess, added another MyCCSession() dummy class today)

 > - config_handler: in this simple form I don't see the need for the
 >   'answer' variable, but maybe it's introduced with expectation of
 >   extending the method (in a way that would require an intermediate
 >   variable like this) pretty soon.  Same for command_handler.
 >

 That was the intention, to have it available to put error responses in
 when we add actual commands and config checking.

 > - signal_handler: I suspect sys.exit should be outside of the if
 >   block.
 >

 actually, it shouldn't be necessary to explicitely call exit() there
 (unless we have some loop outside of the Server run() call).

 Removed the exit() call. I switched the DDNSServer() call and the setting
 of the signal handler around though, to prevent a race-condition (a kill
 signal in the short time after the signal handler is set but before the
 DNSServer() is initialized would be ignored otherwise)

 > - signal_handler: is it a good idea to directly call shutdown() and
 >   exit here?  Assuming we would normally receive the signal in the
 >   main loop, may be it's better to set some flag to indicate the loop
 >   should be terminated?  That way we could unify the shutdown sequence
 >   both by a signal and by a command.  Also, I guess we should disable
 >   the signal handler (so we ignore these signals) once we receive it;
 >   otherwise shutdown() could be called in the middle of it.
 >

 That graceful shutdown is what shutdown() triggers. Just removed the exit
 :)

 > - main: ddns_server should be declared as global; otherwise it would
 >   be always None in signal_handler (note also that the fact that we
 >   cannot this in tests would indicate we'll need to think about how to
 >   test code here.)  But, more fundamentally, I would personally avoid
 >   relying on such a wider scope mutable variable.  Couldn't it be more
 >   sophisticated, e.g, by using a closure?
 >

 ddns_server is in scope there, even without the global keyword (otherwise
 it would trigger an exception, not a recognition of its value being None).
 the global keyword would only be necessary if we want to assign something
 to that name.

 But using a closure would indeed be better; see update, added a
 create_signal_handler(server) call, which returns the handler.

 > - does it make sense to do anything specific for -v (--verbose)?
 >   we should probably still accept it because the bind10 process
 >   may specify it, but shouldn't we simply ignore it (or perhaps
 >   dump a message saying it's obsoleted)?
 >

 I'm not entirely sure what it does for the other modules, but I would like
 to keep it, and make it override the logging settings (or add to it).
 Granted, I did not do that here (and looking at some the other modules, I
 think what they do is wrong, this may be something that warrants wider
 discussion).

 But ok, for now I've added an output message (to stdout; the user
 requested verbosity :p), that says it is not actually used.

 > '''ddns_test.py'''
 >
 > - It would be better to provide as many tests as possible.  For
 >   example, we should be able to test DDNSServer.config_handler and
 >   command_handler.
 >

 Added. Though config_handler is mostly a noop right now.

 Also added one to test whether signal_handler() calls shutdown()


 > '''man page'''
 > <snip>
 >

 I didn't do any external documentation, just provided the skeleton for it
 :p (also the reason of the things above). But ok ok, added some text.
 Anyway, addressed your suggestions now

 > '''Others'''
 > - I made some trivial (I believe) editorial fixes/cleanups.
 > - Do we need changelog?  too early?

 Don't know, if we end up with support for most functionality one big
 changelog entry would be nicer than a number of them ('added dummy',
 'added parsing and error response', 'added acls' etc).

 Anyway, we can discuss on jabber, if we do add entry, here's my proposal:

 [func] jelte
 Added a new module b10-ddns, intended to handle DDNS updates (RFC 2136).
 Currently it is a dummy-module, that can be started and stopped, but has
 no functionality yet.
 (Trac #1451, commit X)

-- 
Ticket URL: <http://bind10.isc.org/ticket/1451#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development