Opened 8 years ago

Closed 8 years ago

#1451 closed task (complete)

Create DDNS process module

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20111220
Component: ddns Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: DDNS
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 12.18 Internal?: no

Description

Create a module (similar to b10-xfrin, b10-xfrout, etc.) that runs as a separate process to handle DDNS UPDATE packets.

It's probably easier to have this module already contain a number of dummy methods for the various part of the DDNS handling.

Subtickets

Attachments (2)

ddns.diff (1.8 KB) - added by jinmei 8 years ago.
exception.diff (2.7 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111220

comment:3 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:4 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 6

Module created, including initial specfile, runnable daemon, log message file, (mostly empty) manpage source, (mostly empty) unit test file.

It does define a stub DDNSSession class, but leaves out the unixsocket class used in xfrout. We may well want to use that but we should implement that in #1454 (at which point we'll know what the result of #1452 looks like).

comment:5 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 follow-up: Changed 8 years ago by 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)
  • 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.
  • 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.
  • 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.
  • signal_handler: I suspect sys.exit should be outside of the if block.
  • 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.
  • 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?
  • 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)?

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.

man page

  • this may be obvious, but I think we should explain what "DDNS" stands for somewhere in documentation. In the current
  • do we need to cleanup this? Or complete it?
           TODO TODO
    
  • In the synopsis:
           b10-ddns [-v] [--verbose]
    
    (I'd rather hide the existence of this option if my understanding of these options is correct, but that aside) shouldn't this be [-v|--verbose]?
  • is it intentional that "zones" is not documented (for configuration)?

Others

  • I made some trivial (I believe) editorial fixes/cleanups.
  • Do we need changelog? too early?

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Thanks for the review

Replying to 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)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jinmei

Replying to 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)

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 Changed 8 years ago by jinmei

And one more thing: I think this should be "data source" (man page)

       process. When the b10-auth DNS server receives a DDNS update, b10-ddns
       updates the zone in the BIND 10 zone data store.

comment:12 in reply to: ↑ 9 Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei
  • Total Hours changed from 6 to 9

Replying to jinmei:

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

comment:13 follow-up: Changed 8 years ago by jinmei

It looks almost okay. I have two last comments.

  • I'm afraid it could be dangerous to do all shutdown jobs in the middle of a signal handler. For example, check_command() could still be called after shutdown(), but the latter may destruct some fundamental attribute of the class at that point. In general, I think it's safer to minimize the things within a signal handler (e.g., just setting a flag) and do other complicated jobs in the usual control flow. I have a proposed patch to implement this idea. (dddns.diff, attached)
  • TestMain? repeats the same pattern and I think it's better to combine them in a separate helper method. See attached diff (exception.diff)

If you are fine with these patched, please just apply them and merge.
If you simply don't agree, I'm okay with it for now; please feel free
to merge the current code. If you don't agree right now but want to
discuss it more, please continue.

Changed 8 years ago by jinmei

Changed 8 years ago by jinmei

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:15 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

It looks almost okay. I have two last comments.

  • I'm afraid it could be dangerous to do all shutdown jobs in the middle of a signal handler. For example, check_command() could still be called after shutdown(), but the latter may destruct some fundamental attribute of the class at that point. In general, I think it's safer to minimize the things within a signal handler (e.g., just setting a flag) and do other complicated jobs in the usual control flow. I have a proposed patch to implement this idea. (dddns.diff, attached)

Ah, now I get it; I was only looking at the code and didn't see the problem (as the proposed patch currently doesn't alter the behaviour). The comment 'and cleanup should go here' was incorrect, and probably the name too (these caused the confusion I think).

Applied your patch, but with a few amendments; I think the logging of the event really should go in the trigger function (as the biggest reason to have two log messages is to be able to see if it didn't shut down immediately and why). And I renamed the former shutdown() to shutdown_cleanup(), for added clarity. Also added a line in docstring that trigger_shutdown() is the function one needs when the module needs to be stopped.

This function is now a noop, which contradicts the no-unused-code principle, but as this whole dummy module is kind of a violation on that, I think it is ok.

  • TestMain? repeats the same pattern and I think it's better to combine them in a separate helper method. See attached diff (exception.diff)

If you are fine with these patched, please just apply them and merge.
If you simply don't agree, I'm okay with it for now; please feel free
to merge the current code. If you don't agree right now but want to
discuss it more, please continue.

Applied as-is.

I assume the changes are trivial enough, but for completeness, I am assigning this back to you for one last time ;)

comment:16 in reply to: ↑ 15 Changed 8 years ago by jinmei

Replying to jelte:

I assume the changes are trivial enough, but for completeness, I am assigning this back to you for one last time ;)

Looks okay, please merge.

comment:17 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:18 Changed 8 years ago by jinmei

  • Total Hours changed from 9 to 12.18

comment:19 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

Thanks, merged, closing ticket.

Note: See TracTickets for help on using tickets.