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

Handle cgroups v2 in OsProbe #76883

Merged
merged 5 commits into from
Sep 1, 2021
Merged

Conversation

pugnascotia
Copy link
Contributor

Closes #76812.

OsProbe was only capable of handle cgroup data in the v1 format.
However, Debian 11 uses cgroups v2 by default, and Elasticsearch isn't
capable of reporting any cgroup information. Therefore, add support for
the v2 layout.

Closes elastic#76812.

`OsProbe` was only capable of handle cgroup data in the v1 format.
However, Debian 11 uses cgroups v2 by default, and Elasticsearch isn't
capable of reporting any cgroup information. Therefore, add support for
the v2 layout.
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Aug 24, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

👍 I assume there's no need to add explicit unit test coverage as we're getting coverage simply by running this on a system using cgroups v2 (Debian 11)?

@pugnascotia
Copy link
Contributor Author

@mark-vieira I added testing for v2.

@pugnascotia
Copy link
Contributor Author

@rjernst I'd appreciate you view on this PR. I'm particularly concerned that the security manager policy changes aren't accurate or sufficient, because the wildcard matching isn't sophisticated enough. For example, if /proc/self/cgroup has:

0::/user.slice/user-1173.slice/session-76.scope

then the code needs to access:

/sys/fs/cgroup/user.slice/user-1173.slice/session-76.scope/cpu.stat

The best that the policy file could manage is:

permission java.io.FilePermission "/sys/fs/cgroup/-", "read";

...which would work, but is significantly more open than what we currently allow.

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

List<String> lines = readProcSelfCgroup();

// cgroup v2
if (lines.size() == 1 && lines.get(0).startsWith("0::")) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine since here we are not actually parsing the line, but elsewhere in this file we do a bare split on the control group line, which doesn’t allow for colons in the underlying path. Looks like an existing issue in this code, but thought I would mention. To ensure we protect against it (could be a followup). See https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8272124

@pugnascotia pugnascotia merged commit e827ec6 into elastic:master Sep 1, 2021
@pugnascotia pugnascotia deleted the cgroups-v2-take2 branch September 1, 2021 08:29
pugnascotia added a commit that referenced this pull request Sep 1, 2021
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Sep 1, 2021
pugnascotia added a commit that referenced this pull request Sep 1, 2021
Re-apply #76883. Somehow a line was missed from security.policy.
pugnascotia added a commit that referenced this pull request Sep 1, 2021
Closes #76812.

`OsProbe` was only capable of handle cgroup data in the v1 format.
However, Debian 11 uses cgroups v2 by default, and Elasticsearch isn't
capable of reporting any cgroup information. Therefore, add support for
the v2 layout.
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in eff6cd6.

pugnascotia added a commit that referenced this pull request Sep 1, 2021
pugnascotia added a commit that referenced this pull request Sep 1, 2021
@pugnascotia
Copy link
Contributor Author

This had to be reverted in the end since the policy changes didn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement >non-issue Team:Core/Infra Meta label for core/infra team v7.16.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgroups stats not available on Debian 11
5 participants