From 29ea9619f8d655a1a2f888c60109f2a800bbdbc4 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 10 Dec 2024 12:31:59 -0800 Subject: [PATCH 01/12] Log warning on same version upgrade to prevent failed status --- ...ning-on-same-version-upgrade-attempts.yaml | 34 +++++++++++++++++++ .../pkg/agent/application/upgrade/upgrade.go | 25 +++++++++----- .../agent/application/upgrade/upgrade_test.go | 17 ++++++++++ .../upgrade_standalone_same_commit_test.go | 2 +- 4 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 changelog/fragments/1733862556-Log-warning-on-same-version-upgrade-attempts.yaml 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/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 43702d44f83..160cae551f4 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -179,6 +179,19 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string u.log.Errorw("Unable to clean downloads before update", "error.message", err, "downloads.path", paths.Downloads()) } + currentVersion := agentVersion{ + version: release.Version(), + snapshot: release.Snapshot(), + hash: release.Commit(), + } + + // check version before download + same, newVersion := isSameVersion(u.log, currentVersion, packageMetadata{}, version) + if same { + u.log.Warn("Upgrade action skipped: upgrade did not occur because its the same version") + return nil, nil + } + det.SetState(details.StateDownloading) sourceURI = u.sourceURI(sourceURI) @@ -206,15 +219,11 @@ 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(), - } - - same, newVersion := isSameVersion(u.log, currentVersion, metadata, version) + // Recheck version here in case of a snapshot->snapshot upgrade on the same version. + same, newVersion = isSameVersion(u.log, currentVersion, metadata, version) if same { - return nil, fmt.Errorf("agent version is already %s", currentVersion) + u.log.Warn("Upgrade action skipped: upgrade did not occur because its the same version") + return nil, nil } u.log.Infow("Unpacking agent package", "version", newVersion) diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index 55c5d3cf996..eefb8f7fe92 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) { diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index a6a2de61087..25e55c85108 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -74,7 +74,7 @@ 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") + assert.ErrorContains(t, err, "failed to find watcher:") }) t.Run(fmt.Sprintf("Upgrade on a repackaged version of agent %s (%s)", currentVersion, unPrivilegedString), func(t *testing.T) { From 96deaee2d555ae8cfb2cea4519b7e6bed11dc937 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 10 Dec 2024 14:01:13 -0800 Subject: [PATCH 02/12] Fix linter --- internal/pkg/agent/application/upgrade/upgrade.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 160cae551f4..0dcce9dd64b 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -186,7 +186,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string } // check version before download - same, newVersion := isSameVersion(u.log, currentVersion, packageMetadata{}, version) + same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version) if same { u.log.Warn("Upgrade action skipped: upgrade did not occur because its the same version") return nil, nil @@ -220,7 +220,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string } // Recheck version here in case of a snapshot->snapshot upgrade on the same version. - same, newVersion = isSameVersion(u.log, currentVersion, metadata, version) + same, newVersion := isSameVersion(u.log, currentVersion, metadata, version) if same { u.log.Warn("Upgrade action skipped: upgrade did not occur because its the same version") return nil, nil From 299ab0b69fc27372a4a4606c2f592902007f9c4c Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Thu, 12 Dec 2024 16:12:06 -0800 Subject: [PATCH 03/12] Fix status reporting --- .../application/coordinator/coordinator.go | 6 ++++ .../pkg/agent/application/upgrade/upgrade.go | 31 ++++++++++--------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go index 3c899735da7..25b490bcfa4 100644 --- a/internal/pkg/agent/application/coordinator/coordinator.go +++ b/internal/pkg/agent/application/coordinator/coordinator.go @@ -574,6 +574,12 @@ 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, but return an error if a same version-upgrade is attempted. + det.SetState(details.StateCompleted) + return err + } c.ClearOverrideState() det.Fail(err) return err diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 0dcce9dd64b..46eb211d1a8 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(), + } + + // check version before download + same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version) + if same { + u.log.Warnf("Upgrade action skipped: %v", ErrUpgradeSameVersion) + 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 @@ -179,19 +193,6 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string u.log.Errorw("Unable to clean downloads before update", "error.message", err, "downloads.path", paths.Downloads()) } - currentVersion := agentVersion{ - version: release.Version(), - snapshot: release.Snapshot(), - hash: release.Commit(), - } - - // check version before download - same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version) - if same { - u.log.Warn("Upgrade action skipped: upgrade did not occur because its the same version") - return nil, nil - } - det.SetState(details.StateDownloading) sourceURI = u.sourceURI(sourceURI) @@ -222,8 +223,8 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string // Recheck version here in case of a snapshot->snapshot upgrade on the same version. same, newVersion := isSameVersion(u.log, currentVersion, metadata, version) if same { - u.log.Warn("Upgrade action skipped: upgrade did not occur because its the same version") - return nil, nil + u.log.Warnf("Upgrade action skipped: %v", ErrUpgradeSameVersion) + return nil, ErrUpgradeSameVersion } u.log.Infow("Unpacking agent package", "version", newVersion) From 6cac6df89e38a6cb832238c02775fc57d45cc715 Mon Sep 17 00:00:00 2001 From: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> Date: Fri, 13 Dec 2024 14:14:14 -0300 Subject: [PATCH 04/12] Update internal/pkg/agent/application/upgrade/upgrade.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Paolo Chilà --- internal/pkg/agent/application/upgrade/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 46eb211d1a8..7d25a4ae4ac 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -172,7 +172,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string // check version before download same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version) if same { - u.log.Warnf("Upgrade action skipped: %v", ErrUpgradeSameVersion) + u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion) return nil, ErrUpgradeSameVersion } From ea65977197e9c02eaad982c45814d5001c044638 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 13 Dec 2024 10:26:49 -0800 Subject: [PATCH 05/12] Detect same version upgrade in version check --- internal/pkg/agent/application/upgrade/upgrade.go | 2 +- testing/integration/upgrade_standalone_same_commit_test.go | 2 +- testing/upgradetest/upgrader.go | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 7d25a4ae4ac..8f8ee45b878 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -223,7 +223,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string // Recheck version here in case of a snapshot->snapshot upgrade on the same version. same, newVersion := isSameVersion(u.log, currentVersion, metadata, version) if same { - u.log.Warnf("Upgrade action skipped: %v", ErrUpgradeSameVersion) + u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion) return nil, ErrUpgradeSameVersion } diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index 25e55c85108..2e741564e2b 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -74,7 +74,7 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) { upgradetest.WithUnprivileged(unprivilegedAvailable), upgradetest.WithDisableHashCheck(true), ) - assert.ErrorContains(t, err, "failed to find watcher:") + assert.ErrorContains(t, err, "upgrade did not occur because it is the same version") }) 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..daef4244d6d 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -332,11 +332,15 @@ func PerformUpgrade( upgradeOutput, err := startFixture.Exec(ctx, upgradeCmdArgs) if err != nil { + logger.Logf("Upgrade err: %v, output: %s", err, upgradeOutput) // Sometimes the gRPC server shuts down before replying to the command which is expected // we can determine this state by the EOF error coming from the server. // If the server is just unavailable/not running, we should not succeed. // Starting with version 8.13.2, this is handled by the upgrade command itself. outputString := string(upgradeOutput) + if strings.Contains(outputString, "upgrade did not occur because it is the same version") { + return fmt.Errorf("upgrade did not occur because it is the same version") + } isConnectionInterrupted := strings.Contains(outputString, "Unavailable") && strings.Contains(outputString, "EOF") if !isConnectionInterrupted { return fmt.Errorf("failed to start agent upgrade to version %q: %w\n%s", endVersionInfo.Binary.Version, err, upgradeOutput) From 5688de5e6e79df3f71ea6e05a2c1adc1cfa44ae6 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Fri, 13 Dec 2024 10:55:03 -0800 Subject: [PATCH 06/12] remove double clear --- internal/pkg/agent/application/coordinator/coordinator.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go index 25b490bcfa4..5c837dbf9fd 100644 --- a/internal/pkg/agent/application/coordinator/coordinator.go +++ b/internal/pkg/agent/application/coordinator/coordinator.go @@ -580,7 +580,6 @@ func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI str det.SetState(details.StateCompleted) return err } - c.ClearOverrideState() det.Fail(err) return err } From 1ea127e5bf65f35a5f1e4bf5f61c787ab7b43eeb Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 17 Dec 2024 10:53:09 -0800 Subject: [PATCH 07/12] Remove pre-check --- .../pkg/agent/application/upgrade/upgrade.go | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 8f8ee45b878..8bf7563f7ca 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -163,19 +163,6 @@ 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(), - } - - // check version before download - same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version) - if same { - 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 @@ -220,7 +207,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) } - // Recheck version here in case of a snapshot->snapshot upgrade on the same version. + currentVersion := agentVersion{ + version: release.Version(), + snapshot: release.Snapshot(), + hash: release.Commit(), + } + same, newVersion := isSameVersion(u.log, currentVersion, metadata, version) if same { u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion) From 72a510679a8cf5298d02a489d0d6d20c430b4e70 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Tue, 17 Dec 2024 11:27:15 -0800 Subject: [PATCH 08/12] Add new func for released version comparisons --- .../pkg/agent/application/upgrade/upgrade.go | 31 ++++++++-- .../agent/application/upgrade/upgrade_test.go | 57 +++++++++++++++++++ 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 8bf7563f7ca..e7e14c8784f 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -163,6 +163,17 @@ 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(), + } + + if isSameReleaseVersion(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 @@ -207,12 +218,6 @@ 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(), - } - same, newVersion := isSameVersion(u.log, currentVersion, metadata, version) if same { u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion) @@ -628,3 +633,17 @@ func IsInProgress(c client.Client, watcherPIDsFetcher func() ([]int, error)) (bo return state.State == cproto.State_UPGRADING, nil } + +// isSameReleaseVersion will if upgradeVersion and currentVersion are equal using only release numbers. +// They are not equal if either are a SNAPSHOT, or if the semver numbers (including prerelease and build identifiers) differ. +func isSameReleaseVersion(current agentVersion, upgradeVersion string) bool { + if current.snapshot { + return false + } + target, _ := agtversion.ParseVersion(upgradeVersion) + 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 eefb8f7fe92..10864d50b93 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1000,3 +1000,60 @@ 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 is same with 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, + }} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expect, isSameReleaseVersion(tc.current, tc.target)) + }) + } +} From 14e78753e6c06295e3348a00de01ae54905fb2c0 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Wed, 18 Dec 2024 07:56:52 -0800 Subject: [PATCH 09/12] do not return an error --- .../application/coordinator/coordinator.go | 5 +-- .../upgrade_standalone_same_commit_test.go | 2 +- testing/upgradetest/upgrader.go | 32 ++++++++++++++++--- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go index 5c837dbf9fd..a04ebf18b9a 100644 --- a/internal/pkg/agent/application/coordinator/coordinator.go +++ b/internal/pkg/agent/application/coordinator/coordinator.go @@ -576,9 +576,10 @@ func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI str if err != nil { c.ClearOverrideState() if errors.Is(err, upgrade.ErrUpgradeSameVersion) { - // Set upgrade state to completed, but return an error if a same version-upgrade is attempted. + c.logger.Info("Same ver upgrade detected.") + // Set upgrade state to completed so update no longer shows in-progress. det.SetState(details.StateCompleted) - return err + return nil } det.Fail(err) return err diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index 2e741564e2b..4f72de4bbe2 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -74,7 +74,7 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) { upgradetest.WithUnprivileged(unprivilegedAvailable), upgradetest.WithDisableHashCheck(true), ) - assert.ErrorContains(t, err, "upgrade did not occur because it is the same version") + 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 daef4244d6d..43ca705ed25 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -332,21 +332,25 @@ func PerformUpgrade( upgradeOutput, err := startFixture.Exec(ctx, upgradeCmdArgs) if err != nil { - logger.Logf("Upgrade err: %v, output: %s", err, upgradeOutput) // Sometimes the gRPC server shuts down before replying to the command which is expected // we can determine this state by the EOF error coming from the server. // If the server is just unavailable/not running, we should not succeed. // Starting with version 8.13.2, this is handled by the upgrade command itself. outputString := string(upgradeOutput) - if strings.Contains(outputString, "upgrade did not occur because it is the same version") { - return fmt.Errorf("upgrade did not occur because it is the same version") - } isConnectionInterrupted := strings.Contains(outputString, "Unavailable") && strings.Contains(outputString, "EOF") if !isConnectionInterrupted { return fmt.Errorf("failed to start agent upgrade to version %q: %w\n%s", endVersionInfo.Binary.Version, err, upgradeOutput) } } + // 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) @@ -657,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 + } + } +} From b4678456a9c6b02f9a6d377b5af93cc574e31f67 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Thu, 19 Dec 2024 11:04:08 -0800 Subject: [PATCH 10/12] Review feedback --- internal/pkg/agent/application/upgrade/upgrade.go | 12 ++++++++---- .../pkg/agent/application/upgrade/upgrade_test.go | 12 ++++++++++-- .../upgrade_standalone_same_commit_test.go | 3 +++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index e7e14c8784f..c809bb9baa7 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -169,7 +169,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string hash: release.Commit(), } - if isSameReleaseVersion(currentVersion, version) { + if isSameReleaseVersion(u.log, currentVersion, version) { u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion) return nil, ErrUpgradeSameVersion } @@ -634,13 +634,17 @@ func IsInProgress(c client.Client, watcherPIDsFetcher func() ([]int, error)) (bo return state.State == cproto.State_UPGRADING, nil } -// isSameReleaseVersion will if upgradeVersion and currentVersion are equal using only release numbers. +// 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(current agentVersion, upgradeVersion string) bool { +func isSameReleaseVersion(log *logger.Logger, current agentVersion, upgradeVersion string) bool { if current.snapshot { return false } - target, _ := agtversion.ParseVersion(upgradeVersion) + 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 diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index 10864d50b93..71deb314ab3 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1030,7 +1030,7 @@ func TestIsSameReleaseVersion(t *testing.T) { target: "1.2.4", expect: false, }, { - name: "target version is same with pre-release", + name: "target version has same major.minor.patch, different pre-release", current: agentVersion{ version: "1.2.3", }, @@ -1050,10 +1050,18 @@ func TestIsSameReleaseVersion(t *testing.T) { }, 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) { - assert.Equal(t, tc.expect, isSameReleaseVersion(tc.current, tc.target)) + 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 4f72de4bbe2..e493d184ac2 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -74,6 +74,9 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) { upgradetest.WithUnprivileged(unprivilegedAvailable), upgradetest.WithDisableHashCheck(true), ) + // 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) }) From eff680d21eec66edf4479e31a32f144952ace91a Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Mon, 30 Dec 2024 09:36:01 -0800 Subject: [PATCH 11/12] Add comments --- internal/pkg/agent/application/upgrade/upgrade.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index c809bb9baa7..122aac54825 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -169,6 +169,8 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string 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 @@ -218,6 +220,8 @@ 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) } + // 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 { u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion) From 0232a31001b006d3705af4be525b6357119caf3b Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Thu, 2 Jan 2025 09:03:39 -0800 Subject: [PATCH 12/12] Remove extra log line --- internal/pkg/agent/application/coordinator/coordinator.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/pkg/agent/application/coordinator/coordinator.go b/internal/pkg/agent/application/coordinator/coordinator.go index ba282b2ffd0..0394d301df9 100644 --- a/internal/pkg/agent/application/coordinator/coordinator.go +++ b/internal/pkg/agent/application/coordinator/coordinator.go @@ -576,7 +576,6 @@ func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI str if err != nil { c.ClearOverrideState() if errors.Is(err, upgrade.ErrUpgradeSameVersion) { - c.logger.Info("Same ver upgrade detected.") // Set upgrade state to completed so update no longer shows in-progress. det.SetState(details.StateCompleted) return nil