Providers are responsible for selecting tools

Providers are responsible for selecting tools

This change makes providers responsible for
selecting bootstrap tools, and ensuring they
are available in storage. The null provider is
updated to select the tools appropriate to the
bootstrap host. Other providers use a default
implementation in environs/bootstrap, which
has been extracted from cmd/juju/bootstrap.go.

One test fails due to a logging change. This is
fixed by https://codereview.appspot.com/14430064/.

Fixes #1236691

https://code.launchpad.net/~axwalk/juju-core/lp1236691-null-provider-default-series-take2/+merge/190032

(do not edit description out of merge proposal)

[axw, 2013-10-09 07:13:51]

Please take a look.

[fwereade, 2013-10-09 18:04:30]

On 2013/10/09 07:13:51, axw wrote:
> Please take a look.

I don't think this is the right approach, I'm afraid. The possibleTools arg
itself only exists because we couldn't justify refactoring any further towards
sanity given time pressure. Basically all this bootstrap stuff is inside out...
environs should generally know what tools they've got available anyway, and
using default-series to pick the bootstrap node's is pretty dumb and pointless
in the first place.

Ping me tomorrow, we can have a long rambling G+ and see what makes sense.

[axw, 2013-10-10 02:14:52]

On 2013/10/09 18:04:30, fwereade wrote:
> On 2013/10/09 07:13:51, axw wrote:
> > Please take a look.
> 
> I don't think this is the right approach, I'm afraid. The possibleTools arg
> itself only exists because we couldn't justify refactoring any further towards
> sanity given time pressure. Basically all this bootstrap stuff is inside
out...
> environs should generally know what tools they've got available anyway, and
> using default-series to pick the bootstrap node's is pretty dumb and pointless
> in the first place.

Fair enough. I started down the road of adding a "BootstrapSeries" method to
Environ, but started thinking it was a lot of change just for one provider.
But... perhaps this is the right thing to do. Once the "wire up prechecker"
branch is landed we'll have EnvironBase to add default behaviour to, to keep the
ugly down.

> Ping me tomorrow, we can have a long rambling G+ and see what makes sense.

Sure thing, will be around before the weekly meeting.

[axw, 2013-10-15 05:17:39]

Please take a look.

[fwereade, 2013-11-14 09:29:45]

Sorry this has taken so long. I'm a bit uncomfortable about the sheer amount of
stuff we do, and feel like there's something more complex than it needs to be...
can we chat about this? Maybe your tomorrow morning?

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/bootstra...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:17: var logger =
loggo.GetLogger("juju.environs.bootstrap")
Ha. Thanks.

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:56: newVersion, toolsList = toolsList.Newest()
I'm not 100% sure this is right, but that's because it's not clear where the
tools list comes from and what its properties are. Any reason we can't get it
inside here?

Dimiter's just landing a List.NewestCompatible method, that might be useful?

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/synctool...
File environs/bootstrap/synctools.go (right):

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/synctool...
environs/bootstrap/synctools.go:42: logger.Warningf("no tools found, so
attempting to build and upload new tools")
Does this work with released versions of the local provider? That needs to
upload tools, and would ideally do so *without* the juju.BootstrapComand level
hack.

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/synctool...
environs/bootstrap/synctools.go:117: // TODO(axw) have syncOrUpload return the
list of tools in the target, and use that.
There's something still a bit off here, in that the copy-from-local behaviour of
upload is conceptually a sync, but ESCOPE.

https://codereview.appspot.com/14433058/diff/5001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/5001/environs/manual/bootstrap.g...
environs/manual/bootstrap.go:39: HardwareCharacteristics
*instance.HardwareCharacteristics
I'll probably see it in a minute -- but when can they be known ahead of time ina
manual bootstrap?

https://codereview.appspot.com/14433058/diff/5001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/local/environ.go#n...
provider/local/environ.go:136: }
This seems to be repeated a lot. Can we pull it out into a common func?

https://codereview.appspot.com/14433058/diff/5001/provider/maas/environ_test.go
File provider/maas/environ_test.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/maas/environ_test....
provider/maas/environ_test.go:448: "agent-version":
version.Current.Number.String(),
That's what it should always choose anyway.

https://codereview.appspot.com/14433058/diff/5001/provider/null/config_test.go
File provider/null/config_test.go (left):

https://codereview.appspot.com/14433058/diff/5001/provider/null/config_test.g...
provider/null/config_test.go:26: gc.TestingT(t)
heh, were we double-running these?

https://codereview.appspot.com/14433058/diff/5001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/null/environ.go#ne...
provider/null/environ.go:56: // properly, by setting default-series dynamically.
We shouldn't set default-series based on the series the bootstrap node happens
to have.

https://codereview.appspot.com/14433058/diff/5001/provider/null/environ.go#ne...
provider/null/environ.go:119: HardwareCharacteristics: &hc,
Ah, ok. Earlier confusion makes sense now -- the question becomes, when are
those ever *not* set?

