Bootstrap uses tools matching major.minor

Bootstrap uses tools matching major.minor

When finding tools to bootstrap with, we only used to look for
tools with the same major version as the cli client. This branch
tightens the check so that we find tools matching major.minor.

A refactoring is also included for find tools functionality. The
various find tools methods no longer take and env, and instead
are given storage instances to load from. The FindBootstrapTools
method is also cleaned up as a result so that it no longer both
looks for tools and sets agent version back into the config. So
the find tools methods are truly read only and do not have side
effects. This will make the migration to simple streams more
clean.

https://code.launchpad.net/~wallyworld/juju-core/bootstrap-exact-tools/+merge/182378

(do not edit description out of merge proposal)

[wallyworld, 2013-08-27 12:55:09]

Please take a look.

[dave_cheney.net, 2013-08-27 12:58:22]

On 2013/08/27 12:55:09, wallyworld wrote:
> Please take a look.

This branch is in conflict, sorry :(

I have a concern that currently on clouds which do not have tools deploy, and
thus fall back to our s3 bucket, will fail to work at all.

The argument could be made that using 1.13.x client with 1.12 tools is just as
broken as not being able to find any tools. I would like to see this discussion
before committing this change.

[jameinel, 2013-08-27 13:08:52]

Some quick feedback before I leave for the day. I haven't gone through
everything yet.

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

https://codereview.appspot.com/13278043/diff/1/cmd/juju/bootstrap_test.go#new...
cmd/juju/bootstrap_test.go:231: checkTools(c, env, v120All)
the comment should be updated to match.

https://codereview.appspot.com/13278043/diff/1/cmd/juju/bootstrap_test.go#new...
cmd/juju/bootstrap_test.go:269: checkTools(c, env, v120All)
ditto

https://codereview.appspot.com/13278043/diff/1/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/13278043/diff/1/environs/storage.go#newcode33
environs/storage.go:33: }
I wonder if we'd want to detect EmptyStorage and filter it out (seems silly to
put it in here), but I'm guessing it doesn't really matter.

[wallyworld, 2013-08-27 13:13:18]

https://codereview.appspot.com/13278043/diff/1/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/13278043/diff/1/environs/storage.go#newcode33
environs/storage.go:33: }
On 2013/08/27 13:08:52, jameinel wrote:
> I wonder if we'd want to detect EmptyStorage and filter it out (seems silly to
> put it in here), but I'm guessing it doesn't really matter.

It doesn't matter, and this code is temporary. PublicStorage() is going away

[wallyworld, 2013-08-27 13:16:47]

> I have a concern that currently on clouds which do not have tools deploy, and
> thus fall back to our s3 bucket, will fail to work at all.
> 

Only EC2 clouds use the s3 bucket. Other clouds use their own public buckets.
This branch does not change how the tools are located.

> The argument could be made that using 1.13.x client with 1.12 tools is just as
> broken as not being able to find any tools. I would like to see this
discussion
> before committing this change.

I discussed this with William on irc and "in person". The feeling was that using
1.x client with 1.y tools could be broken and should not be supported.

[wallyworld, 2013-08-27 13:26:23]

Please take a look.

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

https://codereview.appspot.com/13278043/diff/1/cmd/juju/bootstrap_test.go#new...
cmd/juju/bootstrap_test.go:231: checkTools(c, env, v120All)
On 2013/08/27 13:08:52, jameinel wrote:
> the comment should be updated to match.

Done.

https://codereview.appspot.com/13278043/diff/1/cmd/juju/bootstrap_test.go#new...
cmd/juju/bootstrap_test.go:269: checkTools(c, env, v120All)
On 2013/08/27 13:08:52, jameinel wrote:
> ditto

Done.

[dave_cheney.net, 2013-08-27 13:30:23]

> Only EC2 clouds use the s3 bucket. Other clouds use their own public
> buckets. This branch does not change how the tools are located.
>
>
>> The argument could be made that using 1.13.x client with 1.12 tools is
>
> just as
>>
>> broken as not being able to find any tools. I would like to see this
>
> discussion
>>
>> before committing this change.
>
>
> I discussed this with William on irc and "in person". The feeling was
> that using 1.x client with 1.y tools could be broken and should not be
> supported.

As sound theory, and one I subscribe too. I only wish to note that it
works now, today, and that fact should be kept in mind when the next
release is even less tollerent of mismatched tools bootstrapping
errors.

