Log warning on same version upgrade to prevent failed status (#6273) (#…

…6473)

Log warning on same version upgrade and filter error to prevent upgrade status reporting.
If a user attempts to upgrade to the same version, the elastic-agent upgrade call will exit with an error, but the agent's status will be cleared of any upgrade markers.

Include an earlier check for same-version upgrades before artifacts are downloaded to prevent unnecessary downloads

---------

Co-authored-by: Paolo Chilà <[email protected]>
(cherry picked from commit d354d9f)

Co-authored-by: Michel Laterman <[email protected]>

changelog/fragments/1733862556-Log-warning-on-same-version-upgrade-attempts.yaml

@@ -0,0 +1,34 @@
+# 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: Log warning on same version upgrade attempts
+
+# 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: |
+  Log a warning instead of reporting an error whan a same-version upgrade is
+  attempted. This prevents the agent from reporting a "failed" status.
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+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/owner/repo/1234
+
+# 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/elastic/elastic-agent/issues/6186

internal/pkg/agent/application/coordinator/coordinator.go

@@ -575,6 +575,11 @@ func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI str
 	cb, err := c.upgradeMgr.Upgrade(ctx, version, sourceURI, action, det, skipVerifyOverride, skipDefaultPgp, pgpBytes...)
 	if err != nil {
 		c.ClearOverrideState()
+		if errors.Is(err, upgrade.ErrUpgradeSameVersion) {
+			// Set upgrade state to completed so update no longer shows in-progress.
+			det.SetState(details.StateCompleted)
+			return nil
+		}
 		det.Fail(err)
 		return err
 	}

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

@@ -55,6 +55,7 @@ var agentArtifact = artifact.Artifact{
 }
 
 var ErrWatcherNotStarted = errors.New("watcher did not start in time")
+var ErrUpgradeSameVersion = errors.New("upgrade did not occur because it is the same version")
 
 // Upgrader performs an upgrade
 type Upgrader struct {
@@ -162,6 +163,19 @@ func (av agentVersion) String() string {
 func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, det *details.Details, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
 	u.log.Infow("Upgrading agent", "version", version, "source_uri", sourceURI)
 
+	currentVersion := agentVersion{
+		version:  release.Version(),
+		snapshot: release.Snapshot(),
+		hash:     release.Commit(),
+	}
+
+	// Compare versions and exit before downloading anything if the upgrade
+	// is for the same release version that is currently running
+	if isSameReleaseVersion(u.log, currentVersion, version) {
+		u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion)
+		return nil, ErrUpgradeSameVersion
+	}
+
 	// Inform the Upgrade Marker Watcher that we've started upgrading. Note that this
 	// is only possible to do in-memory since, today, the  process that's initiating
 	// the upgrade is the same as the Agent process in which the Upgrade Marker Watcher is
@@ -206,15 +220,12 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
 		return nil, fmt.Errorf("reading metadata for elastic agent version %s package %q: %w", version, archivePath, err)
 	}
 
-	currentVersion := agentVersion{
-		version:  release.Version(),
-		snapshot: release.Snapshot(),
-		hash:     release.Commit(),
-	}
-
+	// Compare the downloaded version (including git hash) to see if we need to upgrade
+	// versions are the same if the numbers and hash match which may occur in a SNAPSHOT -> SNAPSHOT upgrage
 	same, newVersion := isSameVersion(u.log, currentVersion, metadata, version)
 	if same {
-		return nil, fmt.Errorf("agent version is already %s", currentVersion)
+		u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion)
+		return nil, ErrUpgradeSameVersion
 	}
 
 	u.log.Infow("Unpacking agent package", "version", newVersion)
@@ -626,3 +637,21 @@ func IsInProgress(c client.Client, watcherPIDsFetcher func() ([]int, error)) (bo
 
 	return state.State == cproto.State_UPGRADING, nil
 }
+
+// isSameReleaseVersion will return true if upgradeVersion and currentVersion are equal using only release numbers and SNAPSHOT prerelease qualifiers.
+// They are not equal if either are a SNAPSHOT, or if the semver numbers (including prerelease and build identifiers) differ.
+func isSameReleaseVersion(log *logger.Logger, current agentVersion, upgradeVersion string) bool {
+	if current.snapshot {
+		return false
+	}
+	target, err := agtversion.ParseVersion(upgradeVersion)
+	if err != nil {
+		log.Warnw("Unable too parse version for released version comparison", upgradeVersion, err)
+		return false
+	}
+	targetVersion, targetSnapshot := target.ExtractSnapshotFromVersionString()
+	if targetSnapshot {
+		return false
+	}
+	return current.version == targetVersion
+}

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

