Merge lp:~wallyworld/juju-core/provisioner-api-supported-containers into lp:~go-bot/juju-core/trunk

[Ian Booth]

AddSupportedContainers on provisioner machine api

The AddSupportedContainers api on the provisioner.Machine
will be used by future changes to machine agent initialisation.

A change was also made to the server side implementation so that
the machine doc txn-revno is no longer checked. It was causing issues
with multi-threaded tests where different fields in the machine doc
were being updated. The machine agent initialisation thread is the
only one that updates the supported containers set.

https://codereview.appspot.com/25480047/

AddSupportedContainers on provisioner machine api

The AddSupportedContainers api on the provisioner.Machine
will be used by future changes to machine agent initialisation.

A change was also made to the server side implementation so that
the machine doc txn-revno is no longer checked. It was causing issues
with multi-threaded tests where different fields in the machine doc
were being updated. The machine agent initialisation thread is the
only one that updates the supported containers set.

https://codereview.appspot.com/25480047/

[Ian Booth (wallyworld)]

Reviewers: mp+194982_code.launchpad.net,

Message:
Please take a look.

Description:
AddSupportedContainers on provisioner machine api

The AddSupportedContainers api on the provisioner.Machine
will be used by future changes to machine agent initialisation.

A change was also made to the server side implementation so that
the machine doc txn-revno is no longer checked. It was causing issues
with multi-threaded tests where different fields in the machine doc
were being updated. The machine agent initialisation thread is the
only one that updates the supported containers set.

https://code.launchpad.net/~wallyworld/juju-core/provisioner-api-supported-containers/+merge/194982

(do not edit description out of merge proposal)

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

Affected files (+113, -4 lines):
   A [revision details]
   M state/api/params/internal.go
   M state/api/provisioner/machine.go
   M state/api/provisioner/provisioner_test.go
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_test.go
   M state/machine.go

[John A Meinel (jameinel)]

I really like where this is going, but I think we're missing a couple
tests.

We should have a test that shows when Permission denied is triggered.

And I think we want to show what happens after api.Set* from the API
point of view.

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/machine.go#newcode246
state/api/provisioner/machine.go:246: var results
params.AddSupportedContainersResults
I just noticed that a few other places William changed the api.Client
objects to use

DoSomething([]list) into
DoSomething(...list)

I'm not sure which is more consistent in this case. If you want to look
around at other Machine functions and see if a pattern shows up. I'm
reasonably happy either way.

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

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/provisioner_test.go#newcode418
state/api/provisioner/provisioner_test.go:418: c.Assert(containers,
gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
I like that you assert a refreshed State machine object notices the new
containers.
But you aren't actually asserting the api object itself sees the change.

So probably add one more assertion.

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

https://codereview.appspot.com/25480047/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode770
state/apiserver/provisioner/provisioner_test.go:770: }
We're missing a couple tests here about the permission model of the new
code.

We should be trying to change values from an object that isn't allowed.

It may be that anything that can get to the Provisioner api can set
anything that it would like. But I would *think* that only the
Provisioner running on machine-0 could set the container types for
machine-0. (machine-1 shouldn't be able to see or set the container
types for another arbitrary machine.)

https://codereview.appspot.com/25480047/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/machine.go#newcode246
state/api/provisioner/machine.go:246: var results
params.AddSupportedContainersResults
On 2013/11/13 09:27:52, jameinel wrote:
> I just noticed that a few other places William changed the api.Client
objects to
> use

> DoSomething([]list) into
> DoSomething(...list)

> I'm not sure which is more consistent in this case. If you want to
look around
> at other Machine functions and see if a pattern shows up. I'm
reasonably happy
> either way.

Several other api methods use ... so I changed to that.

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

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/provisioner_test.go#newcode418
state/api/provisioner/provisioner_test.go:418: c.Assert(containers,
gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
On 2013/11/13 09:27:52, jameinel wrote:
> I like that you assert a refreshed State machine object notices the
new
> containers.
> But you aren't actually asserting the api object itself sees the
change.

> So probably add one more assertion.

The API doesn't provider a getter for supported containers since it
doesn't need it (the API is used by the machine agent to set the
supported containers). So nothing to test.

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

https://codereview.appspot.com/25480047/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode770
state/apiserver/provisioner/provisioner_test.go:770: }
On 2013/11/13 09:27:52, jameinel wrote:
> We're missing a couple tests here about the permission model of the
new code.

> We should be trying to change values from an object that isn't
allowed.

> It may be that anything that can get to the Provisioner api can set
anything
> that it would like. But I would *think* that only the Provisioner
running on
> machine-0 could set the container types for machine-0. (machine-1
shouldn't be
> able to see or set the container types for another arbitrary machine.)

Done.

