juju-core

Merge lp://staging/~fwereade/juju-core/environs-rework-instance-launch into lp://staging/~juju/juju-core/trunk

  1. environs-rework-instance-launch
  2. Merge into trunk
Proposed by William Reade
Status: Work in progress
Proposed branch: lp://staging/~fwereade/juju-core/environs-rework-instance-launch
Merge into: lp://staging/~juju/juju-core/trunk
Prerequisite: lp://staging/~fwereade/juju-core/config-default-agent-version
Diff against target: 1198 lines (+413/-330)
12 files modified
environs/bootstrap.go (+194/-40)
environs/cloudinit/cloudinit.go (+0/-6)
environs/config.go (+0/-22)
environs/ec2/ec2.go (+42/-139)
environs/ec2/image.go (+12/-3)
environs/ec2/live_test.go (+9/-10)
environs/ec2/local_test.go (+2/-2)
environs/instance.go (+55/-0)
environs/interface.go (+24/-27)
environs/jujutest/livetests.go (+33/-35)
environs/jujutest/tests.go (+37/-24)
worker/provisioner/provisioner.go (+5/-22)
To merge this branch: bzr merge lp://staging/~fwereade/juju-core/environs-rework-instance-launch
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+150286@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/7394048/

To post a comment you must log in.
Revision history for this message
Dimiter Naydenov (dimitern) wrote :

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#newcode148
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.go#newcode26
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.

https://codereview.appspot.com/7394048/

Reply
Revision history for this message
Roger Peppe (rogpeppe) wrote :

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.

https://codereview.appspot.com/7394048/

Reply
Revision history for this message
William Reade (fwereade) wrote :
Download full text (6.2 KiB)

Reviewers: mp+150286_code.launchpad.net, dimitern, rog,

Message:
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 ne...

Read more...

Reply

Unmerged revisions

929. By William Reade

fix ec2 to work with new Environ interface

928. By William Reade

AgentVersion now defaults to the current version

927. By William Reade

move agent-relevant tools code into environs/agent

926. By William Reade

add arch constraint

925. By William Reade

test machines that will be used with environs now specify realistic series

924. By Dave Cheney

version: update development version to 1.9.10

Also, gofmt

R=
CC=
https://codereview.appspot.com/7400051

923. By Frank Mueller

local: add storage

Added environ.Storage implementation using the local
storage backend. The tests so far are a copy from
environs/jujutest to check if the behavior is adequate.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7346052

922. By Dimiter Naydenov

uniter: WantAllRelationsEvents() filter

This is the first CL in preparation for the
implementation of upgrade-charm. Here, we'll
prepare the Uniter to handle upgrades better,
by restarting the relations watcher in the
filter, when requested with WantAllRelationsEvents().

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

921. By Frank Mueller

local: added storage backend

Added a small web server acting as storage backend.

R=fwereade, dimitern
CC=
https://codereview.appspot.com/7303097

920. By Brad Crittenden

Refactor guts of cmd/juju/get/Run to statecmd.

This is the first of two branches to provide API support for 'juju get'. This
one does the preliminary work of moving the common code to statecmd. The next
branch will add support in client and apiserver.

Did some drive-by changes including typo fixes that were visible in output and
added some content to the README.

R=dfc, jameinel, dimitern, fwereade
CC=
https://codereview.appspot.com/7401043

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches