Log when `container-paths.yml` is loaded #5462

belimawr · 2024-09-06T16:58:53Z

What does this PR do?

See title

Why is it important?

It adds logs when the Elastic-Agent loads container-paths.yml

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

Disruptive User Impact

None

How to test this PR locally

Export the following environmet variables:

FLEET_ENROLL
FLEET_URL
FLEET_ENROLLMENT_TOKEN

Run the container command as root

./elastic-agent container

Stop the Elastic-Agent and run the status command:

./elastic-agent status

You should see the logs:

2024/09/06 16:54:33 container path file '/usr/share/elastic-agent/state/container-paths.yml' found
2024/09/06 16:54:33 state Path: '/usr/share/elastic-agent/state', config path: '/usr/share/elastic-agent/state', logs path: '', socket path: 'unix:///usr/share/elastic-agent/state/data/Td8I7R-Zby36_zF_IOd9QVNlFblNEro3.sock'

~~## Related issues~~

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 · 2024-09-06T16:58:55Z

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

mergify · 2024-09-06T16:59:39Z

This pull request does not have a backport label. Could you fix it @belimawr? 🙏
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.

cmacknz · 2024-09-06T18:06:51Z

internal/pkg/agent/cmd/container.go

@@ -903,6 +905,8 @@ func tryContainerLoadPaths() error {
 	if err != nil {
 		return fmt.Errorf("failed to unpack %s: %w", pathFile, err)
 	}
+	log.Printf("state Path: '%s', config path: '%s', logs path: '%s', socket path: '%s'", paths.StatePath, paths.ConfigPath, paths.LogsPath, paths.SocketPath)


Shouldn't agent be logging this regardless of if it is in a container? Also isn't this in diagnostics, why do we need to log it (from the perspective that every log statement we add adds to the size and cost of the remote monitoring indices)?

This PR was triggered by my work on fixing a flaky test (#5159), I don't recall if the diagnostics were being successfully collected, but I believe they were not because the failing tests do not install the Elastic-Agent, they enrol and run the agent, hence the need to have it in the logs.

This is actually logged in any run of the agent where tryContainerLoadPaths sets the paths. On a correct execution it should be only inside a container.

Thinking more broadly, I agree it makes sense to log it every time the agent runs. Should I update the PR for it?

I diagnostics we get the following in variables.yaml which omits the state path which is container specific, so let's just leave the scope of this one to here.

path: config: C:\Program Files\Elastic\Agent data: C:\Program Files\Elastic\Agent\data home: C:\Program Files\Elastic\Agent\data\elastic-agent-8.14.1-1348b9 logs: C:\Program Files\Elastic\Agent

I'm fine with this log.

Actually, it seems like instead of both log.Printf("container path file '%s' found", pathFile) and log.Printf("state Path: '%s', config path: '%s', logs path: '%s', socket path: '%s'", paths.StatePath, paths.ConfigPath, paths.LogsPath, paths.SocketPath) we should have a single log line just unconditionally logging the state path we are using.

Reading #5361 it also seems like the state path is the problem.

We should log the state path right after this block above:

statePath := envWithDefault("", "STATE_PATH") if statePath == "" { statePath = defaultStateDirectory }

The STATE_PATH mentioned on #5361 was what made that test leave state behind, which made a new execution/installation of the Elastic-Agent not to behave as it should have, even the --force flag does not overwrite the container-paths.yml. That's is, in my opinion, one of the issues, hence this PR to be explicit about which paths the Elastic-Agent is using.

Reading the code and thinking more about it, the data in container-paths.yml can overwrite the state path set by STATE_PATH, so I believe we still need both logs, one indicating which file we read and another indicating the final paths we're using after reading it. I'd keep both log lines.

mergify · 2024-09-10T21:09:24Z

backport-v8.x has been added to help with the transition to the new branch 8.x.

elasticmachine · 2024-09-10T22:27:03Z

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

mergify · 2024-09-11T11:49:21Z

backport-v8.x has been added to help with the transition to the new branch 8.x.

belimawr · 2024-09-13T20:40:18Z

I cannot reproduce those CI failures, I'll just re-run/update the branch

belimawr · 2024-09-13T20:41:40Z

buildkite test this

belimawr · 2024-09-13T21:36:02Z

The tests are failing due to issues creating a 9.0.0 stack.

elastic-sonarqube · 2024-10-15T21:16:03Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

andrzej-stencel

Ideally we should wait for @cmacknz to reply in the thread before merging, although I guess we shouldn't be waiting forever either.

cmacknz · 2024-10-17T20:35:47Z

I still don't love logging this unconditionally, instead of putting this diagnostics, or having it respect a log level, but it's not a hill I'm going to die on if we think it's useful.

I also think the true root cause that caused us to need this is our attempts to run the elastic-agent container command outside of an actual container, which is something we no longer need to do now that we have a way to run integration tests on k8s directly.

cmacknz · 2024-10-17T20:37:00Z

I am talking about

elastic-agent/testing/integration/container_cmd_test.go

Line 171 in 3b1f793

    
           cmd, agentOutput := prepareAgentCMD(t, ctx, agentFixture, []string{"container"}, env)

specifically that should probably be rewritten to actually run in a container, for reference.

belimawr · 2024-10-18T13:21:27Z

I also think the true root cause that caused us to need this is our attempts to run the elastic-agent container command outside of an actual container, which is something we no longer need to do now that we have a way to run integration tests on k8s directly.

That is true. The reason for this PR was really to help debugging tests or other weird situations we might fall into. The tests didn't have diagnostics because the agent wasn't installed, logs are the primary source of information on those cases.

I'm not opposed to closing this PR, I haven't had time to come back to it in a while :/.

Honestly, I love that Beats logs their paths right at start up and I miss it on Elastic-Agent.

belimawr · 2024-10-18T13:23:01Z

I am talking about

elastic-agent/testing/integration/container_cmd_test.go

Line 171 in 3b1f793

cmd, agentOutput := prepareAgentCMD(t, ctx, agentFixture, []string{"container"}, env)

specifically that should probably be rewritten to actually run in a container, for reference.

I agree, now that we have the infrastructure in place, that's definitely a better option.

belimawr added Team:Elastic-Agent Label for the Agent team skip-changelog labels Sep 6, 2024

belimawr requested a review from a team as a code owner September 6, 2024 16:58

belimawr requested review from andrzej-stencel and swiatekm September 6, 2024 16:58

mergify bot assigned belimawr Sep 6, 2024

mergify bot added the backport-skip label Sep 6, 2024

cmacknz reviewed Sep 6, 2024

View reviewed changes

mergify bot added the backport-v8.x label Sep 10, 2024

ycombinator added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Sep 10, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 11, 2024

v1v removed the backport-v8.x label Sep 11, 2024

belimawr force-pushed the log-go-be-container branch from 231b841 to 1a30c6b Compare September 13, 2024 18:59

swiatekm approved these changes Sep 17, 2024

View reviewed changes

ycombinator force-pushed the log-go-be-container branch from 1a30c6b to c97567f Compare September 20, 2024 00:42

belimawr force-pushed the log-go-be-container branch from 6e963b0 to 77afa82 Compare September 27, 2024 20:52

ycombinator force-pushed the log-go-be-container branch 3 times, most recently from e8493ac to 974e961 Compare October 2, 2024 18:21

Log when container-paths.yml is loaded

28bd404

ycombinator force-pushed the log-go-be-container branch from 1a8b0b5 to 28bd404 Compare October 15, 2024 20:22

andrzej-stencel approved these changes Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log when `container-paths.yml` is loaded #5462

Log when `container-paths.yml` is loaded #5462

belimawr commented Sep 6, 2024

elasticmachine commented Sep 6, 2024

mergify bot commented Sep 6, 2024

cmacknz Sep 6, 2024

belimawr Sep 9, 2024

cmacknz Sep 9, 2024

cmacknz Sep 9, 2024

cmacknz Sep 9, 2024

belimawr Sep 10, 2024

belimawr Sep 10, 2024

mergify bot commented Sep 10, 2024

elasticmachine commented Sep 10, 2024

mergify bot commented Sep 11, 2024

belimawr commented Sep 13, 2024

belimawr commented Sep 13, 2024

belimawr commented Sep 13, 2024

elastic-sonarqube bot commented Oct 15, 2024

andrzej-stencel left a comment

cmacknz commented Oct 17, 2024

cmacknz commented Oct 17, 2024

belimawr commented Oct 18, 2024 •

edited

Loading

belimawr commented Oct 18, 2024

Log when container-paths.yml is loaded #5462

Are you sure you want to change the base?

Log when container-paths.yml is loaded #5462

Conversation

belimawr commented Sep 6, 2024

What does this PR do?

Why is it important?

Checklist

Disruptive User Impact

How to test this PR locally

Questions to ask yourself

elasticmachine commented Sep 6, 2024

mergify bot commented Sep 6, 2024

cmacknz Sep 6, 2024

Choose a reason for hiding this comment

belimawr Sep 9, 2024

Choose a reason for hiding this comment

cmacknz Sep 9, 2024

Choose a reason for hiding this comment

cmacknz Sep 9, 2024

Choose a reason for hiding this comment

cmacknz Sep 9, 2024

Choose a reason for hiding this comment

belimawr Sep 10, 2024

Choose a reason for hiding this comment

belimawr Sep 10, 2024

Choose a reason for hiding this comment

mergify bot commented Sep 10, 2024

elasticmachine commented Sep 10, 2024

mergify bot commented Sep 11, 2024

belimawr commented Sep 13, 2024

belimawr commented Sep 13, 2024

belimawr commented Sep 13, 2024

elastic-sonarqube bot commented Oct 15, 2024

Quality Gate passed

andrzej-stencel left a comment

Choose a reason for hiding this comment

cmacknz commented Oct 17, 2024

cmacknz commented Oct 17, 2024

belimawr commented Oct 18, 2024 • edited Loading

belimawr commented Oct 18, 2024

Log when `container-paths.yml` is loaded #5462

Log when `container-paths.yml` is loaded #5462

belimawr commented Oct 18, 2024 •

edited

Loading