Merge lp:~wallyworld/juju-core/retryprovisioning-command into lp:~go-bot/juju-core/trunk
- retryprovisioning-command
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Ian Booth |
Approved revision: | no longer in the source branch. |
Merged at revision: | 2489 |
Proposed branch: | lp:~wallyworld/juju-core/retryprovisioning-command |
Merge into: | lp:~go-bot/juju-core/trunk |
Diff against target: |
314 lines (+168/-18) 9 files modified
cmd/juju/main.go (+1/-0) cmd/juju/main_test.go (+1/-0) cmd/juju/retryprovisioning.go (+59/-0) cmd/juju/retryprovisioning_test.go (+82/-0) state/api/client.go (+8/-9) state/apiserver/client/client.go (+2/-2) state/apiserver/client/client_test.go (+2/-2) state/apiserver/common/setstatus.go (+5/-0) state/apiserver/common/setstatus_test.go (+8/-5) |
To merge this branch: | bzr merge lp:~wallyworld/juju-core/retryprovisioning-command |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email:
|
Commit message
Add resolve-
A new command is added: resolve-
The command is used to enabe the provisioner to
retry provisioning a machine if the previous
attempt failed.
Description of the change
Add resolve-
A new command is added: resolve-
The command is used to enabe the provisioner to
retry provisioning a machine if the previous
attempt failed.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Wayne Witzel III (wwitzel3) wrote : | # |
LGTM with some comments that are more questions than suggestions.
https:/
File cmd/juju/
https:/
cmd/juju/
Canonical Ltd.
Don't know if this even matters, but it is 2014.
https:/
File state/apiserver
https:/
state/apiserver
existingStatus != params.StatusError {
No where else in this package do we use fmt for the errors. For
consistency should we consider creating a new error type and returning
that? It just stands out like a sore thumb to me.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
John A Meinel (jameinel) wrote : | # |
We'll want to change this to Retry instead of Resolve and the command to
"retry-
LGTM.
https:/
File cmd/juju/
https:/
cmd/juju/
"resolve-
We should call this "retry-
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Roger Peppe (rogpeppe) wrote : | # |
https:/
File state/apiserver
https:/
state/apiserver
not in an error state", tag)
This error seems odd to me. Shouldn't it be something more like:
"non-error status has unexpected associated data" ?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
William Reade (fwereade) wrote : | # |
I'll add to the chorus: LGTM
https:/
File cmd/juju/
https:/
cmd/juju/
"resolve-
retry-provisioning, as discussed
https:/
cmd/juju/
errors resolved",
"retries provisioning for a failed machine"?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Ian Booth (wallyworld) wrote : | # |
Please take a look.
https:/
File cmd/juju/
https:/
cmd/juju/
Canonical Ltd.
On 2014/03/26 11:29:16, wwitzel3 wrote:
> Don't know if this even matters, but it is 2014.
Done.
https:/
cmd/juju/
"resolve-
On 2014/03/26 11:55:12, jameinel wrote:
> We should call this "retry-
Done.
https:/
File state/apiserver
https:/
state/apiserver
existingStatus != params.StatusError {
On 2014/03/26 11:29:16, wwitzel3 wrote:
> No where else in this package do we use fmt for the errors. For
consistency
> should we consider creating a new error type and returning that? It
just stands
> out like a sore thumb to me.
See comment below - I'm maintaining consistency with entity.SetStatus()
- see setEntityStatus() earlier in this file.
https:/
state/apiserver
not in an error state", tag)
On 2014/03/26 11:55:23, rog wrote:
> This error seems odd to me. Shouldn't it be something more like:
"non-error
> status has unexpected associated data" ?
This error message is consistent with the error returned under the same
circumstances when setting status on a machine/unit - see
setEntityStatus() earlier in this file. I'd rather keep it as it for
consistency.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~wallyworld/juju-core/retryprovisioning-command 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 'cmd/juju/main.go' |
2 | --- cmd/juju/main.go 2014-03-26 05:44:41 +0000 |
3 | +++ cmd/juju/main.go 2014-03-26 13:26:23 +0000 |
4 | @@ -98,6 +98,7 @@ |
5 | jujucmd.Register(wrap(&ResolvedCommand{})) |
6 | jujucmd.Register(wrap(&DebugLogCommand{sshCmd: &SSHCommand{}})) |
7 | jujucmd.Register(wrap(&DebugHooksCommand{})) |
8 | + jujucmd.Register(wrap(&RetryProvisioningCommand{})) |
9 | |
10 | // Configuration commands. |
11 | jujucmd.Register(wrap(&InitCommand{})) |
12 | |
13 | === modified file 'cmd/juju/main_test.go' |
14 | --- cmd/juju/main_test.go 2014-03-26 05:44:41 +0000 |
15 | +++ cmd/juju/main_test.go 2014-03-26 13:26:23 +0000 |
16 | @@ -239,6 +239,7 @@ |
17 | "remove-service", // alias for destroy-service |
18 | "remove-unit", // alias for destroy-unit |
19 | "resolved", |
20 | + "retry-provisioning", |
21 | "run", |
22 | "scp", |
23 | "set", |
24 | |
25 | === added file 'cmd/juju/retryprovisioning.go' |
26 | --- cmd/juju/retryprovisioning.go 1970-01-01 00:00:00 +0000 |
27 | +++ cmd/juju/retryprovisioning.go 2014-03-26 13:26:23 +0000 |
28 | @@ -0,0 +1,59 @@ |
29 | +// Copyright 2014 Canonical Ltd. |
30 | +// Licensed under the AGPLv3, see LICENCE file for details. |
31 | + |
32 | +package main |
33 | + |
34 | +import ( |
35 | + "fmt" |
36 | + |
37 | + "launchpad.net/juju-core/cmd" |
38 | + "launchpad.net/juju-core/juju" |
39 | + "launchpad.net/juju-core/names" |
40 | +) |
41 | + |
42 | +// RetryProvisioningCommand updates machines' error status to tell |
43 | +// the provisoner that it should try to re-provision the machine. |
44 | +type RetryProvisioningCommand struct { |
45 | + cmd.EnvCommandBase |
46 | + Machines []string |
47 | +} |
48 | + |
49 | +func (c *RetryProvisioningCommand) Info() *cmd.Info { |
50 | + return &cmd.Info{ |
51 | + Name: "retry-provisioning", |
52 | + Args: "<machine> [...]", |
53 | + Purpose: "retries provisioning for failed machines", |
54 | + } |
55 | +} |
56 | + |
57 | +func (c *RetryProvisioningCommand) Init(args []string) error { |
58 | + if len(args) == 0 { |
59 | + return fmt.Errorf("no machine specified") |
60 | + } |
61 | + c.Machines = make([]string, len(args)) |
62 | + for i, arg := range args { |
63 | + if !names.IsMachine(arg) { |
64 | + return fmt.Errorf("invalid machine %q", arg) |
65 | + } |
66 | + c.Machines[i] = names.MachineTag(arg) |
67 | + } |
68 | + return nil |
69 | +} |
70 | + |
71 | +func (c *RetryProvisioningCommand) Run(context *cmd.Context) error { |
72 | + client, err := juju.NewAPIClientFromName(c.EnvName) |
73 | + if err != nil { |
74 | + return err |
75 | + } |
76 | + defer client.Close() |
77 | + results, err := client.RetryProvisioning(c.Machines...) |
78 | + if err != nil { |
79 | + return err |
80 | + } |
81 | + for i, result := range results { |
82 | + if result.Error != nil { |
83 | + fmt.Fprintf(context.Stderr, "cannot retry provisioning %q: %v\n", c.Machines[i], result.Error) |
84 | + } |
85 | + } |
86 | + return nil |
87 | +} |
88 | |
89 | === added file 'cmd/juju/retryprovisioning_test.go' |
90 | --- cmd/juju/retryprovisioning_test.go 1970-01-01 00:00:00 +0000 |
91 | +++ cmd/juju/retryprovisioning_test.go 2014-03-26 13:26:23 +0000 |
92 | @@ -0,0 +1,82 @@ |
93 | +// Copyright 2014 Canonical Ltd. |
94 | +// Licensed under the AGPLv3, see LICENCE file for details. |
95 | + |
96 | +package main |
97 | + |
98 | +import ( |
99 | + "strings" |
100 | + |
101 | + jc "github.com/juju/testing/checkers" |
102 | + gc "launchpad.net/gocheck" |
103 | + |
104 | + jujutesting "launchpad.net/juju-core/juju/testing" |
105 | + "launchpad.net/juju-core/state" |
106 | + "launchpad.net/juju-core/state/api/params" |
107 | + "launchpad.net/juju-core/testing" |
108 | +) |
109 | + |
110 | +type retryProvisioningSuite struct { |
111 | + jujutesting.JujuConnSuite |
112 | +} |
113 | + |
114 | +var _ = gc.Suite(&retryProvisioningSuite{}) |
115 | + |
116 | +var resolvedMachineTests = []struct { |
117 | + args []string |
118 | + err string |
119 | + stdErr string |
120 | +}{ |
121 | + { |
122 | + err: `no machine specified`, |
123 | + }, { |
124 | + args: []string{"jeremy-fisher"}, |
125 | + err: `invalid machine "jeremy-fisher"`, |
126 | + }, { |
127 | + args: []string{"42"}, |
128 | + stdErr: `cannot retry provisioning "machine-42": machine 42 not found`, |
129 | + }, { |
130 | + args: []string{"1"}, |
131 | + stdErr: `cannot retry provisioning "machine-1": machine "machine-1" is not in an error state`, |
132 | + }, { |
133 | + args: []string{"0"}, |
134 | + }, { |
135 | + args: []string{"0", "1"}, |
136 | + stdErr: `cannot retry provisioning "machine-1": machine "machine-1" is not in an error state`, |
137 | + }, |
138 | +} |
139 | + |
140 | +func (s *retryProvisioningSuite) TestResolved(c *gc.C) { |
141 | + m, err := s.State.AddOneMachine(state.MachineTemplate{ |
142 | + Series: "quantal", |
143 | + Jobs: []state.MachineJob{state.JobManageEnviron}, |
144 | + }) |
145 | + c.Assert(err, gc.IsNil) |
146 | + err = m.SetStatus(params.StatusError, "broken", nil) |
147 | + c.Assert(err, gc.IsNil) |
148 | + _, err = s.State.AddOneMachine(state.MachineTemplate{ |
149 | + Series: "quantal", |
150 | + Jobs: []state.MachineJob{state.JobHostUnits}, |
151 | + }) |
152 | + c.Assert(err, gc.IsNil) |
153 | + |
154 | + for i, t := range resolvedMachineTests { |
155 | + c.Logf("test %d: %v", i, t.args) |
156 | + context, err := testing.RunCommand(c, &RetryProvisioningCommand{}, t.args) |
157 | + if t.err != "" { |
158 | + c.Check(err, gc.ErrorMatches, t.err) |
159 | + continue |
160 | + } else { |
161 | + c.Check(err, gc.IsNil) |
162 | + } |
163 | + output := testing.Stderr(context) |
164 | + stripped := strings.Replace(output, "\n", "", -1) |
165 | + c.Check(stripped, gc.Equals, t.stdErr) |
166 | + if t.args[0] == "0" { |
167 | + status, info, data, err := m.Status() |
168 | + c.Check(err, gc.IsNil) |
169 | + c.Check(status, gc.Equals, params.StatusError) |
170 | + c.Check(info, gc.Equals, "broken") |
171 | + c.Check(data["transient"], jc.IsTrue) |
172 | + } |
173 | + } |
174 | +} |
175 | |
176 | === modified file 'state/api/client.go' |
177 | --- state/api/client.go 2014-03-26 09:49:29 +0000 |
178 | +++ state/api/client.go 2014-03-26 13:26:23 +0000 |
179 | @@ -138,18 +138,17 @@ |
180 | return c.call("Resolved", p, nil) |
181 | } |
182 | |
183 | -// ResolveProvisioningError updates the provisioning status of a machine allowing the |
184 | +// RetryProvisioning updates the provisioning status of a machine allowing the |
185 | // provisioner to retry. |
186 | -func (c *Client) ResolveProvisioningError(machine string) error { |
187 | - p := params.Entities{ |
188 | - Entities: []params.Entity{{Tag: machine}}, |
189 | +func (c *Client) RetryProvisioning(machines ...string) ([]params.ErrorResult, error) { |
190 | + p := params.Entities{} |
191 | + p.Entities = make([]params.Entity, len(machines)) |
192 | + for i, machine := range machines { |
193 | + p.Entities[i] = params.Entity{Tag: machine} |
194 | } |
195 | var results params.ErrorResults |
196 | - err := c.st.Call("Client", "", "ResolveProvisioningError", p, &results) |
197 | - if err != nil { |
198 | - return err |
199 | - } |
200 | - return results.OneError() |
201 | + err := c.st.Call("Client", "", "RetryProvisioning", p, &results) |
202 | + return results.Results, err |
203 | } |
204 | |
205 | // PublicAddress returns the public address of the specified |
206 | |
207 | === modified file 'state/apiserver/client/client.go' |
208 | --- state/apiserver/client/client.go 2014-03-26 09:49:29 +0000 |
209 | +++ state/apiserver/client/client.go 2014-03-26 13:26:23 +0000 |
210 | @@ -966,8 +966,8 @@ |
211 | return charm.Quote(fmt.Sprintf("%s-%d-%s", name, revision, uuid)), nil |
212 | } |
213 | |
214 | -// ResolveProvisioningError marks a provisioning error as transient on the machines. |
215 | -func (c *Client) ResolveProvisioningError(p params.Entities) (params.ErrorResults, error) { |
216 | +// RetryProvisioning marks a provisioning error as transient on the machines. |
217 | +func (c *Client) RetryProvisioning(p params.Entities) (params.ErrorResults, error) { |
218 | entityStatus := make([]params.EntityStatus, len(p.Entities)) |
219 | for i, entity := range p.Entities { |
220 | entityStatus[i] = params.EntityStatus{Tag: entity.Tag, Data: params.StatusData{"transient": true}} |
221 | |
222 | === modified file 'state/apiserver/client/client_test.go' |
223 | --- state/apiserver/client/client_test.go 2014-03-26 09:49:29 +0000 |
224 | +++ state/apiserver/client/client_test.go 2014-03-26 13:26:23 +0000 |
225 | @@ -2068,12 +2068,12 @@ |
226 | return strings.TrimPrefix(bundleURL.RequestURI(), "/dummyenv/private/") |
227 | } |
228 | |
229 | -func (s *clientSuite) TestResolveProvisioningError(c *gc.C) { |
230 | +func (s *clientSuite) TestRetryProvisioning(c *gc.C) { |
231 | machine, err := s.State.AddMachine("quantal", state.JobHostUnits) |
232 | c.Assert(err, gc.IsNil) |
233 | err = machine.SetStatus(params.StatusError, "error", nil) |
234 | c.Assert(err, gc.IsNil) |
235 | - err = s.APIState.Client().ResolveProvisioningError(machine.Tag()) |
236 | + _, err = s.APIState.Client().RetryProvisioning(machine.Tag()) |
237 | c.Assert(err, gc.IsNil) |
238 | |
239 | status, info, data, err := machine.Status() |
240 | |
241 | === modified file 'state/apiserver/common/setstatus.go' |
242 | --- state/apiserver/common/setstatus.go 2014-03-26 00:24:30 +0000 |
243 | +++ state/apiserver/common/setstatus.go 2014-03-26 13:26:23 +0000 |
244 | @@ -4,6 +4,8 @@ |
245 | package common |
246 | |
247 | import ( |
248 | + "fmt" |
249 | + |
250 | "launchpad.net/juju-core/state" |
251 | "launchpad.net/juju-core/state/api/params" |
252 | ) |
253 | @@ -84,6 +86,9 @@ |
254 | if !ok { |
255 | return NotSupportedError(tag, "updating status") |
256 | } |
257 | + if len(newData) > 0 && existingStatus != params.StatusError { |
258 | + return fmt.Errorf("machine %q is not in an error state", tag) |
259 | + } |
260 | return entity.SetStatus(existingStatus, existingInfo, newData) |
261 | } |
262 | |
263 | |
264 | === modified file 'state/apiserver/common/setstatus_test.go' |
265 | --- state/apiserver/common/setstatus_test.go 2014-03-26 00:24:30 +0000 |
266 | +++ state/apiserver/common/setstatus_test.go 2014-03-26 13:26:23 +0000 |
267 | @@ -122,16 +122,17 @@ |
268 | st := &fakeState{ |
269 | entities: map[string]entityWithError{ |
270 | "x0": &fakeStatusSetter{status: params.StatusPending, info: "blah", err: fmt.Errorf("x0 fails")}, |
271 | - "x1": &fakeStatusSetter{status: params.StatusStarted, info: "foo", data: params.StatusData{"foo": "blah"}}, |
272 | + "x1": &fakeStatusSetter{status: params.StatusError, info: "foo", data: params.StatusData{"foo": "blah"}}, |
273 | "x2": &fakeStatusSetter{status: params.StatusError, info: "some info"}, |
274 | "x3": &fakeStatusSetter{fetchError: "x3 error"}, |
275 | - "x4": &fakeStatusSetter{status: params.StatusStopped, info: ""}, |
276 | + "x4": &fakeStatusSetter{status: params.StatusStarted}, |
277 | + "x5": &fakeStatusSetter{status: params.StatusStopped, info: ""}, |
278 | }, |
279 | } |
280 | getCanModify := func() (common.AuthFunc, error) { |
281 | return func(tag string) bool { |
282 | switch tag { |
283 | - case "x0", "x1", "x2", "x3": |
284 | + case "x0", "x1", "x2", "x3", "x4": |
285 | return true |
286 | } |
287 | return false |
288 | @@ -145,7 +146,8 @@ |
289 | {Tag: "x2", Data: params.StatusData{"foo": "bar"}}, |
290 | {Tag: "x3", Data: params.StatusData{"foo": "bar"}}, |
291 | {Tag: "x4", Data: params.StatusData{"foo": "bar"}}, |
292 | - {Tag: "x5", Data: nil}, |
293 | + {Tag: "x5", Data: params.StatusData{"foo": "bar"}}, |
294 | + {Tag: "x6", Data: nil}, |
295 | }, |
296 | } |
297 | result, err := s.UpdateStatus(args) |
298 | @@ -156,6 +158,7 @@ |
299 | {nil}, |
300 | {nil}, |
301 | {¶ms.Error{Message: "x3 error"}}, |
302 | + {¶ms.Error{Message: `machine "x4" is not in an error state`}}, |
303 | {apiservertesting.ErrUnauthorized}, |
304 | {apiservertesting.ErrUnauthorized}, |
305 | }, |
306 | @@ -163,7 +166,7 @@ |
307 | get := func(tag string) *fakeStatusSetter { |
308 | return st.entities[tag].(*fakeStatusSetter) |
309 | } |
310 | - c.Assert(get("x1").status, gc.Equals, params.StatusStarted) |
311 | + c.Assert(get("x1").status, gc.Equals, params.StatusError) |
312 | c.Assert(get("x1").info, gc.Equals, "foo") |
313 | c.Assert(get("x1").data, gc.DeepEquals, params.StatusData{"foo": "blah"}) |
314 | c.Assert(get("x2").status, gc.Equals, params.StatusError) |
Reviewers: mp+212781_ code.launchpad. net,
Message:
Please take a look.
Description: provisioning- error command
Add resolve-
A new command is added: resolve- provisioning- error
The command is used to enabe the provisioner to
retry provisioning a machine if the previous
attempt failed.
https:/ /code.launchpad .net/~wallyworl d/juju- core/retryprovi sioning- command/ +merge/ 212781
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/80410044/
Affected files (+143, -5 lines): main_test. go resolveprovisio ningerror. go resolveprovisio ningerror_ test.go /common/ setstatus. go /common/ setstatus_ test.go
A [revision details]
M cmd/juju/main.go
M cmd/juju/
A cmd/juju/
A cmd/juju/
M state/apiserver
M state/apiserver