finish nonced provisioning

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)

[dimitern, 2013-04-10 12:39:39]

Please take a look.

[dimitern, 2013-04-10 12:40:52]

This is the same branch from: https://codereview.appspot.com/8561045/

Recreated the proposal, because I stupidly interrupted lbox propose and screwed
up the branch in LP while it was being pushed. So I needed to recreate both.

[fwereade, 2013-04-10 12:51:49]

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 to the
firewaller as well.
Not sure we need a TODO here. If it's something we change, it's something we
change across the board for all tasks.

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)
"running machine %v agent on inappropriate instance" perhaps?

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}"
Can we hide this, and have `func IsValidUUIDString(s string) bool` instead
please?

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: }
Would be nice to have a separate test for IsValidUUIDString that fails on
something.

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())
"unique" is implied by "nonce" -- can't we just use that?

[TheMue, 2013-04-10 12:59:34]

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 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).

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())
Would like to see a little note about the reason of this prefix here.
Additionally nonce as name should be enough.

[dimitern, 2013-04-10 13:49:01]

*** 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.