BIND 10 #1365: Restarts in #213

[BIND 10 Development]

#1365: Restarts in #213
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111108
                  Component:  Boss   |            Resolution:
  of BIND                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 vorner]:

 > > > > - maybe rename BoB.processes to BoB.components?
 > > >
 > > > I added comment about that, I believe processes is better, since
 there isn't one-to-one mapping between components and processes.
 > >
 > > I see, but then doesn't this contradict the idea?
 > > {{{
 > >                 component = self.processes.pop(pid)
 > > }}}
 > > or
 > > {{{
 > >             components_to_stop = list(self.processes.values())
 > >             for component in components_to_stop:
 > > }}}
 > >
 > > Actually, these code fragments made me make this comment.
 >
 > Actually, each item in the dict corresponds to one process. But the
 values are componenents, as each process belongs to one.

 I (now) know that, but that's not my point.  The point is that such
 code seems to indicate confusion on the relationship between processes
 and components.

 For example, consider the case where a component has multiple
 processes.  Then component.kill() will be called multiple times here:
 {{{
         for component in components_to_stop:
             logger.info(BIND10_SEND_SIGTERM, component.name(),
 component.pid())
             try:
                 component.kill()
 }}}

 (It's also awkward the log message talks about a particular process
 PID, while kill() is a component-wide operation...hmm, and I'd wonder
 what component.pid() means if the component has multiple processes).

 Calling kill() on a component may or may not be safe depending on the
 underlying implementation, but it's not clear from the documentation,
 and, frankly, this code doesn't seem to take into account that
 possibility but simply confuse components with processes.

 Likewise, this code doesn't seem to be well defined to me:
 {{{
                 component = self.processes.pop(pid)
                 logger.info(BIND10_PROCESS_ENDED, component.name(), pid,
                             exit_status)
                 if component.running() and self.runnable:
                     component.failed(exit_status);
 }}}

 If the component has multiple processes, what happens here is that one
 of the processes has died.  Such a component would internally maintain
 a list of processes, and to restart a new one in 'failed', it would
 first have to remove the entry from the list for the dead process then
 spawn a new one.  But, again, since failed() is component-wide method,
 the component cannot even know which process has died.

 All these things seem to suggest that the current abstraction is
 incomplete for the multi-process component model.  IMO, we should
 either:
 - make it well defined throughout the code, or
 - drop the incomplete model for the moment and assume one-to-one
   mapping between processes and component, at least in the boss
   implementation. (in this case it will be more intuitive to me if we
   rename BoB.processes to BoB.components).

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