Assign clean/empty policies obey constraints

Assign clean/empty policies obey constraints

The assign clean/empty policy has been updated to check the 
hardware characteristics of a machine and if these are 
incompatible with any unit constraints, the machine is not used.
If there are no hardware constraints recorded because the machine
is not yet provisioned, the machine is excluded (better to err on
the side of caution).

https://code.launchpad.net/~wallyworld/juju-core/clean-policy-constraints/+merge/174354

Requires: https://code.launchpad.net/~wallyworld/juju-core/add-unit-force-machine/+merge/174101

(do not edit description out of merge proposal)

[wallyworld, 2013-07-12 07:58:40]

Please take a look.

[wallyworld, 2013-07-12 08:07:50]

Please take a look.

[dimitern, 2013-07-12 08:36:27]

LGTM with a few suggestions.

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go#newcod...
state/assign_test.go:774: var eligibleMachinesInUse = "all eligible machines in
use"
Why not const?

https://codereview.appspot.com/11208044/diff/4001/state/machine.go
File state/machine.go (left):

https://codereview.appspot.com/11208044/diff/4001/state/machine.go#oldcode117
state/machine.go:117: CpuCores   *uint64     `bson:"cpucpores,omitempty"`
Nasty!

https://codereview.appspot.com/11208044/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/unit.go#newcode1057
state/unit.go:1057: if cons.Mem != nil && *cons.Mem > 0 {
All these checks seem to fit nicely along with HasContainer - as methods of
constraints.Value.

[mue, 2013-07-15 14:22:43]

LGTM with Dimiters comments.

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go#newcod...
state/assign_test.go:774: var eligibleMachinesInUse = "all eligible machines in
use"
On 2013/07/12 08:36:27, dimitern wrote:
> Why not const?

+1

[fwereade, 2013-07-15 15:01:29]

This has 2 LGTMs so I'm happy, but I would really appreciate a followup unifying
the painfully similar code across hardware/constraints.

So, to be clear, once this pipeline is in, I think the immediately relevant
priorities are:

1) Fix 25/lxc spelling
2) Rename --force-machine to --to
3) Unify appropriate bits of hardware and constraints

...and in practice, given the simplestreams work etc etc, I'd be fine with a bug
tagged tech-debt instead. Please let me know your plans though :).

https://codereview.appspot.com/11208044/diff/4001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/11208044/diff/4001/instance/instance.go#newcode97
instance/instance.go:97: // MustParseHardware constructs a
HardwareCharacteristics.Value from the supplied arguments,
s/.Value//

https://codereview.appspot.com/11208044/diff/4001/instance/instance.go#newcod...
instance/instance.go:221: }
This is all *so* similar to the constraints stuff that it feels really wrong to
leave it separate. How would you feel about moving constraints into the instance
package so we can share these bits?

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go#newcod...
state/assign_test.go:838: hardwareCharacteristics: "cpu-cores=1",
Hmm; is it sane to have unknown HC? I think we may have discussed this once
before but I forget the details.

https://codereview.appspot.com/11208044/diff/4001/state/machine.go
File state/machine.go (left):

https://codereview.appspot.com/11208044/diff/4001/state/machine.go#oldcode117
state/machine.go:117: CpuCores   *uint64     `bson:"cpucpores,omitempty"`
On 2013/07/12 08:36:27, dimitern wrote:
> Nasty!

Ouch, indeed, I had to look quite hard to spot that. Nice catch.

https://codereview.appspot.com/11208044/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/unit.go#newcode1049
state/unit.go:1049: // deploying the unit. This can happen if the machine is not
yt provisioned. It may be that
s/yt/yet/

https://codereview.appspot.com/11208044/diff/4001/state/unit.go#newcode1052
state/unit.go:1052: var suitableInstancData []instanceData
s/Instanc/Instance/

[wallyworld, 2013-07-16 01:32:30]

I've already done the syntax change for lxc:25.

With the suggestion to rename --force-machine to --to, this is not correct. We
want to keep force-machine as is (it deliberately ignores constraimnts). The
--to option will be like force-machine but fail if constraints cannot be met. At
least that's how I remember it.

https://codereview.appspot.com/11208044/diff/4001/instance/instance.go
File instance/instance.go (right):

https://codereview.appspot.com/11208044/diff/4001/instance/instance.go#newcode97
instance/instance.go:97: // MustParseHardware constructs a
HardwareCharacteristics.Value from the supplied arguments,
On 2013/07/15 15:01:30, fwereade wrote:
> s/.Value//

Done.

https://codereview.appspot.com/11208044/diff/4001/instance/instance.go#newcod...
instance/instance.go:221: }
On 2013/07/15 15:01:30, fwereade wrote:
> This is all *so* similar to the constraints stuff that it feels really wrong
to
> leave it separate. How would you feel about moving constraints into the
instance
> package so we can share these bits?

Agreed and I had the same thoughts. I wanted to get this work done to deliver
functionality and followup with a quick discussion on where to put the common
code. I think moving constraints to instance seems reasonable.

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go#newcod...
state/assign_test.go:774: var eligibleMachinesInUse = "all eligible machines in
use"
On 2013/07/15 14:22:44, mue wrote:
> On 2013/07/12 08:36:27, dimitern wrote:
> > Why not const?
> 
> +1

Done.

https://codereview.appspot.com/11208044/diff/4001/state/assign_test.go#newcod...
state/assign_test.go:838: hardwareCharacteristics: "cpu-cores=1",
On 2013/07/15 15:01:30, fwereade wrote:
> Hmm; is it sane to have unknown HC? I think we may have discussed this once
> before but I forget the details.

My recollection is that it depends on the provider but I could be wrong. In most
cases arch, cores and mem will be known always. And only ec2 provides cpupower
afaik. But I didn't want to encode any assumption about what would be known just
in case a provider can give arch and mem but not core or whatever. So there
tests just ensure the worst case scenario is covered, in that we reject a
machine if it has constraint values for which the hardware is not known.

https://codereview.appspot.com/11208044/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11208044/diff/4001/state/unit.go#newcode1049
state/unit.go:1049: // deploying the unit. This can happen if the machine is not
yt provisioned. It may be that
On 2013/07/15 15:01:30, fwereade wrote:
> s/yt/yet/

Done.

https://codereview.appspot.com/11208044/diff/4001/state/unit.go#newcode1052
state/unit.go:1052: var suitableInstancData []instanceData
On 2013/07/15 15:01:30, fwereade wrote:
> s/Instanc/Instance/

Done.

https://codereview.appspot.com/11208044/diff/4001/state/unit.go#newcode1057
state/unit.go:1057: if cons.Mem != nil && *cons.Mem > 0 {
On 2013/07/12 08:36:27, dimitern wrote:
> All these checks seem to fit nicely along with HasContainer - as methods of
> constraints.Value.

Agreed. The hardware characteristics and constraints code needs to be
consolidated in a followup so I was leaving it till then