Introduce InstanceDistributor policy

Introduce InstanceDistributor policy

This change introduces an InstanceDistributor
policy, which will be used to implement automatic
Availability Zone management for AWS and OpenStack.

When an attempt is made to assign a unit to a clean
machine, we gather the instances that have already
been provisioned, and provide those and the unit's
distribution group to the provider.  The provider
may select as many or as few of the candidates as it
wishes to allow; the assignment policy will then
choose one of them for assigning to. If no candidates
are returned, the unit will be assigned to a new
machine.

There are currently no implementations of this policy;
they will be forthcoming when manual AZ support lands
in the amazon/openstack providers.

https://code.launchpad.net/~axwalk/juju-core/provider-unit-assignment-policy/+merge/221481

(do not edit description out of merge proposal)

[axw, 2014-05-30 05:43:52]

Please take a look.

[fwereade, 2014-05-30 08:51:12]

LGTM with a bit of mindful rearrangement.

https://codereview.appspot.com/99660047/diff/1/state/distribution.go
File state/distribution.go (right):

https://codereview.appspot.com/99660047/diff/1/state/distribution.go#newcode42
state/distribution.go:42: distributionGroup, err := service.ServiceInstances()
fwiw, this doesn't feel *quite* right because we don't actually ever need/use
the *Service directly -- the AllUnits could be figured out from the unit name
alone, so grabbing the service doc is just an unnecessary db hit (I think?).

But it's a balance: using the methods we already have for familarity/clarity's
sake, vs writing more primitives that operate more on ids and less on
instantiated objects and *might* eventually lead us in a leaner/cleaner
direction. I have a slight preference for the latter but I'll leave it to your
judgment.

https://codereview.appspot.com/99660047/diff/1/state/distribution.go#newcode54
state/distribution.go:54: func (service *Service) ServiceInstances()
([]instance.Id, error) {
Suggestion: ServiceInstances(*state.Service). I retain a mild discomfort over
spreading the methods on a particular type across files, but just making the
receiver a regular param pretty much eliminates that feeling of ick. I'm not
sure it's *entirely* rational, but...

Anyway, same applies to Unit.distribute above. Doesn't feel like it's quite in
the right place.

https://codereview.appspot.com/99660047/diff/1/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/99660047/diff/1/state/policy.go#newcode204
state/policy.go:204: // TODO(axw) move this comment
where to? ;p

[axw, 2014-05-30 12:42:06]

Please take a look.

https://codereview.appspot.com/99660047/diff/1/state/distribution.go
File state/distribution.go (right):

https://codereview.appspot.com/99660047/diff/1/state/distribution.go#newcode42
state/distribution.go:42: distributionGroup, err := service.ServiceInstances()
On 2014/05/30 08:51:12, fwereade wrote:
> fwiw, this doesn't feel *quite* right because we don't actually ever need/use
> the *Service directly -- the AllUnits could be figured out from the unit name
> alone, so grabbing the service doc is just an unnecessary db hit (I think?).

Indeed it is unnecessary. I've changed ServiceInstances to take a service name,
and extracted the logic of Service.AllUnits out to a free function which takes a
service name.

> But it's a balance: using the methods we already have for familarity/clarity's
> sake, vs writing more primitives that operate more on ids and less on
> instantiated objects and *might* eventually lead us in a leaner/cleaner
> direction. I have a slight preference for the latter but I'll leave it to your
> judgment.

https://codereview.appspot.com/99660047/diff/1/state/distribution.go#newcode54
state/distribution.go:54: func (service *Service) ServiceInstances()
([]instance.Id, error) {
On 2014/05/30 08:51:12, fwereade wrote:
> Suggestion: ServiceInstances(*state.Service). I retain a mild discomfort over
> spreading the methods on a particular type across files, but just making the
> receiver a regular param pretty much eliminates that feeling of ick. I'm not
> sure it's *entirely* rational, but...
> 
> Anyway, same applies to Unit.distribute above. Doesn't feel like it's quite in
> the right place.

Changed both to free functions.

https://codereview.appspot.com/99660047/diff/1/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/99660047/diff/1/state/policy.go#newcode204
state/policy.go:204: // TODO(axw) move this comment
On 2014/05/30 08:51:12, fwereade wrote:
> where to? ;p

Yeah, it's probably best just left here.