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-20120221
                  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):

 Replying to [comment:14 jelte]:

 (let's leave bigger issues open for now.  They are basically well
 beyond the scope of this ticket.  Beside, we've already spent too much
 time on this)

 > > '''cfgmgr_test.py'''
 [...]
 > Refactored the entire test, split it into four separate ones (and redid
 a bit of the internals)

 It generally looks good, but see below for some specific.

 > > '''Others'''
 > >
 > > - stats and xfrout don't seem to be tested.
 >
 > i knew i missed some :p
 >
 > Or actually just one; in the first commit after the review I've added
 the b10-stats test.
 >
 > Xfrout went into a separate commit; [...] I'm not entirely sure whether
 this is correct, so i've put it in a separate commit for now.

 I have a suggestion, see below.

 > > Okay, I actually suspected it was not hanging, just a long timeout.
 > > What about the traceback?
 >
 > I could not reproduce it at this time, and if it is an existing bug I
 consider it a separate problem.

 Okay.

 > > Yeah, I made a ticket before, but it didn't seem to be very easy.
 > >
 >
 > ok, added a catchall there, but we should probably 'try again' :)

 I'd basically leave it to you, but see below.

 '''ccsession.py'''

 - I realized I forgot to mention one thing I had noticed before
   (sigh): is this comment still valid?
 {{{
         # this step should be unnecessary but is the current way cmdctl
 returns stuff
         # so changes are needed there to make this clean (we need a
 command to simply get the
         # full specs for everything, including commands etc, not separate
 gets for that)
 }}}
   at least there are no "separate gets" here now.

 - what's the purpose of the change in commit?
 {{{
 -                self.request_current_config()
 +                self.update_specs_and_config()
 }}}
   Was this a necessary change but forgotten, or is it an effect of
   more recent changes in the branch?  Also, is this necessary?
   If update_spec_and_config() is performed for even every input, this
   seems to be a redundant update.  Maybe it's related to the higher
   level design issue on the relationship between ccsession and
   bindctl.

 '''cmdctl.py'''

 - maybe we should revise the comment "and ignore otherwise" too?

 '''xfrout'''

 - the change to xfrout.py doesn't seem to be correct.  Even if it
   happens to be safe it looks quite fragile (and at the very least
   it's not clear why the thread related code is in that 'if').  IMO
   it's basically another instance that the xfrout design is so
   spaghetti and hard to extend (it also too much relies on threads,
   making the code rather non-understandable)...but even if so that
   will be another topic.  For now I'd work around it in some other
   way, like this:
 {{{#!python
         if self._unix_socket_server:
             self._unix_socket_server.shutdown()
         self._wait_for_threads()

     def _wait_for_threads(self):
         # Wait for all threads to terminate.  This is a dedicated
 subroutine
         # of shutdown(), but is extracted into the separate thread so we
         # can test shutdown without involving thread operation (the test
 would
         # override this method)
         main_thread = threading.currentThread()
         for th in threading.enumerate():
             if th is main_thread:
                 continue
             th.join()
 }}}
   and do this in the test:
 {{{#!python
 class MyXfroutServer(XfroutServer):
     def __init__(self):
         ....
         self._wait_for_threads = lambda : None
 }}}

 '''ccession.cc'''

 - Hmm, since this problem is more generic, I'd keep the code simpler
   for now, rather than introducing the additional try-catch
   encapsulation.

 '''cfgmgr_test.py'''

 - The refactoring generally looks good and correct, but too
   substantial to be sure if they preserve the previous semantics.  In
   particular, I was not sure if test_handle_msg_update_config() was
   okay.
 - test_stopping_message: what's different between the last two chunks
   of tests? ('but if...' and 'If the module...')  Does it test

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