[Upgrade Watcher] Try restarting Agent in multiple ways during rollback #3268

ycombinator · 2023-08-18T01:44:12Z

What does this PR do?

This PR fixes a potential bug where, during an upgrade, if the upgraded Agent process crashes, the Upgrade Watcher initiates a rollback but isn't able to restart the rolled back Agent.

Before this PR, the Upgrade Watcher would attempt to restart the rolled back Agent via a GRPC call to the Agent's Restart() function. However, if the Agent process has crashed, there is no GRPC server available to receive such a call, thus preventing the Agent from being restarted.

With this PR, the Upgrade Watcher continues to use the GRPC Restart() call to restart the Agent. However, if that call fails, the Upgrade Watcher uses a fallback method of asking the Elastic Agent service to restart the Agent.

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~~ This bug was found while writing the integration test in [Integration Test] New Agent after upgrade fails to start #3085. As such, this bug fix will be tested by that test.

How to test this PR locally

Build an Agent package from main (currently at version 8.11.0), unpack it, and install it. This will serve as the version of Agent we will upgrade from.
```
SNAPSHOT=true EXTERNAL=true DEV=true PLATFORMS=linux/arm64 PACKAGES=tar.gz mage package
```
Build an Agent package from the branch for this PR (say at version 8.12.0). This will serve as the version of Agent we will upgrade to. This Agent will deliberately crash upon start up but it also has the bugfix in the Upgrade Watcher code path.
```
DEV=true PLATFORMS=linux/arm64 PACKAGES=tar.gz mage package
```

Upgrade to the 8.12.0 Agent.

sudo elastic-agent upgrade 8.11.0 --source-uri file://$(pwd)/build/distributions/ --skip-verify

Ensure that the Agent version is now 8.12.0. Because the Agent will be crashing, you may only see the binary version, not the daemon version.
```
sudo elastic-agent version
```

Tail the Upgrade Watcher logs. Ensure that the Upgrade Watcher eventually initiates rollback.

sudo tail -F /opt/Elastic/Agent/data/elastic-agent-06be89/logs/elastic-agent-watcher-$(date +%Y%m%d).ndjson

Ensure that the rolled back Agent gets restarted.
Ensure that the Agent version is now back to 8.11.0.
```
sudo elastic-agent version
```

Related issues

Fixes bug explained in [Upgrade Watcher][Crash Checker] Consider Agent process as crashed if its PID remains 0 #3166 (comment).

elasticmachine · 2023-08-18T01:44:15Z

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

ycombinator · 2023-08-18T01:45:55Z

internal/pkg/agent/cmd/run.go

+	// FIXME: for testing only!
+	time.Sleep(10 * time.Second)
+	return errors.New("deliberately crashing agent (after invoking watcher)")
+


Will be removed once this PR is tested and approved.

Removed in 2af8c30.

ycombinator · 2023-08-18T01:46:33Z

version/version.go

@@ -4,5 +4,5 @@

 package version

-const defaultBeatVersion = "8.11.0"
+const defaultBeatVersion = "8.12.0"


Will be removed once this PR is tested and approved.

Removed in 2af8c30.

elasticmachine · 2023-08-18T01:48:28Z

💚 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-08-28T18:37:50.894+0000
Duration: 28 min 55 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6217
Skipped	47
Total	6264

💚 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!)

ycombinator · 2023-08-18T01:48:46Z

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

+		install.StopService(topPath)
+		err := install.StartService(topPath)


Would it better to create a new install.RestartServive(topPath) wrapper around these two calls?

Yes I like that idea. Logging the error for stop service might be helpful as well, just for debugging issues in the future, having the contents of that error in the log could be useful.

Added install.RestartService in 6cb1746. I'm taking care of adding logging to the restart step in #3245.

michalpristas

lgtm, restarting by service should work in rollback cases without a need for check if service is there as for now we don't support upgrade scenario without agent being installed

blakerouse

Change looks good to me.

blakerouse · 2023-08-21T14:33:02Z

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

+		install.StopService(topPath)
+		err := install.StartService(topPath)


Yes I like that idea. Logging the error for stop service might be helpful as well, just for debugging issues in the future, having the contents of that error in the log could be useful.

elasticmachine · 2023-08-21T17:26:36Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.765% (`80/81`)	👍
Files	67.368% (`192/285`)	👍
Classes	66.105% (`353/534`)	👍
Methods	52.896% (`1105/2089`)	👍
Lines	38.53% (`12605/32715`)	👎 -0.024
Conditionals	100.0% (`0/0`)	💚

cmacknz · 2023-08-22T00:48:14Z

internal/pkg/agent/install/install.go

+func RestartService(topPath string) error {
+	err := StopService(topPath)
+	if err != nil {
+		return err
+	}
+
+	return StartService(topPath)
+}


Since we are dealing with unlikely problems already, if the agent were to stop for whatever reason between the Stop and Start calls it would be left stopped.

The most likely reason this could happen is a laptop system being powered off.

It would be much safer to use systemctl restart or equivalent if it exists because it should prevent this situation. The service manager knows you want the services end state to be running in this case, which is not true with an isolated call to stop.

Good point. Fixed in d81fe70.

cmacknz

Thanks!

mergify · 2023-08-28T17:05:48Z

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 bugfix-rollback-restart-agent-no-process upstream/bugfix-rollback-restart-agent-no-process
git merge upstream/main
git push upstream bugfix-rollback-restart-agent-no-process

pierrehilbert · 2023-08-29T06:03:43Z

