The bootstrap node now knows its hardware

The bootstrap node now knows its hardware

It turns out that the solution used here is similar to what
was done in pyjuju. On bootstrapping, we currently write out
a provider-state file with the bootstrap instance id. This is
now augmented to include the hardware charactistics of the
bootstrap node. The bootstrap nodes reads these when it starts.

An implication is that the InstanceId() method on the provider
is no longer required. This was a bit of a hack anyway and I doubt
many will mourn it's loss. 

There's a bit of copy and paste code for parsing hardware 
characteristics, due to a couple of other branches being blocked.
The code would normally be in trunk but I don't want to block this
branch on that. I'll resolve whatever conflicts exist prior to 
landing.

https://code.launchpad.net/~wallyworld/juju-core/bootstrap-node-metadata/+merge/174903

(do not edit description out of merge proposal)

[wallyworld, 2013-07-16 02:38:04]

Please take a look.

[wallyworld, 2013-07-16 02:40:21]

Please take a look.

[wallyworld, 2013-07-16 05:04:30]

Please take a look.

[gz, 2013-07-16 11:29:40]

LGTM.

The basic logic of passing a storage url into bootstrap with cloudconfig, and
after bootstrap storing data that bootstrap will need, is exactly what the
python openstack provider does to fetch the instance id in the correct form. As
that code notes, it is technically introducing a race condition, but we've not
seen any issues with machine bootstrap happening faster than storage
consistency.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go#newc...
cmd/jujud/bootstrap.go:37: f.StringVar(&c.stateInfoURL, "stateinfo-url", "",
"the URL from which to load state information like instanc id and hardware")
*instance id

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go#newc...
cmd/jujud/bootstrap.go:47: return requiredError("state-info-url")
This one says "state-info-url" and earlier it's "stateinfo-url", they should be
the same.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go...
cmd/jujud/bootstrap_test.go:39:
http.DefaultTransport.(*http.Transport).RegisterProtocol("test",
testRoundTripper)
Hm, so the reason we don't need cleanup for this is because it's a test process
scoped fixture?

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go#newco...
environs/bootstrap.go:112: logger.Errorf("cannot load state: %v", err)
Should at least include the URL here, I'm a little worried that if something
goes wrong here the error is going to lack context.

https://codereview.appspot.com/11318043/diff/6001/environs/cloudinit/cloudini...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/cloudinit/cloudini...
environs/cloudinit/cloudinit_test.go:78: StateInfoURL: "http://somefile",
This is a bad test value, even though it's only used for string matching. Use a
.invald url or similar that is guarenteed not to resolve.

https://codereview.appspot.com/11318043/diff/6001/environs/cloudinit/cloudini...
environs/cloudinit/cloudinit_test.go:134: StateInfoURL: "http://somefile",
Likewise.

[mue, 2013-07-16 14:34:38]

Wow, lots of work. LGTM minus some comments.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go#newc...
cmd/jujud/bootstrap.go:46: if len(c.stateInfoURL) == 0 {
I prefer tests like c.stateInfoURL == "" (IMHO more readable).

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go...
cmd/jujud/bootstrap_test.go:22: "net/http"
Please move up to the block of standard packages.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go...
cmd/jujud/bootstrap_test.go:113: args = append(args, []string{"--stateinfo-url",
"test://localhost/provider-state"}...)
As said before "stateinfo-url" or "state-info-url"?

https://codereview.appspot.com/11318043/diff/6001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/azure/environ.go#n...
environs/azure/environ.go:154: // TODO - have hardware characteristics
// TODO(wallyworld) ...

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go#newco...
environs/bootstrap.go:96: func ConfigureBootstrapMachine(
Huh, this number of arguments could need an own type
BootstrapMachineConfiguration.

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go#newco...
environs/bootstrap.go:112: logger.Errorf("cannot load state: %v", err)
On 2013/07/16 11:29:41, gz wrote:
> Should at least include the URL here, I'm a little worried that if something
> goes wrong here the error is going to lack context.

+1

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environ.go#ne...
environs/maas/environ.go:24: "strings"
Again please move up.

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environ.go#ne...
environs/maas/environ.go:138: // TODO add hardware characteristics to
BootstrapState
// TODO(...)

https://codereview.appspot.com/11318043/diff/6001/environs/state.go
File environs/state.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/state.go#newcode20
environs/state.go:20: "net/http"
Again, sort/block packages.

https://codereview.appspot.com/11318043/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/11318043/diff/6001/state/state.go#newcode251
state/state.go:251: Jobs:            jobs})
}) on new line.

[fwereade, 2013-07-16 17:19:57]

