Merge lp:~axwalk/juju-core/startinstance-principalunit into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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.

https://codereview.appspot.com/75250043/

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://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+210746_code.launchpad.net,

Message:
Please take a look.

Description:
Add PrincipalUnits to StartInstanceParams

Also, pass to principalUnits to PrecheckInstance.

This is in preparation for modifying the Azure
provider to prevent "juju add-machine" when in
availability set mode, and use the principal unit's
service name to decide on which Cloud Service to
allocate the instance to.

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)

Please review this at https://codereview.appspot.com/75250043/

Affected files (+139, -16 lines):
   A [revision details]
   M environs/broker.go
   M environs/jujutest/livetests.go
   M provider/manual/environ.go
   M state/addmachine.go
   M state/api/provisioner/machine.go
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_test.go
   M state/policy.go
   M state/prechecker_test.go
   M worker/provisioner/provisioner_task.go

[Ian Booth (wallyworld)]

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

https://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/75250043/

[Horacio Durán (hduran-8)]

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

https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/provisioner_test.go#newcode235
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?

https://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/provisioner_test.go#newcode235
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.

https://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/75250043/

[William Reade (fwereade)]

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/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisioner/provisioner_test.go#newcode568
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/provisioner/provisioner_test.go#newcode607
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#newcode969
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 :)

https://codereview.appspot.com/75250043/

[William Reade (fwereade)]

WIP for last round of changes (then probably WIP again until 1.19)

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisioner/provisioner_test.go#newcode568
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/provisioner/provisioner_test.go#newcode607
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.

https://codereview.appspot.com/75250043/

[William Reade (fwereade)]

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/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisioner/provisioner_test.go#newcode607
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/provisioner/provisioner.go
File state/apiserver/provisioner/provisioner.go (right):

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

?

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

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provisioner/provisioner_test.go#newcode563
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.

https://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provisioner/provisioner.go#newcode334
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/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provisioner/provisioner_test.go#newcode563
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.

https://codereview.appspot.com/75250043/

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/75250043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/75250043/

=== modified file 'environs/broker.go'
--- environs/broker.go	2014-04-01 16:27:22 +0000
+++ environs/broker.go	2014-04-03 02:58:17 +0000
@@ -11,7 +11,7 @@
 )
 
 // StartInstanceParams holds parameters for the
-// InstanceBroker.StartInstace method.
+// InstanceBroker.StartInstance method.
 type StartInstanceParams struct {
 	// Constraints is a set of constraints on
 	// the kind of instance to create.
@@ -23,6 +23,14 @@
 
 	// MachineConfig describes the machine's configuration.
 	MachineConfig *cloudinit.MachineConfig
+
+	// DistributionGroup, if non-nil, is a function
+	// that returns a slice of instance.Ids that belong
+	// to the same distribution group as the machine
+	// being provisioned. The InstanceBroker may use
+	// this information to distribute instances for
+	// high availability.
+	DistributionGroup func() ([]instance.Id, error)
 }
 
 // TODO(wallyworld) - we want this in the environs/instance package but import loops

=== modified file 'provider/manual/environ.go'
--- provider/manual/environ.go	2014-03-28 13:27:45 +0000
+++ provider/manual/environ.go	2014-04-03 02:58:17 +0000
@@ -57,6 +57,7 @@
 }
 
 var _ envtools.SupportsCustomSources = (*manualEnviron)(nil)
+var _ state.Prechecker = (*manualEnviron)(nil)
 
 var errNoStartInstance = errors.New("manual provider cannot start instances")
 var errNoStopInstance = errors.New("manual provider cannot stop instances")

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2014-04-02 15:46:57 +0000
+++ state/api/params/params.go	2014-04-03 02:58:17 +0000
@@ -647,6 +647,19 @@
 	CACert []byte
 }
 
+// DistributionGroupResult contains the result of
+// the DistributionGroup provisioner API call.
+type DistributionGroupResult struct {
+	Error  *Error
+	Result []instance.Id
+}
+
+// DistributionGroupResults is the bulk form of
+// DistributionGroupResult.
+type DistributionGroupResults struct {
+	Results []DistributionGroupResult
+}
+
 // APIHostPortsResult holds the result of an APIHostPorts
 // call. Each element in the top level slice holds
 // the addresses for one API server.

=== modified file 'state/api/provisioner/machine.go'
--- state/api/provisioner/machine.go	2014-03-31 11:59:16 +0000
+++ state/api/provisioner/machine.go	2014-04-03 02:58:17 +0000
@@ -182,6 +182,29 @@
 	return result.Result, nil
 }
 
