Fix read "max" value for cgroup v2 #2837

giuseppe · 2021-03-24T16:55:09Z

and also simplify some other code to behave like cgroup v1

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

read the memory stats from the specified cgroup instead of looking the values up in the ancestor cgroups. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

when the "max" value is found handle it as math.MaxUint64. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

k8s-ci-robot · 2021-03-24T16:55:18Z

Hi @giuseppe. 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.

giuseppe · 2021-03-24T16:55:24Z

@odinuge PTAL

odinuge · 2021-03-24T19:10:31Z

container/common/helpers.go

-				return spec, err
-			}
-			if memoryRoot != "" {
+			if utils.FileExists(path.Join(memoryRoot, "memory.max")) {


odinuge

Just a small nit, otherwise lgtm

odinuge · 2021-03-24T19:10:38Z

container/common/helpers.go

@@ -226,7 +203,10 @@ func readString(dirpath string, file string) string {

 func readUInt64(dirpath string, file string) uint64 {
 	out := readString(dirpath, file)
-	if out == "" || out == "max" {
+	if out == "max" {
+		return math.MaxUint64


nit: The max value for cgroup v1 is 9223372036854771712 (max signed integer rounded to nearest page, at least on x86). So to be correct it should be something like this I guess:

Suggested change

return math.MaxUint64

return math.MaxInt64^0xFFF

I am fine with changing it, but would it be a problem with cgroup v1? "max" is used only on cgroup v2

I don't remember where this value is used, but I just thought it was nice with the same value for both cgroup v2. If you try to write MaxInt64^0xFFF + 0x1 you get max on cgroup v2, and 9223372036854771712 on v1. 9223372036854771712 is also default on v1.

Edit: This is used for more than the memory limits, and my above comments only apply to those. Hmm

giuseppe · 2021-03-25T15:34:43Z

a similar fix for cpu specs: #2839

iwankgb · 2021-03-25T16:16:13Z

container/common/helpers.go

@@ -226,7 +203,10 @@ func readString(dirpath string, file string) string {

 func readUInt64(dirpath string, file string) uint64 {
 	out := readString(dirpath, file)
-	if out == "" || out == "max" {
+	if out == "max" {


Why have you decided to change behaviour for out == ""?

for out == "" I've left the same behavior there was before 2ccad4b

Of course, my bad!

iwankgb · 2021-03-25T16:17:34Z

/ok-to-test

odinuge · 2021-03-26T13:53:58Z

This change is probably related to:

cadvisor/manager/manager.go

Lines 473 to 479 in fa07332

    
           if spec.HasMemory { 
        
           	// Memory.Limit is 0 means there's no limit 
        
           	if spec.Memory.Limit == 0 { 
        
           		m.machineMu.RLock() 
        
           		spec.Memory.Limit = uint64(m.machineInfo.MemoryCapacity) 
        
           		m.machineMu.RUnlock() 
        
           	}

Does that code do anything at all if this change merged?

Also, the description of this value is probably wrong, since it says -1 is default, but it is an unsigned value...

cadvisor/info/v2/container.go

Lines 46 to 48 in fa07332

    
           // The amount of memory requested. Default is unlimited (-1). 
        
           // Units: bytes. 
        
           Limit uint64 `json:"limit,omitempty"`

giuseppe · 2021-03-26T16:03:09Z

yes it will be ignored.

And I think this is the expectation, at least from Kubernetes, since there is a e2e tests failing right now as the memory limit is being set while it is expected to be unlimited (isMemoryUnlimited in pkg/kubelet/stats/helper.go)

移植upstream对kubelet及cadvisor的修改，修复使用cgroupv2时指标收集统计的问题 1. port cadvisor pr google/cadvisor#2839 reading cpu stats on cgroup v2 2. port cadvisor pr google/cadvisor#2837 read "max" value for cgroup v2 3. port cadvisor pr google/cadvisor#2801 gathering of stats for root cgroup on v2 4. port cadvisor pr google/cadvisor#2800: Update heuristic for container creation time 5. Fix cgroup handling for systemd with cgroup v2 6. test: adjust summary test for cgroup v2

giuseppe added 2 commits March 24, 2021 17:42

helpers: simplify cgroup v2 memory stats

cf79551

read the memory stats from the specified cgroup instead of looking the values up in the ancestor cgroups. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

helpers: handle cgroup v2 "max" value

7bd9ea7

when the "max" value is found handle it as math.MaxUint64. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

google-cla bot added the cla: yes label Mar 24, 2021

k8s-ci-robot added the needs-ok-to-test label Mar 24, 2021

giuseppe mentioned this pull request Mar 24, 2021

Failing kubelet /stats/summary API and ResourceMetricsAPI tests on cgroup v2 kubernetes/kubernetes#99230

Closed

odinuge reviewed Mar 24, 2021

View reviewed changes

giuseppe marked this pull request as ready for review March 25, 2021 15:34

iwankgb reviewed Mar 25, 2021

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 25, 2021

iwankgb approved these changes Mar 26, 2021

View reviewed changes

iwankgb merged commit 4765480 into google:master Mar 26, 2021

giuseppe mentioned this pull request May 25, 2021

pkg/kubelet/cm: cgroup-related cleanups kubernetes/kubernetes#102218

Merged

This was referenced May 25, 2021

Cut the 0.40 (or 0.39.2) release to have cgroup v2 fixes #2878

Closed

[0.39] Cgroup v2 fixes backport #2879

Merged

giuseppe mentioned this pull request Jun 1, 2021

vendor: upgrade cadvisor to 0.39.2 kubernetes/kubernetes#102483

Merged

chenchun pushed a commit to chenchun/kubernetes that referenced this pull request Mar 20, 2024

port cadvisor pr google/cadvisor#2837 read "max" value for cgroup v2

e4d3d4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix read "max" value for cgroup v2 #2837

Fix read "max" value for cgroup v2 #2837

giuseppe commented Mar 24, 2021

k8s-ci-robot commented Mar 24, 2021

giuseppe commented Mar 24, 2021

odinuge Mar 24, 2021

odinuge left a comment

odinuge Mar 24, 2021 •

edited

Loading

giuseppe Mar 25, 2021

odinuge Mar 25, 2021

odinuge Mar 25, 2021

giuseppe commented Mar 25, 2021

iwankgb Mar 25, 2021

giuseppe Mar 26, 2021

iwankgb Mar 26, 2021

iwankgb commented Mar 25, 2021

odinuge commented Mar 26, 2021

giuseppe commented Mar 26, 2021

Fix read "max" value for cgroup v2 #2837

Fix read "max" value for cgroup v2 #2837

Conversation

giuseppe commented Mar 24, 2021

k8s-ci-robot commented Mar 24, 2021

giuseppe commented Mar 24, 2021

odinuge Mar 24, 2021

Choose a reason for hiding this comment

odinuge left a comment

Choose a reason for hiding this comment

odinuge Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

giuseppe Mar 25, 2021

Choose a reason for hiding this comment

odinuge Mar 25, 2021

Choose a reason for hiding this comment

odinuge Mar 25, 2021

Choose a reason for hiding this comment

giuseppe commented Mar 25, 2021

iwankgb Mar 25, 2021

Choose a reason for hiding this comment

giuseppe Mar 26, 2021

Choose a reason for hiding this comment

iwankgb Mar 26, 2021

Choose a reason for hiding this comment

iwankgb commented Mar 25, 2021

odinuge commented Mar 26, 2021

giuseppe commented Mar 26, 2021

odinuge Mar 24, 2021 •

edited

Loading