Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log when container-paths.yml is loaded #5462

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+4 −0
Open

Log when container-paths.yml is loaded #5462

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions internal/pkg/agent/cmd/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"io"
"io/fs"
"log"
"net/url"
"os"
"os/exec"
Expand Down Expand Up @@ -894,6 +895,7 @@ func tryContainerLoadPaths() error {
// no container-paths.yml file exists, so nothing to do
return nil
}
log.Printf("container path file '%s' found", pathFile)
cfg, err := config.LoadFile(pathFile)
if err != nil {
return fmt.Errorf("failed to load %s: %w", pathFile, err)
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return setPaths(paths.StatePath, paths.ConfigPath, paths.LogsPath, paths.SocketPath, false)
}

Expand Down