Could you delay this change until the simplestreams stuff is 99% done
so we have a solution better than `please run juju sync-tools before
every bootstrap` ?

[fwereade, 2013-08-27 14:41:00]

On 2013/08/27 13:30:23, dfc wrote:
> > I discussed this with William on irc and "in person". The feeling was
> > that using 1.x client with 1.y tools could be broken and should not be
> > supported.
> 
> As sound theory, and one I subscribe too. I only wish to note that it
> works now, today, and that fact should be kept in mind when the next
> release is even less tollerent of mismatched tools bootstrapping
> errors.

To be precise, I'm saying that *bootstrap* should require the same tools as the
client, because the bootstrap process is particularly vulnerable to subtle
screwups. If we ever want to upgrade we need *some* ability to talk across
versions, even if it is only v1.newer client telling a v1.older environment to
upgrade (but if we ended up *that* restricted I'd be a bit sad).
 
> Could you delay this change until the simplestreams stuff is 99% done
> so we have a solution better than `please run juju sync-tools before
> every bootstrap` ?

This does need to integrate with the auto-sync-tools functionality. Haven't read
through it yet, might be done already.

[fwereade, 2013-08-27 15:13:47]

The behaviour change is AFAICT properly restricted to bootstrap time, and
appears to auto-sync tools correctly when required. Unless dave has other
concerns, LGTM.

But we do need more some tests for the possible error cases, I think. I'd like
to be sure the problem is very clearly communicated.

https://codereview.appspot.com/13278043/diff/10001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap_test.go (right):

https://codereview.appspot.com/13278043/diff/10001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap_test.go:199: }}
There aren't any error tests, and I think there should be, especially for the
cases where there are some tools available but none quite match well enough (and
cli version not matching agent-version, and stuff).

https://codereview.appspot.com/13278043/diff/10001/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/13278043/diff/10001/environs/tools/storage.go#...
environs/tools/storage.go:59: if t.Version.Major != majorVersion || minorVersion
>= 0 && t.Version.Minor != minorVersion {
Can we handle major and minor separately please? My brain paused surprisingly
long on this line.

https://codereview.appspot.com/13278043/diff/10001/environs/tools/tools.go
File environs/tools/tools.go (right):

https://codereview.appspot.com/13278043/diff/10001/environs/tools/tools.go#ne...
environs/tools/tools.go:109: func FindExactTools(storages
[]environs.StorageReader,
I still think this shouldn't really exist, but I think it's flagged at the point
of use.

[wallyworld, 2013-08-28 00:31:47]

Please take a look.

https://codereview.appspot.com/13278043/diff/10001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap_test.go (right):

https://codereview.appspot.com/13278043/diff/10001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap_test.go:199: }}
On 2013/08/27 15:13:47, fwereade wrote:
> There aren't any error tests, and I think there should be, especially for the
> cases where there are some tools available but none quite match well enough
(and
> cli version not matching agent-version, and stuff).

The tests are constructed here to take advantage of knowledge of the underlying
code ie very whitebox. The tools matching is done in the FindBootstrapTools
method used internally by Bootstrap and there are extensive error tests for that
bit. These tests here are designed to test the additional logic added by
Bootstrap, which is setting the agent version to the newest tools.

To make testing more explicit, I've moved the core tests definitions to
environs.testing package and run the entire set both here and in the tools
package.

https://codereview.appspot.com/13278043/diff/10001/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/13278043/diff/10001/environs/tools/storage.go#...
environs/tools/storage.go:59: if t.Version.Major != majorVersion || minorVersion
>= 0 && t.Version.Minor != minorVersion {
On 2013/08/27 15:13:47, fwereade wrote:
> Can we handle major and minor separately please? My brain paused surprisingly
> long on this line.

Done.

https://codereview.appspot.com/13278043/diff/10001/environs/tools/tools.go
File environs/tools/tools.go (right):

https://codereview.appspot.com/13278043/diff/10001/environs/tools/tools.go#ne...
environs/tools/tools.go:109: func FindExactTools(storages
[]environs.StorageReader,
On 2013/08/27 15:13:47, fwereade wrote:
> I still think this shouldn't really exist, but I think it's flagged at the
point
> of use.

Yes, it is.

[dave_cheney.net, 2013-08-28 00:37:41]

NOT LGTM. Please do no submit this til after 1.13.3 has been cut.