Fix bootstrap tools issues

Fix bootstrap tools issues

These bugs are addressed:

lp:1227722 - tools for wrong arch
lp:1282870 - cannot use constraints to select tools
lp:1282869 - bad failure mode for incorrect arch

tl;dr; you can bootstrap environments which run on
architectures which are different to the client
machine architcture, so long as tools are available.
eg ppc client boottrapping an environment on EC2.

Other error conditions are also addressed eg you cannot
upload tools to a cloud if that cloud does not support
the client machine architecture, since that's what the
uploaded tools would be built for. You also cannot
specify an arch constraint different to your client
machine if you are uploading tools.

A lot of the tools logic was duplicated. It has been
refactoroed to used shared code. There's still more
to do however since it's still possible to do bad things
with upgrade-juju.

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

(do not edit description out of merge proposal)

[wallyworld, 2014-03-18 05:20:12]

Please take a look.

[jameinel, 2014-03-18 06:29:10]

I like this patch a lot. The only thing that concerns me is that all of the
"SupportedArchitectures()" are hard-coded values. And actual support is going to
vary a lot by cloud installation (especially for MaaS and Openstack).

I'm fine landing the patch as this, but we'll definitely need to add some bugs
about how we're going to enable other Architectures, or probing for
Architectures.

Maybe change MaaS to be more lenient at least. I'd hate for us to release this
and MaaS doesn't support any of the arm platforms. (I'm pretty sure the
hyperscale work was all done with armhf or arm64 machines.)

LGTM with some discussion.

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

https://codereview.appspot.com/77270043/diff/1/cmd/juju/bootstrap.go#newcode125
cmd/juju/bootstrap.go:125: err = bootstrap.UploadTools(environ,
c.Constraints.Arch, true, c.Series...)
This file looks amazing! :)

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

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/bootstrap_t...
environs/bootstrap/bootstrap_test.go:244: // Can't upload tools is agent version
already set.
"if agent version is already set"

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.go
File environs/bootstrap/synctools.go (left):

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.g...
environs/bootstrap/synctools.go:50: }
I'm a little surprised that we have something called "validate*" and we are
doing more validation before we get to it.
Would it make more sense to move the series validation checks into
validateUploadAllowed?

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

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.g...
environs/bootstrap/synctools.go:31: // Not tested directly since there's
numerous tests which use it in cmd/juju and bootstrapSuite.
I'm not sure if the comment here is relevant.
I agree that it is effectively tested, but having a couple of direct tests can
also help define what *this* part of the code is meant to be doing in isolation.

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.g...
environs/bootstrap/synctools.go:83: func UploadSeries(cfg *config.Config, series
[]string) []string {
I think this should be called "SeriesToUpload".
The problem is that "UploadTools" actually does an Upload, but UploadSeries is
just determining what list of series we would like to use, so I'd rather see it
named something that doesn't look like an action.

https://codereview.appspot.com/77270043/diff/1/provider/joyent/environ.go
File provider/joyent/environ.go (right):

https://codereview.appspot.com/77270043/diff/1/provider/joyent/environ.go#new...
provider/joyent/environ.go:52: }
Is there any way we could detect this from the Provider rather than hard coding
it inside Juju?

It seems like if a cloud start supporting a new arch which we already build
tools for, then we'll have to issue a .patch release of Juju just to change the
bit field. (And I could certainly see it varying by region, etc.)

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

https://codereview.appspot.com/77270043/diff/1/provider/maas/environ.go#newco...
provider/maas/environ.go:137: return []string{arch.AMD64}, nil
Probably we need to ask MaaS itself for this information, in the worst case we
issue a request for all machines and filter them by type?

