Merge pull request juju#11384 from babbageclunk/2.8-downgrade-agent

juju#11384

## Description of change

Previously the upgrader rejected a version change if the target minor or major version was less than the current version. This check has been removed to support rolling back the Juju version by restoring a backup taken before an upgrade.

The same change has been made in the caasupgrader worker - the upgrade process is different in k8s land but this check was essentially the same.

With these changes restoring a previous version of a controller automatically triggers the agents to download the required version of the agent, symlink it and update the upgradedToVersion in the agent.conf.

There was a mention of a bug in upgrading from 1.16 to 1.18: https://bugs.launchpad.net/juju-core/+bug/1299802
My understanding is that this behaviour isn't a concern any more because the unit agent no longer considers the machine agent's version; everything is driven by the model agent version instead.

## QA steps

* Bootstrap with Juju 2.7.
* Deploy some units.
* Take a backup of the controller.
```sh
juju create-backup -m controller
```
* Upgrade to 2.8 with this change, and upgrade the model as well.
```sh
juju upgrade-controller --build-agent
juju upgrade-model
```
* Run juju-restore on the controller machine. At the moment this requires a version of juju-restore hacked to remove the check requiring the same version between controller and backup.
```sh
juju scp -m controller $(which juju-restore) 0:
juju scp -m controller juju-backup-whenever-it-is.tar.gz
juju ssh -m controller 0
# On machine
sudo grep statepassword /var/lib/juju/agents/machine-0/agent.conf
./juju-restore --username machine-0 --password thepassword --verbose juju-backup-whenever.tar.gz
```
* After restore the controller agent will start complaining about downgrading state from 2.8 to 2.7.4 - this is unnecessary because the db is already at 2.7.4, it's just because the agent.conf is wrong. Stop the agent and edit agent.conf to set upgradedToVersion back to 2.7.4:
```sh
$ sudo systemctl stop jujud-machine-0.service
$ sudo vi /var/lib/juju/agents/machine-0/agent.conf
$ sudo systemctl start jujud-machine-0.service
```
(Once the version downgrade changes are made to juju-restore this will happen automatically.)

* The unit and machine agents in the model will update their symlinks to point to the 2.7.4 binary and their agent.confs with the correct upgradedToVersion.
* Their log files will show that the right version of the binary is running.

## Documentation changes

No change to documentation.

## Bug reference

None

Author: jujubot

cmd/jujud/agent/bootstrap_test.go

