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.

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

[Elastic-Agent] Do not use unversioned home with watcher binary path #25423

Merged

Conversation

michalpristas
Copy link
Contributor

@michalpristas michalpristas commented Apr 29, 2021

What does this PR do?

The problem is when agent tries to invoke watcher which checks upgraded agent for failures and at the same time unversioned home path is set.

Usually agent took its home path which is {top_path}/data/elastic-agent-{hash}/elastic-agent and invoked watch sub-command on binary at that path.

When unversioned path is set it tries to run watcher from {top_path}/data/elastic-agent and as so it results in binary not found.

Fixes: #25371

Why is it important?

Reduce noise created by failng attempt to run this. This is just a case when running container sub-command which tempers paths.

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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

cc @michel-laterman

@michalpristas michalpristas added bug needs_backport PR is waiting to be backported to other branches. Team:Ingest Management v7.13.0 Team:Elastic-Agent Label for the Agent team labels Apr 29, 2021
@michalpristas michalpristas self-assigned this Apr 29, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Apr 29, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 29, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #25423 updated

  • Start Time: 2021-04-30T06:34:59.676+0000

  • Duration: 86 min 20 sec

  • Commit: 8a044fc

Test stats 🧪

Test Results
Failed 0
Passed 6781
Skipped 23
Total 6804

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 6781
Skipped 23
Total 6804

@michalpristas
Copy link
Contributor Author

@blakerouse should i do this or should we move binary/create symlink in the setPath func

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

@michalpristas Can you reference the issue this is coming from?

@@ -200,8 +200,8 @@ func (p *noopPidProvider) Name() string { return "noop" }

func (p *noopPidProvider) PID(ctx context.Context) (int, error) { return 0, nil }

func invokeCmd() *exec.Cmd {
homeExePath := filepath.Join(paths.Home(), agentName)
func invokeCmd(topPath string) *exec.Cmd {
Copy link
Member

Choose a reason for hiding this comment

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

I saw the name topPath in the past? What does it exactly mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the structure we have topPath is the top of agent directory structure e.g. /etc/Elastic/Agent
we also use a homePath which points to a home of a certain agent version e.g /etc/Elastic/Agent/data/elastic-agent-abc123/

@ruflin
Copy link
Member

ruflin commented Apr 29, 2021

As a follow up, we should start to think on how we can test these things in an automated way.

@blakerouse
Copy link
Contributor

The only time unversioned path is used is in the container. Self-upgrade is not supported in the container anyway, so could we just disable the watcher completely instead?

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

This change does seem okay and should still go in.

But related to my previous comment, when will the watcher run in a container?

@michalpristas
Copy link
Contributor Author

@blakerouse it will not, the only case when it's running is when specifically told to using DEV flag [allowUpgrade part]

@michalpristas michalpristas changed the title [Elastic-Agent] do not use unversioned home with watcher binary path [Elastic-Agent] Do not use unversioned home with watcher binary path Apr 30, 2021
@michalpristas michalpristas merged commit 7e902cc into elastic:master Apr 30, 2021
michalpristas added a commit to michalpristas/beats that referenced this pull request Apr 30, 2021
michalpristas added a commit to michalpristas/beats that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs_backport PR is waiting to be backported to other branches. Team:Elastic-Agent Label for the Agent team v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[elastic-agent] failed to invoke rollback waterfork
4 participants