Support version in watcher cleanup rollback #4176

pchila · 2024-02-01T13:17:50Z

What does this PR do?

This is a subset of a bigger PR #3950, so CI is not expected to be green, especially regarding the integration tests.

This PR adds new fields in the update marker so that the upgrade watcher Cleanup and Rollback processes can operate correctly with the new elastic-agent-<version>-<hash>.

There is some refactoring to make Cleanup() and Rollback() testable that involves injecting paths rather than obtaining through paths.* functions: this is needed to be able to write unit tests without calling paths.SetTop() which already created flakiness and introduced race conditions in our tests in the past

Why is it important?

Now the agent installation is correctly cleaned up leaving only the new or old version of agent installed, since the watcher is reading the correct paths from the new update marker

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
~~[ ] I have added an entry in ./changelog/fragments using the changelog tool~~
~~[ ] I have added an integration test or an E2E test~~

Author's Checklist

[ ]

How to test this PR locally

Testing this PR is mostly the same as the test procedure for #4174, but now the watcher cleans the installation correctly
After the end of grace period the new agent directory should be the only left inside /opt/Elastic/Agent/data.
If we want to trigger a rollback, it's enough to kill the new agent a few times and then only the old agent installation will be left inside /opt/Elastic/Agent/data.

Known issues:

This has been tested only on Linux. MacOS and Windows fixes will come in a follow-up PR (we need small modifications to make this work mostly related to OS-dependent agent paths).

Related issues

Relates Add the full version number to the installation directory name #2579
Supersedes Use version in path #3950
Requires Use package manifest for install #4173
Requires Use package manifests for upgrade #4174

Use cases

Screenshots

Logs

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

elasticmachine · 2024-02-01T13:17:53Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

elasticmachine · 2024-02-01T13:17:53Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

mergify · 2024-02-02T16:20:22Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b support-version-in-watcher-cleanup-rollback upstream/support-version-in-watcher-cleanup-rollback
git merge upstream/feature/add-agent-version-to-installation-directory-name
git push upstream support-version-in-watcher-cleanup-rollback

cmacknz

The new rollback and cleanup tests are excellent thanks, just one question on handling the "old" marker file format as well.

cmacknz · 2024-02-02T20:03:54Z

internal/pkg/agent/application/upgrade/rollback_test.go

+	err := markUpgrade(log, paths.DataFrom(topDir), newAgentVersion, newAgentHash, newAgentVersionedHome, oldAgentVersion, oldAgentHash, oldAgentVersionedHome, nil, nil)
+	require.NoError(t, err, "error writing fake update marker")


This always creates the marker with the new fields you added, in the case where we come from an old agent, shouldn't it omit them to make the test 100% accurate?

there is a boolean parameter for that in createUpdateMarker that blanks out the new fields to simulate an old upgrade marker just a few lines above this comment
https://github.com/elastic/elastic-agent/pull/4176/files/bf2e849b16f8852a59593e45b8ada9095e190ad1..b17c7132e2b6cd2ff157835379c833435f325fc4#diff-cc4058ffab0db853a72dc782e00a1a7c3c7b7314175bec4538b1f32e69332c66R497-R503

mergify · 2024-02-03T15:54:02Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b support-version-in-watcher-cleanup-rollback upstream/support-version-in-watcher-cleanup-rollback
git merge upstream/feature/add-agent-version-to-installation-directory-name
git push upstream support-version-in-watcher-cleanup-rollback

ycombinator · 2024-02-03T17:38:48Z

Tested this PR locally (on MacOS).

Before upgrade

$ /opt/homebrew/bin/tree . -L 3
.
|-- LICENSE.txt
|-- NOTICE.txt
|-- README.md
|-- co.elastic.elastic-agent.err.log
|-- co.elastic.elastic-agent.out.log
|-- data
|   |-- agent.lock
|   `-- elastic-agent-8.13.0-SNAPSHOT-d04001
|       |-- components
|       |-- elastic-agent
|       |-- elastic-agent.app
|       |-- logs
|       |-- manifest.yaml
|       `-- package.version
|-- elastic-agent -> data/elastic-agent-8.13.0-SNAPSHOT-d04001/elastic-agent.app/Contents/MacOS/elastic-agent
|-- elastic-agent.reference.yml
|-- elastic-agent.yml
|-- fleet.enc
|-- fleet.enc.lock
`-- otel.yml

During upgrade

$ /opt/homebrew/bin/tree . -L 3
.
|-- LICENSE.txt
|-- NOTICE.txt
|-- README.md
|-- co.elastic.elastic-agent.err.log
|-- co.elastic.elastic-agent.out.log
|-- data
|   |-- agent.lock
|   |-- elastic-agent-8.13.0-SNAPSHOT-d04001
|   |   |-- components
|   |   |-- elastic-agent
|   |   |-- elastic-agent.app
|   |   |-- logs
|   |   |-- manifest.yaml
|   |   |-- package.version
|   |   `-- run
|   |-- elastic-agent-8.13.0-repackaged-SNAPSHOT-d04001
|   |   |-- components
|   |   |-- elastic-agent
|   |   |-- elastic-agent.app
|   |   |-- logs
|   |   |-- manifest.yaml
|   |   |-- package.version
|   |   `-- run
|   `-- tmp
|       |-- Hk6rvk9TDibMPcDvpl0jkLE-qDsHWVYL.sock
|       |-- akSPbdqgaHaTY0_J01-dsfYK6JpMz2zn.sock
|       |-- iThI_df0cBKC6YUNGGlKscMkOfz3FBH3.sock
|       `-- xTEtpJ7117ppc6OYvJCaYHbDW8mLjXGe.sock
|-- elastic-agent -> /Library/Elastic/Agent/data/elastic-agent-8.13.0-repackaged-SNAPSHOT-d04001/elastic-agent.app/Contents/MacOS/elastic-agent
|-- elastic-agent.reference.yml
|-- elastic-agent.sock
|-- elastic-agent.yml
|-- fleet.enc
|-- fleet.enc.lock
|-- otel.yml
`-- watcher.lock

