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

Proposed by Ian Booth on 2013-11-13
Status: Merged
Approved by: Ian Booth on 2013-11-14
Approved revision: 2059
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 2013-11-13 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.
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

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/

2057. By Ian Booth on 2013-11-14

Merge trunk

2058. By Ian Booth on 2013-11-14

Code review tweaks, add test

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/

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/

2059. By Ian Booth on 2013-11-14

Add extra test

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
1=== modified file 'state/api/params/internal.go'
2--- state/api/params/internal.go 2013-10-03 13:13:25 +0000
3+++ state/api/params/internal.go 2013-11-14 23:58:54 +0000
4@@ -20,6 +20,19 @@
5 Entities []Entity
6 }
7
8+// AddSupportedContainers holds the arguments for making a AddSupportedContainers
9+// API call.
10+type AddSupportedContainers struct {
11+ Params []AddMachineSupportedContainers
12+}
13+
14+// AddMachineSupportedContainers holds the arguments for making an AddSupportedContainers call
15+// on a given machine.
16+type AddMachineSupportedContainers struct {
17+ MachineTag string
18+ ContainerTypes []instance.ContainerType
19+}
20+
21 // WatchContainer identifies a single container type within a machine.
22 type WatchContainer struct {
23 MachineTag string
24
25=== modified file 'state/api/provisioner/machine.go'
26--- state/api/provisioner/machine.go 2013-09-20 15:07:21 +0000
27+++ state/api/provisioner/machine.go 2013-11-14 23:58:54 +0000
28@@ -240,3 +240,25 @@
29 w := watcher.NewStringsWatcher(m.st.caller, result)
30 return w, nil
31 }
32+
33+// AddSupportedContainers updates the list of containers supported by this machine.
34+func (m *Machine) AddSupportedContainers(containerTypes ...instance.ContainerType) error {
35+ var results params.ErrorResults
36+ args := params.AddSupportedContainers{
37+ Params: []params.AddMachineSupportedContainers{
38+ {MachineTag: m.tag, ContainerTypes: containerTypes},
39+ },
40+ }
41+ err := m.st.caller.Call("Provisioner", "", "AddSupportedContainers", args, &results)
42+ if err != nil {
43+ return err
44+ }
45+ if len(results.Results) != 1 {
46+ return fmt.Errorf("expected one result, got %d", len(results.Results))
47+ }
48+ apiError := results.Results[0].Error
49+ if apiError != nil {
50+ return apiError
51+ }
52+ return nil
53+}
54
55=== modified file 'state/api/provisioner/provisioner_test.go'
56--- state/api/provisioner/provisioner_test.go 2013-11-07 09:09:55 +0000
57+++ state/api/provisioner/provisioner_test.go 2013-11-14 23:58:54 +0000
58@@ -404,3 +404,16 @@
59 c.Assert(stateTools.Version, gc.Equals, cur)
60 c.Assert(stateTools.URL, gc.Not(gc.Equals), "")
61 }
62+
63+func (s *provisionerSuite) TestAddSupportedContainers(c *gc.C) {
64+ apiMachine, err := s.provisioner.Machine(s.machine.Tag())
65+ c.Assert(err, gc.IsNil)
66+ err = apiMachine.AddSupportedContainers(instance.LXC, instance.KVM)
67+ c.Assert(err, gc.IsNil)
68+
69+ err = s.machine.Refresh()
70+ c.Assert(err, gc.IsNil)
71+ containers, ok := s.machine.SupportedContainers()
72+ c.Assert(ok, jc.IsTrue)
73+ c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
74+}
75
76=== modified file 'state/apiserver/provisioner/provisioner.go'
77--- state/apiserver/provisioner/provisioner.go 2013-11-06 13:36:47 +0000
78+++ state/apiserver/provisioner/provisioner.go 2013-11-14 23:58:54 +0000
79@@ -184,6 +184,31 @@
80 return result, nil
81 }
82
83+// AddSupportedContainers updates the list of containers supported by the machines passed in args.
84+func (p *ProvisionerAPI) AddSupportedContainers(
85+ args params.AddSupportedContainers) (params.ErrorResults, error) {
86+
87+ result := params.ErrorResults{
88+ Results: make([]params.ErrorResult, len(args.Params)),
89+ }
90+ for i, arg := range args.Params {
91+ canAccess, err := p.getAuthFunc()
92+ if err != nil {
93+ return result, err
94+ }
95+ machine, err := p.getMachine(canAccess, arg.MachineTag)
96+ if err != nil {
97+ result.Results[i].Error = common.ServerError(err)
98+ continue
99+ }
100+ err = machine.AddSupportedContainers(arg.ContainerTypes)
101+ if err != nil {
102+ result.Results[i].Error = common.ServerError(err)
103+ }
104+ }
105+ return result, nil
106+}
107+
108 // ContainerConfig returns information from the environment config that are
109 // needed for container cloud-init.
110 func (p *ProvisionerAPI) ContainerConfig() (params.ContainerConfig, error) {
111
112=== modified file 'state/apiserver/provisioner/provisioner_test.go'
113--- state/apiserver/provisioner/provisioner_test.go 2013-11-07 09:09:55 +0000
114+++ state/apiserver/provisioner/provisioner_test.go 2013-11-14 23:58:54 +0000
115@@ -737,3 +737,68 @@
116 c.Check(agentTools.URL, gc.Not(gc.Equals), "")
117 c.Check(agentTools.Version, gc.DeepEquals, cur)
118 }
119+
120+func (s *provisionerSuite) TestAddSupportedContainers(c *gc.C) {
121+ args := params.AddSupportedContainers{
122+ Params: []params.AddMachineSupportedContainers{
123+ {
124+ MachineTag: "machine-0",
125+ ContainerTypes: []instance.ContainerType{instance.LXC},
126+ },
127+ {
128+ MachineTag: "machine-1",
129+ ContainerTypes: []instance.ContainerType{instance.LXC, instance.KVM},
130+ },
131+ },
132+ }
133+ results, err := s.provisioner.AddSupportedContainers(args)
134+ c.Assert(err, gc.IsNil)
135+ c.Assert(results.Results, gc.HasLen, 2)
136+ for _, result := range results.Results {
137+ c.Assert(result.Error, gc.IsNil)
138+ }
139+ m0, err := s.State.Machine("0")
140+ c.Assert(err, gc.IsNil)
141+ containers, ok := m0.SupportedContainers()
142+ c.Assert(ok, jc.IsTrue)
143+ c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC})
144+ m1, err := s.State.Machine("1")
145+ c.Assert(err, gc.IsNil)
146+ containers, ok = m1.SupportedContainers()
147+ c.Assert(ok, jc.IsTrue)
148+ c.Assert(containers, gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
149+}
150+
151+func (s *provisionerSuite) TestAddSupportedContainersPermissions(c *gc.C) {
152+ // Login as a machine agent for machine 0.
153+ anAuthorizer := s.authorizer
154+ anAuthorizer.MachineAgent = true
155+ anAuthorizer.Manager = false
156+ anAuthorizer.Tag = s.machines[0].Tag()
157+ aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, anAuthorizer)
158+ c.Assert(err, gc.IsNil)
159+ c.Assert(aProvisioner, gc.NotNil)
160+
161+ args := params.AddSupportedContainers{
162+ Params: []params.AddMachineSupportedContainers{{
163+ MachineTag: "machine-0",
164+ ContainerTypes: []instance.ContainerType{instance.LXC},
165+ }, {
166+ MachineTag: "machine-1",
167+ ContainerTypes: []instance.ContainerType{instance.LXC},
168+ }, {
169+ MachineTag: "machine-42",
170+ ContainerTypes: []instance.ContainerType{instance.LXC},
171+ },
172+ },
173+ }
174+ // Only machine 0 can have it's containers updated.
175+ results, err := aProvisioner.AddSupportedContainers(args)
176+ c.Assert(results, gc.DeepEquals, params.ErrorResults{
177+ Results: []params.ErrorResult{
178+ {Error: nil},
179+ {Error: apiservertesting.ErrUnauthorized},
180+ {Error: apiservertesting.ErrUnauthorized},
181+ },
182+ })
183+}
184
185=== modified file 'state/machine.go'
186--- state/machine.go 2013-11-12 11:14:27 +0000
187+++ state/machine.go 2013-11-14 23:58:54 +0000
188@@ -823,12 +823,11 @@
189
190 // SupportsNoContainers records the fact that this machine doesn't support any containers.
191 func (m *Machine) SupportsNoContainers() (err error) {
192- sameDoc := D{{"txn-revno", m.doc.TxnRevno}}
193 ops := []txn.Op{
194 {
195 C: m.st.machines.Name,
196 Id: m.doc.Id,
197- Assert: append(notDeadDoc, sameDoc...),
198+ Assert: notDeadDoc,
199 Update: D{
200 {"$set", D{
201 {"supportedcontainers", []instance.ContainerType{}},
202@@ -855,12 +854,11 @@
203 return fmt.Errorf("%q is not a valid container type", container)
204 }
205 }
206- sameDoc := D{{"txn-revno", m.doc.TxnRevno}}
207 ops := []txn.Op{
208 {
209 C: m.st.machines.Name,
210 Id: m.doc.Id,
211- Assert: append(notDeadDoc, sameDoc...),
212+ Assert: notDeadDoc,
213 Update: D{
214 {"$addToSet", D{{"supportedcontainers", D{{"$each", containers}}}}},
215 {"$set", D{{"supportedcontainersknown", true}}},

Subscribers

People subscribed via source and target branches

to status/vote changes: