install fails if enroll fails and surface errors #3554

AndersonQ · 2023-10-06T10:03:56Z

For the agent to be actually enrolled it needs to restart after the enroll process is completed, so it'll pickup the new config and "connect" to fleet-server.

This change makes the enroll command to fail if it cannot restart the agent after enrolling on fleet

What does this PR do?

This change makes the enroll command to fail if it cannot restart the agent after enrolling on fleet

Why is it important?

For the agent to be actually enrolled it needs to restart after the enroll process is completed, so it'll pickup the new config and "connect" to fleet-server.

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~~

Related issues

N/A

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

* surface errors that might occur during enroll * fail install command if agent cannot be restarted * do not print success message if there was an enroll error. Print an error message and the error instead * add logs to show the different enroll attempts * add more context t errors * refactor internal/pkg/agent/install/perms_unix.go and add more context to errors restore main version * ignore agent restart error on enroll tests as there is no agent to be restarted * daemonReloadWithBackoff does not retry on context deadline exceeded

mergify · 2023-10-06T10:04:30Z

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 fail-enroll upstream/fail-enroll
git merge upstream/main
git push upstream fail-enroll

mergify · 2023-10-06T10:04:30Z

This pull request does not have a backport label. Could you fix it @AndersonQ? 🙏
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-10-06T16:45:43Z

💚 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-10-16T14:03:07.013+0000
Duration: 26 min 58 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6493
Skipped	59
Total	6552

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

AndersonQ · 2023-10-09T12:34:04Z

/test

elasticmachine · 2023-10-09T13:01:56Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.78% (`81/82`)	👍
Files	67.559% (`202/299`)	👍
Classes	66.187% (`368/556`)	👍
Methods	53.585% (`1166/2176`)	👍 0.046
Lines	39.769% (`13586/34162`)	👍 0.059
Conditionals	100.0% (`0/0`)	💚

elasticmachine · 2023-10-11T12:02:50Z

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

internal/pkg/agent/cmd/enroll_cmd_test.go

internal/pkg/agent/cmd/enroll_cmd.go

internal/pkg/agent/install/perms_unix.go

internal/pkg/agent/cmd/enroll_cmd_test.go

pchila

Aside from the usual backoff awkwardness (not the point of the PR) there are just a couple of small nitpicks.
I agree with @ycombinator that, since we squash commits it would be better to separate small typo corrections or refactors in a dedicated PR

internal/pkg/agent/cmd/enroll_cmd.go

pchila · 2023-10-12T13:23:44Z

internal/pkg/agent/cmd/enroll_cmd_test.go

+			if err != nil &&
+				// There is no agent running, therefore nothing to be restarted.
+				// However, this will cause the Enroll command to return an error
+				// which we'll ignore here.
+				!strings.Contains(err.Error(),
+					"could not reload agent daemon, unable to trigger restart") {
+				t.Fatalf("enrrol coms returned and unexpected error: %v", err)
+			}


From the description it seems that we expect the error about not being able to trigger a restart, we can simplify with:

Suggested change

if err != nil &&

// There is no agent running, therefore nothing to be restarted.

// However, this will cause the Enroll command to return an error

// which we'll ignore here.

!strings.Contains(err.Error(),

"could not reload agent daemon, unable to trigger restart") {

t.Fatalf("enrrol coms returned and unexpected error: %v", err)

}

// There is no agent running, therefore nothing to be restarted.

// However, this will cause the Enroll command to return an error

// which we'll ignore here.

require.ErrorContainsf(t, err, "could not reload agent daemon, unable to trigger restart", "enroll command returned an unexpected error: %v", err)

elastic-sonarqube · 2023-10-16T14:15:08Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

54.0% Coverage
0.0% Duplication

* fix install/enroll cmd not failing when agent restart fails * surface errors that might occur during enroll * fail install command if agent cannot be restarted * do not print success message if there was an enroll error. Print an error message and the error instead * add logs to show the different enroll attempts * add more context t errors * refactor internal/pkg/agent/install/perms_unix.go and add more context to errors restore main version * ignore agent restart error on enroll tests as there is no agent to be restarted * daemonReloadWithBackoff does not retry on context deadline exceeded and context cancelled * fix typos (cherry picked from commit f7e558f)

* fix install/enroll cmd not failing when agent restart fails * surface errors that might occur during enroll * fail install command if agent cannot be restarted * do not print success message if there was an enroll error. Print an error message and the error instead * add logs to show the different enroll attempts * add more context t errors * refactor internal/pkg/agent/install/perms_unix.go and add more context to errors restore main version * ignore agent restart error on enroll tests as there is no agent to be restarted * daemonReloadWithBackoff does not retry on context deadline exceeded and context cancelled * fix typos (cherry picked from commit f7e558f) Co-authored-by: Anderson Queiroz <[email protected]>

This reverts commit f7e558f.

This reverts commit 3d608f6.

This reverts commit f7e558f.

…c#3629)" This reverts commit 1fd44fe.

AndersonQ added 2 commits October 6, 2023 08:39

do not return error if err is nil

b991a7f

AndersonQ self-assigned this Oct 6, 2023

mergify bot added the backport-skip label Oct 6, 2023

AndersonQ added bug Something isn't working Team:Elastic-Agent Label for the Agent team backport-v8.11.0 Automated backport with mergify and removed backport-skip labels Oct 6, 2023

AndersonQ added 2 commits October 6, 2023 18:20

fix daemonReloadWithBackoff

1edccff

fix typos

1dd6216

AndersonQ marked this pull request as ready for review October 11, 2023 12:02

AndersonQ requested a review from a team as a code owner October 11, 2023 12:02

AndersonQ requested review from ycombinator and pchila October 11, 2023 12:02