environs: rework instance creation

environs: rework instance creation

Bootstrap methods, and StartInstance-related methods, were hotbeds of
gradually-diverging non-provider-specific code. They're still pretty bad,
but the worst abuses have been factored out into environs.Bootstrap and
environs.StartInstance. Thoughts?

https://code.launchpad.net/~fwereade/juju-core/environs-rework-instance-launch/+merge/150286

Requires: https://code.launchpad.net/~fwereade/juju-core/config-default-agent-version/+merge/150248

(do not edit description out of merge proposal)

[dimitern, 2013-02-25 11:25:15]

Looks very good so far. So StartInstance is gone, LaunchInstance is the new
thing?

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode18
environs/bootstrap.go:18: // BootstrapParams contains the information required
to start a new
I like this!

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode97
environs/bootstrap.go:97: if err2 := e.StopInstances(insts); err2 != nil {
Nice to have err2, but I've been told repeatedly reusing the same err in nested
ifs is OK :)

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go#newcode233
environs/ec2/ec2.go:233: func (e *environ) IsBootstrapped() (bool, error) {
We should fix openstack and others eventually (probably in a follow-up though,
unless the build breaks, which is inevitable with interface changes).

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go#newcode310
environs/ec2/ec2.go:310: func (e *environ) GetInstanceSpec(series string, cons
state.Constraints) (environs.InstanceSpec, error) {
Comment?

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go#newcode344
environs/ec2/ec2.go:344: func (e *environ) LaunchInstance(spec
environs.InstanceSpec, mcfg *cloudinit.MachineConfig) (environs.Instance, error)
{
Comment?

https://codereview.appspot.com/7394048/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/interface.go#newcod...
environs/interface.go:148: IsBootstrapped() (bool, error)
Good to have this exposed.

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

https://codereview.appspot.com/7394048/diff/2001/environs/jujutest/livetests....
environs/jujutest/livetests.go:26: func StartInstance(e environs.Environ,
machineId string) (environs.Instance, error) {
Nice! About time to cut on the long list of mostly unused args.

[rog, 2013-02-25 11:32:54]

a few comments, incomplete - publishing just in the aid of continuing online
conversation.

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode44
environs/bootstrap.go:44: type WriteCACertFunc func(name string, cert, key
[]byte) error
i'm not entirely sure that this justifies its own type.

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode54
environs/bootstrap.go:54: if bootstrapped, err := e.IsBootstrapped(); err != nil
{
it's a pity that this makes the bootstrap process fundamentally racy (previously
it was possible for a given provider to implement Bootstrap in a race-free way).

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode71
environs/bootstrap.go:71: tools, err := PutTools(e.Storage(), nil)
this is slightly questionable, i think. previously, the storage methods in
Environ were not required to work before Bootstrap was called, so the Bootstrap
method was free to set up any storage required. we're changing that here.

i wonder if we should have a separate Environ.Init method which makes sure that
the environment is set up to be used. It could be that method that lets us know
if the environment is already bootstrapped.

Environ.PrepareForBootstrap, perhaps? then that would leave a provider free to
hold some kind of lock between PrepareForBootstrap and... when the environment
is bootstrapped, which i guess it doesn't know.

[fwereade, 2013-02-25 12:06:11]

a few thoughts

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode18
environs/bootstrap.go:18: // BootstrapParams contains the information required
to start a new
On 2013/02/25 11:25:15, dimitern wrote:
> I like this!

Cheers :).

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode44
environs/bootstrap.go:44: type WriteCACertFunc func(name string, cert, key
[]byte) error
On 2013/02/25 11:32:54, rog wrote:
> i'm not entirely sure that this justifies its own type.

I'ts not the type so much as it is the name, IYSWIM. Func types inside parameter
lists are prettier in  go than they are in C, but once you're past a single
param/result I find they become unreadable all too quickly anyway.

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode54
environs/bootstrap.go:54: if bootstrapped, err := e.IsBootstrapped(); err != nil
{
On 2013/02/25 11:32:54, rog wrote:
> it's a pity that this makes the bootstrap process fundamentally racy
(previously
> it was possible for a given provider to implement Bootstrap in a race-free
way).

In theory, yes. In practice, no provider worthy of the name actually did so; if
we really want, though, I think SetStateServers could be implemented such that
bootstrap could be made be non-racy (in a race-free environment). I don't think
it's a showstopper.

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode71
environs/bootstrap.go:71: tools, err := PutTools(e.Storage(), nil)
On 2013/02/25 11:32:54, rog wrote:
> this is slightly questionable, i think. previously, the storage methods in
> Environ were not required to work before Bootstrap was called, so the
Bootstrap
> method was free to set up any storage required. we're changing that here.
> 
> i wonder if we should have a separate Environ.Init method which makes sure
that
> the environment is set up to be used. It could be that method that lets us
know
> if the environment is already bootstrapped.
> 
> Environ.PrepareForBootstrap, perhaps? then that would leave a provider free to
> hold some kind of lock between PrepareForBootstrap and... when the environment
> is bootstrapped, which i guess it doesn't know.

How would you feel about separating tools storage from everything-else storage
then? This would let us drop all the confusing private/public malarkey and have
simple, configurable, Storage (which can keep those guarantees) and ToolsStorage
(which has to be set up beforehand, and can usually just default to a public
place).

Remember, the only use case for upload tools is for development environments: we
need to write some stuff to populate buckets with tools *anyway*, so why not
just untangle tools storage from environ storage?

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode97
environs/bootstrap.go:97: if err2 := e.StopInstances(insts); err2 != nil {
On 2013/02/25 11:25:15, dimitern wrote:
> Nice to have err2, but I've been told repeatedly reusing the same err in
nested
> ifs is OK :)

Good point, thanks.

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go#newcode233
environs/ec2/ec2.go:233: func (e *environ) IsBootstrapped() (bool, error) {
On 2013/02/25 11:25:15, dimitern wrote:
> We should fix openstack and others eventually (probably in a follow-up though,
> unless the build breaks, which is inevitable with interface changes).

This DESTROYS the build, I'm afraid -- the dummy provider can't handle it
either, and that takes down much of the rest of the codebase. Definitely not
finished, think of this as a preview for sanity-checking :).

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go#newcode310
environs/ec2/ec2.go:310: func (e *environ) GetInstanceSpec(series string, cons
state.Constraints) (environs.InstanceSpec, error) {
On 2013/02/25 11:25:15, dimitern wrote:
> Comment?

I'm not sure duplicating interface method comments is that valuable.

https://codereview.appspot.com/7394048/diff/2001/environs/ec2/ec2.go#newcode344
environs/ec2/ec2.go:344: func (e *environ) LaunchInstance(spec
environs.InstanceSpec, mcfg *cloudinit.MachineConfig) (environs.Instance, error)
{
On 2013/02/25 11:25:15, dimitern wrote:
> Comment?

Ditto

https://codereview.appspot.com/7394048/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/interface.go#newcod...
environs/interface.go:148: IsBootstrapped() (bool, error)
On 2013/02/25 11:25:15, dimitern wrote:
> Good to have this exposed.

Cheers

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

https://codereview.appspot.com/7394048/diff/2001/environs/jujutest/livetests....
environs/jujutest/livetests.go:26: func StartInstance(e environs.Environ,
machineId string) (environs.Instance, error) {
On 2013/02/25 11:25:15, dimitern wrote:
> Nice! About time to cut on the long list of mostly unused args.
Thanks :)