Use single watcher for containr setup

Use single watcher for containr setup

Add a WatchAllContainers API so that a single watcher
can be used to start (as needed) each container provisioner,
instead of using one eatcher per contsiner type.

This branch will need trunk merged and be updated to use the
revised SetSupportedContainers API call once my other branch 
gets approved and landed.

https://code.launchpad.net/~wallyworld/juju-core/single-container-watcher/+merge/195711

(do not edit description out of merge proposal)

[wallyworld, 2013-11-19 06:52:12]

Please take a look.

[jameinel, 2013-11-20 12:36:45]

Some comments, but the rest LGTM

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

https://codereview.appspot.com/28890043/diff/1/cmd/jujud/machine.go#newcode241
cmd/jujud/machine.go:241: watcherName := fmt.Sprintf("%s-container-watcher",
machine.Id())
Should we be pulling out naming into its own function, rather than just doing it
on the fly?

I don't know what the name is actually used for. It seems like it is just a hint
about what is going on, but if anything else actually wants to reference
something by name, I'd like us to have a common function for it.

https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:245: // to the lifecycles of all containers on
the machine.
Since it has been raised, I just want a quick overview. I think this new API
means that old agents (1.16 agents) will continue to work with newer API server
(1.18). But that newer agents will not work with an older API server.

I'm pretty sure we decided that we are OK in that situation.

Do you agree?

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:149: return p.WatchContainers(args)
This looks *very* strange to me. If WatchAllContainers is just implemented by
passing the same parameters to WatchContainers, why aren't we just calling
WatchContainers from the Agent. (Do we actually need a new API).

I would be less surprised if they used a common helper but one set a parameter
differently than the other.

But if we can get the same result by just passing the right args to the existing
API, I'd rather just do that.

To put it another way, I think you changed p.WatchContainers so that if you pass
no type it suddenly gives you everything. That might actually be better than a
new WatchAllContainers API.

https://codereview.appspot.com/28890043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/28890043/diff/1/state/watcher.go#newcode204
state/watcher.go:204: isChild := fmt.Sprintf("^%s/[a-z]*/%s$", m.doc.Id,
names.NumberSnippet)
I'm curious if this is the actual syntax we'll use for container types (they
must all be only lowercase ascii letters).
I think we have some named regexes in the names? package.
It feels like if we are going to match it here, we probably need a matching
"when asking for X it must also match this regex so we know you aren't asking
for something we won't find later".

One potentially cheeky way to implement this is to actually have 

ANYCTYPE = instance.ContainerType("[a-z]*")

or whatever by the name. But you *could* just pass that to WatchContainers and
have it work.

Might just be crackful.

[wallyworld, 2013-11-21 12:16:09]

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

https://codereview.appspot.com/28890043/diff/1/cmd/jujud/machine.go#newcode241
cmd/jujud/machine.go:241: watcherName := fmt.Sprintf("%s-container-watcher",
machine.Id())
On 2013/11/20 12:36:45, jameinel wrote:
> Should we be pulling out naming into its own function, rather than just doing
it
> on the fly?
> 
> I don't know what the name is actually used for. It seems like it is just a
hint
> about what is going on, but if anything else actually wants to reference
> something by name, I'd like us to have a common function for it.

The name is passed as a parameter when initialising the container handler
struct, and recorded as a field in that struct for use later. It is used to stop
the worker when it's job is done. Here is the only place the name is
constructed.

https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:245: // to the lifecycles of all containers on
the machine.
On 2013/11/20 12:36:45, jameinel wrote:
> Since it has been raised, I just want a quick overview. I think this new API
> means that old agents (1.16 agents) will continue to work with newer API
server
> (1.18). But that newer agents will not work with an older API server.
> 
> I'm pretty sure we decided that we are OK in that situation.
> 
> Do you agree?

Yes, I agree with the assessment. Whether for 1.18 we want to put in the work to
ensure backwards compatibility, I'm not sure. But I do think it's reasonable to
expect people to upgrade their state servers to the latest version they are
running, even if they want to have some nodes still on an older version.

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:149: return p.WatchContainers(args)
On 2013/11/20 12:36:45, jameinel wrote:
> This looks *very* strange to me. If WatchAllContainers is just implemented by
> passing the same parameters to WatchContainers, why aren't we just calling
> WatchContainers from the Agent. (Do we actually need a new API).
> 
> I would be less surprised if they used a common helper but one set a parameter
> differently than the other.
> 
> But if we can get the same result by just passing the right args to the
existing
> API, I'd rather just do that.
> 
> To put it another way, I think you changed p.WatchContainers so that if you
pass
> no type it suddenly gives you everything. That might actually be better than a
> new WatchAllContainers API.