@@ -410,9 +410,9 @@ func (s *BootstrapSuite) TestInitializeEnvironmentInvalidOplogSize(c *gc.C) {
 }
 
 func (s *BootstrapSuite) TestInitializeEnvironmentToolsNotFound(c *gc.C) {
-	// bootstrap with 1.99.1 but there will be no tools so version will be reset.
+	// bootstrap with 2.99.1 but there will be no tools so version will be reset.
 	cfg, err := s.bootstrapParams.ControllerModelConfig.Apply(map[string]interface{}{
-		"agent-version": "1.99.1",
+		"agent-version": "2.99.1",
 	})
 	c.Assert(err, jc.ErrorIsNil)
 	s.bootstrapParams.ControllerModelConfig = cfg
@@ -433,7 +433,7 @@ func (s *BootstrapSuite) TestInitializeEnvironmentToolsNotFound(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	vers, ok := cfg.AgentVersion()
 	c.Assert(ok, jc.IsTrue)
-	c.Assert(vers.String(), gc.Equals, "1.99.0")
+	c.Assert(vers.String(), gc.Equals, "2.99.0")
 }
 
 func (s *BootstrapSuite) TestSetConstraints(c *gc.C) {

environs/bootstrap/bootstrap_test.go

@@ -667,7 +667,7 @@ func (s *bootstrapSuite) TestBootstrapBuildAgent(c *gc.C) {
 			BuildAgentTarball: func(build bool, ver *version.Number, _ string) (*sync.BuiltAgent, error) {
 				c.Logf("BuildAgentTarball version %s", ver)
 				c.Assert(build, jc.IsTrue)
-				c.Assert(ver.String(), gc.Equals, "1.99.0.1")
+				c.Assert(ver.String(), gc.Equals, "2.99.0.1")
 				localVer := *ver
 				return &sync.BuiltAgent{
 					Dir:      c.MkDir(),
@@ -687,7 +687,7 @@ func (s *bootstrapSuite) TestBootstrapBuildAgent(c *gc.C) {
 	cfg := env.instanceConfig.Bootstrap.ControllerModelConfig
 	agentVersion, valid := cfg.AgentVersion()
 	c.Check(valid, jc.IsTrue)
-	c.Check(agentVersion.String(), gc.Equals, "1.99.0")
+	c.Check(agentVersion.String(), gc.Equals, "2.99.0")
 }
 
 func (s *bootstrapSuite) assertBootstrapPackagedToolsAvailable(c *gc.C, clientArch string) {

featuretests/upgrade_test.go

@@ -76,8 +76,8 @@ func (s *upgradeSuite) SetUpTest(c *gc.C) {
 		Arch:   arch.HostArch(),
 		Series: series.MustHostSeries(),
 	}
-	s.oldVersion.Major = 1
-	s.oldVersion.Minor = 16
+	s.oldVersion.Major = 2
+	s.oldVersion.Minor = 1
 
 	// Don't wait so long in tests.
 	s.PatchValue(&upgradesteps.UpgradeStartTimeoutMaster, time.Millisecond*50)

testing/environ.go

@@ -37,7 +37,7 @@ var (
 )
 
 // FakeVersionNumber is a valid version number that can be used in testing.
-var FakeVersionNumber = version.MustParse("1.99.0")
+var FakeVersionNumber = version.MustParse("2.99.0")
 
 // ModelTag is a defined known valid UUID that can be used in testing.
 var ModelTag = names.NewModelTag("deadbeef-0bad-400d-8000-4b1d0d06f00d")

worker/caasupgrader/upgrader.go

@@ -155,15 +155,18 @@ func (u *Upgrader) loop() error {
 		} else if !upgrader.AllowedTargetVersion(
 			u.config.OrigAgentVersion,
 			jujuversion.Current,
-			!u.config.UpgradeStepsWaiter.IsUnlocked(),
 			wantVersion,
 		) {
 			logger.Warningf("desired agent binary version: %s is older than current %s, refusing to downgrade",
 				wantVersion, jujuversion.Current)
 			u.config.InitialUpgradeCheckComplete.Unlock()
 			continue
 		}
-		logger.Debugf("upgrade requested for %v from %v to %v", u.tag, jujuversion.Current, wantVersion)
+		direction := "upgrade"
+		if wantVersion.Compare(jujuversion.Current) == -1 {
+			direction = "downgrade"
+		}
+		logger.Debugf("%s requested for %v from %v to %v", direction, u.tag, jujuversion.Current, wantVersion)
 		err = u.operatorUpgrader.Upgrade(u.tag.String(), wantVersion)
 		if err != nil {
 			return errors.Annotatef(err, "requesting upgrade for %f from %v to %v", u.tag, jujuversion.Current, wantVersion)

worker/caasupgrader/upgrader_test.go

@@ -121,6 +121,23 @@ func (s *UpgraderSuite) TestUpgraderDowngradePatch(c *gc.C) {
 	s.operatorUpgrader.CheckCall(c, 0, "Upgrade", "machine-0", s.upgraderClient.desired)
 }
 
+func (s *UpgraderSuite) TestUpgraderDowngradeMinor(c *gc.C) {
+	// We'll allow this for the case of restoring a backup from a
+	// previous juju version.
+	vers := version.MustParse("6.6.7")
+	s.PatchValue(&jujuversion.Current, vers)
+	s.upgraderClient.desired = version.MustParse("6.5.10")
+
+	u := s.makeUpgrader(c, names.NewMachineTag("0"))
+	workertest.CleanKill(c, u)
+
+	s.expectInitialUpgradeCheckNotDone(c)
+	c.Assert(s.upgraderClient.actual.Number, gc.DeepEquals, vers)
+	s.upgraderClient.CheckCallNames(c, "SetVersion", "DesiredVersion")
+	s.operatorUpgrader.CheckCallNames(c, "Upgrade")
+	s.operatorUpgrader.CheckCall(c, 0, "Upgrade", "machine-0", s.upgraderClient.desired)
+}
+
 func (s *UpgraderSuite) expectInitialUpgradeCheckDone(c *gc.C) {
 	c.Assert(s.initialCheckComplete.IsUnlocked(), jc.IsTrue)
 }

worker/upgrader/upgrader.go

@@ -115,16 +115,10 @@ func (u *Upgrader) Stop() error {
 func AllowedTargetVersion(
 	origAgentVersion version.Number,
 	curVersion version.Number,
-	upgradeStepsRunning bool,
 	targetVersion version.Number,
 ) bool {
-	if upgradeStepsRunning && targetVersion == origAgentVersion {
-		return true
-	}
-	if targetVersion.Major < curVersion.Major {
-		return false
-	}
-	if targetVersion.Major == curVersion.Major && targetVersion.Minor < curVersion.Minor {
+	// Don't allow downgrading from higher versions to version 1.x
+	if curVersion.Major >= 2 && targetVersion.Major == 1 {
 		return false
 	}
 	return true
@@ -208,20 +202,20 @@ func (u *Upgrader) loop() error {
 		} else if !AllowedTargetVersion(
 			u.config.OrigAgentVersion,
 			haveVersion,
-			!u.config.UpgradeStepsWaiter.IsUnlocked(),
 			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
-			// downgrade when its associate machine agent has not
-			// finished upgrading.
-			logger.Infof("desired agent binary version: %s is older than current %s, refusing to downgrade",
-				wantVersion, haveVersion)
+			// Don't allow downgrading to v1.x - we don't support
+			// restoring from a 1.x backup.
+			logger.Infof("desired agent binary version: %s is older than 2.0.0, refusing to downgrade",
+				wantVersion)
 			u.config.InitialUpgradeCheckComplete.Unlock()
 			continue
 		}
-		logger.Infof("upgrade requested from %v to %v", haveVersion, wantVersion)
+		direction := "upgrade"
+		if wantVersion.Compare(haveVersion) == -1 {
+			direction = "downgrade"
+		}
+		logger.Infof("%s requested from %v to %v", direction, haveVersion, wantVersion)
 
 		// Check if tools have already been downloaded.
 		wantVersionBinary := toBinaryVersion(wantVersion)

worker/upgrader/upgrader_test.go

@@ -296,7 +296,9 @@ func (s *UpgraderSuite) TestUsesAlreadyDownloadedToolsIfAvailable(c *gc.C) {
 	})
 }
 
-func (s *UpgraderSuite) TestUpgraderRefusesToDowngradeMinorVersions(c *gc.C) {
+func (s *UpgraderSuite) TestUpgraderAllowsDowngradingMinorVersions(c *gc.C) {
+	// We allow this scenario to allow reverting upgrades by restoring
+	// a backup from the previous version.
 	stor := s.DefaultToolsStorage
 	origTools := envtesting.PrimeTools(c, stor, s.DataDir(), s.Environ.Config().AgentStream(), version.MustParseBinary("5.4.3-precise-amd64"))
 	s.patchVersion(origTools.Version)
@@ -305,16 +307,45 @@ func (s *UpgraderSuite) TestUpgraderRefusesToDowngradeMinorVersions(c *gc.C) {
 	err := statetesting.SetAgentVersion(s.State, downgradeTools.Version.Number)
 	c.Assert(err, jc.ErrorIsNil)
 
+	u := s.makeUpgrader(c)
+	err = u.Stop()
+	s.expectInitialUpgradeCheckNotDone(c)
+
+	envtesting.CheckUpgraderReadyError(c, err, &upgrader.UpgradeReadyError{
+		AgentName: s.machine.Tag().String(),
+		OldTools:  origTools.Version,
+		NewTools:  downgradeTools.Version,
+		DataDir:   s.DataDir(),
+	})
+	foundTools, err := agenttools.ReadTools(s.DataDir(), downgradeTools.Version)
+	c.Assert(err, jc.ErrorIsNil)
+	downgradeTools.URL = fmt.Sprintf("https://%s/model/%s/tools/5.3.3-precise-amd64",
+		s.APIState.Addr(), coretesting.ModelTag.Id())
+	envtesting.CheckTools(c, foundTools, downgradeTools)
+}
+
+func (s *UpgraderSuite) TestUpgraderForbidsDowngradingToMajorVersion1(c *gc.C) {
+	// We allow this scenario to allow reverting upgrades by restoring
+	// a backup from the previous version.
+	stor := s.DefaultToolsStorage
+	origTools := envtesting.PrimeTools(c, stor, s.DataDir(), s.Environ.Config().AgentStream(), version.MustParseBinary("2.4.3-precise-amd64"))
+	s.patchVersion(origTools.Version)
+	downgradeTools := envtesting.AssertUploadFakeToolsVersions(
+		c, stor, s.Environ.Config().AgentStream(), s.Environ.Config().AgentStream(), version.MustParseBinary("1.25.3-precise-amd64"))[0]
+	err := statetesting.SetAgentVersion(s.State, downgradeTools.Version.Number)
+	c.Assert(err, jc.ErrorIsNil)
+
 	u := s.makeUpgrader(c)
 	err = u.Stop()
 	s.expectInitialUpgradeCheckDone(c)
-	// If the upgrade would have triggered, we would have gotten an
-	// UpgradeReadyError, since it was skipped, we get no error
-	c.Check(err, jc.ErrorIsNil)
+
+	// If the upgrade had been allowed we would get an
+	// UpgradeReadyError.
+	c.Assert(err, jc.ErrorIsNil)
 	_, err = agenttools.ReadTools(s.DataDir(), downgradeTools.Version)
-	// TODO: ReadTools *should* be returning some form of errors.NotFound,
-	// however, it just passes back a fmt.Errorf so we live with it
-	// c.Assert(err, jc.Satisfies, errors.IsNotFound)
+	// TODO: ReadTools *should* be returning some form of
+	// errors.NotFound, however, it just passes back a fmt.Errorf so
+	// we live with it c.Assert(err, jc.Satisfies, errors.IsNotFound)
 	c.Check(err, gc.ErrorMatches, "cannot read agent metadata in directory.*"+utils.NoSuchFileErrRegexp)
 }
 
@@ -372,7 +403,9 @@ func (s *UpgraderSuite) TestUpgraderAllowsDowngradeToOrigVersionIfUpgradeInProgr
 	envtesting.CheckTools(c, foundTools, downgradeTools)
 }
 
-func (s *UpgraderSuite) TestUpgraderRefusesDowngradeToOrigVersionIfUpgradeNotInProgress(c *gc.C) {
+func (s *UpgraderSuite) TestUpgraderAllowsDowngradeToOrigVersionIfUpgradeNotInProgress(c *gc.C) {
+	// We now allow this to support restoring a backup from a previous
+	// version.
 	downgradeVersion := version.MustParseBinary("5.3.0-precise-amd64")
 	s.confVersion = downgradeVersion.Number
 	s.upgradeStepsComplete.Unlock()
@@ -382,16 +415,28 @@ func (s *UpgraderSuite) TestUpgraderRefusesDowngradeToOrigVersionIfUpgradeNotInP
 	s.patchVersion(origTools.Version)
 	envtesting.AssertUploadFakeToolsVersions(
 		c, stor, s.Environ.Config().AgentStream(), s.Environ.Config().AgentStream(), downgradeVersion)
+
+	prevTools := envtesting.AssertUploadFakeToolsVersions(
+		c, stor, s.Environ.Config().AgentStream(), s.Environ.Config().AgentStream(), downgradeVersion)[0]
+
 	err := statetesting.SetAgentVersion(s.State, downgradeVersion.Number)
 	c.Assert(err, jc.ErrorIsNil)
 
 	u := s.makeUpgrader(c)
 	err = u.Stop()
-	s.expectInitialUpgradeCheckDone(c)
+	s.expectInitialUpgradeCheckNotDone(c)
 
-	// If the upgrade would have triggered, we would have gotten an
-	// UpgradeReadyError, since it was skipped, we get no error
-	c.Check(err, jc.ErrorIsNil)
+	envtesting.CheckUpgraderReadyError(c, err, &upgrader.UpgradeReadyError{
+		AgentName: s.machine.Tag().String(),
+		OldTools:  origTools.Version,
+		NewTools:  prevTools.Version,
+		DataDir:   s.DataDir(),
+	})
+	foundTools, err := agenttools.ReadTools(s.DataDir(), prevTools.Version)
+	c.Assert(err, jc.ErrorIsNil)
+	prevTools.URL = fmt.Sprintf("https://%s/model/%s/tools/5.3.0-precise-amd64",
+		s.APIState.Addr(), coretesting.ModelTag.Id())
+	envtesting.CheckTools(c, foundTools, prevTools)
 }
 
 func (s *UpgraderSuite) TestChecksSpaceBeforeDownloading(c *gc.C) {
@@ -444,30 +489,26 @@ func (s *UpgraderSuite) TestChecksSpaceBeforeDownloading(c *gc.C) {
 }
 
 type allowedTest struct {
-	original       string
-	current        string
-	target         string
-	upgradeRunning bool
-	allowed        bool
+	original string
+	current  string
+	target   string
+	allowed  bool
 }
 
 func (s *AllowedTargetVersionSuite) TestAllowedTargetVersionSuite(c *gc.C) {
 	cases := []allowedTest{
-		{original: "1.2.3", current: "1.2.3", upgradeRunning: false, target: "1.3.3", allowed: true},
-		{original: "1.2.3", current: "1.2.3", upgradeRunning: false, target: "1.2.3", allowed: true},
-		{original: "1.2.3", current: "1.2.3", upgradeRunning: false, target: "2.2.3", allowed: true},
-		{original: "1.2.3", current: "1.2.3", upgradeRunning: false, target: "1.1.3", allowed: false},
-		{original: "1.2.3", current: "1.2.3", upgradeRunning: false, target: "1.2.2", allowed: true}, // downgrade between builds
-		{original: "1.2.3", current: "1.2.3", upgradeRunning: false, target: "0.2.3", allowed: false},
-		{original: "0.2.3", current: "1.2.3", upgradeRunning: false, target: "0.2.3", allowed: false},
-		{original: "0.2.3", current: "1.2.3", upgradeRunning: true, target: "0.2.3", allowed: true}, // downgrade during upgrade
+		{original: "2.7.4", current: "2.7.4", target: "2.8.0", allowed: true},  // normal upgrade
+		{original: "2.8.0", current: "2.8.0", target: "2.7.4", allowed: true},  // downgrade caused by restore after upgrade
+		{original: "2.8.0", current: "2.8.0", target: "1.2.3", allowed: false}, // can't downgrade to v1.x
+		{original: "2.7.4", current: "2.7.4", target: "2.7.5", allowed: true},  // point release
+		{original: "2.7.4", current: "2.8.0", target: "2.7.4", allowed: true},  // downgrade after upgrade but before config file updated
 	}
 	for i, test := range cases {
 		c.Logf("test case %d, %#v", i, test)
 		original := version.MustParse(test.original)
 		current := version.MustParse(test.current)
 		target := version.MustParse(test.target)
-		result := upgrader.AllowedTargetVersion(original, current, test.upgradeRunning, target)
+		result := upgrader.AllowedTargetVersion(original, current, target)
 		c.Check(result, gc.Equals, test.allowed)
 	}
 }