BIND 10 #213: Change hard-coded process startups to configuration-driven
BIND 10 Development
do-not-reply at isc.org
Tue Nov 1 17:08:52 UTC 2011
#213: Change hard-coded process startups to configuration-driven
-------------------------------------+-------------------------------------
Reporter: shane | Owner: jinmei
Type: | Status: reviewing
enhancement | Milestone:
Priority: major | Sprint-20111108
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):
> Replying to part of [comment:30 vorner] before my lunch break...
...continued...
Replying to [comment:30 vorner]:
> > Also, for pid() to always work correctly, I suspect we should reset
> > _procinfo to None once the component stops running.
[...]
> > > I added some documentation there.
> >
> > Maybe my intent wasn't clearly delivered...let me rephrase it:
> > Assume I wanted to completely override `_start_internal()` for some
> > reason. Then I should set `_procinfo` in the override version;
[...]
> > otherwise kill() won't work correctly. And, just like I commented
> > about "You should also register..." part, if this is something all
> > overridden the original implementation should do, I'd move it to
> > start(), [...]
>
> Ah, I added the kill later than the documentation. It should say kill
should be overridden in the case you provide _start_internal. In that
case, everything that uses _procinfo would be overriden.
All these three points now seem to be related this point: it was not
clear what was the intended guideline to customize the start/stop details.
Also, as indicated by the missing documentation update to kill(),
it's not clear which set of methods are supposed to be overridden at
once. Even though documentation helps, it seems to be more error prone.
I'd suggest something like this:
- Introduce a separate class, say ComponentInternal. It has "start",
"stop", "name", and "pid" methods. This class is an abstract base
class; it's not expected to be used directly, and all of these
methods are expected to be overridden in a derived class.
- Define a derived DefaultComponentInternal. Move the current
_start_internal (renamed to start), _stop_internal (renamed to
stop), name and pid, _procinfo to this class. This class takes a
callable object (start_func) on initialization to allow small
customization for startup only.
- Component is constructed with a concrete ComponentInternal, whose
default is DefaultComponentInternal. Component.start() calls
component_internal.start(), etc, instead of Component._start().
_procinfo.
- Say if the user wants complete customization, define a separate
ComponentInternal derived class and use it with Component.
If the user wants a small customization for startup, only pass a
different callable to the Component (or set
DefaultComponentInternal._start_func to the user's callable in some
way)
This way it's clearer what is expected to be overridden as a set.
It's also safer in terms of avoiding to accidentally refer to things
like _procinfo with partially overridden methods.
> > I'd also envision some external developers want to use the BIND 10
> > framework to control a third party server process and want the boss
> > process to invoke the third party server without modifying the BIND 10
> > source code. [...]
>
> Well, in that case we would need a way to push the code into the running
master somehow. I'm not against that, but I had hoped that in the long
term, the only 'special' components would be the real core ones (msgq,
sockcreator, cfgmgr). The rest would have all the parameters & other
needed info in the specification and started the usual way. Then we could
take 'special' out of the spec file as well.
>
> If it turns out we really need a way to push additional code into boss
that does a different startup, I hope we would come up with a way how to
modify the dictionary with special components (eg. each plugin or whatever
it would be would plug itself into some place to be known).
I have no further comment on this. This point will be out of scope of
this ticket anyway.
> > Okay, but do you mean this?
> > {{{
> > Note that this will allow you to stop (by invoking reconfigure) a
core
> > component. There should be some kind of layer protecting users
from ever
> > doing so (users must not stop the config manager, message queue
and stuff
> > like that or the system won't start again).
> > }}}
> > I fail to understand that this means that the caller shouldn't stop a
> > core component. It's probably better to describe that more
> > explicitly.
>
> No, that doesn't mean caller shouldn't stop a core component, that is
completely safe. The caller shouldn't stop a component which can't be
started again. Stoping b10-auth set as core is completely OK. I updated
the doc to be more clear about it.
Okay.
> Replying to [comment:27 jinmei]:
> > > > - kill_started_processes(): from a quick look at the library
> > > > reference, os.kill(signal.SIGTERM) is less portable [...]
> > >
> > > The killing was moved into the components which handle them in
concrete way.
> >
> > As for kill(), I'd use higher level abstraction than SIGKILL instead
> > of SIGTERM. to match the abstraction level of the python interface.
>
> I'm not sure I understand here. Currently the component uses the
terminate() and kill() calls from the procinfo.process(). That is what was
used before.
>
> Do you mean something else?
I meant this comment is UNIX (or POSIX) specific while Python's
subprocess class is more generic:
{{{
If the forcefull is true, it uses SIGKILL instead of SIGTERM.
}}}
I'd say something like this instead:
{{{
If the forcefull is true, it literally "kills" the component
instead of "stopping" it. If the component represents a
process in a POSIX based OS, this means the process is sent
a SIGKILL signal instead of SIGTERM.
}}}
> > > Not really, I missed that one. But it's unhappy in the current
state, it doesn't do what it should, as there's no right place. We should
finish the socket creator soon enough.
> >
> > Hmm...I'm especially not happy about letting xfrin with the root
> > privilege as a result of this. [...]
>
> This one is not in there right now, but I might have a proposal for a
workaround. We may define a special drop-root-privileges component to
place the dropping in the middle of the startup by the priorities. Does it
sound reasonable?
Probably, as a workaround (by definition as long as it works and is
relatively short term, any solution should be reasonable). Note also
that the experimental 213-incremental branch has another workaround
(not specifically intending to address this, but as a result of the
attempt of keeping the original behavior as much as possible).
> > Ah, I thought DHCP daemons were invoked by the boss process, if that's
> > not the case, forget the DHCP comment per se. But if we'd say "if a
> > user wants...", I'd also argue that it's the same for
> > xfrin/xfrout/auth, etc. (or is this perhaps an intermediate state so
> > we preserve the current operational practice and it will eventually
> > move outside the boss spec? if so, that's okay to me)
>
> Actually, there was a code to start it depending the config, but the
config wasn't in the spec file yet.
Ah, cfg_start_dhcp6 (in master) is always False (unless the test code
explicitly modifies it). Confusing:-) While it's not a problem of
this task per se, it would be nicer to add some comment around here:
{{{
if self.cfg_start_dhcp6:
self.start_dhcp6(c_channel_env)
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/213#comment:34>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list