https://codereview.appspot.com/25480047/

[John A Meinel (jameinel)]

LGTM

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

https://codereview.appspot.com/25480047/diff/20001/state/apiserver/provisioner/provisioner_test.go#newcode801
state/apiserver/provisioner/provisioner_test.go:801: })
The only other test I've seen here is a machine that doesn't exist still
shows up as ErrUnauthorized rather than ErrNotFound.
I think you can do that with just adding

{
    MachineTag: "machine-42",
    ...
},

https://codereview.appspot.com/25480047/

[Ian Booth (wallyworld)]

On 2013/11/14 10:38:00, jameinel wrote:
> LGTM

https://codereview.appspot.com/25480047/diff/20001/state/apiserver/provisioner/provisioner_test.go
> File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/25480047/diff/20001/state/apiserver/provisioner/provisioner_test.go#newcode801
> state/apiserver/provisioner/provisioner_test.go:801: })
> The only other test I've seen here is a machine that doesn't exist
still shows
> up as ErrUnauthorized rather than ErrNotFound.
> I think you can do that with just adding

> {
> MachineTag: "machine-42",
> ...
> },

Done

https://codereview.appspot.com/25480047/

=== modified file 'state/api/params/internal.go'
--- state/api/params/internal.go	2013-10-03 13:13:25 +0000
+++ state/api/params/internal.go	2013-11-14 23:58:54 +0000
@@ -20,6 +20,19 @@
 	Entities []Entity
 }
 
+// AddSupportedContainers holds the arguments for making a AddSupportedContainers
+// API call.
+type AddSupportedContainers struct {
+	Params []AddMachineSupportedContainers
+}
+
+// AddMachineSupportedContainers holds the arguments for making an AddSupportedContainers call
+// on a given machine.
+type AddMachineSupportedContainers struct {
+	MachineTag     string
+	ContainerTypes []instance.ContainerType
+}
+
 // WatchContainer identifies a single container type within a machine.
 type WatchContainer struct {
 	MachineTag    string

=== modified file 'state/api/provisioner/machine.go'
--- state/api/provisioner/machine.go	2013-09-20 15:07:21 +0000
+++ state/api/provisioner/machine.go	2013-11-14 23:58:54 +0000
@@ -240,3 +240,25 @@
 	w := watcher.NewStringsWatcher(m.st.caller, result)
 	return w, nil
 }
+
+// AddSupportedContainers updates the list of containers supported by this machine.
+func (m *Machine) AddSupportedContainers(containerTypes ...instance.ContainerType) error {
+	var results params.ErrorResults
+	args := params.AddSupportedContainers{
+		Params: []params.AddMachineSupportedContainers{
+			{MachineTag: m.tag, ContainerTypes: containerTypes},
+		},
+	}
+	err := m.st.caller.Call("Provisioner", "", "AddSupportedContainers", args, &results)
+	if err != nil {
+		return err
+	}
+	if len(results.Results) != 1 {
+		return fmt.Errorf("expected one result, got %d", len(results.Results))
+	}
+	apiError := results.Results[0].Error
+	if apiError != nil {
+		return apiError
+	}
+	return nil
+}

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2013-11-07 09:09:55 +0000
+++ state/api/provisioner/provisioner_test.go	2013-11-14 23:58:54 +0000
@@ -404,3 +404,16 @@
 	c.Assert(stateTools.Version, gc.Equals, cur)
 	c.Assert(stateTools.URL, gc.Not(gc.Equals), "")
 }
+
+func (s *provisionerSuite) TestAddSupportedContainers(c *gc.C) {
+	apiMachine, err := s.provisioner.Machine(s.machine.Tag())
+	c.Assert(err, gc.IsNil)
+	err = apiMachine.AddSupportedContainers(instance.LXC, instance.KVM)
+	c.Assert(err, gc.IsNil)
+
+	err = s.machine.Refresh()
+	c.Assert(err, gc.IsNil)
+	containers, ok := s.machine.SupportedContainers()
+	c.Assert(ok, jc.IsTrue)
+	c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
+}

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2013-11-06 13:36:47 +0000
+++ state/apiserver/provisioner/provisioner.go	2013-11-14 23:58:54 +0000
@@ -184,6 +184,31 @@
 	return result, nil
 }
 
