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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 jelte]:

 > > - 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
 >
 > Agree (but not right now ;)) (also goes for some of the testing stuff I
 guess, added another MyCCSession() dummy class today)

 Okay.  Create a ticket for it?

 > > - main: ddns_server should be declared as global
 >
 > 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.

 Ah, right, I misunderstood it and thought it was in a function (but I
 think it's actually better to put this logic in a separate function.
 See below).  In any case, the revised code with a closure looks good.

 > > '''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).

 Okay, actually, I thought it might be too early and it would be too
 verbose if we add an entry for every subtask.  So, I'm okay with
 making a single unified changelog entry later.

 Now, comments on the revised code itself.  I've noticed some other
 things I didn't notice (or did overlook) in the first review.  So I've
 added a few new points.

 (I made a few minor changes again)

 '''ddns.py'''

 - I'd remove DDNSSession._handle for now.  My understanding is that
   this is a specific way of defining a callable for a new thread.
   Unless/until we decide using threads and decide using them that
   specific way, this skeleton definition could be misleading.

 - the DDNSServer class: it may be nice if we log it when it's shutting
   down.  we may also want to log it exactly before the process exits
   (in main).

 - create_signal_handler: do we need to check if ddns_server is None?

 - main: in my vague memory don't we have unified log level policy?  If
   so, we should use a constant variable to represent the most
   appropriate one rather than hardcoding something like '10':
 {{{
         logger.debug(10, DDNS_STOPPED_BY_KEYBOARD)
 }}}

 - regarding this log, DDNS_STOPPED_BY_KEYBOARD doesn't seem to be
   correct.  At least it's already used somewhere else.

 - main: I guess we should catch other exceptions (with 'Exception')
   and log it should it ever happen.

 - main: it's probably better to extract the `__main__` block into a
   separate function (say main()) and we simply call it from the global
   context.  That way we can test that part too (and we need to test
   them if only to check all log messages are used).

 '''ddns_message.mes'''

 - there's a "normalizer" script for message files:
   tools/reorder_message_file.py (hmm it seems to require python2).  it
   may be a good idea to apply it.  If you do it, it would also be a
   good idea to add a comment that it's recommended to apply the script
   when someone updates the message file with a new ID.
 - 'process' and 'daemon' coexist.  it would be better to be
   consistent.

 '''man page'''

 - (also related to the spec file): on a closer look, this sounds a bit
   awkward:
 {{{
        DDNS. Each entry has one element called update_acls, which itself
 is a
        list of ACLs that define update permissions.
 }}}
   Since ACL stands for "access control list", "list of ACLs" sounds
   odd (unless it's really a list of lists).  update_acl"s" is awkward
   for the same reason.  My suggestion is to revise the spec item name
   to "update_acl", and update the description to:
 {{{
        DDNS. Each entry has one element called update_acl, which is a
        list of access control rules that define update permissions.
 }}}

 '''ddns_test.in'''

 Do we need this?  (If so it should be added to configure.ac)

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