Refactor container provisioner

This branch changes how containers are provisioned.
Before, the container provisioner task is started with the machine agent. Now the start up of that task is delayed until a container is actually requested. The basic concept is:
- set up container listener
- when container of type is requested:
  * stop container listener
  * ensure dependencies of that container type are installed
  * start container provisioning task


https://code.launchpad.net/~wallyworld/juju-core/lazy-container-provisioners/+merge/194795

(do not edit description out of merge proposal)

[fwereade, 2013-11-13 09:31:00]

These may be redundant now, I'll look at the rest again.

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:31: Errors []*Error
Do we really need another list-of-errors type? Don't we already have this?

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:38: ContainerTypes []instance.ContainerType
Is this really "add", or "set"?

[jameinel, 2013-11-13 09:37:57]

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:31: Errors []*Error
On 2013/11/13 09:31:01, fwereade wrote:
> Do we really need another list-of-errors type? Don't we already have this?

ErrorResults

It isn't exactly just a list but I think it is probably better to reuse it.

[fwereade, 2013-11-13 10:26:10]

A few thoughts -- generally looking solid though.

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
File worker/provisioner/container_initialisation.go (right):

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:61: // it will be because a
container has been added.
Are you sure? Don't we get an empty initial event? I think we should...

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:72: cs.watcher.Stop()
error? I can see an argument against, but it should be made explicitly in a
comment :)

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:86: return
startProvisionerWorker(cs.runner, provisionerType, cs.provisioner, cs.config)
Should we have mirror logic in the provisioner that switches back to one of
these when there are no more containers? Just a thought, probably not actually
worth it.

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:97: return nil
suggestion -- keep this type focused on runner manipulation, and make the
individual provisioner kinds responsible for setting up their own dependencies?
That'd make Handle (with non-empty ids) become a simple
start-provisioner-stop-self deal, and I *think* the runner should be able to
handle that trivially already without the atomic int32 lark.

[wallyworld, 2013-11-14 05:25:08]

Please take a look.

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:31: Errors []*Error
On 2013/11/13 09:37:57, jameinel wrote:
> On 2013/11/13 09:31:01, fwereade wrote:
> > Do we really need another list-of-errors type? Don't we already have this?
> 
> ErrorResults
> 
> It isn't exactly just a list but I think it is probably better to reuse it.

I didn't know that existed - I fixed the upstream branch

https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:38: ContainerTypes []instance.ContainerType
On 2013/11/13 09:31:01, fwereade wrote:
> Is this really "add", or "set"?

It has been designed as add, so that additional container types can be recorded
if necessary (eg as and when dependencies are downloaded or whatever). Not
something we necessarily do today, but I thought it the most flexible approach.

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
File worker/provisioner/container_initialisation.go (right):

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:61: // it will be because a
container has been added.
On 2013/11/13 10:26:11, fwereade wrote:
> Are you sure? Don't we get an empty initial event? I think we should...

I added a check for the initial event to discard it

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:72: cs.watcher.Stop()
On 2013/11/13 10:26:11, fwereade wrote:
> error? I can see an argument against, but it should be made explicitly in a
> comment :)

I logged a warning

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:86: return
startProvisionerWorker(cs.runner, provisionerType, cs.provisioner, cs.config)
On 2013/11/13 10:26:11, fwereade wrote:
> Should we have mirror logic in the provisioner that switches back to one of
> these when there are no more containers? Just a thought, probably not actually
> worth it.

We don't need this IMO. If there are no more containers, we just wait till
another is requested.

https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/contain...
worker/provisioner/container_initialisation.go:97: return nil
On 2013/11/13 10:26:11, fwereade wrote:
> suggestion -- keep this type focused on runner manipulation, and make the
> individual provisioner kinds responsible for setting up their own
dependencies?
> That'd make Handle (with non-empty ids) become a simple
> start-provisioner-stop-self deal, and I *think* the runner should be able to
> handle that trivially already without the atomic int32 lark.

I still think we want the int32 check just in case (it is very little overhead).
The provisioner task is responsible for starting a container and I'd like to
keep it with that responsibility only. There may we be another component created
in the container package to manage installing the dependencies for each
container type. For now though, I just want to have it as an empty hook to show
the intent that this is something we need to do.

[william.reade, 2013-11-18 15:30:58]

I don't really love the proliferation of watchers here -- isn't it simpler to
have one task responsible for starting/stopping the various container
provsioners as they're necessary? It's a new watcher in state but not, I think,
a very complex one -- and it'd do an end-run around this whole business with the
ContainerSetup worker knowing its own name, and stopping itself in a
non-standard way.

I'll try to be on for a chat (my) tonight, otherwise we can talk (your) tonight.

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

https://codereview.appspot.com/25040043/diff/120001/cmd/jujud/machine.go#newc...
cmd/jujud/machine.go:237: if err :=
machine.AddSupportedContainers(containers...); err != nil {
This does look to me more like we're setting the full set of supported
containers, not just adding them. And if we *were* adding them -- rather than
setting them -- wouldn't we need another api call to get the full set, so we
could start the right ContainerSetup workers below?

https://codereview.appspot.com/25040043/diff/120001/state/api/provisioner/mac...
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/25040043/diff/120001/state/api/provisioner/mac...
state/api/provisioner/machine.go:247: args := params.AddSupportedContainers{
Why do we keep naming the params after what they're used for instead of what
they are?

https://codereview.appspot.com/25040043/diff/120001/state/apiserver/provision...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/25040043/diff/120001/state/apiserver/provision...
state/apiserver/provisioner/provisioner.go:188: func (p *ProvisionerAPI)
AddSupportedContainers(
Wait a mo, this isn't actually used by the provisioner, is it?

https://codereview.appspot.com/25040043/diff/120001/worker/provisioner/contai...
File worker/provisioner/container_initialisation.go (right):

https://codereview.appspot.com/25040043/diff/120001/worker/provisioner/contai...
worker/provisioner/container_initialisation.go:80: if err :=
cs.ensureContainerDependencies(); err != nil {
I still think this is better done lazily inside the relevant broker, so that you
can just create one and use it when necessary -- rather than add this implicit
dependency on another worker (this change means that the only legitimate way to
start a provisioner worker is via this other worker, which is a bit
surprising... I understand that's what we'll be doing, but I suspect it's going
to bite someone at some point).

And since it's not implemented... can we just leave it out for now, and have
that fight next CL? ;p

https://codereview.appspot.com/25040043/diff/120001/worker/provisioner/contai...
worker/provisioner/container_initialisation.go:118: return
NewProvisioner(provisionerType, provisioner, config), nil
This is all still kinda messed up (nothing to do with this CL though). I'm sure
we had noble plans to just have NewProvisioner(watcher, broker) at one stage --
is there anything blocking us from gradually moving in that direction?

[william.reade, 2013-11-18 23:22:22]

OK, after much discussion, this LGTM given a SetSupportedContainers API rather
than AddSupportedContainers. I think there's still room to improve further, but
demanding perfection at the expense of progress is sucky and I won't do that.
Any more. Today...