Refactor and improve machine supported containers

Refactor and improve machine supported containers

Change the machine supported containers api to set containers
rather than adding. In addition, add functionality to iterate 
any existing containers, and if they are not supported, set
their status to error.

https://code.launchpad.net/~wallyworld/juju-core/improve-supported-containers-setup/+merge/195701

(do not edit description out of merge proposal)

[wallyworld, 2013-11-19 03:09:11]

Please take a look.

[jameinel, 2013-11-20 10:30:10]

I have some comments, and I think there can be a bit more work done. But if you
agree, I think you can just land it. LGTM

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

https://codereview.appspot.com/28790043/diff/1/cmd/jujud/machine.go#oldcode239
cmd/jujud/machine.go:239: }
same here

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

https://codereview.appspot.com/28790043/diff/1/cmd/jujud/machine.go#newcode235
cmd/jujud/machine.go:235: return fmt.Errorf("clearing supported containers for
%s: %v", tag, err)
Should this be something like "unable to clear supported containers" or
"failed while clearing supported containers".
Just "clearing supported containers for X: ERR" doesn't actually sound like an
error.

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

https://codereview.appspot.com/28790043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:25: type SetSupportedContainersParams struct {
Given William's comment about "naming this what it is vs what it does"

Would this be better as just:

type ContainerParams struct {
  Machines []MachineContainers
}

or something along those lines?

I can understand that it is the explicit params being passed to a given API, so
probably not a strict here, but I also don't think it would be terrible if 2
APIs shared the parameters, which means maybe it shouldn't be named after the
api.

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

https://codereview.appspot.com/28790043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:204: err =
machine.AddSupportedContainers(arg.ContainerTypes)
Given that the agent itself is already doing this if len()==0 {
SupportsNoContainers() )

should we make this an Err because it indicates a logic error from the client
side?

Alternatively, why do we need SupportsNoContainers rather than just passing the
empty list? (I slightly prefer 2 functions so that passing items you don't know
doesn't accidentally succeed when you don't actually get what you thought you
were getting, which would leave this as an error case.)

Anyway, I'd like to see that either we get rid of the check in jujud or the
check here (and make it an error to supply 0).

If we need it for compatibility, then put a comment to that effect and about
when we could remove the compatibility. (eg, // needed to support Juju 1.16
which will pass an empty list here)

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

https://codereview.appspot.com/28790043/diff/1/state/machine.go#newcode872
state/machine.go:872: currentContainers, err := m.Containers()
Can you pull all of this into its own function? It seems logically a distinct
operation even if we still do it at the end of setting the supported containers
list.

https://codereview.appspot.com/28790043/diff/1/state/machine.go#newcode872
state/machine.go:872: currentContainers, err := m.Containers()
Can you move this block into its own function and just call it here?

It seems logically an independent action, even if we do call it on every update
to the list of supported containers.

I'm slightly concerned about the "restart" overhead from this. (On upgrade all
machine agents end up restarted, and they'll all probe and send these requests
yet again.) But I *think* this is still O(N) sort of stuff, which should be ok.
(if you have M machines and an average of N containers, you'll loop over M*N of
them, but only one time, so that seems ok.)

