BIND 10 #640: cfgmgr hanging and command and configurations for no-longer-running components

[BIND 10 Development]

#640: cfgmgr hanging and command and configurations for no-longer-running
components
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:  major  |  Sprint-20120207
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 First, comments on the rest of the original branch (before you updated
 it based on the initial comments).

 '''some higher level points'''

 The relationship between bindctl and module CC session is complicated
 and difficult to understand.  For example, it took some time for me to
 understand the interaction of UIModuleCCSession and bindctl - these
 two (defined in separate modules) are tightly coupled and work based
 on some implicit assumption of each other's features.  So, to
 understand changes to the module CC session I needed to look into
 bindctl.  Is it only me, or is this a known issue and kind of a longer
 term TODO?  In any case, however, I'm not requesting it be resolved
 within this ticket.

 Maybe unrelated to this ticket but: does bindctl try to get the full
 latest spec and config data of all running modules on accepting every
 command?  If so, is the overhead acceptable?

 When the application doesn't die gracefully, sending the stop message
 can be skipped.  Is it a TODO thing when it supports some kind of
 RAII?  For that matter, the whatever successor of msgq may make it
 possible for cfgmgr to (easily) detect it when a specific module is
 disconnected.  If so, we may not have to have a "send stopping"
 primitive, either directly or indirectly.

 '''ccsession.py'''

 I've come up with additional comments on this file while reading other
 part of the branch.

 - request_specifications: it would be helpful if it had more detailed
   description, including it clears the previous data.  It would also
   be helpful how this method (via update_specs_and_config()) is
   expected to be used - and, for that matter, whether
   request_specification() is expected to be public or essentially
   private (in the latter case adding `__` will clarify the intent a
   bit clearer, although this will make some update to the test code).
 - request_specifications: not directly related to this ticket, but I
   suspect we can remove or at least revise the comment.

 '''ccsession_mock.py'''

 - I'd like to have some pydoc description for MockModuleCCSession.

 '''cfgmgr_test.py'''

 - This part is a bit confusing:
 {{{#!python
         # drop the two messages for now
         self.assertEqual(len(self.fake_session.message_queue), 2)
         self.fake_session.get_message("Cmdctl", None)
         self.fake_session.get_message("TestModule", None)

         self.assertEqual(len(self.fake_session.message_queue), 0)
 }}}
   It's probably for clearing the faked message queue for the
   convenience of subsequent tests, but this part itself raises some
   unrelated questions: what are these two messages?  Where do the
   magic string of "Cmdctl" or "TestModule" come from?  Maybe the
   answer can be found if I read the other tests above this part (I
   didn't), but unless these preceding tests are prerequisite of the
   added tests, I'd rather be free from the duty of understanding it.
   Also related, test_handle_msg() now looks too long and is generally
   difficult to understand when something is added.  I'd suggest
   separating the message handling tests into independent sub tests.  I
   suspect the handling of "stopping" can be a separate test case, and
   if so, it will much easier to understand.

 '''cmdctl.py.in'''

 - While the comment says: "If it is None, delete the module if it is
   known (and ignore otherwise)", it seems that None will be set in the
   "otherwise" case.  And, is it okay to ignore it anyway?  If this is
   a bogus input I think we should rather return an error, log it, or
   both.   Or, if it's an okay value, the comment should be updated.

 '''cmdctl_test.py'''

 - I think it should also test the case when arg[1] is NULL and args[0]
   is not in modules_spec.  See also the comment on cmdctl.py.

 '''bind10_src.py'''

 - Is "rest of the system" correct (or appropriate)?
 {{{#!python
         # If ccsession is still there, inform rest of the system this
 module
         # is stopping.
 }}}
   I guess it's only delivered to cfgmgr and cmdctl (and perhaps
   bindctl via cmdctl).
 - Is it possible that self.ccs is None?
 {{{#!python
         if self.ccs is not None:
             self.ccs.send_stopping()
 }}}

 '''ddns_test.py'''

 - Why doesn't this use `MockModuleCCSession`?

 '''Others'''

 - stats and xfrout don't seem to be tested.

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