|
|
|
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)
Patch Set 1 #
Total comments: 10
Patch Set 2 : MaaS tag support in constraints #
Total comments: 1
Patch Set 3 : MaaS tag support in constraints #Patch Set 4 : MaaS tag support in constraints #
Total comments: 1
Total messages: 11
|
natefinch
Please take a look.
|
12 years, 3 months ago (2013年09月24日 17:23:38 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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.
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.
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.
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.
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).
Please take a look.
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.
Please take a look.
Please take a look.
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.