https://codereview.appspot.com/28790043/diff/1/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/28790043/diff/1/state/machine_test.go#newcode1154
state/machine_test.go:1154: func (s *MachineSuite)
TestSetSupportedContainersSingle(c *gc.C) {
I didn't go over this particularly thoroughly as it looks like just changing Add
to Set. If there are bits to pay attention to, please point it out.

[wallyworld, 2013-11-21 12:35:32]

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

https://codereview.appspot.com/28790043/diff/1/cmd/jujud/machine.go#newcode235
cmd/jujud/machine.go:235: return fmt.Errorf("clearing supported containers for
%s: %v", tag, err)
On 2013/11/20 10:30:10, jameinel wrote:
> Should this be something like "unable to clear supported containers" or
> "failed while clearing supported containers".
> Just "clearing supported containers for X: ERR" doesn't actually sound like an
> error.

I had thought our convention for error strings was to use a verb like "writing",
"reading", "deleting" or whatever, followed by a phrase describing what was
being written, read etc. 

eg some examples in existing code

fmt.Errorf("processing config file: %v", err)
fmt.Errorf("reading config file: %v", err)
fmt.Errorf("opening config file: %v", err)

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

https://codereview.appspot.com/28790043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:25: type SetSupportedContainersParams struct {
On 2013/11/20 10:30:10, jameinel wrote:
> Given William's comment about "naming this what it is vs what it does"
> 
> Would this be better as just:
> 
> type ContainerParams struct {
>   Machines []MachineContainers
> }
> 
> or something along those lines?
> 
> I can understand that it is the explicit params being passed to a given API,
so
> probably not a strict here, but I also don't think it would be terrible if 2
> APIs shared the parameters, which means maybe it shouldn't be named after the
> api.

I don't *think* just ContainerParams is generally sufficient since there may be
another api which needs to be passed a different ContainerParams with different
contents, so better to be explicit to avoid such potential clashes. Hmmm. Not
sure. I can see both sides. I'll see if it makes sense to change what's there.

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

https://codereview.appspot.com/28790043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:204: err =
machine.AddSupportedContainers(arg.ContainerTypes)
On 2013/11/20 10:30:10, jameinel wrote:
> Given that the agent itself is already doing this if len()==0 {
> SupportsNoContainers() )
> 
> should we make this an Err because it indicates a logic error from the client
> side?
> 
> Alternatively, why do we need SupportsNoContainers rather than just passing
the
> empty list? (I slightly prefer 2 functions so that passing items you don't
know
> doesn't accidentally succeed when you don't actually get what you thought you
> were getting, which would leave this as an error case.)
> 
> Anyway, I'd like to see that either we get rid of the check in jujud or the
> check here (and make it an error to supply 0).
> 
> If we need it for compatibility, then put a comment to that effect and about
> when we could remove the compatibility. (eg, // needed to support Juju 1.16
> which will pass an empty list here)

I like 2 functions as well since I prefer not to have the caller pass an empty
list to mean "none". But this is 2 functions on the caller side of the api.
Here, I am having the caller internally invoke a single server side api and
doing the empty list trick, thereby keeping the internal api footprint as small
as possible. 

The check is jujud is sort of different. There, the supported containers for a
machine is figured out. If none are supported, I have it call
SupportsNoContainers() as opposed to SetSupportedContainers(empty list) since I
like the explicit approach. But internally, this is mapped to a single server
side api, which then expands back out to calling one of 2 functions on the
machine object. 

I could simply introduce a SupportsNoContainers to the internal api. But I
thought it better to reuse an existing internal api where possible.

I hope the above makes sense.

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

https://codereview.appspot.com/28790043/diff/1/state/machine.go#newcode872
state/machine.go:872: currentContainers, err := m.Containers()
On 2013/11/20 10:30:10, jameinel wrote:
> Can you pull all of this into its own function? It seems logically a distinct
> operation even if we still do it at the end of setting the supported
containers
> list.

Ok

https://codereview.appspot.com/28790043/diff/1/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/28790043/diff/1/state/machine_test.go#newcode1154
state/machine_test.go:1154: func (s *MachineSuite)
TestSetSupportedContainersSingle(c *gc.C) {
On 2013/11/20 10:30:10, jameinel wrote:
> I didn't go over this particularly thoroughly as it looks like just changing
Add
> to Set. If there are bits to pay attention to, please point it out.

Just a name change

[wallyworld, 2013-11-23 10:37:06]

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

https://codereview.appspot.com/28790043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:25: type SetSupportedContainersParams struct {
On 2013/11/20 10:30:10, jameinel wrote:
> Given William's comment about "naming this what it is vs what it does"
> 
> Would this be better as just:
> 
> type ContainerParams struct {
>   Machines []MachineContainers
> }
> 
> or something along those lines?
> 
> I can understand that it is the explicit params being passed to a given API,
so
> probably not a strict here, but I also don't think it would be terrible if 2
> APIs shared the parameters, which means maybe it shouldn't be named after the
> api.


I renamed to MachineContainersParams