But definitely there is plans to have MaaS support armhf, and probably arm64 (if
it doesn't already). And I would imagine PPC would be in the plans.

https://codereview.appspot.com/77270043/diff/1/provider/openstack/provider.go
File provider/openstack/provider.go (right):

https://codereview.appspot.com/77270043/diff/1/provider/openstack/provider.go...
provider/openstack/provider.go:525: return supportedArches, nil
While we do have this value inside juju-core, it certainly seems like something
that is going to be specific to a given Openstack deployment, and we should
really be trying to detect this at runtime, rather than having it hard coded
internally.

[wallyworld, 2014-03-18 06:58:28]

On 2014/03/18 06:29:10, jameinel wrote:
> I like this patch a lot. The only thing that concerns me is that all of the
> "SupportedArchitectures()" are hard-coded values. And actual support is going
to
> vary a lot by cloud installation (especially for MaaS and Openstack).
> 
> I'm fine landing the patch as this, but we'll definitely need to add some bugs
> about how we're going to enable other Architectures, or probing for
> Architectures.
> 
> Maybe change MaaS to be more lenient at least. I'd hate for us to release this
> and MaaS doesn't support any of the arm platforms. (I'm pretty sure the
> hyperscale work was all done with armhf or arm64 machines.)
> 

I agree and I should have been more verbose in my covering letter. The plan is
indeed for the SupportedArchitecures() implementation on each environs.Environ
to probe for what archietcures can be run. But I'm not sure just yet how to do
that an what form it will actually take. We were effectively previously
hardcoding the types of tools we were searching for so yeah, it's not a
regression as such. I wanted to land as is because there's plenty of value
already and we can iterate. There's also more work required to consolidate the
upload/sync tools stuff. It's still a bit all over the place.

[wallyworld, 2014-03-19 01:21:48]

Please take a look.

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

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/bootstrap_t...
environs/bootstrap/bootstrap_test.go:244: // Can't upload tools is agent version
already set.
On 2014/03/18 06:29:11, jameinel wrote:
> "if agent version is already set"

Done.

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.go
File environs/bootstrap/synctools.go (left):

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.g...
environs/bootstrap/synctools.go:50: }
On 2014/03/18 06:29:11, jameinel wrote:
> I'm a little surprised that we have something called "validate*" and we are
> doing more validation before we get to it.
> Would it make more sense to move the series validation checks into
> validateUploadAllowed?

Different purposes - the series check just ensures the bootstrapSeries parameter
is syntactically correct. The validateUploadAllowed does semantic checking to
ensure the upload can proceed based on business rules eg arches must match. The
series check is not expected to fail and is a failsafe with a terse error
message. The upload validation error is quite verbose since it is plausible that
failure will occur and the user needs feedback. So I'd prefer to keep as is but
am happy to discuss if you want.

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

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.g...
environs/bootstrap/synctools.go:31: // Not tested directly since there's
numerous tests which use it in cmd/juju and bootstrapSuite.
On 2014/03/18 06:29:11, jameinel wrote:
> I'm not sure if the comment here is relevant.
> I agree that it is effectively tested, but having a couple of direct tests can
> also help define what *this* part of the code is meant to be doing in
isolation.

Ok. The functionality was previously not exported. It was only exported when I
removed a bunch of duplicate code. But I'll add a direct test.

https://codereview.appspot.com/77270043/diff/1/environs/bootstrap/synctools.g...
environs/bootstrap/synctools.go:83: func UploadSeries(cfg *config.Config, series
[]string) []string {
On 2014/03/18 06:29:11, jameinel wrote:
> I think this should be called "SeriesToUpload".
> The problem is that "UploadTools" actually does an Upload, but UploadSeries is
> just determining what list of series we would like to use, so I'd rather see
it
> named something that doesn't look like an action.

Done.

https://codereview.appspot.com/77270043/diff/1/provider/joyent/environ.go
File provider/joyent/environ.go (right):

https://codereview.appspot.com/77270043/diff/1/provider/joyent/environ.go#new...
provider/joyent/environ.go:52: }
On 2014/03/18 06:29:11, jameinel wrote:
> Is there any way we could detect this from the Provider rather than hard
coding
> it inside Juju?
> 
> It seems like if a cloud start supporting a new arch which we already build
> tools for, then we'll have to issue a .patch release of Juju just to change
the
> bit field. (And I could certainly see it varying by region, etc.)

As discussed, I'll follow up with a simplestreams lookup to get the arches, and
for now will hard code to nominally support everything.