Always launch more recent watcher #4491

pchila · 2024-03-27T15:22:40Z

What does this PR do?

This PR changes the way elastic-agent launches the upgrade watcher in the final phases of the upgrade process.
Before this change, the elastic-agent binary from the incoming version (the one specified in the upgrade command) was always used but this meant that during a downgrade an older version of elastic agent would be launched, potentially not supporting new fields in the upgrade marker generated during the upgrade.

Now the agent will check the current and incoming version and always select the newer one to launch the watcher process: during a downgrade this means that the current agent binary will function as a watcher to check that the incoming version functions correctly after the restart.

Why is it important?

We want to avoid issues during watch/cleanup/rollback phases of upgrades even if the incoming version is older than the current install.

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

Related issues

Closes Agent should always launch the more recent watcher version when downgrading #4072

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 · 2024-03-27T15:23:20Z

This pull request does not have a backport label. Could you fix it @pchila? 🙏
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 · 2024-04-10T08:26:12Z

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

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

cmacknz · 2024-04-12T19:46:06Z

pkg/version/version_parser.go

-	switch major {
-	case 8:
-		return NewParsedSemVer(7, 17, 10, psv.Prerelease(), psv.BuildMetadata()), nil
+	for i := 0; i < minPrereleaseTokens; i++ {


What problem is this solving? Do we need this code to handle something in our current or planned release versioning scheme or is it just in case?

This is the correct implementation for comparing 2 semver version strings. Prerelease strings should be compared token by token as strings or numbers depending on their format.

Right now we only use "SNAPSHOT" prerelease string and the previous impl would just compare 2 versions based on the presence of prerelease string without comparing those, leading to versions like
1.2.3-SNAPSHOT , 1.2.3-alpha, 1.2.3-SNAPHOT.extraprereleasetoken not to have an definite order.

This new implementation is able to parse the tokens and correctly determine precedence based on the prerelease token: an example when this is gonna come into play is of we decide to use repackaged agents in upgrade tests as 1.2.3-SNAPSHOT.repackaged now has a defined order when compared with 1.2.3-SNAPSHOT

This is a part of a general overhaul of agent version handling correct versioning both at packaging and runtime which I then extracted to put in a separate PR as it would make this change too big.

Edit: this is the other PR's commit containing the rest of changes 863a638

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

michalpristas · 2024-04-16T09:48:25Z

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

+func selectWatcherExecutable(topDir string, previous agentInstall, current agentInstall) string {
+	// check if the upgraded version is less than the previous (currently installed) version
+	if current.parsedVersion.Less(*previous.parsedVersion) {
+		// use the current agent executable for watch, if downgrading the old agent doesn't understand the current agent's path structure.


comment says use the current as currently installed but it's not current as a variable name. it gets confusing

Agreed but this is the existing names that follow where the symlink points.
So after rotating the symlink the currently running agent is named previous and the new one is named current.
I don't know if this is more confusing or swapping the variable names so when we look at it in different places in the code it gets confusing...

can we maybe rephrase
use the current agent executable for watch, if downgrading the old agent doesn't understand the current agent's path structure.
to
if downgrading the target agent may not understand the current agent's path structure.

blakerouse

Really don't like the hard-coding of 8.13.0.

changelog/fragments/1712737337-always-launch-more-recent-watcher.yaml

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

pchila · 2024-04-16T14:37:26Z

I don't really like that this is hard-coded. Will this need to be updated when we create a new release? Why not use version.Agent?

@blakerouse
we need to be sure we have a parseable version for the agent that is orchestrating the upgrade. We store that as a string in the package.version file and today it can be any string. I agree it's a pretty slim chance that we have a wrongly formatted version string but we have to handle the possibility. The alternative here is to perform a rollback of the already installed agent or figure out some kind of fallback behaviour (which where the 8.13.0 comes in because it's the version where the new update marker has been introduced).

If you want I can change that fallback to always run the newly installed agent watcher (dangerous for downgrade) or stop the upgrade process at this point and roll back everything...

…for comparison

pchila · 2024-04-16T16:52:08Z

Removed getParsedVersionWithFallback function in favor of reading the current parsed version at startup from the package.version file

Co-authored-by: Blake Rouse <[email protected]>

blakerouse

Thanks for the fixes.

Like this implement of the pressed version string much better.

pchila · 2024-04-17T16:28:18Z

buildkite test this

elastic-sonarqube · 2024-04-17T16:58:21Z

Quality Gate passed

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

1 New issue
0 Security Hotspots
84.0% 84.0% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

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 labels Mar 27, 2024

pchila self-assigned this Mar 27, 2024

mergify bot added the backport-skip label Mar 27, 2024

pchila force-pushed the always-launch-more-recent-watcher branch 8 times, most recently from 7de5860 to b7bfbb2 Compare April 10, 2024 08:24

pchila marked this pull request as ready for review April 10, 2024 08:26

pchila requested a review from a team as a code owner April 10, 2024 08:26

pchila requested review from michalpristas and blakerouse April 10, 2024 08:26

cmacknz reviewed Apr 12, 2024

View reviewed changes

pchila mentioned this pull request Apr 15, 2024

Generalize package version #4573

Closed

7 tasks

michalpristas approved these changes Apr 16, 2024

View reviewed changes

blakerouse requested changes Apr 16, 2024

View reviewed changes

pchila added 5 commits April 16, 2024 18:50

remove GetPreviousVersion()

360fe61

implement correct prerelease comparison of parsed versions

8d3eb06

WIP select newer watcher

1ad2ae6

changelog

fee3422

Create a parsed semver when reading agent package version and use it …

c3821e7

…for comparison

pchila force-pushed the always-launch-more-recent-watcher branch from ece5816 to c3821e7 Compare April 16, 2024 16:50

Apply suggestions from code review

3fbd7ce

Co-authored-by: Blake Rouse <[email protected]>

blakerouse approved these changes Apr 16, 2024

View reviewed changes

fix linter errors

dc999ec

pchila enabled auto-merge (squash) April 17, 2024 19:09

pchila merged commit 761f4b4 into elastic:main Apr 17, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always launch more recent watcher #4491

Always launch more recent watcher #4491

pchila commented Mar 27, 2024 •

edited

Loading

mergify bot commented Mar 27, 2024

elasticmachine commented Apr 10, 2024

cmacknz Apr 12, 2024

pchila Apr 15, 2024 •

edited

Loading

michalpristas Apr 16, 2024

pchila Apr 16, 2024

michalpristas Apr 17, 2024 •

edited

Loading

blakerouse left a comment

pchila commented Apr 16, 2024 •

edited

Loading

pchila commented Apr 16, 2024

blakerouse left a comment

pchila commented Apr 17, 2024

elastic-sonarqube bot commented Apr 17, 2024

Always launch more recent watcher #4491

Always launch more recent watcher #4491

Conversation

pchila commented Mar 27, 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

mergify bot commented Mar 27, 2024

elasticmachine commented Apr 10, 2024

cmacknz Apr 12, 2024

Choose a reason for hiding this comment

pchila Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

michalpristas Apr 16, 2024

Choose a reason for hiding this comment

pchila Apr 16, 2024

Choose a reason for hiding this comment

michalpristas Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

pchila commented Apr 16, 2024 • edited Loading

pchila commented Apr 16, 2024

blakerouse left a comment

Choose a reason for hiding this comment

pchila commented Apr 17, 2024

elastic-sonarqube bot commented Apr 17, 2024

Quality Gate passed

pchila commented Mar 27, 2024 •

edited

Loading

pchila Apr 15, 2024 •

edited

Loading

michalpristas Apr 17, 2024 •

edited

Loading

pchila commented Apr 16, 2024 •

edited

Loading