Add "container" to constraints

Add "container" to constraints

This branch adds support for a container constraint to be
specified. The constraint is specified like so:
container=lxc
It is stored in state for environment constraints and
service constraints but not used yet.

The container type and other related definitions were
moved from the state package to instance to avoid
dependency issues.

https://code.launchpad.net/~wallyworld/juju-core/container-constraint/+merge/171235

(do not edit description out of merge proposal)

[wallyworld, 2013-06-25 08:24:03]

Please take a look.

[fwereade, 2013-06-26 14:10:01]

I think we might need some thought about what happens in state -- I think we
should also barf if we set container constraints there.

And, how should they interact with the provider code that handles constraints? I
suspect that environs should barf if they're given constraints that include a
container value that's not nil or empty.

https://codereview.appspot.com/10534043/diff/1/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/10534043/diff/1/cmd/juju/addmachine_test.go#ne...
cmd/juju/addmachine_test.go:98: 
Do these inherit from environment constraints when they're applied? If so,
there's potential for confusion here.

https://codereview.appspot.com/10534043/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/10534043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:170: 
Eww! Can we raise a bug for that?

[wallyworld, 2013-06-27 13:58:58]

On 2013/06/26 14:10:01, fwereade wrote:
> I think we might need some thought about what happens in state -- I think we
> should also barf if we set container constraints there.
> 
> And, how should they interact with the provider code that handles constraints?
I
> suspect that environs should barf if they're given constraints that include a
> container value that's not nil or empty.
>

I'll look at this and raise an error in the appropriate place(s).
 
> https://codereview.appspot.com/10534043/diff/1/cmd/juju/addmachine_test.go
> File cmd/juju/addmachine_test.go (right):
> 
>
https://codereview.appspot.com/10534043/diff/1/cmd/juju/addmachine_test.go#ne...
> cmd/juju/addmachine_test.go:98: 
> Do these inherit from environment constraints when they're applied? If so,
> there's potential for confusion here.
> 

Both the machine provisioning and deployment constraints do/will inherit from
the environment constraints. The provisioning constraints do today. The use of
deployment constraints is not written yet. But I think it's sensible to say 
juju bootstrap --constraints mem=4G container=lxc
in order to set the default deployment rules, which in the above case would be
to deploy charms onto a 4GB lxc container. But the user could also say:
juju deploy mysql --constraints container=
which would deploy the charm onto a 4G raw instance. 
or
juju deploy mysql --constraints mem=2G
which would deploy the charm onto a 2G lxc container. 

I still think we will may want to differentiate between provisioning and
deployment constraints?

> https://codereview.appspot.com/10534043/diff/1/constraints/constraints.go
> File constraints/constraints.go (right):
> 
>
https://codereview.appspot.com/10534043/diff/1/constraints/constraints.go#new...
> constraints/constraints.go:170: 
> Eww! Can we raise a bug for that?

We can. I need to figure out exactly where the issue is. I *think* it's in
goyaml.

[dimitern, 2013-06-27 14:28:17]

diff is screwed, please repropose

[wallyworld, 2013-06-27 14:30:31]

Please take a look.

[dimitern, 2013-06-27 15:12:44]

Looks good, LGTM with fwereade's concerns fixed.

[wallyworld, 2013-06-28 01:43:09]

We need to store the container constraint value in the environment constraints
so that it can be used as a default deployment constraint. The only place we
don't need to store container is in machine constraints. Provisioning ignores
the container value anyway, but I've explicitly cleared any container value that
may be present when writing out machine constraints to avoid confusion.

[wallyworld, 2013-06-28 01:45:38]

Please take a look.

[fwereade, 2013-06-28 12:38:21]

LGTM, just a couple of notes:

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go
File constraints/constraints.go (left):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go...
constraints/constraints.go:155: }
Please make the ordering of the fields and their handling consistent. I'd
recommend alphabetical but any scheme's fine really so long as it's applied
consistently.

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go...
constraints/constraints.go:207: v.Container = &ctype
Is this an indication that the domain here is larger than that of ContainerType,
and that it might be better to just store it as a *string on Value? YMMV, I
don't know which is best myself, and I'm a bit suspicious of my own motivations
in suggesting it.

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go#newco...
instance/instance.go:24: // ParseSupportedContainerType converts the specified
string into a supported
OrNone

[thumper, 2013-06-30 22:46:54]

LGTM - with a few suggestions