After upgrade

$ pwd
/Library/Elastic/Agent
$ /opt/homebrew/bin/tree . -L 3
.
|-- LICENSE.txt
|-- NOTICE.txt
|-- README.md
|-- co.elastic.elastic-agent.err.log
|-- co.elastic.elastic-agent.out.log
|-- data
|   |-- agent.lock
|   |-- elastic-agent-8.13.0-repackaged-SNAPSHOT-d04001
|   |   |-- components
|   |   |-- elastic-agent
|   |   |-- elastic-agent.app
|   |   |-- logs
|   |   |-- manifest.yaml
|   |   |-- package.version
|   |   `-- run
|   `-- tmp
|       |-- Hk6rvk9TDibMPcDvpl0jkLE-qDsHWVYL.sock
|       |-- akSPbdqgaHaTY0_J01-dsfYK6JpMz2zn.sock
|       |-- iThI_df0cBKC6YUNGGlKscMkOfz3FBH3.sock
|       `-- xTEtpJ7117ppc6OYvJCaYHbDW8mLjXGe.sock
|-- elastic-agent -> /Library/Elastic/Agent/data/elastic-agent-8.13.0-repackaged-SNAPSHOT-d04001/elastic-agent.app/Contents/MacOS/elastic-agent
|-- elastic-agent.reference.yml
|-- elastic-agent.sock
|-- elastic-agent.yml
|-- fleet.enc
|-- fleet.enc.lock
|-- otel.yml
`-- watcher.lock

Directory cleanup (of old Agent files) is happening correctly after upgrade. The only thing that looks a bit odd is that the watcher.lock file is still present after the upgrade. But I'm not sure if this is also the case on main. Checking that now...

ycombinator · 2024-02-04T11:38:09Z

The only thing that looks a bit odd is that the watcher.lock file is still present after the upgrade. But I'm not sure if this is also the case on main. Checking that now...

This issue exists when upgrading from 8.12.0 -> 8.13.0-SNAPSHOT built from main. It also exists when upgrading from 8.11.3 -> 8.12.0 and 8.10.0 -> 8.11.3. So it's definitely not an issue introduced by this PR. Moving on with code review...

internal/pkg/agent/application/upgrade/rollback_test.go

ycombinator · 2024-02-04T11:56:41Z

internal/pkg/agent/application/upgrade/step_unpack.go

@@ -81,7 +81,7 @@ func unzip(log *logger.Logger, archivePath, dataDir string) (UnpackResult, error
 			return UnpackResult{}, fmt.Errorf("parsing package manifest: %w", err)
 		}
 		pm.mappings = manifest.Package.PathMappings
-		versionedHome = path.Clean(pm.Map(manifest.Package.VersionedHome))
+		versionedHome = filepath.Clean(pm.Map(manifest.Package.VersionedHome))


Good catch!

ycombinator

Left one general nitpick about table-based testing. PR changes LGTM — love that we are now passing in topDir/dataDir and the unit testing that enabled.

elastic-sonarqube · 2024-02-05T07:56:12Z

Quality Gate passed

The SonarQube Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
50.8% 50.8% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

* extract directory paths to allow unit testing (instead of nested paths.* calls) * introduce tests for watcher cleanup and rollback * close data directory after cleanup

