From 3b9fbd18bc381bc1222028c93ec950869d3b33ce Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Mon, 15 Sep 2025 18:29:21 +0300 Subject: [PATCH 1/4] Enhancement/5235 wrap errors when marking upgrade (#9366) * enhancement(5235): added markUpgradeFunc to abstract markUpgrade * enhancement(5235): using markUpgradeFunc in Upgrade function * enhancement(5235): wrapping writeFile errors in markUpgrade enhancement(5235): added goerrors in step_mark * enhancement(5235): using writeFile wrapper in markUpgrade * enhancement(5235): added tests mark upgrade tests enhancement(5235): updated step mark test to mock writeFile * enhancement(5235): updated markUpgrade function so that it can be tested, jecting dependencies. updated relevant tests * enhancement(5235): updated use of markUpgrade in rollback and rollback tests * 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 * enhancement(5235): added abstractions for changesymlink and rollbackInstall in upgrader, updated error handling tests to use these abstractions * enhancement(5235): added error handling test for changesymlink (cherry picked from commit 2e4e777312bf49f9f5214262ef2991299977de3e) # Conflicts: # internal/pkg/agent/application/upgrade/step_mark.go # internal/pkg/agent/application/upgrade/step_mark_test.go # internal/pkg/agent/application/upgrade/upgrade.go # internal/pkg/agent/application/upgrade/upgrade_test.go --- .../pkg/agent/application/upgrade/rollback.go | 2 +- .../application/upgrade/rollback_test.go | 2 + .../agent/application/upgrade/step_mark.go | 57 ++- .../application/upgrade/step_mark_test.go | 359 ++++++++++++++++++ .../pkg/agent/application/upgrade/upgrade.go | 45 ++- .../agent/application/upgrade/upgrade_test.go | 136 +++++++ 6 files changed, 589 insertions(+), 12 deletions(-) create mode 100644 internal/pkg/agent/application/upgrade/step_mark_test.go diff --git a/internal/pkg/agent/application/upgrade/rollback.go b/internal/pkg/agent/application/upgrade/rollback.go index b562ab323b6..ef0ed1b9516 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 d4c36916e09..8b190e38a69 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, diff --git a/internal/pkg/agent/application/upgrade/step_mark.go b/internal/pkg/agent/application/upgrade/step_mark.go index 08510af1151..4633673f06a 100644 --- a/internal/pkg/agent/application/upgrade/step_mark.go +++ b/internal/pkg/agent/application/upgrade/step_mark.go @@ -5,6 +5,12 @@ package upgrade import ( +<<<<<<< HEAD +======= + "encoding/json" + goerrors "errors" + "fmt" +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) "os" "path/filepath" "time" @@ -125,12 +131,51 @@ type agentInstall struct { versionedHome string } +type updateActiveCommitFunc func(log *logger.Logger, topDirPath, hash string, writeFile writeFileFunc) error + // markUpgrade marks update happened so we can handle grace period +<<<<<<< HEAD func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details) error { +======= +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 { +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) + + 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 len(previousAgent.hash) > hashLen { - previousAgent.hash = previousAgent.hash[:hashLen] + return nil } +<<<<<<< HEAD marker := &UpdateMarker{ Version: agent.version, @@ -160,14 +205,16 @@ func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent ag } return nil +======= +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) } // 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 := os.WriteFile(activeCommitPath, []byte(hash), 0600); err != nil { - return errors.New(err, errors.TypeFilesystem, "failed to update active commit", errors.M(errors.MetaKeyPath, activeCommitPath)) + 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))) } return nil diff --git a/internal/pkg/agent/application/upgrade/step_mark_test.go b/internal/pkg/agent/application/upgrade/step_mark_test.go new file mode 100644 index 00000000000..34ac01e4888 --- /dev/null +++ b/internal/pkg/agent/application/upgrade/step_mark_test.go @@ -0,0 +1,359 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License 2.0; +// you may not use this file except in compliance with the Elastic License 2.0. + +package upgrade + +import ( + "errors" + "os" + "path/filepath" + "testing" + "time" + + "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/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" +) + +func TestSaveAndLoadMarker_NoLoss(t *testing.T) { + // Create a temporary directory for the test + tempDir := t.TempDir() + markerFile := filepath.Join(tempDir, "test-marker.yaml") + + // Populate all fields of UpdateMarker + originalMarker := &UpdateMarker{ + Version: "8.5.0", + Hash: "abc123", + VersionedHome: "home/v8.5.0", + UpdatedOn: time.Now(), + PrevVersion: "8.4.0", + PrevHash: "xyz789", + PrevVersionedHome: "home/v8.4.0", + Acked: true, + Action: &fleetapi.ActionUpgrade{ + ActionID: "action-123", + ActionType: "UPGRADE", + Data: fleetapi.ActionUpgradeData{ + Version: "8.5.0", + SourceURI: "https://example.com/upgrade", + }, + }, + Details: details.NewDetails( + "8.5.0", + details.StateScheduled, + "action-123"), + DesiredOutcome: OUTCOME_UPGRADE, + } + + // Save the marker to the temporary file + err := saveMarkerToPath(originalMarker, markerFile, true) + require.NoError(t, err, "Failed to save marker") + + // Load the marker from the temporary file + loadedMarker, err := loadMarker(markerFile) + require.NoError(t, err, "Failed to load marker") + + // Compare time separately due to potential precision differences + require.WithinDuration(t, originalMarker.UpdatedOn, loadedMarker.UpdatedOn, time.Second, "UpdatedOn mismatch") + + // Compare details separately to avoid issues with comparing observers + require.True(t, originalMarker.Details.Equals(loadedMarker.Details), "Details mismatch") + + // Set the same time for deep comparison to avoid time precision issues + originalMarkerCopy := *originalMarker + originalMarkerCopy.UpdatedOn = loadedMarker.UpdatedOn + + originalMarkerCopy.Details = nil // Details are already compared separately + loadedMarkerCopy := *loadedMarker + loadedMarkerCopy.Details = nil // Details are already compared separately + + // Compare the rest of the fields + require.Equal(t, originalMarkerCopy, loadedMarkerCopy, "Loaded marker does not match original marker") + + // Clean up the temporary file + err = os.Remove(markerFile) + require.NoError(t, err, "Failed to clean up marker file") +} + +func TestDesiredOutcome_Serialization(t *testing.T) { + tempDir := t.TempDir() + + testCases := []struct { + name string + desiredOutcome UpgradeOutcome + expectError bool + }{ + { + name: "OUTCOME_UPGRADE", + desiredOutcome: OUTCOME_UPGRADE, + expectError: false, + }, + { + name: "OUTCOME_ROLLBACK", + desiredOutcome: OUTCOME_ROLLBACK, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + markerFile := filepath.Join(tempDir, tc.name+"-marker.yaml") + + // Create marker with specific DesiredOutcome + + originalMarker := &UpdateMarker{ + Version: "8.5.0", + Hash: "abc123", + VersionedHome: "home/v8.5.0", + UpdatedOn: time.Now(), + PrevVersion: "8.4.0", + PrevHash: "xyz789", + PrevVersionedHome: "home/v8.4.0", + Acked: true, + Action: &fleetapi.ActionUpgrade{ + ActionID: "action-123", + ActionType: "UPGRADE", + Data: fleetapi.ActionUpgradeData{ + Version: "8.5.0", + SourceURI: "https://example.com/upgrade", + }, + }, + Details: details.NewDetails( + "8.5.0", + details.StateScheduled, + "action-123"), + DesiredOutcome: tc.desiredOutcome, + } + + // Save the marker + err := saveMarkerToPath(originalMarker, markerFile, true) + if tc.expectError { + require.Error(t, err, "Expected error during save for %s", tc.name) + return + } + require.NoError(t, err, "Failed to save marker for %s", tc.name) + + // Load the marker + loadedMarker, err := loadMarker(markerFile) + require.NoError(t, err, "Failed to load marker for %s", tc.name) + require.NotNil(t, loadedMarker, "loaded marker should not be nil") + + // For valid values, also check they match expected constants + switch tc.desiredOutcome { + case OUTCOME_UPGRADE: + require.Equal(t, OUTCOME_UPGRADE, loadedMarker.DesiredOutcome, "OUTCOME_UPGRADE mismatch") + case OUTCOME_ROLLBACK: + require.Equal(t, OUTCOME_ROLLBACK, loadedMarker.DesiredOutcome, "OUTCOME_ROLLBACK mismatch") + default: + require.Equal(t, OUTCOME_UPGRADE, loadedMarker.DesiredOutcome, + "DesiredOutcome not preserved during serialization for %s (expected: %d, got: %d)", + tc.name, originalMarker.DesiredOutcome, loadedMarker.DesiredOutcome) + } + + // Clean up + err = os.Remove(markerFile) + require.NoError(t, err, "Failed to clean up marker file for %s", tc.name) + }) + } +} + +func TestDesiredOutcome_InvalidYAMLContent(t *testing.T) { + tempDir := t.TempDir() + markerFile := filepath.Join(tempDir, "invalid-marker.yaml") + + // Test cases with invalid YAML content for DesiredOutcome + testCases := []struct { + name string + yamlContent string + expectError bool + expectedValue UpgradeOutcome + }{ + { + name: "Missing value", + yamlContent: ` +version: "8.5.0" +hash: "abc123" +versioned_home: "home/v8.5.0" +updated_on: 2023-01-01T00:00:00Z +`, + expectError: false, + expectedValue: OUTCOME_UPGRADE, + }, + { + name: "ProperValue", + yamlContent: ` +version: "8.5.0" +hash: "abc123" +versioned_home: "home/v8.5.0" +updated_on: 2023-01-01T00:00:00Z +desired_outcome: "UPGRADE" +`, + expectError: false, + expectedValue: OUTCOME_UPGRADE, + }, + { + name: "RollbackValue", + yamlContent: ` +version: "8.5.0" +hash: "abc123" +versioned_home: "home/v8.5.0" +updated_on: 2023-01-01T00:00:00Z +desired_outcome: "ROLLBACK" +`, + expectError: false, + expectedValue: OUTCOME_ROLLBACK, + }, + { + name: "StringValue", + yamlContent: ` +version: "8.5.0" +hash: "abc123" +versioned_home: "home/v8.5.0" +updated_on: 2023-01-01T00:00:00Z +desired_outcome: "invalid_string" +`, + expectError: true, + }, + { + name: "FloatValue", + yamlContent: ` +version: "8.5.0" +hash: "abc123" +versioned_home: "home/v8.5.0" +updated_on: 2023-01-01T00:00:00Z +desired_outcome: 1.5 +`, + expectError: true, + }, + { + name: "BooleanValue", + yamlContent: ` +version: "8.5.0" +hash: "abc123" +versioned_home: "home/v8.5.0" +updated_on: 2023-01-01T00:00:00Z +desired_outcome: true +`, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Write invalid YAML content to file + err := os.WriteFile(markerFile, []byte(tc.yamlContent), 0644) + require.NoError(t, err, "Failed to write test YAML file") + + // Try to load the marker + marker, err := loadMarker(markerFile) + if tc.expectError { + require.Error(t, err, "Expected error when loading invalid YAML for %s", tc.name) + } else { + require.NoError(t, err, "Unexpected error when loading YAML for %s", tc.name) + require.Equal(t, tc.expectedValue, marker.DesiredOutcome) + } + + // Clean up + err = os.Remove(markerFile) + require.NoError(t, err, "Failed to clean up marker file") + }) + } +} + +func TestMarkUpgrade(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 + + testError := errors.New("test error") + + type testCase struct { + fileName string + expectedError error + markUpgrade markUpgradeFunc + } + + testCases := map[string]testCase{ + "should return error if it fails updating the active commit file": { + fileName: "commit", + 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 marker file": { + fileName: "marker", + 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 + }), + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + baseDir := t.TempDir() + paths.SetTop(baseDir) + + err := tc.markUpgrade(log, paths.Data(), agent, previousAgent, action, upgradeDetails, desiredOutcome) + require.Error(t, err) + 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) + }) + } + +} diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index bc1a8061f18..1d57d00cf9c 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -67,6 +67,22 @@ type unpackHandler interface { getPackageMetadata(archivePath string) (packageMetadata, error) } +<<<<<<< HEAD +======= +// Types used to abstract copyActionStore, copyRunDirectory and github.com/otiai10/copy.Copy +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 +type readFileFunc func(name string) ([]byte, error) +type writeFileFunc func(name string, data []byte, perm fs.FileMode) error + +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) // Upgrader performs an upgrade type Upgrader struct { log *logger.Logger @@ -81,6 +97,14 @@ type Upgrader struct { unpacker unpackHandler isDiskSpaceErrorFunc func(err error) bool extractAgentVersion func(metadata packageMetadata, upgradeVersion string) agentVersion +<<<<<<< HEAD +======= + copyActionStore copyActionStoreFunc + copyRunDirectory copyRunDirectoryFunc + markUpgrade markUpgradeFunc + changeSymlink changeSymlinkFunc + rollbackInstall rollbackInstallFunc +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) } // IsUpgradeable when agent is installed and running as a service or flag was provided. @@ -101,6 +125,15 @@ func NewUpgrader(log *logger.Logger, settings *artifact.Config, agentInfo info.A artifactDownloader: newArtifactDownloader(settings, log), unpacker: newUnpacker(log), isDiskSpaceErrorFunc: upgradeErrors.IsDiskSpaceError, +<<<<<<< HEAD +======= + extractAgentVersion: extractAgentVersion, + copyActionStore: copyActionStoreProvider(os.ReadFile, os.WriteFile), + copyRunDirectory: copyRunDirectoryProvider(os.MkdirAll, copy.Copy), + markUpgrade: markUpgradeProvider(UpdateActiveCommit, os.WriteFile), + changeSymlink: changeSymlink, + rollbackInstall: rollbackInstall, +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) }, nil } @@ -304,9 +337,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) } @@ -329,13 +362,13 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string versionedHome: currentVersionedHome, } - if err := markUpgrade(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 action, det); 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) } @@ -344,14 +377,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 2d77524edb2..613c6c92f33 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1216,6 +1216,142 @@ func TestUpgradeErrorHandling(t *testing.T) { } }, }, +<<<<<<< HEAD +======= + "should return error if copyActionStore 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 testError + } + }, + }, + "should return error if copyRunDirectory fails": { + isDiskSpaceErrorResult: false, + expectedError: testError, + upgraderMocker: func(upgrader *Upgrader) { + upgrader.artifactDownloader = &mockArtifactDownloader{} + 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 testError + } + }, + }, + "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, + 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.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 + } + }, + }, +>>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) "should add disk space error to the error chain if downloadArtifact fails with disk space error": { isDiskSpaceErrorResult: true, expectedError: upgradeErrors.ErrInsufficientDiskSpace, From 53ae1fe3593e87f051412270ffd4dd1fa90ecfe3 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Wed, 17 Sep 2025 17:38:28 +0300 Subject: [PATCH 2/4] resolve conflicts --- .../agent/application/upgrade/step_mark.go | 41 ------------------- .../pkg/agent/application/upgrade/upgrade.go | 10 ----- .../agent/application/upgrade/upgrade_test.go | 3 -- 3 files changed, 54 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/step_mark.go b/internal/pkg/agent/application/upgrade/step_mark.go index 4633673f06a..efcb40b1bbd 100644 --- a/internal/pkg/agent/application/upgrade/step_mark.go +++ b/internal/pkg/agent/application/upgrade/step_mark.go @@ -5,12 +5,7 @@ package upgrade import ( -<<<<<<< HEAD -======= - "encoding/json" goerrors "errors" - "fmt" ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) "os" "path/filepath" "time" @@ -134,12 +129,8 @@ type agentInstall struct { type updateActiveCommitFunc func(log *logger.Logger, topDirPath, hash string, writeFile writeFileFunc) error // markUpgrade marks update happened so we can handle grace period -<<<<<<< HEAD -func markUpgrade(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details) error { -======= 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 { ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) if len(previousAgent.hash) > hashLen { previousAgent.hash = previousAgent.hash[:hashLen] @@ -175,38 +166,6 @@ func markUpgradeProvider(updateActiveCommit updateActiveCommitFunc, writeFile wr return nil } -<<<<<<< HEAD - - 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, - } - - 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 := os.WriteFile(markerPath, markerBytes, 0600); err != nil { - return errors.New(err, errors.TypeFilesystem, "failed to create update marker file", errors.M(errors.MetaKeyPath, markerPath)) - } - - if err := UpdateActiveCommit(log, paths.Top(), agent.hash); err != nil { - return err - } - - return nil -======= ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) } // UpdateActiveCommit updates active.commit file to point to active version. diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index 1d57d00cf9c..180b8c5e021 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -67,8 +67,6 @@ type unpackHandler interface { getPackageMetadata(archivePath string) (packageMetadata, error) } -<<<<<<< HEAD -======= // Types used to abstract copyActionStore, copyRunDirectory and github.com/otiai10/copy.Copy type copyActionStoreFunc func(log *logger.Logger, newHome string) error type copyRunDirectoryFunc func(log *logger.Logger, oldRunPath, newRunPath string) error @@ -82,7 +80,6 @@ type mkdirAllFunc func(name string, perm fs.FileMode) error type readFileFunc func(name string) ([]byte, error) type writeFileFunc func(name string, data []byte, perm fs.FileMode) error ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) // Upgrader performs an upgrade type Upgrader struct { log *logger.Logger @@ -97,14 +94,11 @@ type Upgrader struct { unpacker unpackHandler isDiskSpaceErrorFunc func(err error) bool extractAgentVersion func(metadata packageMetadata, upgradeVersion string) agentVersion -<<<<<<< HEAD -======= copyActionStore copyActionStoreFunc copyRunDirectory copyRunDirectoryFunc markUpgrade markUpgradeFunc changeSymlink changeSymlinkFunc rollbackInstall rollbackInstallFunc ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) } // IsUpgradeable when agent is installed and running as a service or flag was provided. @@ -125,15 +119,11 @@ func NewUpgrader(log *logger.Logger, settings *artifact.Config, agentInfo info.A artifactDownloader: newArtifactDownloader(settings, log), unpacker: newUnpacker(log), isDiskSpaceErrorFunc: upgradeErrors.IsDiskSpaceError, -<<<<<<< HEAD -======= - extractAgentVersion: extractAgentVersion, copyActionStore: copyActionStoreProvider(os.ReadFile, os.WriteFile), copyRunDirectory: copyRunDirectoryProvider(os.MkdirAll, copy.Copy), markUpgrade: markUpgradeProvider(UpdateActiveCommit, os.WriteFile), changeSymlink: changeSymlink, rollbackInstall: rollbackInstall, ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) }, nil } diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index 613c6c92f33..cb2810d22d9 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/internal/pkg/agent/application/upgrade/upgrade_test.go @@ -1216,8 +1216,6 @@ func TestUpgradeErrorHandling(t *testing.T) { } }, }, -<<<<<<< HEAD -======= "should return error if copyActionStore fails": { isDiskSpaceErrorResult: false, expectedError: testError, @@ -1351,7 +1349,6 @@ func TestUpgradeErrorHandling(t *testing.T) { } }, }, ->>>>>>> 2e4e77731 (Enhancement/5235 wrap errors when marking upgrade (#9366)) "should add disk space error to the error chain if downloadArtifact fails with disk space error": { isDiskSpaceErrorResult: true, expectedError: upgradeErrors.ErrInsufficientDiskSpace, From 0c8cbf7320580dec404c281f1aa5a33b17469540 Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Wed, 17 Sep 2025 17:44:43 +0300 Subject: [PATCH 3/4] removed unused tests --- .../application/upgrade/step_mark_test.go | 250 +----------------- 1 file changed, 1 insertion(+), 249 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/step_mark_test.go b/internal/pkg/agent/application/upgrade/step_mark_test.go index 34ac01e4888..0d50b0fbaae 100644 --- a/internal/pkg/agent/application/upgrade/step_mark_test.go +++ b/internal/pkg/agent/application/upgrade/step_mark_test.go @@ -7,9 +7,7 @@ package upgrade import ( "errors" "os" - "path/filepath" "testing" - "time" "github.com/stretchr/testify/require" @@ -20,251 +18,6 @@ import ( "github.com/elastic/elastic-agent/pkg/core/logger/loggertest" ) -func TestSaveAndLoadMarker_NoLoss(t *testing.T) { - // Create a temporary directory for the test - tempDir := t.TempDir() - markerFile := filepath.Join(tempDir, "test-marker.yaml") - - // Populate all fields of UpdateMarker - originalMarker := &UpdateMarker{ - Version: "8.5.0", - Hash: "abc123", - VersionedHome: "home/v8.5.0", - UpdatedOn: time.Now(), - PrevVersion: "8.4.0", - PrevHash: "xyz789", - PrevVersionedHome: "home/v8.4.0", - Acked: true, - Action: &fleetapi.ActionUpgrade{ - ActionID: "action-123", - ActionType: "UPGRADE", - Data: fleetapi.ActionUpgradeData{ - Version: "8.5.0", - SourceURI: "https://example.com/upgrade", - }, - }, - Details: details.NewDetails( - "8.5.0", - details.StateScheduled, - "action-123"), - DesiredOutcome: OUTCOME_UPGRADE, - } - - // Save the marker to the temporary file - err := saveMarkerToPath(originalMarker, markerFile, true) - require.NoError(t, err, "Failed to save marker") - - // Load the marker from the temporary file - loadedMarker, err := loadMarker(markerFile) - require.NoError(t, err, "Failed to load marker") - - // Compare time separately due to potential precision differences - require.WithinDuration(t, originalMarker.UpdatedOn, loadedMarker.UpdatedOn, time.Second, "UpdatedOn mismatch") - - // Compare details separately to avoid issues with comparing observers - require.True(t, originalMarker.Details.Equals(loadedMarker.Details), "Details mismatch") - - // Set the same time for deep comparison to avoid time precision issues - originalMarkerCopy := *originalMarker - originalMarkerCopy.UpdatedOn = loadedMarker.UpdatedOn - - originalMarkerCopy.Details = nil // Details are already compared separately - loadedMarkerCopy := *loadedMarker - loadedMarkerCopy.Details = nil // Details are already compared separately - - // Compare the rest of the fields - require.Equal(t, originalMarkerCopy, loadedMarkerCopy, "Loaded marker does not match original marker") - - // Clean up the temporary file - err = os.Remove(markerFile) - require.NoError(t, err, "Failed to clean up marker file") -} - -func TestDesiredOutcome_Serialization(t *testing.T) { - tempDir := t.TempDir() - - testCases := []struct { - name string - desiredOutcome UpgradeOutcome - expectError bool - }{ - { - name: "OUTCOME_UPGRADE", - desiredOutcome: OUTCOME_UPGRADE, - expectError: false, - }, - { - name: "OUTCOME_ROLLBACK", - desiredOutcome: OUTCOME_ROLLBACK, - expectError: false, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - markerFile := filepath.Join(tempDir, tc.name+"-marker.yaml") - - // Create marker with specific DesiredOutcome - - originalMarker := &UpdateMarker{ - Version: "8.5.0", - Hash: "abc123", - VersionedHome: "home/v8.5.0", - UpdatedOn: time.Now(), - PrevVersion: "8.4.0", - PrevHash: "xyz789", - PrevVersionedHome: "home/v8.4.0", - Acked: true, - Action: &fleetapi.ActionUpgrade{ - ActionID: "action-123", - ActionType: "UPGRADE", - Data: fleetapi.ActionUpgradeData{ - Version: "8.5.0", - SourceURI: "https://example.com/upgrade", - }, - }, - Details: details.NewDetails( - "8.5.0", - details.StateScheduled, - "action-123"), - DesiredOutcome: tc.desiredOutcome, - } - - // Save the marker - err := saveMarkerToPath(originalMarker, markerFile, true) - if tc.expectError { - require.Error(t, err, "Expected error during save for %s", tc.name) - return - } - require.NoError(t, err, "Failed to save marker for %s", tc.name) - - // Load the marker - loadedMarker, err := loadMarker(markerFile) - require.NoError(t, err, "Failed to load marker for %s", tc.name) - require.NotNil(t, loadedMarker, "loaded marker should not be nil") - - // For valid values, also check they match expected constants - switch tc.desiredOutcome { - case OUTCOME_UPGRADE: - require.Equal(t, OUTCOME_UPGRADE, loadedMarker.DesiredOutcome, "OUTCOME_UPGRADE mismatch") - case OUTCOME_ROLLBACK: - require.Equal(t, OUTCOME_ROLLBACK, loadedMarker.DesiredOutcome, "OUTCOME_ROLLBACK mismatch") - default: - require.Equal(t, OUTCOME_UPGRADE, loadedMarker.DesiredOutcome, - "DesiredOutcome not preserved during serialization for %s (expected: %d, got: %d)", - tc.name, originalMarker.DesiredOutcome, loadedMarker.DesiredOutcome) - } - - // Clean up - err = os.Remove(markerFile) - require.NoError(t, err, "Failed to clean up marker file for %s", tc.name) - }) - } -} - -func TestDesiredOutcome_InvalidYAMLContent(t *testing.T) { - tempDir := t.TempDir() - markerFile := filepath.Join(tempDir, "invalid-marker.yaml") - - // Test cases with invalid YAML content for DesiredOutcome - testCases := []struct { - name string - yamlContent string - expectError bool - expectedValue UpgradeOutcome - }{ - { - name: "Missing value", - yamlContent: ` -version: "8.5.0" -hash: "abc123" -versioned_home: "home/v8.5.0" -updated_on: 2023-01-01T00:00:00Z -`, - expectError: false, - expectedValue: OUTCOME_UPGRADE, - }, - { - name: "ProperValue", - yamlContent: ` -version: "8.5.0" -hash: "abc123" -versioned_home: "home/v8.5.0" -updated_on: 2023-01-01T00:00:00Z -desired_outcome: "UPGRADE" -`, - expectError: false, - expectedValue: OUTCOME_UPGRADE, - }, - { - name: "RollbackValue", - yamlContent: ` -version: "8.5.0" -hash: "abc123" -versioned_home: "home/v8.5.0" -updated_on: 2023-01-01T00:00:00Z -desired_outcome: "ROLLBACK" -`, - expectError: false, - expectedValue: OUTCOME_ROLLBACK, - }, - { - name: "StringValue", - yamlContent: ` -version: "8.5.0" -hash: "abc123" -versioned_home: "home/v8.5.0" -updated_on: 2023-01-01T00:00:00Z -desired_outcome: "invalid_string" -`, - expectError: true, - }, - { - name: "FloatValue", - yamlContent: ` -version: "8.5.0" -hash: "abc123" -versioned_home: "home/v8.5.0" -updated_on: 2023-01-01T00:00:00Z -desired_outcome: 1.5 -`, - expectError: true, - }, - { - name: "BooleanValue", - yamlContent: ` -version: "8.5.0" -hash: "abc123" -versioned_home: "home/v8.5.0" -updated_on: 2023-01-01T00:00:00Z -desired_outcome: true -`, - expectError: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - // Write invalid YAML content to file - err := os.WriteFile(markerFile, []byte(tc.yamlContent), 0644) - require.NoError(t, err, "Failed to write test YAML file") - - // Try to load the marker - marker, err := loadMarker(markerFile) - if tc.expectError { - require.Error(t, err, "Expected error when loading invalid YAML for %s", tc.name) - } else { - require.NoError(t, err, "Unexpected error when loading YAML for %s", tc.name) - require.Equal(t, tc.expectedValue, marker.DesiredOutcome) - } - - // Clean up - err = os.Remove(markerFile) - require.NoError(t, err, "Failed to clean up marker file") - }) - } -} - func TestMarkUpgrade(t *testing.T) { log, _ := loggertest.New("test") agent := agentInstall{ @@ -286,7 +39,6 @@ func TestMarkUpgrade(t *testing.T) { }, } upgradeDetails := details.NewDetails("8.5.0", details.StateScheduled, "action-123") - desiredOutcome := OUTCOME_UPGRADE testError := errors.New("test error") @@ -322,7 +74,7 @@ func TestMarkUpgrade(t *testing.T) { baseDir := t.TempDir() paths.SetTop(baseDir) - err := tc.markUpgrade(log, paths.Data(), agent, previousAgent, action, upgradeDetails, desiredOutcome) + err := tc.markUpgrade(log, paths.Data(), agent, previousAgent, action, upgradeDetails) require.Error(t, err) require.ErrorIs(t, err, tc.expectedError) }) From 4d99162f9e6f0950b707cf6cb56f816a92acc5fa Mon Sep 17 00:00:00 2001 From: Kaan Yalti Date: Wed, 17 Sep 2025 17:44:56 +0300 Subject: [PATCH 4/4] removed unused var --- internal/pkg/agent/application/upgrade/step_mark.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/step_mark.go b/internal/pkg/agent/application/upgrade/step_mark.go index efcb40b1bbd..924d5567473 100644 --- a/internal/pkg/agent/application/upgrade/step_mark.go +++ b/internal/pkg/agent/application/upgrade/step_mark.go @@ -130,7 +130,7 @@ type updateActiveCommitFunc func(log *logger.Logger, topDirPath, hash string, wr // 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 { + return func(log *logger.Logger, dataDirPath string, agent, previousAgent agentInstall, action *fleetapi.ActionUpgrade, upgradeDetails *details.Details) error { if len(previousAgent.hash) > hashLen { previousAgent.hash = previousAgent.hash[:hashLen] @@ -146,7 +146,6 @@ func markUpgradeProvider(updateActiveCommit updateActiveCommitFunc, writeFile wr PrevVersionedHome: previousAgent.versionedHome, Action: action, Details: upgradeDetails, - DesiredOutcome: desiredOutcome, } markerBytes, err := yaml.Marshal(newMarkerSerializer(marker))