environs: add machineNonce to StartInstance()

environs: add machineNonce to StartInstance()

Continuing from the previous CL, this propagates
the machine nonce throughout environs.

Now the machineNonce is passed to StartInstance()
in all providers. In a follow-up the support for
handling nonces in state will be added.

https://code.launchpad.net/~dimitern/juju-core/026-enable-nonced-provisioning/+merge/157432

Requires: https://code.launchpad.net/~dimitern/juju-core/025-add-machinenonce-to-agent-conf-and-machine-config/+merge/157422

(do not edit description out of merge proposal)

[dimitern, 2013-04-05 16:52:54]

Please take a look.

[fwereade, 2013-04-05 17:19:56]

I'm a bit lairy of the lack of testing here. I accept (and HATE HATE HATE) that
we can't really do much about the real environs, but we should definitely check
that the provisioner sends an acceptable value.

https://codereview.appspot.com/8429044/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/8429044/diff/1/environs/interface.go#newcode169
environs/interface.go:169: // with the provided machine identifier and nonce
value. The given
s/and nonce value//

https://codereview.appspot.com/8429044/diff/1/environs/interface.go#newcode171
environs/interface.go:171: // to.  Using the same machine id as another running
instance can
Replace this sentence with: "The nonce, which must be unique within an
environment, is used by juju to protect against the consequences of multiple
instances being started with the same machine id."

https://codereview.appspot.com/8429044/diff/1/environs/interface.go#newcode174
environs/interface.go:174: // a set of tools compatible with the current
version.
Drop this nonsense about tools -- I might have forgotten this myself, sorry.

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

https://codereview.appspot.com/8429044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:274: inst, err :=
p.environ.StartInstance(m.Id(), "", m.Series(), cons, stateInfo, apiInfo)
This needs to have a value that isn't "", and that should be tested. Thankfully
you've already added it to dummy.StartInstanceOp, so that should be pretty easy.

[fwereade, 2013-04-05 21:07:51]

On 2013/04/05 17:19:56, fwereade wrote:
> This needs to have a value that isn't "", and that should be tested.
Thankfully
> you've already added it to dummy.StartInstanceOp, so that should be pretty
easy.

Incidentally, this change should be tested live. WIP for now.

[dimitern, 2013-04-08 12:33:40]

Please take a look.

https://codereview.appspot.com/8429044/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/8429044/diff/1/environs/interface.go#newcode169
environs/interface.go:169: // with the provided machine identifier and nonce
value. The given
On 2013/04/05 17:19:57, fwereade wrote:
> s/and nonce value//

Done.

https://codereview.appspot.com/8429044/diff/1/environs/interface.go#newcode171
environs/interface.go:171: // to.  Using the same machine id as another running
instance can
On 2013/04/05 17:19:57, fwereade wrote:
> Replace this sentence with: "The nonce, which must be unique within an
> environment, is used by juju to protect against the consequences of multiple
> instances being started with the same machine id."

Done.

https://codereview.appspot.com/8429044/diff/1/environs/interface.go#newcode174
environs/interface.go:174: // a set of tools compatible with the current
version.
On 2013/04/05 17:19:57, fwereade wrote:
> Drop this nonsense about tools -- I might have forgotten this myself, sorry.

Done.

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

https://codereview.appspot.com/8429044/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:274: inst, err :=
p.environ.StartInstance(m.Id(), "", m.Series(), cons, stateInfo, apiInfo)
On 2013/04/05 17:19:57, fwereade wrote:
> This needs to have a value that isn't "", and that should be tested.
Thankfully
> you've already added it to dummy.StartInstanceOp, so that should be pretty
easy.