https://codereview.appspot.com/14433058/diff/5001/provider/null/suite_test.go
File provider/null/suite_test.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/null/suite_test.go...
provider/null/suite_test.go:13: gc.TestingT(t)
ah, I see. much nicer, thanks

[axw, 2013-11-15 03:51:47]

Please take a look.

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/bootstra...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:56: newVersion, toolsList = toolsList.Newest()
On 2013/11/14 09:29:45, fwereade wrote:
> I'm not 100% sure this is right, but that's because it's not clear where the
> tools list comes from and what its properties are. Any reason we can't get it
> inside here?
> 
> Dimiter's just landing a List.NewestCompatible method, that might be useful?

Practically, the list will always come from EnsureToolsAvailabilty. That
function uses environs.FindBootstrapTools, which returns a list of tools with an
appropriate version.

I'm not sure that NewestCompatible is appropriate here.

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/synctool...
File environs/bootstrap/synctools.go (right):

https://codereview.appspot.com/14433058/diff/5001/environs/bootstrap/synctool...
environs/bootstrap/synctools.go:42: logger.Warningf("no tools found, so
attempting to build and upload new tools")
On 2013/11/14 09:29:45, fwereade wrote:
> Does this work with released versions of the local provider? That needs to
> upload tools, and would ideally do so *without* the juju.BootstrapComand level
> hack.

There's no change there. AFAICT, the hack in BootstrapCmd isn't necessary, but I
would like to run any change there by thumper, and preferably do it in another
CL.

For posterity: if the hack in BootstrapCmd goes, there's a change necessary in
provider/local/environprovider.go too, to enable auto-upload. Inside the Open
method, the bit that sets agent-version must be deleted.

https://codereview.appspot.com/14433058/diff/5001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/5001/environs/manual/bootstrap.g...
environs/manual/bootstrap.go:39: HardwareCharacteristics
*instance.HardwareCharacteristics
On 2013/11/14 09:29:45, fwereade wrote:
> I'll probably see it in a minute -- but when can they be known ahead of time
ina
> manual bootstrap?

"Ahead of time" here just means before the environs/manual.Bootstrap function is
called, not "ahead of bootstrap time".

The null/manual provider detects series/hardware in the provider's Bootstrap
method, and then passes it down. See provider/null/environ.go.

https://codereview.appspot.com/14433058/diff/5001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/local/environ.go#n...
provider/local/environ.go:136: }
On 2013/11/14 09:29:45, fwereade wrote:
> This seems to be repeated a lot. Can we pull it out into a common func?

Done. I hadn't done this before, as I didn't want to hide what was happening. If
you think of a better name that describes what's happening underneath, I'd love
to hear it.

https://codereview.appspot.com/14433058/diff/5001/provider/maas/environ_test.go
File provider/maas/environ_test.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/maas/environ_test....
provider/maas/environ_test.go:448: "agent-version":
version.Current.Number.String(),
On 2013/11/14 09:29:45, fwereade wrote:
> That's what it should always choose anyway.

This is just about disabling auto-upload, which requires agent-version not to be
set.

https://codereview.appspot.com/14433058/diff/5001/provider/null/config_test.go
File provider/null/config_test.go (left):

https://codereview.appspot.com/14433058/diff/5001/provider/null/config_test.g...
provider/null/config_test.go:26: gc.TestingT(t)
On 2013/11/14 09:29:45, fwereade wrote:
> heh, were we double-running these?

Just tidying up.

https://codereview.appspot.com/14433058/diff/5001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14433058/diff/5001/provider/null/environ.go#ne...
provider/null/environ.go:56: // properly, by setting default-series dynamically.
On 2013/11/14 09:29:45, fwereade wrote:
> We shouldn't set default-series based on the series the bootstrap node happens
> to have.

Oops, the comment and the fields are relics of my earlier attempt, and can be
deleted.

https://codereview.appspot.com/14433058/diff/5001/provider/null/environ.go#ne...
provider/null/environ.go:119: HardwareCharacteristics: &hc,
On 2013/11/14 09:29:45, fwereade wrote:
> Ah, ok. Earlier confusion makes sense now -- the question becomes, when are
> those ever *not* set?

At the moment, never. This is the one and only user of manual.Bootstrap, for
now.

[wallyworld, 2013-11-17 23:48:46]

