fix: return total system memory if the memory limit is not specified

[ningziwen]

Signed-off-by: Ziwen Ning ningziwe@amazon.com

containerd/nerdctl#1589

[estesp]

gopsutil is a direct dependency so should be in the first require clause; go mod tidy should have put this in the correct place in go.mod; either way, that is why the CI project check failed.

[estesp]

gopsutil is a direct dependency so should be in the first require clause; go mod tidy should have put this in the correct place in go.mod; either way, that is why the CI project check failed.

[ningziwen]

Thanks! Fixed. I'm still figuring out how to test it and how to fix v1 so it is in draft.

[kzys]

Is it hard to implement by ourselves? cgroups is Linux-only whereas gopsutil supports other OSes and brings a bunch of dependencies.

[ningziwen]

Yeah I can implement it without adding gopsutil.

[kzys]

1. I prefer to not have this type of Do What I Mean behavior in low-level packages. There may be customers who want to check whether the limit is configured or not. The current behavior (returning MaxUint64) may not be the best, but the new behavior is even harder for them.
2. There are other limits we can set on cgroups (see https://github.com/containerd/cgroups/search?q=limit). Would we do the same for others?

[ningziwen]

1. I prefer to not have this type of Do What I Mean behavior in low-level packages. There may be customers who want to check whether the limit is configured or not. The current behavior (returning MaxUint64) may not be the best, but the new behavior is even harder for them.
2. There are other limits we can set on cgroups (see https://github.com/containerd/cgroups/search?q=limit). Would we do the same for others?

@kzys

If we change this in containerd, there is not a perfect way to check if the memory is configured. Returning Math.max memory can not only mean the memory is not configured, it can also mean the memory is really set to Math.max (that may be not possible today but may cause issue one day later). Do you think it is ok to use Math.max to check in containerd?

Similarily, if other consumers are using Math.max to judge if the memory is configured, they are currently having same defects.

So there are a few options below:

Option 1 transfer from Math.max to host memory limit in containerd and keep cgroup same.

Pros:

• lowest risk to impact other consumers of cgroup

Cons:

• not perfectly logical to use Math.max to check if memory is configured in containerd, and other cgroup consuemrs. May cause issues in the future.

Option 2 Change default memory limit to host memory limit.
Pros:

• Consumers don't have to handle the default memory limit overwriting.

Cons:

• May break existing consumer's workflow.

Option 3 Change default memory limit to NA in cgroup layer. Containerd and other consumers can transfer to NA to whatever they want (such as host memory limit).

Pros:

• Consumer's conditional check will be logical.

Cons:

• May break existing consumer's workflow.

Option 4 Keep cgroup same. Do some refactoring to add additional response field to tell the consumers that the memory limit is not configured. Containerd and other consumers can overwrite the memory limit to whatever they want when the memory limit is not configured

Pros:

• Won't break any consumers.
• Consumer's conditional check will be logical.

Cons:

• Cgroup will need to return an additional field in the Stat() response.

Considering option 2 is not preferred, I'm slightly leaning to Option 4. We can just add a field around here to show if the memory limit is configured.

cgroups/cgroup2/stats/metrics.pb.go

Line 297 in 157c4da

UsageLimit uint64 `protobuf:"varint,33,opt,name=usage_limit,json=usageLimit,proto3" json:"usage_limit,omitempty"`

(Similar for cgroup v1)

For the second question, I think we can apply the same option for other limits to keep them consistent.

Let me know if you have better ideas.

[dcantah]

I'll give some thought to what might make sense to return here as well, but for the introduction of the new dependency it really should not be needed. It just reads /proc/meminfo, splits by line and then we just need the MemTotal section:

dcantah@dcantah ~ $ cat /proc/meminfo
MemTotal:       16338456 kB
MemFree:         5120496 kB
MemAvailable:   11223128 kB
Buffers:          889596 kB
... omitted for brevity

[kzys]

I like Option 1 or Option 4.

@AkihiroSuda What do you think?

[ningziwen]

@AkihiroSuda WDYT?

[AkihiroSuda]

Returning Math.max memory can not only mean the memory is not configured, it can also mean the memory is really set to Math.max (that may be not possible today but may cause issue one day later). Do you think it is ok to use Math.max to check in containerd?

I think this is fine.
Nobody will actually set the cgroup memory limit to 16EB until the ecosystem migrates to an 128-bit arch with >= 64-bit physical address bus, which isn’t realistic in this century.

[AkihiroSuda]

But I agree Option 4 looks better

[AkihiroSuda]

We can just add a field around here to show if the memory limit is configured.

Can we just use zero or nil value to indicate that without adding a new field?