Code review comment for lp:~axwalk/juju-core/lp1247232-bootstrap-majorminorpatch

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, toolsList = sameTools.Newest()
+ }
+ }
+ logger.Infof("picked bootstrap tools version: %s", bootstrapVersion)
   return toolsList, nil
  }

+func sameVersionIgnoringBuild(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.

Index: environs/bootstrap/bootstrap_test.go
=== 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 09:19:41 +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,57 @@
   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"),
+ }}
+
+ 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