* Use package manifest for install (#4173) * use version in path for rpm and deb * install elastic-agent remapping paths * Use package manifests for upgrade (#4174) * map paths when upgrading using .tar.gz packages * use structured output from unpack to identify old and new agents directories * copy state directories and rotate symlink correctly * Support version in watcher cleanup rollback (#4176) * extract directory paths to allow unit testing (instead of nested paths.* calls) * introduce tests for watcher cleanup and rollback * close data directory after cleanup * Reintroduce upgrade version check (#4177) * Fix shutdownCallback() directories * reintroduce version check in upgrade * Use watcher that supports paths in update marker (#4178) * Invoke agent watcher capable of handling new paths * Adapt TestShutdownCallback to run multiple testcases * Fix shutdownCallback to work with version in path * Fix MacOS relink step * Add commit hash to manifest * document manifest-aware upgrade --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> * small fixes based on PR feedback * Add wait for watcher (#4229) * Fix rollbackInstall order of execution and handle errors * Add wait for watcher up and running --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

pchila added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team skip-changelog labels Feb 1, 2024

pchila self-assigned this Feb 1, 2024

pchila requested a review from a team as a code owner February 1, 2024 13:17

pchila requested review from michalpristas and blakerouse and removed request for a team February 1, 2024 13:17

pchila requested review from ycombinator, cmacknz and AndersonQ February 1, 2024 13:18

This was referenced Feb 1, 2024

Reintroduce upgrade version check #4177

Merged

Use watcher that supports paths in update marker #4178

Merged

Use version in path #3950

Closed

pchila force-pushed the support-version-in-watcher-cleanup-rollback branch from 0ec9173 to b17c713 Compare February 2, 2024 16:48

cmacknz reviewed Feb 2, 2024

View reviewed changes

pchila mentioned this pull request Feb 3, 2024

Use package manifests for upgrade #4174

Merged

3 tasks

pchila added 8 commits February 3, 2024 17:00

extract data dir path from makerFilePath()

d3441b9

inject directory paths into prevSymlinkPath() and markerFilePath()

933a75b

refactor path.Data() to allow passing a custom top directory

450b4f3

more directory path injection

b83b33e

WIP - introduce tests for watcher cleanup and rollback

1159c32

Polish watcher Cleanup() tests and load marker data from file

2e33434

add watcher Rollback() tests

61ddd97

Close data directory after cleanup

d04001c

pchila force-pushed the support-version-in-watcher-cleanup-rollback branch from b17c713 to d04001c Compare February 3, 2024 16:00

pchila requested a review from cmacknz February 3, 2024 16:18

ycombinator reviewed Feb 4, 2024

View reviewed changes

internal/pkg/agent/application/upgrade/rollback_test.go Outdated Show resolved Hide resolved

ycombinator reviewed Feb 4, 2024

View reviewed changes

ycombinator approved these changes Feb 4, 2024

View reviewed changes

Use maps in table-driven unit tests for Rollback and Cleanup

1dc1be8

michalpristas approved these changes Feb 5, 2024

View reviewed changes

pchila merged commit a831764 into elastic:feature/add-agent-version-to-installation-directory-name Feb 5, 2024
8 of 9 checks passed

pchila mentioned this pull request Feb 6, 2024

Feature/add agent version to installation directory name #4193

Merged

5 tasks

pchila deleted the support-version-in-watcher-cleanup-rollback branch February 13, 2024 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support version in watcher cleanup rollback #4176

Support version in watcher cleanup rollback #4176

pchila commented Feb 1, 2024 •

edited

Loading

elasticmachine commented Feb 1, 2024

elasticmachine commented Feb 1, 2024

mergify bot commented Feb 2, 2024

cmacknz left a comment

cmacknz Feb 2, 2024

pchila Feb 3, 2024

mergify bot commented Feb 3, 2024

ycombinator commented Feb 3, 2024

ycombinator commented Feb 4, 2024

ycombinator Feb 4, 2024

ycombinator left a comment

elastic-sonarqube bot commented Feb 5, 2024

		err := markUpgrade(log, paths.DataFrom(topDir), newAgentVersion, newAgentHash, newAgentVersionedHome, oldAgentVersion, oldAgentHash, oldAgentVersionedHome, nil, nil)
		require.NoError(t, err, "error writing fake update marker")

Support version in watcher cleanup rollback #4176

Support version in watcher cleanup rollback #4176

Conversation

pchila commented Feb 1, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

elasticmachine commented Feb 1, 2024

elasticmachine commented Feb 1, 2024

mergify bot commented Feb 2, 2024

cmacknz left a comment

Choose a reason for hiding this comment

cmacknz Feb 2, 2024

Choose a reason for hiding this comment

pchila Feb 3, 2024

Choose a reason for hiding this comment

mergify bot commented Feb 3, 2024

ycombinator commented Feb 3, 2024

Before upgrade

During upgrade

After upgrade

ycombinator commented Feb 4, 2024

ycombinator Feb 4, 2024

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Feb 5, 2024

Quality Gate passed

pchila commented Feb 1, 2024 •

edited

Loading