cmd/jujud: strawman provisioning agent

cmd/jujud: strawman provisioning agent

This is a strawman provisioning agent proposal.

Following Gustavo's suggestion, this revision does not include
any functionality to respond to machines changes.

https://code.launchpad.net/~dave-cheney/juju/go-provisioning-strawman/+merge/107714

(do not edit description out of merge proposal)

[rog, 2012-05-29 08:41:18]

this is looking nice.
a few minor remarks below.

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:17: const PROVIDER_MACHINE_ID = "provider-machine-id"
s/PROVIDER_MACHINE_ID/providerMachineId/

UNDERSCORED_CAPS aren't conventional.

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:120: return fmt.Errorf("machine-%010d already reports
a provider id %q, skipping", m.Id(), id)
s/machine-%010d/machine %d/

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:208: return "", fmt.Errorf("findProviderId:
machine-%010d key not found: %q", m.Id(), PROVIDER_MACHINE_ID)
i think 'machine %d' would work better - the %010 stuff is detail internal to
state.

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:210: if _, ok := id.(string); !ok {
rather than doing the type conversion twice, perhaps:

if id, ok := id.(string); ok {
    return id, nil
}
return "", fmt.Errorf("machine %d has invalid value for %s: %#v", m.Id(),
PROVIDER_MACHINE_ID, id)

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:221: if len(insts) < 1 {
this can't happen.

[dave_cheney.net, 2012-05-30 06:06:10]

Please take a look.

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:17: const PROVIDER_MACHINE_ID = "provider-machine-id"
Beleted

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:120: return fmt.Errorf("machine-%010d already reports
a provider id %q, skipping", m.Id(), id)
On 2012/05/29 08:41:18, rog wrote:
> s/machine-%010d/machine %d/

Done, maybe *state.Machine should grow a .String() method

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:208: return "", fmt.Errorf("findProviderId:
machine-%010d key not found: %q", m.Id(), PROVIDER_MACHINE_ID)
On 2012/05/29 08:41:18, rog wrote:
> i think 'machine %d' would work better - the %010 stuff is detail internal to
> state.

Done.

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:210: if _, ok := id.(string); !ok {
Done. Ta

On 2012/05/29 08:41:18, rog wrote:
> rather than doing the type conversion twice, perhaps:
> 
> if id, ok := id.(string); ok {
>     return id, nil
> }
> return "", fmt.Errorf("machine %d has invalid value for %s: %#v", m.Id(),
> PROVIDER_MACHINE_ID, id)

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:210: if _, ok := id.(string); !ok {
On 2012/05/29 08:41:18, rog wrote:
> rather than doing the type conversion twice, perhaps:
> 
> if id, ok := id.(string); ok {
>     return id, nil
> }
> return "", fmt.Errorf("machine %d has invalid value for %s: %#v", m.Id(),
> PROVIDER_MACHINE_ID, id)

Done.

https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:221: if len(insts) < 1 {
I don't think it hurts to check.

On 2012/05/29 08:41:18, rog wrote:
> this can't happen.

[niemeyer, 2012-05-30 21:09:20]

This is going in a great direction Dave, and I'm excited to see this.

At the same time, I'm concerned about the approach. I'm not comfortable with us
knowingly getting in critical chunks of the functionality while knowing that it
"lacks robust
error checking and retry logic", as I can't help you fixing issues and catching
up bugs that you tell me are known but you don't care about right now.

I'd prefer if this spike, which is a great test bed, was now broken down into
smaller chunks that you are actually *sure* about, so that we and the other
developers can jointly review and agree is a good step forward.

I don't mind if each of those steps are not entirely functional, for example. We
can have a first one that is just about connecting and reconnecting to the
environment reliably, for instance, ignoring any machine management, and so on.

What follows is a few additional ideas on it:

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:17: const PROVIDER_MACHINE_ID = "provider-machine-id"
providerMachineId would be the convention, I think

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:18: log.Debug = true
Shouldn't the suite be using the testing package's log helper instead?

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:126: <-time.After(2 * time.Second)
That's not great. That's the kind of reason that we need a proper Stop method on
the provisioning agent, so that we can be sure that it is happy to terminate,
rather than waiting a random amount of time for it to finish running. Otherwise,
we're getting into exactly the same kind of issue we got into in the Python
implementation.

Please have a look at the underlying infrastructure of watchers for inspiration.
They reliably terminate, both erroring or not. They also do that synchronously
so that it is possible to wait, and to check for errors from them.

We need something like that for the provisioning agent.

[niemeyer, 2012-05-30 21:27:20]

Some more comments.

Also, it just occurred to me that, as a very first step getting just the testing
environment in place, with zero functionality, in a way that reliably starts and
stops, is already a fantastic task on itself.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:53: a.State, err = state.Open(&a.Conf.StateInfo)
This is a rather extensive function that might be broken down further in more
manageable bits for clarity.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:75: // step 2. listen for changes to the environment
or the machine topology and action both.
Step 1 needs to be able to be more resilient, and step 2 must be able to
fallback to step 1 in case of issues with the connection. It must also be able
to deal with Stop of the watcher and re-watching, but that should be somewhat
natural.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:122: return fmt.Errorf("machine-%010d already reports
a provider id %q, skipping", m.Id(), id)
We should never talk about internal ZooKeeper keys to the user. This should be
saying "machine %d" instead.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:134: // store the reference from the provider in ZK
Let's please not reference ZooKeeper all the time. It is the environment state
that we have at hand, with a nice abstraction that we created precisely so we
don't think in terms of ZooKeeper all the time. ZooKeeper is hopefully going
away soon, but the state will remain as-is.

[dave_cheney.net, 2012-05-30 21:52:30]

Hi Gustavo,

Thank you for your feedback.

> At the same time, I'm concerned about the approach. I'm not comfortable
> with us knowingly getting in critical chunks of the functionality while
> knowing that it "lacks robust
> error checking and retry logic", as I can't help you fixing issues and
> catching up bugs that you tell me are known but you don't care about
> right now.

To explain myself (as I wrote that description a few days ago). The PA
does check all errors, but at the moment does not attempt to retry any
operations -- they all bubble up to the Run method and cause the
program to exit.

> I'd prefer if this spike, which is a great test bed, was now broken down
> into smaller chunks that you are actually *sure* about, so that we and
> the other developers can jointly review and agree is a good step
> forward.

So would I, this proposal was an attempt to provide a minimum viable
provisioning agent.

> I don't mind if each of those steps are not entirely functional, for
> example. We can have a first one that is just about connecting and
> reconnecting to the environment reliably, for instance, ignoring any
> machine management, and so on.

Yes, I can certainly do that.

> What follows is a few additional ideas on it:
>
>
> https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go
> File cmd/jujud/provisioning.go (right):
>
>
https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
>
> cmd/jujud/provisioning.go:17: const PROVIDER_MACHINE_ID =
> "provider-machine-id"
> providerMachineId would be the convention, I think

That is a relic, the constant is not used anymore now.

>
https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning_test.go
> File cmd/jujud/provisioning_test.go (right):
>
>
https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning_test....
> cmd/jujud/provisioning_test.go:18: log.Debug = true
> Shouldn't the suite be using the testing package's log helper instead?

I'll figure out how to use that properly, this was actually an attempt
to simulate the --debug flag to the PA, which didn't work.

>
https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning_test....
> cmd/jujud/provisioning_test.go:126: <-time.After(2 * time.Second)
> That's not great. That's the kind of reason that we need a proper Stop
> method on the provisioning agent, so that we can be sure that it is
> happy to terminate, rather than waiting a random amount of time for it
> to finish running. Otherwise, we're getting into exactly the same kind
> of issue we got into in the Python implementation.

Understood. This timer is a currently required because the PA expects
to be running from main, via ProvisioningAgent.Run. I will have a
think about how to add a Tomb that all agents in that package can use
to exit reliably.

> Please have a look at the underlying infrastructure of watchers for
> inspiration. They reliably terminate, both erroring or not. They also do
> that synchronously so that it is possible to wait, and to check for
> errors from them.
>
> We need something like that for the provisioning agent.

[dave_cheney.net, 2012-05-31 07:10:01]

Please take a look.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:17: const PROVIDER_MACHINE_ID = "provider-machine-id"
On 2012/05/30 21:09:21, niemeyer wrote:
> providerMachineId would be the convention, I think

Done.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:75: // step 2. listen for changes to the environment
or the machine topology and action both.
Understood, will redo.

On 2012/05/30 21:27:20, niemeyer wrote:
> Step 1 needs to be able to be more resilient, and step 2 must be able to
> fallback to step 1 in case of issues with the connection. It must also be able
> to deal with Stop of the watcher and re-watching, but that should be somewhat
> natural.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:122: return fmt.Errorf("machine-%010d already reports
a provider id %q, skipping", m.Id(), id)
I think it is time that state.Machine grows a String method, then discussions
like these go away.

On 2012/05/30 21:27:20, niemeyer wrote:
> We should never talk about internal ZooKeeper keys to the user. This should be
> saying "machine %d" instead.

https://codereview.appspot.com/6250068/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:134: // store the reference from the provider in ZK
On 2012/05/30 21:27:20, niemeyer wrote:
> Let's please not reference ZooKeeper all the time. It is the environment state
> that we have at hand, with a nice abstraction that we created precisely so we
> don't think in terms of ZooKeeper all the time. ZooKeeper is hopefully going
> away soon, but the state will remain as-is.

Done.

[dave_cheney.net, 2012-05-31 07:26:21]

Please take a look.

[niemeyer, 2012-05-31 11:46:59]

Thanks Dave. That is now looking like a great step forward that we can polish
for integration easily.

Some comments:

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:37: st, err := state.Open(&a.Conf.StateInfo)
What happens if the connection is broken? We'll need to reestablish it and
restart from the beginning, since all the assumptions are now wrong. Maybe this
has to be moved into the outer loop or something. Will leave details with you at
this point.

That said, I suggest keeping this for the branch coming right after this one. It
feels like what we have here is a solid step forward, and we can comfortably
iterate over the state reestablishment in a new branch before continuing with
everything else.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:56: type environment struct {
Those two types feel unnecessary. It looks like your model would work equally
well if we had environWatcher and machinesWatcher both within the Provisioner
struct in place of the environment and machines fields, plus stopMachinesWatcher
and stopEnvironWatcher methods (the two invalidate ones), with no further
changes.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:71: func (e *environment) invalidate() {
This should return the error it finds. Call sites can selectively ignore it, but
methods such as Provisioner.Stop should return the problems it finds, so we can
have a handle on them if we want (testing should definitely check, for
instance).

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:73: log.Printf("provisioner: environment watcher
exited: %v", e.watcher.Stop())
That Stop call is the most critical task done by this method. It shouldn't be
disguised at the end of a log call.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:136: func (p *Provisioner) innerLoop() {
Nice organization.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:152: log.Printf("provisioning: new configuartion
applied")
s/configuartion/environment configuration/ (note typo)

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:159: }
I'm wondering if rather than "continue" in the problems above, we should
"break", and here have something that verifies if the state is still sane? 
Otherwise this will be an infinite loop, won't it?

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:167: p.environment.invalidate()
I suggest logic like this here:

p.tomb.Kill(nil)
err1 := p.stopEnvironWatcher()
err2 := p.stopMachinesWatcher()
err3 := p.tomb.Wait()
for err := []error{err3, err2, err1} {
    if err != nil {
        return err
    }
}
return nil

[dave_cheney.net, 2012-05-31 22:40:03]

Please take a look.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:37: st, err := state.Open(&a.Conf.StateInfo)
On 2012/05/31 11:46:59, niemeyer wrote:
> What happens if the connection is broken? We'll need to reestablish it and
> restart from the beginning, since all the assumptions are now wrong. Maybe
this
> has to be moved into the outer loop or something. Will leave details with you
at
> this point.

I have been operating under the assumption (I think it's correct based on what
William and Roger were talking about yesterday) that all the agents will run
under upstart so can assume that if they exit or SEGV they will be restarted.

> That said, I suggest keeping this for the branch coming right after this one.
It
> feels like what we have here is a solid step forward, and we can comfortably
> iterate over the state reestablishment in a new branch before continuing with
> everything else.

I agree, i'd like to see that as a future enhancement, and lean on upstart for
the moment. See also comments about state.IsClosed/IsValid below.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:56: type environment struct {
On 2012/05/31 11:46:59, niemeyer wrote:
> Those two types feel unnecessary. It looks like your model would work equally
> well if we had environWatcher and machinesWatcher both within the Provisioner
> struct in place of the environment and machines fields, plus
stopMachinesWatcher
> and stopEnvironWatcher methods (the two invalidate ones), with no further
> changes.

This was an indulgence. I am besotted with the ability to bind a few methods to
a structure and then embed the structure as a way of composing more complex
types. I've redone it, but retained a wrapper methods.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:71: func (e *environment) invalidate() {
On 2012/05/31 11:46:59, niemeyer wrote:
> This should return the error it finds. Call sites can selectively ignore it,
but
> methods such as Provisioner.Stop should return the problems it finds, so we
can
> have a handle on them if we want (testing should definitely check, for
> instance).

Done.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:73: log.Printf("provisioner: environment watcher
exited: %v", e.watcher.Stop())
On 2012/05/31 11:46:59, niemeyer wrote:
> That Stop call is the most critical task done by this method. It shouldn't be
> disguised at the end of a log call.

Done.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:136: func (p *Provisioner) innerLoop() {
On 2012/05/31 11:46:59, niemeyer wrote:
> Nice organization.

Thank you. As much as I would like to have only one loop, there is a hard
requirement to have a valid Environ before reading anything off the machines
channel. To remove that would mean that every machine change has to be recorded
in some structure which is serviced by a goroutine. That means more tomb.Tombs
to check, locks around the Envrion, etc. This is most of what would be required
for a concurrent PA.

I do want to make the PA concurrent in the future, but not just yet.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:152: log.Printf("provisioning: new configuartion
applied")
On 2012/05/31 11:46:59, niemeyer wrote:
> s/configuartion/environment configuration/ (note typo)

Done.

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:159: }
On 2012/05/31 11:46:59, niemeyer wrote:
> I'm wondering if rather than "continue" in the problems above, we should
> "break", and here have something that verifies if the state is still sane? 
> Otherwise this will be an infinite loop, won't it?

At the moment, by my reading, a watcher is always returned from any of the calls
to state.New*Watcher, but if the underlying state is broken, they will
immediately go into Dead state and close their channel.

What I think is needed here is the state/watcher.go methods to be able to return
an error when you ask for a watcher. Or, a method like state.IsClosed() (or
similar) if it existed could be put a the top of the loop, ie.

for !p.st.IsClosed() { select { ... } }

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:167: p.environment.invalidate()
> p.tomb.Kill(nil)
> err1 := p.stopEnvironWatcher()
> err2 := p.stopMachinesWatcher()
> err3 := p.tomb.Wait()
> for err := []error{err3, err2, err1} {
>     if err != nil {
>         return err
>     }
> }
> return nil

SGTM. I'm never sure how to merge several error values into one.

[dave_cheney.net, 2012-06-01 01:58:27]

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:37: st, err := state.Open(&a.Conf.StateInfo)
> What happens if the connection is broken? We'll need to reestablish it and
> restart from the beginning, since all the assumptions are now wrong. Maybe
this
> has to be moved into the outer loop or something. Will leave details with you
at
> this point.

I looked into this more this morning and it looks like if the underlying
connection to ZK is interrupted then the c client reconnects behind the scenes.
I tested this by running the PA, then shutting down zk, then starting it again a
minute later. None of the watchers exited.

I'm going to keep playing with this, but at this point, if gozk never reports an
error when it cant talk to the zk server, I can't think of a way to detect
connection failure.

Cheers

Dave

[dave_cheney.net, 2012-06-01 06:08:06]

Please take a look.

[dave_cheney.net, 2012-06-03 22:55:48]

Please take a look.

[niemeyer, 2012-06-04 23:22:23]

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6250068/diff/5004/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:37: st, err := state.Open(&a.Conf.StateInfo)
On 2012/06/01 01:58:27, dfc wrote:
> > What happens if the connection is broken? We'll need to reestablish it and
> > restart from the beginning, since all the assumptions are now wrong. Maybe
> this
> > has to be moved into the outer loop or something. Will leave details with
you
> at
> > this point.
> 
> I looked into this more this morning and it looks like if the underlying
> connection to ZK is interrupted then the c client reconnects behind the
scenes.
> I tested this by running the PA, then shutting down zk, then starting it again
a
> minute later. None of the watchers exited.
> 
> I'm going to keep playing with this, but at this point, if gozk never reports
an
> error when it cant talk to the zk server, I can't think of a way to detect
> connection failure.

We've covered this on juju-dev@. A TODO would be good here as well.

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

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:56: // environChanges returns a channel that will
receive the new *ConfigNode
As we discussed, that logic can be simplified as we can bubble up onto the top
from any hiccups, rather than retrying in place. I'm happy both for this branch
to be changed to cover that, or for it to be integrated with these and then
refactored. I'll comment in-place and let the decision with you.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:57: // when a change is detected.
// when a change in the environment configuration is detected.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:65: // stopEnvironWatcher stops and invalidates the
current environWatcher.
s/and invalidates//

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:69: log.Printf("provisioning: environWatcher reported
error on Stop: %v", err)
This is Printf rather than Debugf, so we need to be a bit nicer to the user (no
method and field names). Something like this might do:

"environment configuration watcher: %v"

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:76: // changes returns a channel that will receive the
new *ConfigNode when a
Needs updating.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:89: log.Printf("provisioning: machinesWatcehr reported
error on Stop: %v", err)
"machines watcher: %v"

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:107: // TODO(dfc) we need a method like
state.IsValid() here to exit cleanly if
IsConnected?

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:116: continue
This should return an error.. it won't help to continue here. The state will be
broken.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:121: log.Printf("provisioner: unable to create
environment from supplied configuration: %v", err)
"provisioner loaded invalid environment configuration: %v"

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:124: log.Printf("provisioner: valid environment
configured")
"provisioner loaded new environment configuration"

We spent a good while in London fiddling with messages and trying to imagine how
the actual error and log messages look like with prefixes, and the conclusion we
got to is that it doesn't really improve much the understanding, and that we've
been terribly inconsistent so far. The agreement we're trying to get to (which
still accepts input) is being a bit more clear in the messages themselves.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:133: // TODO(dfc) we need a method like
state.IsValid() here to exit cleanly if
IsConnected?

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:142: return fmt.Errorf("environWatcher closed")
"environment configuration watcher was closed"

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:146: log.Printf("provisioner: new configuration
received, but was not valid: %v", err)
"provisioner loaded invalid environment configuration: %v"

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:150: log.Printf("provisioner: new environment
configuartion applied")
"provisioner loaded new environment configuration"

[dave_cheney.net, 2012-06-05 07:59:54]

Please take a look.

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

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:56: // environChanges returns a channel that will
receive the new *ConfigNode
On 2012/06/04 23:22:23, niemeyer wrote:
> As we discussed, that logic can be simplified as we can bubble up onto the top
> from any hiccups, rather than retrying in place. I'm happy both for this
branch
> to be changed to cover that, or for it to be integrated with these and then
> refactored. I'll comment in-place and let the decision with you.

PTAL. This has been incorporated into the loop/innerloop

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:57: // when a change is detected.
On 2012/06/04 23:22:23, niemeyer wrote:
> // when a change in the environment configuration is detected.

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:65: // stopEnvironWatcher stops and invalidates the
current environWatcher.
On 2012/06/04 23:22:23, niemeyer wrote:
> s/and invalidates//

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:69: log.Printf("provisioning: environWatcher reported
error on Stop: %v", err)
On 2012/06/04 23:22:23, niemeyer wrote:
> This is Printf rather than Debugf, so we need to be a bit nicer to the user
(no
> method and field names). Something like this might do:
> 
> "environment configuration watcher: %v"

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:76: // changes returns a channel that will receive the
new *ConfigNode when a
On 2012/06/04 23:22:23, niemeyer wrote:
> Needs updating.

Deleted.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:89: log.Printf("provisioning: machinesWatcehr reported
error on Stop: %v", err)
On 2012/06/04 23:22:23, niemeyer wrote:
> "machines watcher: %v"

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:107: // TODO(dfc) we need a method like
state.IsValid() here to exit cleanly if
On 2012/06/04 23:22:23, niemeyer wrote:
> IsConnected?

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:116: continue
On 2012/06/04 23:22:23, niemeyer wrote:
> This should return an error.. it won't help to continue here. The state will
be
> broken.

PTAL, the collection of the error (if present) is handled by the defer now.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:121: log.Printf("provisioner: unable to create
environment from supplied configuration: %v", err)
On 2012/06/04 23:22:23, niemeyer wrote:
> "provisioner loaded invalid environment configuration: %v" 

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:124: log.Printf("provisioner: valid environment
configured")
Fair enough. I have no preference apart from consistency, so +1

On 2012/06/04 23:22:23, niemeyer wrote:
> "provisioner loaded new environment configuration"
> 
> We spent a good while in London fiddling with messages and trying to imagine
how
> the actual error and log messages look like with prefixes, and the conclusion
we
> got to is that it doesn't really improve much the understanding, and that
we've
> been terribly inconsistent so far. The agreement we're trying to get to (which
> still accepts input) is being a bit more clear in the messages themselves.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:133: // TODO(dfc) we need a method like
state.IsValid() here to exit cleanly if
On 2012/06/04 23:22:23, niemeyer wrote:
> IsConnected?

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:142: return fmt.Errorf("environWatcher closed")
On 2012/06/04 23:22:23, niemeyer wrote:
> "environment configuration watcher was closed"

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:146: log.Printf("provisioner: new configuration
received, but was not valid: %v", err)
On 2012/06/04 23:22:23, niemeyer wrote:
> "provisioner loaded invalid environment configuration: %v" 

Done.

https://codereview.appspot.com/6250068/diff/16001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:150: log.Printf("provisioner: new environment
configuartion applied")
On 2012/06/04 23:22:23, niemeyer wrote:
> "provisioner loaded new environment configuration"

Done.

[niemeyer, 2012-06-05 14:40:18]

That last set of changes are awesome. LGTM.

[dave_cheney.net, 2012-06-05 21:39:00]

*** Submitted:

cmd/jujud: strawman provisioning agent

This is a strawman provisioning agent proposal.

Following Gustavo's suggestion, this revision does not include
any functionality to respond to machines changes.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6250068