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

Fix behavior of cgroups path discovery #136

Merged

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Mar 25, 2024

What does this PR do?

Fixes #132
and fixes #139

This sanitizes the "relative" namespace mounts that we get when we monitor the host system from within newer versions of docker.

This also adds much more complex logic for setting the v2 rootpath we use for fetching metrics from v2 cgroups paths.

This also aggressively cleans up the unit tests in cgroup because they were a mess

With this change, cgroups metric collection works properly on docker configs where where the container is using a private namespace. This value is configurable from docker 1.41+

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.md

How to test this PR

  • run go test ./tests from the repo root
  • build metricbeat with this version of elastic-agent-system-metrics, run system/process metrics.

@fearful-symmetry fearful-symmetry self-assigned this Mar 25, 2024
@fearful-symmetry fearful-symmetry marked this pull request as ready for review April 18, 2024 16:25
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner April 18, 2024 16:25
@fearful-symmetry fearful-symmetry requested review from ycombinator and leehinman and removed request for a team April 18, 2024 16:25
@fearful-symmetry
Copy link
Contributor Author

Decided to just fix #139 while I'm at it, so I can at least run the tests.

@fearful-symmetry fearful-symmetry changed the title Fix namespacing behavior of cgroups lookup Fix behavior of cgroups path discovery Apr 18, 2024
@fearful-symmetry
Copy link
Contributor Author

Okay cleaning up the unit tests is harder than I thought

@fearful-symmetry
Copy link
Contributor Author

Alright, the fix is still kind of broken for cases where we're trying to monitor a process on the host system that doesn't belong to a root cgroup. I think we need to use the host paths to find the actual cgroup on the host system, and then construct the bath inside hostfs from within the container.

@fearful-symmetry
Copy link
Contributor Author

So, tests are finally passing. One thing I'd like to add is some kind of manual bypass/setting for ContainerizedRootMount, as a sort of failsafe, in case there's some weird edge case I haven't been able to tease out.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Apr 28, 2024
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

Looks ok.

Can you add some examples to the PR description about what is now supported and any details on which versions of docker this applies to.

@fearful-symmetry fearful-symmetry merged commit 182b3ee into elastic:main May 1, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
3 participants