+// DistributionGroup returns a slice of instance.Ids
+// that belong to the same distribution group as this
+// Machine. The provisioner may use this information
+// to distribute instances for high availability.
+func (m *Machine) DistributionGroup() ([]instance.Id, error) {
+	var results params.DistributionGroupResults
+	args := params.Entities{
+		Entities: []params.Entity{{Tag: m.tag}},
+	}
+	err := m.st.caller.Call("Provisioner", "", "DistributionGroup", args, &results)
+	if err != nil {
+		return nil, err
+	}
+	if len(results.Results) != 1 {
+		return nil, fmt.Errorf("expected one result, got %d", len(results.Results))
+	}
+	result := results.Results[0]
+	if result.Error != nil {
+		return nil, result.Error
+	}
+	return result.Result, nil
+}
+
 // SetProvisioned sets the provider specific machine id, nonce and also metadata for
 // this machine. Once set, the instance id cannot be changed.
 func (m *Machine) SetProvisioned(id instance.Id, nonce string, characteristics *instance.HardwareCharacteristics) error {

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2014-04-02 10:36:23 +0000
+++ state/api/provisioner/provisioner_test.go	2014-04-03 02:58:17 +0000
@@ -246,6 +246,52 @@
 	c.Assert(series, gc.Equals, "quantal")
 }
 
+func (s *provisionerSuite) TestDistributionGroup(c *gc.C) {
+	apiMachine, err := s.provisioner.Machine(s.machine.Tag())
+	c.Assert(err, gc.IsNil)
+	instances, err := apiMachine.DistributionGroup()
+	c.Assert(err, gc.IsNil)
+	c.Assert(instances, gc.DeepEquals, []instance.Id{"i-manager"})
+
+	machine1, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	apiMachine, err = s.provisioner.Machine(machine1.Tag())
+	c.Assert(err, gc.IsNil)
+	wordpress := s.AddTestingService(c, "wordpress", s.AddTestingCharm(c, "wordpress"))
+
+	err = apiMachine.SetProvisioned("i-d", "fake", nil)
+	c.Assert(err, gc.IsNil)
+	instances, err = apiMachine.DistributionGroup()
+	c.Assert(err, gc.IsNil)
+	c.Assert(instances, gc.HasLen, 0) // no units assigned
+
+	var unitNames []string
+	for i := 0; i < 3; i++ {
+		unit, err := wordpress.AddUnit()
+		c.Assert(err, gc.IsNil)
+		unitNames = append(unitNames, unit.Name())
+		err = unit.AssignToMachine(machine1)
+		c.Assert(err, gc.IsNil)
+		instances, err := apiMachine.DistributionGroup()
+		c.Assert(err, gc.IsNil)
+		c.Assert(instances, gc.DeepEquals, []instance.Id{"i-d"})
+	}
+}
+
+func (s *provisionerSuite) TestDistributionGroupMachineNotFound(c *gc.C) {
+	stateMachine, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	apiMachine, err := s.provisioner.Machine(stateMachine.Tag())
+	c.Assert(err, gc.IsNil)
+	err = apiMachine.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	err = apiMachine.Remove()
+	c.Assert(err, gc.IsNil)
+	_, err = apiMachine.DistributionGroup()
+	c.Assert(err, gc.ErrorMatches, "machine 1 not found")
+	c.Assert(err, jc.Satisfies, params.IsCodeNotFound)
+}
+
 func (s *provisionerSuite) TestConstraints(c *gc.C) {
 	// Create a fresh machine with some constraints.
 	template := state.MachineTemplate{

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2014-03-31 11:59:16 +0000
+++ state/apiserver/provisioner/provisioner.go	2014-04-03 02:58:17 +0000
@@ -11,6 +11,7 @@
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/apiserver/common"
 	"launchpad.net/juju-core/state/watcher"
+	"launchpad.net/juju-core/utils/set"
 )
 
 // ProvisionerAPI provides access to the Provisioner API facade.
@@ -282,6 +283,107 @@
 	return result, nil
 }
 
