cmd/jujud: machine agent API connect

cmd/jujud: machine agent API connect

This is a fairly substantial refactor of the juju agent
stuff to use worker.Runner.

This also reinstates the password changing, but
only for the machine agent (in fact the unit
agent password changing is not important,
as that password does not go into the cloudinit
data).

https://code.launchpad.net/~rogpeppe/juju-core/305-alt-jujud-use-tasks-package/+merge/170856

Requires: https://code.launchpad.net/~rogpeppe/juju-core/311-juju-bootstrap-state-change-password-1.6/+merge/170854

(do not edit description out of merge proposal)

[rog, 2013-06-21 17:10:44]

Please take a look.

[fwereade, 2013-06-22 14:06:11]

There's a lot to love in this branch, but I have a couple of significant issues.
One is about the testing of the agents' tasks (it was a sleeping dog until we
started remodelling its kennel...) and the other is undoubtedly a shared
communication issue (that also tangles with the testing question).

In short, I just proposed https://codereview.appspot.com/10439046/, without
checking with you first, on the basis that the approach we agreed was to build
up the API infrastructure in parallel; and not to touch the state
infrastructure, except by moving responsibilities over to the API side
one-by-one, until we'd got to the point where we could drastically simplify the
state stuff.

I am ofc very happy that we can now run tasks against the API, because that's
critically important, but it's come with a lot of other stuff that doesn't seem
very relevant to that proximate goal. It's not useless work -- it's necessary
work -- but it's also work that doesn't help us today, and doing things in this
order has pushed back the time at which we could start to integrate API tasks. 

Happily, ofc, this is *almost* entirely harmless, because the API hasn't has the
resourcing love it's deserved over the last few weeks; so nobody's actually been
waiting on your branch, and the only casualties are at the junction of your
branch and mine.

