environs: use new tool-finding funcs

environs: use new tool-finding funcs

...and defer tool choice to the last moment, along with a whole bunch of
other common machine-configuration logic. This resulted directly in:

  * new environs.FinishMachineConfig, that configures a MachineConfig
  	according to the contents of an environment configuration;
  * expected, simple, but invasive changes to ec2, openstack, maas, and
  	dummy provider;
  * a slimmed-down environs.Bootstrap (just checks predictable errors);
  * a serious reshuffling of the cmd/juju.Bootstrap tests;
  * minor tweaks here and there to other tests;

...and indirectly, but usefully:

  * --upload-tools versions are now always dev

We can't quite trash the old code yet: sync-tools still uses it. That's
coming next.

https://code.launchpad.net/~fwereade/juju-core/environs-tools-provisioning-integration/+merge/158805

Requires: https://code.launchpad.net/~fwereade/juju-core/environs-tools-provisioning/+merge/158780

(do not edit description out of merge proposal)

[fwereade, 2013-04-14 19:19:23]

Please take a look.

[rog, 2013-04-15 12:28:08]

some preliminary comments.

a fantastic direction in general, but FinishMachineConfig in particular could do
with some work, i think.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:64: type bootstrapTest struct {
i'd quite like to see comments on at least some of these.
here are two i had to stare at a bit longer than i wanted:

// version sets version.Current if non-empty.
version string

// uploads holds all the tools versions to be
// uploaded to the environment's private storage.
// If empty, tools the same as version.Current tools
// but with the environment's default series are
// uploaded.
uploads []string

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:133: var bootstrapTests = []bootstrapTest{{
nice to have this table-driven.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/upgradejuju.go#newcode272
cmd/juju/upgradejuju.go:272: vers.Build += 1
++ ?

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode11
environs/bootstrap.go:11: func Bootstrap(environ Environ, cons
constraints.Value) error {
i'm not sure why we still have this function any more.
it was created entirely so that the certificate generation
was factored out into common code, but since that's now
in cloudinit, clients might as well just call Bootstrap
directly.

i appreciate that that doing that rename would be a hassle, and we might
yet find another use for code that wraps environ.Bootstrap,
but i'd just do:

func Bootstrap(environ Environ, cons constraints.Value) error {
    return environs.Bootstrap(cons)
}

and leave it to the environment itself to do whatever validation it requires -
in particular, cloudinit.FinishMachineConfig already checks all these things.

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode14
environs/bootstrap.go:14: return fmt.Errorf("environment configuration missing
admin-secret")
s/missing/is missing/ ?

or

s/missing/has no/

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode21
environs/bootstrap.go:21: // we'll be here to catch this problem early.
i don't see the point of this.
if we make authorized-keys optional in the config, then it may well be because
we want to allow people to bootstrap without them. this if statement just adds
some speculative knowledge to this code without any particular reason AFAICS.
this code itself does not care about authorized-keys one way or the other.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode17
environs/cloudinit.go:17: // inspecting a plain config.Config.
why are the constraints an argument to this function? they don't interact with
the config.Config, so don't really seem to pertain to the description above.

ISTM that providers can equally well fill out mcfg.Constraints for themselves.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode18
environs/cloudinit.go:18: func FinishMachineConfig(mcfg
*cloudinit.MachineConfig, cfg *config.Config, cons constraints.Value) (err
error) {
nice functionality but i'm not keen on the name.

i'm wondering why all these fields are in MachineConfig at all if we can work
them out from the config.Config, which the MachineConfig already has.

why not just make this function internal to environs/cloudinit and call it
within Configure?

then we can remove all the fields that this function sets from the
MachineConfig.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode43
environs/cloudinit.go:43: mcfg.APIInfo = &api.Info{Password: passwordHash,
CACert: caCert}
this is erasing the APIInfo from the machine config passed in.

i realise that when StateServer is true, we don't actually use it, but it still
seems icky.

i'm tending more and more to the view that MachineConfig.StateServer should
really be MachineConfig.Bootstrap, BTW.

https://codereview.appspot.com/8726044/diff/1/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/config.go#newcode153
environs/config.go:153: // BootstrapConfig returns a copy of the supplied
configuration with
// BootstrapConfig returns a copy of the supplied configuration
// suitable for passing to Bootstrap.
// It removes secret attributes and verifies that
// the configuration is suitable.

?

https://codereview.appspot.com/8726044/diff/1/environs/config.go#newcode177
environs/config.go:177: panic("cannot create bootstrap config without an
agent-version")
i don't think the panic is warranted here - i'd just return an error.

alternatively document the fact that it panics.

[rog, 2013-04-15 13:24:10]

rest of review below.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode406
environs/ec2/ec2.go:406: panic("series should have been chosen by now")
i'm not keen on this panic.
knowledge of why it's ok to panic here is encoded somewhere in
FindBootstrapTools, and it's not immediately clear why that should always return
tools with exactly one series.

i'd return an error rather than panicking, or at the least put the series check
right next to the FindBootstrapTools call.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode406
environs/ec2/ec2.go:406: panic("series should have been chosen by now")
BTW is it really worth having a log.Errorf message as well as panic? if we're
not logging panic messages, we're doing something wrong.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode420
environs/ec2/ec2.go:420: log.Errorf("expected arch in %v; got %v", arches,
spec.image.arch)
log.Errorf("found unexpected architecture %v; expected one of %v",
spec.image.arch, arches)

?

and again, i'd just return an error rather than panicking.

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

https://codereview.appspot.com/8726044/diff/1/environs/interface.go#newcode150
environs/interface.go:150: Bootstrap(cons constraints.Value) error
Bootstrap is getting simpler again, yay!

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

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:291: if len(series) != 1 {
please no panic

https://codereview.appspot.com/8726044/diff/1/environs/maas/maas_test.go
File environs/maas/maas_test.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/maas/maas_test.go#newc...
environs/maas/maas_test.go:15: testing.LoggingSuite
+1

https://codereview.appspot.com/8726044/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (left):

https://codereview.appspot.com/8726044/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:753: // TODO(fwereade): use scfg.constraints to
pick instance spec before
nice to see this go

[dimitern, 2013-04-15 17:27:23]

I like there this is going, esp. removing redundant code; mostly trivial
suggestions. But I have some questions and concerns:

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:73: func (test bootstrapTest) run(c *C) {
I like the interesting approach - run being a method.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:99: c.Logf("found: " + list.String())
move line this after the c.Check please.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:130: 
d

https://codereview.appspot.com/8726044/diff/1/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/main_test.go#newcode167
cmd/juju/main_test.go:167: // breakJuju writes a dummy environment with
incomplete configuration.
I think this is wrong - it removes the possibility to break intentionally a
dummy provider method to return an error.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit_test.go#newc...
environs/cloudinit_test.go:60: cons := constraints.MustParse("mem=1T
cpu-power=999999999")
why such huge values? i don't see the point, unless you fail somewhere because
they cannot be satisfied.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode405
environs/ec2/ec2.go:405: log.Errorf("expected one series; got %v", series)
environs/ec2: ...

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode420
environs/ec2/ec2.go:420: log.Errorf("expected arch in %v; got %v", arches,
spec.image.arch)
environs/ec2: ...

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

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:120: log.Errorf("cannot release failed bootstrap
instance: %v", err)
environs/maas: ...

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:292: log.Errorf("expected one series; got %v", series)
environs/maas: ...

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:304: if err := environ.releaseInstance(instance); err
!= nil {
you're losing the original error here, isn't that a problem?

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:305: log.Errorf("error releasing failed instance: %v",
err)
environs/maas: ...

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go
File environs/maas/environ_test.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go#n...
environs/maas/environ_test.go:267: instance, err = env.StartInstance("2",
"xxxxx", series, constraints.Value{}, stateInfo, apiInfo)
s/xxxxx/fake_nonce/

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go#n...
environs/maas/environ_test.go:270: c.Check(err, FitsTypeOf,
(*environs.NotFoundError)(nil))
shouldn't this be c.Check(environs.IsNotFound(err), Equals, true) instead?

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go#n...
environs/maas/environ_test.go:365: c.Check(err, FitsTypeOf,
(*environs.NotFoundError)(nil))
ditto

[fwereade, 2013-04-16 07:39:13]

Please take a look.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:64: type bootstrapTest struct {
On 2013/04/15 12:28:09, rog wrote:
> i'd quite like to see comments on at least some of these.
> here are two i had to stare at a bit longer than i wanted:
> 
> // version sets version.Current if non-empty.
> version string
> 
> // uploads holds all the tools versions to be
> // uploaded to the environment's private storage.
> // If empty, tools the same as version.Current tools
> // but with the environment's default series are
> // uploaded.
> uploads []string

Good idea, thanks.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:73: func (test bootstrapTest) run(c *C) {
On 2013/04/15 17:27:23, dimitern wrote:
> I like the interesting approach - run being a method.

Yeah, I was wondering whether it'd draw comment. Having the code right next to
the data definition is definitely a boon, and it's nice to be able to defer
without thinking; I'll let it settle and see if I still like it next week.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:99: c.Logf("found: " + list.String())
On 2013/04/15 17:27:23, dimitern wrote:
> move line this after the c.Check please.

Done.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:130: 
On 2013/04/15 17:27:23, dimitern wrote:
> d

Done.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/main_test.go#newcode167
cmd/juju/main_test.go:167: // breakJuju writes a dummy environment with
incomplete configuration.
On 2013/04/15 17:27:23, dimitern wrote:
> I think this is wrong - it removes the possibility to break intentionally a
> dummy provider method to return an error.

If anyone really wants to do that, they still can. But there's no need to go
through all that dummy-provider-breakage malarkey (which still exists, and can
still be used) when we just want to make sure that the command fails at Run
time.

https://codereview.appspot.com/8726044/diff/1/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/upgradejuju.go#newcode272
cmd/juju/upgradejuju.go:272: vers.Build += 1
On 2013/04/15 12:28:09, rog wrote:
> ++ ?

Why not, eh?

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode11
environs/bootstrap.go:11: func Bootstrap(environ Environ, cons
constraints.Value) error {
On 2013/04/15 12:28:09, rog wrote:
> i'm not sure why we still have this function any more.
> it was created entirely so that the certificate generation
> was factored out into common code, but since that's now
> in cloudinit, clients might as well just call Bootstrap
> directly.
> 
> i appreciate that that doing that rename would be a hassle, and we might
> yet find another use for code that wraps environ.Bootstrap,
> but i'd just do:
> 
> func Bootstrap(environ Environ, cons constraints.Value) error {
>     return environs.Bootstrap(cons)
> }
> 
> and leave it to the environment itself to do whatever validation it requires -
> in particular, cloudinit.FinishMachineConfig already checks all these things.

IMO there's a lot of benefit to having consistent errors for predictable
failures rather than leaving it to the providers to detect or wrap problems in
their own arbitrary way. It's definitely nice to know there's a problem with the
config *before* the N roundtrips involved in figuring out what to run, and what
to run it on, etc.

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode14
environs/bootstrap.go:14: return fmt.Errorf("environment configuration missing
admin-secret")
On 2013/04/15 12:28:09, rog wrote:
> s/missing/is missing/ ?
> 
> or
> 
> s/missing/has no/

Done.

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode21
environs/bootstrap.go:21: // we'll be here to catch this problem early.
On 2013/04/15 12:28:09, rog wrote:
> i don't see the point of this.
> if we make authorized-keys optional in the config, then it may well be because
> we want to allow people to bootstrap without them. this if statement just adds
> some speculative knowledge to this code without any particular reason AFAICS.
> this code itself does not care about authorized-keys one way or the other.

Authorized-keys *are* optional in the config. *But* it's impossible to construct
a config without them, and this fact is not immediately obvious: this means it's
easy to "fix" that and make people's broken bootstrap nodes inaccessible. I
think that avoiding that is worth a speculative if and a comment explaining it.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode17
environs/cloudinit.go:17: // inspecting a plain config.Config.
On 2013/04/15 12:28:09, rog wrote:
> why are the constraints an argument to this function? they don't interact with
> the config.Config, so don't really seem to pertain to the description above.
> 
> ISTM that providers can equally well fill out mcfg.Constraints for themselves.

If it's a param somewhere, it's a lot harder to forget to do it.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode18
environs/cloudinit.go:18: func FinishMachineConfig(mcfg
*cloudinit.MachineConfig, cfg *config.Config, cons constraints.Value) (err
error) {
On 2013/04/15 12:28:09, rog wrote:
> nice functionality but i'm not keen on the name.
> 
> i'm wondering why all these fields are in MachineConfig at all if we can work
> them out from the config.Config, which the MachineConfig already has.
> 
> why not just make this function internal to environs/cloudinit and call it
> within Configure?
> 
> then we can remove all the fields that this function sets from the
> MachineConfig.

Because MachineConfig is crack, as I maintained from the beginning. But I didn't
want to touch it if I didn't have to; and I didn't: so I didn't. I think this is
a step towards sanity, and I don't want to overcook this branch any further.
I'll comment it though.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode43
environs/cloudinit.go:43: mcfg.APIInfo = &api.Info{Password: passwordHash,
CACert: caCert}
On 2013/04/15 12:28:09, rog wrote:
> this is erasing the APIInfo from the machine config passed in.
> 
> i realise that when StateServer is true, we don't actually use it, but it
still
> seems icky.
> 
> i'm tending more and more to the view that MachineConfig.StateServer should
> really be MachineConfig.Bootstrap, BTW.

I can error out if it's already set, I guess.

re StateServer/Bootstrap, I keep wanting to do it too, but actually it's *both*,
and they're distinct concepts, and I prefer to vacillate until HA state makes
the right direction clear. I *think* you're right.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit_test.go#newc...
environs/cloudinit_test.go:60: cons := constraints.MustParse("mem=1T
cpu-power=999999999")
On 2013/04/15 17:27:23, dimitern wrote:
> why such huge values? i don't see the point, unless you fail somewhere because
> they cannot be satisfied.

Just for fun. I'm tired of requesting sensible constraints :).

https://codereview.appspot.com/8726044/diff/1/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/config.go#newcode153
environs/config.go:153: // BootstrapConfig returns a copy of the supplied
configuration with
On 2013/04/15 12:28:09, rog wrote:
> // BootstrapConfig returns a copy of the supplied configuration
> // suitable for passing to Bootstrap.
> // It removes secret attributes and verifies that
> // the configuration is suitable.
> 
> ?

Alternative done - reasonable?

https://codereview.appspot.com/8726044/diff/1/environs/config.go#newcode177
environs/config.go:177: panic("cannot create bootstrap config without an
agent-version")
On 2013/04/15 12:28:09, rog wrote:
> i don't think the panic is warranted here - i'd just return an error.
> 
> alternatively document the fact that it panics.

Fair enough.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode406
environs/ec2/ec2.go:406: panic("series should have been chosen by now")
On 2013/04/15 13:24:10, rog wrote:
> BTW is it really worth having a log.Errorf message as well as panic? if we're
> not logging panic messages, we're doing something wrong.

Yeah, maybe not. Seemed like a good idea at the time.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode406
environs/ec2/ec2.go:406: panic("series should have been chosen by now")
On 2013/04/15 13:24:10, rog wrote:
> i'm not keen on this panic.
> knowledge of why it's ok to panic here is encoded somewhere in
> FindBootstrapTools, and it's not immediately clear why that should always
return
> tools with exactly one series.
> 
> i'd return an error rather than panicking, or at the least put the series
check
> right next to the FindBootstrapTools call.

I'm conflicted. I think that FindBootstrapSeries (and the other "input",
FindInstanceTools) are not actually relevant here: it's convenient they do each
return just one series, but this is a method for starting a specific instance
and the nature of juju indicates that if we haven't already picked a series by
now the programmer has screwed up.

So I kinda feel panic is more appropriate really, but not enough so to fight
about it.

https://codereview.appspot.com/8726044/diff/1/environs/ec2/ec2.go#newcode420
environs/ec2/ec2.go:420: log.Errorf("expected arch in %v; got %v", arches,
spec.image.arch)
On 2013/04/15 13:24:10, rog wrote:
> log.Errorf("found unexpected architecture %v; expected one of %v",
> spec.image.arch, arches)
> 
> ?
> 
> and again, i'd just return an error rather than panicking.

I think this is an even clearer-cut case of "obvious programmer error" but,
yeah, panics don't make me very happy either.

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

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:120: log.Errorf("cannot release failed bootstrap
instance: %v", err)
On 2013/04/15 17:27:23, dimitern wrote:
> environs/maas: ...

Done.

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:291: if len(series) != 1 {
On 2013/04/15 13:24:10, rog wrote:
> please no panic

Same waffling applies. Done.

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:304: if err := environ.releaseInstance(instance); err
!= nil {
On 2013/04/15 17:27:23, dimitern wrote:
> you're losing the original error here, isn't that a problem?

Am I? http://play.golang.org/

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ.go#newcod...
environs/maas/environ.go:305: log.Errorf("error releasing failed instance: %v",
err)
On 2013/04/15 17:27:23, dimitern wrote:
> environs/maas: ...

Done.

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go
File environs/maas/environ_test.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go#n...
environs/maas/environ_test.go:267: instance, err = env.StartInstance("2",
"xxxxx", series, constraints.Value{}, stateInfo, apiInfo)
On 2013/04/15 17:27:23, dimitern wrote:
> s/xxxxx/fake_nonce/

Done.

https://codereview.appspot.com/8726044/diff/1/environs/maas/environ_test.go#n...
environs/maas/environ_test.go:270: c.Check(err, FitsTypeOf,
(*environs.NotFoundError)(nil))
On 2013/04/15 17:27:23, dimitern wrote:
> shouldn't this be c.Check(environs.IsNotFound(err), Equals, true) instead?

Don't think we have one of those. Pondered tweaking it, decided I had bigger
fish to fry.

[dimitern, 2013-04-16 11:36:15]

LGTM, thanks for the changes. One comment though:

https://codereview.appspot.com/8726044/diff/1/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/8726044/diff/1/cmd/juju/main_test.go#newcode167
cmd/juju/main_test.go:167: // breakJuju writes a dummy environment with
incomplete configuration.
On 2013/04/16 07:39:13, fwereade wrote:
> On 2013/04/15 17:27:23, dimitern wrote:
> > I think this is wrong - it removes the possibility to break intentionally a
> > dummy provider method to return an error.
> 
> If anyone really wants to do that, they still can. But there's no need to go
> through all that dummy-provider-breakage malarkey (which still exists, and can
> still be used) when we just want to make sure that the command fails at Run
> time.

Then drop the environMethod arg please, because it's not used anymore.

[rog, 2013-04-16 12:22:12]

LGTM with murmured grunts of discomfort.

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode11
environs/bootstrap.go:11: func Bootstrap(environ Environ, cons
constraints.Value) error {
On 2013/04/16 07:39:13, fwereade wrote:
> On 2013/04/15 12:28:09, rog wrote:
> > i'm not sure why we still have this function any more.
> > it was created entirely so that the certificate generation
> > was factored out into common code, but since that's now
> > in cloudinit, clients might as well just call Bootstrap
> > directly.
> > 
> > i appreciate that that doing that rename would be a hassle, and we might
> > yet find another use for code that wraps environ.Bootstrap,
> > but i'd just do:
> > 
> > func Bootstrap(environ Environ, cons constraints.Value) error {
> >     return environs.Bootstrap(cons)
> > }
> > 
> > and leave it to the environment itself to do whatever validation it requires
-
> > in particular, cloudinit.FinishMachineConfig already checks all these
things.
> 
> IMO there's a lot of benefit to having consistent errors for predictable
> failures rather than leaving it to the providers to detect or wrap problems in
> their own arbitrary way. It's definitely nice to know there's a problem with
the
> config *before* the N roundtrips involved in figuring out what to run, and
what
> to run it on, etc.

it's a trade-off. this is unnecessarily duplicated code bloat - it adds a
maintenance burden for not that much benefit AFAICS and means we're baking in
assumptions in places where they're not necessary.

i feel we're over-thinking this to the detriment of the code base.

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode21
environs/bootstrap.go:21: // we'll be here to catch this problem early.
On 2013/04/16 07:39:13, fwereade wrote:
> On 2013/04/15 12:28:09, rog wrote:
> > i don't see the point of this.
> > if we make authorized-keys optional in the config, then it may well be
because
> > we want to allow people to bootstrap without them. this if statement just
adds
> > some speculative knowledge to this code without any particular reason
AFAICS.
> > this code itself does not care about authorized-keys one way or the other.
> 
> Authorized-keys *are* optional in the config. *But* it's impossible to
construct
> a config without them, and this fact is not immediately obvious: this means
it's
> easy to "fix" that and make people's broken bootstrap nodes inaccessible. I
> think that avoiding that is worth a speculative if and a comment explaining
it.

the check is already done in FinishMachineConfig. *two* speculative if
statmements seems like overkill to me.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode17
environs/cloudinit.go:17: // inspecting a plain config.Config.
On 2013/04/16 07:39:13, fwereade wrote:
> On 2013/04/15 12:28:09, rog wrote:
> > why are the constraints an argument to this function? they don't interact
with
> > the config.Config, so don't really seem to pertain to the description above.
> > 
> > ISTM that providers can equally well fill out mcfg.Constraints for
themselves.
> 
> If it's a param somewhere, it's a lot harder to forget to do it.

there are plenty of other fields in MachineConfig that are mandatory.
why do the constraints get special treatment?

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode43
environs/cloudinit.go:43: mcfg.APIInfo = &api.Info{Password: passwordHash,
CACert: caCert}
On 2013/04/16 07:39:13, fwereade wrote:
> On 2013/04/15 12:28:09, rog wrote:
> > this is erasing the APIInfo from the machine config passed in.
> > 
> > i realise that when StateServer is true, we don't actually use it, but it
> still
> > seems icky.
> > 
> > i'm tending more and more to the view that MachineConfig.StateServer should
> > really be MachineConfig.Bootstrap, BTW.
> 
> I can error out if it's already set, I guess.
> 
> re StateServer/Bootstrap, I keep wanting to do it too, but actually it's
*both*,
> and they're distinct concepts, and I prefer to vacillate until HA state makes
> the right direction clear. I *think* you're right.

yes, they're distinct concepts, and we only implement Bootstrap currently, hence
my inclination to change the name.

https://codereview.appspot.com/8726044/diff/8002/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/8726044/diff/8002/environs/cloudinit.go#newcode21
environs/cloudinit.go:21: // or otherwise).
please document the fields that this function sets, otherwise someone writing a
provider cannot know which fields in the MachineConfig they are expected to set
without delving into the source and hoping they got it right.

[fwereade, 2013-04-17 06:40:55]

Roger, you seem to be mistaking the purpose of this branch. It is *not* to fix
MachineConfig, it is to *use* the pre-existing MachineConfig despite its flaws
and make it slightly easier for developers to do the Right Thing. On that basis
I've also reverted FindBootstrapTools's name because, on reflection,
EnsureAgentVersion is actually worse: agent-version should be ensured, but I
think it should be done *before* we pass the config into bootstrap. That'll be a
followup though.

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode11
environs/bootstrap.go:11: func Bootstrap(environ Environ, cons
constraints.Value) error {
On 2013/04/16 12:22:12, rog wrote:
> it's a trade-off. this is unnecessarily duplicated code bloat - it adds a
> maintenance burden for not that much benefit AFAICS and means we're baking in
> assumptions in places where they're not necessary.
> 
> i feel we're over-thinking this to the detriment of the code base.
> 

Just yesterday you were complaining about a change in bootstrap ordering that
caused us to do work before discovering that work was meaningless. Is it your
position that we *should* check knowably bogus data before acting on it, or that
we should *not*?

https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode21
environs/bootstrap.go:21: // we'll be here to catch this problem early.
On 2013/04/16 12:22:12, rog wrote:
> On 2013/04/16 07:39:13, fwereade wrote:
> > On 2013/04/15 12:28:09, rog wrote:
> > > i don't see the point of this.
> > > if we make authorized-keys optional in the config, then it may well be
> because
> > > we want to allow people to bootstrap without them. this if statement just
> adds
> > > some speculative knowledge to this code without any particular reason
> AFAICS.
> > > this code itself does not care about authorized-keys one way or the other.
> > 
> > Authorized-keys *are* optional in the config. *But* it's impossible to
> construct
> > a config without them, and this fact is not immediately obvious: this means
> it's
> > easy to "fix" that and make people's broken bootstrap nodes inaccessible. I
> > think that avoiding that is worth a speculative if and a comment explaining
> it.
> 
> the check is already done in FinishMachineConfig. *two* speculative if
> statmements seems like overkill to me.

Machine configuration is so incoherent that two checks -- one for knowably bad
input data, and one for bad final output -- seems to me like the absolute
minimum necessary. If I could do one on StartInstance I would, and if I could
make userdata generation common I would do that as well. But I'm trying to
maintain focus here with a *minimum* of structural changes.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode17
environs/cloudinit.go:17: // inspecting a plain config.Config.
On 2013/04/16 12:22:12, rog wrote:
> On 2013/04/16 07:39:13, fwereade wrote:
> > On 2013/04/15 12:28:09, rog wrote:
> > > why are the constraints an argument to this function? they don't interact
> with
> > > the config.Config, so don't really seem to pertain to the description
above.
> > > 
> > > ISTM that providers can equally well fill out mcfg.Constraints for
> themselves.
> > 
> > If it's a param somewhere, it's a lot harder to forget to do it.
> 
> there are plenty of other fields in MachineConfig that are mandatory.
> why do the constraints get special treatment?

Because they were bound up with the code I was writing, and I wasn't going to do
them badly just because other things are done badly too.

https://codereview.appspot.com/8726044/diff/1/environs/cloudinit.go#newcode43
environs/cloudinit.go:43: mcfg.APIInfo = &api.Info{Password: passwordHash,
CACert: caCert}
On 2013/04/16 12:22:12, rog wrote:
> On 2013/04/16 07:39:13, fwereade wrote:
> > On 2013/04/15 12:28:09, rog wrote:
> > > this is erasing the APIInfo from the machine config passed in.
> > > 
> > > i realise that when StateServer is true, we don't actually use it, but it
> > still
> > > seems icky.
> > > 
> > > i'm tending more and more to the view that MachineConfig.StateServer
should
> > > really be MachineConfig.Bootstrap, BTW.
> > 
> > I can error out if it's already set, I guess.
> > 
> > re StateServer/Bootstrap, I keep wanting to do it too, but actually it's
> *both*,
> > and they're distinct concepts, and I prefer to vacillate until HA state
makes
> > the right direction clear. I *think* you're right.
> 
> yes, they're distinct concepts, and we only implement Bootstrap currently,
hence
> my inclination to change the name.

It's stupid and (probably) wrong, but it is exactly as stupid and wrong as it
was before, and leaving it alone makes very clear that this change is about
fixing the inputs, not fixing MachineConfig itself. If I were to start changing
MachineConfig I wouldn't be able to stop.

https://codereview.appspot.com/8726044/diff/8002/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/8726044/diff/8002/environs/cloudinit.go#newcode21
environs/cloudinit.go:21: // or otherwise).
On 2013/04/16 12:22:12, rog wrote:
> please document the fields that this function sets, otherwise someone writing
a
> provider cannot know which fields in the MachineConfig they are expected to
set
> without delving into the source and hoping they got it right.

This is exactly as it was before, and documenting it here won't change that.

[fwereade, 2013-04-17 06:43:04]

*** Submitted:

environs: use new tool-finding funcs

...and defer tool choice to the last moment, along with a whole bunch of
other common machine-configuration logic. This resulted directly in:

  * new environs.FinishMachineConfig, that configures a MachineConfig
  	according to the contents of an environment configuration;
  * expected, simple, but invasive changes to ec2, openstack, maas, and
  	dummy provider;
  * a slimmed-down environs.Bootstrap (just checks predictable errors);
  * a serious reshuffling of the cmd/juju.Bootstrap tests;
  * minor tweaks here and there to other tests;

...and indirectly, but usefully:

  * --upload-tools versions are now always dev

We can't quite trash the old code yet: sync-tools still uses it. That's
coming next.

R=rog, dimitern
CC=
https://codereview.appspot.com/8726044

[rog, 2013-04-17 09:29:30]

On 17 April 2013 07:40,  <fwereade@gmail.com> wrote:
> Roger, you seem to be mistaking the purpose of this branch. It is *not*
> to fix MachineConfig, it is to *use* the pre-existing MachineConfig
> despite its flaws and make it slightly easier for developers to do the
> Right Thing. On that basis I've also reverted FindBootstrapTools's name
> because, on reflection, EnsureAgentVersion is actually worse:
> agent-version should be ensured, but I think it should be done *before*
> we pass the config into bootstrap. That'll be a followup though.
>
>
>
> https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go
> File environs/bootstrap.go (right):
>
> https://codereview.appspot.com/8726044/diff/1/environs/bootstrap.go#newcode11
> environs/bootstrap.go:11: func Bootstrap(environ Environ, cons
> constraints.Value) error {
> On 2013/04/16 12:22:12, rog wrote:
>>
>> it's a trade-off. this is unnecessarily duplicated code bloat - it
>
> adds a
>>
>> maintenance burden for not that much benefit AFAICS and means we're
>
> baking in
>>
>> assumptions in places where they're not necessary.
>
>
>> i feel we're over-thinking this to the detriment of the code base.
>
>
>
> Just yesterday you were complaining about a change in bootstrap ordering
> that caused us to do work before discovering that work was meaningless.
> Is it your position that we *should* check knowably bogus data before
> acting on it, or that we should *not*?

I think there's no one right answer there - it's a compromise.

I don't mind too much about the error case - if you've got a bogus
config and it takes a little extra time to tell you that, I don't think
it's a terrible thing.

>> the check is already done in FinishMachineConfig. *two* speculative if
>> statmements seems like overkill to me.
>
>
> Machine configuration is so incoherent that two checks -- one for
> knowably bad input data, and one for bad final output -- seems to me
> like the absolute minimum necessary.

I'm not sure I understand you here. All the checks are on the input, no?
And we can't check for bad final output as we can't run the shell
script before it gets sent to the remote machine.

>> > If it's a param somewhere, it's a lot harder to forget to do it.
>
>
>> there are plenty of other fields in MachineConfig that are mandatory.
>> why do the constraints get special treatment?
>
>
> Because they were bound up with the code I was writing, and I wasn't
> going to do them badly just because other things are done badly too.

MachineConfig seems to me to be a reasonable
way of specifying the parameters that go into generating the cloud-init
user data. All the fields are well documented and there are checks
that the fields are as expected. The constraints are just another
parameter.

You assert several times that MachineConfig is bad - I would be
interested in exploring
just why you think so, so we can move towards fixing it.

> This is exactly as it was before, and documenting it here won't change
> that.

It's *not* exactly as it was before. Beforehand, it was possible
to read the doc comments in MachineConfig and know exactly
what parameters you needed to pass to make a valid configuration.

Now you can't do that, as *some* of them (you can only tell which ones
by reading the code) are now expected to be filled out by FinishMachineConfig.