Add support for CgroupsV2 in beats, migrate away from gosigar #27242

fearful-symmetry · 2021-08-04T22:30:42Z

What does this PR do?

This PR accomplishes a few different things:

Adds support for cgroupsV2
Continues our deprecation of elastic/gosigar, by moving the cgroups code into libbeat. A considerable portion of the code here is pre-existing code from gosigar that's been reorganized.
Moves the opt library from metricbeat/internal/metrics to libbeat, since we use it here too.
Refactors the existing cgroups V1 code to be more in line with the refactor of the system module, removing the MapStr manipulation code in favor of having the data format hard-coded into the structs, and makes the cgroups code overall more "metrics first"

Note that supporting cgroups V1 and V2 involves some compromises, as the two versions structure themselves and report data fairly differently, and as such, most of the metrics APIs require the consumer to differentiate between V1 and V2 if they want to access raw metrics. V1 and V2 can also cooexist on the same system, so this must happen on a pid-by-pid basis.

Also, I'm still testing this on V1 and mixed v1/v2 systems, but the code is otherwise ready for review.

There's also the issue of dashboards. The system Docker dashboard relies on many of the V1 fields that aren't present in V2, and I'm not really sure how to deal with them. Last I tried, we don't really have any mechanism for dashboards to operate with the logic of "use this field if present, otherwise use this other field." We may also just want to re-write the dashboard entirely to just use fields that are present in V1 and V2, at the expense of losing some of the visualizations.

Also keep in mind that most of system/process is going to be aggressively refactored after this, so any issues, unless they're serious, with the code in libbeat/metric/system/process and metricbeat/system/process will almost certainly be fixed as part of 7.16.

What's up with cgroupsV2

Cgroups V2 introduces a few major changes compared to cgroups V1, which necessitated a lot of extra code:

A new hierarchy for processes. This "unified" hierarchy requires new logic for code that wants to track processes in a cgroup
New controllers. The various resource controllers (cpuacct, memory, etc) have changed dramatically, and in a few cases have merged with other controllers for V2. This requires entirely new metrics code.

File structure

