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

 Replying to [comment:9 jelte]:

 > > First off, it was not clear which part of the original report was
 > > solved.  I see the potential that this set of changes can make bindctl
 > > more responsive in the reported scenario, but was it identified why
 > > cfgmgr was hanging (or did it really hang)?  And does this branch
 > > solve that problem?  What about the traceback output?  (BTW I
 > > understand this branch cannot solve the issue of configuring inactive
 > > component).
 > >
 >
 > Oh, sorry for not being clear; that ConfigManager timeout was actually
 closely related; the timeout values for communication from bindctl to
 configmanager and from configmanager to any other (in this case non-
 running) module are the same; so the 'configmanager is not responding' was
 not because configmanager was hanging, but because it was simply waiting
 for a timeout itself. So this would address the root cause, but not that
 specific problem (we probably need to make cmdctl->cfgmgr timeouts a bit
 higher than the rest).

 Okay, I actually suspected it was not hanging, just a long timeout.
 What about the traceback?

 > > '''ccsession.cc'''
 > >
 > > - Not a big deal, but `sendStopping()` is a private method, not very
 > >   big, and used only within the ModuleCCSession destructor.  Is it
 > >   worth a separate method?
 >
 > I personally prefer it in a separate method;

 That's fine.

 > > - logging itself could cause an exception, so the destructor could
 > >   still throw.  However, this is not the only point that has this
 > >   problem, so maybe we can leave it open for now...
 > >
 >
 > hmm, imo we should either make logging calls exception-free or add
 exception-free ones, since it is usually about the only thing you can do
 in a destructor.

 Yeah, I made a ticket before, but it didn't seem to be very easy.

 > > - In our convention we'd explicitly add 'virtual' (for clarity) to the
 > >   destructor declaration.
 >
 > actually, it's even needed for technical reasons for classes that are
 subclassed, but this one isn't supposed to be subclassed afaik I realize
 we can't stop people from doing it, but personally i tend to look at the
 virtualness of the destructor to see if it should be subclassed in the
 first place. Of course, one could argue that it would be better to add it
 when it's not necessary than to not have it when it is, so if you insist
 i'll make it virtual.

 I meant this is because it has a base class and the destructor is
 derived from it, not because this class could be derived from others.
 I thought it was in the coding guideline, but I couldn't find it...

 > > '''cfgmgr.py'''
 > >
 > > - if `_handle_module_stopping` is private, should it better be
 > >   prefixed with `__`?  Same for `_send_module_spec_to_cmdctl`.
 >
 > as you can see from the rest of the code here, i tend to only use the
 one when I don't think the class/method name mangling is necessary. Should
 we start doing this as a principle? (and if so, should we update all
 'private' methods?)

 I think it's a good practice, but not a strong opinion.  In Python
 it's not fully enforcible anyway.

 > > - Not directly related to this ticket, but the if-else in handle_msg
 > >   is getting too big to understand/manage.  I guess it's time to
 > >   consider refactoring, maybe in some OO way.
 >
 > depends on how; as it is now, it is indeed getting long, but is
 essentially a switch. I'm open to proposals, but some 'smart' solutions
 might actually be more complicated :p

 It's probably related to extract common initial patterns of Python
 apps.  In any case it'd be beyond the scope of this ticket.

 > > '''ccsession_test.py'''
 > >
 > > - test_stop: not necessarily a problem, but it seems the same pattern
 > >   repeats in multiple tests.  It would be better to unify the common
 > >   sequence of tests for conciseness.
 >
 > Yes, actually, more generally, this is partly why I was looking at the
 callcounter thingy a few nights ago; [...]

 I'm not sure if we are talking about the same thing...what I meant is
 something rather simple: repeating something like this:
 {{{#!python
         fake_session = FakeModuleCCSession()
         self.assertFalse("Spec1" in fake_session.subscriptions)
         mccs = self.create_session("spec1.spec", None, None, fake_session)
         self.assertTrue("Spec1" in fake_session.subscriptions)
 }}}
 but anyway, I don't request it be solved within this ticket.  The
 branch is already quite large, so it's better to complete it only with
 focused and relevant changes.

 One other comment on the revised diff:

 '''(lettuce)bind10_control.py'''

 - run_bindctl: I'd set the default for cmdctl_port to None to avoid
   duplicate hardcoding of 47805.

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