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

Issue 8561045: finish nonced provisioning (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 8 months ago by dimitern
Modified:
9 years, 8 months ago
Reviewers:
fwereade, mp+157878, TheMue
Visibility:
Public.

Description

finish nonced provisioning Now the provisioner uses all that was done in the previous CLs and brings it together: generating an unique nonce at StartInstance time and passing that to SetProvisioned as well. MA.Entity now returns ErrTerminateAgent when the machine is not provisioned. The nonce is generated using trivial.NewUUID() https://code.launchpad.net/~dimitern/juju-core/028-provisioner-use-nonces/+merge/157878 Requires: https://code.launchpad.net/~dimitern/juju-core/027-state-supports-nonced-provisioning/+merge/157853 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -10 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/machine.go View 1 chunk +11 lines, -1 line 4 comments Download
M cmd/jujud/machine_test.go View 2 chunks +5 lines, -2 lines 0 comments Download
M trivial/trivial_test.go View 1 chunk +1 line, -1 line 0 comments Download
M trivial/uuid.go View 1 chunk +3 lines, -0 lines 2 comments Download
M worker/provisioner/provisioner.go View 2 chunks +8 lines, -4 lines 2 comments Download
M worker/provisioner/provisioner_test.go View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 8
dimitern
Please take a look.
9 years, 8 months ago (2013-04-09 13:27:42 UTC) #1
TheMue
Mostly OK, but I'm not happy with the term ValidUUID, see comment. https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go File trivial/uuid.go ...
9 years, 8 months ago (2013-04-09 15:12:22 UTC) #2
dimitern
On 2013/04/09 15:12:22, TheMue wrote: > Mostly OK, but I'm not happy with the term ...
9 years, 8 months ago (2013-04-09 15:14:32 UTC) #3
TheMue
On 2013/04/09 15:14:32, dimitern wrote: > On 2013/04/09 15:12:22, TheMue wrote: > > Mostly OK, ...
9 years, 8 months ago (2013-04-09 15:19:26 UTC) #4
dimitern
On 2013/04/09 15:19:26, TheMue wrote: > On 2013/04/09 15:14:32, dimitern wrote: > > On 2013/04/09 ...
9 years, 8 months ago (2013-04-09 15:30:31 UTC) #5
TheMue
On 2013/04/09 15:30:31, dimitern wrote: > On 2013/04/09 15:19:26, TheMue wrote: > > On 2013/04/09 ...
9 years, 8 months ago (2013-04-09 15:48:19 UTC) #6
fwereade
LGTM https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode135 cmd/jujud/machine.go:135: if m.CheckProvisioned(a.Conf.MachineNonce) { I'd prefer to invert this ...
9 years, 8 months ago (2013-04-09 21:27:21 UTC) #7
dimitern
9 years, 8 months ago (2013-04-10 12:36:40 UTC) #8
I screwed up the branch by interrupting lbox propose (remembered to change the
description to include the changes to NewProvisioner) - it was too late, and now
the branch in LP needs to be deleted and pushed again. I'll recreate this CL
afterwards, so I'm closing this one.

https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode135
cmd/jujud/machine.go:135: if m.CheckProvisioned(a.Conf.MachineNonce) {
On 2013/04/09 21:27:21, fwereade wrote:
> I'd prefer to invert this and have the error return inside the if -- it just
> feels a bit less mentally costly.

Done.

https://codereview.appspot.com/8561045/diff/1/cmd/jujud/machine.go#newcode138
cmd/jujud/machine.go:138: // Not provisioned -> no need to run the agent
further.
On 2013/04/09 21:27:21, fwereade wrote:
> // The agent is running on a different machine to the one it
> // should be according to state. It must stop immediately.

Done.

https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go
File trivial/uuid.go (right):

https://codereview.appspot.com/8561045/diff/1/trivial/uuid.go#newcode12
trivial/uuid.go:12: // ValidUUID specifies the regular expression that matches
an UUID.
On 2013/04/09 15:12:22, TheMue wrote:
> As told before, that's not correct. A UUID is a 16 octet binary value. The
> regexp is only matching the *string representation* of a UUID, and here
> especially version 4 variant 1. There are others that a valid too but would
not
> be covered by this regexp.

The only thing this regexp validates is the UUID generated by NewUUID - it
doesn't generate all the possible range of versions and variants, right? I
updated the comment as discussed to include (version 4, variant 2) and changed
the name to ValidUUIDString. I also split the test for UUID in its own module.

https://codereview.appspot.com/8561045/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/8561045/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:278: uniqueNonce := uuid.String()
On 2013/04/09 21:27:21, fwereade wrote:
> Please badge this with the tag of the machine running the provisioner, to
match
> BootstrapNonce's style.

As discussed, by passing the machine id to NewProvisioner, we have this
information here and it's used to get the tag for the badge.
Sign in to reply to this message.

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