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

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)

[dimitern, 2013-04-09 13:27:42]

Please take a look.

[TheMue, 2013-04-09 15:12:22]

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

[dimitern, 2013-04-09 15:14:32]

On 2013/04/09 15:12:22, TheMue wrote:
> 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 (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.
> 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.

Then it's worth mentioning this in the comment, like: "ValidUUID specifies the
regular expression to match a UUID version 4, variant 1. "? I still think it's
worth having it as an exported const, just for reasons like this.

[TheMue, 2013-04-09 15:19:26]

On 2013/04/09 15:14:32, dimitern wrote:
> On 2013/04/09 15:12:22, TheMue wrote:
> > 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 (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.
> > 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.
> 
> Then it's worth mentioning this in the comment, like: "ValidUUID specifies the
> regular expression to match a UUID version 4, variant 1. "? I still think it's
> worth having it as an exported const, just for reasons like this.

Having a const is ok (I btw tested elsewhere simply with HasLen 36). But the
naming is wrong. Sadly a correct name would be long: ValidVer4Var1UUIDString. ;)
But ValidUUIDString should be fine.

[dimitern, 2013-04-09 15:30:31]

On 2013/04/09 15:19:26, TheMue wrote:
> On 2013/04/09 15:14:32, dimitern wrote:
> > On 2013/04/09 15:12:22, TheMue wrote:
> > > 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 (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.
> > > 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.
> > 
> > Then it's worth mentioning this in the comment, like: "ValidUUID specifies
the
> > regular expression to match a UUID version 4, variant 1. "? I still think
it's
> > worth having it as an exported const, just for reasons like this.
> 
> Having a const is ok (I btw tested elsewhere simply with HasLen 36). But the
> naming is wrong. Sadly a correct name would be long: ValidVer4Var1UUIDString.
;)
> But ValidUUIDString should be fine.

It is a string, there's no doubt about it. If you really insist on the suffix,
ok, although I think is making the name longer for no good reason.

[TheMue, 2013-04-09 15:48:19]

On 2013/04/09 15:30:31, dimitern wrote:
> On 2013/04/09 15:19:26, TheMue wrote:
> > On 2013/04/09 15:14:32, dimitern wrote:
> > > On 2013/04/09 15:12:22, TheMue wrote:
> > > > 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 (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.
> > > > 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.
> > > 
> > > Then it's worth mentioning this in the comment, like: "ValidUUID specifies
> the
> > > regular expression to match a UUID version 4, variant 1. "? I still think
> it's
> > > worth having it as an exported const, just for reasons like this.
> > 
> > Having a const is ok (I btw tested elsewhere simply with HasLen 36). But the
> > naming is wrong. Sadly a correct name would be long:
ValidVer4Var1UUIDString.
> ;)
> > But ValidUUIDString should be fine.
> 
> It is a string, there's no doubt about it. If you really insist on the suffix,
> ok, although I think is making the name longer for no good reason.

As it is used in the direct context of the type UUID I would like ti have it
named correctly.

[fwereade, 2013-04-09 21:27:21]

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 and have the error return inside the if -- it just
feels a bit less mentally costly.

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.
// The agent is running on a different machine to the one it
// should be according to state. It must stop immediately.

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()
Please badge this with the tag of the machine running the provisioner, to match
BootstrapNonce's style.

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

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.