Merge lp://staging/~dimitern/juju-core/027-state-supports-nonced-provisioning into lp://staging/~juju/juju-core/trunk

Reviewers: mp+157853_code.launchpad.net,

Message:
Please take a look.

Description:
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
bool, error; 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)

Please review this at https://codereview.appspot.com/8561044/

Affected files:
   A [revision details]
   M cmd/juju/ssh_test.go
   M cmd/juju/status_test.go
   M state/apiserver/api_test.go
   M state/machine.go
   M state/machine_test.go
   M state/megawatcher_internal_test.go
   M state/state.go
   M state/state_test.go
   M state/unit_test.go
   M worker/deployer/deployer_test.go
   M worker/firewaller/firewaller_test.go
   M worker/provisioner/provisioner.go
   M worker/uniter/filter_test.go
   M worker/uniter/uniter_test.go

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.go#newcode272
worker/provisioner/provisioner.go:272: }
nonce := "fake_nonce"

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode274
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.go#newcode280
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.go#newcode288
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.go#newcode292
worker/provisioner/provisioner.go:292: // killed again.
We should try to stop the instance right away anyway, I think. TODO it
maybe?

https://codereview.appspot.com/8561044/

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.go#newcode272
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.go#newcode274
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.go#newcode280
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.go#newcode287
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.go#newcode288
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.go#newcode292
worker/provisioner/provisioner.go:292: // killed again.
On 2013/04/09 13:26:27, fwereade wrote:
> We should try to stop the instance rig...

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

https://codereview.appspot.com/8561044/

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.

https://codereview.appspot.com/8561044/

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#newcode912
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.

https://codereview.appspot.com/8561044/

On 10 April 2013 03:05, <email address hidden> 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.

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:<email address hidden>>
wrote:

> > On 10 April 2013 03:05, <mailto:<email address hidden>> 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/

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:<email address hidden> 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:<email address hidden>>
> > wrote:
> >
> >> > On 10 April 2013 03:05, <mailto:<email address hidden>> 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/

https://codereview.appspot.com/8561044/

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

https://codereview.appspot.com/8561044/