+100 to the general idea, but NOT LGTM as is. I'm unconvinced by the changes to
BootstrapState (poorly named though it may be, it's about locating the state/api
endpoints given only creds and control-bucket), and the compatibility issue is a
dealbreaker.

I'd prefer you didn't use agent.Conf really, but I could be convinced either
way... it's not like that file can really become any more overloaded or
incoherent than it already is.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go#newc...
cmd/jujud/bootstrap.go:37: f.StringVar(&c.stateInfoURL, "stateinfo-url", "",
"the URL from which to load state information like instanc id and hardware")
On 2013/07/16 11:29:41, gz wrote:
> *instance id

Can't do this and remain compatibile. Needs to go via either agent.Conf or a
different file.

https://codereview.appspot.com/11318043/diff/6001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/11318043/diff/6001/environs/dummy/environs.go#...
environs/dummy/environs.go:400: }
<3

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environprovid...
File environs/maas/environprovider.go (left):

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environprovid...
environs/maas/environprovider.go:87: return instance.Id(info.InstanceId), nil
I suspect there's more that can be deleted here..?

https://codereview.appspot.com/11318043/diff/6001/environs/state.go
File environs/state.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/state.go#newcode35
environs/state.go:35: Characteristics []instance.HardwareCharacteristics
`yaml:"characteristics"`
I don't think it's sensible to put this in the same file. This is very much a
special bootstrap-only case, while this file will be used and rewritten over the
life of the environment, and I don't think it's a good idea to muddy its
meaning.

[wallyworld, 2013-07-17 04:59:29]

Please take a look.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go#newc...
cmd/jujud/bootstrap.go:37: f.StringVar(&c.stateInfoURL, "stateinfo-url", "",
"the URL from which to load state information like instanc id and hardware")
On 2013/07/16 17:19:57, fwereade wrote:
> On 2013/07/16 11:29:41, gz wrote:
> > *instance id
> 
> Can't do this and remain compatibile. Needs to go via either agent.Conf or a
> different file.

Ok, I've used a file "/tmp/provider-state-url" which is created by cloud-init.
The state info URL is written there.
The bootstrap cmd later on reads the URL from this file instead of expecting a
cmd line parameter.
I used /tmp because it really is a one off passing across of information and I
didn't want to pollute /var/lib/juju with unnecessary, transient stuff.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap.go#newc...
cmd/jujud/bootstrap.go:47: return requiredError("state-info-url")
On 2013/07/16 11:29:41, gz wrote:
> This one says "state-info-url" and earlier it's "stateinfo-url", they should
be
> the same.

This code is gone now

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go...
cmd/jujud/bootstrap_test.go:22: "net/http"
On 2013/07/16 14:34:38, mue wrote:
> Please move up to the block of standard packages.

Done.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go...
cmd/jujud/bootstrap_test.go:39:
http.DefaultTransport.(*http.Transport).RegisterProtocol("test",
testRoundTripper)
On 2013/07/16 11:29:41, gz wrote:
> Hm, so the reason we don't need cleanup for this is because it's a test
process
> scoped fixture?

Yes, believe so. This code came from elsewhere.

https://codereview.appspot.com/11318043/diff/6001/cmd/jujud/bootstrap_test.go...
cmd/jujud/bootstrap_test.go:113: args = append(args, []string{"--stateinfo-url",
"test://localhost/provider-state"}...)
On 2013/07/16 14:34:38, mue wrote:
> As said before "stateinfo-url" or "state-info-url"?

Fair question. This code is now removed so is no longer relevant

https://codereview.appspot.com/11318043/diff/6001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/azure/environ.go#n...
environs/azure/environ.go:154: // TODO - have hardware characteristics
On 2013/07/16 14:34:38, mue wrote:
> // TODO(wallyworld) ...

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/bootstrap.go#newco...
environs/bootstrap.go:112: logger.Errorf("cannot load state: %v", err)
On 2013/07/16 14:34:38, mue wrote:
> On 2013/07/16 11:29:41, gz wrote:
> > Should at least include the URL here, I'm a little worried that if something
> > goes wrong here the error is going to lack context.
> 
> +1

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/cloudinit/cloudini...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/cloudinit/cloudini...
environs/cloudinit/cloudinit_test.go:78: StateInfoURL: "http://somefile",
On 2013/07/16 11:29:41, gz wrote:
> This is a bad test value, even though it's only used for string matching. Use
a
> .invald url or similar that is guarenteed not to resolve.

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/cloudinit/cloudini...
environs/cloudinit/cloudinit_test.go:134: StateInfoURL: "http://somefile",
On 2013/07/16 11:29:41, gz wrote:
> Likewise.

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environ.go#ne...
environs/maas/environ.go:24: "strings"
On 2013/07/16 14:34:38, mue wrote:
> Again please move up.