Unrelated CI failures. Force merge this one as it will bring some additional stability to our upgrade process.

…ck (#3268) * Try restarting via the service as a backup * [Testing] Crashing agent * Adding CHANGELOG fragment * [Testing] Bump up version * Adding install.RestartService * Removing testing code * Use service manager's restart * Updating comment (cherry picked from commit 9377dca)

…ck (#3268) (#3311) * Try restarting via the service as a backup * [Testing] Crashing agent * Adding CHANGELOG fragment * [Testing] Bump up version * Adding install.RestartService * Removing testing code * Use service manager's restart * Updating comment (cherry picked from commit 9377dca) Co-authored-by: Shaunak Kashyap <[email protected]>

ycombinator added bug Something isn't working Team:Elastic-Agent Label for the Agent team labels Aug 18, 2023

ycombinator requested a review from a team as a code owner August 18, 2023 01:44

ycombinator requested review from michalpristas and pchila August 18, 2023 01:44

ycombinator added the backport-skip label Aug 18, 2023

mergify bot assigned ycombinator Aug 18, 2023

ycombinator commented Aug 18, 2023

View reviewed changes

ycombinator force-pushed the bugfix-rollback-restart-agent-no-process branch from 7b20499 to 06be899 Compare August 18, 2023 01:46

ycombinator commented Aug 18, 2023

View reviewed changes

pierrehilbert requested a review from blakerouse August 21, 2023 12:19

michalpristas approved these changes Aug 21, 2023

View reviewed changes

blakerouse approved these changes Aug 21, 2023

View reviewed changes

ycombinator enabled auto-merge (squash) August 21, 2023 17:00

ycombinator mentioned this pull request Aug 21, 2023

[Upgrade Watcher][Crash Checker] Consider Agent process as crashed if its PID remains 0 #3166

Merged

7 tasks

cmacknz reviewed Aug 22, 2023

View reviewed changes

ycombinator force-pushed the bugfix-rollback-restart-agent-no-process branch from 2af8c30 to f5560d0 Compare August 22, 2023 01:28

ycombinator requested a review from cmacknz August 22, 2023 01:30

cmacknz approved these changes Aug 22, 2023

View reviewed changes

ycombinator force-pushed the bugfix-rollback-restart-agent-no-process branch from 45f327a to 8917a6e Compare August 24, 2023 21:12

pchila approved these changes Aug 25, 2023

View reviewed changes

ycombinator force-pushed the bugfix-rollback-restart-agent-no-process branch from 8917a6e to 14ea553 Compare August 25, 2023 14:30

ycombinator added 3 commits August 28, 2023 11:37

Try restarting via the service as a backup

6e37315

[Testing] Crashing agent

f10fbff

Adding CHANGELOG fragment

cfbd11a

ycombinator added 5 commits August 28, 2023 11:37

[Testing] Bump up version

c2d41f2

Adding install.RestartService

06e005e

Removing testing code

628cdc4

Use service manager's restart

3e8a845

Updating comment

44bd7ec

ycombinator force-pushed the bugfix-rollback-restart-agent-no-process branch from 14ea553 to 44bd7ec Compare August 28, 2023 18:37

ycombinator added backport-v8.9.0 Automated backport with mergify backport-v8.10.0 Automated backport with mergify and removed backport-skip backport-v8.9.0 Automated backport with mergify labels Aug 28, 2023

pierrehilbert disabled auto-merge August 29, 2023 06:03

pierrehilbert merged commit 9377dca into elastic:main Aug 29, 2023
16 checks passed

mergify bot mentioned this pull request Aug 29, 2023

[8.10](backport #3268) [Upgrade Watcher] Try restarting Agent in multiple ways during rollback #3311

Merged

ycombinator deleted the bugfix-rollback-restart-agent-no-process branch August 29, 2023 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Upgrade Watcher] Try restarting Agent in multiple ways during rollback #3268

[Upgrade Watcher] Try restarting Agent in multiple ways during rollback #3268

ycombinator commented Aug 18, 2023 •

edited

Loading

elasticmachine commented Aug 18, 2023

ycombinator Aug 18, 2023

ycombinator Aug 21, 2023

ycombinator Aug 18, 2023

ycombinator Aug 21, 2023

elasticmachine commented Aug 18, 2023 •

edited

Loading

Build stats

Test stats 🧪

ycombinator Aug 18, 2023

blakerouse Aug 21, 2023

ycombinator Aug 21, 2023

michalpristas left a comment

blakerouse left a comment

blakerouse Aug 21, 2023

elasticmachine commented Aug 21, 2023 •

edited

Loading

cmacknz Aug 22, 2023

ycombinator Aug 22, 2023

cmacknz left a comment

mergify bot commented Aug 28, 2023

pierrehilbert commented Aug 29, 2023

		install.StopService(topPath)
		err := install.StartService(topPath)

[Upgrade Watcher] Try restarting Agent in multiple ways during rollback #3268

[Upgrade Watcher] Try restarting Agent in multiple ways during rollback #3268

Conversation

ycombinator commented Aug 18, 2023 • edited Loading

What does this PR do?

Checklist

How to test this PR locally

Related issues

elasticmachine commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 18, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalpristas left a comment

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 21, 2023 • edited Loading

🌐 Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmacknz left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 28, 2023

pierrehilbert commented Aug 29, 2023

ycombinator commented Aug 18, 2023 •

edited

Loading

elasticmachine commented Aug 18, 2023 •

edited

Loading

elasticmachine commented Aug 21, 2023 •

edited

Loading