From c68a0ed724c2ac01a9ab30f792143c6e2ef49224 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 14 Aug 2025 15:27:00 +0300 Subject: [PATCH 01/10] enhancement(5235): added markUpgradeFunc to abstract markUpgrade --- 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 73b76d63e3d..8e7330f40bb 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -229,6 +229,8 @@ func checkUpgrade(log *logger.Logger, currentVersion, newVersion agentVersion, m return nil } +var markUpgradeFunc = markUpgrade + // Upgrade upgrades running agent, function returns shutdown callback that must be called by reexec. 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) From 33bc78fdeb88d8f8638aba1323bb3adea9f44f60 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 14 Aug 2025 15:27:18 +0300 Subject: [PATCH 02/10] enhancement(5235): using markUpgradeFunc in Upgrade function --- 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 8e7330f40bb..150ccfd2058 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -387,7 +387,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string versionedHome: currentVersionedHome, } - if err := markUpgrade(u.log, + if err := markUpgradeFunc(u.log, paths.Data(), // data dir to place the marker in current, // new agent version data previous, // old agent version data From 285eabb726cf75c4fad10317fdb045de300a82e8 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 14 Aug 2025 15:27:57 +0300 Subject: [PATCH 03/10] enhancement(5235): wrapping writeFile errors in markUpgrade enhancement(5235): added goerrors in step_mark --- internal/pkg/agent/application/upgrade/step_mark.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/step_mark.go b/internal/pkg/agent/application/upgrade/step_mark.go index 65b4e878a40..e2560a292a9 100644 --- a/internal/pkg/agent/application/upgrade/step_mark.go +++ b/internal/pkg/agent/application/upgrade/step_mark.go @@ -6,6 +6,7 @@ package upgrade import ( "encoding/json" + goerrors "errors" "fmt" "os" "path/filepath" @@ -224,7 +225,7 @@ func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent ag markerPath := markerFilePath(dataDirPath) log.Infow("Writing upgrade marker file", "file.path", markerPath, "hash", marker.Hash, "prev_hash", marker.PrevHash) if err := os.WriteFile(markerPath, markerBytes, 0600); err != nil { - return errors.New(err, errors.TypeFilesystem, "failed to create update marker file", errors.M(errors.MetaKeyPath, markerPath)) + return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to create update marker file", errors.M(errors.MetaKeyPath, markerPath))) } if err := UpdateActiveCommit(log, paths.Top(), agent.hash); err != nil { @@ -239,7 +240,7 @@ func UpdateActiveCommit(log *logger.Logger, topDirPath, hash string) error { activeCommitPath := filepath.Join(topDirPath, agentCommitFile) log.Infow("Updating active commit", "file.path", activeCommitPath, "hash", hash) if err := os.WriteFile(activeCommitPath, []byte(hash), 0600); err != nil { - return errors.New(err, errors.TypeFilesystem, "failed to update active commit", errors.M(errors.MetaKeyPath, activeCommitPath)) + return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to update active commit", errors.M(errors.MetaKeyPath, activeCommitPath))) } return nil From 6f9b9499ddda77c8c8e7fea2537ed65e6943723b Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 14 Aug 2025 19:40:12 +0300 Subject: [PATCH 04/10] enhancement(5235): using writeFile wrapper in markUpgrade --- internal/pkg/agent/application/upgrade/step_mark.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/step_mark.go b/internal/pkg/agent/application/upgrade/step_mark.go index e2560a292a9..cb4383cca4a 100644 --- a/internal/pkg/agent/application/upgrade/step_mark.go +++ b/internal/pkg/agent/application/upgrade/step_mark.go @@ -15,6 +15,7 @@ import ( "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" + "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/common" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" "github.com/elastic/elastic-agent/internal/pkg/agent/errors" "github.com/elastic/elastic-agent/internal/pkg/fleetapi" @@ -224,7 +225,7 @@ func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent ag markerPath := markerFilePath(dataDirPath) log.Infow("Writing upgrade marker file", "file.path", markerPath, "hash", marker.Hash, "prev_hash", marker.PrevHash) - if err := os.WriteFile(markerPath, markerBytes, 0600); err != nil { + if err := common.WriteFile(markerPath, markerBytes, 0600); err != nil { return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to create update marker file", errors.M(errors.MetaKeyPath, markerPath))) } @@ -239,7 +240,7 @@ func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent ag func UpdateActiveCommit(log *logger.Logger, topDirPath, hash string) error { activeCommitPath := filepath.Join(topDirPath, agentCommitFile) log.Infow("Updating active commit", "file.path", activeCommitPath, "hash", hash) - if err := os.WriteFile(activeCommitPath, []byte(hash), 0600); err != nil { + if err := common.WriteFile(activeCommitPath, []byte(hash), 0600); err != nil { return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to update active commit", errors.M(errors.MetaKeyPath, activeCommitPath))) } From 99d09a571d3067fa65b445e45046896f70fb2160 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 14 Aug 2025 15:28:50 +0300 Subject: [PATCH 05/10] enhancement(5235): added tests mark upgrade tests enhancement(5235): updated step mark test to mock writeFile --- .../application/upgrade/step_mark_test.go | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/internal/pkg/agent/application/upgrade/step_mark_test.go b/internal/pkg/agent/application/upgrade/step_mark_test.go index fc1731e7b24..082b80eca85 100644 --- a/internal/pkg/agent/application/upgrade/step_mark_test.go +++ b/internal/pkg/agent/application/upgrade/step_mark_test.go @@ -12,8 +12,11 @@ import ( "github.com/stretchr/testify/require" + "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" + "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/common" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" "github.com/elastic/elastic-agent/internal/pkg/fleetapi" + "github.com/elastic/elastic-agent/pkg/core/logger/loggertest" ) func TestSaveAndLoadMarker_NoLoss(t *testing.T) { @@ -260,3 +263,68 @@ desired_outcome: true }) } } + +// Test asserting that errors from os.WriteFile are wrapped and returned +func TestMarkUpgradeWriteFileError(t *testing.T) { + log, _ := loggertest.New("test") + agent := agentInstall{ + version: "8.5.0", + hash: "abc123", + versionedHome: "home/v8.5.0", + } + previousAgent := agentInstall{ + version: "8.4.0", + hash: "xyz789", + versionedHome: "home/v8.4.0", + } + action := &fleetapi.ActionUpgrade{ + ActionID: "action-123", + ActionType: "UPGRADE", + Data: fleetapi.ActionUpgradeData{ + Version: "8.5.0", + SourceURI: "https://example.com/upgrade", + }, + } + upgradeDetails := details.NewDetails("8.5.0", details.StateScheduled, "action-123") + desiredOutcome := OUTCOME_UPGRADE + + type testCase struct { + fileName string + expectedError error + } + + testCases := map[string]testCase{ + "should return error if it fails writing to top dir": { + fileName: "commit", + expectedError: os.ErrPermission, + }, + "should return error if it fails writing to data dir": { + fileName: "marker", + expectedError: os.ErrPermission, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + baseDir := t.TempDir() + paths.SetTop(baseDir) + + markerPath := markerFilePath(paths.Data()) + + setStdlibMock := common.PrepareStdLibMocks(common.StdLibMocks{ + WriteFileMock: func(name string, data []byte, perm os.FileMode) error { + if tc.fileName == "marker" && name == markerPath { + return tc.expectedError + } + return tc.expectedError + }, + }) + + setStdlibMock(t, common.WriteFileFuncName) + + err := markUpgrade(log, paths.Data(), agent, previousAgent, action, upgradeDetails, desiredOutcome) + require.Error(t, err) + require.ErrorIs(t, err, os.ErrPermission) + }) + } +} From d3c9ce540cfd5d2ab64687131687d87fb12dbcc5 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 4 Sep 2025 14:19:58 +0300 Subject: [PATCH 06/10] enhancement(5235): updated markUpgrade function so that it can be tested, jecting dependencies. updated relevant tests --- .../agent/application/upgrade/step_mark.go | 75 ++++++++++--------- .../application/upgrade/step_mark_test.go | 73 ++++++++++++------ 2 files changed, 90 insertions(+), 58 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/step_mark.go b/internal/pkg/agent/application/upgrade/step_mark.go index cb4383cca4a..f61c7705dce 100644 --- a/internal/pkg/agent/application/upgrade/step_mark.go +++ b/internal/pkg/agent/application/upgrade/step_mark.go @@ -15,7 +15,6 @@ import ( "gopkg.in/yaml.v3" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" - "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/common" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" "github.com/elastic/elastic-agent/internal/pkg/agent/errors" "github.com/elastic/elastic-agent/internal/pkg/fleetapi" @@ -198,49 +197,53 @@ type agentInstall struct { versionedHome string } -// markUpgrade marks update happened so we can handle grace period -func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details, desiredOutcome UpgradeOutcome) error { - - if len(previousAgent.hash) > hashLen { - previousAgent.hash = previousAgent.hash[:hashLen] - } - - marker := &UpdateMarker{ - Version: agent.version, - Hash: agent.hash, - VersionedHome: agent.versionedHome, - UpdatedOn: time.Now(), - PrevVersion: previousAgent.version, - PrevHash: previousAgent.hash, - PrevVersionedHome: previousAgent.versionedHome, - Action: action, - Details: upgradeDetails, - DesiredOutcome: desiredOutcome, - } - - markerBytes, err := yaml.Marshal(newMarkerSerializer(marker)) - if err != nil { - return errors.New(err, errors.TypeConfig, "failed to parse marker file") - } +type updateActiveCommitFunc func(log *logger.Logger, topDirPath, hash string, writeFile writeFileFunc) error - markerPath := markerFilePath(dataDirPath) - log.Infow("Writing upgrade marker file", "file.path", markerPath, "hash", marker.Hash, "prev_hash", marker.PrevHash) - if err := common.WriteFile(markerPath, markerBytes, 0600); err != nil { - return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to create update marker file", errors.M(errors.MetaKeyPath, markerPath))) - } +// markUpgrade marks update happened so we can handle grace period +func markUpgradeProvider(updateActiveCommit updateActiveCommitFunc, writeFile writeFileFunc) markUpgradeFunc { + return func(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details, desiredOutcome UpgradeOutcome) error { + + if len(previousAgent.hash) > hashLen { + previousAgent.hash = previousAgent.hash[:hashLen] + } + + marker := &UpdateMarker{ + Version: agent.version, + Hash: agent.hash, + VersionedHome: agent.versionedHome, + UpdatedOn: time.Now(), + PrevVersion: previousAgent.version, + PrevHash: previousAgent.hash, + PrevVersionedHome: previousAgent.versionedHome, + Action: action, + Details: upgradeDetails, + DesiredOutcome: desiredOutcome, + } + + markerBytes, err := yaml.Marshal(newMarkerSerializer(marker)) + if err != nil { + return errors.New(err, errors.TypeConfig, "failed to parse marker file") + } + + markerPath := markerFilePath(dataDirPath) + log.Infow("Writing upgrade marker file", "file.path", markerPath, "hash", marker.Hash, "prev_hash", marker.PrevHash) + if err := writeFile(markerPath, markerBytes, 0600); err != nil { + return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to create update marker file", errors.M(errors.MetaKeyPath, markerPath))) + } + + if err := updateActiveCommit(log, paths.Top(), agent.hash, writeFile); err != nil { + return err + } - if err := UpdateActiveCommit(log, paths.Top(), agent.hash); err != nil { - return err + return nil } - - return nil } // UpdateActiveCommit updates active.commit file to point to active version. -func UpdateActiveCommit(log *logger.Logger, topDirPath, hash string) error { +func UpdateActiveCommit(log *logger.Logger, topDirPath, hash string, writeFile writeFileFunc) error { activeCommitPath := filepath.Join(topDirPath, agentCommitFile) log.Infow("Updating active commit", "file.path", activeCommitPath, "hash", hash) - if err := common.WriteFile(activeCommitPath, []byte(hash), 0600); err != nil { + if err := writeFile(activeCommitPath, []byte(hash), 0600); err != nil { return goerrors.Join(err, errors.New(errors.TypeFilesystem, "failed to update active commit", errors.M(errors.MetaKeyPath, activeCommitPath))) } diff --git a/internal/pkg/agent/application/upgrade/step_mark_test.go b/internal/pkg/agent/application/upgrade/step_mark_test.go index 082b80eca85..34ac01e4888 100644 --- a/internal/pkg/agent/application/upgrade/step_mark_test.go +++ b/internal/pkg/agent/application/upgrade/step_mark_test.go @@ -5,6 +5,7 @@ package upgrade import ( + "errors" "os" "path/filepath" "testing" @@ -13,9 +14,9 @@ import ( "github.com/stretchr/testify/require" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" - "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/common" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" "github.com/elastic/elastic-agent/internal/pkg/fleetapi" + "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/core/logger/loggertest" ) @@ -264,8 +265,7 @@ desired_outcome: true } } -// Test asserting that errors from os.WriteFile are wrapped and returned -func TestMarkUpgradeWriteFileError(t *testing.T) { +func TestMarkUpgrade(t *testing.T) { log, _ := loggertest.New("test") agent := agentInstall{ version: "8.5.0", @@ -288,19 +288,32 @@ func TestMarkUpgradeWriteFileError(t *testing.T) { upgradeDetails := details.NewDetails("8.5.0", details.StateScheduled, "action-123") desiredOutcome := OUTCOME_UPGRADE + testError := errors.New("test error") + type testCase struct { fileName string expectedError error + markUpgrade markUpgradeFunc } testCases := map[string]testCase{ - "should return error if it fails writing to top dir": { + "should return error if it fails updating the active commit file": { fileName: "commit", - expectedError: os.ErrPermission, + expectedError: testError, + markUpgrade: markUpgradeProvider(func(log *logger.Logger, topDirPath, hash string, writeFile writeFileFunc) error { + return testError + }, func(name string, data []byte, perm os.FileMode) error { + return nil + }), }, - "should return error if it fails writing to data dir": { + "should return error if it fails writing to marker file": { fileName: "marker", - expectedError: os.ErrPermission, + expectedError: testError, + markUpgrade: markUpgradeProvider(func(log *logger.Logger, topDirPath, hash string, writeFile writeFileFunc) error { + return nil + }, func(name string, data []byte, perm os.FileMode) error { + return testError + }), }, } @@ -309,22 +322,38 @@ func TestMarkUpgradeWriteFileError(t *testing.T) { baseDir := t.TempDir() paths.SetTop(baseDir) - markerPath := markerFilePath(paths.Data()) - - setStdlibMock := common.PrepareStdLibMocks(common.StdLibMocks{ - WriteFileMock: func(name string, data []byte, perm os.FileMode) error { - if tc.fileName == "marker" && name == markerPath { - return tc.expectedError - } - return tc.expectedError - }, - }) - - setStdlibMock(t, common.WriteFileFuncName) - - err := markUpgrade(log, paths.Data(), agent, previousAgent, action, upgradeDetails, desiredOutcome) + err := tc.markUpgrade(log, paths.Data(), agent, previousAgent, action, upgradeDetails, desiredOutcome) require.Error(t, err) - require.ErrorIs(t, err, os.ErrPermission) + require.ErrorIs(t, err, tc.expectedError) + }) + } +} + +func TestUpdateActiveCommit(t *testing.T) { + log, _ := loggertest.New("test") + testError := errors.New("test error") + testCases := map[string]struct { + expectedError error + writeFileFunc writeFileFunc + }{ + "should return error if it fails writing to file": { + expectedError: testError, + writeFileFunc: func(name string, data []byte, perm os.FileMode) error { + return testError + }, + }, + "should not return error if it writes to file": { + expectedError: nil, + writeFileFunc: func(name string, data []byte, perm os.FileMode) error { + return nil + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := UpdateActiveCommit(log, paths.Top(), "hash", tc.writeFileFunc) + require.ErrorIs(t, err, tc.expectedError) }) } + } From 06e43ebe033e44c4469bad4991103e073694c948 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 4 Sep 2025 14:20:40 +0300 Subject: [PATCH 07/10] enhancement(5235): updated use of markUpgrade in rollback and rollback tests --- internal/pkg/agent/application/upgrade/rollback.go | 2 +- internal/pkg/agent/application/upgrade/rollback_test.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index 90be1bbe2df..b0ba5379cc9 100644 --- a/internal/pkg/agent/application/upgrade/rollback.go +++ b/internal/pkg/agent/application/upgrade/rollback.go @@ -52,7 +52,7 @@ func Rollback(ctx context.Context, log *logger.Logger, c client.Client, topDirPa } // revert active commit - if err := UpdateActiveCommit(log, topDirPath, prevHash); err != nil { + if err := UpdateActiveCommit(log, topDirPath, prevHash, os.WriteFile); err != nil { return err } diff --git a/internal/pkg/agent/application/upgrade/rollback_test.go b/internal/pkg/agent/application/upgrade/rollback_test.go index 3f9cc0a33ab..2548ca1d90f 100644 --- a/internal/pkg/agent/application/upgrade/rollback_test.go +++ b/internal/pkg/agent/application/upgrade/rollback_test.go @@ -504,6 +504,8 @@ func createUpdateMarker(t *testing.T, log *logger.Logger, topDir, newAgentVersio hash: oldAgentHash, versionedHome: oldAgentVersionedHome, } + + markUpgrade := markUpgradeProvider(UpdateActiveCommit, os.WriteFile) err := markUpgrade(log, paths.DataFrom(topDir), newAgentInstall, From 923ee3f70174be3de1336be5193fbedc719e02f1 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 4 Sep 2025 14:21:25 +0300 Subject: [PATCH 08/10] enhancement(5235): abstracted markupgrade from upgrader, added relevant types and updated the upgrader struct. added tests case for mark upgrade error handling in the upgrade function --- .../pkg/agent/application/upgrade/upgrade.go | 7 ++-- .../agent/application/upgrade/upgrade_test.go | 33 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 150ccfd2058..5aaa6e7fb34 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -82,6 +82,7 @@ type unpackHandler interface { type copyActionStoreFunc func(log *logger.Logger, newHome string) error type copyRunDirectoryFunc func(log *logger.Logger, oldRunPath, newRunPath string) error type fileDirCopyFunc func(from, to string, opts ...copy.Options) error +type markUpgradeFunc func(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details, desiredOutcome UpgradeOutcome) error // Types used to abstract stdlib functions type mkdirAllFunc func(name string, perm fs.FileMode) error @@ -104,6 +105,7 @@ type Upgrader struct { extractAgentVersion func(metadata packageMetadata, upgradeVersion string) agentVersion copyActionStore copyActionStoreFunc copyRunDirectory copyRunDirectoryFunc + markUpgrade markUpgradeFunc } // IsUpgradeable when agent is installed and running as a service or flag was provided. @@ -127,6 +129,7 @@ func NewUpgrader(log *logger.Logger, settings *artifact.Config, agentInfo info.A extractAgentVersion: extractAgentVersion, copyActionStore: copyActionStoreProvider(os.ReadFile, os.WriteFile), copyRunDirectory: copyRunDirectoryProvider(os.MkdirAll, copy.Copy), + markUpgrade: markUpgradeProvider(UpdateActiveCommit, os.WriteFile), }, nil } @@ -229,8 +232,6 @@ func checkUpgrade(log *logger.Logger, currentVersion, newVersion agentVersion, m return nil } -var markUpgradeFunc = markUpgrade - // Upgrade upgrades running agent, function returns shutdown callback that must be called by reexec. 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) @@ -387,7 +388,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string versionedHome: currentVersionedHome, } - if err := markUpgradeFunc(u.log, + if err := u.markUpgrade(u.log, paths.Data(), // data dir to place the marker in current, // new agent version data previous, // old agent version data diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index a8561817b85..e4cfd2df768 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1438,6 +1438,39 @@ func TestUpgradeErrorHandling(t *testing.T) { } }, }, + "should return error if markUpgrade fails": { + isDiskSpaceErrorResult: false, + expectedError: testError, + upgraderMocker: func(upgrader *Upgrader) { + upgrader.artifactDownloader = &mockArtifactDownloader{} + upgrader.extractAgentVersion = func(metadata packageMetadata, upgradeVersion string) agentVersion { + return agentVersion{ + version: upgradeVersion, + snapshot: false, + hash: metadata.hash, + } + } + upgrader.unpacker = &mockUnpacker{ + returnPackageMetadata: packageMetadata{ + manifest: &v1.PackageManifest{}, + hash: "hash", + }, + returnUnpackResult: UnpackResult{ + Hash: "hash", + VersionedHome: "versionedHome", + }, + } + upgrader.copyActionStore = func(log *logger.Logger, newHome string) error { + return nil + } + upgrader.copyRunDirectory = func(log *logger.Logger, oldRunPath, newRunPath string) error { + return nil + } + upgrader.markUpgrade = func(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details, desiredOutcome UpgradeOutcome) error { + return testError + } + }, + }, "should add disk space error to the error chain if downloadArtifact fails with disk space error": { isDiskSpaceErrorResult: true, expectedError: upgradeErrors.ErrInsufficientDiskSpace, From 370397653cee9f6d7282cbbc9ac94f3068925eb1 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 4 Sep 2025 15:27:55 +0300 Subject: [PATCH 09/10] enhancement(5235): added abstractions for changesymlink and rollbackInstall in upgrader, updated error handling tests to use these abstractions --- .../pkg/agent/application/upgrade/upgrade.go | 16 +++++++++++----- .../agent/application/upgrade/upgrade_test.go | 6 ++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 5aaa6e7fb34..09444b4698a 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -83,6 +83,8 @@ type copyActionStoreFunc func(log *logger.Logger, newHome string) error type copyRunDirectoryFunc func(log *logger.Logger, oldRunPath, newRunPath string) error type fileDirCopyFunc func(from, to string, opts ...copy.Options) error type markUpgradeFunc func(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details, desiredOutcome UpgradeOutcome) error +type changeSymlinkFunc func(log *logger.Logger, topDirPath, symlinkPath, newTarget string) error +type rollbackInstallFunc func(ctx context.Context, log *logger.Logger, topDirPath, versionedHome, oldVersionedHome string) error // Types used to abstract stdlib functions type mkdirAllFunc func(name string, perm fs.FileMode) error @@ -106,6 +108,8 @@ type Upgrader struct { copyActionStore copyActionStoreFunc copyRunDirectory copyRunDirectoryFunc markUpgrade markUpgradeFunc + changeSymlink changeSymlinkFunc + rollbackInstall rollbackInstallFunc } // IsUpgradeable when agent is installed and running as a service or flag was provided. @@ -130,6 +134,8 @@ func NewUpgrader(log *logger.Logger, settings *artifact.Config, agentInfo info.A copyActionStore: copyActionStoreProvider(os.ReadFile, os.WriteFile), copyRunDirectory: copyRunDirectoryProvider(os.MkdirAll, copy.Copy), markUpgrade: markUpgradeProvider(UpdateActiveCommit, os.WriteFile), + changeSymlink: changeSymlink, + rollbackInstall: rollbackInstall, }, nil } @@ -363,9 +369,9 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string return nil, fmt.Errorf("calculating home path relative to top, home: %q top: %q : %w", paths.Home(), paths.Top(), err) } - if err := changeSymlink(u.log, paths.Top(), symlinkPath, newPath); err != nil { + if err := u.changeSymlink(u.log, paths.Top(), symlinkPath, newPath); err != nil { u.log.Errorw("Rolling back: changing symlink failed", "error.message", err) - rollbackErr := rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) + rollbackErr := u.rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) return nil, goerrors.Join(err, rollbackErr) } @@ -394,7 +400,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string previous, // old agent version data action, det, OUTCOME_UPGRADE); err != nil { u.log.Errorw("Rolling back: marking upgrade failed", "error.message", err) - rollbackErr := rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) + rollbackErr := u.rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) return nil, goerrors.Join(err, rollbackErr) } @@ -403,14 +409,14 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string var watcherCmd *exec.Cmd if watcherCmd, err = InvokeWatcher(u.log, watcherExecutable); err != nil { u.log.Errorw("Rolling back: starting watcher failed", "error.message", err) - rollbackErr := rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) + rollbackErr := u.rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) return nil, goerrors.Join(err, rollbackErr) } watcherWaitErr := waitForWatcher(ctx, u.log, markerFilePath(paths.Data()), watcherMaxWaitTime) if watcherWaitErr != nil { killWatcherErr := watcherCmd.Process.Kill() - rollbackErr := rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) + rollbackErr := u.rollbackInstall(ctx, u.log, paths.Top(), hashedDir, currentVersionedHome) return nil, goerrors.Join(watcherWaitErr, killWatcherErr, rollbackErr) } diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index e4cfd2df768..15e69b09d42 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1466,6 +1466,12 @@ func TestUpgradeErrorHandling(t *testing.T) { upgrader.copyRunDirectory = func(log *logger.Logger, oldRunPath, newRunPath string) error { return nil } + upgrader.changeSymlink = func(log *logger.Logger, topDirPath, symlinkPath, newTarget string) error { + return nil + } + upgrader.rollbackInstall = func(ctx context.Context, log *logger.Logger, topDirPath, versionedHome, oldVersionedHome string) error { + return nil + } upgrader.markUpgrade = func(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details, desiredOutcome UpgradeOutcome) error { return testError } From bbca4d909085ec8a7dfb04609c06cd6afd54316c Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Thu, 4 Sep 2025 15:29:21 +0300 Subject: [PATCH 10/10] enhancement(5235): added error handling test for changesymlink --- .../agent/application/upgrade/upgrade_test.go | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index 15e69b09d42..8082e1322b8 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1438,6 +1438,42 @@ func TestUpgradeErrorHandling(t *testing.T) { } }, }, + "should return error if changeSymlink fails": { + isDiskSpaceErrorResult: false, + expectedError: testError, + upgraderMocker: func(upgrader *Upgrader) { + upgrader.artifactDownloader = &mockArtifactDownloader{} + upgrader.extractAgentVersion = func(metadata packageMetadata, upgradeVersion string) agentVersion { + return agentVersion{ + version: upgradeVersion, + snapshot: false, + hash: metadata.hash, + } + } + upgrader.unpacker = &mockUnpacker{ + returnPackageMetadata: packageMetadata{ + manifest: &v1.PackageManifest{}, + hash: "hash", + }, + returnUnpackResult: UnpackResult{ + Hash: "hash", + VersionedHome: "versionedHome", + }, + } + upgrader.copyActionStore = func(log *logger.Logger, newHome string) error { + return nil + } + upgrader.copyRunDirectory = func(log *logger.Logger, oldRunPath, newRunPath string) error { + return nil + } + upgrader.rollbackInstall = func(ctx context.Context, log *logger.Logger, topDirPath, versionedHome, oldVersionedHome string) error { + return nil + } + upgrader.changeSymlink = func(log *logger.Logger, topDirPath, symlinkPath, newTarget string) error { + return testError + } + }, + }, "should return error if markUpgrade fails": { isDiskSpaceErrorResult: false, expectedError: testError,