Add constraints validation to providers

Add constraints validation to providers

Each provider has a constraints validator which
is used when setting constraints on a machine or
service, as well as when constraints are merged.
The validation step allows conflicting constraints
like instance-type and mem to be rejected, and also
unsupported constraints to be logged with a warning.
The merge step allows things like instance-type to
mask other incompatible constraints like mem or arch,
and visa versa.

https://code.launchpad.net/~wallyworld/juju-core/instance-type-constraint/+merge/216244

Requires: https://code.launchpad.net/~wallyworld/juju-core/constraints-validation-merge/+merge/215807

(do not edit description out of merge proposal)

[wallyworld, 2014-04-17 03:22:32]

Please take a look.

[wallyworld, 2014-04-17 03:28:53]

Please take a look.

[wallyworld, 2014-04-17 06:13:09]

Please take a look.

[fwereade, 2014-04-18 08:25:04]

I think there's a SetEnvironConstraints somewhere that needs to be validated,
and I'm a bit uncertain about some of the what-we-support choices; but this is
generally looking very solid. Thanks.

I also think we need some more integration with the Prechecker -- those probably
ought to check the constraints internally -- but that's fine as a followup.

https://codereview.appspot.com/88780043/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/88780043/diff/40001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:107: logger.Warningf("%v", err)
Not for this CL, but we really do need a channel by which we can pass this sort
of warning back to a user on the other side of the API (bootstrap is fine, I
know, but all the other times....).

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/ec2.go#newcod...
provider/ec2/ec2.go:369: []string{constraints.Mem, constraints.CpuCores})
CpuPower too, I think

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/local_test.go...
provider/ec2/local_test.go:378: c.Assert(cons, gc.DeepEquals,
constraints.MustParse("arch=i386 instance-type=foo cpu-power=10"))
Ideally instance-type's value will itself be validated here -- such that
cc2.x8large is rejected in the regions that don't provide them, etc etc. That'd
be reasonable as a followup, possibly alongside the API that lists available
instance-type values (also maybe good to do the same for arch?).

https://codereview.appspot.com/88780043/diff/40001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/local/environ.go#...
provider/local/environ.go:306: constraints.Tags,
Really, mem and cpu-cores are basically unsupported too (even if we *could* do
something with them in some cases).

This seems fine as it is (progress not perfection) but we might want a followup
that massages some of these a bit.

https://codereview.appspot.com/88780043/diff/40001/provider/manual/environ.go
File provider/manual/environ.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/manual/environ.go...
provider/manual/environ.go:273: constraints.Tags,
Not sure about this. Shouldn't these all be supported? Constraints for
add-machine are a bit weird, but constraints for deploying service units on
particular machines absolutely should be accepted and matched against the
machine's hardware characteristics, I think.

https://codereview.appspot.com/88780043/diff/40001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/88780043/diff/40001/state/machine.go#newcode1097
state/machine.go:1097: logger.Warningf("setting constraints on machine %q: %v",
m.Tag(), err)
m.Id()?

[wallyworld, 2014-04-18 13:51:11]

On 2014/04/18 08:25:04, fwereade wrote:
> I think there's a SetEnvironConstraints somewhere that needs to be validated,
> and I'm a bit uncertain about some of the what-we-support choices; but this is
> generally looking very solid. Thanks.
> 

I added checks to the SetEnvironConstraints method.

> I also think we need some more integration with the Prechecker -- those
probably
> ought to check the constraints internally -- but that's fine as a followup.
> 

Next branch adds stuff to Prechecker.

[wallyworld, 2014-04-18 13:51:18]

Please take a look.

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/ec2.go#newcod...
provider/ec2/ec2.go:369: []string{constraints.Mem, constraints.CpuCores})
On 2014/04/18 08:25:04, fwereade wrote:
> CpuPower too, I think

