Support instance type constraints

Support instance type constraints

A new constraint, "instance-type" is supported
for EC2 and Openstack providers. For all other
providers, a warning is logged and the constraint
is ignored.

To support this, a new state policy is introduced:
ConstraintsValidator. This is invoked at the time a
new machine is added, or a unit is added to a service.
It checks that any instance type exists and for ec2
that the architecture is valid. If it errors, nothing
is written to state.

EC2 and Opentack have different ways of combining
env and deployment constraints. eg for EC2, root disk
is specified separately to instance type but for 
Openstack is part of the instance type.

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

(do not edit description out of merge proposal)

[wallyworld, 2014-04-04 06:40:42]

Please take a look.

[fwereade, 2014-04-07 09:44:44]

discussed briefly live, but for the record: not lgtm as is because the meaning
of "instance-type" changes per-provider. In particular, ec2 instance-type does
not imply root-disk, but openstack instance-type does; and, worse, in ec2
instance-type *may* imply arch but does not necessarily do so. I'm not really
sure about other providers, I'm afraid.

What all this essentially means is that we need to delegate
fallback/inconsistency calculations to the environs themselves. See the python
code (juju.machine.constraints IIRC) for the original mechanism, which may help
to clarify what I'm babbling about here.

One upside: this will let us fail early when (say) a user asks for an arch that
isn't available in a given cloud. IIRC python did this super-early, and while we
won't want to do it client-side we will want to do so before we write to the db.

FWIW, additional note: to be done-done for this feature we'll want to expose a
client API that lists instance types and their characteristics, so that the gui
can produce nice little dropdowns etc.

[wallyworld, 2014-04-08 06:31:07]

Please take a look.

[fwereade, 2014-04-11 12:53:02]

I think this needs another round or two... if I'm not making myself clear here,
I'll be around on sunday night to talk to tim; and some of monday morning if
that's too early for you.

https://codereview.appspot.com/84310044/diff/20001/constraints/constraints.go
File constraints/constraints.go (left):

https://codereview.appspot.com/84310044/diff/20001/constraints/constraints.go...
constraints/constraints.go:106: func (v Value) WithFallbacks(v0 Value) Value {
I'd really like to make this a free func, lest we imply that this operation is
meaningful in the face of differing provider implementations.

https://codereview.appspot.com/84310044/diff/20001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/84310044/diff/20001/constraints/constraints.go...
constraints/constraints.go:122: // specifying any other constraint term
overrides instance type.
But not every constraint does overlap with instance-type...

https://codereview.appspot.com/84310044/diff/20001/constraints/constraints.go...
constraints/constraints.go:210: func (v *Value) Remove(attrNames []string)
(Value, error) {
func (v *Value) Without(attrNames []string)?

Remove implies mutation.

https://codereview.appspot.com/84310044/diff/20001/environs/jujutest/livetest...
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/84310044/diff/20001/environs/jujutest/livetest...
environs/jujutest/livetests.go:165: err :=
prechecker.PrecheckInstance("precise")
/me raises eyebrow, declines further comment for now

https://codereview.appspot.com/84310044/diff/20001/provider/azure/environ_tes...
File provider/azure/environ_test.go (right):

https://codereview.appspot.com/84310044/diff/20001/provider/azure/environ_tes...
provider/azure/environ_test.go:1541: `for azure provider "testenv"`},
Fine, so long as we're clear that azure *does* have instance types and we'll
need to add them soon enough :).

https://codereview.appspot.com/84310044/diff/20001/provider/common/constraint...
File provider/common/constraints.go (right):

https://codereview.appspot.com/84310044/diff/20001/provider/common/constraint...
provider/common/constraints.go:16: func ValidateConstraints(
I still think this is something like `MergeConstraints(basic, fallback
constraints.Value) constraints.Value`, and that we need a distinct
`ValidateConstraints(constraints.Value) error` (and the property that merging
two validated sets of constraints should always result in valid constraints)

https://codereview.appspot.com/84310044/diff/20001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/84310044/diff/20001/provider/ec2/ec2.go#newcod...
provider/ec2/ec2.go:388: if archMatches(itype.Arches, result.Arch) {
doesn't WithFallbacks clear out non-InstanceType values?

https://codereview.appspot.com/84310044/diff/20001/provider/ec2/image_test.go
File provider/ec2/image_test.go (right):

https://codereview.appspot.com/84310044/diff/20001/provider/ec2/image_test.go...
provider/ec2/image_test.go:133: cons:   "instance-type=cc1.4xlarge
cpu-power=400",
This has to be an error.

https://codereview.appspot.com/84310044/diff/20001/provider/ec2/image_test.go...
provider/ec2/image_test.go:143: cons, _ :=
validateConstraints(constraints.MustParse(t.cons), constraints.Value{})
Yeah, this is a hack. Please separate validation and merging.

https://codereview.appspot.com/84310044/diff/20001/provider/openstack/image.go
File provider/openstack/image.go (right):

https://codereview.appspot.com/84310044/diff/20001/provider/openstack/image.g...
provider/openstack/image.go:68: consWithoutInstType.InstanceType = nil
Yeah, this breaks my brain, here and in ec2. If there's an instance type in the
constraints, all the overlapping fields should be cleared out *before it's even
recorded in state*. When we get them coming back into the provider, a quick
check for validity is fine (and good) but by the time we're picking
images/instances we should know which we're using and have cleared out the ones
we're not.

https://codereview.appspot.com/84310044/diff/20001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/84310044/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:363: if err := st.precheckInstance(parentTemplate.Series);
err != nil {
Hmm, I see. I'd rather keep passing constraints in here (along with placement
directives when axw's work lands), just so we can give the provider the best
possible chance of rejecting invalid combinations.

https://codereview.appspot.com/84310044/diff/20001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/84310044/diff/20001/state/policy.go#newcode114
state/policy.go:114: resultCons := cons.WithFallbacks(envCons)
Can we just just demand that an environ implement something here? Just make it
part of the interface?

https://codereview.appspot.com/84310044/diff/20001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/84310044/diff/20001/state/service.go#newcode805
state/service.go:805: func (s *Service) SetConstraints(cons constraints.Value)
(err error) {
These need to be validated, *without* being combined with anything else. This is
the biggest reason we need to separate validation from merging.

OK, you *could* get away with a merge-with-no-constraints, but... eww. Please
don't ;).