Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Supported architectures and metadata source fix. #6023
Conversation
anastasiamac
added some commits
Aug 11, 2016
axw
reviewed
Aug 19, 2016
| @@ -907,9 +902,6 @@ func (e *environ) ConstraintsValidator() (constraints.Validator, error) { | ||
| validator := constraints.NewValidator() | ||
| validator.RegisterUnsupported([]string{constraints.CpuPower, constraints.VirtType}) | ||
| validator.RegisterConflicts([]string{constraints.InstanceType}, []string{constraints.Mem}) | ||
| - validator.RegisterVocabulary(constraints.Arch, []string{ |
axw
Aug 19, 2016
Member
I think we should keep a restricted set of arches for the dummy provider. Then we can continue validating things like unsupported arch in --build-agent.
axw
reviewed
Aug 19, 2016
| @@ -314,12 +318,6 @@ var bootstrapTests = []bootstrapTest{{ | ||
| args: []string{"--build-agent", "--constraints", "arch=ppc64el"}, | ||
| err: `failed to bootstrap model: cannot use agent built for "ppc64el" using a machine running on "amd64"`, | ||
| -}, { | ||
| - info: "--build-agent rejects non-supported arch", |
axw
Aug 19, 2016
Member
IIANM, this is taken away because you changed the dummy provider. I think the dummy provider should continue rejecting some architectures, specifically so we can test things like this.
axw
reviewed
Aug 19, 2016
| +} | ||
| + | ||
| +// IntersectVocabulary is defined on Validator. | ||
| +func (v *validator) IntersectVocabulary(attributeName string, allowedValues interface{}) { |
axw
reviewed
Aug 19, 2016
| @@ -218,8 +206,10 @@ func Bootstrap(ctx environs.BootstrapContext, environ environs.Environ, args Boo | ||
| // If no arch is specified as a constraint, we'll bootstrap | ||
| // on the same arch as the client used to bootstrap. | ||
| var bootstrapArch string | ||
| - if bootstrapConstraints.Arch != nil { | ||
| - bootstrapArch = *bootstrapConstraints.Arch | ||
| + if args.BootstrapConstraints.Arch != nil { |
axw
Aug 19, 2016
Member
revert back to checking just BootstrapConstraints, since they're expected to be merged already
anastasiamac
Aug 19, 2016
Member
They are not merged here yet. They are merged in juju/cmd/commands/bootstrap. But if this method is not called from bootstrap cli, they still need to be merged. Validator that does it properly as well as validates them is only available later on. Hence, this logic here to ensure that we are getting correct image metadata.
axw
reviewed
Aug 19, 2016
| + return err | ||
| + } | ||
| + | ||
| + // IFF there is any image metadata, then and only then do we intersect |
axw
reviewed
Aug 19, 2016
| }) | ||
| } | ||
| + if len(params.Architectures) != 0 { |
axw
Aug 19, 2016
Member
do we need to make this conditional? is a nil slice treated differently to a non-nil but empty slice?
axw
reviewed
Aug 19, 2016
| Stream: params.Stream, | ||
| }) | ||
| + if len(params.Architectures) != 0 { |
axw
Aug 19, 2016
•
Edited 1 time
-
axw
Aug 19, 2016
Member
-
Aug 19, 2016
do we need to make this conditional? is a nil slice treated differently to a non-nil but empty slice?
wallyworld
reviewed
Aug 19, 2016
| - bootstrapArch = *bootstrapConstraints.Arch | ||
| + if args.BootstrapConstraints.Arch != nil { | ||
| + bootstrapArch = *args.BootstrapConstraints.Arch | ||
| + } else if args.ModelConstraints.Arch != nil { |
wallyworld
Aug 19, 2016
Owner
bootstrap constraints like arch need to take higher precedence than model constraints
anastasiamac
Aug 19, 2016
Member
they do :)
if bootstrap arch is set, use it.. else if model arch is set, use it.. else use host arch..
No?
wallyworld
reviewed
Aug 19, 2016
| + // IFF there is any image metadata, then and only then do we intersect | ||
| + // the vocabulary with the architectures from the metadata. | ||
| + if len(architectures) != 0 { | ||
| + architectures.Add(arch.HostArch()) |
wallyworld
Aug 19, 2016
Owner
Why add the host arch? That's not relevant to the valid arches in the cloud being bootstrapped into.
wallyworld
reviewed
Aug 19, 2016
| var availableTools coretools.List | ||
| if !args.BuildAgent { | ||
| - availableTools, err = findPackagedTools(environ, args.AgentVersion, &bootstrapArch, bootstrapSeries) | ||
| + availableTools, err = findPackagedTools(environ, args.AgentVersion, bootstrapConstraints.Arch, bootstrapSeries) |
wallyworld
Aug 19, 2016
Owner
The arch we use to find tools isn't the boostrapConstraints arch - I think it's undesirable to override that the user has specified. The original implementation copied the constraints arch to a separate variable and updated that variable from the host arch if not specified.
axw
Aug 19, 2016
Member
I agree that we shouldn't update bootstrapConstraints.Arch, but I also think the existing code isn't quite right? Consider if you have a MAAS with only ARM64 machines (e.g.). On an AMD64 client, we're going to look for only AMD64 tools, limiting what the provider can bootstrap anyway. I'd revert to how it was for now, but there may be a bug there that needs addressing later.
anastasiamac
Aug 19, 2016
Member
Reverted but also added comments to ensure that noone else "fixes" it :) and that Andrew's concern is documented :)
wallyworld
reviewed
Aug 19, 2016
| - return nil, errors.NotValidf("multiple series with bootstrap image") | ||
| - } | ||
| - seriesVersion, err := series.SeriesVersion(allSeries[0]) | ||
| + seriesVersion, err := series.SeriesVersion(*bootstrapSeries) |
wallyworld
Aug 19, 2016
Owner
is bootstrap series always not nil? it is optional on the CLI
what's the rational for changing the logic here?
anastasiamac
Aug 19, 2016
Member
The rational is that we used to get series from tools we've loaded. Now we are getting image metadata before we get tools. So we know there is only one.
However, you are right that I have remove the error and we should definitely err if bootstrap series is nil here. Adding the check :D
wallyworld
reviewed
Aug 19, 2016
| imageConstraint := imagemetadata.NewImageConstraint(simplestreams.LookupParams{ | ||
| CloudSpec: region, | ||
| - Series: availableTools.AllSeries(), |
wallyworld
Aug 19, 2016
Owner
we can't remove the series and arch constraints on image metadata look up or else we could an image which is incompatible with the tools
anastasiamac
Aug 19, 2016
Member
We are getting tools later now. At this stage, we do not know what tools we have yet. So we are getting all image metadata.
wallyworld
reviewed
Aug 19, 2016
| @@ -128,8 +128,11 @@ func (s *bootstrapSuite) TestBootstrapEmptyConstraints(c *gc.C) { | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| c.Assert(env.bootstrapCount, gc.Equals, 1) | ||
| env.args.AvailableTools = nil | ||
| + // Should have host arch, since none was explicitly specified. |
wallyworld
Aug 19, 2016
Owner
if none is specified we do need to use the host arch when looking for tools but i'm not sure of the correctness in setting the constraint attribute - constraints are what the user asked for explicitly
wallyworld
reviewed
Aug 19, 2016
| @@ -33,9 +30,11 @@ func ValidateImageMetadata(params *simplestreams.MetadataLookupParams) ([]string | ||
| Endpoint: params.Endpoint, | ||
| }, | ||
| Series: []string{params.Series}, | ||
| - Arches: params.Architectures, |
wallyworld
Aug 19, 2016
Owner
we always want to filter images on architecture, unless filtering is done later before the image is used
anastasiamac
Aug 19, 2016
Member
Yeah, we do. I had it in the conditional couple of more lines down. But Andrew has rightly pointed out that conditional is not needed.
Anyway, the logic is there.
anastasiamac
added some commits
Aug 19, 2016
axw
reviewed
Aug 19, 2016
| + return err | ||
| + } | ||
| + | ||
| + // IFF there is any image metadata, then and only then do we update |
axw
Aug 19, 2016
Member
the comment and if test are unnecessary now; you can update with an empty set and it'll just do nothing. fewer conditions makes it easier to understand
anastasiamac
Aug 19, 2016
Member
I've found that updating validator vocabulary with empty set will misbehave if validator did not contain this vocabulary. An empty set will become valid vocabulary :D and, in our case, no architecture will be considered valid by that validator.
Since now most of provider validators - except for LXD, MAAS and Azure - will not have architecture vocabulary, the conditional is needed. We only want to update vocabulary if we have non-empty list here.
axw
Aug 19, 2016
•
Edited 1 time
-
anastasiamac
Aug 19, 2016
Member
-
Aug 19, 2016
I've found that updating validator vocabulary with empty set will misbehave if validator did not contain this vocabulary. An empty set will become valid vocabulary :D and, in our case, no architecture will be considered valid by that validator.
Isn't that exactly what we want? If the provider has no architecture vocab (because it defers to image metadata), and there is no image metadata, then we should not accept any arch?
I've spent so much time in this, my eyes got glazed over... When you put it so eloquently - yes, it is exactly what we want. I've remove the conditional and the code :D
|
LGTM when remaining issues are resolved. |
anastasiamac
added some commits
Aug 19, 2016
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
anastasiamac commentedAug 18, 2016
•
Edited 1 time
-
anastasiamac
Aug 18, 2016
This PR addresses several bugs that reported symptoms with the underlying problem coming from supported architectures. It also specifically fixes using --metadata-source at bootstrap.
Notable changes:
QA steps:
On an environment with limited/no internet or on private cloud:
i. disabled Official Images data sources;
ii. generated image metadata for an existing ami and stored it locally;
iii. bootstrapped using metadata-source pointing to local metadata directory;
iv. ran status waiting for everything to get status
(Review request: http://reviews.vapour.ws/r/5471/)