BIND 10 #213: Change hard-coded process startups to configuration-driven

[BIND 10 Development]

#213: Change hard-coded process startups to configuration-driven
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:  major  |  Sprint-20111025
                  Component:  Boss   |            Resolution:
  of BIND                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  9
Feature Depending on Ticket:         |           Total Hours:
        Add Hours to Ticket:         |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''General'''

 There were too many changes and too many removed tests, so it was
 difficult to be confident about compatibility, stability, regression
 freeness, etc...(some were already noted in the tickets, and I'm okay
 with deferring compatibility support for some of the existing ones,
 but I'm still now sure if the rest of the changes are okay in terms of
 this sense).

 Maybe it was deemed to be too difficult or simply impossible, but due
 to the above concerns I'd still like to try step-by-step transition.
 A possibility would be:
 1. first merge the component module (this should be safe,
 2. choose a small part of the existing functionality, and replace only
   that part with the new module.  do it carefully by preserving the
   semantics of the original test as much as possible.  Maybe we should
   initially simply add code to call the corresponding component module
   but do nothing, then confirm the behavior with tests, and then
   remove the old code while letting the component module do the real
   job.
 3. repeat the 2nd process until we remove all old code.

 I'll provide feedback on some details of the rest of diff anyway.

 And one more thing: Do we need a changelog entry for this task?  Or is
 the plan to provide it when we complete the entire migration?

 '''bind10_config.py.in'''

 I don't understand the trick here:
 {{{
     LIBEXECDIR = ("@libexecdir@/@PACKAGE@"). \
         replace("${exec_prefix}", "@exec_prefix@"). \
         replace("${prefix}", "@prefix@")
 }}}
 The relationship between @libexecdir@, ${exec_prefix}, @exec_prefix@,
 ${prefix}, and @prefix@ is not clear.  Please add comments about the
 intent.

 '''bind10_src.py.in'''

 - Some of the removed log IDs seem to still remain in the message
   file.  e.g.
 {{{
 -        logger.info(BIND10_CONFIGURATION_START_AUTH, self.cfg_start_auth)
 -        logger.info(BIND10_CONFIGURATION_START_RESOLVER,
 self.cfg_start_resolver)
 }}}
   (There seem to be more) These orphan messages should be identified
   comprehensively and cleaned up.

 - `BoB.__init__()`: ideally I'd like to move the setting of
   `__core_components` outside of the bind10 module (and even more
   ideally make it "configurable" by a command line option, etc).
 - Also about `__core_components`: the intent of the priorities should
   be documented (commented).

 - config_handler(): I'm often confused about this point, but doesn't
   this callback only get the difference from the current
   configuration?  If that's the case, I suspect it doesn't meet the
   assumption of the component module, that is, it could accidentally
   kill a running component.

 - config_handler(): shouldn't we catch any exception while handling
   the new config and make sure return some "answer"?

 - config_handler(): while this point may become moot due to the
   previous point (and it's not really about this branch), but I don't
   see the need for having a temporary variable 'answer' here:
 {{{
         answer = isc.config.ccsession.create_answer(0)
         return answer
 }}}

 - kill_started_processes(): from a quick look at the library
   reference, os.kill(signal.SIGTERM) is less portable than
   Poepen.kill(), which was used for the original code (especially if
   we consider Windows support), and in that sense this is a
   regression.  The same comment applies to other cases where os.kill()
   is used.

 - read_bind10_config(): the method's doc string may need to be
   updated, depending on whether this work also means we are going to
   decide how/where a list of running components should be defined.

 - start_msgq(): It's not about this branch, but variable 'c_channel'
   is confusing to me because it doesn't sound like a process.  I'd
   rather say 'msgq', 'msgq_proc', etc.

 - start_simple(): this part of doc is not valid any more:
 {{{
             The port and address arguments are for log messages only.
 }}}

 - start_auth(), etc: While the comment above start_auth() seems to
   suggest the use of them in tests, they now are not used in
   tests (overkilling?).  Also, as already stated, ideally I'd move
   these outside the bind10 module.

 - start_all_processes(): I'd add comment that other processes will be
   started in read_bind10_config():
 {{{
         # Extract the parameters associated with Bob.  This can only be
         # done after the CC Session is started.
 }}}

 - start_all_processes(): is it okay to remove this?
 {{{
 -        # Everything after the main components can run as non-root.
 -        # TODO: this is only temporary - once the privileged socket
 creator is
 -        # fully working, nothing else will run as root.
 -        if self.uid is not None:
 -            posix.setuid(self.uid)
 }}}

 - stop_all_processes(): this is now one-line method so may be removed
   (the caller could directly do what this method does)

 - stop_process(): I was not sure how we could remove this comment due
   to this branch:
 {{{
 -        # TODO: Some timeout to solve processes that don't want to die
 would
 -        # help. We can even store it in the dict, it is used only as a
 set
 }}}
   and this line (or the concept of "expected_shutdowns"):
 {{{
 -        self.expected_shutdowns[process] = 1
 }}}
   (I'm not necessarily saying we can't.  I simply didn't understand).

 - component_shutdown(): shouldn't this check be at the beginning of the
   method?  At least before setting exitcode?
 {{{
         if not self.__started:
             raise Exception("Component failed during startup");
 }}}

 - component_shutdown(): if I read the code correctly, `__stopping` can
   never be True in the context where this method can be called:
 {{{
         if self.__stopping:
             return
 }}}
   If the intent is to prevent a duplicate call, we'll probably need a
   dedicated attribute like "shutting_down", and set it to True in this
   own method.

 - reap_children(): I was not sure if it's safe to remove this:
 {{{
 -                self.dead_processes[proc_info.pid] = proc_info
 }}}

 - reap_children(): maybe we should pass the exit_status to
   component.failed() so that we can log the event with that
   information?  (btw, otherwise we won't need a specific variable).

 - restart_processes(): I was not sure if we can safely remove these:
 {{{
 -            if proc_info.name in self.expected_shutdowns:
 -                # We don't restart, we wanted it to die
 -                del self.expected_shutdowns[proc_info.name]
 -                continue
 }}}
   see also comments about stop_process().

 - register_process() doesn't have doc string.

 '''bob.spec'''

 - not actually about the spec itself, but when we specify the default
   for some configuration parameters, the application that uses them
   should get the default from the spec, rather than hardcoding them in
   the source code.  One example:
 {{{
                 component = creator(params.get('process', cname),
 self.__boss,
                                     params.get('kind', 'dispensable'),
                                     params.get('address'),
                                     params.get('params'))
 }}}

 - I'm not sure "b10-xxx" is the best choice for the component name.
   It seems more like a kind of component, rather than the program
   (file) name, so things like auth (or Auth) sound a bit better to
   me.  But this is probably a minor preference matter.

 - likewise, while "params" are defined as a list of strings,
   component_test uses integers:
 {{{
         plan = configurator._build_plan({}, {
             'component': {
                 'kind': 'needed',
                 'params': [1, 2],
                 'address': 'address'
             }
         })
 }}}
 - I can't find "priority" in the spec.
 - dhcp related config is missing, too.

 '''bind10_test.py.in'''

 General comment: TestStartStopProcessesBob was completely removed and
 TestBossComponents was completely newly introduced.  Both are pretty
 big, and the changes are not trivial, so I was not sure such changes
 in tests ensure safe refactoring.  (For example, the original tests
 check whether auth/resolver are started based on some
 run-time/config-time conditions.  Such kind of tests seem to be
 missing).  Overall, I've not seen anything obviously wrong in the
 added tests, but I couldn't be confident that these tests are
 sufficient, and I was not really comfortable about that.

 - TestBossComponents.test_correct_run: about this point, my
   understanding is that we are expected to return an error answer in such
   cases:
 {{{
         # Is it OK to raise, or should it catch also and convert to error
         # answer?
         self.assertRaises(Exception, bob.config_handler,
                           {'components': compconf})
 }}}
  See also the comment about config_handler() above.

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