In general I like that responsibility for setting up tools has been delegated to
the providers. My main concern is that, as implemented, the bootstrap is not
exiting early enough if the environment is already bootstrapped.

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:100: 
One implication of removing this check and running it as part of
bootstrap.Bootstrap() below is that we may well end up uploading tools only to
then tell the user "sorry, already bootstrapped". This check really does need to
be run asap in the bootstrap process, before we do anything, so that we fail as
early as possible.

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstra...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:50: // and update the agent-version
configuration attribute.
s/update/updates

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:51: func SelectBootstrapTools(environ
environs.Environ, toolsList coretools.List) (coretools.List, error) {
could we rename toolsList to availableTools or possibleTools to better reflect
the purpose of the parameter? callers of this method tend to pass in a
possibleTools variable so maybe that is the name to use

https://codereview.appspot.com/14433058/diff/9001/environs/manual/fakessh.go
File environs/manual/fakessh.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/manual/fakessh.go#...
environs/manual/fakessh.go:1: // Copyright 2013 Canonical Ltd.
Why did this file get renamed from fakessh_test.go?
The contents are only for testing. In general, we only use non "_test.go" files
for testing if there are methods called from outside the package (I don't think
that's the case here?); and if we do, we usually introduce a "testing" package
into which such files are put to make it clear that the contents are for testing
and not production business logic.

https://codereview.appspot.com/14433058/diff/9001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/testing/tools.go#n...
environs/testing/tools.go:42: sync.DefaultToolsLocation = c.MkDir() // stop sync
from going to s3
\o/

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.g...
provider/common/bootstrap.go:29: // before potentially uploading any tools.
Trouble is, we may already have uploaded tools prior to getting to this point.

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.g...
provider/common/bootstrap.go:76: func SetBootstrapTools(env environs.Environ,
series string, arch *string) (coretools.List, error) {
bikeshed - perhaps call this either SetupBootstrapTools or EnsureBootstrapTools.
I think I prefer the latter.

[axw, 2013-11-18 05:10:01]

Please take a look.

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:100: 
On 2013/11/17 23:48:46, wallyworld wrote:
> One implication of removing this check and running it as part of
> bootstrap.Bootstrap() below is that we may well end up uploading tools only to
> then tell the user "sorry, already bootstrapped". This check really does need
to
> be run asap in the bootstrap process, before we do anything, so that we fail
as
> early as possible.

There are a couple of nice things about putting it inside Bootstrap, one being
that Bootstrap fails if the machine is already bootstrapped (no need to test
that property via the CLI command). But you're right, we shouldn't break
behaviour for --upload-tools. If nothing else, it'll make developers' lives
harder.

I'll revert this.

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstra...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:50: // and update the agent-version
configuration attribute.
On 2013/11/17 23:48:46, wallyworld wrote:
> s/update/updates

Done.

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstra...
environs/bootstrap/bootstrap.go:51: func SelectBootstrapTools(environ
environs.Environ, toolsList coretools.List) (coretools.List, error) {
On 2013/11/17 23:48:46, wallyworld wrote:
> could we rename toolsList to availableTools or possibleTools to better reflect
> the purpose of the parameter? callers of this method tend to pass in a
> possibleTools variable so maybe that is the name to use

Done.

https://codereview.appspot.com/14433058/diff/9001/environs/manual/fakessh.go
File environs/manual/fakessh.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/manual/fakessh.go#...
environs/manual/fakessh.go:1: // Copyright 2013 Canonical Ltd.
On 2013/11/17 23:48:46, wallyworld wrote:
> Why did this file get renamed from fakessh_test.go?
> The contents are only for testing. In general, we only use non "_test.go"
files
> for testing if there are methods called from outside the package (I don't
think
> that's the case here?); and if we do, we usually introduce a "testing" package
> into which such files are put to make it clear that the contents are for
testing
> and not production business logic.

I had intended to use it in a separate package, but didn't end up doing that.
I'll revert this.

BTW: normally I'd put the test code in a separate package, but in this case it
needs to know some internals of the package (or otherwise we expose parts that
are even more private, such as the detection script).

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.g...
provider/common/bootstrap.go:29: // before potentially uploading any tools.
On 2013/11/17 23:48:46, wallyworld wrote:
> Trouble is, we may already have uploaded tools prior to getting to this point.

Fair enough, reverted this and related changes.

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.g...
provider/common/bootstrap.go:76: func SetBootstrapTools(env environs.Environ,
series string, arch *string) (coretools.List, error) {
On 2013/11/17 23:48:46, wallyworld wrote:
> bikeshed - perhaps call this either SetupBootstrapTools or
EnsureBootstrapTools.
> I think I prefer the latter.

EnsureBootstrapTools sounds decent, I'll go with that.

[wallyworld, 2013-11-18 05:48:33]

Thanks :-)
LGTM

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:100: 
On 2013/11/18 05:10:01, axw wrote:
> On 2013/11/17 23:48:46, wallyworld wrote:
> > One implication of removing this check and running it as part of
> > bootstrap.Bootstrap() below is that we may well end up uploading tools only
to
> > then tell the user "sorry, already bootstrapped". This check really does
need
> to
> > be run asap in the bootstrap process, before we do anything, so that we fail
> as
> > early as possible.
> 
> There are a couple of nice things about putting it inside Bootstrap, one being
> that Bootstrap fails if the machine is already bootstrapped (no need to test
> that property via the CLI command). But you're right, we shouldn't break
> behaviour for --upload-tools. If nothing else, it'll make developers' lives
> harder.
> 
> I'll revert this.

Yeah, I agree it's a bit unfortunate. It's not just developers also - people
like Dave and Marco regularly run from source to get the latest goodness.