-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add cgroup v2 support for container id detector #3508
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3508 +/- ##
=====================================
Coverage 78.0% 78.1%
=====================================
Files 164 166 +2
Lines 11815 11845 +30
=====================================
+ Hits 9226 9252 +26
- Misses 2393 2395 +2
- Partials 196 198 +2
|
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this doesn't work with podman. I tested it also with minikube docker, and found that I didn't get the proper containerID from V1, but the correct one from V2.
I made a gist of what I saw from /proc/self/cgroup and /proc/self/mointinfo https://gist.github.com/MadVikingGod/6ecba436717948c1a332c63f250f973e
|
||
const cgroupV2Path = "/proc/self/mountinfo" | ||
|
||
var cgroupV2ContainerIDRe = regexp.MustCompile(`.*/docker/containers/([0-9a-f]{64})/.*`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very docker specific. I found with podman there is a different format for the container layers, and I am also checking containerd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same is used in Java here (https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/ca8e7b1385d95517f28103d834aba960a0baa477/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/CgroupV2ContainerIdExtractor.java#L27), so it would need to be fixed in both.
JavaScript is taking a different approach, I remember that we talked about containerd there and making it work:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @breedx-splk , @abhee11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svrnm Thanks for the heads-up. Looks like you pasted the same link twice, but it reads like the second one intended to link to a js discussion? Curious what the alt approach is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@breedx-splk thanks for calling out the wrong link, here's the JS one: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/f0a93685cfb43543b7ca577dd370d56576b49e3f/detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts#L54
|
||
const cgroupV1Path = "/proc/self/cgroup" | ||
|
||
var cgroupV1ContainerIDRe = regexp.MustCompile(`^.*/(?:.*-)?([0-9a-f]+)(?:\.|\s*$)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is still the right way to capture this for V1.
When used with podman I'm capturing the userID of the pod, not a container or an empty string.
This is based on a conversation in [opentelemetry-go #3508](open-telemetry/opentelemetry-go#3508) and to be more consistent with [the js cgroupv2 parser impl](https://github.com/open-telemetry/opentelemetry-js-contrib/blob/f0a93685cfb43543b7ca577dd370d56576b49e3f/detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts#L68). Unsurprisingly, podman does not include the word `docker` in the `mountinfo` file. As a result, the container id parsing would fail from inside a podman container. This fixes that up to be more compatible.
…y-go into container-id-cgroup-v2
I found a strange case where my
In this case, @MadVikingGod What was your environment to get those |
I started minikube with a different
From what I've seen I don't know if there is a reliable way to get the containerID from within the context of the running container. It does seem like docker might do it this way, but that doesn't look like it's guaranteed between versions, and different runtimes do things differently. |
A concrete example of reproducing the incorrect container id in the cgroup v2 file in k8s EnvironmentOS: macOS 14.4.1 (ARM) Steps
Result:
Result
The id on the
Result
Result
SummaryThe container id |
Resolves #3501
The new container id detector tries to resolve the container id from the cgroup v1 file, if successful, the result is used. If v1 fails, then the detector falls back to the cgroup v2 file.