From 0b4a2d0c3330e6a4a719a918b89291a306fd8fae Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Mon, 19 May 2025 12:22:29 -0700 Subject: [PATCH 01/19] Pass rollback window duration to upgrade watcher command --- internal/pkg/agent/application/application.go | 2 +- .../pkg/agent/application/upgrade/rollback.go | 4 +-- .../application/upgrade/rollback_darwin.go | 4 ++- .../application/upgrade/rollback_linux.go | 4 ++- .../application/upgrade/rollback_windows.go | 4 ++- .../application/upgrade/step_download.go | 2 +- .../pkg/agent/application/upgrade/upgrade.go | 36 ++++++++++--------- .../agent/application/upgrade/upgrade_test.go | 20 +++++------ internal/pkg/agent/cmd/run.go | 2 +- internal/pkg/agent/cmd/watch.go | 20 ++++++++--- internal/pkg/agent/configuration/upgrade.go | 8 ++--- .../pkg/agent/configuration/upgrade_test.go | 6 ++-- 12 files changed, 65 insertions(+), 47 deletions(-) diff --git a/internal/pkg/agent/application/application.go b/internal/pkg/agent/application/application.go index f887e37d0dc..45190169351 100644 --- a/internal/pkg/agent/application/application.go +++ b/internal/pkg/agent/application/application.go @@ -118,7 +118,7 @@ func New( // monitoring is not supported in bootstrap mode https://github.com/elastic/elastic-agent/issues/1761 isMonitoringSupported := !disableMonitoring && cfg.Settings.V1MonitoringEnabled - upgrader, err := upgrade.NewUpgrader(log, cfg.Settings.DownloadConfig, agentInfo) + upgrader, err := upgrade.NewUpgrader(log, cfg.Settings.DownloadConfig, cfg.Settings.Upgrade, agentInfo) if err != nil { return nil, nil, nil, fmt.Errorf("failed to create upgrader: %w", err) } diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 90be1bbe2df..f91cabaa875 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -144,13 +144,13 @@ func cleanup(log *logger.Logger, topDirPath, currentVersionedHome, currentHash s // InvokeWatcher invokes an agent instance using watcher argument for watching behavior of // agent during upgrade period. -func InvokeWatcher(log *logger.Logger, agentExecutable string) (*exec.Cmd, error) { +func InvokeWatcher(log *logger.Logger, agentExecutable string, rollbackWindow time.Duration) (*exec.Cmd, error) { if !IsUpgradeable() { log.Info("agent is not upgradable, not starting watcher") return nil, nil } - cmd := invokeCmd(agentExecutable) + cmd := invokeCmd(agentExecutable, rollbackWindow) log.Infow("Starting upgrade watcher", "path", cmd.Path, "args", cmd.Args, "env", cmd.Env, "dir", cmd.Dir) if err := cmd.Start(); err != nil { return nil, fmt.Errorf("failed to start Upgrade Watcher: %w", err) diff --git a/internal/pkg/agent/application/upgrade/rollback_darwin.go b/internal/pkg/agent/application/upgrade/rollback_darwin.go index 041abf11b40..d1d97957a0f 100644 --- a/internal/pkg/agent/application/upgrade/rollback_darwin.go +++ b/internal/pkg/agent/application/upgrade/rollback_darwin.go @@ -7,6 +7,7 @@ package upgrade import ( + "fmt" "os" "os/exec" "syscall" @@ -21,11 +22,12 @@ const ( afterRestartDelay = 2 * time.Second ) -func invokeCmd(agentExecutable string) *exec.Cmd { +func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { // #nosec G204 -- user cannot inject any parameters to this command cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), + "--rollback.window", fmt.Sprintf("%ds", rollbackWindow.Seconds()), ) var cred = &syscall.Credential{ diff --git a/internal/pkg/agent/application/upgrade/rollback_linux.go b/internal/pkg/agent/application/upgrade/rollback_linux.go index bdaf918a2b6..0536ee6e2c2 100644 --- a/internal/pkg/agent/application/upgrade/rollback_linux.go +++ b/internal/pkg/agent/application/upgrade/rollback_linux.go @@ -7,6 +7,7 @@ package upgrade import ( + "fmt" "os" "os/exec" "syscall" @@ -21,11 +22,12 @@ const ( afterRestartDelay = 2 * time.Second ) -func invokeCmd(agentExecutable string) *exec.Cmd { +func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { // #nosec G204 -- user cannot inject any parameters to this command cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), + "--rollback.window", fmt.Sprintf("%ds", rollbackWindow.Seconds()), ) var cred = &syscall.Credential{ diff --git a/internal/pkg/agent/application/upgrade/rollback_windows.go b/internal/pkg/agent/application/upgrade/rollback_windows.go index b7c273c9385..a33d9253617 100644 --- a/internal/pkg/agent/application/upgrade/rollback_windows.go +++ b/internal/pkg/agent/application/upgrade/rollback_windows.go @@ -7,6 +7,7 @@ package upgrade import ( + "fmt" "os/exec" "time" @@ -19,11 +20,12 @@ const ( afterRestartDelay = 20 * time.Second ) -func invokeCmd(agentExecutable string) *exec.Cmd { +func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { // #nosec G204 -- user cannot inject any parameters to this command cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), + "--rollback.window", fmt.Sprintf("%ds", rollbackWindow.Seconds()), ) return cmd } diff --git a/internal/pkg/agent/application/upgrade/step_download.go b/internal/pkg/agent/application/upgrade/step_download.go index 58d56c81f52..7a2e4ff0afd 100644 --- a/internal/pkg/agent/application/upgrade/step_download.go +++ b/internal/pkg/agent/application/upgrade/step_download.go @@ -50,7 +50,7 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, parsedVersion *agtversi pgpBytes = u.appendFallbackPGP(parsedVersion, pgpBytes) // do not update source config - settings := *u.settings + settings := *u.downloadSettings var downloaderFunc downloader var factory downloaderFactory var verifier download.Verifier diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 19d3b67cb2b..9ee347df534 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -70,12 +70,13 @@ func init() { // Upgrader performs an upgrade type Upgrader struct { - log *logger.Logger - settings *artifact.Config - agentInfo info.Agent - upgradeable bool - fleetServerURI string - markerWatcher MarkerWatcher + log *logger.Logger + downloadSettings *artifact.Config + upgradeSettings *configuration.UpgradeConfig + agentInfo info.Agent + upgradeable bool + fleetServerURI string + markerWatcher MarkerWatcher } // IsUpgradeable when agent is installed and running as a service or flag was provided. @@ -86,13 +87,14 @@ func IsUpgradeable() bool { } // NewUpgrader creates an upgrader which is capable of performing upgrade operation -func NewUpgrader(log *logger.Logger, settings *artifact.Config, agentInfo info.Agent) (*Upgrader, error) { +func NewUpgrader(log *logger.Logger, downloadSettings *artifact.Config, upgradeSettings *configuration.UpgradeConfig, agentInfo info.Agent) (*Upgrader, error) { return &Upgrader{ - log: log, - settings: settings, - agentInfo: agentInfo, - upgradeable: IsUpgradeable(), - markerWatcher: newMarkerFileWatcher(markerFilePath(paths.Data()), log), + log: log, + downloadSettings: downloadSettings, + upgradeSettings: upgradeSettings, + agentInfo: agentInfo, + upgradeable: IsUpgradeable(), + markerWatcher: newMarkerFileWatcher(markerFilePath(paths.Data()), log), }, nil } @@ -133,17 +135,17 @@ func (u *Upgrader) Reload(rawConfig *config.Config) error { if cfg.Settings.DownloadConfig.SourceURI != "" { u.log.Infof("Source URI changed from %q to %q", - u.settings.SourceURI, + u.downloadSettings.SourceURI, cfg.Settings.DownloadConfig.SourceURI) } else { // source uri unset, reset to default u.log.Infof("Source URI reset from %q to %q", - u.settings.SourceURI, + u.downloadSettings.SourceURI, artifact.DefaultSourceURI) cfg.Settings.DownloadConfig.SourceURI = artifact.DefaultSourceURI } - u.settings = cfg.Settings.DownloadConfig + u.downloadSettings = cfg.Settings.DownloadConfig return nil } @@ -352,7 +354,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string watcherExecutable := selectWatcherExecutable(paths.Top(), previous, current) var watcherCmd *exec.Cmd - if watcherCmd, err = InvokeWatcher(u.log, watcherExecutable); err != nil { + if watcherCmd, err = InvokeWatcher(u.log, watcherExecutable, u.upgradeSettings.Rollback.Window); err != nil { u.log.Errorw("Rolling back: starting watcher failed", "error.message", err) rollbackErr := rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) return nil, goerrors.Join(err, rollbackErr) @@ -477,7 +479,7 @@ func (u *Upgrader) sourceURI(retrievedURI string) string { return retrievedURI } - return u.settings.SourceURI + return u.downloadSettings.SourceURI } func extractAgentVersion(metadata packageMetadata, upgradeVersion string) agentVersion { diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index 17d19252f6e..e08c13dbecd 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -271,21 +271,21 @@ func TestUpgraderReload(t *testing.T) { log, _ := loggertest.New("") u := Upgrader{ - log: log, - settings: artifact.DefaultConfig(), + log: log, + downloadSettings: artifact.DefaultConfig(), } err := u.Reload(config.MustNewConfigFrom(cfgyaml)) require.NoError(t, err, "error reloading config") - assert.Equal(t, &want, u.settings) + assert.Equal(t, &want, u.downloadSettings) } func TestUpgraderAckAction(t *testing.T) { log, _ := loggertest.New("") u := Upgrader{ - log: log, - settings: artifact.DefaultConfig(), + log: log, + downloadSettings: artifact.DefaultConfig(), } action := fleetapi.NewAction(fleetapi.ActionTypeUpgrade) @@ -574,8 +574,8 @@ agent.download: log, _ := loggertest.New("") u := Upgrader{ - log: log, - settings: artifact.DefaultConfig(), + log: log, + downloadSettings: artifact.DefaultConfig(), } cfg, err := config.NewConfigFrom(tc.cfg) @@ -584,11 +584,11 @@ agent.download: err = u.Reload(cfg) require.NoError(t, err, "error reloading config") - assert.Equal(t, tc.sourceURL, u.settings.SourceURI) + assert.Equal(t, tc.sourceURL, u.downloadSettings.SourceURI) if tc.proxyURL != "" { - require.NotNilf(t, u.settings.Proxy.URL, + require.NotNilf(t, u.downloadSettings.Proxy.URL, "ProxyURI should not be nil, want %s", tc.proxyURL) - assert.Equal(t, tc.proxyURL, u.settings.Proxy.URL.String()) + assert.Equal(t, tc.proxyURL, u.downloadSettings.Proxy.URL.String()) } }) } diff --git a/internal/pkg/agent/cmd/run.go b/internal/pkg/agent/cmd/run.go index f91f2a37790..edb4254a622 100644 --- a/internal/pkg/agent/cmd/run.go +++ b/internal/pkg/agent/cmd/run.go @@ -256,7 +256,7 @@ func runElasticAgent(ctx context.Context, cancel context.CancelFunc, override ap } // initiate agent watcher - if _, err := upgrade.InvokeWatcher(l, paths.TopBinaryPath()); err != nil { + if _, err := upgrade.InvokeWatcher(l, paths.TopBinaryPath(), cfg.Settings.Upgrade.Rollback.Window); err != nil { // we should not fail because watcher is not working l.Error(errors.New(err, "failed to invoke rollback watcher")) } diff --git a/internal/pkg/agent/cmd/watch.go b/internal/pkg/agent/cmd/watch.go index f203c1814f7..8e93b46448f 100644 --- a/internal/pkg/agent/cmd/watch.go +++ b/internal/pkg/agent/cmd/watch.go @@ -33,8 +33,9 @@ import ( ) const ( - watcherName = "elastic-agent-watcher" - watcherLockFile = "watcher.lock" + watcherName = "elastic-agent-watcher" + watcherLockFile = "watcher.lock" + flagRollbackWindow = "rollback-window" ) func newWatchCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command { @@ -42,7 +43,7 @@ func newWatchCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command Use: "watch", Short: "Watch the Elastic Agent for failures and initiate rollback", Long: `This command watches Elastic Agent for failures and initiates rollback if necessary.`, - Run: func(_ *cobra.Command, _ []string) { + Run: func(c *cobra.Command, _ []string) { cfg := getConfig(streams) log, err := configuredLogger(cfg, watcherName) if err != nil { @@ -53,7 +54,7 @@ func newWatchCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command // Make sure to flush any buffered logs before we're done. defer log.Sync() //nolint:errcheck // flushing buffered logs is best effort. - if err := watchCmd(log, paths.Top(), cfg.Settings.Upgrade.Watcher, new(upgradeAgentWatcher), new(upgradeInstallationModifier)); err != nil { + if err := watchCmd(c, log, paths.Top(), cfg.Settings.Upgrade.Watcher, new(upgradeAgentWatcher), new(upgradeInstallationModifier)); err != nil { log.Errorw("Watch command failed", "error.message", err) fmt.Fprintf(streams.Err, "Watch command failed: %v\n%s\n", err, troubleshootMessage()) os.Exit(4) @@ -61,6 +62,8 @@ func newWatchCommandWithArgs(_ []string, streams *cli.IOStreams) *cobra.Command }, } + cmd.Flags().DurationP(flagRollbackWindow, "", configuration.DefaultRollbackWindowDuration, "Duration in which Agent may be manually rolled back") + return cmd } @@ -73,7 +76,7 @@ type installationModifier interface { Rollback(ctx context.Context, log *logger.Logger, c client.Client, topDirPath, prevVersionedHome, prevHash string) error } -func watchCmd(log *logp.Logger, topDir string, cfg *configuration.UpgradeWatcherConfig, watcher agentWatcher, installModifier installationModifier) error { +func watchCmd(cmd *cobra.Command, log *logp.Logger, topDir string, cfg *configuration.UpgradeWatcherConfig, watcher agentWatcher, installModifier installationModifier) error { log.Infow("Upgrade Watcher started", "process.pid", os.Getpid(), "agent.version", version.GetAgentPackageVersion(), "config", cfg) dataDir := paths.DataFrom(topDir) marker, err := upgrade.LoadMarker(dataDir) @@ -102,6 +105,12 @@ func watchCmd(log *logp.Logger, topDir string, cfg *configuration.UpgradeWatcher _ = locker.Unlock() }() + rollbackWindow, err := cmd.Flags().GetDuration(flagRollbackWindow) + if err != nil { + return fmt.Errorf("failed to retrieve %s flag value while watching the agent upgrade: %w", flagRollbackWindow, err) + } + _ = rollbackWindow // TODO: use rollbackWindow when https://github.com/elastic/elastic-agent/issues/6884 is implemented + isWithinGrace, tilGrace := gracePeriod(marker, cfg.GracePeriod) if isTerminalState(marker) || !isWithinGrace { stateString := "" @@ -109,6 +118,7 @@ func watchCmd(log *logp.Logger, topDir string, cfg *configuration.UpgradeWatcher stateString = string(marker.Details.State) } log.Infof("not within grace [updatedOn %v] %v or agent have been rolled back [state: %s]", marker.UpdatedOn.String(), time.Since(marker.UpdatedOn).String(), stateString) + // if it is started outside of upgrade loop // if we're not within grace and marker is still there it might mean // that cleanup was not performed ok, cleanup everything except current version diff --git a/internal/pkg/agent/configuration/upgrade.go b/internal/pkg/agent/configuration/upgrade.go index 405b405ec46..ed1f6f395b2 100644 --- a/internal/pkg/agent/configuration/upgrade.go +++ b/internal/pkg/agent/configuration/upgrade.go @@ -13,9 +13,9 @@ const ( // interval between checks for new (upgraded) Agent returning an error status. defaultStatusCheckInterval = 30 * time.Second - // period during which an upgraded Agent can be asked to rollback to the previous - // Agent version on disk. - defaultRollbackWindowDuration = 7 * 24 * time.Hour // 7 days + // DefaultRollbackWindowDuration is the period during which an upgraded Agent can be asked to + // rollback to the previous Agent version on disk. + DefaultRollbackWindowDuration = 7 * 24 * time.Hour // 7 days ) // UpgradeConfig is the configuration related to Agent upgrades. @@ -45,7 +45,7 @@ func DefaultUpgradeConfig() *UpgradeConfig { }, }, Rollback: &UpgradeRollbackConfig{ - Window: defaultRollbackWindowDuration, + Window: DefaultRollbackWindowDuration, }, } } diff --git a/internal/pkg/agent/configuration/upgrade_test.go b/internal/pkg/agent/configuration/upgrade_test.go index 0005e15ace8..0888662c0d6 100644 --- a/internal/pkg/agent/configuration/upgrade_test.go +++ b/internal/pkg/agent/configuration/upgrade_test.go @@ -28,7 +28,7 @@ func TestParseUpgradeConfig(t *testing.T) { }, }, Rollback: &UpgradeRollbackConfig{ - Window: defaultRollbackWindowDuration, + Window: DefaultRollbackWindowDuration, }, }, }, @@ -46,7 +46,7 @@ func TestParseUpgradeConfig(t *testing.T) { }, }, Rollback: &UpgradeRollbackConfig{ - Window: defaultRollbackWindowDuration, + Window: DefaultRollbackWindowDuration, }, }, }, @@ -66,7 +66,7 @@ func TestParseUpgradeConfig(t *testing.T) { }, }, Rollback: &UpgradeRollbackConfig{ - Window: defaultRollbackWindowDuration, + Window: DefaultRollbackWindowDuration, }, }, }, From b7928cb84137534b046585e8eeea181e4825cc8a Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Mon, 19 May 2025 13:24:13 -0700 Subject: [PATCH 02/19] Fix format specifier for duration in seconds --- internal/pkg/agent/application/upgrade/rollback_darwin.go | 2 +- internal/pkg/agent/application/upgrade/rollback_linux.go | 2 +- internal/pkg/agent/application/upgrade/rollback_windows.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback_darwin.go b/internal/pkg/agent/application/upgrade/rollback_darwin.go index d1d97957a0f..55585e98515 100644 --- a/internal/pkg/agent/application/upgrade/rollback_darwin.go +++ b/internal/pkg/agent/application/upgrade/rollback_darwin.go @@ -27,7 +27,7 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%ds", rollbackWindow.Seconds()), + "--rollback.window", fmt.Sprintf("%fs", rollbackWindow.Seconds()), ) var cred = &syscall.Credential{ diff --git a/internal/pkg/agent/application/upgrade/rollback_linux.go b/internal/pkg/agent/application/upgrade/rollback_linux.go index 0536ee6e2c2..a5fd7c639fc 100644 --- a/internal/pkg/agent/application/upgrade/rollback_linux.go +++ b/internal/pkg/agent/application/upgrade/rollback_linux.go @@ -27,7 +27,7 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%ds", rollbackWindow.Seconds()), + "--rollback.window", fmt.Sprintf("%fs", rollbackWindow.Seconds()), ) var cred = &syscall.Credential{ diff --git a/internal/pkg/agent/application/upgrade/rollback_windows.go b/internal/pkg/agent/application/upgrade/rollback_windows.go index a33d9253617..2dccff17e46 100644 --- a/internal/pkg/agent/application/upgrade/rollback_windows.go +++ b/internal/pkg/agent/application/upgrade/rollback_windows.go @@ -25,7 +25,7 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%ds", rollbackWindow.Seconds()), + "--rollback.window", fmt.Sprintf("%fs", rollbackWindow.Seconds()), ) return cmd } From b86d5c1c41da22b6723ba50bdffc9eeafc758706 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Mon, 19 May 2025 13:24:51 -0700 Subject: [PATCH 03/19] Update tests to match new signature --- .../coordinator/coordinator_unit_test.go | 2 ++ .../application/upgrade/step_download_test.go | 26 ++++++++++--------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/internal/pkg/agent/application/coordinator/coordinator_unit_test.go b/internal/pkg/agent/application/coordinator/coordinator_unit_test.go index 1a24b6d33a2..94606208d4c 100644 --- a/internal/pkg/agent/application/coordinator/coordinator_unit_test.go +++ b/internal/pkg/agent/application/coordinator/coordinator_unit_test.go @@ -35,6 +35,7 @@ import ( "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade" "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/configuration" "github.com/elastic/elastic-agent/internal/pkg/agent/transpiler" "github.com/elastic/elastic-agent/internal/pkg/config" monitoringCfg "github.com/elastic/elastic-agent/internal/pkg/core/monitoring/config" @@ -446,6 +447,7 @@ func TestCoordinatorReportsInvalidPolicy(t *testing.T) { upgradeMgr, err := upgrade.NewUpgrader( log, &artifact.Config{}, + &configuration.UpgradeConfig{}, &info.AgentInfo{}, ) require.NoError(t, err, "errored when creating a new upgrader") diff --git a/internal/pkg/agent/application/upgrade/step_download_test.go b/internal/pkg/agent/application/upgrade/step_download_test.go index f1e20427c25..2a5939ef6f1 100644 --- a/internal/pkg/agent/application/upgrade/step_download_test.go +++ b/internal/pkg/agent/application/upgrade/step_download_test.go @@ -19,6 +19,7 @@ import ( "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" + "github.com/elastic/elastic-agent/internal/pkg/agent/configuration" "github.com/elastic/elastic-agent/internal/pkg/agent/errors" "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/core/logger/loggertest" @@ -78,12 +79,13 @@ func TestDownloadWithRetries(t *testing.T) { expectedDownloadPath := "https://artifacts.elastic.co/downloads/beats/elastic-agent" testLogger, obs := loggertest.New("TestDownloadWithRetries") - settings := artifact.Config{ + downloadSettings := artifact.Config{ RetrySleepInitDuration: 20 * time.Millisecond, HTTPTransportSettings: httpcommon.HTTPTransportSettings{ Timeout: 2 * time.Second, }, } + upgradeSettings := configuration.UpgradeConfig{} // Successful immediately (no retries) t.Run("successful_immediately", func(t *testing.T) { @@ -91,16 +93,16 @@ func TestDownloadWithRetries(t *testing.T) { return &mockDownloader{expectedDownloadPath, nil}, nil } - u, err := NewUpgrader(testLogger, &settings, &info.AgentInfo{}) + u, err := NewUpgrader(testLogger, &downloadSettings, &upgradeSettings, &info.AgentInfo{}) require.NoError(t, err) parsedVersion, err := agtversion.ParseVersion("8.9.0") require.NoError(t, err) upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion) - minRetryDeadline := time.Now().Add(settings.Timeout) + minRetryDeadline := time.Now().Add(downloadSettings.Timeout) - path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &settings, upgradeDetails) + path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &downloadSettings, upgradeDetails) require.NoError(t, err) require.Equal(t, expectedDownloadPath, path) @@ -141,16 +143,16 @@ func TestDownloadWithRetries(t *testing.T) { return nil, nil } - u, err := NewUpgrader(testLogger, &settings, &info.AgentInfo{}) + u, err := NewUpgrader(testLogger, &downloadSettings, &upgradeSettings, &info.AgentInfo{}) require.NoError(t, err) parsedVersion, err := agtversion.ParseVersion("8.9.0") require.NoError(t, err) upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion) - minRetryDeadline := time.Now().Add(settings.Timeout) + minRetryDeadline := time.Now().Add(downloadSettings.Timeout) - path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &settings, upgradeDetails) + path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &downloadSettings, upgradeDetails) require.NoError(t, err) require.Equal(t, expectedDownloadPath, path) @@ -196,16 +198,16 @@ func TestDownloadWithRetries(t *testing.T) { return nil, nil } - u, err := NewUpgrader(testLogger, &settings, &info.AgentInfo{}) + u, err := NewUpgrader(testLogger, &downloadSettings, &upgradeSettings, &info.AgentInfo{}) require.NoError(t, err) parsedVersion, err := agtversion.ParseVersion("8.9.0") require.NoError(t, err) upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion) - minRetryDeadline := time.Now().Add(settings.Timeout) + minRetryDeadline := time.Now().Add(downloadSettings.Timeout) - path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &settings, upgradeDetails) + path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &downloadSettings, upgradeDetails) require.NoError(t, err) require.Equal(t, expectedDownloadPath, path) @@ -231,7 +233,7 @@ func TestDownloadWithRetries(t *testing.T) { // Download timeout expired (before all retries are exhausted) t.Run("download_timeout_expired", func(t *testing.T) { - testCaseSettings := settings + testCaseSettings := downloadSettings testCaseSettings.Timeout = 500 * time.Millisecond testCaseSettings.RetrySleepInitDuration = 10 * time.Millisecond // exponential backoff with 10ms init and 500ms timeout should fit at least 3 attempts. @@ -241,7 +243,7 @@ func TestDownloadWithRetries(t *testing.T) { return &mockDownloader{"", errors.New("download failed")}, nil } - u, err := NewUpgrader(testLogger, &settings, &info.AgentInfo{}) + u, err := NewUpgrader(testLogger, &downloadSettings, &upgradeSettings, &info.AgentInfo{}) require.NoError(t, err) parsedVersion, err := agtversion.ParseVersion("8.9.0") From 87c1473e2f85860730df6d79b135466ff05bb36c Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Mon, 19 May 2025 15:33:27 -0700 Subject: [PATCH 04/19] Fixing format specifier to omit decimals --- internal/pkg/agent/application/upgrade/rollback_darwin.go | 2 +- internal/pkg/agent/application/upgrade/rollback_linux.go | 2 +- internal/pkg/agent/application/upgrade/rollback_windows.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback_darwin.go b/internal/pkg/agent/application/upgrade/rollback_darwin.go index 55585e98515..af1051ce894 100644 --- a/internal/pkg/agent/application/upgrade/rollback_darwin.go +++ b/internal/pkg/agent/application/upgrade/rollback_darwin.go @@ -27,7 +27,7 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%fs", rollbackWindow.Seconds()), + "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), ) var cred = &syscall.Credential{ diff --git a/internal/pkg/agent/application/upgrade/rollback_linux.go b/internal/pkg/agent/application/upgrade/rollback_linux.go index a5fd7c639fc..5cddf6a2e85 100644 --- a/internal/pkg/agent/application/upgrade/rollback_linux.go +++ b/internal/pkg/agent/application/upgrade/rollback_linux.go @@ -27,7 +27,7 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%fs", rollbackWindow.Seconds()), + "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), ) var cred = &syscall.Credential{ diff --git a/internal/pkg/agent/application/upgrade/rollback_windows.go b/internal/pkg/agent/application/upgrade/rollback_windows.go index 2dccff17e46..da1e4873ab6 100644 --- a/internal/pkg/agent/application/upgrade/rollback_windows.go +++ b/internal/pkg/agent/application/upgrade/rollback_windows.go @@ -25,7 +25,7 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { cmd := exec.Command(agentExecutable, watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%fs", rollbackWindow.Seconds()), + "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), ) return cmd } From e4427ae67a3b28f288ff1f5e05da673367baec03 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Mon, 19 May 2025 15:33:41 -0700 Subject: [PATCH 05/19] Adding unit test --- .../application/upgrade/rollback_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 3f9cc0a33ab..8067d04c690 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "runtime" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -328,6 +329,24 @@ func TestRollback(t *testing.T) { } } +func TestInvokeCmd(t *testing.T) { + agentExecutable := "elastic-agent" + rollbackWindow := 2*time.Hour + 15*time.Minute + cmd := invokeCmd(agentExecutable, rollbackWindow) + + // Expected command: + // elastic-agent watch --path.config /some/path --path.home /some/path --rollback.window 8100s + require.Len(t, cmd.Args, 8) + require.Equal(t, agentExecutable, cmd.Args[0]) + require.Equal(t, "watch", cmd.Args[1]) + require.Equal(t, "--path.config", cmd.Args[2]) + require.NotEmpty(t, cmd.Args[3]) + require.Equal(t, "--path.home", cmd.Args[4]) + require.NotEmpty(t, cmd.Args[5]) + require.Equal(t, "--rollback.window", cmd.Args[6]) + require.Equal(t, "8100s", cmd.Args[7]) +} + // checkFilesAfterCleanup is a convenience function to check the file structure within topDir. // *AgentHome paths must be the expected old and new agent paths relative to topDir (typically in the form of "data/elastic-agent-*") func checkFilesAfterCleanup(t *testing.T, topDir, newAgentHome string, oldAgentHomes ...string) { From f7bf2b125248c9f40c1bac26e021c7b983016b50 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Tue, 20 May 2025 17:27:24 -0700 Subject: [PATCH 06/19] Extracting common logic to helper function --- .../pkg/agent/application/upgrade/rollback.go | 11 ++++++++++- .../agent/application/upgrade/rollback_darwin.go | 16 +++------------- .../agent/application/upgrade/rollback_linux.go | 16 +++------------- .../agent/application/upgrade/rollback_test.go | 4 ++-- .../application/upgrade/rollback_windows.go | 13 ++----------- 5 files changed, 20 insertions(+), 40 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index f91cabaa875..575813ab187 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -150,7 +150,7 @@ func InvokeWatcher(log *logger.Logger, agentExecutable string, rollbackWindow ti return nil, nil } - cmd := invokeCmd(agentExecutable, rollbackWindow) + cmd := makeOSWatchCmd(makeBaseWatchCmd(agentExecutable, rollbackWindow)) log.Infow("Starting upgrade watcher", "path", cmd.Path, "args", cmd.Args, "env", cmd.Env, "dir", cmd.Dir) if err := cmd.Start(); err != nil { return nil, fmt.Errorf("failed to start Upgrade Watcher: %w", err) @@ -238,3 +238,12 @@ func restartAgent(ctx context.Context, log *logger.Logger, c client.Client) erro close(signal) return nil } + +func makeBaseWatchCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { + // #nosec G204 -- user cannot inject any parameters to this command + return exec.Command(agentExecutable, watcherSubcommand, + "--path.config", paths.Config(), + "--path.home", paths.Top(), + "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), + ) +} diff --git a/internal/pkg/agent/application/upgrade/rollback_darwin.go b/internal/pkg/agent/application/upgrade/rollback_darwin.go index af1051ce894..5ac8069fd47 100644 --- a/internal/pkg/agent/application/upgrade/rollback_darwin.go +++ b/internal/pkg/agent/application/upgrade/rollback_darwin.go @@ -7,13 +7,10 @@ package upgrade import ( - "fmt" "os" "os/exec" "syscall" "time" - - "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" ) const ( @@ -22,14 +19,7 @@ const ( afterRestartDelay = 2 * time.Second ) -func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { - // #nosec G204 -- user cannot inject any parameters to this command - cmd := exec.Command(agentExecutable, watcherSubcommand, - "--path.config", paths.Config(), - "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), - ) - +func makeOSWatchCmd(baseWatchCmd *exec.Cmd) *exec.Cmd { var cred = &syscall.Credential{ Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), @@ -40,6 +30,6 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { Credential: cred, Setsid: true, } - cmd.SysProcAttr = sysproc - return cmd + baseWatchCmd.SysProcAttr = sysproc + return baseWatchCmd } diff --git a/internal/pkg/agent/application/upgrade/rollback_linux.go b/internal/pkg/agent/application/upgrade/rollback_linux.go index 5cddf6a2e85..ef9e46e6003 100644 --- a/internal/pkg/agent/application/upgrade/rollback_linux.go +++ b/internal/pkg/agent/application/upgrade/rollback_linux.go @@ -7,13 +7,10 @@ package upgrade import ( - "fmt" "os" "os/exec" "syscall" "time" - - "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" ) const ( @@ -22,14 +19,7 @@ const ( afterRestartDelay = 2 * time.Second ) -func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { - // #nosec G204 -- user cannot inject any parameters to this command - cmd := exec.Command(agentExecutable, watcherSubcommand, - "--path.config", paths.Config(), - "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), - ) - +func makeOSWatchCmd(baseWatchCmd *exec.Cmd) *exec.Cmd { var cred = &syscall.Credential{ Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), @@ -42,6 +32,6 @@ func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { // propagate sigint instead of sigkill so we can ignore it Pdeathsig: syscall.SIGINT, } - cmd.SysProcAttr = sysproc - return cmd + baseWatchCmd.SysProcAttr = sysproc + return baseWatchCmd } diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 8067d04c690..2d9762cd003 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -329,10 +329,10 @@ func TestRollback(t *testing.T) { } } -func TestInvokeCmd(t *testing.T) { +func TestMakeBaseWatchCmd(t *testing.T) { agentExecutable := "elastic-agent" rollbackWindow := 2*time.Hour + 15*time.Minute - cmd := invokeCmd(agentExecutable, rollbackWindow) + cmd := makeBaseWatchCmd(agentExecutable, rollbackWindow) // Expected command: // elastic-agent watch --path.config /some/path --path.home /some/path --rollback.window 8100s diff --git a/internal/pkg/agent/application/upgrade/rollback_windows.go b/internal/pkg/agent/application/upgrade/rollback_windows.go index da1e4873ab6..e9d3ffc281a 100644 --- a/internal/pkg/agent/application/upgrade/rollback_windows.go +++ b/internal/pkg/agent/application/upgrade/rollback_windows.go @@ -7,11 +7,8 @@ package upgrade import ( - "fmt" "os/exec" "time" - - "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" ) const ( @@ -20,12 +17,6 @@ const ( afterRestartDelay = 20 * time.Second ) -func invokeCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { - // #nosec G204 -- user cannot inject any parameters to this command - cmd := exec.Command(agentExecutable, watcherSubcommand, - "--path.config", paths.Config(), - "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), - ) - return cmd +func makeOSWatchCmd(baseWatchCmd *exec.Cmd) *exec.Cmd { + return baseWatchCmd } From 687fd4c65e9c13ce867958c28ab16ad5552a6920 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Tue, 20 May 2025 22:37:43 -0700 Subject: [PATCH 07/19] Only pass --rollback-window arg to watcher CLI if watcher version supports it --- .../pkg/agent/application/upgrade/rollback.go | 22 ++++++++-- .../application/upgrade/rollback_test.go | 44 +++++++++++++------ .../pkg/agent/application/upgrade/upgrade.go | 7 ++- 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 575813ab187..eaeead68743 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -24,6 +24,7 @@ import ( "github.com/elastic/elastic-agent/pkg/control/v2/client" "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/utils" + "github.com/elastic/elastic-agent/pkg/version" ) const ( @@ -33,6 +34,9 @@ const ( restartBackoffMax = 90 * time.Second ) +// Rollback window feature is only available starting with version >= 9.1.0-SNAPSHOT. +var rollbackWindowMinVersion = version.NewParsedSemVer(9, 1, 0, "SNAPSHOT", "") + // Rollback rollbacks to previous version which was functioning before upgrade. func Rollback(ctx context.Context, log *logger.Logger, c client.Client, topDirPath, prevVersionedHome, prevHash string) error { symlinkPath := filepath.Join(topDirPath, agentName) @@ -240,10 +244,20 @@ func restartAgent(ctx context.Context, log *logger.Logger, c client.Client) erro } func makeBaseWatchCmd(agentExecutable string, rollbackWindow time.Duration) *exec.Cmd { - // #nosec G204 -- user cannot inject any parameters to this command - return exec.Command(agentExecutable, watcherSubcommand, + cmdArgs := []string{ + watcherSubcommand, "--path.config", paths.Config(), "--path.home", paths.Top(), - "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds()), - ) + } + + if rollbackWindow > 0 { + cmdArgs = append(cmdArgs, "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds())) + } + + // #nosec G204 -- user cannot inject any parameters to this command + return exec.Command(agentExecutable, cmdArgs...) +} + +func isRollbackWindowFeatureAvailable(watcherVersion *version.ParsedSemVer) bool { + return !watcherVersion.Less(*rollbackWindowMinVersion) } diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 2d9762cd003..862317569f8 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -331,20 +331,36 @@ func TestRollback(t *testing.T) { func TestMakeBaseWatchCmd(t *testing.T) { agentExecutable := "elastic-agent" - rollbackWindow := 2*time.Hour + 15*time.Minute - cmd := makeBaseWatchCmd(agentExecutable, rollbackWindow) - - // Expected command: - // elastic-agent watch --path.config /some/path --path.home /some/path --rollback.window 8100s - require.Len(t, cmd.Args, 8) - require.Equal(t, agentExecutable, cmd.Args[0]) - require.Equal(t, "watch", cmd.Args[1]) - require.Equal(t, "--path.config", cmd.Args[2]) - require.NotEmpty(t, cmd.Args[3]) - require.Equal(t, "--path.home", cmd.Args[4]) - require.NotEmpty(t, cmd.Args[5]) - require.Equal(t, "--rollback.window", cmd.Args[6]) - require.Equal(t, "8100s", cmd.Args[7]) + t.Run("no_rollback_window", func(t *testing.T) { + cmd := makeBaseWatchCmd(agentExecutable, 0) + + // Expected command: + // elastic-agent watch --path.config /some/path --path.home /some/path + require.Len(t, cmd.Args, 6) + require.Equal(t, agentExecutable, cmd.Args[0]) + require.Equal(t, "watch", cmd.Args[1]) + require.Equal(t, "--path.config", cmd.Args[2]) + require.NotEmpty(t, cmd.Args[3]) + require.Equal(t, "--path.home", cmd.Args[4]) + require.NotEmpty(t, cmd.Args[5]) + }) + + t.Run("with_rollback_window", func(t *testing.T) { + cmd := makeBaseWatchCmd(agentExecutable, 2*time.Hour+15*time.Minute) + + // Expected command: + // elastic-agent watch --path.config /some/path --path.home /some/path --rollback.window 8100s + require.Len(t, cmd.Args, 8) + require.Equal(t, agentExecutable, cmd.Args[0]) + require.Equal(t, "watch", cmd.Args[1]) + require.Equal(t, "--path.config", cmd.Args[2]) + require.NotEmpty(t, cmd.Args[3]) + require.Equal(t, "--path.home", cmd.Args[4]) + require.NotEmpty(t, cmd.Args[5]) + require.Equal(t, "--rollback.window", cmd.Args[6]) + require.Equal(t, "8100s", cmd.Args[7]) + + }) } // checkFilesAfterCleanup is a convenience function to check the file structure within topDir. diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 9ee347df534..7d5beafa83a 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -353,8 +353,13 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string watcherExecutable := selectWatcherExecutable(paths.Top(), previous, current) + rollbackWindow := u.upgradeSettings.Rollback.Window + if !isRollbackWindowFeatureAvailable(parsedVersion) { + rollbackWindow = 0 + } + var watcherCmd *exec.Cmd - if watcherCmd, err = InvokeWatcher(u.log, watcherExecutable, u.upgradeSettings.Rollback.Window); err != nil { + if watcherCmd, err = InvokeWatcher(u.log, watcherExecutable, rollbackWindow); err != nil { u.log.Errorw("Rolling back: starting watcher failed", "error.message", err) rollbackErr := rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) return nil, goerrors.Join(err, rollbackErr) From 0329197e417b04b7639344a678a9daecb18bbd10 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Wed, 21 May 2025 03:08:20 -0700 Subject: [PATCH 08/19] Fix flag name --- internal/pkg/agent/application/upgrade/rollback.go | 2 +- internal/pkg/agent/application/upgrade/rollback_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index eaeead68743..8afb87e8420 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -251,7 +251,7 @@ func makeBaseWatchCmd(agentExecutable string, rollbackWindow time.Duration) *exe } if rollbackWindow > 0 { - cmdArgs = append(cmdArgs, "--rollback.window", fmt.Sprintf("%.fs", rollbackWindow.Seconds())) + cmdArgs = append(cmdArgs, "--rollback-window", fmt.Sprintf("%.fs", rollbackWindow.Seconds())) } // #nosec G204 -- user cannot inject any parameters to this command diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 862317569f8..4a4552ce000 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -349,7 +349,7 @@ func TestMakeBaseWatchCmd(t *testing.T) { cmd := makeBaseWatchCmd(agentExecutable, 2*time.Hour+15*time.Minute) // Expected command: - // elastic-agent watch --path.config /some/path --path.home /some/path --rollback.window 8100s + // elastic-agent watch --path.config /some/path --path.home /some/path --rollback-window 8100s require.Len(t, cmd.Args, 8) require.Equal(t, agentExecutable, cmd.Args[0]) require.Equal(t, "watch", cmd.Args[1]) @@ -357,7 +357,7 @@ func TestMakeBaseWatchCmd(t *testing.T) { require.NotEmpty(t, cmd.Args[3]) require.Equal(t, "--path.home", cmd.Args[4]) require.NotEmpty(t, cmd.Args[5]) - require.Equal(t, "--rollback.window", cmd.Args[6]) + require.Equal(t, "--rollback-window", cmd.Args[6]) require.Equal(t, "8100s", cmd.Args[7]) }) From 43f23ad210125760e75edfb8163f0645c9804ecc Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Fri, 6 Jun 2025 16:35:09 -0700 Subject: [PATCH 09/19] Rename isRollbackWindowFeatureAvailable -> isRollbackWindowSupported --- internal/pkg/agent/application/upgrade/rollback.go | 2 +- internal/pkg/agent/application/upgrade/upgrade.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 8afb87e8420..22dd38075c7 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -258,6 +258,6 @@ func makeBaseWatchCmd(agentExecutable string, rollbackWindow time.Duration) *exe return exec.Command(agentExecutable, cmdArgs...) } -func isRollbackWindowFeatureAvailable(watcherVersion *version.ParsedSemVer) bool { +func isRollbackWindowSupported(watcherVersion *version.ParsedSemVer) bool { return !watcherVersion.Less(*rollbackWindowMinVersion) } diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 7d5beafa83a..781ed167b9c 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -354,7 +354,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string watcherExecutable := selectWatcherExecutable(paths.Top(), previous, current) rollbackWindow := u.upgradeSettings.Rollback.Window - if !isRollbackWindowFeatureAvailable(parsedVersion) { + if !isRollbackWindowSupported(parsedVersion) { rollbackWindow = 0 } From a5e9bcf4f76bbaa341863e909bea63345b7ceebb Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Sat, 7 Jun 2025 09:58:30 -0700 Subject: [PATCH 10/19] Use duration.String() to print out rollback window --- internal/pkg/agent/application/upgrade/rollback.go | 2 +- internal/pkg/agent/application/upgrade/rollback_test.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 22dd38075c7..3a80574bc4e 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -251,7 +251,7 @@ func makeBaseWatchCmd(agentExecutable string, rollbackWindow time.Duration) *exe } if rollbackWindow > 0 { - cmdArgs = append(cmdArgs, "--rollback-window", fmt.Sprintf("%.fs", rollbackWindow.Seconds())) + cmdArgs = append(cmdArgs, "--rollback-window", fmt.Sprintf("%s", rollbackWindow.String())) } // #nosec G204 -- user cannot inject any parameters to this command diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 4a4552ce000..db1114c84f0 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -346,7 +346,8 @@ func TestMakeBaseWatchCmd(t *testing.T) { }) t.Run("with_rollback_window", func(t *testing.T) { - cmd := makeBaseWatchCmd(agentExecutable, 2*time.Hour+15*time.Minute) + rollbackWindow := 2*time.Hour + 15*time.Minute + cmd := makeBaseWatchCmd(agentExecutable, rollbackWindow) // Expected command: // elastic-agent watch --path.config /some/path --path.home /some/path --rollback-window 8100s @@ -358,7 +359,7 @@ func TestMakeBaseWatchCmd(t *testing.T) { require.Equal(t, "--path.home", cmd.Args[4]) require.NotEmpty(t, cmd.Args[5]) require.Equal(t, "--rollback-window", cmd.Args[6]) - require.Equal(t, "8100s", cmd.Args[7]) + require.Equal(t, rollbackWindow.String(), cmd.Args[7]) }) } From e5d1c0b1f3ad53a9204dfad06838bd7cfb6c2b93 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Sat, 7 Jun 2025 09:58:50 -0700 Subject: [PATCH 11/19] Add godoc for selectWatcherExecutable function --- internal/pkg/agent/application/upgrade/upgrade.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 781ed167b9c..72656c495e4 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -384,6 +384,8 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string return cb, nil } +// selectWatcherExecutable returns the path to the watcher executable for the Agent that has the newer +// of the two Agent versions involved in the upgrade. 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) { From 0969f6f56a24d590211247d8a85f0b30cbe8eb9b Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Sat, 7 Jun 2025 10:09:47 -0700 Subject: [PATCH 12/19] Simplify assertions --- .../application/upgrade/rollback_test.go | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index db1114c84f0..c896ea3b774 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -330,19 +330,23 @@ func TestRollback(t *testing.T) { } func TestMakeBaseWatchCmd(t *testing.T) { + exec, _ := os.Executable() + execDir := filepath.Dir(exec) + agentExecutable := "elastic-agent" t.Run("no_rollback_window", func(t *testing.T) { cmd := makeBaseWatchCmd(agentExecutable, 0) // Expected command: // elastic-agent watch --path.config /some/path --path.home /some/path - require.Len(t, cmd.Args, 6) - require.Equal(t, agentExecutable, cmd.Args[0]) - require.Equal(t, "watch", cmd.Args[1]) - require.Equal(t, "--path.config", cmd.Args[2]) - require.NotEmpty(t, cmd.Args[3]) - require.Equal(t, "--path.home", cmd.Args[4]) - require.NotEmpty(t, cmd.Args[5]) + require.Equal(t, cmd.Args, []string{ + agentExecutable, + "watch", + "--path.config", + execDir, + "--path.home", + execDir, + }) }) t.Run("with_rollback_window", func(t *testing.T) { @@ -351,16 +355,16 @@ func TestMakeBaseWatchCmd(t *testing.T) { // Expected command: // elastic-agent watch --path.config /some/path --path.home /some/path --rollback-window 8100s - require.Len(t, cmd.Args, 8) - require.Equal(t, agentExecutable, cmd.Args[0]) - require.Equal(t, "watch", cmd.Args[1]) - require.Equal(t, "--path.config", cmd.Args[2]) - require.NotEmpty(t, cmd.Args[3]) - require.Equal(t, "--path.home", cmd.Args[4]) - require.NotEmpty(t, cmd.Args[5]) - require.Equal(t, "--rollback-window", cmd.Args[6]) - require.Equal(t, rollbackWindow.String(), cmd.Args[7]) - + require.Equal(t, cmd.Args, []string{ + agentExecutable, + "watch", + "--path.config", + execDir, + "--path.home", + execDir, + "--rollback-window", + rollbackWindow.String(), + }) }) } From d3ebd314cc893a77615f02c15ccba704ef165df5 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Sat, 7 Jun 2025 10:15:03 -0700 Subject: [PATCH 13/19] Clarify TODO comment --- internal/pkg/agent/cmd/watch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/agent/cmd/watch.go b/internal/pkg/agent/cmd/watch.go index 8e93b46448f..7a073b9b5e5 100644 --- a/internal/pkg/agent/cmd/watch.go +++ b/internal/pkg/agent/cmd/watch.go @@ -109,7 +109,7 @@ func watchCmd(cmd *cobra.Command, log *logp.Logger, topDir string, cfg *configur if err != nil { return fmt.Errorf("failed to retrieve %s flag value while watching the agent upgrade: %w", flagRollbackWindow, err) } - _ = rollbackWindow // TODO: use rollbackWindow when https://github.com/elastic/elastic-agent/issues/6884 is implemented + _ = rollbackWindow // TODO: use rollbackWindow when implementing https://github.com/elastic/elastic-agent/issues/6884 isWithinGrace, tilGrace := gracePeriod(marker, cfg.GracePeriod) if isTerminalState(marker) || !isWithinGrace { From c0307c710ba19518eac007ee7ed11b28ae2820b8 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Sat, 7 Jun 2025 10:58:06 -0700 Subject: [PATCH 14/19] Evaluate symlinks in path --- internal/pkg/agent/application/upgrade/rollback_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index c896ea3b774..81a024e362b 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -330,7 +330,10 @@ func TestRollback(t *testing.T) { } func TestMakeBaseWatchCmd(t *testing.T) { - exec, _ := os.Executable() + exec, err := os.Executable() + require.NoError(t, err) + exec, err = filepath.EvalSymlinks(exec) + require.NoError(t, err) execDir := filepath.Dir(exec) agentExecutable := "elastic-agent" From 6f4a36c6a74fcd50ae266c0ad64a6275ffd502a2 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Tue, 10 Jun 2025 16:46:57 -0700 Subject: [PATCH 15/19] Add comments explaining why rollbackWindow is set to 0 --- internal/pkg/agent/application/upgrade/rollback.go | 6 ++++-- internal/pkg/agent/application/upgrade/upgrade.go | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 3a80574bc4e..1fe62f7fefd 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -258,6 +258,8 @@ func makeBaseWatchCmd(agentExecutable string, rollbackWindow time.Duration) *exe return exec.Command(agentExecutable, cmdArgs...) } -func isRollbackWindowSupported(watcherVersion *version.ParsedSemVer) bool { - return !watcherVersion.Less(*rollbackWindowMinVersion) +// isRollbackWindowSupported checks if the rollback window feature is supported by the version of +// the target Agent, i.e the version being upgraded *to*. +func isRollbackWindowSupported(targetAgentVersion *version.ParsedSemVer) bool { + return !targetAgentVersion.Less(*rollbackWindowMinVersion) } diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 72656c495e4..3c7fe7edabd 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -353,6 +353,11 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string watcherExecutable := selectWatcherExecutable(paths.Top(), previous, current) + // Check if the target Agent version supports the rollback window feature. If it doesn't, + // it won't know how to perform a deferred cleanup after a successful upgrade, so we need + // to rely on the Upgrade Watcher to do the cleanup immediately after it has deemed the upgrade + // to be successful. We ask the Upgrade Watcher to do this immediate cleanup by passing + // a rollback window of 0 seconds to the Upgrade Watcher. rollbackWindow := u.upgradeSettings.Rollback.Window if !isRollbackWindowSupported(parsedVersion) { rollbackWindow = 0 From cb32f6d2602bdd785805423eec1ce9874c65475b Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Tue, 10 Jun 2025 16:51:29 -0700 Subject: [PATCH 16/19] Add test for isRollbackWindowSupported --- .../application/upgrade/rollback_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 81a024e362b..bdb75628b7b 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -20,6 +20,7 @@ import ( "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/core/logger/loggertest" + "github.com/elastic/elastic-agent/pkg/version" mocks "github.com/elastic/elastic-agent/testing/mocks/pkg/control/v2/client" ) @@ -329,6 +330,36 @@ func TestRollback(t *testing.T) { } } +func TestIsRollbackWindowSupported(t *testing.T) { + tests := map[string]struct { + version string + want bool + }{ + "supported_version": { + version: "9.2.0-SNAPSHOT", + want: true, + }, + "older_version": { + version: "9.0.0-SNAPSHOT", + want: false, + }, + "exactly_minimum_version": { + version: "9.1.0", + want: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + v, err := version.ParseVersion(tt.version) + require.NoError(t, err) + + got := isRollbackWindowSupported(v) + assert.Equal(t, tt.want, got, "isRollbackWindowSupported(%q)", tt.version) + }) + } +} + func TestMakeBaseWatchCmd(t *testing.T) { exec, err := os.Executable() require.NoError(t, err) From 07117639de6b1848a0e2410aa6d5234d7707bbf3 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Tue, 10 Jun 2025 17:00:21 -0700 Subject: [PATCH 17/19] Add upgrade settings to Reload --- internal/pkg/agent/application/upgrade/upgrade.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 3c7fe7edabd..197239b8554 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -109,7 +109,7 @@ func (u *Upgrader) SetClient(c fleetclient.Sender) { u.log.Debugf("Set client changed URI to %s", u.fleetServerURI) } -// Reload reloads the artifact configuration for the upgrader. +// Reload reloads the artifact download and upgrade configurations for the upgrader. // As of today, December 2023, fleet-server does not send most of the configuration // defined in artifact.Config, what will likely change in the near future. func (u *Upgrader) Reload(rawConfig *config.Config) error { @@ -146,6 +146,7 @@ func (u *Upgrader) Reload(rawConfig *config.Config) error { } u.downloadSettings = cfg.Settings.DownloadConfig + u.upgradeSettings = cfg.Settings.Upgrade return nil } From 596f79c6248e9b2f7a49bc18c26cb7fea8d872e3 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Fri, 27 Jun 2025 16:51:59 -0700 Subject: [PATCH 18/19] Removing extra line introduced while resolving conflicts --- internal/pkg/agent/cmd/watch.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/pkg/agent/cmd/watch.go b/internal/pkg/agent/cmd/watch.go index 7a073b9b5e5..f31736d4695 100644 --- a/internal/pkg/agent/cmd/watch.go +++ b/internal/pkg/agent/cmd/watch.go @@ -118,7 +118,6 @@ func watchCmd(cmd *cobra.Command, log *logp.Logger, topDir string, cfg *configur stateString = string(marker.Details.State) } log.Infof("not within grace [updatedOn %v] %v or agent have been rolled back [state: %s]", marker.UpdatedOn.String(), time.Since(marker.UpdatedOn).String(), stateString) - // if it is started outside of upgrade loop // if we're not within grace and marker is still there it might mean // that cleanup was not performed ok, cleanup everything except current version From de48e61fce978b2db423f69c4babcc48d5416668 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Fri, 27 Jun 2025 17:34:03 -0700 Subject: [PATCH 19/19] Update unit test --- internal/pkg/agent/cmd/watch_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/pkg/agent/cmd/watch_test.go b/internal/pkg/agent/cmd/watch_test.go index 9451c476543..3cf36f03d47 100644 --- a/internal/pkg/agent/cmd/watch_test.go +++ b/internal/pkg/agent/cmd/watch_test.go @@ -275,8 +275,9 @@ func Test_watchCmd(t *testing.T) { tmpDir := t.TempDir() mockWatcher := cmdmocks.NewAgentWatcher(t) mockInstallModifier := cmdmocks.NewInstallationModifier(t) + cmd := newWatchCommandWithArgs(nil, nil) tt.setupUpgradeMarker(t, tmpDir, mockWatcher, mockInstallModifier) - tt.wantErr(t, watchCmd(log, tmpDir, tt.args.cfg, mockWatcher, mockInstallModifier), fmt.Sprintf("watchCmd(%v, ...)", tt.args.cfg)) + tt.wantErr(t, watchCmd(cmd, log, tmpDir, tt.args.cfg, mockWatcher, mockInstallModifier), fmt.Sprintf("watchCmd(%v, ...)", tt.args.cfg)) t.Logf("watchCmd logs:\n%v", obs.All()) }) }