Simpler startInstance & friends?

[William Reade]

On Tue, 2013-03-12 at 11:17 +0700, Jeroen Vermeulen wrote:
> Hi all,
> 
> Raphaël Badin and I have been looking at the startInstance() and
> userData() code in the ec2 and openstack providers.  It was pretty hard
> to figure out what goes on.  But having had to do that anyway, we think
> we have found a way to simplify it and make it more manageable.  I'm
> bringing it up here because somebody may find it useful.

I'm +100 on the spirit of these suggestions, but there are a few devils
in the details.

> The only call sites we are aware of for startInstance are Bootstrap()
> and StartInstance().  It looks to us as if this code could be simpler
> with 3 changes:
> 
> 1. The caller passes "tools" to startInstance(), but if tools is nil,
> startInstance() looks for tools.  This can only happen if the caller is
> StartInstance().  Moving the check in there enables other changes below.

We shouldn't be passing tools in at all: at the business logic level, we
should be dealing in terms of series and constraints. Tools can only be
chosen only once we've figured out what instance we need to run on: we
can't use them alone to specify the instance, but that's the effect of
the current interface. So, this definitely needs to change; and in fact
this change is scheduled (Tim Penhey is aiming to make the smallest
change possible to get tools out of the interface and series/constraints
in, and this should be happening soon.)

We *could* and probably should be using the list of *available* tools to
restrict what instances we choose -- no sense considering i386
images/instances if we only have amd64 tools available -- but this isn't
an overriding concern for me at this stage.

> 2. startInstance() can take a user-data blob as a parameter.  The caller
> first creates it by calling userData().
> 
> This takes away a lot of the information going into startInstance().
> The big differences between the two call sites are now at the call site,
> instead of inside startInstance().

IMO the internal startInstance calls should be part of the interface,
but the parameters should probably be rather different.

> 3. The startInstanceParams struct type is effectively a wrapper for
> cloudinit.MachineConfig.  Once it's no longer shared between
> startInstance() and userData(), it becomes easier just to use MachineConfig.

I tried something very much like this and decided it was a bad idea: I
don't want MachineConfig in the Environ interface at all, because it's
the wrong level of abstraction. All the work that goes into creating
that MachineConfig is currently performed way too early, I think:
there's *very* little in there that cannot be expressed with something
like:

type InstanceConfig struct {
    Series      string
    Constraints *state.Constraints
    MachineId   string
    Bootstrap   bool
}

...with a free function that can build a MachineConfig from that and an
Environ. (I may be missing a param or two, my attempt was a couple of
weeks ago now... but the incomplete sketch still lives at
https://codereview.appspot.com/7394048/ and may provide useful
inspiration.)

I'm finishing off my swap days today, but please hit me (fwereade) up in
irc for more discussion tomorrow if you're interested in exploring this
further. The costs of implementing a new provider are significant, and
the unhealthy mix of business logic and provider-specific work is a big
part of the problem; while we don't have space in core and blue to do
everything we'd like to, we'd really appreciate it if you guys would fix
what you can as you need to make your own work easier :).

> I don't know if anybody would be interested in trying this, but since we
> had to figure out the code anyway, we might as well share.

It's greatly appreciated, and I'm heartened to feel we're pretty much in
agreement about the problems and roughly aligned on the solutions.

Cheers
William

> 
> Jeroen
>