Merge lp:~jameinel/juju-core/1.18-dowgrade-patch-level into lp:juju-core/1.18

[John A Meinel]

This backs off a little bit on the "refuse to downgrade" to "refuse to downgrade across MAJOR or MINOR versions".

This allows the CI infrastructure to continue to test 'upgrades' when Juju will default to bootstrapping a version that is newer than the target of the test. (they *want* to bootstrap 1.18.0 and then upgrade it to 1.18.1, but bootstrap will go immediately to 1.18.1, so they have to bootstrap, downgrade, and then upgrade again. Not an ideal situation.)

If it was just CI, then I might push harder to fix it elsewhere, but I can see users getting a bugfix patch that actually breaks something and needing a way out.

We still refuse to go across MAJOR or MINOR versions, because we almost definitely are going to be changing things like agent.conf there and when they get to the target version, it will just be broken.

This backs off a little bit on the "refuse to downgrade" to "refuse to downgrade across MAJOR or MINOR versions".

This allows the CI infrastructure to continue to test 'upgrades' when Juju will default to bootstrapping a version that is newer than the target of the test. (they *want* to bootstrap 1.18.0 and then upgrade it to 1.18.1, but bootstrap will go immediately to 1.18.1, so they have to bootstrap, downgrade, and then upgrade again. Not an ideal situation.)

If it was just CI, then I might push harder to fix it elsewhere, but I can see users getting a bugfix patch that actually breaks something and needing a way out.

We still refuse to go across MAJOR or MINOR versions, because we almost definitely are going to be changing things like agent.conf there and when they get to the target version, it will just be broken.

[Ian Booth (wallyworld)]

review: Approve

=== modified file 'worker/upgrader/export_test.go'
--- worker/upgrader/export_test.go	2014-03-21 03:27:16 +0000
+++ worker/upgrader/export_test.go	2014-04-10 18:59:35 +0000
@@ -8,7 +8,10 @@
 	"launchpad.net/juju-core/utils"
 )
 
-var RetryAfter = &retryAfter
+var (
+	RetryAfter           = &retryAfter
+	AllowedTargetVersion = allowedTargetVersion
+)
 
 func EnsureTools(u *Upgrader, agentTools *tools.Tools, hostnameVerification utils.SSLHostnameVerification) error {
 	return u.ensureTools(agentTools, hostnameVerification)

=== modified file 'worker/upgrader/upgrader.go'
--- worker/upgrader/upgrader.go	2014-04-09 04:50:46 +0000
+++ worker/upgrader/upgrader.go	2014-04-10 18:59:35 +0000
@@ -73,6 +73,18 @@
 	return u.Wait()
 }
 
+// allowedTargetVersion checks if targetVersion is too different from
+// curVersion to allow a downgrade.
+func allowedTargetVersion(curVersion, targetVersion version.Number) bool {
+	if targetVersion.Major < curVersion.Major {
+		return false
+	}
+	if targetVersion.Major == curVersion.Major && targetVersion.Minor < curVersion.Minor {
+		return false
+	}
+	return true
+}
+
 func (u *Upgrader) loop() error {
 	currentTools := &coretools.Tools{Version: version.Current}
 	err := u.st.SetVersion(u.tag, currentTools.Version)
@@ -112,7 +124,9 @@
 		case <-dying:
 			return nil
 		}
-		if wantVersion.Compare(version.Current.Number) < 0 {
+		if wantVersion == currentTools.Version.Number {
+			continue
+		} else if !allowedTargetVersion(version.Current.Number, wantVersion) {
 			// See also bug #1299802 where when upgrading from
 			// 1.16 to 1.18 there is a race condition that can
 			// cause the unit agent to upgrade, and then want to
@@ -122,33 +136,31 @@
 				wantVersion, version.Current)
 			continue
 		}
