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] Disable modules.d in metricbeat #27860

Merged
merged 2 commits into from
Sep 20, 2021

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Sep 10, 2021

Currently when Elastic Agent is started, on post-install of metricbeat the system.yml file in the modules.d directoy is renamed to system.yml.disabled to make sure it is not run. It turns out there might be some edge cases where this does not work. To not rely on renaming of files, instead the modules are fully disabled.

Closes #27857

Currently when Elastic Agent is started, on post-install of metricbeat the system.yml file in the modules.d directoy is renamed to system.yml.disabled to make sure it is not run. It turns out there might be some edge cases where this does not work. To not rely on renaming of files, instead the modules are fully disabled.

Closes elastic#27857
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 10, 2021
"-E", "management.enabled=true",
"-E", "logging.level=debug",
"-E", "gc_percent=${METRICBEAT_GOGC:100}",
"-E", "metricbeat.config.modules.enabled=false"
Copy link
Member Author

Choose a reason for hiding this comment

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

@michalpristas When I initially started this change I was looking for a rule add_config or similar that the only thing this would od is adding this field to the config. Instead it seems all these configs we pass in as cmd line flags. What is the reason for this?

The concerns I have with it is that when I use inspect, not sure if the above ones show up and at one point, we might hit some limits on the length of params.

Can you maybe share a bit of background on why we did it this way?

Copy link
Contributor

Choose a reason for hiding this comment

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

we did this initially to enable management and later on to set debug level nothing else. all the other options here are just using the stuff we introduced initially to have management enabled.
add_config_option sound good but we need to make it clear what to do in case of naming conflict. something like we have with copy rule where we specify overwrite/merge/noop

Copy link
Member Author

Choose a reason for hiding this comment

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

Managemement and logging can also be set as config options, or does management have to be passed in as environment variable?

@ruflin ruflin self-assigned this Sep 10, 2021
@ruflin ruflin added the Team:Elastic-Agent Label for the Agent team label Sep 10, 2021
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 10, 2021
@michalpristas
Copy link
Contributor

michalpristas commented Sep 10, 2021

nice. i was trying to repro on Windows but i did not hit the case. this seems like a better approach than renaming

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 10, 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 preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-10T12:14:54.059+0000

  • Duration: 76 min 2 sec

  • Commit: c1beeb9

Test stats 🧪

Test Results
Failed 0
Passed 7044
Skipped 16
Total 7060

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 7044
Skipped 16
Total 7060

@ruflin
Copy link
Member Author

ruflin commented Sep 10, 2021

Manual testing seems to confirm that this change works but I would like to add some test for it too first.

@ruflin ruflin added backport-v7.15.0 Automated backport with mergify backport-v7.16.0 Automated backport with mergify labels Sep 10, 2021
@ruflin
Copy link
Member Author

ruflin commented Sep 10, 2021

To simplify the testing, I opened #27862. But it will unfortunately not cover this case yet as it uses a cmd line flag. That could be a follow up PR to introduce cmd line flags and with the golden files in place any change would be directly visible.

@ruflin
Copy link
Member Author

ruflin commented Sep 13, 2021

@michalpristas Did not get to setup a Windows envrionment yet to test this manually. If you have one running, would be great if you could test it quickly.

@michalpristas
Copy link
Contributor

i can test it

@michalpristas
Copy link
Contributor

michalpristas commented Sep 13, 2021

looks ok, i dont see any errors
tested on win10

@ruflin ruflin marked this pull request as ready for review September 16, 2021 06:35
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@ruflin ruflin merged commit 41f17ab into elastic:master Sep 20, 2021
@ruflin ruflin deleted the metricbeat-module-fix branch September 20, 2021 06:15
mergify bot pushed a commit that referenced this pull request Sep 20, 2021
Currently when Elastic Agent is started, on post-install of metricbeat the system.yml file in the modules.d directoy is renamed to system.yml.disabled to make sure it is not run. It turns out there might be some edge cases where this does not work. To not rely on renaming of files, instead the modules are fully disabled.

Closes #27857

(cherry picked from commit 41f17ab)
mergify bot pushed a commit that referenced this pull request Sep 20, 2021
Currently when Elastic Agent is started, on post-install of metricbeat the system.yml file in the modules.d directoy is renamed to system.yml.disabled to make sure it is not run. It turns out there might be some edge cases where this does not work. To not rely on renaming of files, instead the modules are fully disabled.

Closes #27857

(cherry picked from commit 41f17ab)

# Conflicts:
#	x-pack/elastic-agent/pkg/agent/program/supported.go
ruflin added a commit that referenced this pull request Sep 20, 2021
Currently when Elastic Agent is started, on post-install of metricbeat the system.yml file in the modules.d directoy is renamed to system.yml.disabled to make sure it is not run. It turns out there might be some edge cases where this does not work. To not rely on renaming of files, instead the modules are fully disabled.

Closes #27857

(cherry picked from commit 41f17ab)

Co-authored-by: Nicolas Ruflin <spam@ruflin.com>
v1v added a commit to v1v/beats that referenced this pull request Sep 20, 2021
* upstream/master: (658 commits)
  Add complete k8s metadata through composable provider (elastic#27691)
  Revert "Fix issue where --insecure didn't propogate to Fleet Server ES connection (elastic#27969)" (elastic#27997)
  Remove deprecated kafka fields (elastic#27938)
  [Filebeat] Add Base64 encoded HMAC & UUID template functions to httpjson input (elastic#27873)
  Improve httpjson template function join (elastic#27996)
  Remove kubernetes.container.image alias (elastic#27898)
  [Elastic Agent] Golden files for program tests (elastic#27862)
  [Elastic Agent] Disable modules.d in metricbeat (elastic#27860)
  libbeat/common/seccomp: provide default policy for linux arm64 (elastic#27955)
  Fix logger statement in aws-s3 input (elastic#27982)
  Fix wrong merge (elastic#27976)
  Fix issue where --insecure didn't propogate to Fleet Server ES connection (elastic#27969)
  Forward-port 7.14.2 changelog to master (elastic#27975)
  [Filebeat] Removing duplicate modules (aliases) Observability (elastic#27919)
  Fix path in vagrant windows script (elastic#27966)
  [Filebeat] Removing duplicate modules (aliases) and Cyberark (elastic#27915)
  No changelog for 8.0.0-alpha2 (elastic#27961)
  Add write access to 'url.value' from 'request.transforms'. (elastic#27937)
  Docker: remove deprecated fields (elastic#27933)
  Filebeat: Make all filesets disabled in default configuration (elastic#27762)
  ...
Icedroid pushed a commit to Icedroid/beats that referenced this pull request Nov 1, 2021
Currently when Elastic Agent is started, on post-install of metricbeat the system.yml file in the modules.d directoy is renamed to system.yml.disabled to make sure it is not run. It turns out there might be some edge cases where this does not work. To not rely on renaming of files, instead the modules are fully disabled.

Closes elastic#27857
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.15.0 Automated backport with mergify backport-v7.16.0 Automated backport with mergify Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Elastic Agent] system.yml is not disabled in Metricbeat
4 participants