juju-core

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

  1. provisioner-api-supported-containers
  2. Merge into trunk
Proposed by Ian Booth
Status: Merged
Approved by: Ian Booth
Approved revision: no longer in the source branch.
Merged at revision: 2064
Proposed branch: lp:~wallyworld/juju-core/provisioner-api-supported-containers
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 215 lines (+140/-4)
6 files modified
state/api/params/internal.go (+13/-0)
state/api/provisioner/machine.go (+22/-0)
state/api/provisioner/provisioner_test.go (+13/-0)
state/apiserver/provisioner/provisioner.go (+25/-0)
state/apiserver/provisioner/provisioner_test.go (+65/-0)
state/machine.go (+2/-4)
To merge this branch: bzr merge lp:~wallyworld/juju-core/provisioner-api-supported-containers
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+194982@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

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

Revision history for this message
John A Meinel (jameinel) wrote :

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/

Revision history for this message
Ian Booth (wallyworld) wrote :

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/

Revision history for this message
John A Meinel (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",
    ...
},

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

Revision history for this message
Ian Booth (wallyworld) wrote :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
20 Entities []Entity20 Entities []Entity
21}21}
2222
23// AddSupportedContainers holds the arguments for making a AddSupportedContainers
24// API call.
25type AddSupportedContainers struct {
26 Params []AddMachineSupportedContainers
27}
28
29// AddMachineSupportedContainers holds the arguments for making an AddSupportedContainers call
30// on a given machine.
31type AddMachineSupportedContainers struct {
32 MachineTag string
33 ContainerTypes []instance.ContainerType
34}
35
23// WatchContainer identifies a single container type within a machine.36// WatchContainer identifies a single container type within a machine.
24type WatchContainer struct {37type WatchContainer struct {
25 MachineTag string38 MachineTag string
2639
=== 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 @@
240 w := watcher.NewStringsWatcher(m.st.caller, result)240 w := watcher.NewStringsWatcher(m.st.caller, result)
241 return w, nil241 return w, nil
242}242}
243
244// AddSupportedContainers updates the list of containers supported by this machine.
245func (m *Machine) AddSupportedContainers(containerTypes ...instance.ContainerType) error {
246 var results params.ErrorResults
247 args := params.AddSupportedContainers{
248 Params: []params.AddMachineSupportedContainers{
249 {MachineTag: m.tag, ContainerTypes: containerTypes},
250 },
251 }
252 err := m.st.caller.Call("Provisioner", "", "AddSupportedContainers", args, &results)
253 if err != nil {
254 return err
255 }
256 if len(results.Results) != 1 {
257 return fmt.Errorf("expected one result, got %d", len(results.Results))
258 }
259 apiError := results.Results[0].Error
260 if apiError != nil {
261 return apiError
262 }
263 return nil
264}
243265
=== 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 @@
404 c.Assert(stateTools.Version, gc.Equals, cur)404 c.Assert(stateTools.Version, gc.Equals, cur)
405 c.Assert(stateTools.URL, gc.Not(gc.Equals), "")405 c.Assert(stateTools.URL, gc.Not(gc.Equals), "")
406}406}
407
408func (s *provisionerSuite) TestAddSupportedContainers(c *gc.C) {
409 apiMachine, err := s.provisioner.Machine(s.machine.Tag())
410 c.Assert(err, gc.IsNil)
411 err = apiMachine.AddSupportedContainers(instance.LXC, instance.KVM)
412 c.Assert(err, gc.IsNil)
413
414 err = s.machine.Refresh()
415 c.Assert(err, gc.IsNil)
416 containers, ok := s.machine.SupportedContainers()
417 c.Assert(ok, jc.IsTrue)
418 c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
419}
407420
=== 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 @@
184 return result, nil184 return result, nil
185}185}
186186
187// AddSupportedContainers updates the list of containers supported by the machines passed in args.
188func (p *ProvisionerAPI) AddSupportedContainers(
189 args params.AddSupportedContainers) (params.ErrorResults, error) {
190
191 result := params.ErrorResults{
192 Results: make([]params.ErrorResult, len(args.Params)),
193 }
194 for i, arg := range args.Params {
195 canAccess, err := p.getAuthFunc()
196 if err != nil {
197 return result, err
198 }
199 machine, err := p.getMachine(canAccess, arg.MachineTag)
200 if err != nil {
201 result.Results[i].Error = common.ServerError(err)
202 continue
203 }
204 err = machine.AddSupportedContainers(arg.ContainerTypes)
205 if err != nil {
206 result.Results[i].Error = common.ServerError(err)
207 }
208 }
209 return result, nil
210}
211
187// ContainerConfig returns information from the environment config that are212// ContainerConfig returns information from the environment config that are
188// needed for container cloud-init.213// needed for container cloud-init.
189func (p *ProvisionerAPI) ContainerConfig() (params.ContainerConfig, error) {214func (p *ProvisionerAPI) ContainerConfig() (params.ContainerConfig, error) {
190215
=== 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 @@
737 c.Check(agentTools.URL, gc.Not(gc.Equals), "")737 c.Check(agentTools.URL, gc.Not(gc.Equals), "")
738 c.Check(agentTools.Version, gc.DeepEquals, cur)738 c.Check(agentTools.Version, gc.DeepEquals, cur)
739}739}
740
741func (s *provisionerSuite) TestAddSupportedContainers(c *gc.C) {
742 args := params.AddSupportedContainers{
743 Params: []params.AddMachineSupportedContainers{
744 {
745 MachineTag: "machine-0",
746 ContainerTypes: []instance.ContainerType{instance.LXC},
747 },
748 {
749 MachineTag: "machine-1",
750 ContainerTypes: []instance.ContainerType{instance.LXC, instance.KVM},
751 },
752 },
753 }
754 results, err := s.provisioner.AddSupportedContainers(args)
755 c.Assert(err, gc.IsNil)
756 c.Assert(results.Results, gc.HasLen, 2)
757 for _, result := range results.Results {
758 c.Assert(result.Error, gc.IsNil)
759 }
760 m0, err := s.State.Machine("0")
761 c.Assert(err, gc.IsNil)
762 containers, ok := m0.SupportedContainers()
763 c.Assert(ok, jc.IsTrue)
764 c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC})
765 m1, err := s.State.Machine("1")
766 c.Assert(err, gc.IsNil)
767 containers, ok = m1.SupportedContainers()
768 c.Assert(ok, jc.IsTrue)
769 c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
770}
771
772func (s *provisionerSuite) TestAddSupportedContainersPermissions(c *gc.C) {
773 // Login as a machine agent for machine 0.
774 anAuthorizer := s.authorizer
775 anAuthorizer.MachineAgent = true
776 anAuthorizer.Manager = false
777 anAuthorizer.Tag = s.machines[0].Tag()
778 aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, anAuthorizer)
779 c.Assert(err, gc.IsNil)
780 c.Assert(aProvisioner, gc.NotNil)
781
782 args := params.AddSupportedContainers{
783 Params: []params.AddMachineSupportedContainers{{
784 MachineTag: "machine-0",
785 ContainerTypes: []instance.ContainerType{instance.LXC},
786 }, {
787 MachineTag: "machine-1",
788 ContainerTypes: []instance.ContainerType{instance.LXC},
789 }, {
790 MachineTag: "machine-42",
791 ContainerTypes: []instance.ContainerType{instance.LXC},
792 },
793 },
794 }
795 // Only machine 0 can have it's containers updated.
796 results, err := aProvisioner.AddSupportedContainers(args)
797 c.Assert(results, gc.DeepEquals, params.ErrorResults{
798 Results: []params.ErrorResult{
799 {Error: nil},
800 {Error: apiservertesting.ErrUnauthorized},
801 {Error: apiservertesting.ErrUnauthorized},
802 },
803 })
804}
740805
=== 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 @@
823823
824// SupportsNoContainers records the fact that this machine doesn't support any containers.824// SupportsNoContainers records the fact that this machine doesn't support any containers.
825func (m *Machine) SupportsNoContainers() (err error) {825func (m *Machine) SupportsNoContainers() (err error) {
826 sameDoc := D{{"txn-revno", m.doc.TxnRevno}}
827 ops := []txn.Op{826 ops := []txn.Op{
828 {827 {
829 C: m.st.machines.Name,828 C: m.st.machines.Name,
830 Id: m.doc.Id,829 Id: m.doc.Id,
831 Assert: append(notDeadDoc, sameDoc...),830 Assert: notDeadDoc,
832 Update: D{831 Update: D{
833 {"$set", D{832 {"$set", D{
834 {"supportedcontainers", []instance.ContainerType{}},833 {"supportedcontainers", []instance.ContainerType{}},
@@ -855,12 +854,11 @@
855 return fmt.Errorf("%q is not a valid container type", container)854 return fmt.Errorf("%q is not a valid container type", container)
856 }855 }
857 }856 }
858 sameDoc := D{{"txn-revno", m.doc.TxnRevno}}
859 ops := []txn.Op{857 ops := []txn.Op{
860 {858 {
861 C: m.st.machines.Name,859 C: m.st.machines.Name,
862 Id: m.doc.Id,860 Id: m.doc.Id,
863 Assert: append(notDeadDoc, sameDoc...),861 Assert: notDeadDoc,
864 Update: D{862 Update: D{
865 {"$addToSet", D{{"supportedcontainers", D{{"$each", containers}}}}},863 {"$addToSet", D{{"supportedcontainers", D{{"$each", containers}}}}},
866 {"$set", D{{"supportedcontainersknown", true}}},864 {"$set", D{{"supportedcontainersknown", true}}},

Subscribers

People subscribed via source and target branches

to status/vote changes: