Create installation marker on upgrade from versions < 8.8.0 #2655

ycombinator · 2023-05-10T19:18:46Z

What does this PR do?

This PR checks if Agent is being run as part of an ongoing upgrade (i.e. being re-exec'd) and, if so, creates the installation marker file if the previous version of the Agent (the one being upgraded from) was one before the installation marker file was introduced.

Why is it important?

In v8.8.0, we introduced an installation marker file to indicate that an Agent was running as installed (this was necessary to implement the custom installation --base-path feature). When an installed Agent that's older than v8.8.0 is upgraded, this installation marker file is not present. In such cases, we need to create it manually post-upgrade. Otherwise, the upgrade will be unsuccessful.

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~~ will be covered as part of Port over Agent Upgrade test #2618

How to test this PR locally

Install a version of Elastic Agent older than 8.8.0.

cd /tmp
wget https://artifacts.elastic.co/downloads/beats/elastic-agent/elastic-agent-8.2.3-darwin-aarch64.tar.gz
tar xzf elastic-agent-8.2.3-darwin-aarch64.tar.gz
cd elastic-agent-8.2.3-darwin-aarch64
sudo ./elastic-agent install

Build Agent with this PR (should result in an Agent with version 8.9.0-SNAPSHOT).

cd /path/to/agent/repo
DEV=true SNAPSHOT=true EXTERNAL=true PLATFORMS="darwin/arm64" PACKAGES="tar.gz" mage -v clean package

Upgrade to the Agent built with this PR.

cd
sudo elastic-agent upgrade --source-uri file:///path/to/agent/repo/build/distributions/ --skip-verify 8.9.0-SNAPSHOT

Wait until the upgrade watcher has finished running. This should be 10 minutes. Waiting ensures that the Agent is either successfully completed the upgrade or has been rolled back. You can also keep checking the process list and wait until there is no elastic-agent watch process present.
```
ps auxw | grep 'elastic-agent watch' | grep -v grep
```
Check that the new version of Agent is running.
```
sudo elastic-agent version
```
Check that the running Agent is healthy.
```
sudo elastic-agent status
```

Related issues

Fixes [Bug] Upgrade not working #2645

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?
...

mergify · 2023-05-10T19:19:20Z

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2023-05-10T19:24:41Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-05-11T07:52:14.278+0000
Duration: 17 min 10 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5675
Skipped	23
Total	5698

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-05-10T19:41:21Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.592% (`70/71`)	👍
Files	68.98% (`169/245`)	👍
Classes	67.965% (`314/462`)	👍
Methods	54.077% (`955/1766`)	👍
Lines	39.755% (`10957/27561`)	👍 0.01
Conditionals	100.0% (`0/0`)	💚

cmacknz · 2023-05-10T20:04:07Z

internal/pkg/agent/cmd/run.go

+	// https://github.com/elastic/elastic-agent/issues/2645).
+
+	installMarkerVersion := semver.MustParse("8.8.0")
+	prevVersion, err := semver.NewVersion(upgradeMarker.PrevVersion)


Probably worth checking to see if this PrevVersion field always exists in every version of the upgrade marker file that can be created. The 7.17.0 release is a good place to start checking.

Does 8.8.0 always create the installation marker? If it does then the absence of the installation marker implies that this is an agent version prior to 8.8.0 installed in the default location since that was the only option. Is that possibly safer since it doesn't depend on the format of the upgrade marker file?

I have no issue with approach as long as we can prove it will always work.

Does 8.8.0 always create the installation marker? If it does then the absence of the installation marker implies that this is an agent version prior to 8.8.0 installed in the default location since that was the only option. Is that possibly safer since it doesn't depend on the format of the upgrade marker file?

Yes, we always create the installation marker in 8.8.0 when Agent is installed using elastic-agent install. That said, I also like the idea of not depending on the format of the upgrade marker file to determine the pre-upgrade version.

I do think we need to check for the existence of the upgrade marker file, though, to know that we're in the midst of an upgrade. Otherwise, we'll end up creating the installation marker even if a user simply ran elastic-agent run or more likely, in the Docker use case (probably harmless).

Yes, checking for the existence of the file should be fine. Looking inside it is more likely to have unexpected results. I'm not sure what guarantees we have about its format so that likely means we have none.

The format is guaranteed by the mapping of the UpdateMarker struct
It's not a file that a user is supposed to touch...
OTOH I don't know if we ever formalized such format anywhere and if we want to keep it forward/backward compatible by putting in place some rules about what is required/optional

blakerouse

Looks good.

blakerouse · 2023-05-11T02:08:56Z

internal/pkg/agent/cmd/run.go

+	// If the installation marker file is present, we're all set.
+	if info.RunningInstalled() {
+		return nil
+	}


I think adding a comment saying that only installed Elastic Agent's can be self-upgraded so if this returns true then we know that the marker is present as info.RunningInstalled() checks for the marker to be present.

Done in 4a420fa.

…grade

pchila · 2023-05-11T09:05:41Z

internal/pkg/agent/cmd/run.go

+	// Otherwise, we're being upgraded from a version of an installed Agent
+	// that didn't use an installation marker file (that is, before v8.8.0).
+	// So create the file now.
+	if err := info.CreateInstallMarker(paths.Top()); err != nil {


This has a bit of overlap with the upgrade migrations I am trying to implement in #2597: I can see creating the install marker as part of an upgrade migration then the PrevVersion (yes I have to rely on the update marker to know which version we are coming from) is < 8.8.0

It's not something to do now but we could think of moving this part into its own migration when available

Yes! While writing this I was thinking how it parallels DB migrations so, indeed, there's some overlap here. ++ to incorporating this and #2597 into a more structured migrations process if that makes sense.

pchila

LGTM

* Create installation marker on upgrade from versions < 8.8.0 * Removing debugging lines * Mention re-exec for additional clarity * Adding changelog entry * Removing scratch statement * Re-ordering imports * Error check * Do not depend on parsing pre-upgrade version from upgrade marker file * Clarify comment about only installed Agent being eligible for self-upgrade (cherry picked from commit 9df6514)

…2668) * Create installation marker on upgrade from versions < 8.8.0 * Removing debugging lines * Mention re-exec for additional clarity * Adding changelog entry * Removing scratch statement * Re-ordering imports * Error check * Do not depend on parsing pre-upgrade version from upgrade marker file * Clarify comment about only installed Agent being eligible for self-upgrade (cherry picked from commit 9df6514) Co-authored-by: Shaunak Kashyap <[email protected]>

Create installation marker on upgrade from versions < 8.8.0

007414f

mergify bot assigned ycombinator May 10, 2023

mergify bot added the backport-skip label May 10, 2023

ycombinator added 2 commits May 10, 2023 15:20

Removing debugging lines

d984c62

Mention re-exec for additional clarity

e67efdf

ycombinator added backport-v8.8.0 Automated backport with mergify and removed backport-skip labels May 10, 2023

ycombinator added the QA:Needs Validation Needs validation by the QA Team label May 10, 2023

ycombinator added 4 commits May 10, 2023 15:52

Adding changelog entry

c41af33

Removing scratch statement

c3690b8

Re-ordering imports

0c22c18

Error check

9c30e2c

cmacknz reviewed May 10, 2023

View reviewed changes

Do not depend on parsing pre-upgrade version from upgrade marker file

4848bb2

ycombinator marked this pull request as ready for review May 10, 2023 20:19

ycombinator requested a review from a team as a code owner May 10, 2023 20:19

ycombinator requested review from AndersonQ, pchila and cmacknz May 10, 2023 20:19

ycombinator added the Team:Elastic-Agent Label for the Agent team label May 10, 2023

ycombinator enabled auto-merge (squash) May 10, 2023 22:29

ycombinator disabled auto-merge May 10, 2023 22:29

blakerouse previously approved these changes May 11, 2023

View reviewed changes

Clarify comment about only installed Agent being eligible for self-up…

4a420fa

…grade

ycombinator dismissed blakerouse’s stale review via 4a420fa May 11, 2023 07:51

pchila reviewed May 11, 2023

View reviewed changes

pchila approved these changes May 11, 2023

View reviewed changes

ycombinator merged commit 9df6514 into elastic:main May 11, 2023

ycombinator deleted the upgrade-installation-marker branch May 11, 2023 12:59

mergify bot mentioned this pull request May 11, 2023

[8.8](backport #2655) Create installation marker on upgrade from versions < 8.8.0 #2668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create installation marker on upgrade from versions < 8.8.0 #2655

Create installation marker on upgrade from versions < 8.8.0 #2655

ycombinator commented May 10, 2023 •

edited

Loading

mergify bot commented May 10, 2023

elasticmachine commented May 10, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented May 10, 2023 •

edited

Loading

cmacknz May 10, 2023

cmacknz May 10, 2023

ycombinator May 10, 2023

cmacknz May 10, 2023

pchila May 11, 2023 •

edited

Loading

blakerouse left a comment

blakerouse May 11, 2023

ycombinator May 11, 2023

pchila May 11, 2023 •

edited

Loading

ycombinator May 11, 2023

pchila left a comment

Create installation marker on upgrade from versions < 8.8.0 #2655

Create installation marker on upgrade from versions < 8.8.0 #2655

Conversation

ycombinator commented May 10, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

mergify bot commented May 10, 2023

elasticmachine commented May 10, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented May 10, 2023 • edited Loading

🌐 Coverage report

cmacknz May 10, 2023

Choose a reason for hiding this comment

cmacknz May 10, 2023

Choose a reason for hiding this comment

ycombinator May 10, 2023

Choose a reason for hiding this comment

cmacknz May 10, 2023

Choose a reason for hiding this comment

pchila May 11, 2023 • edited Loading

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

blakerouse May 11, 2023

Choose a reason for hiding this comment

ycombinator May 11, 2023

Choose a reason for hiding this comment

pchila May 11, 2023 • edited Loading

Choose a reason for hiding this comment

ycombinator May 11, 2023

Choose a reason for hiding this comment

pchila left a comment

Choose a reason for hiding this comment

ycombinator commented May 10, 2023 •

edited

Loading

elasticmachine commented May 10, 2023 •

edited

Loading

elasticmachine commented May 10, 2023 •

edited

Loading

pchila May 11, 2023 •

edited

Loading

pchila May 11, 2023 •

edited

Loading