MaaS tag support in constraints

MaaS tag support in constraints

Added a new constraint called tags. In MaaS this corresponds to maas-tags, it is currently unsupported in other providers, though it could be implemented in the future to allow a pass-through for provider-specific features.  Tags are simply comma-delimited strings.

Added code to disable constraints.Value serialization to yaml, since goyaml has bugs with how it handles slices.

https://code.launchpad.net/~natefinch/juju-core/013-maas-tags/+merge/187298

(do not edit description out of merge proposal)

[natefinch, 2013-09-24 17:23:38]

Please take a look.

[natefinch, 2013-09-24 17:39:39]

Note, most of the changed files are small mechanical changes only. I added a
constraints.IsEmpty method since before we were checking for empty constraints
by comparing against the default intialized struct, and now that the constraints
struct has a slice in it, you can't do == comparisons. The meat of the change is
in constraints.go and constraints_test.go.

[gz, 2013-09-25 10:50:42]

Looks right overall, I have some doubts about what we want to store constraints
as in the db.

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

https://codereview.appspot.com/13802045/diff/1/constraints/constraints.go#new...
constraints/constraints.go:56: v.Container == nil &&
Indentation is a little funny here, why the extra level after Arch?

https://codereview.appspot.com/13802045/diff/1/constraints/constraints.go#new...
constraints/constraints.go:356: tags := strings.Split(s, ",")
Previously, space seperated tags were also accepted, as in:
    juju set-contraints "tags=one two"
or
    juju set-contraints "tags=one, two"

https://codereview.appspot.com/13802045/diff/1/environs/instances/instancetyp...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/13802045/diff/1/environs/instances/instancetyp...
environs/instances/instancetype.go:138: machineTags := map[string]struct{}{}
Using an empty struct value seems a funny way to emulate a set, though I guess
we don't have the option of None as per the old python way, and the js version
using bool has downsides as well.

https://codereview.appspot.com/13802045/diff/1/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/13802045/diff/1/instance/instance.go#newcode224
instance/instance.go:224: tags := strings.Split(s, ",")
Just splitting on comma should be fine hee, as we'll only parse things we've
already serialised, right?

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

https://codereview.appspot.com/13802045/diff/1/state/constraints.go#newcode25
state/constraints.go:25: Tags      []string ",omitempty"
I would much prefer we record tag constraints as a plain string, in normalised
form, when putting them in the database. A simple list of tags is fine, but I
don't like our migration strategy for constraints, and if we want in future to
for instance support NOT(tag) and other boolean operations, being able to do so
without changing the schema seems far preferable for the minimal amount of extra
processing required.

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

https://codereview.appspot.com/13802045/diff/1/state/machine.go#newcode125
state/machine.go:125: Tags       []string    `bson:"tags,omitempty"`
Storing the tags a machine had as a list seems okay in comparison.

[natefinch, 2013-09-25 13:46:24]

I think we addressed most of this in the standup and later conversations?  We
seem fine with comma as the separator, and storing as a list of strings in the
DB for now.  Let me know if there's anything else you think needs doing.

[fwereade, 2013-09-25 14:48:42]

LGTM assuming you're ok with the following.

https://codereview.appspot.com/13802045/diff/1/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/13802045/diff/1/provider/ec2/ec2.go#newcode97
provider/ec2/ec2.go:97: // Tags currently not supported by EC2
Eh, tricky. This throws up the issue of handling unsupported constraints again,
and it gets harder to dodge each time. At this stage, for consistency's sake, I
think you should just log an "ignored unsupported XXX constraint" at
provisioning time for these non-tag providers; we can tighten it up later if we
have to.

https://codereview.appspot.com/13802045/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/13802045/diff/1/provider/maas/environ.go#newco...
provider/maas/environ.go:159: }
Does maas not also return filled-in hardware characteristics from StartInstance?
If not, that's a bug -- it means that maas nodes effectively can't be
automatically reused. If it does, I think it probably ought to include the
actual tags of the machine that was provisioned (not just the ones we
specified).

Either way, it's fine to defer the necessary work to a followup.

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

https://codereview.appspot.com/13802045/diff/1/state/constraints.go#newcode25
state/constraints.go:25: Tags      []string ",omitempty"
On 2013/09/25 10:50:42, gz wrote:
> I would much prefer we record tag constraints as a plain string, in normalised
> form, when putting them in the database. A simple list of tags is fine, but I
> don't like our migration strategy for constraints, and if we want in future to
> for instance support NOT(tag) and other boolean operations, being able to do
so
> without changing the schema seems far preferable for the minimal amount of
extra
> processing required.

I think the prospect of fancy tag expressions is sufficiently remote (never
implemented in maas, right?) that we should stick with the natural
representation of the data as it exists today.

[fwereade, 2013-09-25 14:52:54]

Hmm, I missed a rather important file somehow. Important question which may
rescind the approval above:

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

https://codereview.appspot.com/13802045/diff/1/constraints/constraints_test.g...
constraints/constraints_test.go:290: c.Check(con.Tags, gc.IsNil)
Hold on, how do we handle fallbacks? "tags=" on a service ought to mask explicit
tags on the environment, and that should be distinct from nil tags (which would
fall back).

[natefinch, 2013-09-25 17:54:25]

Please take a look.

[fwereade, 2013-09-25 18:36:34]

LGTM

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

https://codereview.appspot.com/13802045/diff/14001/constraints/constraints_te...
constraints/constraints_test.go:350: {RootDisk: uint64p(109876)},
Add some roundtrip tests with both nil and {} please.

Might be worth doing the equivalent in state.

[natefinch, 2013-09-27 15:44:26]

Please take a look.

[natefinch, 2013-09-27 18:08:49]

Please take a look.

[fwereade, 2013-09-27 18:23:29]

LGTM as far as it goes, let's get this in.

We also need to return hardware characteristics from StartInstance (and maybe
maas doesn't even do that..?) -- including tags -- and we should try to match
existing machines with tags inside the unit assignment bits in state.

https://codereview.appspot.com/13802045/diff/36001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/13802045/diff/36001/constraints/constraints.go...
constraints/constraints.go:50: // empty list will override any default tags,
where a nil list will not.
Add a comment here explaining the sad situation.