Merge lp:~wallyworld/juju-core/machineswithtransienterrors-api into lp:~go-bot/juju-core/trunk
- machineswithtransienterrors-api
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Ian Booth |
Approved revision: | no longer in the source branch. |
Merged at revision: | 2483 |
Proposed branch: | lp:~wallyworld/juju-core/machineswithtransienterrors-api |
Merge into: | lp:~go-bot/juju-core/trunk |
Diff against target: |
707 lines (+361/-34) 17 files modified
state/api/client.go (+14/-0) state/api/machiner/machine.go (+1/-1) state/api/params/internal.go (+7/-4) state/api/provisioner/machine.go (+3/-3) state/api/provisioner/provisioner.go (+23/-0) state/api/provisioner/provisioner_test.go (+50/-2) state/api/uniter/unit.go (+1/-1) state/apiserver/client/client.go (+18/-4) state/apiserver/client/client_test.go (+15/-0) state/apiserver/common/setstatus.go (+50/-0) state/apiserver/common/setstatus_test.go (+66/-2) state/apiserver/machine/machiner_test.go (+1/-1) state/apiserver/provisioner/provisioner.go (+36/-1) state/apiserver/provisioner/provisioner_test.go (+68/-13) state/apiserver/uniter/uniter_test.go (+1/-1) state/interface.go (+6/-0) worker/provisioner/provisioner_task.go (+1/-1) |
To merge this branch: | bzr merge lp:~wallyworld/juju-core/machineswithtransienterrors-api |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email:
|
Commit message
New APIs for provisioner retries.
Add new APIs to support allowing the provisioner to retry
machines with errors. A machine in error status can be
retried if its status data has transient=true.
1. MachinesWithTra
to find machines which can be retried.
2. ResolveProvisio
the user to manually mark a machine as resolved.
Description of the change
New APIs for provisioner retries.
Add new APIs to support allowing the provisioner to retry
machines with errors. A machine in error status can be
retried if its status data has transient=true.
1. MachinesWithTra
to find machines which can be retried.
2. ResolveProvisio
the user to manually mark a machine as resolved.
Existing param structs - SetEntityStatus and SetStatus - were
moved from internal.go to params.go and renamed since they
are now used by the client.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Andrew Wilkins (axwalk) wrote : | # |
https:/
File state/api/client.go (right):
https:/
state/api/
Data: params.
This is a bit too leaky. The client API should be taking (machine) tags
and setting the transient=true status on the server side, otherwise
we're stuck with this implementation detail forever.
https:/
File state/apiserver
https:/
state/apiserver
canAccessFunc(
Should do the access check first to avoid an unnecessary mongo trip.
https:/
state/apiserver
!canAccess {
This is confusing, but I think you just want
if err == nil && !canAccess {
err = common.ErrPerm
}
... right?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
Please take a look.
https:/
File state/api/client.go (right):
https:/
state/api/
Data: params.
On 2014/03/26 02:54:08, axw wrote:
> This is a bit too leaky. The client API should be taking (machine)
tags and
> setting the transient=true status on the server side, otherwise we're
stuck with
> this implementation detail forever.
I didn't want to encode the ResolveProvisio
case we decided not to go with it long term, and thought a generic
UpdateMachineStatus API would be generically useful. The implementation
detail is behind a client fasçade. But it is leaky I agree in terms of
being stuck supporting this client version. I'll change it.
https:/
File state/apiserver
https:/
state/apiserver
canAccessFunc(
On 2014/03/26 02:54:08, axw wrote:
> Should do the access check first to avoid an unnecessary mongo trip.
Done.
https:/
state/apiserver
!canAccess {
On 2014/03/26 02:54:08, axw wrote:
> This is confusing, but I think you just want
> if err == nil && !canAccess {
> err = common.ErrPerm
> }
> ... right?
Changed to due above refactoring
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Andrew Wilkins (axwalk) wrote : | # |
Thanks for the Client change. Sorry, I missed a couple of things the
first time round.
https:/
File state/api/
https:/
state/api/
gc.ErrorMatches, "permission denied")
This seems like a weird situation to be in. If there's a machine that
the caller is not allowed to read, I think it should just be excluded.
https:/
File state/apiserver
https:/
state/apiserver
nil {
err == nil implies canAccess (otherwise it'd be ErrPerm), so just "if
err == nil"
https:/
state/apiserver
result.
There's a potential type assertion panic here, if something sets
transient to something other than a bool. Not sure if we want to be
bothered working around that.
https:/
state/apiserver
It doesn't seem right to me that MachinesWithTra
returning errors for individual machines. Machine that are inaccessible
simply shouldn't show up, I think. The caller isn't asking about them
specifically.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
Please take a look.
https:/
File state/api/
https:/
state/api/
gc.ErrorMatches, "permission denied")
On 2014/03/26 04:57:01, axw wrote:
> This seems like a weird situation to be in. If there's a machine that
the caller
> is not allowed to read, I think it should just be excluded.
Done.
https:/
File state/apiserver
https:/
state/apiserver
nil {
On 2014/03/26 04:57:01, axw wrote:
> err == nil implies canAccess (otherwise it'd be ErrPerm), so just "if
err ==
> nil"
Done.
https:/
state/apiserver
result.
On 2014/03/26 04:57:01, axw wrote:
> There's a potential type assertion panic here, if something sets
transient to
> something other than a bool. Not sure if we want to be bothered
working around
> that.
I thought about it but forgot. Fixed.
https:/
state/apiserver
On 2014/03/26 04:57:01, axw wrote:
> It doesn't seem right to me that MachinesWithTra
returning
> errors for individual machines. Machine that are inaccessible simply
shouldn't
> show up, I think. The caller isn't asking about them specifically.
Hmmmm. I think that's a valid point. I've changed it.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Andrew Wilkins (axwalk) wrote : | # |
On 2014/03/26 05:17:59, wallyworld wrote:
> Please take a look.
https:/
> File state/api/
https:/
> state/api/
> gc.ErrorMatches, "permission denied")
> On 2014/03/26 04:57:01, axw wrote:
> > This seems like a weird situation to be in. If there's a machine
that the
> caller
> > is not allowed to read, I think it should just be excluded.
> Done.
https:/
> File state/apiserver
https:/
> state/apiserver
nil {
> On 2014/03/26 04:57:01, axw wrote:
> > err == nil implies canAccess (otherwise it'd be ErrPerm), so just
"if err ==
> > nil"
> Done.
https:/
> state/apiserver
> result.
> On 2014/03/26 04:57:01, axw wrote:
> > There's a potential type assertion panic here, if something sets
transient to
> > something other than a bool. Not sure if we want to be bothered
working around
> > that.
> I thought about it but forgot. Fixed.
https:/
> state/apiserver
> On 2014/03/26 04:57:01, axw wrote:
> > It doesn't seem right to me that MachinesWithTra
be
> returning
> > errors for individual machines. Machine that are inaccessible simply
shouldn't
> > show up, I think. The caller isn't asking about them specifically.
> Hmmmm. I think that's a valid point. I've changed it.
Thanks, LGTM
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~wallyworld/juju-core/machineswithtransienterrors-api into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
Preview Diff
1 | === modified file 'state/api/client.go' |
2 | --- state/api/client.go 2014-03-24 13:27:29 +0000 |
3 | +++ state/api/client.go 2014-03-26 05:17:40 +0000 |
4 | @@ -138,6 +138,20 @@ |
5 | return c.call("Resolved", p, nil) |
6 | } |
7 | |
8 | +// ResolveProvisioningError updates the provisioning status of a machine allowing the |
9 | +// provisioner to retry. |
10 | +func (c *Client) ResolveProvisioningError(machine string) error { |
11 | + p := params.Entities{ |
12 | + Entities: []params.Entity{{Tag: machine}}, |
13 | + } |
14 | + var results params.ErrorResults |
15 | + err := c.st.Call("Client", "", "ResolveProvisioningError", p, &results) |
16 | + if err != nil { |
17 | + return err |
18 | + } |
19 | + return results.OneError() |
20 | +} |
21 | + |
22 | // PublicAddress returns the public address of the specified |
23 | // machine or unit. |
24 | func (c *Client) PublicAddress(target string) (string, error) { |
25 | |
26 | === modified file 'state/api/machiner/machine.go' |
27 | --- state/api/machiner/machine.go 2014-03-24 13:44:31 +0000 |
28 | +++ state/api/machiner/machine.go 2014-03-26 05:17:40 +0000 |
29 | @@ -42,7 +42,7 @@ |
30 | func (m *Machine) SetStatus(status params.Status, info string, data params.StatusData) error { |
31 | var result params.ErrorResults |
32 | args := params.SetStatus{ |
33 | - Entities: []params.SetEntityStatus{ |
34 | + Entities: []params.EntityStatus{ |
35 | {Tag: m.tag, Status: status, Info: info, Data: data}, |
36 | }, |
37 | } |
38 | |
39 | === modified file 'state/api/params/internal.go' |
40 | --- state/api/params/internal.go 2014-03-21 11:52:30 +0000 |
41 | +++ state/api/params/internal.go 2014-03-26 05:17:40 +0000 |
42 | @@ -315,25 +315,28 @@ |
43 | Machines []MachineSetProvisioned |
44 | } |
45 | |
46 | -// SetEntityStatus holds an entity tag, status and extra info. |
47 | -type SetEntityStatus struct { |
48 | +// EntityStatus holds an entity tag, status and extra info. |
49 | +type EntityStatus struct { |
50 | Tag string |
51 | Status Status |
52 | Info string |
53 | Data StatusData |
54 | } |
55 | |
56 | -// SetStatus holds the parameters for making a SetStatus call. |
57 | +// SetStatus holds the parameters for making a SetStatus/UpdateStatus call. |
58 | type SetStatus struct { |
59 | - Entities []SetEntityStatus |
60 | + Entities []EntityStatus |
61 | } |
62 | |
63 | // StatusResult holds an entity status, extra information, or an |
64 | // error. |
65 | type StatusResult struct { |
66 | Error *Error |
67 | + Id string |
68 | + Life Life |
69 | Status Status |
70 | Info string |
71 | + Data StatusData |
72 | } |
73 | |
74 | // StatusResults holds multiple status results. |
75 | |
76 | === modified file 'state/api/provisioner/machine.go' |
77 | --- state/api/provisioner/machine.go 2014-03-24 13:27:29 +0000 |
78 | +++ state/api/provisioner/machine.go 2014-03-26 05:17:40 +0000 |
79 | @@ -56,11 +56,11 @@ |
80 | } |
81 | |
82 | // SetStatus sets the status of the machine. |
83 | -func (m *Machine) SetStatus(status params.Status, info string) error { |
84 | +func (m *Machine) SetStatus(status params.Status, info string, data params.StatusData) error { |
85 | var result params.ErrorResults |
86 | args := params.SetStatus{ |
87 | - Entities: []params.SetEntityStatus{ |
88 | - {Tag: m.tag, Status: status, Info: info}, |
89 | + Entities: []params.EntityStatus{ |
90 | + {Tag: m.tag, Status: status, Info: info, Data: data}, |
91 | }, |
92 | } |
93 | err := m.st.call("SetStatus", args, &result) |
94 | |
95 | === modified file 'state/api/provisioner/provisioner.go' |
96 | --- state/api/provisioner/provisioner.go 2014-03-21 18:40:43 +0000 |
97 | +++ state/api/provisioner/provisioner.go 2014-03-26 05:17:40 +0000 |
98 | @@ -6,6 +6,7 @@ |
99 | import ( |
100 | "fmt" |
101 | |
102 | + "launchpad.net/juju-core/names" |
103 | "launchpad.net/juju-core/state/api/base" |
104 | "launchpad.net/juju-core/state/api/common" |
105 | "launchpad.net/juju-core/state/api/params" |
106 | @@ -107,3 +108,25 @@ |
107 | err = st.call("ContainerConfig", nil, &result) |
108 | return result, err |
109 | } |
110 | + |
111 | +// MachinesWithTransientErrors returns a slice of machines and corresponding status information |
112 | +// for those machines which have transient provisioning errors. |
113 | +func (st *State) MachinesWithTransientErrors() ([]*Machine, []params.StatusResult, error) { |
114 | + var results params.StatusResults |
115 | + err := st.call("MachinesWithTransientErrors", nil, &results) |
116 | + if err != nil { |
117 | + return nil, nil, err |
118 | + } |
119 | + machines := make([]*Machine, len(results.Results)) |
120 | + for i, status := range results.Results { |
121 | + if status.Error != nil { |
122 | + continue |
123 | + } |
124 | + machines[i] = &Machine{ |
125 | + tag: names.MachineTag(status.Id), |
126 | + life: status.Life, |
127 | + st: st, |
128 | + } |
129 | + } |
130 | + return machines, results.Results, nil |
131 | +} |
132 | |
133 | === modified file 'state/api/provisioner/provisioner_test.go' |
134 | --- state/api/provisioner/provisioner_test.go 2014-03-21 18:18:08 +0000 |
135 | +++ state/api/provisioner/provisioner_test.go 2014-03-26 05:17:40 +0000 |
136 | @@ -88,13 +88,61 @@ |
137 | c.Assert(status, gc.Equals, params.StatusPending) |
138 | c.Assert(info, gc.Equals, "") |
139 | |
140 | - err = apiMachine.SetStatus(params.StatusStarted, "blah") |
141 | + err = apiMachine.SetStatus(params.StatusStarted, "blah", nil) |
142 | c.Assert(err, gc.IsNil) |
143 | |
144 | status, info, err = apiMachine.Status() |
145 | c.Assert(err, gc.IsNil) |
146 | c.Assert(status, gc.Equals, params.StatusStarted) |
147 | c.Assert(info, gc.Equals, "blah") |
148 | + _, _, data, err := s.machine.Status() |
149 | + c.Assert(err, gc.IsNil) |
150 | + c.Assert(data, gc.HasLen, 0) |
151 | +} |
152 | + |
153 | +func (s *provisionerSuite) TestGetSetStatusWithData(c *gc.C) { |
154 | + apiMachine, err := s.provisioner.Machine(s.machine.Tag()) |
155 | + c.Assert(err, gc.IsNil) |
156 | + |
157 | + err = apiMachine.SetStatus(params.StatusError, "blah", params.StatusData{"foo": "bar"}) |
158 | + c.Assert(err, gc.IsNil) |
159 | + |
160 | + status, info, err := apiMachine.Status() |
161 | + c.Assert(err, gc.IsNil) |
162 | + c.Assert(status, gc.Equals, params.StatusError) |
163 | + c.Assert(info, gc.Equals, "blah") |
164 | + _, _, data, err := s.machine.Status() |
165 | + c.Assert(err, gc.IsNil) |
166 | + c.Assert(data, gc.DeepEquals, params.StatusData{"foo": "bar"}) |
167 | +} |
168 | + |
169 | +func (s *provisionerSuite) TestMachinesWithTransientErrors(c *gc.C) { |
170 | + machine, err := s.State.AddMachine("quantal", state.JobHostUnits) |
171 | + c.Assert(err, gc.IsNil) |
172 | + err = machine.SetStatus(params.StatusError, "blah", params.StatusData{"transient": true}) |
173 | + c.Assert(err, gc.IsNil) |
174 | + password, err := utils.RandomPassword() |
175 | + c.Assert(err, gc.IsNil) |
176 | + err = machine.SetPassword(password) |
177 | + c.Assert(err, gc.IsNil) |
178 | + err = machine.SetProvisioned("i-manager", "fake_nonce", nil) |
179 | + c.Assert(err, gc.IsNil) |
180 | + st := s.OpenAPIAsMachine(c, machine.Tag(), password, "fake_nonce") |
181 | + c.Assert(s.st, gc.NotNil) |
182 | + p := st.Provisioner() |
183 | + |
184 | + machines, info, err := p.MachinesWithTransientErrors() |
185 | + c.Assert(err, gc.IsNil) |
186 | + c.Assert(machines, gc.HasLen, 1) |
187 | + c.Assert(machines[0].Id(), gc.Equals, "1") |
188 | + c.Assert(info, gc.HasLen, 1) |
189 | + c.Assert(info[0], gc.DeepEquals, params.StatusResult{ |
190 | + Id: "1", |
191 | + Life: "alive", |
192 | + Status: "error", |
193 | + Info: "blah", |
194 | + Data: params.StatusData{"transient": true}, |
195 | + }) |
196 | } |
197 | |
198 | func (s *provisionerSuite) TestEnsureDeadAndRemove(c *gc.C) { |
199 | @@ -254,7 +302,7 @@ |
200 | |
201 | // Change something other than the containers and make sure it's |
202 | // not detected. |
203 | - err = apiMachine.SetStatus(params.StatusStarted, "not really") |
204 | + err = apiMachine.SetStatus(params.StatusStarted, "not really", nil) |
205 | c.Assert(err, gc.IsNil) |
206 | wc.AssertNoChange() |
207 | |
208 | |
209 | === modified file 'state/api/uniter/unit.go' |
210 | --- state/api/uniter/unit.go 2014-03-21 18:40:43 +0000 |
211 | +++ state/api/uniter/unit.go 2014-03-26 05:17:40 +0000 |
212 | @@ -58,7 +58,7 @@ |
213 | func (u *Unit) SetStatus(status params.Status, info string, data params.StatusData) error { |
214 | var result params.ErrorResults |
215 | args := params.SetStatus{ |
216 | - Entities: []params.SetEntityStatus{ |
217 | + Entities: []params.EntityStatus{ |
218 | {Tag: u.tag, Status: status, Info: info, Data: data}, |
219 | }, |
220 | } |
221 | |
222 | === modified file 'state/apiserver/client/client.go' |
223 | --- state/apiserver/client/client.go 2014-03-20 22:35:39 +0000 |
224 | +++ state/apiserver/client/client.go 2014-03-26 05:17:40 +0000 |
225 | @@ -38,6 +38,8 @@ |
226 | resources *common.Resources |
227 | client *Client |
228 | dataDir string |
229 | + // statusSetter provides common methods for updating an entity's provisioning status. |
230 | + statusSetter *common.StatusSetter |
231 | } |
232 | |
233 | // Client serves client-specific API methods. |
234 | @@ -48,10 +50,11 @@ |
235 | // NewAPI creates a new instance of the Client API. |
236 | func NewAPI(st *state.State, resources *common.Resources, authorizer common.Authorizer, datadir string) *API { |
237 | r := &API{ |
238 | - state: st, |
239 | - auth: authorizer, |
240 | - resources: resources, |
241 | - dataDir: datadir, |
242 | + state: st, |
243 | + auth: authorizer, |
244 | + resources: resources, |
245 | + dataDir: datadir, |
246 | + statusSetter: common.NewStatusSetter(st, common.AuthAlways(true)), |
247 | } |
248 | r.client = &Client{ |
249 | api: r, |
250 | @@ -947,3 +950,14 @@ |
251 | } |
252 | return charm.Quote(fmt.Sprintf("%s-%d-%s", name, revision, uuid)), nil |
253 | } |
254 | + |
255 | +// ResolveProvisioningError marks a provisioning error as transient on the machines. |
256 | +func (c *Client) ResolveProvisioningError(p params.Entities) (params.ErrorResults, error) { |
257 | + entityStatus := make([]params.EntityStatus, len(p.Entities)) |
258 | + for i, entity := range p.Entities { |
259 | + entityStatus[i] = params.EntityStatus{Tag: entity.Tag, Data: params.StatusData{"transient": true}} |
260 | + } |
261 | + return c.api.statusSetter.UpdateStatus(params.SetStatus{ |
262 | + Entities: entityStatus, |
263 | + }) |
264 | +} |
265 | |
266 | === modified file 'state/apiserver/client/client_test.go' |
267 | --- state/apiserver/client/client_test.go 2014-03-24 20:08:45 +0000 |
268 | +++ state/apiserver/client/client_test.go 2014-03-26 05:17:40 +0000 |
269 | @@ -2004,3 +2004,18 @@ |
270 | func getArchiveName(bundleURL *url.URL) string { |
271 | return strings.TrimPrefix(bundleURL.RequestURI(), "/dummyenv/private/") |
272 | } |
273 | + |
274 | +func (s *clientSuite) TestResolveProvisioningError(c *gc.C) { |
275 | + machine, err := s.State.AddMachine("quantal", state.JobHostUnits) |
276 | + c.Assert(err, gc.IsNil) |
277 | + err = machine.SetStatus(params.StatusError, "error", nil) |
278 | + c.Assert(err, gc.IsNil) |
279 | + err = s.APIState.Client().ResolveProvisioningError(machine.Tag()) |
280 | + c.Assert(err, gc.IsNil) |
281 | + |
282 | + status, info, data, err := machine.Status() |
283 | + c.Assert(err, gc.IsNil) |
284 | + c.Assert(status, gc.Equals, params.StatusError) |
285 | + c.Assert(info, gc.Equals, "error") |
286 | + c.Assert(data["transient"], gc.Equals, true) |
287 | +} |
288 | |
289 | === modified file 'state/apiserver/common/setstatus.go' |
290 | --- state/apiserver/common/setstatus.go 2014-01-20 21:00:43 +0000 |
291 | +++ state/apiserver/common/setstatus.go 2014-03-26 05:17:40 +0000 |
292 | @@ -58,3 +58,53 @@ |
293 | } |
294 | return result, nil |
295 | } |
296 | + |
297 | +func (s *StatusSetter) updateEntityStatusData(tag string, data params.StatusData) error { |
298 | + entity0, err := s.st.FindEntity(tag) |
299 | + if err != nil { |
300 | + return err |
301 | + } |
302 | + statusGetter, ok := entity0.(state.StatusGetter) |
303 | + if !ok { |
304 | + return NotSupportedError(tag, "getting status") |
305 | + } |
306 | + existingStatus, existingInfo, existingData, err := statusGetter.Status() |
307 | + if err != nil { |
308 | + return err |
309 | + } |
310 | + newData := existingData |
311 | + if newData == nil { |
312 | + newData = data |
313 | + } else { |
314 | + for k, v := range data { |
315 | + newData[k] = v |
316 | + } |
317 | + } |
318 | + entity, ok := entity0.(state.StatusSetter) |
319 | + if !ok { |
320 | + return NotSupportedError(tag, "updating status") |
321 | + } |
322 | + return entity.SetStatus(existingStatus, existingInfo, newData) |
323 | +} |
324 | + |
325 | +// UpdateStatus updates the status data of each given entity. |
326 | +func (s *StatusSetter) UpdateStatus(args params.SetStatus) (params.ErrorResults, error) { |
327 | + result := params.ErrorResults{ |
328 | + Results: make([]params.ErrorResult, len(args.Entities)), |
329 | + } |
330 | + if len(args.Entities) == 0 { |
331 | + return result, nil |
332 | + } |
333 | + canModify, err := s.getCanModify() |
334 | + if err != nil { |
335 | + return params.ErrorResults{}, err |
336 | + } |
337 | + for i, arg := range args.Entities { |
338 | + err := ErrPerm |
339 | + if canModify(arg.Tag) { |
340 | + err = s.updateEntityStatusData(arg.Tag, arg.Data) |
341 | + } |
342 | + result.Results[i].Error = ServerError(err) |
343 | + } |
344 | + return result, nil |
345 | +} |
346 | |
347 | === modified file 'state/apiserver/common/setstatus_test.go' |
348 | --- state/apiserver/common/setstatus_test.go 2014-01-20 21:29:08 +0000 |
349 | +++ state/apiserver/common/setstatus_test.go 2014-03-26 05:17:40 +0000 |
350 | @@ -34,6 +34,17 @@ |
351 | return s.err |
352 | } |
353 | |
354 | +func (s *fakeStatusSetter) Status() (status params.Status, info string, data params.StatusData, err error) { |
355 | + return s.status, s.info, s.data, nil |
356 | +} |
357 | + |
358 | +func (s *fakeStatusSetter) UpdateStatus(data params.StatusData) error { |
359 | + for k, v := range data { |
360 | + s.data[k] = v |
361 | + } |
362 | + return s.err |
363 | +} |
364 | + |
365 | func (*statusSetterSuite) TestSetStatus(c *gc.C) { |
366 | st := &fakeState{ |
367 | entities: map[string]entityWithError{ |
368 | @@ -55,7 +66,7 @@ |
369 | } |
370 | s := common.NewStatusSetter(st, getCanModify) |
371 | args := params.SetStatus{ |
372 | - Entities: []params.SetEntityStatus{ |
373 | + Entities: []params.EntityStatus{ |
374 | {"x0", params.StatusStarted, "bar", nil}, |
375 | {"x1", params.StatusStopped, "", nil}, |
376 | {"x2", params.StatusPending, "not really", nil}, |
377 | @@ -91,7 +102,7 @@ |
378 | } |
379 | s := common.NewStatusSetter(&fakeState{}, getCanModify) |
380 | args := params.SetStatus{ |
381 | - Entities: []params.SetEntityStatus{{"x0", "", "", nil}}, |
382 | + Entities: []params.EntityStatus{{"x0", "", "", nil}}, |
383 | } |
384 | _, err := s.SetStatus(args) |
385 | c.Assert(err, gc.ErrorMatches, "pow") |
386 | @@ -106,3 +117,56 @@ |
387 | c.Assert(err, gc.IsNil) |
388 | c.Assert(result.Results, gc.HasLen, 0) |
389 | } |
390 | + |
391 | +func (*statusSetterSuite) TestUpdateStatus(c *gc.C) { |
392 | + st := &fakeState{ |
393 | + entities: map[string]entityWithError{ |
394 | + "x0": &fakeStatusSetter{status: params.StatusPending, info: "blah", err: fmt.Errorf("x0 fails")}, |
395 | + "x1": &fakeStatusSetter{status: params.StatusStarted, info: "foo", data: params.StatusData{"foo": "blah"}}, |
396 | + "x2": &fakeStatusSetter{status: params.StatusError, info: "some info"}, |
397 | + "x3": &fakeStatusSetter{fetchError: "x3 error"}, |
398 | + "x4": &fakeStatusSetter{status: params.StatusStopped, info: ""}, |
399 | + }, |
400 | + } |
401 | + getCanModify := func() (common.AuthFunc, error) { |
402 | + return func(tag string) bool { |
403 | + switch tag { |
404 | + case "x0", "x1", "x2", "x3": |
405 | + return true |
406 | + } |
407 | + return false |
408 | + }, nil |
409 | + } |
410 | + s := common.NewStatusSetter(st, getCanModify) |
411 | + args := params.SetStatus{ |
412 | + Entities: []params.EntityStatus{ |
413 | + {Tag: "x0", Data: nil}, |
414 | + {Tag: "x1", Data: nil}, |
415 | + {Tag: "x2", Data: params.StatusData{"foo": "bar"}}, |
416 | + {Tag: "x3", Data: params.StatusData{"foo": "bar"}}, |
417 | + {Tag: "x4", Data: params.StatusData{"foo": "bar"}}, |
418 | + {Tag: "x5", Data: nil}, |
419 | + }, |
420 | + } |
421 | + result, err := s.UpdateStatus(args) |
422 | + c.Assert(err, gc.IsNil) |
423 | + c.Assert(result, gc.DeepEquals, params.ErrorResults{ |
424 | + Results: []params.ErrorResult{ |
425 | + {¶ms.Error{Message: "x0 fails"}}, |
426 | + {nil}, |
427 | + {nil}, |
428 | + {¶ms.Error{Message: "x3 error"}}, |
429 | + {apiservertesting.ErrUnauthorized}, |
430 | + {apiservertesting.ErrUnauthorized}, |
431 | + }, |
432 | + }) |
433 | + get := func(tag string) *fakeStatusSetter { |
434 | + return st.entities[tag].(*fakeStatusSetter) |
435 | + } |
436 | + c.Assert(get("x1").status, gc.Equals, params.StatusStarted) |
437 | + c.Assert(get("x1").info, gc.Equals, "foo") |
438 | + c.Assert(get("x1").data, gc.DeepEquals, params.StatusData{"foo": "blah"}) |
439 | + c.Assert(get("x2").status, gc.Equals, params.StatusError) |
440 | + c.Assert(get("x2").info, gc.Equals, "some info") |
441 | + c.Assert(get("x2").data, gc.DeepEquals, params.StatusData{"foo": "bar"}) |
442 | +} |
443 | |
444 | === modified file 'state/apiserver/machine/machiner_test.go' |
445 | --- state/apiserver/machine/machiner_test.go 2014-01-20 21:00:43 +0000 |
446 | +++ state/apiserver/machine/machiner_test.go 2014-03-26 05:17:40 +0000 |
447 | @@ -57,7 +57,7 @@ |
448 | c.Assert(err, gc.IsNil) |
449 | |
450 | args := params.SetStatus{ |
451 | - Entities: []params.SetEntityStatus{ |
452 | + Entities: []params.EntityStatus{ |
453 | {Tag: "machine-1", Status: params.StatusError, Info: "not really"}, |
454 | {Tag: "machine-0", Status: params.StatusStopped, Info: "foobar"}, |
455 | {Tag: "machine-42", Status: params.StatusStarted, Info: "blah"}, |
456 | |
457 | === modified file 'state/apiserver/provisioner/provisioner.go' |
458 | --- state/apiserver/provisioner/provisioner.go 2014-03-20 13:57:50 +0000 |
459 | +++ state/apiserver/provisioner/provisioner.go 2014-03-26 05:17:40 +0000 |
460 | @@ -214,13 +214,48 @@ |
461 | machine, err := p.getMachine(canAccess, entity.Tag) |
462 | if err == nil { |
463 | r := &result.Results[i] |
464 | - r.Status, r.Info, _, err = machine.Status() |
465 | + r.Status, r.Info, r.Data, err = machine.Status() |
466 | } |
467 | result.Results[i].Error = common.ServerError(err) |
468 | } |
469 | return result, nil |
470 | } |
471 | |
472 | +// MachinesWithTransientErrors returns status data for machines with provisioning |
473 | +// errors which are transient. |
474 | +func (p *ProvisionerAPI) MachinesWithTransientErrors() (params.StatusResults, error) { |
475 | + results := params.StatusResults{} |
476 | + canAccessFunc, err := p.getAuthFunc() |
477 | + if err != nil { |
478 | + return results, err |
479 | + } |
480 | + // TODO (wallyworld) - add state.State API for more efficient machines query |
481 | + machines, err := p.st.AllMachines() |
482 | + if err != nil { |
483 | + return results, err |
484 | + } |
485 | + for _, machine := range machines { |
486 | + if !canAccessFunc(machine.Tag()) { |
487 | + continue |
488 | + } |
489 | + result := params.StatusResult{} |
490 | + if result.Status, result.Info, result.Data, err = machine.Status(); err != nil { |
491 | + continue |
492 | + } |
493 | + if result.Status != params.StatusError { |
494 | + continue |
495 | + } |
496 | + // Transient errors are marked as such in the status data. |
497 | + if transient, ok := result.Data["transient"].(bool); !ok || !transient { |
498 | + continue |
499 | + } |
500 | + result.Id = machine.Id() |
501 | + result.Life = params.Life(machine.Life().String()) |
502 | + results.Results = append(results.Results, result) |
503 | + } |
504 | + return results, nil |
505 | +} |
506 | + |
507 | // Series returns the deployed series for each given machine entity. |
508 | func (p *ProvisionerAPI) Series(args params.Entities) (params.StringResults, error) { |
509 | result := params.StringResults{ |
510 | |
511 | === modified file 'state/apiserver/provisioner/provisioner_test.go' |
512 | --- state/apiserver/provisioner/provisioner_test.go 2014-03-18 02:36:58 +0000 |
513 | +++ state/apiserver/provisioner/provisioner_test.go 2014-03-26 05:17:40 +0000 |
514 | @@ -57,7 +57,7 @@ |
515 | if withStateServer { |
516 | s.machines = append(s.machines, testing.AddStateServerMachine(c, s.State)) |
517 | } |
518 | - for i := 0; i < 3; i++ { |
519 | + for i := 0; i < 4; i++ { |
520 | machine, err := s.State.AddMachine("quantal", state.JobHostUnits) |
521 | c.Check(err, gc.IsNil) |
522 | s.machines = append(s.machines, machine) |
523 | @@ -119,6 +119,7 @@ |
524 | {Tag: s.machines[0].Tag(), Password: "xxx0-1234567890123457890"}, |
525 | {Tag: s.machines[1].Tag(), Password: "xxx1-1234567890123457890"}, |
526 | {Tag: s.machines[2].Tag(), Password: "xxx2-1234567890123457890"}, |
527 | + {Tag: s.machines[3].Tag(), Password: "xxx3-1234567890123457890"}, |
528 | {Tag: "machine-42", Password: "foo"}, |
529 | {Tag: "unit-foo-0", Password: "zzz"}, |
530 | {Tag: "service-bar", Password: "abc"}, |
531 | @@ -131,6 +132,7 @@ |
532 | {nil}, |
533 | {nil}, |
534 | {nil}, |
535 | + {nil}, |
536 | {apiservertesting.NotFoundError("machine 42")}, |
537 | {apiservertesting.ErrUnauthorized}, |
538 | {apiservertesting.ErrUnauthorized}, |
539 | @@ -317,8 +319,9 @@ |
540 | c.Assert(err, gc.IsNil) |
541 | |
542 | args := params.SetStatus{ |
543 | - Entities: []params.SetEntityStatus{ |
544 | - {Tag: s.machines[0].Tag(), Status: params.StatusError, Info: "not really"}, |
545 | + Entities: []params.EntityStatus{ |
546 | + {Tag: s.machines[0].Tag(), Status: params.StatusError, Info: "not really", |
547 | + Data: params.StatusData{"foo": "bar"}}, |
548 | {Tag: s.machines[1].Tag(), Status: params.StatusStopped, Info: "foobar"}, |
549 | {Tag: s.machines[2].Tag(), Status: params.StatusStarted, Info: "again"}, |
550 | {Tag: "machine-42", Status: params.StatusStarted, Info: "blah"}, |
551 | @@ -339,9 +342,58 @@ |
552 | }) |
553 | |
554 | // Verify the changes. |
555 | - s.assertStatus(c, 0, params.StatusError, "not really") |
556 | - s.assertStatus(c, 1, params.StatusStopped, "foobar") |
557 | - s.assertStatus(c, 2, params.StatusStarted, "again") |
558 | + s.assertStatus(c, 0, params.StatusError, "not really", params.StatusData{"foo": "bar"}) |
559 | + s.assertStatus(c, 1, params.StatusStopped, "foobar", params.StatusData{}) |
560 | + s.assertStatus(c, 2, params.StatusStarted, "again", params.StatusData{}) |
561 | +} |
562 | + |
563 | +func (s *withoutStateServerSuite) TestMachinesWithTransientErrors(c *gc.C) { |
564 | + err := s.machines[0].SetStatus(params.StatusStarted, "blah", nil) |
565 | + c.Assert(err, gc.IsNil) |
566 | + err = s.machines[1].SetStatus(params.StatusError, "transient error", |
567 | + params.StatusData{"transient": true, "foo": "bar"}) |
568 | + c.Assert(err, gc.IsNil) |
569 | + err = s.machines[2].SetStatus(params.StatusError, "error", params.StatusData{"transient": false}) |
570 | + c.Assert(err, gc.IsNil) |
571 | + err = s.machines[3].SetStatus(params.StatusError, "error", nil) |
572 | + c.Assert(err, gc.IsNil) |
573 | + |
574 | + result, err := s.provisioner.MachinesWithTransientErrors() |
575 | + c.Assert(err, gc.IsNil) |
576 | + c.Assert(result, gc.DeepEquals, params.StatusResults{ |
577 | + Results: []params.StatusResult{ |
578 | + {Id: "1", Life: "alive", Status: "error", Info: "transient error", |
579 | + Data: params.StatusData{"transient": true, "foo": "bar"}}, |
580 | + }, |
581 | + }) |
582 | +} |
583 | + |
584 | +func (s *withoutStateServerSuite) TestMachinesWithTransientErrorsPermission(c *gc.C) { |
585 | + // Machines where there's permission issues are omitted. |
586 | + anAuthorizer := s.authorizer |
587 | + anAuthorizer.MachineAgent = true |
588 | + anAuthorizer.EnvironManager = false |
589 | + anAuthorizer.Tag = "machine-1" |
590 | + aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, |
591 | + anAuthorizer) |
592 | + err = s.machines[0].SetStatus(params.StatusStarted, "blah", nil) |
593 | + c.Assert(err, gc.IsNil) |
594 | + err = s.machines[1].SetStatus(params.StatusError, "transient error", |
595 | + params.StatusData{"transient": true, "foo": "bar"}) |
596 | + c.Assert(err, gc.IsNil) |
597 | + err = s.machines[2].SetStatus(params.StatusError, "error", params.StatusData{"transient": false}) |
598 | + c.Assert(err, gc.IsNil) |
599 | + err = s.machines[3].SetStatus(params.StatusError, "error", nil) |
600 | + c.Assert(err, gc.IsNil) |
601 | + |
602 | + result, err := aProvisioner.MachinesWithTransientErrors() |
603 | + c.Assert(err, gc.IsNil) |
604 | + c.Assert(result, gc.DeepEquals, params.StatusResults{ |
605 | + Results: []params.StatusResult{ |
606 | + {Id: "1", Life: "alive", Status: "error", Info: "transient error", |
607 | + Data: params.StatusData{"transient": true, "foo": "bar"}}, |
608 | + }, |
609 | + }) |
610 | } |
611 | |
612 | func (s *withoutStateServerSuite) TestEnsureDead(c *gc.C) { |
613 | @@ -384,11 +436,14 @@ |
614 | c.Assert(s.machines[index].Life(), gc.Equals, expectLife) |
615 | } |
616 | |
617 | -func (s *withoutStateServerSuite) assertStatus(c *gc.C, index int, expectStatus params.Status, expectInfo string) { |
618 | - status, info, _, err := s.machines[index].Status() |
619 | +func (s *withoutStateServerSuite) assertStatus(c *gc.C, index int, expectStatus params.Status, expectInfo string, |
620 | + expectData params.StatusData) { |
621 | + |
622 | + status, info, data, err := s.machines[index].Status() |
623 | c.Assert(err, gc.IsNil) |
624 | c.Assert(status, gc.Equals, expectStatus) |
625 | c.Assert(info, gc.Equals, expectInfo) |
626 | + c.Assert(data, gc.DeepEquals, expectData) |
627 | } |
628 | |
629 | func (s *withoutStateServerSuite) TestWatchContainers(c *gc.C) { |
630 | @@ -482,7 +537,7 @@ |
631 | c.Assert(err, gc.IsNil) |
632 | err = s.machines[1].SetStatus(params.StatusStopped, "foo", nil) |
633 | c.Assert(err, gc.IsNil) |
634 | - err = s.machines[2].SetStatus(params.StatusError, "not really", nil) |
635 | + err = s.machines[2].SetStatus(params.StatusError, "not really", params.StatusData{"foo": "bar"}) |
636 | c.Assert(err, gc.IsNil) |
637 | |
638 | args := params.Entities{Entities: []params.Entity{ |
639 | @@ -497,9 +552,9 @@ |
640 | c.Assert(err, gc.IsNil) |
641 | c.Assert(result, gc.DeepEquals, params.StatusResults{ |
642 | Results: []params.StatusResult{ |
643 | - {Status: params.StatusStarted, Info: "blah"}, |
644 | - {Status: params.StatusStopped, Info: "foo"}, |
645 | - {Status: params.StatusError, Info: "not really"}, |
646 | + {Status: params.StatusStarted, Info: "blah", Data: params.StatusData{}}, |
647 | + {Status: params.StatusStopped, Info: "foo", Data: params.StatusData{}}, |
648 | + {Status: params.StatusError, Info: "not really", Data: params.StatusData{"foo": "bar"}}, |
649 | {Error: apiservertesting.NotFoundError("machine 42")}, |
650 | {Error: apiservertesting.ErrUnauthorized}, |
651 | {Error: apiservertesting.ErrUnauthorized}, |
652 | @@ -650,7 +705,7 @@ |
653 | c.Assert(err, gc.IsNil) |
654 | c.Assert(result, gc.DeepEquals, params.StringsWatchResult{ |
655 | StringsWatcherId: "1", |
656 | - Changes: []string{"0", "1", "2"}, |
657 | + Changes: []string{"0", "1", "2", "3"}, |
658 | }) |
659 | |
660 | // Verify the resources were registered and stop them when done. |
661 | |
662 | === modified file 'state/apiserver/uniter/uniter_test.go' |
663 | --- state/apiserver/uniter/uniter_test.go 2014-03-17 13:22:55 +0000 |
664 | +++ state/apiserver/uniter/uniter_test.go 2014-03-26 05:17:40 +0000 |
665 | @@ -109,7 +109,7 @@ |
666 | c.Assert(err, gc.IsNil) |
667 | |
668 | args := params.SetStatus{ |
669 | - Entities: []params.SetEntityStatus{ |
670 | + Entities: []params.EntityStatus{ |
671 | {Tag: "unit-mysql-0", Status: params.StatusError, Info: "not really"}, |
672 | {Tag: "unit-wordpress-0", Status: params.StatusStopped, Info: "foobar"}, |
673 | {Tag: "unit-foo-42", Status: params.StatusStarted, Info: "blah"}, |
674 | |
675 | === modified file 'state/interface.go' |
676 | --- state/interface.go 2014-01-20 17:47:59 +0000 |
677 | +++ state/interface.go 2014-03-26 05:17:40 +0000 |
678 | @@ -37,9 +37,15 @@ |
679 | SetStatus(status params.Status, info string, data params.StatusData) error |
680 | } |
681 | |
682 | +type StatusGetter interface { |
683 | + Status() (status params.Status, info string, data params.StatusData, err error) |
684 | +} |
685 | + |
686 | var ( |
687 | _ StatusSetter = (*Machine)(nil) |
688 | _ StatusSetter = (*Unit)(nil) |
689 | + _ StatusGetter = (*Machine)(nil) |
690 | + _ StatusGetter = (*Unit)(nil) |
691 | ) |
692 | |
693 | // Lifer represents an entity with a life. |
694 | |
695 | === modified file 'state/machine_test.go' |
696 | === modified file 'worker/provisioner/provisioner_task.go' |
697 | --- worker/provisioner/provisioner_task.go 2014-03-13 05:09:14 +0000 |
698 | +++ worker/provisioner/provisioner_task.go 2014-03-26 05:17:40 +0000 |
699 | @@ -405,7 +405,7 @@ |
700 | // time until the error is resolved, but don't return an |
701 | // error; just keep going with the other machines. |
702 | logger.Errorf("cannot start instance for machine %q: %v", machine, err) |
703 | - if err1 := machine.SetStatus(params.StatusError, err.Error()); err1 != nil { |
704 | + if err1 := machine.SetStatus(params.StatusError, err.Error(), nil); err1 != nil { |
705 | // Something is wrong with this machine, better report it back. |
706 | logger.Errorf("cannot set error status for machine %q: %v", machine, err1) |
707 | return err1 |
Reviewers: mp+212749_ code.launchpad. net,
Message:
Please take a look.
Description:
New APIs for provisioner retries.
Add new APIs to support allowing the provisioner to retry
machines with errors. A machine in error status can be
retried if its status data has transient=true.
1. MachinesWithTra nsientErrors is called by the provisioner ningError is a client API which allows
to find machines which can be retried.
2. ResolveProvisio
the user to manually mark a machine as resolved.
Existing param structs - SetEntityStatus and SetStatus - were
moved from internal.go to params.go and renamed since they
are now used by the client.
https:/ /code.launchpad .net/~wallyworl d/juju- core/machineswi thtransienterro rs-api/ +merge/ 212749
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/80230043/
Affected files (+379, -43 lines): machiner/ machine. go params/ internal. go params/ params. go provisioner/ machine. go provisioner/ provisioner. go provisioner/ provisioner_ test.go uniter/ unit.go /client/ client. go /client/ client_ test.go /common/ setstatus. go /common/ setstatus_ test.go /machine/ machiner_ test.go /provisioner/ provisioner. go /provisioner/ provisioner_ test.go /uniter/ uniter_ test.go test.go provisioner/ provisioner_ task.go
A [revision details]
M state/api/client.go
M state/api/
M state/api/
M state/api/
M state/api/
M state/api/
M state/api/
M state/api/
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/interface.go
M state/machine_
M worker/