+// AddSupportedContainers updates the list of containers supported by the machines passed in args.
+func (p *ProvisionerAPI) AddSupportedContainers(
+	args params.AddSupportedContainers) (params.ErrorResults, error) {
+
+	result := params.ErrorResults{
+		Results: make([]params.ErrorResult, len(args.Params)),
+	}
+	for i, arg := range args.Params {
+		canAccess, err := p.getAuthFunc()
+		if err != nil {
+			return result, err
+		}
+		machine, err := p.getMachine(canAccess, arg.MachineTag)
+		if err != nil {
+			result.Results[i].Error = common.ServerError(err)
+			continue
+		}
+		err = machine.AddSupportedContainers(arg.ContainerTypes)
+		if err != nil {
+			result.Results[i].Error = common.ServerError(err)
+		}
+	}
+	return result, nil
+}
+
 // ContainerConfig returns information from the environment config that are
 // needed for container cloud-init.
 func (p *ProvisionerAPI) ContainerConfig() (params.ContainerConfig, error) {

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2013-11-07 09:09:55 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2013-11-14 23:58:54 +0000
@@ -737,3 +737,68 @@
 	c.Check(agentTools.URL, gc.Not(gc.Equals), "")
 	c.Check(agentTools.Version, gc.DeepEquals, cur)
 }
+
+func (s *provisionerSuite) TestAddSupportedContainers(c *gc.C) {
+	args := params.AddSupportedContainers{
+		Params: []params.AddMachineSupportedContainers{
+			{
+				MachineTag:     "machine-0",
+				ContainerTypes: []instance.ContainerType{instance.LXC},
+			},
+			{
+				MachineTag:     "machine-1",
+				ContainerTypes: []instance.ContainerType{instance.LXC, instance.KVM},
+			},
+		},
+	}
+	results, err := s.provisioner.AddSupportedContainers(args)
+	c.Assert(err, gc.IsNil)
+	c.Assert(results.Results, gc.HasLen, 2)
+	for _, result := range results.Results {
+		c.Assert(result.Error, gc.IsNil)
+	}
+	m0, err := s.State.Machine("0")
+	c.Assert(err, gc.IsNil)
+	containers, ok := m0.SupportedContainers()
+	c.Assert(ok, jc.IsTrue)
+	c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC})
+	m1, err := s.State.Machine("1")
+	c.Assert(err, gc.IsNil)
+	containers, ok = m1.SupportedContainers()
+	c.Assert(ok, jc.IsTrue)
+	c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
+}
+
+func (s *provisionerSuite) TestAddSupportedContainersPermissions(c *gc.C) {
+	// Login as a machine agent for machine 0.
+	anAuthorizer := s.authorizer
+	anAuthorizer.MachineAgent = true
+	anAuthorizer.Manager = false
+	anAuthorizer.Tag = s.machines[0].Tag()
+	aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, anAuthorizer)
+	c.Assert(err, gc.IsNil)
+	c.Assert(aProvisioner, gc.NotNil)
+
+	args := params.AddSupportedContainers{
+		Params: []params.AddMachineSupportedContainers{{
+			MachineTag:     "machine-0",
+			ContainerTypes: []instance.ContainerType{instance.LXC},
+		}, {
+			MachineTag:     "machine-1",
+			ContainerTypes: []instance.ContainerType{instance.LXC},
+		}, {
+			MachineTag:     "machine-42",
+			ContainerTypes: []instance.ContainerType{instance.LXC},
+		},
+		},
+	}
+	// Only machine 0 can have it's containers updated.
+	results, err := aProvisioner.AddSupportedContainers(args)
+	c.Assert(results, gc.DeepEquals, params.ErrorResults{
+		Results: []params.ErrorResult{
+			{Error: nil},
+			{Error: apiservertesting.ErrUnauthorized},
+			{Error: apiservertesting.ErrUnauthorized},
+		},
+	})
+}

=== modified file 'state/machine.go'
--- state/machine.go	2013-11-12 11:14:27 +0000
+++ state/machine.go	2013-11-14 23:58:54 +0000
@@ -823,12 +823,11 @@
 
 // SupportsNoContainers records the fact that this machine doesn't support any containers.
 func (m *Machine) SupportsNoContainers() (err error) {
-	sameDoc := D{{"txn-revno", m.doc.TxnRevno}}
 	ops := []txn.Op{
 		{
 			C:      m.st.machines.Name,
 			Id:     m.doc.Id,
-			Assert: append(notDeadDoc, sameDoc...),
+			Assert: notDeadDoc,
 			Update: D{
 				{"$set", D{
 					{"supportedcontainers", []instance.ContainerType{}},
@@ -855,12 +854,11 @@
 			return fmt.Errorf("%q is not a valid container type", container)
 		}
 	}
-	sameDoc := D{{"txn-revno", m.doc.TxnRevno}}
 	ops := []txn.Op{
 		{
 			C:      m.st.machines.Name,
 			Id:     m.doc.Id,
-			Assert: append(notDeadDoc, sameDoc...),
+			Assert: notDeadDoc,
 			Update: D{
 				{"$addToSet", D{{"supportedcontainers", D{{"$each", containers}}}}},
 				{"$set", D{{"supportedcontainersknown", true}}},