Add DistributionGroup to StartInstanceParams

Add DistributionGroup to StartInstanceParams

DistributionGroup returns instances that
StartInstance should consider when provisioning
for high availability (i.e. the new instance +
those in DistributionGroup should be distributed
for high availability.)

DistributionGroup is implemented in terms of
a new provisioner API method of the same name.
This API returns different instances depending
on the machine passed in. For an environ manager,
all other environ manager instances are returned;
for a non-environ manager, all other instances
with services in common are returned.

This is in preparation for modifying the Azure
provider to group instances into the same cloud
service when in availability-sets-enabled mode.

(Note: the previous patch included a change to
PrecheckInstance; this has been reverted, and
will be implemented differently in another CL.)

https://code.launchpad.net/~axwalk/juju-core/startinstance-principalunit/+merge/210746

Requires: https://code.launchpad.net/~axwalk/juju-core/startinstance-param-struct/+merge/210738

(do not edit description out of merge proposal)

[axw, 2014-03-13 06:40:19]

Please take a look.

[wallyworld, 2014-03-14 02:12:53]

Looks ok pending William's +1.
But there's a missing test for the client PrincipalUnits API call in
state/api/provisioner

[axw, 2014-03-14 02:31:19]

Please take a look.

[axw, 2014-03-19 08:10:38]

Please take a look.

[hduran, 2014-03-19 17:31:41]

https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/prov...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/prov...
state/api/provisioner/provisioner_test.go:235: 
In  CommonServiceInstances there are four return statements, three of them with
error states yet this test only sees the possibility of a successful run of
Call, is there no way to test the other three possible outcomes?

[axw, 2014-03-20 03:58:36]

Please take a look.

https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/prov...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/prov...
state/api/provisioner/provisioner_test.go:235: 
On 2014/03/19 17:31:42, hduran wrote:
> In  CommonServiceInstances there are four return statements, three of them
with
> error states yet this test only sees the possibility of a successful run of
> Call, is there no way to test the other three possible outcomes?

Testing the first two errors is not possible without creating a custom API
server. Kind of a cop out, but precisely none of the other API methods test them
either.

The third error can be tested, and in fact is tested in
apiserver/provisioner_test.go. I will add a test here, too, though.

[axw, 2014-03-20 06:40:50]

Please take a look.

[fwereade, 2014-03-20 09:23:53]

Looks really good. I'm open to arguments re notfound/unauth; the only thing I'm
not sure about is handling for container provisioners. Have these been directly
considered?

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
state/apiserver/provisioner/provisioner_test.go:568: // show up in the results,
but it will have a blank instance.Id.
I'm not 100% sure about that. Feels better to filter them out at API level than
to make every provider have to deal with empty instances in the group.

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
state/apiserver/provisioner/provisioner_test.go:607: {Error:
apiservertesting.NotFoundError("machine 42")},
I'm wondering about notfound vs unauthorized. For an env provisioner, I suspect
that the Right Thing is to NotFound unknown top-level machines, and to
Unauthorized all hosted machines regardless of existence; and for container
provisioners, to Unauthorized everything except precisely those containers for
which the provisioner is directly responsible; but that's a pile of tedious
complexity, and if we just use unauth instead of notfound across the board we
might end up with simpler code and less theoretical info leakage. Thoughts?

https://codereview.appspot.com/75250043/diff/80001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/75250043/diff/80001/state/state_test.go#newcod...
state/state_test.go:969: err := s.State.EnsureAvailability(3,
constraints.Value{}, "quantal")
fwiw, it makes me ever so happy to see this just casually being called in tests
:)

[axw, 2014-03-21 07:55:56]

Please take a look.

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
state/apiserver/provisioner/provisioner_test.go:568: // show up in the results,
but it will have a blank instance.Id.
On 2014/03/20 09:23:53, fwereade wrote:
> I'm not 100% sure about that. Feels better to filter them out at API level
than
> to make every provider have to deal with empty instances in the group.

Sorry, that's actually a leftover comment from a previous implementation. It
doesn't do that anymore. I've fixed up the comments.

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
state/apiserver/provisioner/provisioner_test.go:607: {Error:
apiservertesting.NotFoundError("machine 42")},
On 2014/03/20 09:23:53, fwereade wrote:
> I'm wondering about notfound vs unauthorized. For an env provisioner, I
suspect
> that the Right Thing is to NotFound unknown top-level machines, and to
> Unauthorized all hosted machines regardless of existence; and for container
> provisioners, to Unauthorized everything except precisely those containers for
> which the provisioner is directly responsible; but that's a pile of tedious
> complexity, and if we just use unauth instead of notfound across the board we
> might end up with simpler code and less theoretical info leakage. Thoughts?

At the moment, the auth function allows the env provisioner to access; a machine
agent may access its own machine; and hosted machines are only accessible to the
machine agent that contains it. That seems sane to me. The tests don't reflect
this, as there's no containers. I'll update the tests.

However... I don't plan to use DistributionGroup on container managers, at least
not yet. You can't really change how a container is distributed, since its host
is fixed before we get to this point.

[fwereade, 2014-03-24 10:12:20]

LGTM. Taking DG into account when choosing pre-existing instances is not in
scope for this CL, but please make sure it's on your list of
necessary-features-before-done-done.

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisione...
state/apiserver/provisioner/provisioner_test.go:607: {Error:
apiservertesting.NotFoundError("machine 42")},
On 2014/03/21 07:55:56, axw wrote:
> However... I don't plan to use DistributionGroup on container managers, at
least
> not yet. You can't really change how a container is distributed, since its
host
> is fixed before we get to this point.

That's fine, I think, although we still need to pay some attention to DG in
containers in *some* way -- ie, if we have a containerised service, we still
want to distribute its units' containers across real instances that are
themselves distributed. That's a different piece of code though.

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
state/apiserver/provisioner/provisioner.go:334: // Sort values to simplify
testing.
copy(instanceIds, instanceIdSet.SortedValues())

?

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
state/apiserver/provisioner/provisioner_test.go:563: // Unassign wordpress/1
from machine-1.
blank lines before comments for ease of reading please

https://codereview.appspot.com/75250043/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/state.go#newcode323
state/state.go:323: func (st *State) ManagerMachines() (machines []*Machine, err
error) {
Please liaise with rog, this may need to change and I want to be sure he knows
about it.

[axw, 2014-03-24 14:12:20]

Please take a look.

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
state/apiserver/provisioner/provisioner.go:334: // Sort values to simplify
testing.
On 2014/03/24 10:12:20, fwereade wrote:
> copy(instanceIds, instanceIdSet.SortedValues())
> 
> ?

Nope, different types: []instance.Id/[]string.

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provision...
state/apiserver/provisioner/provisioner_test.go:563: // Unassign wordpress/1
from machine-1.
On 2014/03/24 10:12:20, fwereade wrote:
> blank lines before comments for ease of reading please

Done.

https://codereview.appspot.com/75250043/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/state.go#newcode323
state/state.go:323: func (st *State) ManagerMachines() (machines []*Machine, err
error) {
On 2014/03/24 10:12:20, fwereade wrote:
> Please liaise with rog, this may need to change and I want to be sure he knows
> about it.

Rog brought State.StateServerInfo to my attention, which does what I need.
There'll be up to 7 (replicaset.Max) additional State.Machine calls, but this is
low volume so I think that's fine.

[fwereade, 2014-03-27 12:39:25]

LGTM

[axw, 2014-04-03 03:01:59]

Please take a look.