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

[Andrew Wilkins]

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://codereview.appspot.com/88840043/

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://codereview.appspot.com/88840043/

[Andrew Wilkins (axwalk)]

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...

[John A Meinel (jameinel)]

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/

[Andrew Wilkins (axwalk)]

Please take a look.

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

[Andrew Wilkins (axwalk)]

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/

[Andrew Wilkins (axwalk)]

Please take a look.

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

=== modified file 'environs/bootstrap/bootstrap.go'
--- environs/bootstrap/bootstrap.go	2014-03-28 13:27:45 +0000
+++ environs/bootstrap/bootstrap.go	2014-04-17 10:10:59 +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,46 @@
 			return nil, fmt.Errorf("failed to update environment configuration: %v", err)
 		}
 	}
+	bootstrapVersion := newVersion
+	// 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.
+	if !isCompatibleVersion(newVersion, version.Current.Number) {
+		compatibleVersion, compatibleTools := findCompatibleTools(possibleTools, version.Current.Number)
+		if len(compatibleTools) == 0 {
+			logger.Warningf(
+				"failed to find %s tools, will attempt to use %s",
+				version.Current.Number, newVersion,
+			)
+		} else {
+			bootstrapVersion, toolsList = compatibleVersion, compatibleTools
+		}
+	}
+	logger.Infof("picked bootstrap tools version: %s", bootstrapVersion)
 	return toolsList, nil
 }
 
+// findCompatibleTools finds tools in the list that have the same major, minor
+// and patch level as version.Current.
+//
+// Build number is not important to match; uploaded tools will have
+// incremented build number, and we want to match them.
+func findCompatibleTools(possibleTools coretools.List, version version.Number) (version.Number, coretools.List) {
+	var compatibleTools coretools.List
+	for _, tools := range possibleTools {
+		if isCompatibleVersion(tools.Version.Number, version) {
+			compatibleTools = append(compatibleTools, tools)
+		}
+	}
+	return compatibleTools.Newest()
+}
+
+func isCompatibleVersion(v1, v2 version.Number) bool {
+	v1.Build = 0
+	v2.Build = 0
+	return v1.Compare(v2) == 0
+}
+
 // EnsureNotBootstrapped returns nil if the environment is not
 // bootstrapped, and an error if it is or if the function was not able
 // to tell.

=== modified file 'environs/bootstrap/bootstrap_test.go'
--- environs/bootstrap/bootstrap_test.go	2014-04-09 16:36:12 +0000
+++ environs/bootstrap/bootstrap_test.go	2014-04-17 10:10:59 +0000
@@ -26,6 +26,7 @@
 	"launchpad.net/juju-core/provider/dummy"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/version"
 )
 
@@ -413,6 +414,64 @@
 	s.assertUploadTools(c, vers, true, extraCfg, "")
 }
 
+func (s *bootstrapSuite) TestSetBootstrapTools(c *gc.C) {
+	availableVersions := []version.Binary{
+		version.MustParseBinary("1.18.0-trusty-arm64"),
+		version.MustParseBinary("1.18.1-trusty-arm64"),
+		version.MustParseBinary("1.18.1.1-trusty-arm64"),
+		version.MustParseBinary("1.18.1.2-trusty-arm64"),
+		version.MustParseBinary("1.18.1.3-trusty-arm64"),
+	}
+	availableTools := make(tools.List, len(availableVersions))
+	for i, v := range availableVersions {
+		availableTools[i] = &tools.Tools{Version: v}
+	}
+
+	type test struct {
+		currentVersion       version.Number
+		expectedTools        version.Number
+		expectedAgentVersion version.Number
+	}
+	tests := []test{{
+		currentVersion:       version.MustParse("1.18.0"),
+		expectedTools:        version.MustParse("1.18.0"),
+		expectedAgentVersion: version.MustParse("1.18.1.3"),
+	}, {
+		currentVersion:       version.MustParse("1.18.1.4"),
+		expectedTools:        version.MustParse("1.18.1.3"),
+		expectedAgentVersion: version.MustParse("1.18.1.3"),
+	}, {
+		// build number is ignored unless major/minor don't
+		// match the latest.
+		currentVersion:       version.MustParse("1.18.1.2"),
+		expectedTools:        version.MustParse("1.18.1.3"),
+		expectedAgentVersion: version.MustParse("1.18.1.3"),
+	}, {
+		// If the current patch level exceeds whatever's in
+		// the tools source (e.g. when bootstrapping from trunk)
+		// then the latest available tools will be chosen.
+		currentVersion:       version.MustParse("1.18.2"),
+		expectedTools:        version.MustParse("1.18.1.3"),
+		expectedAgentVersion: version.MustParse("1.18.1.3"),
+	}}
+
+	env := newEnviron("foo", useDefaultKeys, nil)
+	for i, t := range tests {
+		c.Logf("test %d: %+v", i, t)
+		cfg, err := env.Config().Remove([]string{"agent-version"})
+		c.Assert(err, gc.IsNil)
+		err = env.SetConfig(cfg)
+		c.Assert(err, gc.IsNil)
+		s.PatchValue(&version.Current.Number, t.currentVersion)
+		bootstrapTools, err := bootstrap.SetBootstrapTools(env, availableTools)
+		c.Assert(err, gc.IsNil)
+		c.Assert(bootstrapTools, gc.HasLen, 1)
+		c.Assert(bootstrapTools[0].Version.Number, gc.Equals, t.expectedTools)
+		agentVersion, _ := env.Config().AgentVersion()
+		c.Assert(agentVersion, gc.Equals, t.expectedAgentVersion)
+	}
+}
+
 type bootstrapEnviron struct {
 	name             string
 	cfg              *config.Config

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