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

[Ingest Manager] Logging to file disabled on enroll #24466

Merged
merged 3 commits into from
Mar 10, 2021

Conversation

michalpristas
Copy link
Contributor

@michalpristas michalpristas commented Mar 10, 2021

What does this PR do?

The problem here is that enroll runs simultaneously with running service and both try to log into same file.
Although for enroll there's no need to do that as it is invoked either manually (with std output) or as executed from install command with std redirecting to a caller.

This PR makes sure to create internal logging file elastic-agent-json.log only in case it is really needed.

Why is it important?

Fixes: #24173

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.

@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 Mar 10, 2021
@elasticmachine
Copy link
Collaborator

💚 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 #24466 opened

  • Start Time: 2021-03-10T10:15:50.517+0000

  • Duration: 104 min 38 sec

  • Commit: 9edef27

Test stats 🧪

Test Results
Failed 0
Passed 45840
Skipped 5097
Total 50937

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 45840
Skipped 5097
Total 50937

// enroll is invoked either manually or from install with redirected IO
// no need to log to file
cfg.Settings.LoggingConfig.ToFiles = false
cfg.Settings.LoggingConfig.ToStderr = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Does this need to be set on any other subcommands?

Copy link
Contributor Author

@michalpristas michalpristas Mar 10, 2021

Choose a reason for hiding this comment

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

no we use just install, uninstall, upgrade and reexec (on win) where we don't use logger
enroll where we use it
watcher has its own configuration
and run which is a normal state

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the response.

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.

Looks good.

@michalpristas michalpristas merged commit 9dc5dd8 into elastic:master Mar 10, 2021
michalpristas added a commit to michalpristas/beats that referenced this pull request Mar 10, 2021
[Ingest Manager] Logging to file disabled on enroll (elastic#24466)
michalpristas added a commit to michalpristas/beats that referenced this pull request Mar 10, 2021
[Ingest Manager] Logging to file disabled on enroll (elastic#24466)
michalpristas added a commit that referenced this pull request Mar 11, 2021
[Ingest Manager] Logging to file disabled on enroll (#24466)
michalpristas added a commit that referenced this pull request Mar 15, 2021
 Cherry-pick #24466 to 7.12: Logging to file disabled on enroll  (#24474)
v1v added a commit to v1v/beats that referenced this pull request Mar 16, 2021
…reporting-changes

* upstream/master: (23 commits)
  [Auditbeat] btmp offset check (elastic#24515)
  Clarify that the Tomcat module is for ingesting access logs (elastic#24543)
  [Ingest Manager] Move logging defaults to agent (elastic#24535)
  Update input-http-endpoint.asciidoc (elastic#24490)
  Fix typo in mqtt input docs (elastic#24509)
  [Elastic Agent] Add verification check when updating communication to Kibana. (elastic#24489)
  Refactor use of system.hostfs to fix cgroup metrics (elastic#24334)
  Add test for close.reader.after_interval to filestream input (elastic#24423)
  chore(ci): use beat_version instead of PR version (elastic#24446)
  Add syntax for multiple selector logging (elastic#24207) (elastic#24497)
  Update Golang to 1.15.9 (elastic#24442)
  [Elastic Agent] Add the ability to provide custom CA's inside of docker. (elastic#24486)
  Add tests for encoding settings of filestream input (elastic#24426)
  [Ingest Manager] Sync on rename on windows (elastic#24504)
  Port four Harvester tests of log input to filestream in Golang (elastic#24250)
  [DOCS] Restructure content for SSL settings (elastic#24342)
  Move example to the correct location in reference docs (elastic#24455)
  Add unit tests for harvester.go of input-logfile package (elastic#24107)
  Fix type for uwsgi.status.worker.rss field (elastic#24468)
  [Ingest Manager] Logging to file disabled on enroll (elastic#24466)
  ...
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"failed to rotate backups" error after running agent enroll command.
3 participants