For ease of browsing through this PR (Github honestly isn't great at presenting large PRs), here's the breakdown of what's in here:

libbeat/metric/system/cgroup // The entirety of the cgroup implementation. The files in this base directory are (mostly) unchanged files from gosigar.
├── cgcommon // Files that are shared by cgv1 and cgv2. Mostly struct definitions and helper code
├── cgv1 // Metrics  for cgroups v1. This is almost entirely code from elastic/gosigar that's been refactored.
├── cgv2 // metrics for cgroups v2. This is entirely new code.
└── testdata // data used by by the various *_test.go files

This also includes:

libbeat/opt Which is mostly existing code that was moved from metricbeat/internal.
Changes to cmd/instance, add_process_metadata and add_docker_metadata to migrate away from gosigar
changes to metricbeat/internal to deal with moving the libbeat/opt code.

Why is it important?

Cgroups V2 is coming, and is already default on Fedora. It's supported by most other software at this point.

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.

How to test this PR locally

Pull down and build
Note what cgroups version your host is using. You can see with grep cgroup /proc/self/mountinfo
Test the system/process metricset, as well as add_process_metadata and add_docker_metadata

elasticmachine · 2021-08-04T22:30:44Z

Pinging @elastic/integrations (Team:Integrations)

elasticmachine · 2021-08-04T22:38:12Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-08-11T20:07:48.440+0000
Duration: 141 min 16 sec
Commit: eeda969

Test stats 🧪

Test	Results
Failed	0
Passed	52571
Skipped	5206
Total	57777

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	52571
Skipped	5206
Total	57777

jsoriano

Looks quite good. I haven't reviewed the code moved from gosigar, if we want to make any refactor there I would leave it for a separate review. If there are relevant changes there please point to them 🙂

libbeat/cmd/instance/metrics/metrics.go

libbeat/metric/system/process/process.go

jsoriano · 2021-08-10T17:09:26Z

libbeat/metric/system/process/process.go

@@ -576,19 +531,22 @@ func (procStats *Stats) getSingleProcess(pid int, newProcs ProcsMap) *Process {

 	err = process.getDetails(procStats.isWhitelistedEnvVar)
 	if err != nil {
-		procStats.logger.Debugf("Error getting details for process %s with pid=%d: %v", process.Name, process.Pid, err)
+		procStats.logger.Errorf("Error getting details for process %s with pid=%d: %v", process.Name, process.Pid, err)


Maybe this was intentionally logged at the debug level? 😬 I can imagine this flooding logs in hosts with many short-lived processes.

Btw, this reminds me to #9111.

Yah, just decided to refactor most of this. Even if this library is going away in 7.16, it's still kind of a mess.

libbeat/metric/system/process/process.go

libbeat/processors/add_process_metadata/gosigar_cid_provider.go

metricbeat/module/system/process/_meta/data.json

metricbeat/module/system/process/_meta/fields.yml

…ffling CI errors

fearful-symmetry · 2021-08-11T04:37:06Z

@jsoriano thanks for the review! addressed most of the issues, and finally tracked down the bug that was breaking CI.

jsoriano

Minor comments added, nothing blocking, but there still seems to be related failures in CI.

libbeat/metric/system/process/process.go

libbeat/processors/add_process_metadata/gosigar_cid_provider.go

jsoriano · 2021-08-11T11:45:19Z

libbeat/metric/system/cgroup/util.go

@@ -202,6 +202,10 @@ func SubsystemMountpoints(rootfsMountpoint string, subsystems map[string]struct{

 		// V2 option
 		if mount.filesystemType == "cgroup2" {
+			if len(mount.mountpoint) < 3 {
+				// Why is this happening? I don't know.


😃

Where does it happen? How does mountinfo look in these cases?

Think you're looking at an older version, that's while I was debugging an issue with hybrid cgroups.

Yes, but I was curious about what appeared in mountinfo when this happens 🙂

Oh! The cgroups line just...isn't there.

libbeat/metric/system/cgroup/util.go

jsoriano · 2021-08-12T08:51:51Z

libbeat/processors/add_process_metadata/add_process_metadata_test.go

+	result, err := provider.GetCid(1)
+	assert.NoError(t, err)
+	assert.Equal(t, "2dcbab615aebfa9313feffc5cfdacd381543cfa04c6be3f39ac656e55ef34805", result)
+}


👍 thanks!

…c#27242) * finish first round of cgroupsv2 support * fix tests * remove old code, fix PctOpt * fix imports * go mod tidy * try to fix make notice again * try again * remove unneeded test files * fix test paths * fix crossbuild issues * fix tests, remove debug statement * change metadata, fight with mapping defs * fix v1 test, remove debug line * somewhat hacky fix for fields issues * fix v1 fetch, update fields again * fix omitempty issue * make update * remove older test * fix tests, cgv1 logic * remove old debug statement * fix issue with how ubuntu mixes cgroups * clean up error handling in libbeat * changes based on feedback, increased error verbosity to try to fix baffling CI errors * fix fields, add more error messages for weird CI bug * I give up, add tons of debug statements * fix issue with docker containers running under hybrid cgroups * fix hostfs state check * fix more broken tests * fix names, log levels * more changes, docs, test * still making the mapping checks happy * fix libbeat code I broke (cherry picked from commit d898533)

#27346) * finish first round of cgroupsv2 support * fix tests * remove old code, fix PctOpt * fix imports * go mod tidy * try to fix make notice again * try again * remove unneeded test files * fix test paths * fix crossbuild issues * fix tests, remove debug statement * change metadata, fight with mapping defs * fix v1 test, remove debug line * somewhat hacky fix for fields issues * fix v1 fetch, update fields again * fix omitempty issue * make update * remove older test * fix tests, cgv1 logic * remove old debug statement * fix issue with how ubuntu mixes cgroups * clean up error handling in libbeat * changes based on feedback, increased error verbosity to try to fix baffling CI errors * fix fields, add more error messages for weird CI bug * I give up, add tons of debug statements * fix issue with docker containers running under hybrid cgroups * fix hostfs state check * fix more broken tests * fix names, log levels * more changes, docs, test * still making the mapping checks happy * fix libbeat code I broke (cherry picked from commit d898533)

jsoriano · 2021-08-18T08:29:31Z

libbeat/metric/system/cgroup/util.go

+				logp.L().Infof(`PID %d contains a cgroups V2 path (%s) but no V2 mountpoint was found.
+This may be because metricbeat is running inside a container on a hybrid system.
+To monitor cgroups V2 processess in this way, mount the unified (V2) hierarchy inside
+the container as /sys/fs/cgroup/unified and start metricbeat with --system.hostfs.`, pid, line)


@fearful-symmetry in an elastic-agent started with elastic-package stack in my laptop, I see continuously (every collection period) this message, I think we should move it to the debug level. In a system where this happens this is going to be continuosly logged.

I use a quite vanilla Ubuntu 18.04.5 LTS, with Linux 4.15, and docker 20.10.7. So maybe this is not so an edge case.

Also, as far as I know, both my host and the docker containers are using cgroups V1 (I don't see any cgroups2 mountpoint or any unified cgroup) so I wonder if this comment about hybrid systems can be misleading in this case.

For reference, this is what I see in logs:

2021-08-18T08:15:09.797Z INFO cgroup/util.go:257 PID 10 contains a cgroups V2 path (0::/system.slice/containerd.service) but no V2 mountpoint was found. This may be because metricbeat is running inside a container on a hybrid system. To monitor cgroups V2 processess in this way, mount the unified (V2) hierarchy inside the container as /sys/fs/cgroup/unified and start metricbeat with --system.hostfs.

This is how /proc/10/cgroup looks like in the container:

12:freezer:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 11:devices:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 10:cpu,cpuacct:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 9:pids:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 8:hugetlb:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 7:blkio:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 6:perf_event:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 5:cpuset:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 4:memory:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 3:net_cls,net_prio:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 2:rdma:/ 1:name=systemd:/docker/b257ef6e9abdac56a8a07bf2e35abc7a064b0638e893bc54ab98d925bba13ecd 0::/system.slice/containerd.service

@jsoriano

0::/system.slice/containerd.service

So, that's the V2 controller there. You said you didn't see any unified mountpoint on the system? Can you get me /proc/self/mountinfo ?

However, I think you're right that we might want some kind of behavior to prevent this message from getting spammy. Unfortunately, from within a docker container it's really hard to get that info without the user mounting sysfs inside the container. Maybe just have it print the message at startup?

Oh, you are right, there is some cgroup2 around, I didn't see it.

$ cat /proc/self/mountinfo | grep cgroup2 34 33 0:29 / /sys/fs/cgroup/unified rw,nosuid,nodev,noexec,relatime shared:10 - cgroup2 cgroup rw,nsdelegate

So yeah, let's keep the message but trying to make it less verbose.

fearful-symmetry added 3 commits August 4, 2021 12:44

finish first round of cgroupsv2 support

f57a803

Merge remote-tracking branch 'upstream/master' into cgroups2

2a4de30

fix tests

e2db3b9

fearful-symmetry added enhancement Team:Integrations Label for the Integrations team v7.15.0 labels Aug 4, 2021

fearful-symmetry requested a review from a team August 4, 2021 22:30

fearful-symmetry self-assigned this Aug 4, 2021

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 Aug 4, 2021

remove old code, fix PctOpt

2485e87

fearful-symmetry added 17 commits August 4, 2021 20:30

fix imports

bbf1523

go mod tidy

8315ab8

Merge remote-tracking branch 'upstream/master' into cgroups2

6c58941

try to fix make notice again

4897385

try again

6e42fff

remove unneeded test files

0cb854c

fix test paths

963a65c

fix crossbuild issues

5c2c376

fix tests, remove debug statement

9c8bc8c

change metadata, fight with mapping defs

90cd5ec

fix v1 test, remove debug line

c9c2263

somewhat hacky fix for fields issues

1afde0e

fix v1 fetch, update fields again

ea4ed18

Merge remote-tracking branch 'upstream/master' into cgroups2

7cb31a1

fix omitempty issue

40ed2f0

make update

783877d

remove older test

b53184b

fix tests, cgv1 logic

9e6a9be

exekias mentioned this pull request Aug 10, 2021

[Metricbeat][System module] psi: pressure stall information #27223

Closed

jsoriano self-requested a review August 10, 2021 15:01

fearful-symmetry added 3 commits August 10, 2021 08:06

remove old debug statement

f72ed57

fix issue with how ubuntu mixes cgroups

5fdfb0a

clean up error handling in libbeat

10a5395

jsoriano reviewed Aug 10, 2021

View reviewed changes

fearful-symmetry added 6 commits August 10, 2021 12:14

changes based on feedback, increased error verbosity to try to fix ba…

ba7259a

…ffling CI errors

fix fields, add more error messages for weird CI bug

a092765

I give up, add tons of debug statements

a93271c

fix issue with docker containers running under hybrid cgroups

e1cf240

fix hostfs state check

2e9b94d

fix more broken tests

0f03ac8

fearful-symmetry requested review from jsoriano and a team August 11, 2021 04:36

fix names, log levels

038f397

jsoriano approved these changes Aug 11, 2021

View reviewed changes

fearful-symmetry added 3 commits August 11, 2021 10:51

more changes, docs, test

dd7a681

still making the mapping checks happy

b2ccbf1

fix libbeat code I broke

eeda969

fearful-symmetry requested a review from a team August 11, 2021 21:02

jsoriano approved these changes Aug 12, 2021

View reviewed changes

fearful-symmetry merged commit d898533 into elastic:master Aug 12, 2021

This was referenced Aug 12, 2021

Add changelog for cgroupsV2 #27344

Merged

Cherry-pick #27242 to 7.x: Add support for CgroupsV2 in beats, migrate away from gosigar #27346

Merged

jsoriano reviewed Aug 18, 2021

View reviewed changes

tetianakravchenko mentioned this pull request Sep 26, 2023

[system][process] Add metric_type metadata for object fields, set stack restriction to 8.9.0 version elastic/integrations#7660

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CgroupsV2 in beats, migrate away from gosigar #27242

Add support for CgroupsV2 in beats, migrate away from gosigar #27242

fearful-symmetry commented Aug 4, 2021 •

edited

Loading

elasticmachine commented Aug 4, 2021

elasticmachine commented Aug 4, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

jsoriano left a comment

jsoriano Aug 10, 2021

jsoriano Aug 10, 2021

fearful-symmetry Aug 10, 2021

fearful-symmetry commented Aug 11, 2021

jsoriano left a comment

jsoriano Aug 11, 2021

fearful-symmetry Aug 11, 2021

jsoriano Aug 11, 2021

fearful-symmetry Aug 11, 2021

jsoriano Aug 12, 2021

jsoriano Aug 18, 2021 •

edited

Loading

fearful-symmetry Aug 18, 2021

jsoriano Aug 19, 2021

Add support for CgroupsV2 in beats, migrate away from gosigar #27242

Add support for CgroupsV2 in beats, migrate away from gosigar #27242

Conversation

fearful-symmetry commented Aug 4, 2021 • edited Loading

What does this PR do?

What's up with cgroupsV2

File structure

Why is it important?

Checklist

How to test this PR locally

elasticmachine commented Aug 4, 2021

elasticmachine commented Aug 4, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fearful-symmetry commented Aug 11, 2021

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fearful-symmetry commented Aug 4, 2021 •

edited

Loading

elasticmachine commented Aug 4, 2021 •

edited by jenkins-beats-ci bot

Loading

jsoriano Aug 18, 2021 •

edited

Loading