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

Issue 8620043: finish nonced provisioning (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by dimitern
Modified:
9 years, 7 months ago
Reviewers:
mp+158089
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(), badged with the machine tag the provisioner is running on (the machine id is now passed to NewProvisioner). Also, TestUUID in trivial was extracted into its own test module. Added IsValidUUIDString() and UUIDFromString() -> UUID, error to uuid.go, and tests. https://code.launchpad.net/~dimitern/juju-core/028-provisioner-use-nonces/+merge/158089 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 14

Patch Set 2 : finish nonced provisioning #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -41 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/machine.go View 1 2 chunks +13 lines, -2 lines 0 comments Download
M cmd/jujud/machine_test.go View 1 chunk +3 lines, -0 lines 0 comments Download
M trivial/trivial_test.go View 1 chunk +0 lines, -18 lines 0 comments Download
M trivial/uuid.go View 1 1 chunk +24 lines, -0 lines 0 comments Download
A trivial/uuid_test.go View 1 1 chunk +41 lines, -0 lines 0 comments Download
M worker/provisioner/provisioner.go View 1 4 chunks +12 lines, -4 lines 0 comments Download
M worker/provisioner/provisioner_test.go View 1 15 chunks +20 lines, -17 lines 0 comments Download

Messages

Total messages: 5
dimitern
Please take a look.
9 years, 7 months ago (2013-04-10 12:39:39 UTC) #1
dimitern
This is the same branch from: https://codereview.appspot.com/8561045/ Recreated the proposal, because I stupidly interrupted lbox ...
9 years, 7 months ago (2013-04-10 12:40:52 UTC) #2
fwereade
LGTM modulo below https://codereview.appspot.com/8620043/diff/1/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/8620043/diff/1/cmd/jujud/machine.go#newcode121 cmd/jujud/machine.go:121: // TODO(dimitern) pass the machine id ...
9 years, 7 months ago (2013-04-10 12:51:49 UTC) #3
TheMue
LGTM, only two notes. https://codereview.appspot.com/8620043/diff/1/trivial/uuid.go File trivial/uuid.go (right): https://codereview.appspot.com/8620043/diff/1/trivial/uuid.go#newcode13 trivial/uuid.go:13: const ValidUUIDString = "[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}" On ...
9 years, 7 months ago (2013-04-10 12:59:34 UTC) #4
dimitern
9 years, 7 months ago (2013-04-10 13:49:01 UTC) #5
*** Submitted:

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(),
badged with the machine tag the provisioner is
running on (the machine id is now passed to
NewProvisioner).

Also, TestUUID in trivial was extracted into
its own test module. Added IsValidUUIDString()
and UUIDFromString() -> UUID, error to uuid.go,
and tests.

R=fwereade, TheMue
CC=
https://codereview.appspot.com/8620043

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

https://codereview.appspot.com/8620043/diff/1/cmd/jujud/machine.go#newcode121
cmd/jujud/machine.go:121: // TODO(dimitern) pass the machine id to the
firewaller as well.
On 2013/04/10 12:51:49, fwereade wrote:
> Not sure we need a TODO here. If it's something we change, it's something we
> change across the board for all tasks.

Removed.

https://codereview.appspot.com/8620043/diff/1/cmd/jujud/machine.go#newcode142
cmd/jujud/machine.go:142: log.Errorf("cmd/jujud: tried to start an agent for
unprovisioned machine %q", m)
On 2013/04/10 12:51:49, fwereade wrote:
> "running machine %v agent on inappropriate instance" perhaps?

Done.

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

https://codereview.appspot.com/8620043/diff/1/trivial/uuid.go#newcode13
trivial/uuid.go:13: const ValidUUIDString =
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}"
On 2013/04/10 12:51:49, fwereade wrote:
> Can we hide this, and have `func IsValidUUIDString(s string) bool` instead
> please?

Done.

https://codereview.appspot.com/8620043/diff/1/trivial/uuid.go#newcode13
trivial/uuid.go:13: const ValidUUIDString =
"[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[8,9,a,b][0-9a-f]{3}-[0-9a-f]{12}"
On 2013/04/10 12:59:34, TheMue wrote:
> On 2013/04/10 12:51:49, fwereade wrote:
> > Can we hide this, and have `func IsValidUUIDString(s string) bool` instead
> > please?
> 
> I would like a UUIDFromString(s string) (UUID, error), which is then used in
> IsValidUUIDString() (return true if err is nil).

I don't think this was necessary now, but I did it anyway.

https://codereview.appspot.com/8620043/diff/1/trivial/uuid_test.go
File trivial/uuid_test.go (right):

https://codereview.appspot.com/8620043/diff/1/trivial/uuid_test.go#newcode28
trivial/uuid_test.go:28: }
On 2013/04/10 12:51:49, fwereade wrote:
> Would be nice to have a separate test for IsValidUUIDString that fails on
> something.

Done.

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

https://codereview.appspot.com/8620043/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:280: uniqueNonce := fmt.Sprintf("%s:%s",
state.MachineTag(p.machineId), uuid.String())
On 2013/04/10 12:59:34, TheMue wrote:
> Would like to see a little note about the reason of this prefix here.
> Additionally nonce as name should be enough.

Done.

https://codereview.appspot.com/8620043/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:280: uniqueNonce := fmt.Sprintf("%s:%s",
state.MachineTag(p.machineId), uuid.String())
On 2013/04/10 12:51:49, fwereade wrote:
> "unique" is implied by "nonce" -- can't we just use that?

Done.
Sign in to reply to this message.

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