Fix cleanup manual rollback (#11236)

* Fix error handling during cleanup

* Fix versionedHomes to always be relative paths

* Exit watcher when finished rolling back

* update watchCmd unit tests to user versioned homes relative to topDir

* fixup! update watchCmd unit tests to user versioned homes relative to topDir

* fixup! Fix versionedHomes to always be relative paths

* Fix versionedHomesToKeep handling in watcher cleanup

Author: pchila

internal/pkg/agent/application/upgrade/manual_rollback.go

@@ -102,6 +102,13 @@ func rollbackUsingAgentInstalls(log *logger.Logger, watcherHelper WatcherHelper,
 		return "", "", fmt.Errorf("version %q not listed among the available rollbacks: %w", rollbackVersion, ErrNoRollbacksAvailable)
 	}
 
+	if filepath.IsAbs(targetInstall) {
+		targetInstall, err = filepath.Rel(topDir, targetInstall)
+		if err != nil {
+			return "", "", fmt.Errorf("error calculating path of install %q relative to %q: %w", targetInstall, topDir, err)
+		}
+	}
+
 	prevAgentParsedVersion, err := version.ParseVersion(targetTTLMarker.Version)
 	if err != nil {
 		return "", "", fmt.Errorf("parsing version of target install %+v: %w", targetInstall, err)

internal/pkg/agent/application/upgrade/rollback.go

@@ -174,16 +174,22 @@ func cleanup(log *logger.Logger, topDirPath string, removeMarker, keepLogs bool,
 
 	dirPrefix := fmt.Sprintf("%s-", AgentName)
 
-	relativeHomePaths := make([]string, len(versionedHomesToKeep))
-	for i, h := range versionedHomesToKeep {
-		relHomePath, err := filepath.Rel("data", h)
+	log.Infof("versioned homes to keep: %v", versionedHomesToKeep)
+
+	var cumulativeError error
+	relativeHomePaths := make([]string, 0, len(versionedHomesToKeep))
+	for _, h := range versionedHomesToKeep {
+		relHomePath, err := filepath.Rel(dataDirPath, filepath.Join(topDirPath, h))
 		if err != nil {
-			return fmt.Errorf("extracting elastic-agent path relative to data directory from %s: %w", h, err)
+			cumulativeError = goerrors.Join(cumulativeError, fmt.Errorf("extracting elastic-agent path relative to data directory from %s: %w", h, err))
+			// best effort: try to use the entry as-is, without calculating the path relative to `data`
+			relHomePath = h
 		}
-		relativeHomePaths[i] = relHomePath
+		relativeHomePaths = append(relativeHomePaths, relHomePath)
 	}
 
-	var errs []error
+	log.Infof("Starting cleanup of versioned homes. Keeping: %v", relativeHomePaths)
+
 	for _, dir := range subdirs {
 		if slices.Contains(relativeHomePaths, dir) {
 			continue
@@ -200,11 +206,11 @@ func cleanup(log *logger.Logger, topDirPath string, removeMarker, keepLogs bool,
 			ignoredDirs = append(ignoredDirs, "logs")
 		}
 		if cleanupErr := install.RemoveBut(hashedDir, true, ignoredDirs...); cleanupErr != nil {
-			errs = append(errs, cleanupErr)
+			cumulativeError = goerrors.Join(cumulativeError, cleanupErr)
 		}
 	}
 
-	return goerrors.Join(errs...)
+	return cumulativeError
 }
 
 // InvokeWatcher invokes an agent instance using watcher argument for watching behavior of

internal/pkg/agent/cmd/watch.go

@@ -102,6 +102,7 @@ func newWatchCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command
 					fmt.Fprintf(streams.Err, "Rollback command failed: %v\n", err)
 					os.Exit(errorRollbackFailed)
 				}
+				return
 			}
 
 			if err = withAppLocker(log, func() error {
@@ -200,7 +201,19 @@ func watchCmd(log *logp.Logger, topDir string, cfg *configuration.UpgradeWatcher
 		// hash is the same as hash of agent which initiated watcher.
 		versionedHomesToKeep := make([]string, 0, len(marker.RollbacksAvailable)+1)
 		// current version needs to be kept
-		versionedHomesToKeep = append(versionedHomesToKeep, paths.VersionedHome(topDir))
+		if marker.Details != nil && marker.Details.State == details.StateRollback {
+			// we need to keep the previous versioned home (we have rolled back)
+			versionedHomesToKeep = append(versionedHomesToKeep, marker.PrevVersionedHome)
+		} else {
+			// we need to keep the upgraded version, since it has not been rolled back
+			absCurrentVersionedHome := paths.VersionedHome(topDir)
+			currentVersionedHome, err := filepath.Rel(topDir, absCurrentVersionedHome)
+			if err != nil {
+				return fmt.Errorf("extracting current home path %q relative to %q: %w", absCurrentVersionedHome, topDir, err)
+			}
+			versionedHomesToKeep = append(versionedHomesToKeep, currentVersionedHome)
+		}
+
 		versionedHomesToKeep = appendAvailableRollbacks(log, marker, versionedHomesToKeep)
 		log.Infof("About to clean up upgrade. Keeping versioned homes: %v", versionedHomesToKeep)
 		if err := installModifier.Cleanup(log, paths.Top(), true, false, versionedHomesToKeep...); err != nil {
@@ -320,6 +333,14 @@ func rollback(log *logp.Logger, topDir string, client client.Client, installModi
 	// FIXME get the hash from the list of installs or the manifest or the versioned home
 	// This is only a placeholder in case there is no versionedHome defined (which we always have)
 	hash := ""
+	if filepath.IsAbs(versionedHome) {
+		// if the versioned home is an absolute path we need to normalize it relative to the current topDir as the
+		// cleanup() will expect relative paths
+		versionedHome, err = filepath.Rel(topDir, versionedHome)
+		if err != nil {
+			return fmt.Errorf("extract from %q a path relative to %q: %w", versionedHome, topDir, err)
+		}
+	}
 	err = installModifier.Rollback(context.Background(), log, client, topDir, versionedHome, hash, WithPreRestartHook(updateMarkerAndDetails))
 	if err != nil {
 		return fmt.Errorf("rolling back: %w", err)

internal/pkg/agent/cmd/watch_test.go

@@ -122,11 +122,11 @@ func Test_watchCmd(t *testing.T) {
 					&upgrade.UpdateMarker{
 						Version:           "4.5.6",
 						Hash:              "newver",
-						VersionedHome:     "elastic-agent-4.5.6-newver",
+						VersionedHome:     filepath.Join("data", "elastic-agent-4.5.6-newver"),
 						UpdatedOn:         time.Now(),
 						PrevVersion:       "1.2.3",
 						PrevHash:          "prvver",
-						PrevVersionedHome: "elastic-agent-prvver",
+						PrevVersionedHome: filepath.Join("data", "elastic-agent-prvver"),
 						Acked:             false,
 						Action:            nil,
 						Details:           nil, //details.NewDetails("4.5.6", details.StateReplacing, ""),
@@ -143,7 +143,7 @@ func Test_watchCmd(t *testing.T) {
 				expectedRemoveMarkerFlag := runtime.GOOS != "windows"
 
 				installModifier.EXPECT().
-					Cleanup(mock.Anything, topDir, expectedRemoveMarkerFlag, false, "elastic-agent-4.5.6-newver").
+					Cleanup(mock.Anything, topDir, expectedRemoveMarkerFlag, false, filepath.Join("data", "elastic-agent-4.5.6-newver")).
 					Return(nil)
 			},
 			args: args{
@@ -157,16 +157,17 @@ func Test_watchCmd(t *testing.T) {
 				dataDirPath := paths.DataFrom(topDir)
 				err := os.MkdirAll(dataDirPath, 0755)
 				require.NoError(t, err)
+				previousVersionedHome := filepath.Join("data", "elastic-agent-9.2.0-prvver")
 				err = upgrade.SaveMarker(
 					dataDirPath,
 					&upgrade.UpdateMarker{
 						Version:           "9.3.0",
 						Hash:              "newver",
-						VersionedHome:     "elastic-agent-9.3.0-newver",
+						VersionedHome:     filepath.Join("data", "elastic-agent-9.3.0-newver"),
 						UpdatedOn:         time.Now(),
 						PrevVersion:       "9.2.0",
 						PrevHash:          "prvver",
-						PrevVersionedHome: "elastic-agent-9.2.0-prvver",
+						PrevVersionedHome: previousVersionedHome,
 						Acked:             false,
 						Action:            nil,
 						Details:           nil,
@@ -179,7 +180,7 @@ func Test_watchCmd(t *testing.T) {
 					Watch(mock.Anything, mock.Anything, mock.Anything, mock.Anything).
 					Return(errors.New("some watch error due to agent misbehaving"))
 				installModifier.EXPECT().
-					Rollback(mock.Anything, mock.Anything, mock.Anything, paths.Top(), "elastic-agent-9.2.0-prvver", "prvver", mock.MatchedBy(func(opt upgrade.RollbackOption) bool {
+					Rollback(mock.Anything, mock.Anything, mock.Anything, paths.Top(), previousVersionedHome, "prvver", mock.MatchedBy(func(opt upgrade.RollbackOption) bool {
 						settings := upgrade.NewRollbackSettings()
 						opt(settings)
 
@@ -198,16 +199,17 @@ func Test_watchCmd(t *testing.T) {
 				dataDirPath := paths.DataFrom(topDir)
 				err := os.MkdirAll(dataDirPath, 0755)
 				require.NoError(t, err)
+				previousVersionedHome := filepath.Join("data", "elastic-agent-1.2.3-prvver")
 				err = upgrade.SaveMarker(
 					dataDirPath,
 					&upgrade.UpdateMarker{
 						Version:           "4.5.6",
 						Hash:              "newver",
-						VersionedHome:     "elastic-agent-4.5.6-newver",
+						VersionedHome:     filepath.Join("data", "elastic-agent-4.5.6-newver"),
 						UpdatedOn:         time.Now(),
 						PrevVersion:       "1.2.3",
 						PrevHash:          "prvver",
-						PrevVersionedHome: "elastic-agent-1.2.3-prvver",
+						PrevVersionedHome: previousVersionedHome,
 						Acked:             false,
 						Action:            nil,
 						Details:           nil, //details.NewDetails("4.5.6", details.StateReplacing, ""),
@@ -225,7 +227,7 @@ func Test_watchCmd(t *testing.T) {
 						mock.Anything,
 						mock.Anything,
 						paths.Top(),
-						"elastic-agent-1.2.3-prvver",
+						previousVersionedHome,
 						"prvver",
 						mock.MatchedBy(func(opt upgrade.RollbackOption) bool {
 							settings := upgrade.NewRollbackSettings()
@@ -245,16 +247,17 @@ func Test_watchCmd(t *testing.T) {
 				dataDirPath := paths.DataFrom(topDir)
 				err := os.MkdirAll(dataDirPath, 0755)
 				require.NoError(t, err)
+				previousVersionedHome := filepath.Join("data", "elastic-agent-prvver")
 				err = upgrade.SaveMarker(
 					dataDirPath,
 					&upgrade.UpdateMarker{
 						Version:           "4.5.6",
 						Hash:              "newver",
-						VersionedHome:     "elastic-agent-4.5.6-newver",
+						VersionedHome:     filepath.Join("data", "elastic-agent-4.5.6-newver"),
 						UpdatedOn:         time.Now(),
 						PrevVersion:       "1.2.3",
 						PrevHash:          "prvver",
-						PrevVersionedHome: "elastic-agent-prvver",
+						PrevVersionedHome: previousVersionedHome,
 						Acked:             false,
 						Action:            nil,
 						Details: &details.Details{
@@ -271,7 +274,7 @@ func Test_watchCmd(t *testing.T) {
 				// topdir, prevVersionedHome and prevHash are not taken from the upgrade marker, otherwise they would be
 				// <topDir, "topDir/data/elastic-agent-prvver", "prvver">
 				installModifier.EXPECT().
-					Cleanup(mock.Anything, paths.Top(), true, false, paths.VersionedHome(topDir)).
+					Cleanup(mock.Anything, paths.Top(), true, false, previousVersionedHome).
 					Return(nil)
 			},
 			args: args{
@@ -291,11 +294,11 @@ func Test_watchCmd(t *testing.T) {
 					&upgrade.UpdateMarker{
 						Version:           "4.5.6",
 						Hash:              "newver",
-						VersionedHome:     "elastic-agent-4.5.6-newver",
+						VersionedHome:     filepath.Join("data", "elastic-agent-4.5.6-newver"),
 						UpdatedOn:         updatedOn,
 						PrevVersion:       "1.2.3",
 						PrevHash:          "prvver",
-						PrevVersionedHome: "elastic-agent-prvver",
+						PrevVersionedHome: filepath.Join("data", "elastic-agent-prvver"),
 						Acked:             false,
 						Action:            nil,
 						Details:           nil,
@@ -304,10 +307,10 @@ func Test_watchCmd(t *testing.T) {
 				)
 				require.NoError(t, err)
 
-				// topdir, prevVersionedHome and prevHash are not taken from the upgrade marker, otherwise they would be
-				// <topDir, "topDir/data/elastic-agent-prvver", "prvver">
+				// topdir, versionedHome and hash are not taken from the upgrade marker, otherwise they would be
+				// <topDir, "data/elastic-agent-4.5.6-newver">
 				installModifier.EXPECT().
-					Cleanup(mock.Anything, paths.Top(), true, false, paths.VersionedHome(topDir)).
+					Cleanup(mock.Anything, paths.Top(), true, false, filepath.Join("data", "elastic-agent-unknow")).
 					Return(nil)
 			},
 			args: args{
@@ -321,7 +324,7 @@ func Test_watchCmd(t *testing.T) {
 			wantErr: assert.NoError,
 		},
 		{
-			name: "Desired outcome is rollback no upgrade details, no rollback and simple cleanup",
+			name: "Default config, no rollback and simple cleanup",
 			setupUpgradeMarker: func(t *testing.T, tmpDir string, watcher *mockAgentWatcher, installModifier *mockInstallationModifier) {
 				dataDirPath := paths.DataFrom(tmpDir)
 				err := os.MkdirAll(dataDirPath, 0755)
@@ -333,11 +336,11 @@ func Test_watchCmd(t *testing.T) {
 					&upgrade.UpdateMarker{
 						Version:           "4.5.6",
 						Hash:              "newver",
-						VersionedHome:     "elastic-agent-4.5.6-newver",
+						VersionedHome:     filepath.Join("data", "elastic-agent-4.5.6-newver"),
 						UpdatedOn:         updatedOn,
 						PrevVersion:       "1.2.3",
 						PrevHash:          "prvver",
-						PrevVersionedHome: "elastic-agent-prvver",
+						PrevVersionedHome: filepath.Join("data", "elastic-agent-prvver"),
 						Acked:             false,
 						Action: &fleetapi.ActionUpgrade{
 							ActionID:   "action-id",
@@ -351,7 +354,7 @@ func Test_watchCmd(t *testing.T) {
 				require.NoError(t, err)
 
 				installModifier.EXPECT().
-					Cleanup(mock.Anything, paths.Top(), true, false, paths.VersionedHome(tmpDir)).
+					Cleanup(mock.Anything, paths.Top(), true, false, filepath.Join("data", "elastic-agent-unknow")).
 					Return(nil)
 			},
 			args: args{