Ignore enroll cmd failing to restart daemon if running on a container #3631

AndersonQ · 2023-10-18T17:50:13Z

What does this PR do?

Makes the container command to ignore the enroll command not being able to restart the daemon. There is no daemon on a container, therefore nothing to be restarted.

Why is it important?

If the enroll command fails when the agent container is started for the 1st time to enroll on fleet-server, it'd fail and exit.

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

How to test this PR locally

Start a agent container to enroll on fleet, it should succeed

docker run \
  --env FLEET_ENROLL=1 \
  --env FLEET_URL=https://fleet-url:8220/ \
  --env FLEET_ENROLLMENT_TOKEN=SOME_TOKEN \
  --env FLEET_INSERUCE=true \
  docker.elastic.co/beats/elastic-agent:8.12.0-SNAPSHOT

Related issues

Closes Elastic Agent enroll fails to restart daemon on docker #3628

Logs

See #3628

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 · 2023-10-18T17:50:16Z

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

elastic-sonarqube · 2023-10-18T18:02:36Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

72.7% Coverage
0.0% Duplication

elasticmachine · 2023-10-18T18:15:43Z

💔 Build Failed

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-19T15:03:42.778+0000
Duration: 6 min 20 sec

Steps errors

Expand to view the steps failures

`check`

Took 1 min 7 sec . View more details here
Description: make check-ci

❕ Flaky test report

No test was executed to be analysed.

🤖 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-10-18T18:15:52Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.81% (`83/84`)	👍
Files	67.105% (`204/304`)	👍
Classes	65.78% (`371/564`)	👍 0.061
Methods	53.107% (`1171/2205`)	👍 0.064
Lines	39.337% (`13640/34675`)	👎 -0.025
Conditionals	100.0% (`0/0`)	💚

pierrehilbert · 2023-10-18T19:33:42Z

@AndersonQ Shouldn't we have a changelog as this is a bug?

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

pchila

I would like to see the ErrDaemonReload.Is implementation cleaned up before this code is merged.

pchila · 2023-10-19T10:02:52Z

internal/pkg/agent/cmd/container.go

@@ -318,6 +318,10 @@ func runContainerCmd(streams *cli.IOStreams, cfg setupConfig) error {
 		}
 		err = enroll.Wait()
 		if err != nil {
+			if errors.Is(err, &ErrDaemonReload{}) {


If you are using errors.Is it usually means that you want to check a specific error value (which is usually a global value in some package): for example

errors.Is(err, fs.ErrNotExist)

where the error value is defined like this

I see that you forced the implementation of errors.Is on the error type using errors.As below and this is a bit of a smell (you are changing the semantics of errors.Is).

The way I see we have 2 options here:

Define a global error value instead of a type for ErrDaemonReload , wrap it as the inner error along with the real error and keep using errors.Is

Keep the new type ErrDaemonReload, remove the Is custom implementation from the type and do all the checks using errors.As explicitly as you are asserting the type not the value

pchila · 2023-10-19T10:04:53Z

internal/pkg/agent/cmd/enroll_cmd.go

+func (e *ErrDaemonReload) Is(err error) bool {
+	var errDaemonReload *ErrDaemonReload
+	return errors.As(err, &errDaemonReload)
+}


See comment above: Is should compare the error values, not types

pchila · 2023-10-19T10:06:16Z

internal/pkg/agent/cmd/enroll_cmd_test.go

+	err := fmt.Errorf("some wrapping: %w",
+		&ErrDaemonReload{err: errors.New("some error")})
+
+	assert.True(t, errors.Is(err, &ErrDaemonReload{}))


see comments above

mergify · 2023-10-24T00:23:44Z

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

AndersonQ · 2024-09-03T11:59:24Z

got stale

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

AndersonQ self-assigned this Oct 18, 2023

AndersonQ requested a review from a team as a code owner October 18, 2023 17:50

AndersonQ requested review from michalpristas and pchila October 18, 2023 17:50

AndersonQ requested a review from cmacknz October 18, 2023 17:59

AndersonQ added 2 commits October 19, 2023 06:35

ignore enroll cmd failing to restart daemon if running on a container

bbac783

Revert "Revert "install fails if enroll fails (elastic#3554)" (elasti…

d8b9f6d

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

pchila requested changes Oct 19, 2023

View reviewed changes

wip

8a20ac8

AndersonQ force-pushed the 3628-docker-enroll branch from 7ab23e6 to 8a20ac8 Compare October 19, 2023 15:03

AndersonQ marked this pull request as draft October 19, 2023 15:04

cmacknz mentioned this pull request Oct 26, 2023

Enrolling into Fleet can fail because the control socket doesn't exist #3664

Closed

AndersonQ closed this Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore enroll cmd failing to restart daemon if running on a container #3631

Ignore enroll cmd failing to restart daemon if running on a container #3631

AndersonQ commented Oct 18, 2023 •

edited

Loading

elasticmachine commented Oct 18, 2023

elastic-sonarqube bot commented Oct 18, 2023

elasticmachine commented Oct 18, 2023 •

edited

Loading

Build stats

`check`

elasticmachine commented Oct 18, 2023

pierrehilbert commented Oct 18, 2023

pchila left a comment

pchila Oct 19, 2023

pchila Oct 19, 2023

pchila Oct 19, 2023

mergify bot commented Oct 24, 2023

AndersonQ commented Sep 3, 2024

Ignore enroll cmd failing to restart daemon if running on a container #3631

Ignore enroll cmd failing to restart daemon if running on a container #3631

Conversation

AndersonQ commented Oct 18, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Logs

Questions to ask yourself

elasticmachine commented Oct 18, 2023

elastic-sonarqube bot commented Oct 18, 2023

elasticmachine commented Oct 18, 2023 • edited Loading

💔 Build Failed

Build stats

Steps errors

check

❕ Flaky test report

🤖 GitHub comments

elasticmachine commented Oct 18, 2023

🌐 Coverage report

pierrehilbert commented Oct 18, 2023

pchila left a comment

Choose a reason for hiding this comment

pchila Oct 19, 2023

Choose a reason for hiding this comment

pchila Oct 19, 2023

Choose a reason for hiding this comment

pchila Oct 19, 2023

Choose a reason for hiding this comment

mergify bot commented Oct 24, 2023

AndersonQ commented Sep 3, 2024

AndersonQ commented Oct 18, 2023 •

edited

Loading

elasticmachine commented Oct 18, 2023 •

edited

Loading

`check`