Merge lp://qastaging/~axwalk/juju-core/lp1247232-bootstrap-majorminorpatch into lp://qastaging/~go-bot/juju-core/trunk

Reviewers: mp+216278_code.launchpad.net,

Message:
Please take a look.

Description:
environs/bootstrap: bootstrap major.minor.patch

Bootstrap tools selection is changed so that it
will select tools with the same major, minor and
patch level as the current (CLI) tools if possible.
If there are no such tools, the most recent tools
with matching major/minor are chosen as before and
a warning will be logged.

This change allows us to change bootstrap behaviour
even within a minor release series (except of course
for existing releases).

The most recent tools with matching major/minor will
be set in agent-version, which causes the machine
agent to immediately upgrade.

Fixes lp:1247232

https://code.launchpad.net/~axwalk/juju-core/lp1247232-bootstrap-majorminorpatch/+merge/216278

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/88840043/

Affected files (+85, -2 lines):
   A [revision details]
   M environs/bootstrap/bootstrap.go
   M environs/bootstrap/bootstrap_test.go
   M tools/list.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140417053051-il9peyrj37nd277q
+New revision: <email address hidden>

Index: tools/list.go
=== modified file 'tools/list.go'
--- tools/list.go 2014-02-16 23:35:10 +0000
+++ tools/list.go 2014-04-17 09:19:41 +0000
@@ -130,7 +130,6 @@
    }
   }
   if len(result) == 0 {
- logger.Errorf("cannot match %#v", f)
    return nil, ErrNoMatches
   }
   return result, nil

Index: environs/bootstrap/bootstrap.go
=== modified file 'environs/bootstrap/bootstrap.go'
--- environs/bootstrap/bootstrap.go 2014-03-28 13:27:45 +0000
+++ environs/bootstrap/bootstrap.go 2014-04-17 09:19:41 +0000
@@ -56,7 +56,7 @@
   }
   var newVersion version.Number
   newVersion, toolsList := possibleTools.Newest()
- logger.Infof("picked newest version: %s", newVersion)
+ logger.Infof("newest version: %s", newVersion)
   cfg := environ.Config()
   if agentVersion, _ := cfg.AgentVersion(); agentVersion != newVersion {
    cfg, err := cfg.Apply(map[string]interface{}{
@@ -69,9 +69,39 @@
     return nil, fmt.Errorf("failed to update environment
configuration: %v", err)
    }
   }
+ // We should only ever bootstrap the exact same version as the client,
+ // or we risk bootstrap incompatibility. We still set agent-version to
+ // the newest version, so the agent will immediately upgrade itself.
+ //
+ // Build number is not important to match; uploaded tools will have
+ // incremented build number, and we want to match them.
+ bootstrapVersion := newVersion
+ if !sameVersionIgnoringBuild(newVersion, version.Current.Number) {
+ var sameTools coretools.List
+ for _, tools := range possibleTools {
+ if sameVersionIgnoringBuild(tools.Version.Number,
version.Current.Number) {
+ sameTools = append(sameTools, tools)
+ }
+ }
+ if len(sameTools) == 0 {
+ logger.Warningf(
+ "failed to find %s tools, will attempt to use %s",
+ version.Current.Number, newVersion,
+ )
+ } else {
+ bootstrapVersion...

I'd really like to see this tested live, though I'm not sure it will be
easy to set up the test (you'll have to populate a couple versions of
the tools, and then bootstrap with an old version of them).

We'll also certainly want to let CI know about it right away, since it
will change how they do some of their testing.

Mostly, though, LGTM.

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

https://codereview.appspot.com/88840043/diff/1/environs/bootstrap/bootstrap.go#newcode72
environs/bootstrap/bootstrap.go:72: // We should only ever bootstrap the
exact same version as the client,
This whole chunk of code seems quite localized, is it possible to just
pull it into a helper function to keep the units of work focused?

https://codereview.appspot.com/88840043/

Please take a look.

https://codereview.appspot.com/88840043/

On 2014/04/17 09:45:47, jameinel wrote:
> I'd really like to see this tested live, though I'm not sure it will
be easy to
> set up the test (you'll have to populate a couple versions of the
tools, and
> then bootstrap with an old version of them).

I did test it live by applying the patch to 1.17.6. It successfully
bootstrapped with 1.17.6, and then immediately upgraded to 1.17.7.

> We'll also certainly want to let CI know about it right away, since it
will
> change how they do some of their testing.

> Mostly, though, LGTM.

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

https://codereview.appspot.com/88840043/diff/1/environs/bootstrap/bootstrap.go#newcode72
> environs/bootstrap/bootstrap.go:72: // We should only ever bootstrap
the exact
> same version as the client,
> This whole chunk of code seems quite localized, is it possible to just
pull it
> into a helper function to keep the units of work focused?

I pulled out a little bit, but half of it is specific to the context.

https://codereview.appspot.com/88840043/

Please take a look.

https://codereview.appspot.com/88840043/