-		if wantVersion != currentTools.Version.Number {
-			logger.Infof("upgrade requested from %v to %v", currentTools.Version, wantVersion)
-			// TODO(dimitern) 2013-10-03 bug #1234715
-			// Add a testing HTTPS storage to verify the
-			// disableSSLHostnameVerification behavior here.
-			wantTools, hostnameVerification, err = u.st.Tools(u.tag)
-			if err != nil {
-				// Not being able to lookup Tools is considered fatal
-				return err
-			}
-			// The worker cannot be stopped while we're downloading
-			// the tools - this means that even if the API is going down
-			// repeatedly (causing the agent to be stopped), as long
-			// as we have got as far as this, we will still be able to
-			// upgrade the agent.
-			err := u.ensureTools(wantTools, hostnameVerification)
-			if err == nil {
-				return &UpgradeReadyError{
-					OldTools:  version.Current,
-					NewTools:  wantTools.Version,
-					AgentName: u.tag,
-					DataDir:   u.dataDir,
-				}
-			}
-			logger.Errorf("failed to fetch tools from %q: %v", wantTools.URL, err)
-			retry = retryAfter()
-		}
+		logger.Infof("upgrade requested from %v to %v", currentTools.Version, wantVersion)
+		// TODO(dimitern) 2013-10-03 bug #1234715
+		// Add a testing HTTPS storage to verify the
+		// disableSSLHostnameVerification behavior here.
+		wantTools, hostnameVerification, err = u.st.Tools(u.tag)
+		if err != nil {
+			// Not being able to lookup Tools is considered fatal
+			return err
+		}
+		// The worker cannot be stopped while we're downloading
+		// the tools - this means that even if the API is going down
+		// repeatedly (causing the agent to be stopped), as long
+		// as we have got as far as this, we will still be able to
+		// upgrade the agent.
+		err := u.ensureTools(wantTools, hostnameVerification)
+		if err == nil {
+			return &UpgradeReadyError{
+				OldTools:  version.Current,
+				NewTools:  wantTools.Version,
+				AgentName: u.tag,
+				DataDir:   u.dataDir,
+			}
+		}
+		logger.Errorf("failed to fetch tools from %q: %v", wantTools.URL, err)
+		retry = retryAfter()
 	}
 }
 

=== modified file 'worker/upgrader/upgrader_test.go'
--- worker/upgrader/upgrader_test.go	2014-04-09 04:50:46 +0000
+++ worker/upgrader/upgrader_test.go	2014-04-10 18:59:35 +0000
@@ -42,7 +42,10 @@
 	oldRetryAfter func() <-chan time.Time
 }
 
+type AllowedTargetVersionSuite struct{}
+
 var _ = gc.Suite(&UpgraderSuite{})
+var _ = gc.Suite(&AllowedTargetVersionSuite{})
 
 func (s *UpgraderSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
@@ -232,7 +235,7 @@
 	c.Assert(err, gc.IsNil)
 }
 
-func (s *UpgraderSuite) TestUpgraderRefusesToDowngrade(c *gc.C) {
+func (s *UpgraderSuite) TestUpgraderRefusesToDowngradeMinorVersions(c *gc.C) {
 	stor := s.Conn.Environ.Storage()
 	origTools := envtesting.PrimeTools(c, stor, s.DataDir(), version.MustParseBinary("5.4.3-precise-amd64"))
 	s.PatchValue(&version.Current, origTools.Version)
@@ -252,3 +255,50 @@
 	// c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 	c.Check(err, gc.ErrorMatches, "cannot read tools metadata in tools directory.*no such file or directory")
 }
+
+func (s *UpgraderSuite) TestUpgraderAllowsDowngradingPatchVersions(c *gc.C) {
+	stor := s.Conn.Environ.Storage()
+	origTools := envtesting.PrimeTools(c, stor, s.DataDir(), version.MustParseBinary("5.4.3-precise-amd64"))
+	s.PatchValue(&version.Current, origTools.Version)
+	downgradeTools := envtesting.AssertUploadFakeToolsVersions(
+		c, stor, version.MustParseBinary("5.4.2-precise-amd64"))[0]
+	err := statetesting.SetAgentVersion(s.State, downgradeTools.Version.Number)
+	c.Assert(err, gc.IsNil)
+
+	dummy.SetStorageDelay(coretesting.ShortWait)
+
+	u := s.makeUpgrader()
+	err = u.Stop()
+	envtesting.CheckUpgraderReadyError(c, err, &upgrader.UpgradeReadyError{
+		AgentName: s.machine.Tag(),
+		OldTools:  origTools.Version,
+		NewTools:  downgradeTools.Version,
+		DataDir:   s.DataDir(),
+	})
+	foundTools, err := agenttools.ReadTools(s.DataDir(), downgradeTools.Version)
+	c.Assert(err, gc.IsNil)
+	envtesting.CheckTools(c, foundTools, downgradeTools)
+}
+
+type allowedTest struct {
+	current string
+	target  string
+	allowed bool
+}
+
+func (s *AllowedTargetVersionSuite) TestAllowedTargetVersionSuite(c *gc.C) {
+	cases := []allowedTest{
+		{current: "1.2.3", target: "1.3.3", allowed: true},
+		{current: "1.2.3", target: "1.2.3", allowed: true},
+		{current: "1.2.3", target: "2.2.3", allowed: true},
+		{current: "1.2.3", target: "1.1.3", allowed: false},
+		{current: "1.2.3", target: "1.2.2", allowed: true},
+		{current: "1.2.3", target: "0.2.3", allowed: false},
+	}
+	for i, test := range cases {
+		c.Logf("test case %d, %#v", i, test)
+		current := version.MustParse(test.current)
+		target := version.MustParse(test.target)
+		c.Check(upgrader.AllowedTargetVersion(current, target), gc.Equals, test.allowed)
+	}
+}