Always launch more recent watcher (#4491)

* remove GetPreviousVersion()

* implement correct prerelease comparison of parsed versions

* WIP select newer watcher

* changelog

* Create a parsed semver when reading agent package version and use it for comparison

* Apply suggestions from code review

Co-authored-by: Blake Rouse <blake.rouse@elastic.co>

* fix linter errors

---------

Co-authored-by: Blake Rouse <blake.rouse@elastic.co>

Author: pchila

changelog/fragments/1712737337-always-launch-more-recent-watcher.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# Change summary; a 80ish characters long description of the change.
+summary: Always select the more recent watcher during agent upgrade/downgrade
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+#description:
+
+# Affected component; a word indicating the component this changeset affects.
+component: elastic-agent
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/4491
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+#issue: https://github.com/owner/repo/1234

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

@@ -16,6 +16,7 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
 	"github.com/elastic/elastic-agent/internal/pkg/fleetapi"
 	"github.com/elastic/elastic-agent/pkg/core/logger"
+	"github.com/elastic/elastic-agent/pkg/version"
 )
 
 const markerFilename = ".update-marker"
@@ -116,6 +117,7 @@ func newMarkerSerializer(m *UpdateMarker) *updateMarkerSerializer {
 }
 
 type agentInstall struct {
+	parsedVersion *version.ParsedSemVer
 	version       string
 	hash          string
 	versionedHome string

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

@@ -36,6 +36,7 @@ import (
 	"github.com/elastic/elastic-agent/pkg/control/v2/cproto"
 	"github.com/elastic/elastic-agent/pkg/core/logger"
 	agtversion "github.com/elastic/elastic-agent/pkg/version"
+	currentagtversion "github.com/elastic/elastic-agent/version"
 )
 
 const (
@@ -272,12 +273,15 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
 	// while the `previous` install is the currently executing elastic-agent that is no longer reachable via the symlink.
 	// After the restart at the end of the function, everything lines up correctly.
 	current := agentInstall{
+		parsedVersion: parsedVersion,
 		version:       version,
 		hash:          unpackRes.Hash,
 		versionedHome: unpackRes.VersionedHome,
 	}
 
+	previousParsedVersion := currentagtversion.GetParsedAgentPackageVersion()
 	previous := agentInstall{
+		parsedVersion: previousParsedVersion,
 		version:       release.VersionWithSnapshot(),
 		hash:          release.Commit(),
 		versionedHome: currentVersionedHome,
@@ -293,15 +297,8 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
 		return nil, goerrors.Join(err, rollbackErr)
 	}
 
-	minParsedVersionForNewUpdateMarker := agtversion.NewParsedSemVer(8, 13, 0, "", "")
-	var watcherExecutable string
-	if parsedVersion.Less(*minParsedVersionForNewUpdateMarker) {
-		// use the current agent executable for watch, if downgrading the old agent doesn't understand the current agent's path structure.
-		watcherExecutable = paths.BinaryPath(paths.VersionedHome(paths.Top()), agentName)
-	} else {
-		// use the new agent executable as it should be able to parse the new update marker
-		watcherExecutable = paths.BinaryPath(filepath.Join(paths.Top(), unpackRes.VersionedHome), agentName)
-	}
+	var watcherExecutable = selectWatcherExecutable(paths.Top(), previous, current)
+
 	var watcherCmd *exec.Cmd
 	if watcherCmd, err = InvokeWatcher(u.log, watcherExecutable); err != nil {
 		u.log.Errorw("Rolling back: starting watcher failed", "error.message", err)
@@ -328,6 +325,17 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
 	return cb, nil
 }
 
+func selectWatcherExecutable(topDir string, previous agentInstall, current agentInstall) string {
+	// check if the upgraded version is less than the previous (currently installed) version
+	if current.parsedVersion.Less(*previous.parsedVersion) {
+		// use the current agent executable for watch, if downgrading the old agent doesn't understand the current agent's path structure.
+		return paths.BinaryPath(filepath.Join(topDir, previous.versionedHome), agentName)
+	} else {
+		// use the new agent executable as it should be able to parse the new update marker
+		return paths.BinaryPath(filepath.Join(topDir, current.versionedHome), agentName)
+	}
+}
+
 func waitForWatcher(ctx context.Context, log *logger.Logger, markerFilePath string, waitTime time.Duration) error {
 	return waitForWatcherWithTimeoutCreationFunc(ctx, log, markerFilePath, waitTime, context.WithTimeout)
 }
@@ -417,8 +425,7 @@ func isSameVersion(log *logger.Logger, current agentVersion, metadata packageMet
 	} else {
 		// extract version info from the version string (we can ignore parsing errors as it would have never passed the download step)
 		parsedVersion, _ := agtversion.ParseVersion(upgradeVersion)
-		newVersion.version = strings.TrimSuffix(parsedVersion.VersionWithPrerelease(), snapshotSuffix)
-		newVersion.snapshot = parsedVersion.IsSnapshot()
+		newVersion.version, newVersion.snapshot = parsedVersion.ExtractSnapshotFromVersionString()
 	}
 	newVersion.hash = metadata.hash
 

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

@@ -22,6 +22,7 @@ import (
 
 	"github.com/elastic/elastic-agent-libs/transport/httpcommon"
 	"github.com/elastic/elastic-agent-libs/transport/tlscommon"
+	"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
@@ -32,6 +33,7 @@ import (
 	"github.com/elastic/elastic-agent/pkg/control/v2/client/mocks"
 	"github.com/elastic/elastic-agent/pkg/control/v2/cproto"
 	"github.com/elastic/elastic-agent/pkg/core/logger"
+	agtversion "github.com/elastic/elastic-agent/pkg/version"
 )
 
 func Test_CopyFile(t *testing.T) {
@@ -693,7 +695,7 @@ func TestIsSameVersion(t *testing.T) {
 					manifest: nil,
 					hash:     "abcdef",
 				},
-				version: "1.2.3-repackaged-SNAPSHOT",
+				version: "1.2.3-SNAPSHOT.repackaged",
 			},
 			want: want{
 				same:       false,
@@ -903,3 +905,80 @@ func writeState(t *testing.T, path string, state details.State) {
 		assert.NoError(t, err, "error writing out the test upgrade marker")
 	}
 }
+
+func Test_selectWatcherExecutable(t *testing.T) {
+	type args struct {
+		previous agentInstall
+		current  agentInstall
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "Simple upgrade, we should launch the new (current) watcher",
+			args: args{
+				previous: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(1, 2, 3, "", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-1.2.3-somehash"),
+				},
+				current: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(4, 5, 6, "", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-4.5.6-someotherhash"),
+				},
+			},
+			want: filepath.Join("data", "elastic-agent-4.5.6-someotherhash"),
+		},
+		{
+			name: "Simple downgrade, we should launch the currently installed (previous) watcher",
+			args: args{
+				previous: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(4, 5, 6, "", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-4.5.6-someotherhash"),
+				},
+				current: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(1, 2, 3, "", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-1.2.3-somehash"),
+				},
+			},
+			want: filepath.Join("data", "elastic-agent-4.5.6-someotherhash"),
+		},
+		{
+			name: "Upgrade from snapshot to released version, we should launch the new (current) watcher",
+			args: args{
+				previous: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(1, 2, 3, "SNAPSHOT", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-1.2.3-SNAPSHOT-somehash"),
+				},
+				current: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(1, 2, 3, "", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-1.2.3-someotherhash"),
+				},
+			},
+			want: filepath.Join("data", "elastic-agent-1.2.3-someotherhash"),
+		},
+		{
+			name: "Downgrade from released version to SNAPSHOT, we should launch the currently installed (previous) watcher",
+			args: args{
+				previous: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(1, 2, 3, "", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-1.2.3-somehash"),
+				},
+				current: agentInstall{
+					parsedVersion: agtversion.NewParsedSemVer(1, 2, 3, "SNAPSHOT", ""),
+					versionedHome: filepath.Join("data", "elastic-agent-1.2.3-SNAPSHOT-someotherhash"),
+				},
+			},
+
+			want: filepath.Join("data", "elastic-agent-1.2.3-somehash"),
+		},
+	}
+	// Just need a top dir path. This test does not make any operation on the filesystem, so a temp dir path is as good as any
+	fakeTopDir := filepath.Join(t.TempDir(), "Elastic", "Agent")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, paths.BinaryPath(filepath.Join(fakeTopDir, tt.want), agentName), selectWatcherExecutable(fakeTopDir, tt.args.previous, tt.args.current), "selectWatcherExecutable(%v, %v)", tt.args.previous, tt.args.current)
+		})
+	}
+}