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

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:15 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)
 >

 ack

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

 oh, indeed, somehow i skipped over that too (i though i had removed that
 already)

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

 You are right, it shouldn't be necessary in the first place. Removed it.

 > '''cmdctl.py'''
 >
 > - maybe we should revise the comment "and ignore otherwise" too?
 >

 Ack, removed.

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

 Oh hey, cute :) much better, thanks for the suggestion, I put it in

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

 so, er, revert to the original? done.

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

 I ran it through the coverage tool, and all code still seems to be covered
 at least. Made one more modification, to hit another path.

 > - test_stopping_message: what's different between the last two chunks
 >   of tests? ('but if...' and 'If the module...')

 none, somehow i added the same test twice, removed the second.

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