Fix missing metrics on cgroups v2

[mhermida]

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).

[k8s-ci-robot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[bobbypage]

/cc @kolyshkin @giuseppe

[bobbypage]

remove this line

[bobbypage]

LGTM on the change overall, I tested out cadvisor on cgroupv2 image without this change and added some debug logging of the cgroupsPath and they looked like this:

cgroupsPath:map[:/sys/fs/cgroup/system.slice/systemd-update-utmp.service]
cgroupsPath:map[:/sys/fs/cgroup/system.slice/containerd.service]
cgroupsPath:map[:/sys/fs/cgroup/system.slice/containerd.service]
cgroupsPath:map[:/sys/fs/cgroup/system.slice/docker.socket]

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 "" and from there we can get the individual controller files.

[kolyshkin]

Overall SGTM, although I don't like the complication of having to add cgroup2UnifiedMode to every call. Perhaps this can be dropped, and getControllerPath should determine whether we use v2 all by itself (the call is very cheap as we cache it).

I'm on PTO, let me take a closer look later this week.

[mhermida]

Hi @bobbypage, @kolyshkin
thanks for your reviews and comments.

...although I don't like the complication of having to add cgroup2UnifiedMode to every call.

yes, I don't like it much either, but the only way of avoiding it I found was to call cgroups.IsCgroup2UnifiedMode() within getControllerPath and I wasn't very convinced about that one too, so whatever you think is best.

[giuseppe]

the cgroups.IsCgroup2UnifiedMode() result is memoized, so it should be fine to call it from getControllerPath()

[mhermida]

Thanks @giuseppe!!
The thing dropping the parameter cgroup2UnifiedMode from getControllerPath (which could also be dropped from getSpecInternal) and calling cgroups.IsCgroup2UnifiedMode() from the functions is that testing becomes a bit more complex as we need to adapt the behaviour of cgroups.IsCgroup2UnifiedMode() to cgroups v1 or cgroups v2 depending on which case are the tests for. Not sure how to do that...

[giuseppe]

I am fine with the current version. Deferring to @bobbypage and @kolyshkin :)

[bobbypage]

ack, I'm happy with change here too, we can always come back and refactor the cgroupv2 check. Thank you for the fix.

[bobbypage]

/ok-to-test

[kolyshkin]

The thing dropping the parameter cgroup2UnifiedMode from getControllerPath (which could also be dropped from getSpecInternal) and calling cgroups.IsCgroup2UnifiedMode() from the functions is that testing becomes a bit more complex as we need to adapt the behaviour of cgroups.IsCgroup2UnifiedMode() to cgroups v1 or cgroups v2 depending on which case are the tests for. Not sure how to do that...

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 isV2() everywhere you need it.

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.

[kolyshkin]

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).

Filed #3009. Should be easy to implement.