moving the container bits in instance package to their own file, and tests for
container=none

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go
File constraints/constraints.go (left):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go...
constraints/constraints.go:155: }
On 2013/06/28 12:38:21, fwereade wrote:
> Please make the ordering of the fields and their handling consistent. I'd
> recommend alphabetical but any scheme's fine really so long as it's applied
> consistently.

+1 on alphabetical

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints_te...
File constraints/constraints_test.go (right):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints_te...
constraints/constraints_test.go:51: args:    []string{"container="},
I'd really like to see "container=none" to be internally handled as
"container=".

I think setting to none is more understandable to a user than setting to the
empty string.

What do you think?

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go#newco...
instance/instance.go:11: type ContainerType string
I'm a fan of smaller files.  Can we put the container related stuff into
instance/container.go ?

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go#newco...
instance/instance.go:14: NONE = ContainerType("none")
\o/

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go#newco...
instance/instance.go:24: // ParseSupportedContainerType converts the specified
string into a supported
On 2013/06/28 12:38:21, fwereade wrote:
> OrNone

It took me a few minutes to parse the above comment.  The method name at the
start of the comment doesn't match the method name.

https://codereview.appspot.com/10534043/diff/23001/instance/instance_test.go
File instance/instance_test.go (right):

https://codereview.appspot.com/10534043/diff/23001/instance/instance_test.go#...
instance/instance_test.go:1: // Copyright 2013 Canonical Ltd.
and container_test.go

https://codereview.appspot.com/10534043/diff/23001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10534043/diff/23001/state/state.go#newcode293
state/state.go:293: // clearing it avoids potential confusion.
The pedant in me says you should make the comment justified :-)

In emacs this is meta-q

[wallyworld, 2013-07-01 01:09:59]

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go
File constraints/constraints.go (left):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go...
constraints/constraints.go:155: }
On 2013/06/30 22:46:54, thumper wrote:
> On 2013/06/28 12:38:21, fwereade wrote:
> > Please make the ordering of the fields and their handling consistent. I'd
> > recommend alphabetical but any scheme's fine really so long as it's applied
> > consistently.
> 
> +1 on alphabetical

Done.

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints.go...
constraints/constraints.go:207: v.Container = &ctype
On 2013/06/28 12:38:21, fwereade wrote:
> Is this an indication that the domain here is larger than that of
ContainerType,
> and that it might be better to just store it as a *string on Value? YMMV, I
> don't know which is best myself, and I'm a bit suspicious of my own
motivations
> in suggesting it.

This wouldn't be an issue if we used getters (or Python). I'd rather that when
people type cons.ContainerType they get a value of the correct type, even if it
means we have a few internal implementation complications.

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints_te...
File constraints/constraints_test.go (right):

https://codereview.appspot.com/10534043/diff/23001/constraints/constraints_te...
constraints/constraints_test.go:51: args:    []string{"container="},
On 2013/06/30 22:46:54, thumper wrote:
> I'd really like to see "container=none" to be internally handled as
> "container=".
> 
> I think setting to none is more understandable to a user than setting to the
> empty string.
> 
> What do you think?

Thanks for mentioning this. "container=" is like saying "arch=" ie we don't
care. I did add a new container type "none", but forgot to add a test for it.

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go#newco...
instance/instance.go:11: type ContainerType string
On 2013/06/30 22:46:54, thumper wrote:
> I'm a fan of smaller files.  Can we put the container related stuff into
> instance/container.go ?

Done.

https://codereview.appspot.com/10534043/diff/23001/instance/instance.go#newco...
instance/instance.go:24: // ParseSupportedContainerType converts the specified
string into a supported
On 2013/06/30 22:46:54, thumper wrote:
> On 2013/06/28 12:38:21, fwereade wrote:
> > OrNone
> 
> It took me a few minutes to parse the above comment.  The method name at the
> start of the comment doesn't match the method name.

Done.

https://codereview.appspot.com/10534043/diff/23001/instance/instance_test.go
File instance/instance_test.go (right):

https://codereview.appspot.com/10534043/diff/23001/instance/instance_test.go#...
instance/instance_test.go:1: // Copyright 2013 Canonical Ltd.
On 2013/06/30 22:46:54, thumper wrote:
> and container_test.go

Done.

https://codereview.appspot.com/10534043/diff/23001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10534043/diff/23001/state/state.go#newcode293
state/state.go:293: // clearing it avoids potential confusion.
On 2013/06/30 22:46:54, thumper wrote:
> The pedant in me says you should make the comment justified :-)
> 
> In emacs this is meta-q

I did it like this deliberately. I wanted the top line to stand alone.