+// DistributionGroup returns, for each given machine entity,
+// a slice of instance.Ids that belong to the same distribution
+// group as that machine. This information may be used to
+// distribute instances for high availability.
+func (p *ProvisionerAPI) DistributionGroup(args params.Entities) (params.DistributionGroupResults, error) {
+	result := params.DistributionGroupResults{
+		Results: make([]params.DistributionGroupResult, len(args.Entities)),
+	}
+	canAccess, err := p.getAuthFunc()
+	if err != nil {
+		return result, err
+	}
+	for i, entity := range args.Entities {
+		machine, err := p.getMachine(canAccess, entity.Tag)
+		if err == nil {
+			// If the machine is an environment manager, return
+			// environment manager instances. Otherwise, return
+			// instances with services in common with the machine
+			// being provisioned.
+			if machine.IsManager() {
+				result.Results[i].Result, err = environManagerInstances(p.st)
+			} else {
+				result.Results[i].Result, err = commonServiceInstances(p.st, machine)
+			}
+		}
+		result.Results[i].Error = common.ServerError(err)
+	}
+	return result, nil
+}
+
+// environManagerInstances returns all environ manager instances.
+func environManagerInstances(st *state.State) ([]instance.Id, error) {
+	info, err := st.StateServerInfo()
+	if err != nil {
+		return nil, err
+	}
+	instances := make([]instance.Id, 0, len(info.MachineIds))
+	for _, id := range info.MachineIds {
+		machine, err := st.Machine(id)
+		if err != nil {
+			return nil, err
+		}
+		instanceId, err := machine.InstanceId()
+		if err == nil {
+			instances = append(instances, instanceId)
+		} else if !state.IsNotProvisionedError(err) {
+			return nil, err
+		}
+	}
+	return instances, nil
+}
+
+// commonServiceInstances returns instances with
+// services in common with the specified machine.
+func commonServiceInstances(st *state.State, m *state.Machine) ([]instance.Id, error) {
+	units, err := m.Units()
+	if err != nil {
+		return nil, err
+	}
+	var instanceIdSet set.Strings
+	for _, unit := range units {
+		if !unit.IsPrincipal() {
+			continue
+		}
+		service, err := unit.Service()
+		if err != nil {
+			return nil, err
+		}
+		allUnits, err := service.AllUnits()
+		if err != nil {
+			return nil, err
+		}
+		for _, unit := range allUnits {
+			machineId, err := unit.AssignedMachineId()
+			if state.IsNotAssigned(err) {
+				continue
+			} else if err != nil {
+				return nil, err
+			}
+			machine, err := st.Machine(machineId)
+			if err != nil {
+				return nil, err
+			}
+			instanceId, err := machine.InstanceId()
+			if err == nil {
+				instanceIdSet.Add(string(instanceId))
+			} else if state.IsNotProvisionedError(err) {
+				continue
+			} else {
+				return nil, err
+			}
+		}
+	}
+	instanceIds := make([]instance.Id, instanceIdSet.Size())
+	// Sort values to simplify testing.
+	for i, instanceId := range instanceIdSet.SortedValues() {
+		instanceIds[i] = instance.Id(instanceId)
+	}
+	return instanceIds, nil
+}
+
 // Constraints returns the constraints for each given machine entity.
 func (p *ProvisionerAPI) Constraints(args params.Entities) (params.ConstraintsResults, error) {
 	result := params.ConstraintsResults{

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2014-04-02 10:36:23 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2014-04-03 02:58:17 +0000
@@ -598,6 +598,133 @@
 	})
 }
 
