state: add nonced provisioning support

state: add nonced provisioning support

Next step in nonced provisioning; adding support
in state: machine.SetInstanceId() renamed to
SetProvisioned(), taking instance id and nonce.
It allows you change it only once, so some tests
that assumed too much had to be changed (e.g.
changing instance id several times just to
trigger a machine change). Also CheckProvisioned()
is added to machine, taking a nonce and returning
true is returned only when the instance id is set 
and the nonce matches.

In the upcoming CL we'll bring everything
together - PA generating an unique nonce,
and checking it.

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 12:11:05]

Please take a look.

[fwereade, 2013-04-09 13:26:27]

Couple of tweaks

https://codereview.appspot.com/8561044/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode407
state/machine.go:407: func (m *Machine) SetProvisioned(id InstanceId, nonce
string) error {
Please check that neither id nor nonce are "".

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode410
state/machine.go:410: {{"instanceid", ""}},
Hmm. I think it might be better to simplify this -- just assert that instanceid
and nonce are empty, and leave the rest of the error handling as is.

I'm worried that this is a situation in which arbitrarily delayed transactions
can have real-world effects, but I can't see what to do about it. One to think
about...

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode418
state/machine.go:418: errMsg := "cannot set instance id of machine %q: %v"
defer trivial.ErrorContextf(...)

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

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:272: }
nonce := "fake_nonce"

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:274: inst, err :=
p.environ.StartInstance(m.Id(), "fake_nonce", m.Series(), cons, stateInfo,
apiInfo)
s/"fake_nonce"/nonce/

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:280: if err1 :=
m.SetStatus(params.MachineError, err.Error()); err1 != nil {
s/err1/err/, I think?

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:288: if err := m.SetProvisioned(inst.Id(),
""); err != nil {
I think SetProvisioned with an empty nonce should fail, really. s/""/nonce/
anyway.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:292: // killed again.
We should try to stop the instance right away anyway, I think. TODO it maybe?

[TheMue, 2013-04-09 13:31:41]

Picture gets clearer. See Williams comments. From my side one additional.

https://codereview.appspot.com/8561044/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode418
state/machine.go:418: errMsg := "cannot set instance id of machine %q: %v"
On 2013/04/09 13:26:27, fwereade wrote:
> defer trivial.ErrorContextf(...)

+1

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

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:287: // TODO(dimitern) generate an unique
random nonce in a follow-up.
Take trivial.NewUUID() for it.

[dimitern, 2013-04-09 14:04:22]

Please take a look.

https://codereview.appspot.com/8561044/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode407
state/machine.go:407: func (m *Machine) SetProvisioned(id InstanceId, nonce
string) error {
On 2013/04/09 13:26:27, fwereade wrote:
> Please check that neither id nor nonce are "".

I wanted to postpone that, in case there is a use case in tests to reset the
provisioned state, by passing "", "". But this is not needed now, so I'll add
the check, thanks.

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode410
state/machine.go:410: {{"instanceid", ""}},
On 2013/04/09 13:26:27, fwereade wrote:
> Hmm. I think it might be better to simplify this -- just assert that
instanceid
> and nonce are empty, and leave the rest of the error handling as is.
> 
> I'm worried that this is a situation in which arbitrarily delayed transactions
> can have real-world effects, but I can't see what to do about it. One to think
> about...

I raised my concerns online about replaying transactions, etc. I'll do it.

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode418
state/machine.go:418: errMsg := "cannot set instance id of machine %q: %v"
On 2013/04/09 13:26:27, fwereade wrote:
> defer trivial.ErrorContextf(...)

Ha! One place where using it is actually a good idea :)

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

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:272: }
On 2013/04/09 13:26:27, fwereade wrote:
> nonce := "fake_nonce"

Done.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:274: inst, err :=
p.environ.StartInstance(m.Id(), "fake_nonce", m.Series(), cons, stateInfo,
apiInfo)
On 2013/04/09 13:26:27, fwereade wrote:
> s/"fake_nonce"/nonce/

Done.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:280: if err1 :=
m.SetStatus(params.MachineError, err.Error()); err1 != nil {
On 2013/04/09 13:26:27, fwereade wrote:
> s/err1/err/, I think?

We'll lose the context of the previous error, won't we?

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:287: // TODO(dimitern) generate an unique
random nonce in a follow-up.
On 2013/04/09 13:31:41, TheMue wrote:
> Take trivial.NewUUID() for it.

That's coming in the follow-up :)

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:288: if err := m.SetProvisioned(inst.Id(),
""); err != nil {
On 2013/04/09 13:26:27, fwereade wrote:
> I think SetProvisioned with an empty nonce should fail, really. s/""/nonce/
> anyway.

Done.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:292: // killed again.
On 2013/04/09 13:26:27, fwereade wrote:
> We should try to stop the instance right away anyway, I think. TODO it maybe?

We'll do that a bit later in the loop. But I guess it's worth a TODO.

[fwereade, 2013-04-09 15:49:20]

LGTM with the below

https://codereview.appspot.com/8561044/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode413
state/machine.go:413: notSetYet := D{{"$and", []D{
You don't need a $and, you can just have D{{"instanceid", ""}, {"nonce", ""}}

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode438
state/machine.go:438: // CheckProvisioned returns true, if the machine was
provisioned with
s/,//

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode439
state/machine.go:439: // an instance id and the given nonce.
s/an instance id and/

https://codereview.appspot.com/8561044/diff/7001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/state.go#newcode192
state/state.go:192: return st.addMachine(series, instanceId, BootstrapNonce,
jobs)
Please test the change in InjectMachine

[dimitern, 2013-04-09 16:51:26]

Please take a look.

https://codereview.appspot.com/8561044/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode413
state/machine.go:413: notSetYet := D{{"$and", []D{
On 2013/04/09 15:49:20, fwereade wrote:
> You don't need a $and, you can just have D{{"instanceid", ""}, {"nonce", ""}}

ofc, forgot about that. Done.

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode438
state/machine.go:438: // CheckProvisioned returns true, if the machine was
provisioned with
On 2013/04/09 15:49:20, fwereade wrote:
> s/,//

Done.

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode439
state/machine.go:439: // an instance id and the given nonce.
On 2013/04/09 15:49:20, fwereade wrote:
> s/an instance id and/

Done.

https://codereview.appspot.com/8561044/diff/7001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/state.go#newcode192
state/state.go:192: return st.addMachine(series, instanceId, BootstrapNonce,
jobs)
On 2013/04/09 15:49:20, fwereade wrote:
> Please test the change in InjectMachine

Done.

[rog, 2013-04-09 17:52:09]

LGTM with a couple of trivial thoughts below

https://codereview.appspot.com/8561044/diff/7001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:912: stm, err := s.State.AddMachine("series",
state.JobHostUnits)
i have to say i found the logic here really hard to follow. i know the logic
comes straight from state, but i'd like a few comments. the use of oldId and
newId make things harder too, i think. i'd just use the string constants.

https://codereview.appspot.com/8561044/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode439
state/machine.go:439: // an instance id and the given nonce.
i think i'd say "if the machine has been provisioned with the given nonce", as
it's not possible to call SetProvisioned *without* an instance id.

[dave_cheney.net, 2013-04-10 02:05:13]

not lgtm.

Why did SetProvisioned grow another field ? Reading this diff we're always
passing either one of two values "fake-nonce" or state.Bootstrap nonce, but as
far as I can tell nothing checks those values, ie, there are no tests that
assert "fake-nonce" is "fake-nonce"

To be clear, I am not objecting to this change, just the addition of the nonce
argument on SetProvisioned. Why can't the nonce value be assigned inside
SetProvisioned without having to pass it in from the caller ?

[rog, 2013-04-10 08:54:58]

On 10 April 2013 03:05,  <dave@cheney.net> wrote:
> not lgtm.
>
> Why did SetProvisioned grow another field ? Reading this diff we're
> always passing either one of two values "fake-nonce" or state.Bootstrap
> nonce, but as far as I can tell nothing checks those values, ie, there
> are no tests that assert "fake-nonce" is "fake-nonce"
>
> To be clear, I am not objecting to this change, just the addition of the
> nonce argument on SetProvisioned. Why can't the nonce value be assigned
> inside SetProvisioned without having to pass it in from the caller ?

because it needs to be passed into Environ.StartInstance before it's added to
the state.

[dave_cheney.net, 2013-04-10 10:47:14]

Could an opaque value be returned from SetProvisioned which is passed to
StartInstance ?

On 10/04/2013, at 18:54, roger peppe <rogpeppe@gmail.com> wrote:

> On 10 April 2013 03:05,  <dave@cheney.net> wrote:
>> not lgtm.
>> 
>> Why did SetProvisioned grow another field ? Reading this diff we're
>> always passing either one of two values "fake-nonce" or state.Bootstrap
>> nonce, but as far as I can tell nothing checks those values, ie, there
>> are no tests that assert "fake-nonce" is "fake-nonce"
>> 
>> To be clear, I am not objecting to this change, just the addition of the
>> nonce argument on SetProvisioned. Why can't the nonce value be assigned
>> inside SetProvisioned without having to pass it in from the caller ?
> 
> because it needs to be passed into Environ.StartInstance before it's added to
> the state.

[dimitern, 2013-04-10 11:00:31]

On 2013/04/10 10:47:14, dfc wrote:
> Could an opaque value be returned from SetProvisioned which is passed to
> StartInstance ?
> 
> On 10/04/2013, at 18:54, roger peppe <mailto:rogpeppe@gmail.com> wrote:
> 
> > On 10 April 2013 03:05,  <mailto:dave@cheney.net> wrote:
> >> not lgtm.
> >> 
> >> Why did SetProvisioned grow another field ? Reading this diff we're
> >> always passing either one of two values "fake-nonce" or state.Bootstrap
> >> nonce, but as far as I can tell nothing checks those values, ie, there
> >> are no tests that assert "fake-nonce" is "fake-nonce"
> >> 
> >> To be clear, I am not objecting to this change, just the addition of the
> >> nonce argument on SetProvisioned. Why can't the nonce value be assigned
> >> inside SetProvisioned without having to pass it in from the caller ?
> > 
> > because it needs to be passed into Environ.StartInstance before it's added
to
> > the state.

It's the other way around: so SetProvisioned needs to be called by the
provisioner after calling StartInstance. Provisioner generates the nonce, and
passes it to both, so later when the machiner starts, it can verify the nonce
(passed from StartInstance down through cloudinit and eventually agent.Conf)
matches the one in state (set by SetProvisioned). The generation of the nonce is
the next CL.

[dave_cheney.net, 2013-04-10 11:03:33]

Fair enough, objection withdrawn. 

On 10/04/2013, at 21:00, dimiter.naydenov@canonical.com wrote:

> On 2013/04/10 10:47:14, dfc wrote:
>> Could an opaque value be returned from SetProvisioned which is passed
> to
>> StartInstance ?
> 
>> On 10/04/2013, at 18:54, roger peppe <mailto:rogpeppe@gmail.com>
> wrote:
> 
>> > On 10 April 2013 03:05,  <mailto:dave@cheney.net> wrote:
>> >> not lgtm.
>> >>
>> >> Why did SetProvisioned grow another field ? Reading this diff we're
>> >> always passing either one of two values "fake-nonce" or
> state.Bootstrap
>> >> nonce, but as far as I can tell nothing checks those values, ie,
> there
>> >> are no tests that assert "fake-nonce" is "fake-nonce"
>> >>
>> >> To be clear, I am not objecting to this change, just the addition
> of the
>> >> nonce argument on SetProvisioned. Why can't the nonce value be
> assigned
>> >> inside SetProvisioned without having to pass it in from the caller
> ?
>> >
>> > because it needs to be passed into Environ.StartInstance before it's
> added to
>> > the state.
> 
> It's the other way around: so SetProvisioned needs to be called by the
> provisioner after calling StartInstance. Provisioner generates the
> nonce, and passes it to both, so later when the machiner starts, it can
> verify the nonce (passed from StartInstance down through cloudinit and
> eventually agent.Conf) matches the one in state (set by SetProvisioned).
> The generation of the nonce is the next CL.
> 
> https://codereview.appspot.com/8561044/

[dimitern, 2013-04-10 11:10:02]

Thanks for the understanding! Sorry that I wasn't clear enough earlier.

On 2013/04/10 11:03:33, dfc wrote:
> Fair enough, objection withdrawn. 
> 
> On 10/04/2013, at 21:00, mailto:dimiter.naydenov@canonical.com wrote:
> 
> > On 2013/04/10 10:47:14, dfc wrote:
> >> Could an opaque value be returned from SetProvisioned which is passed
> > to
> >> StartInstance ?
> > 
> >> On 10/04/2013, at 18:54, roger peppe <mailto:rogpeppe@gmail.com>
> > wrote:
> > 
> >> > On 10 April 2013 03:05,  <mailto:dave@cheney.net> wrote:
> >> >> not lgtm.
> >> >>
> >> >> Why did SetProvisioned grow another field ? Reading this diff we're
> >> >> always passing either one of two values "fake-nonce" or
> > state.Bootstrap
> >> >> nonce, but as far as I can tell nothing checks those values, ie,
> > there
> >> >> are no tests that assert "fake-nonce" is "fake-nonce"
> >> >>
> >> >> To be clear, I am not objecting to this change, just the addition
> > of the
> >> >> nonce argument on SetProvisioned. Why can't the nonce value be
> > assigned
> >> >> inside SetProvisioned without having to pass it in from the caller
> > ?
> >> >
> >> > because it needs to be passed into Environ.StartInstance before it's
> > added to
> >> > the state.
> > 
> > It's the other way around: so SetProvisioned needs to be called by the
> > provisioner after calling StartInstance. Provisioner generates the
> > nonce, and passes it to both, so later when the machiner starts, it can
> > verify the nonce (passed from StartInstance down through cloudinit and
> > eventually agent.Conf) matches the one in state (set by SetProvisioned).
> > The generation of the nonce is the next CL.
> > 
> > https://codereview.appspot.com/8561044/

[dimitern, 2013-04-10 11:51:00]

*** Submitted:

state: add nonced provisioning support

Next step in nonced provisioning; adding support
in state: machine.SetInstanceId() renamed to
SetProvisioned(), taking instance id and nonce.
It allows you change it only once, so some tests
that assumed too much had to be changed (e.g.
changing instance id several times just to
trigger a machine change). Also CheckProvisioned()
is added to machine, taking a nonce and returning
true is returned only when the instance id is set 
and the nonce matches.

In the upcoming CL we'll bring everything
together - PA generating an unique nonce,
and checking it.

R=fwereade, TheMue, rog, dfc
CC=
https://codereview.appspot.com/8561044

https://codereview.appspot.com/8561044/diff/7001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:912: stm, err := s.State.AddMachine("series",
state.JobHostUnits)
On 2013/04/09 17:52:09, rog wrote:
> i have to say i found the logic here really hard to follow. i know the logic
> comes straight from state, but i'd like a few comments. the use of oldId and
> newId make things harder too, i think. i'd just use the string constants.

Added comments, hopefully now it's easier to follow.

https://codereview.appspot.com/8561044/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode439
state/machine.go:439: // an instance id and the given nonce.
On 2013/04/09 17:52:09, rog wrote:
> i think i'd say "if the machine has been provisioned with the given nonce", as
> it's not possible to call SetProvisioned *without* an instance id.

Done.