I thought we relied on whatever go fmt thinks is correct?
When did we decide to do something different?

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environ.go#ne...
environs/maas/environ.go:138: // TODO add hardware characteristics to
BootstrapState
On 2013/07/16 14:34:38, mue wrote:
> // TODO(...)

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environprovid...
File environs/maas/environprovider.go (left):

https://codereview.appspot.com/11318043/diff/6001/environs/maas/environprovid...
environs/maas/environprovider.go:87: return instance.Id(info.InstanceId), nil
On 2013/07/16 17:19:57, fwereade wrote:
> I suspect there's more that can be deleted here..?

Yes, good pickup. I deleted InstanceId from machineInfo{}

https://codereview.appspot.com/11318043/diff/6001/environs/state.go
File environs/state.go (right):

https://codereview.appspot.com/11318043/diff/6001/environs/state.go#newcode20
environs/state.go:20: "net/http"
On 2013/07/16 14:34:38, mue wrote:
> Again, sort/block packages.

Done.

https://codereview.appspot.com/11318043/diff/6001/environs/state.go#newcode35
environs/state.go:35: Characteristics []instance.HardwareCharacteristics
`yaml:"characteristics"`
On 2013/07/16 17:19:57, fwereade wrote:
> I don't think it's sensible to put this in the same file. This is very much a
> special bootstrap-only case, while this file will be used and rewritten over
the
> life of the environment, and I don't think it's a good idea to muddy its
> meaning.

As I think we agreed on irc, i'll leave this here and add a comment. The struct
name does imply this extra info can go here.

https://codereview.appspot.com/11318043/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/11318043/diff/6001/state/state.go#newcode251
state/state.go:251: Jobs:            jobs})
On 2013/07/16 14:34:38, mue wrote:
> }) on new line.

Done.

[wallyworld, 2013-07-17 06:28:45]

Please take a look.

[fwereade, 2013-07-17 09:12:03]

LGTM apart from duplication of "/tmp/provider-state-url". Other comments left to
your judgment.

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:22: var providerStateURLFile = "/tmp/provider-state-url"
There is a "bootstrap" dir somewhere in DataDir (agents/bootstrap perhaps?);
might be better to put it in there. Have a quick look for it, see whether it
actually makes sense to use it.

Either way, I think it'd be best to define where we get it from OAOO rather than
risk ignorant changes to one definition or the other.

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:98: return environs.ConfigureBootstrapMachine(st, cfg,
c.Constraints, c.Conf.DataDir, jobs, stateInfoURL)
I'd really like it if we read the contents of stateInfoURL here and passed down
explicit HardwareCharacteristics. Any particular reason why not?

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/export_test.go
File cmd/jujud/export_test.go (right):

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/export_test.go#n...
cmd/jujud/export_test.go:8: }
Not actually necessary to use the export_test mechanism here. What with the
"main" package being weird, all the tests are in-package anyway.

Maybe we *should* be using it regardless, but if we do this we should move all
the test-specific bits in here at once.

[wallyworld, 2013-07-18 11:32:43]

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:22: var providerStateURLFile = "/tmp/provider-state-url"
On 2013/07/17 09:12:03, fwereade wrote:
> There is a "bootstrap" dir somewhere in DataDir (agents/bootstrap perhaps?);
> might be better to put it in there. Have a quick look for it, see whether it
> actually makes sense to use it.
> 
> Either way, I think it'd be best to define where we get it from OAOO rather
than
> risk ignorant changes to one definition or the other.

I declared a const in environs/cloudinit/cloudinit.go. I had to put it there to
avoid import loops. The agent stuff is not really related and I didn't want to
mess that up. This file really is transient and only required during bootstrap.

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/bootstrap.go#new...
cmd/jujud/bootstrap.go:98: return environs.ConfigureBootstrapMachine(st, cfg,
c.Constraints, c.Conf.DataDir, jobs, stateInfoURL)
On 2013/07/17 09:12:03, fwereade wrote:
> I'd really like it if we read the contents of stateInfoURL here and passed
down
> explicit HardwareCharacteristics. Any particular reason why not?

The instanceId needs to be loaded as well, not just hardware. So I wanted to
pass down one param instead of multiple. Nonetheless, I've changed the method
sig.

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/export_test.go
File cmd/jujud/export_test.go (right):

https://codereview.appspot.com/11318043/diff/20001/cmd/jujud/export_test.go#n...
cmd/jujud/export_test.go:8: }
On 2013/07/17 09:12:03, fwereade wrote:
> Not actually necessary to use the export_test mechanism here. What with the
> "main" package being weird, all the tests are in-package anyway.
> 
> Maybe we *should* be using it regardless, but if we do this we should move all
> the test-specific bits in here at once.

For now, I removed the export_test