But... I think they're fairly significant casualties, and we need to figure out
where we go from here. Discuss live? I'm pretty certain there'll be a happy
solution somewhere, but at the moment I have a trilemma of <land mine, fix bug
for release, delay API integration>; <land yours, unblock integration, don't
release bugfix>; and <land neither, mess with both goals> which is somewhat
likely regardless.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go
File cmd/jujud/agent.go (left):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#oldcode190
cmd/jujud/agent.go:190: func RunAgentLoop(c *agent.Conf, a Agent) error {
Grar, I just wrote tests for this, and it was really tedious ;p.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode142
cmd/jujud/agent.go:142: func openAPIState(c *agent.Conf, a Agent) (*api.State,
AgentAPIState, error) {
I still feel there's something a little bit back-to-front about the newPassword
stuff, but that's probably just my brain.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode194
cmd/jujud/agent.go:194: // Return and let upstart deal with the restart.
if err := ug.ChangeAgentTools(); err != nil {
    return err
}

?

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode205
cmd/jujud/agent.go:205: // newCloseTask returns a task that wraps the given
task,
newCloseWorker

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode206
cmd/jujud/agent.go:206: // closing the given closer when it finishes.
...so long as someone calls Wait, anyway -- Kill won't do it. I don't think that
the fact it's a Worker quite excuses this... can we maybe just beef it up a
little so it uses its own tomb and goroutine, and closes the closer before
calling Done?

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode81
cmd/jujud/machine.go:81: if a.MachineId == "0" {
state.BootstrapMachineId or whatever it is?

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode82
cmd/jujud/machine.go:82: a.runner.StartWorker("state", func() (worker.Worker,
error) {
Can we maybe use the tasks' .String()s as their ids? Or is that a bad idea?

(it can't be done today... but see below)

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:142: return NewUpgrader(st, m, a.Conf.DataDir), nil
TODO sometime -- pass m by id?

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:157: runner.StartWorker("firewaller", func()
(worker.Worker, error) {
It's hard to test these except by side effect, and I don't think that's quite
good enough. See https://codereview.appspot.com/10439046/ for my take on a fix
for the situation before this CL here; somewhere else in here, I suggested using
the worker.String() method to identify the workers to the Runner; and I now see
why that won't work as stated, but I think the side-effect tests we're currently
depending on are basically unacceptable.

This attitude is particularly informed by working on that CL, which integrates
Cleaner and Resumer; in particular, testing Resumer by side effect is plainly
insane, but it's equally not ok to skip the testing entirely. I'm not even very
happy with the run-and-stop-and-hope-the-side-effects-don't-screw-something-up
approach taken there, so it'd maybe be nicest of all if we could just ask an
Agent for a list of Workers -- that hadn't yet been started -- and thus be able
to check both the returned tasks, sans uncontrolled side effects, on individual
agents; *and* verify the worker setup against a mocked-out agent that returns a
set of small, simple, safe tasks.

[rog, 2013-06-23 13:07:06]

On 22 June 2013 15:06,  <fwereade@gmail.com> wrote:

Thanks a lot for the review.

> There's a lot to love in this branch, but I have a couple of significant
> issues. One is about the testing of the agents' tasks (it was a sleeping
> dog until we started remodelling its kennel...) and the other is
> undoubtedly a shared communication issue (that also tangles with the
> testing question).
>
> In short, I just proposed https://codereview.appspot.com/10439046/,
> without checking with you first, on the basis that the approach we
> agreed was to build up the API infrastructure in parallel; and not to
> touch the state infrastructure, except by moving responsibilities over
> to the API side one-by-one, until we'd got to the point where we could
> drastically simplify the state stuff.

I'm sorry - since you commented favourably on an earlier version
of this proposal (https://codereview.appspot.com/9735045) about two weeks ago,
I assumed that this was all OK.

I have to say I was surprised when I saw your proposal.

> But... I think they're fairly significant casualties, and we need to
> figure out where we go from here. Discuss live? I'm pretty certain
> there'll be a happy solution somewhere, but at the moment I have a
> trilemma of <land mine, fix bug for release, delay API integration>;
> <land yours, unblock integration, don't release bugfix>; and <land
> neither, mess with both goals> which is somewhat likely regardless.

I'd opt for <land mine; add more tests>.

My deliberate approach when making this proposal has been
to avoid it  changing affect externally visible behaviour, so all the existing
tests can remain, so it's obvious that the new code isn't breaking
any of the behaviour from before (well, to the extent of our test
coverage, of course!).

This, of course, is one of the significant advantages of having
tests that rely on externally visible behaviour only - we can
refactor without needing to rewrite hundreds of lines of test code.

I appreciate that you don't think that gives us good enough
test coverage (you may well be right), and I'm happy to add
tests that are more whiteboxy, but I think the existing integration
tests are important - they're the only non-live tests that check,
end to end, that jujud does what it should be doing.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode142
cmd/jujud/agent.go:142: func openAPIState(c *agent.Conf, a Agent) (*api.State,
AgentAPIState, error) {
On 2013/06/22 14:06:11, fwereade wrote:
> I still feel there's something a little bit back-to-front about the
newPassword
> stuff, but that's probably just my brain.

the other way to phrase it would be as a function
that takes an interface or function which it uses to
get the entity and set the password.

neither is ideal - perhaps a better solution is just
to move OpenAPI out of environs/agent into this code.
then things might be more obvious.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode194
cmd/jujud/agent.go:194: // Return and let upstart deal with the restart.
On 2013/06/22 14:06:11, fwereade wrote:
> if err := ug.ChangeAgentTools(); err != nil {
>     return err
> }
> 
> ?

Done.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode206
cmd/jujud/agent.go:206: // closing the given closer when it finishes.
On 2013/06/22 14:06:11, fwereade wrote:
> ...so long as someone calls Wait, anyway -- Kill won't do it. I don't think
that
> the fact it's a Worker quite excuses this...

Given that we always Stop workers (otherwise we're doing it wrong
and leaking the whole worker), we are always guaranteed to call Wait.

I *think* this is ok.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode81
cmd/jujud/machine.go:81: if a.MachineId == "0" {
On 2013/06/22 14:06:11, fwereade wrote:
> state.BootstrapMachineId or whatever it is?

there doesn't seem to be such a thing.
do you think it's worth adding? i think that
the constant "0" is probably fine - lots of test code
assumes that the first machine to be created has
that id, and zero the number is good and special.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode82
cmd/jujud/machine.go:82: a.runner.StartWorker("state", func() (worker.Worker,
error) {
On 2013/06/22 14:06:11, fwereade wrote:
> Can we maybe use the tasks' .String()s as their ids? Or is that a bad idea?
> 
> (it can't be done today... but see below)

tbh the only reason the a task has a String now is because there was no other
way of producing a decent error message with the old tasks runner. I have every
intention of removing the String method at some point in the near future.

using a string key for workers was a deliberate decision, and seems to work fine
to me. it means that the runner's client is entirely in control of the keys -
the various workers don't need to be written with a particular id name space in
mind.

do you have a particular problem with the string keys?

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:157: runner.StartWorker("firewaller", func()
(worker.Worker, error) {
On 2013/06/22 14:06:11, fwereade wrote:
> It's hard to test these except by side effect, and I don't think that's quite
> good enough. See https://codereview.appspot.com/10439046/ for my take on a fix
> for the situation before this CL here; somewhere else in here, I suggested
using
> the worker.String() method to identify the workers to the Runner; and I now
see
> why that won't work as stated, but I think the side-effect tests we're
currently
> depending on are basically unacceptable.
> 
> This attitude is particularly informed by working on that CL, which integrates
> Cleaner and Resumer; in particular, testing Resumer by side effect is plainly
> insane, but it's equally not ok to skip the testing entirely. I'm not even
very
> happy with the run-and-stop-and-hope-the-side-effects-don't-screw-something-up
> approach taken there, so it'd maybe be nicest of all if we could just ask an
> Agent for a list of Workers -- that hadn't yet been started -- and thus be
able
> to check both the returned tasks, sans uncontrolled side effects, on
individual
> agents; *and* verify the worker setup against a mocked-out agent that returns
a
> set of small, simple, safe tasks.

I'm not sure what side-effects you're concerned about here.

Aren't those "side effects" actually the main operation
of jujud, and isn't it actually quite good to check that
it all works together? Do we plan on adding any workers
that have uncontrolled side effects?

If you want a more direct test, it should be straightforward enough
to outfit worker.Runner with a way of inspecting its
currently running tasks. Then we'd just poll the agent's
runner until the running tasks matched the expected tasks.

[fwereade, 2013-06-26 08:39:15]

As discussed, LGTM with (1) an immediate followup to add Cleaner and Resumer,
even if we can't test the Cleaner very well (add/destroy a service, wait a sync
interval plus a bit, wait for the unit to be destroyed) or the Resumer at all;
and (2) a subsequent followup that actually makes this stuff quickly and cleanly
testable without demanding duplicated test scaffolding from the other various
worker tests.

Thanks again -- I still think this is a great change, and even its negative
aspects don't make us any worse off than we were before. It's just that I can't
really countenance sticking with the old testing model long-term.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode142
cmd/jujud/agent.go:142: func openAPIState(c *agent.Conf, a Agent) (*api.State,
AgentAPIState, error) {
On 2013/06/23 13:07:06, rog wrote:
> On 2013/06/22 14:06:11, fwereade wrote:
> > I still feel there's something a little bit back-to-front about the
> newPassword
> > stuff, but that's probably just my brain.
> 
> the other way to phrase it would be as a function
> that takes an interface or function which it uses to
> get the entity and set the password.
> 
> neither is ideal - perhaps a better solution is just
> to move OpenAPI out of environs/agent into this code.
> then things might be more obvious.

Maybe better still to combine the bulk of this package and that into a top-level
"agent" package. But, yeah, I'm fine with this at the moment, it just itches a
little.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/agent.go#newcode206
cmd/jujud/agent.go:206: // closing the given closer when it finishes.
On 2013/06/23 13:07:06, rog wrote:
> On 2013/06/22 14:06:11, fwereade wrote:
> > ...so long as someone calls Wait, anyway -- Kill won't do it. I don't think
> that
> > the fact it's a Worker quite excuses this...
> 
> Given that we always Stop workers (otherwise we're doing it wrong
> and leaking the whole worker), we are always guaranteed to call Wait.
> 
> I *think* this is ok.
> 

Ah, you're right, sorry.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode81
cmd/jujud/machine.go:81: if a.MachineId == "0" {
On 2013/06/23 13:07:06, rog wrote:
> On 2013/06/22 14:06:11, fwereade wrote:
> > state.BootstrapMachineId or whatever it is?
> 
> there doesn't seem to be such a thing.
> do you think it's worth adding? i think that
> the constant "0" is probably fine - lots of test code
> assumes that the first machine to be created has
> that id, and zero the number is good and special.

Huh, I could have sworn there was. If there isn't, there should be (for crying
out loud, we have BootstrapNonce but no BootstrapMachineId? How?).

(Tests might assume that machine numbering starts at 0; that's not great but not
terrible, I think. But it's really not ok to assume that machine 0 has any
particular properties; not in tests and definitely not in code. I don't think
either niemeyer or I ever failed to bang that drum given the opportunity to do
so ;p. If machine 0 has special properties -- which, ok, it does -- then the
specialness really ought to be somehow indicated in the name. Hence, please use
a constant not a magic number.)

There aren't that many places where 0 is special though. Provider bootstrap has
to use it, yes; and probably bootstrap-state; and I accept its necessity here;
anywhere else?

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:157: runner.StartWorker("firewaller", func()
(worker.Worker, error) {
On 2013/06/23 13:07:06, rog wrote:
> On 2013/06/22 14:06:11, fwereade wrote:
> > It's hard to test these except by side effect, and I don't think that's
quite
> > good enough. See https://codereview.appspot.com/10439046/ for my take on a
fix
> > for the situation before this CL here; somewhere else in here, I suggested
> using
> > the worker.String() method to identify the workers to the Runner; and I now
> see
> > why that won't work as stated, but I think the side-effect tests we're
> currently
> > depending on are basically unacceptable.
> > 
> > This attitude is particularly informed by working on that CL, which
integrates
> > Cleaner and Resumer; in particular, testing Resumer by side effect is
plainly
> > insane, but it's equally not ok to skip the testing entirely. I'm not even
> very
> > happy with the
run-and-stop-and-hope-the-side-effects-don't-screw-something-up
> > approach taken there, so it'd maybe be nicest of all if we could just ask an
> > Agent for a list of Workers -- that hadn't yet been started -- and thus be
> able
> > to check both the returned tasks, sans uncontrolled side effects, on
> individual
> > agents; *and* verify the worker setup against a mocked-out agent that
returns
> a
> > set of small, simple, safe tasks.
> 
> I'm not sure what side-effects you're concerned about here.
> 
> Aren't those "side effects" actually the main operation
> of jujud, and isn't it actually quite good to check that
> it all works together? Do we plan on adding any workers
> that have uncontrolled side effects?
> 
> If you want a more direct test, it should be straightforward enough
> to outfit worker.Runner with a way of inspecting its
> currently running tasks. Then we'd just poll the agent's
> runner until the running tasks matched the expected tasks.
> 

We've already got scary side effects that we have to patch out -- if you want a
real test of the machiner you need root (think of the deployer). And then
consider *changes* to existing workers that add bad side effects -- say the
machiner starts writing out authorized-keys. It'll be tedious enough writing the
machiner tests properly, but then you have to remember to fix the jujud tests as
well, and nobody's going to do that before they've blown away their dev
machine's authorized_keys... and they might not even know what broke it.

I really think these tests are a bad idea, and that we'd have a much safer and
happier time if we drew an accessible abstraction boundary between the thing
that defines the tasks and the thing that starts them.

[jameinel, 2013-06-26 09:33:48]

We chatted about this quite a bit on IRC. LGTM with some of the tweaks we talked
about.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:106: func (a *MachineAgent) APIWorker() (worker.Worker,
error) {
Having a method called APIWorker which might also have a side effect of starting
the StateWorker seems a bit off.

Better to have a method called StartWorkers, which then calls out to APIWorker
and StateWorker. You mentioned on IRC that API has to be opened before state,
but we can ensure that happens in StartWorkers.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/unit.go#newcode67
cmd/jujud/unit.go:67: // TODO(rog) use method expression when we can use go1.1
features.
We probably want to have a common syntax for this sort of thing. 'go1.1' might
be sufficient, though maybe:

// TODO go1.1 replace with a method expression instead of closure

[rog, 2013-06-26 12:13:49]

Please take a look.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode81
cmd/jujud/machine.go:81: if a.MachineId == "0" {
On 2013/06/26 08:39:15, fwereade wrote:
> On 2013/06/23 13:07:06, rog wrote:
> > On 2013/06/22 14:06:11, fwereade wrote:
> > > state.BootstrapMachineId or whatever it is?
> > 
> > there doesn't seem to be such a thing.
> > do you think it's worth adding? i think that
> > the constant "0" is probably fine - lots of test code
> > assumes that the first machine to be created has
> > that id, and zero the number is good and special.
> 
> Huh, I could have sworn there was. If there isn't, there should be (for crying
> out loud, we have BootstrapNonce but no BootstrapMachineId? How?).
> 
> (Tests might assume that machine numbering starts at 0; that's not great but
not
> terrible, I think. But it's really not ok to assume that machine 0 has any
> particular properties; not in tests and definitely not in code. I don't think
> either niemeyer or I ever failed to bang that drum given the opportunity to do
> so ;p. If machine 0 has special properties -- which, ok, it does -- then the
> specialness really ought to be somehow indicated in the name. Hence, please
use
> a constant not a magic number.)
> 
> There aren't that many places where 0 is special though. Provider bootstrap
has
> to use it, yes; and probably bootstrap-state; and I accept its necessity here;
> anywhere else?

I don't think so. If anything, perhaps it could be a constant inside cmd/jujud,
which is the place that starts and knows about the bootstrap machine.

It occurs to me that this logic will also be wrong in the HA future. When other
API servers have started, we won't actually want to follow the bootstrap-machine
path here - we should connect to those other API servers and then decide what to
do (we might well want to allow the bootstrap machine to be repurposed as a
non-manager machine).

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:106: func (a *MachineAgent) APIWorker() (worker.Worker,
error) {
On 2013/06/26 09:33:48, jameinel wrote:
> Having a method called APIWorker which might also have a side effect of
starting
> the StateWorker seems a bit off.
> 
> Better to have a method called StartWorkers, which then calls out to APIWorker
> and StateWorker. You mentioned on IRC that API has to be opened before state,
> but we can ensure that happens in StartWorkers.

As discussed, I'm unable to think of a decent way of doing this in the way
you're suggesting. StartWorkers would need to connect to the API itself,
something that APIWorker also needs to do, so if we want to avoid making two
connections in quick succession, there needs to be some dubious
interconnectedness between StartWorkers and APIWorker, which I think ends up
worse than what we have here (here's how it might look, for the record:
http://paste.ubuntu.com/5801256/)

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:142: return NewUpgrader(st, m, a.Conf.DataDir), nil
On 2013/06/22 14:06:11, fwereade wrote:
> TODO sometime -- pass m by id?

Done.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/unit.go#newcode67
cmd/jujud/unit.go:67: // TODO(rog) use method expression when we can use go1.1
features.
On 2013/06/26 09:33:48, jameinel wrote:
> We probably want to have a common syntax for this sort of thing. 'go1.1' might
> be sufficient, though maybe:
> 
> // TODO go1.1 replace with a method expression instead of closure

Done.

[jameinel, 2013-06-26 12:15:19]

a couple of stale comments, but LGTM

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:84: // that need a state connection Unless we're
bootstrapping, we
... state connection. Unless we are bootstrapping, we need to

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:87: // the machine requires it.  Note that startStateWorker
can be
s/startStateWorker/ensureStateWorker/

We probably don't need the 'Note' given that an 'ensure' function is almost
always something you can call whenever you want.

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:129: // If a state worker is necessary, APIWorker calls
startStateWorker.
ensureStateWorker

[fwereade, 2013-06-26 12:24:40]

LGTM with one thought,probably fine to ignore it.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode81
cmd/jujud/machine.go:81: if a.MachineId == "0" {
On 2013/06/26 12:13:49, rog wrote:
> On 2013/06/26 08:39:15, fwereade wrote:
> > On 2013/06/23 13:07:06, rog wrote:
> > > On 2013/06/22 14:06:11, fwereade wrote:
> > > > state.BootstrapMachineId or whatever it is?
> > > 
> > > there doesn't seem to be such a thing.
> > > do you think it's worth adding? i think that
> > > the constant "0" is probably fine - lots of test code
> > > assumes that the first machine to be created has
> > > that id, and zero the number is good and special.
> > 
> > Huh, I could have sworn there was. If there isn't, there should be (for
crying
> > out loud, we have BootstrapNonce but no BootstrapMachineId? How?).
> > 
> > (Tests might assume that machine numbering starts at 0; that's not great but
> not
> > terrible, I think. But it's really not ok to assume that machine 0 has any
> > particular properties; not in tests and definitely not in code. I don't
think
> > either niemeyer or I ever failed to bang that drum given the opportunity to
do
> > so ;p. If machine 0 has special properties -- which, ok, it does -- then the
> > specialness really ought to be somehow indicated in the name. Hence, please
> use
> > a constant not a magic number.)
> > 
> > There aren't that many places where 0 is special though. Provider bootstrap
> has
> > to use it, yes; and probably bootstrap-state; and I accept its necessity
here;
> > anywhere else?
> 
> I don't think so. If anything, perhaps it could be a constant inside
cmd/jujud,
> which is the place that starts and knows about the bootstrap machine.
> 
> It occurs to me that this logic will also be wrong in the HA future. When
other
> API servers have started, we won't actually want to follow the
bootstrap-machine
> path here - we should connect to those other API servers and then decide what
to
> do (we might well want to allow the bootstrap machine to be repurposed as a
> non-manager machine).
> 

That feels like a vote for "machine agents should know how to bootstrap
themselves if some more-independent condition holds" :).

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:105: ensureStateWorker()
What if, just for this CL, we were just to always start a state worker? Only a
thought, but it might simplify things a bit...

[rog, 2013-06-26 13:27:16]

Please take a look.

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/5001/cmd/jujud/machine.go#newcode81
cmd/jujud/machine.go:81: if a.MachineId == "0" {
On 2013/06/26 12:24:40, fwereade wrote:
> On 2013/06/26 12:13:49, rog wrote:
> > On 2013/06/26 08:39:15, fwereade wrote:
> > > On 2013/06/23 13:07:06, rog wrote:
> > > > On 2013/06/22 14:06:11, fwereade wrote:
> > > > > state.BootstrapMachineId or whatever it is?
> > > > 
> > > > there doesn't seem to be such a thing.
> > > > do you think it's worth adding? i think that
> > > > the constant "0" is probably fine - lots of test code
> > > > assumes that the first machine to be created has
> > > > that id, and zero the number is good and special.
> > > 
> > > Huh, I could have sworn there was. If there isn't, there should be (for
> crying
> > > out loud, we have BootstrapNonce but no BootstrapMachineId? How?).
> > > 
> > > (Tests might assume that machine numbering starts at 0; that's not great
but
> > not
> > > terrible, I think. But it's really not ok to assume that machine 0 has any
> > > particular properties; not in tests and definitely not in code. I don't
> think
> > > either niemeyer or I ever failed to bang that drum given the opportunity
to
> do
> > > so ;p. If machine 0 has special properties -- which, ok, it does -- then
the
> > > specialness really ought to be somehow indicated in the name. Hence,
please
> > use
> > > a constant not a magic number.)
> > > 
> > > There aren't that many places where 0 is special though. Provider
bootstrap
> > has
> > > to use it, yes; and probably bootstrap-state; and I accept its necessity
> here;
> > > anywhere else?
> > 
> > I don't think so. If anything, perhaps it could be a constant inside
> cmd/jujud,
> > which is the place that starts and knows about the bootstrap machine.
> > 
> > It occurs to me that this logic will also be wrong in the HA future. When
> other
> > API servers have started, we won't actually want to follow the
> bootstrap-machine
> > path here - we should connect to those other API servers and then decide
what
> to
> > do (we might well want to allow the bootstrap machine to be repurposed as a
> > non-manager machine).
> > 
> 
> That feels like a vote for "machine agents should know how to bootstrap
> themselves if some more-independent condition holds" :).

I'm not sure. Needs some thought.

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10259049/diff/16001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:105: ensureStateWorker()
On 2013/06/26 12:24:40, fwereade wrote:
> What if, just for this CL, we were just to always start a state worker? Only a
> thought, but it might simplify things a bit...

I'll leave things as they are, if that's ok. This skeleton is there and
hopefully appropriate for future purpose, and saves someone from needing to try
to remember how it was supposed to work when we get there.