The client API for watching containers is currently
WatchContainers(containerType)

There's no sensible way (IMO) to adapt this to watch all containers. I don't
like the idea of messing with this to say WatchContainers("") means watch all.
Hence I introduced a new explicit WatchAllContainers() API. But under the
covers, I could look at having the WatchAllContainers client call the current
WatchContainers server API with "" as the container type. At least this hack
would not be user facing.

https://codereview.appspot.com/28890043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/28890043/diff/1/state/watcher.go#newcode204
state/watcher.go:204: isChild := fmt.Sprintf("^%s/[a-z]*/%s$", m.doc.Id,
names.NumberSnippet)
On 2013/11/20 12:36:45, jameinel wrote:
> I'm curious if this is the actual syntax we'll use for container types (they
> must all be only lowercase ascii letters).
> I think we have some named regexes in the names? package.
> It feels like if we are going to match it here, we probably need a matching
> "when asking for X it must also match this regex so we know you aren't asking
> for something we won't find later".
> 
> One potentially cheeky way to implement this is to actually have 
> 
> ANYCTYPE = instance.ContainerType("[a-z]*")
> 
> or whatever by the name. But you *could* just pass that to WatchContainers and
> have it work.
> 
> Might just be crackful.

I thought they would be all lower case letters. I don't see there being any
pattern as such, just any lower case letters. I could add a named regexp. Didn't
seem worth it, but it's easy enough to do.

[jameinel, 2013-11-23 05:08:59]

https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:245: // to the lifecycles of all containers on
the machine.
I believe the official stance is that newer API server is expected. So for
*Agents* we have to support Agent N, API N+2, but not the other way around. So
we should be ok.

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:149: return p.WatchContainers(args)
Except the way you've implemented it, WatchContainers("") *does* give you this
behavior (over the API).
If you don't actually want that, then we should factor the code into a common
helper, and say pass in the appropriate watcher.

That way someone calling WatchContainers wouldn't be allowed to pass in the
empty string.

https://codereview.appspot.com/28890043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/28890043/diff/1/state/watcher.go#newcode204
state/watcher.go:204: isChild := fmt.Sprintf("^%s/[a-z]*/%s$", m.doc.Id,
names.NumberSnippet)
My concern here is that it fails silently and will cause someone who is adding a
new type in the future to scratch their head wondering why it isn't working.

I could even argue for %s/[^/]+/%s

Actually, we probably want a + rather than a * anyway, because we don't want to
match 0//1 right?

If you don't see a great place to make the failure more active, fine, but naming
it helps increase its visibility a little bit, at least.

[wallyworld, 2013-11-29 00:02:45]

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:149: return p.WatchContainers(args)
On 2013/11/23 05:09:00, jameinel wrote:
> Except the way you've implemented it, WatchContainers("") *does* give you this
> behavior (over the API).
> If you don't actually want that, then we should factor the code into a common
> helper, and say pass in the appropriate watcher.
> 
> That way someone calling WatchContainers wouldn't be allowed to pass in the
> empty string.

I've enhanced the client api for WatchContainers to reject invalid container
types

https://codereview.appspot.com/28890043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/28890043/diff/1/state/watcher.go#newcode204
state/watcher.go:204: isChild := fmt.Sprintf("^%s/[a-z]*/%s$", m.doc.Id,
names.NumberSnippet)
On 2013/11/23 05:09:00, jameinel wrote:
> My concern here is that it fails silently and will cause someone who is adding
a
> new type in the future to scratch their head wondering why it isn't working.
> 
> I could even argue for %s/[^/]+/%s
> 
> Actually, we probably want a + rather than a * anyway, because we don't want
to
> match 0//1 right?
> 
> If you don't see a great place to make the failure more active, fine, but
naming
> it helps increase its visibility a little bit, at least.

Bah, the * instead of + was a typo.

We already have a constant defined in names/machine.go

ContainerSnippet = "(/[a-z]+/" + NumberSnippet + ")"

So I've pulled out the [a-z]+ and made it a new constant:

ContainerTypeSnippet = "[a-z]+"

I think it's reasonable to assume all container type names will be [a-z]+
I could construct a regexp by iterating over all container types and making a
regexp like [lxc|kvm] but I don't thin that adds enough value?