+func (s *withoutStateServerSuite) TestDistributionGroup(c *gc.C) {
+	addUnits := func(name string, machines ...*state.Machine) (units []*state.Unit) {
+		svc := s.AddTestingService(c, name, s.AddTestingCharm(c, name))
+		for _, m := range machines {
+			unit, err := svc.AddUnit()
+			c.Assert(err, gc.IsNil)
+			err = unit.AssignToMachine(m)
+			c.Assert(err, gc.IsNil)
+			units = append(units, unit)
+		}
+		return units
+	}
+	setProvisioned := func(id string) {
+		m, err := s.State.Machine(id)
+		c.Assert(err, gc.IsNil)
+		err = m.SetProvisioned(instance.Id("machine-"+id+"-inst"), "nonce", nil)
+		c.Assert(err, gc.IsNil)
+	}
+
+	mysqlUnit := addUnits("mysql", s.machines[0], s.machines[3])[0]
+	wordpressUnits := addUnits("wordpress", s.machines[0], s.machines[1], s.machines[2])
+
+	// Unassign wordpress/1 from machine-1.
+	// The unit should not show up in the results.
+	err := wordpressUnits[1].UnassignFromMachine()
+	c.Assert(err, gc.IsNil)
+
+	// Provision machines 1, 2 and 3. Machine-0 remains
+	// unprovisioned, and machine-1 has no units, and so
+	// neither will show up in the results.
+	setProvisioned("1")
+	setProvisioned("2")
+	setProvisioned("3")
+
+	// Add a few state servers, provision two of them.
+	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	setProvisioned("5")
+	setProvisioned("7")
+
+	// Create a logging service, subordinate to mysql.
+	s.AddTestingService(c, "logging", s.AddTestingCharm(c, "logging"))
+	eps, err := s.State.InferEndpoints([]string{"mysql", "logging"})
+	c.Assert(err, gc.IsNil)
+	rel, err := s.State.AddRelation(eps...)
+	c.Assert(err, gc.IsNil)
+	ru, err := rel.Unit(mysqlUnit)
+	c.Assert(err, gc.IsNil)
+	err = ru.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+
+	args := params.Entities{Entities: []params.Entity{
+		{Tag: s.machines[0].Tag()},
+		{Tag: s.machines[1].Tag()},
+		{Tag: s.machines[2].Tag()},
+		{Tag: s.machines[3].Tag()},
+		{Tag: "machine-5"},
+	}}
+	result, err := s.provisioner.DistributionGroup(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.DistributionGroupResults{
+		Results: []params.DistributionGroupResult{
+			{Result: []instance.Id{"machine-2-inst", "machine-3-inst"}},
+			{Result: []instance.Id{}},
+			{Result: []instance.Id{"machine-2-inst"}},
+			{Result: []instance.Id{"machine-3-inst"}},
+			{Result: []instance.Id{"machine-5-inst", "machine-7-inst"}},
+		},
+	})
+}
+
+func (s *withoutStateServerSuite) TestDistributionGroupEnvironManagerAuth(c *gc.C) {
+	args := params.Entities{Entities: []params.Entity{
+		{Tag: "machine-0"},
+		{Tag: "machine-42"},
+		{Tag: "machine-0-lxc-99"},
+		{Tag: "unit-foo-0"},
+		{Tag: "service-bar"},
+	}}
+	result, err := s.provisioner.DistributionGroup(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.DistributionGroupResults{
+		Results: []params.DistributionGroupResult{
+			// environ manager may access any top-level machines.
+			{Result: []instance.Id{}},
+			{Error: apiservertesting.NotFoundError("machine 42")},
+			// only a machine agent for the container or its
+			// parent may access it.
+			{Error: apiservertesting.ErrUnauthorized},
+			// non-machines always unauthorized
+			{Error: apiservertesting.ErrUnauthorized},
+			{Error: apiservertesting.ErrUnauthorized},
+		},
+	})
+}
+
+func (s *withoutStateServerSuite) TestDistributionGroupMachineAgentAuth(c *gc.C) {
+	anAuthorizer := s.authorizer
+	anAuthorizer.Tag = "machine-1"
+	anAuthorizer.EnvironManager = false
+	anAuthorizer.MachineAgent = true
+	provisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, anAuthorizer)
+	c.Check(err, gc.IsNil)
+	args := params.Entities{Entities: []params.Entity{
+		{Tag: "machine-0"},
+		{Tag: "machine-1"},
+		{Tag: "machine-42"},
+		{Tag: "machine-0-lxc-99"},
+		{Tag: "machine-1-lxc-99"},
+		{Tag: "machine-1-lxc-99-lxc-100"},
+	}}
+	result, err := provisioner.DistributionGroup(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(result, gc.DeepEquals, params.DistributionGroupResults{
+		Results: []params.DistributionGroupResult{
+			{Error: apiservertesting.ErrUnauthorized},
+			{Result: []instance.Id{}},
+			{Error: apiservertesting.ErrUnauthorized},
+			// only a machine agent for the container or its
+			// parent may access it.
+			{Error: apiservertesting.ErrUnauthorized},
+			{Error: apiservertesting.NotFoundError("machine 1/lxc/99")},
+			{Error: apiservertesting.ErrUnauthorized},
+		},
+	})
+}
+
 func (s *withoutStateServerSuite) TestConstraints(c *gc.C) {
 	// Add a machine with some constraints.
 	template := state.MachineTemplate{

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2014-04-02 11:35:49 +0000
+++ worker/provisioner/provisioner_task.go	2014-04-03 02:58:17 +0000
@@ -426,9 +426,10 @@
 		return err
 	}
 	inst, metadata, err := task.broker.StartInstance(environs.StartInstanceParams{
-		Constraints:   cons,
-		Tools:         possibleTools,
-		MachineConfig: machineConfig,
+		Constraints:       cons,
+		Tools:             possibleTools,
+		MachineConfig:     machineConfig,
+		DistributionGroup: machine.DistributionGroup,
 	})
 	if err != nil {
 		// Set the state to error, so the machine will be skipped next