Done.

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/local_test.go...
provider/ec2/local_test.go:378: c.Assert(cons, gc.DeepEquals,
constraints.MustParse("arch=i386 instance-type=foo cpu-power=10"))
On 2014/04/18 08:25:04, fwereade wrote:
> Ideally instance-type's value will itself be validated here -- such that
> cc2.x8large is rejected in the regions that don't provide them, etc etc.
That'd
> be reasonable as a followup, possibly alongside the API that lists available
> instance-type values (also maybe good to do the same for arch?).

The instance-type check is done in the next branch as part of the
PrecheckInstance() step. I thought about doing it on the constraints.Validate()
step but this would involve plugging provider specific behaviour into the
constraints package and we already have the pre check stuff that will do the
job, and will cause an error before a new machine record is added to state. It
won't prevent service constraints from being set with a bad instance type
however. But I thought the implementation chosen is a viable approach
nonetheless.

https://codereview.appspot.com/88780043/diff/40001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/local/environ.go#...
provider/local/environ.go:306: constraints.Tags,
On 2014/04/18 08:25:04, fwereade wrote:
> Really, mem and cpu-cores are basically unsupported too (even if we *could* do
> something with them in some cases).
> 
> This seems fine as it is (progress not perfection) but we might want a
followup
> that massages some of these a bit.

I think we should support mem, and I recall that we actually use it to set the
container size (or were going to).  I'll add cpu cores to the list though.

https://codereview.appspot.com/88780043/diff/40001/provider/manual/environ.go
File provider/manual/environ.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/manual/environ.go...
provider/manual/environ.go:273: constraints.Tags,
On 2014/04/18 08:25:04, fwereade wrote:
> Not sure about this. Shouldn't these all be supported? Constraints for
> add-machine are a bit weird, but constraints for deploying service units on
> particular machines absolutely should be accepted and matched against the
> machine's hardware characteristics, I think.

Tags I don't think we need, nor instance-type or cpu-power. I'll remove the
rest.

https://codereview.appspot.com/88780043/diff/40001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/88780043/diff/40001/state/machine.go#newcode1097
state/machine.go:1097: logger.Warningf("setting constraints on machine %q: %v",
m.Tag(), err)
On 2014/04/18 08:25:04, fwereade wrote:
> m.Id()?

Done.

[fwereade, 2014-04-21 10:48:14]

LGTM if you promise to do a followup that implements constraint vocabularies ;).

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/88780043/diff/40001/provider/ec2/local_test.go...
provider/ec2/local_test.go:378: c.Assert(cons, gc.DeepEquals,
constraints.MustParse("arch=i386 instance-type=foo cpu-power=10"))
On 2014/04/18 13:51:19, wallyworld wrote:
> On 2014/04/18 08:25:04, fwereade wrote:
> > Ideally instance-type's value will itself be validated here -- such that
> > cc2.x8large is rejected in the regions that don't provide them, etc etc.
> That'd
> > be reasonable as a followup, possibly alongside the API that lists available
> > instance-type values (also maybe good to do the same for arch?).
> 
> The instance-type check is done in the next branch as part of the
> PrecheckInstance() step. I thought about doing it on the
constraints.Validate()
> step but this would involve plugging provider specific behaviour into the
> constraints package and we already have the pre check stuff that will do the
> job, and will cause an error before a new machine record is added to state. It
> won't prevent service constraints from being set with a bad instance type
> however. But I thought the implementation chosen is a viable approach
> nonetheless.

Hmm. ISTM that there are several, uh, vocab-constrained constraints -- arch,
instance-type, and tags -- and each of them vary by cloud, and it'll almost
certainly be to our benefit to be able to reject the crazy ones before they get
into state anywhere at all -- I'm really quite concerned about the delay between
the bad specification and the actual error reporting.

It can go into a followup, this clearly remains a good direction, but I think we
do need to be able to fail fast on those values. Some of them are potentially
tricky, ofc -- the set of arches available depends on the juju tools available,
and the images, but we've at least partly addressed that issue already iirc.

https://codereview.appspot.com/88780043/diff/60001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/88780043/diff/60001/state/service.go#newcode810
state/service.go:810: 
I'm also bothered that we don't have any way to communicate this back to the
user [who isn't running debug-log, at least]. Out of scope, but irritating.