WIP: stream fallback #8080

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Member

babbageclunk commented Nov 15, 2017

Work in progress The stream fallback is done, but searching for agents across multiple datasources isn't yet.

Description of change

If someone bootstraps with a beta or with an explicitly set devel stream, we weren't then considering released agent binaries as possible candidates for upgrades (because they are on the released stream).
Change the metadata lookup so that we check the released stream if a controller is on the devel stream.

Similarly, if someone has configured an agent source, we should still be able to check the cloud datasource and the public streams for better binary versions.

QA steps

Bootstrap a controller with 2.2-beta3 (using a released 2.2 binary). Then run juju upgrade-juju -m controller. The controller should be upgraded to 2.2.6.

Documentation changes

It's possible this needs documentation? I couldn't find any documentation for how we find agent binaries in streams, but if there is we should add a mention of the fallback to it.

},
}
- items, resolveInfo, err := simplestreams.GetMetadata(sources, params)
+ if len(cons.Streams) > 0 {
+ params.ValueParams.MirrorContentId = ToolsContentId(cons.Streams[0])
@babbageclunk

babbageclunk Nov 15, 2017

Member

I'm a bit unsure about this part. It feels odd to rely on the knowledge that the first stream will be the one we're officially on.

@wallyworld

wallyworld Nov 15, 2017

Owner

It's not - the there will potentially be multiple mirror content ids. The one to use will coincide with the agent binary index actually used.

@babbageclunk

babbageclunk Nov 15, 2017

Member

You mean it's not right? So does that mean that ValueParams.MirrorContentId needs to be a []string as well? Or hmm, I guess it needs to be a func that takes the stream name and returns the corresponding content id?

@@ -99,18 +99,21 @@ func Fetch(
ValueTemplate: Metadata{},
},
}
- items, resolveInfo, err := simplestreams.GetMetadata(src, params)
+ results, err := simplestreams.GetMetadata(src, params)
@wallyworld

wallyworld Nov 15, 2017

Owner

GetMetadata should return NotFound if len(results) == 0
This simplifies all of the call sites to not require a separate error check

environs/imagedownloads/simplestreams.go
+ if len(results) < 1 {
+ return nil, nil, errors.NotFoundf("simplestreams metadata results")
+ }
+ md := make([]*Metadata, len(results[0].Items))
@wallyworld

wallyworld Nov 15, 2017

Owner

Why results[0]?
We should be iterating over all returned results

@babbageclunk

babbageclunk Nov 15, 2017

Member

In image downloads? We'll never pass UseAllSources=true here, why iterate through the results?

environs/imagemetadata/simplestreams.go
+ ids := make([]string, 0, nrArches*nrSeries*nrStreams)
+ for _, stream := range streams {
+ stream = idStream(stream)
+ for _, arch := range ic.Arches {
@wallyworld

wallyworld Nov 15, 2017

Owner

this could be pulled out to a separate func; inline would do

@babbageclunk

babbageclunk Nov 15, 2017

Member

Do you mean pull the arch and ser loops into a local func? Why do that? (I guess it would reduce rightward drift?)

environs/simplestreams/simplestreams.go
+// onlySigned is false and no signed metadata is found in a source,
+// the source is used to look for unsigned metadata. Each source is
+// tried in turn - if AllResults is false then the results from the
+// first signed (or unsigned) match are returned. If AllResults is
@wallyworld

wallyworld Nov 15, 2017

Owner

not so much "match" as the first datasource containing a valid index

@babbageclunk

babbageclunk Nov 15, 2017

Member

Thanks, will update.

environs/tools/tools.go
+
+ streams := []string{stream}
+ if fallback {
+ streams = streamFallbacks[stream]
@wallyworld

wallyworld Nov 15, 2017

Owner

if they pass in a stream that's not in fallbacks, we need to just use that

@babbageclunk

babbageclunk Nov 15, 2017

Member

Ok, makes sense.

babbageclunk added some commits Oct 23, 2017

Add StreamFallback flag to ToolsConstraint
If this is true, we'll look across streams when searching a source for
tools metadata. Change the fetching code to use stream fallback, while
the MergeMetadata path doesn't (since it already goes across streams,
and fallback here would introduce lots of duplicates).
Refactor simplestreams.GetMetadata to return a list
This is a step towards allowing it to return results from more than one
source.
Change LookupParams.Stream to .Streams
Rather than using a fallback flag on the ToolsConstraint.
Review changes from wallyworld
Not including the MirrorContentId changes which will be more
substantial.

jujubot added a commit that referenced this pull request Nov 16, 2017

Merge pull request #8092 from babbageclunk/stream-fallback
Find agent binaries using fallback across streams

### Description of change

If someone bootstraps with a beta or with an explicitly set devel stream, we weren't then considering released agent binaries as possible candidates for upgrades (because they are on the released stream). 
Change the metadata lookup so that we check the released stream if a controller is on the devel stream.

This is another go at #8080 without trying to change the simplestreams code - handling mirrors and resolve info was too involved. This version combines the results just above `tools/simplestreams/Fetch`, which means it handles mirrors right for each stream and doesn't need to combine resolve info because it's thrown away at that point anyway.

## QA steps

Bootstrap a controller with 2.2-beta3 (using a released 2.2 binary). Then run `juju upgrade-juju -m controller`. The controller should be upgraded to 2.2.6.

## Documentation changes

It's possible this needs documentation? I couldn't find any documentation for how we find agent binaries in streams, but if there is we should add a mention of the fallback to it.

@wallyworld wallyworld closed this Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment