-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 missing metrics on cgroups v2 #3007
Conversation
Hi @mhermida. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @kolyshkin @giuseppe |
container/common/helpers.go
Outdated
@@ -195,7 +195,8 @@ func getSpecInternal(cgroupPaths map[string]string, machineInfoFactory info.Mach | |||
} | |||
|
|||
// Processes, read it's value from pids path directly | |||
pidsRoot, ok := cgroupPaths["pids"] | |||
//pidsRoot, ok := cgroupPaths["pids"] |
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.
remove this line
LGTM on the change overall, I tested out cadvisor on cgroupv2 image without this change and added some debug logging of the
so as a result, none of the subcontrollers were found. So I think change here is appropriate, on cgroupv2 we will only have a single cgroupsPath with key |
Overall SGTM, although I don't like the complication of having to add I'm on PTO, let me take a closer look later this week. |
Hi @bobbypage, @kolyshkin
yes, I don't like it much either, but the only way of avoiding it I found was to call |
the |
Thanks @giuseppe!! |
I am fine with the current version. Deferring to @bobbypage and @kolyshkin :) |
ack, I'm happy with change here too, we can always come back and refactor the cgroupv2 check. Thank you for the fix. |
/ok-to-test |
I see two approaches. The first one would be to check whether you have v1 or v2 right inside the test, and only use test cases for the available version. This relies on the fact that CI is executed in both cgroup v1 and cgroup v2 environments (which AFAIK is not entirely true at the moment, but this can and should be fixed). The upside is it's trivial to implement, the downside you need different test environments (which, to my mind, is OK). The alternative, somethat more complicated, is to add something like // isV2 can be overridden by some unit tests.
var isV2 = cgroups.IsCgroup2UnifiedMode to the code, and use the new Now, you can override it on a per-test basis, something like (note this is entirely untested): func TestThis(t *testing.T) {
isV2Saved := isV2
t.Cleanup(func() {isV2 = isV2Saved})
// Tests for cgroup v1.
isV2 = func() bool {return false}
....
// Tests for cgroup v2.
isV2 = func() bool {return true}
.... The downsides are (1) this brings in some complexity into the test code and (2) you can'd execute such tests in parallel (if you want to, this calls for more complexity). Overall, I'd choose the first solution, but I'm OK with the code as it is. |
Filed #3009. Should be easy to implement. |
Path lookups fail in the cgroups v2 cgroupPaths map (common/helpers.go, getSpecInternal) that only has key "" and some metrics are missing (cpu cfs related ones, for instance).