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:  9
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei
 * totalhours:  6 => 9


Comment:

 Replying to [comment:9 jinmei]:
 > Replying to [comment:8 jelte]:
 >
 > >
 > > 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?
 >

 ack, actually, I created two, one for this, and one for the tests (which
 is IMO a similar but different task), #1506 and #1507

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

 right

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

 ok, removed

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

 I've added two, one when shutdown() is called, and right after the loop in
 run() (when everything is really done). I chose run() over main() as the
 location, because this way it is on the same level as the RUNNING log
 message.

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

 no not really. removed.

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

 correct on both accounts, i had added the line for a small experiment
 related to -v. Had removed the rest, but forgot this one. Removed now
 (used the constant 10 there since it was supposed to be removed right away
 anyway).

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

 Added. I am inclined to put in another one as well, in the run() loop, so
 an error does not stop the server on that level. After brief discussion on
 jabber (with you ;)) I decided against it and just added the comment
 there.

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

 done. Also added tests (not for logging, but to make sure the exceptions
 are caught)

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

 that didn't change anything :) but I added a comment

 > - 'process' and 'daemon' coexist.  it would be better to be
 >   consistent.
 >

 chose 'process'

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

 ok, changed update_acls to update_acl, and took over your description.

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

 oh, no. removed.

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