diff --git a/changelog/fragments/1733862556-Log-warning-on-same-version-upgrade-attempts.yaml b/changelog/fragments/1733862556-Log-warning-on-same-version-upgrade-attempts.yaml new file mode 100644 index 00000000000..fb2f01d09ed --- /dev/null +++ b/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 diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go index 718192f55f6..0394d301df9 100644 --- a/internal/pkg/agent/application/coordinator/coordinator.go +++ b/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 } diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 43702d44f83..122aac54825 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/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 +} diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index 55c5d3cf996..71deb314ab3 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/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)) + }) + } +} diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index a6a2de61087..e493d184ac2 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/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) { diff --git a/testing/upgradetest/upgrader.go b/testing/upgradetest/upgrader.go index 7094e49fda5..43ca705ed25 100644 --- a/testing/upgradetest/upgrader.go +++ b/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 + } + } +}