Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(112)

Issue 8330043: provisioner: set machine error state; other stuff (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 12 months ago by dimitern
Modified:
10 years, 12 months ago
Reviewers:
mp+156927, fwereade, rog
Visibility:
Public.

Description

provisioner: set machine error state; other stuff The main thing here is the provisioner will use the new machine.SetStatus() to signal failed (error + info: the actual error). In addition, now when a machine goes into error state, it will be skipped, rather than causing PA to die, and also it'll be skipped next time there's a change (only unprovisioned machines not in error state will be picked for provisioning). https://code.launchpad.net/~dimitern/juju-core/023-provisioner-set-error-status/+merge/156927 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 17

Patch Set 2 : provisioner: set machine error state; other stuff #

Patch Set 3 : provisioner: set machine error state; other stuff #

Total comments: 22

Patch Set 4 : provisioner: set machine error state; other stuff #

Total comments: 8

Patch Set 5 : provisioner: set machine error state; other stuff #

Patch Set 6 : provisioner: set machine error state; other stuff #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -28 lines) Patch
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/main_test.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M state/machine.go View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M state/machine_test.go View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M worker/provisioner/provisioner.go View 1 2 3 4 7 chunks +53 lines, -19 lines 0 comments Download
M worker/provisioner/provisioner_test.go View 1 2 5 chunks +44 lines, -2 lines 0 comments Download

Messages

Total messages: 11
dimitern
Please take a look.
10 years, 12 months ago (2013-04-03 17:44:41 UTC) #1
fwereade
Several points, let me know what you think. https://codereview.appspot.com/8330043/diff/1/cmd/juju/main_test.go File cmd/juju/main_test.go (left): https://codereview.appspot.com/8330043/diff/1/cmd/juju/main_test.go#oldcode175 cmd/juju/main_test.go:175: return ...
10 years, 12 months ago (2013-04-04 06:50:08 UTC) #2
dfc
https://codereview.appspot.com/8330043/diff/1/state/machine.go File state/machine.go (right): https://codereview.appspot.com/8330043/diff/1/state/machine.go#newcode487 state/machine.go:487: panic("machine error status with no info") Why panic ? ...
10 years, 12 months ago (2013-04-04 06:54:36 UTC) #3
dimitern
Please take a look. https://codereview.appspot.com/8330043/diff/1/cmd/juju/main_test.go File cmd/juju/main_test.go (left): https://codereview.appspot.com/8330043/diff/1/cmd/juju/main_test.go#oldcode175 cmd/juju/main_test.go:175: return fmt.Sprintf("dummy.%s is broken", environMethod) ...
10 years, 12 months ago (2013-04-04 10:44:29 UTC) #4
dimitern
As discussed the following changes will be made shortly: * SetInstanceId() won't change the status, ...
10 years, 12 months ago (2013-04-04 12:32:04 UTC) #5
dimitern
Please take a look.
10 years, 12 months ago (2013-04-04 13:10:19 UTC) #6
fwereade
LGTM with the following changes: https://codereview.appspot.com/8330043/diff/11001/cmd/juju/main_test.go File cmd/juju/main_test.go (right): https://codereview.appspot.com/8330043/diff/11001/cmd/juju/main_test.go#newcode169 cmd/juju/main_test.go:169: // environMethod is called. ...
10 years, 12 months ago (2013-04-04 13:36:21 UTC) #7
dimitern
Please take a look. https://codereview.appspot.com/8330043/diff/11001/cmd/juju/main_test.go File cmd/juju/main_test.go (right): https://codereview.appspot.com/8330043/diff/11001/cmd/juju/main_test.go#newcode169 cmd/juju/main_test.go:169: // environMethod is called. On ...
10 years, 12 months ago (2013-04-04 15:39:22 UTC) #8
rog
LGTM with the below comments addressed. https://codereview.appspot.com/8330043/diff/19001/worker/provisioner/provisioner.go File worker/provisioner/provisioner.go (right): https://codereview.appspot.com/8330043/diff/19001/worker/provisioner/provisioner.go#newcode166 worker/provisioner/provisioner.go:166: if err := ...
10 years, 12 months ago (2013-04-04 16:47:07 UTC) #9
dimitern
Please take a look. https://codereview.appspot.com/8330043/diff/19001/worker/provisioner/provisioner.go File worker/provisioner/provisioner.go (right): https://codereview.appspot.com/8330043/diff/19001/worker/provisioner/provisioner.go#newcode166 worker/provisioner/provisioner.go:166: if err := p.stopInstances(append(stopping, unknown...)); ...
10 years, 12 months ago (2013-04-04 17:10:17 UTC) #10
dimitern
10 years, 12 months ago (2013-04-04 17:20:32 UTC) #11
*** Submitted:

provisioner: set machine error state; other stuff

The main thing here is the provisioner will use the
new machine.SetStatus() to signal failed (error +
info: the actual error).
In addition, now when a machine goes into error state,
it will be skipped, rather than causing PA to die, and
also it'll be skipped next time there's a change (only
unprovisioned machines not in error state will be picked
for provisioning).

R=fwereade, dfc, rog
CC=
https://codereview.appspot.com/8330043
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b