Done. I also run the live tests with EC2, and the provisioning works (although I
had other issues, but due to registering a new account, etc. I don't think
they're related to this).

[rog, 2013-04-08 12:46:48]

LGTM with one query below

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go#newcode291
environs/ec2/ec2.go:291: machineNonce: state.BootstrapNonce,
why do we need a special value here?

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go#newcode383
environs/ec2/ec2.go:383: func (e *environ) StartInstance(machineId, machineNonce
string, series string, cons constraints.Value, info *state.Info, apiInfo
*api.Info) (environs.Instance, error) {
let's factor out these args into a struct at some point!

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

https://codereview.appspot.com/8429044/diff/7001/state/state.go#newcode71
state/state.go:71: const BootstrapNonce = "user-admin:bootstrap"
or just "bootstrap" ?

[TheMue, 2013-04-08 12:53:28]

Looks fine, only one little comment.

https://codereview.appspot.com/8429044/diff/7001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8429044/diff/7001/juju/conn.go#newcode51
juju/conn.go:51: log.Noticef("juju: unauthorized error while connecting to state
server; retrying")
Sounds to me like the error is unauthorized, maybe better "authorization error
while ...".

[fwereade, 2013-04-08 13:26:42]

LGTM

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go#newcode383
environs/ec2/ec2.go:383: func (e *environ) StartInstance(machineId, machineNonce
string, series string, cons constraints.Value, info *state.Info, apiInfo
*api.Info) (environs.Instance, error) {
On 2013/04/08 12:46:48, rog wrote:
> let's factor out these args into a struct at some point!

+1, but not this branch -- definitely next time we have to change the signature
though.

https://codereview.appspot.com/8429044/diff/7001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/8429044/diff/7001/environs/jujutest/livetests....
environs/jujutest/livetests.go:686: inst, err := t.Env.StartInstance("4",
"fake_nonce", "unknownseries", constraints.Value{},
testing.InvalidStateInfo("4"), testing.InvalidAPIInfo("4"))
You can add another test that does pass an empty nonce and check that all the
providers return an error.

https://codereview.appspot.com/8429044/diff/7001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8429044/diff/7001/juju/conn.go#newcode51
juju/conn.go:51: log.Noticef("juju: unauthorized error while connecting to state
server; retrying")
On 2013/04/08 12:53:28, TheMue wrote:
> Sounds to me like the error is unauthorized, maybe better "authorization error
> while ...".

+1

https://codereview.appspot.com/8429044/diff/7001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/8429044/diff/7001/juju/testing/conn.go#newcode80
juju/testing/conn.go:80: "invalid nonce",
I prefer the term "fake" to "invalid" in this context.

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

https://codereview.appspot.com/8429044/diff/7001/state/state.go#newcode71
state/state.go:71: const BootstrapNonce = "user-admin:bootstrap"
On 2013/04/08 12:46:48, rog wrote:
> or just "bootstrap" ?

I rather like badging them with the tag of the responsible entity.

[dimitern, 2013-04-08 15:33:17]

*** Submitted:

environs: add machineNonce to StartInstance()

Continuing from the previous CL, this propagates
the machine nonce throughout environs.

Now the machineNonce is passed to StartInstance()
in all providers. In a follow-up the support for
handling nonces in state will be added.

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

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go#newcode291
environs/ec2/ec2.go:291: machineNonce: state.BootstrapNonce,
On 2013/04/08 12:46:48, rog wrote:
> why do we need a special value here?

As discussed: we need a special value anyway, because this is set at bootstrap
time, rather than by the provisioner. It has to be non-empty (otherwise it feels
wrong), and it has to have some value. I lean towards agreeing with fwereade
about having context of the user who created the environment (user-admin).

https://codereview.appspot.com/8429044/diff/7001/environs/ec2/ec2.go#newcode383
environs/ec2/ec2.go:383: func (e *environ) StartInstance(machineId, machineNonce
string, series string, cons constraints.Value, info *state.Info, apiInfo
*api.Info) (environs.Instance, error) {
On 2013/04/08 13:26:42, fwereade wrote:
> On 2013/04/08 12:46:48, rog wrote:
> > let's factor out these args into a struct at some point!
> 
> +1, but not this branch -- definitely next time we have to change the
signature
> though.

I agree, next time these args should be factored out into a struct.

https://codereview.appspot.com/8429044/diff/7001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/8429044/diff/7001/environs/jujutest/livetests....
environs/jujutest/livetests.go:686: inst, err := t.Env.StartInstance("4",
"fake_nonce", "unknownseries", constraints.Value{},
testing.InvalidStateInfo("4"), testing.InvalidAPIInfo("4"))
On 2013/04/08 13:26:42, fwereade wrote:
> You can add another test that does pass an empty nonce and check that all the
> providers return an error.

Done.

https://codereview.appspot.com/8429044/diff/7001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8429044/diff/7001/juju/conn.go#newcode51
juju/conn.go:51: log.Noticef("juju: unauthorized error while connecting to state
server; retrying")
On 2013/04/08 13:26:42, fwereade wrote:
> On 2013/04/08 12:53:28, TheMue wrote:
> > Sounds to me like the error is unauthorized, maybe better "authorization
error
> > while ...".
> 
> +1

Done.

https://codereview.appspot.com/8429044/diff/7001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/8429044/diff/7001/juju/testing/conn.go#newcode80
juju/testing/conn.go:80: "invalid nonce",
On 2013/04/08 13:26:42, fwereade wrote:
> I prefer the term "fake" to "invalid" in this context.

Done.

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

https://codereview.appspot.com/8429044/diff/7001/state/state.go#newcode71
state/state.go:71: const BootstrapNonce = "user-admin:bootstrap"
On 2013/04/08 13:26:42, fwereade wrote:
> On 2013/04/08 12:46:48, rog wrote:
> > or just "bootstrap" ?
> 
> I rather like badging them with the tag of the responsible entity.

(see above about discussion)