@@ -733,6 +733,23 @@ func TestIsSameVersion(t *testing.T) {
 				newVersion: agentVersion123SNAPSHOTghijkl,
 			},
 		},
+		{
+			name: "same version and snapshot, no hash (SNAPSHOT upgrade before download)",
+			args: args{
+				current: agentVersion123SNAPSHOTabcdef,
+				metadata: packageMetadata{
+					manifest: nil,
+				},
+				version: "1.2.3-SNAPSHOT",
+			},
+			want: want{
+				same: false,
+				newVersion: agentVersion{
+					version:  "1.2.3",
+					snapshot: true,
+				},
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -983,3 +1000,68 @@ func Test_selectWatcherExecutable(t *testing.T) {
 		})
 	}
 }
+
+func TestIsSameReleaseVersion(t *testing.T) {
+	tests := []struct {
+		name    string
+		current agentVersion
+		target  string
+		expect  bool
+	}{{
+		name: "current version is snapshot",
+		current: agentVersion{
+			version:  "1.2.3",
+			snapshot: true,
+		},
+		target: "1.2.3",
+		expect: false,
+	}, {
+		name: "target version is snapshot",
+		current: agentVersion{
+			version: "1.2.3",
+		},
+		target: "1.2.3-SNAPSHOT",
+		expect: false,
+	}, {
+		name: "target version is different version",
+		current: agentVersion{
+			version: "1.2.3",
+		},
+		target: "1.2.4",
+		expect: false,
+	}, {
+		name: "target version has same major.minor.patch, different pre-release",
+		current: agentVersion{
+			version: "1.2.3",
+		},
+		target: "1.2.3-custom.info",
+		expect: false,
+	}, {
+		name: "target version is same with build",
+		current: agentVersion{
+			version: "1.2.3",
+		},
+		target: "1.2.3+buildID",
+		expect: false,
+	}, {
+		name: "target version is same",
+		current: agentVersion{
+			version: "1.2.3",
+		},
+		target: "1.2.3",
+		expect: true,
+	}, {
+		name: "target version is invalid",
+		current: agentVersion{
+			version: "1.2.3",
+		},
+		target: "a.b.c",
+		expect: false,
+	}}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			log, _ := loggertest.New(tc.name)
+			assert.Equal(t, tc.expect, isSameReleaseVersion(log, tc.current, tc.target))
+		})
+	}
+}

testing/integration/upgrade_standalone_same_commit_test.go

@@ -74,7 +74,10 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) {
 			upgradetest.WithUnprivileged(unprivilegedAvailable),
 			upgradetest.WithDisableHashCheck(true),
 		)
-		assert.ErrorContainsf(t, err, fmt.Sprintf("agent version is already %s", currentVersion), "upgrade should fail indicating we are already at the same version")
+		// PerformUpgrade will exit after calling `elastic-agent upgrade ...` if a subsequent call to
+		// `elastic-agent status` returns the running state with no upgrade details. This indicates that
+		// the agent did a nop upgrade.
+		assert.NoError(t, err)
 	})
 
 	t.Run(fmt.Sprintf("Upgrade on a repackaged version of agent %s (%s)", currentVersion, unPrivilegedString), func(t *testing.T) {

testing/upgradetest/upgrader.go

@@ -343,6 +343,14 @@ func PerformUpgrade(
 		}
 	}
 
+	// check status
+	if status := getStatus(ctx, startFixture); status != nil {
+		if status.State == 2 && status.UpgradeDetails == nil {
+			logger.Logf("Agent status indicates no upgrade is in progress.")
+			return nil
+		}
+	}
+
 	// wait for the watcher to show up
 	logger.Logf("waiting for upgrade watcher to start")
 	err = WaitForWatcher(ctx, 5*time.Minute, 10*time.Second)
@@ -653,3 +661,23 @@ func windowsBaseTemp() (string, error) {
 	}
 	return baseTmp, nil
 }
+
+// getStatus will attempt to get the agent status with retries if enounters an error
+func getStatus(ctx context.Context, fixture *atesting.Fixture) *atesting.AgentStatusOutput {
+	ctx, cancel := context.WithTimeout(ctx, time.Second*30)
+	defer cancel()
+	ticker := time.NewTicker(time.Second * 5)
+	defer ticker.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			return nil
+		case <-ticker.C:
+			status, err := fixture.ExecStatus(ctx)
+			if err != nil {
+				continue
+			}
+			return &status
+		}
+	}
+}