Skip to content
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

libcontainer: set pids limit to max when set to 0 #4015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haircommander
Copy link
Contributor

potentially fixes #4014

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

libcontainer/cgroups/fs/pids.go Show resolved Hide resolved
libcontainer/cgroups/fs2/pids.go Show resolved Hide resolved
@lifubang
Copy link
Member

Some tests need to be fixed.
After you fix all the errors, I think you should squash all your commits to one, and then add an integration test for PidsLimit to avoid this error when someone refactor the code.

@haircommander
Copy link
Contributor Author

I can't see cs9 failure but I'm hoping it's a flake? regardless, comments addressed, PTAL

@lifubang
Copy link
Member

I can't see cs9 failure but I'm hoping it's a flake?

not ok 19 runc run (cgroup v2 resources.unified override)
# (in test file tests/integration/cgroups.bats, line 262)
#   `[ "$status" -eq 0 ]' failed
# runc spec (status=0):
#
# runc run -d --console-socket /tmp/bats-run-YlegjG/runc.T1kloa/tty/sock test_cgroups_unified (status=1):
# time="2023-09-15T13:29:09Z" level=error msg="runc run failed: unable to start container process: container init was OOM-killed (memory limit too low?)"

I think maybe we need #3987 merged to fix this issue.

@lifubang
Copy link
Member

I think maybe we need #3987 merged to fix this issue.

CI has been fixed by #4020

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, thanks.
Left some small suggestions.

libcontainer/integration/exec_test.go Show resolved Hide resolved
libcontainer/specconv/spec_linux.go Show resolved Hide resolved
update.go Show resolved Hide resolved
@kolyshkin
Copy link
Contributor

kolyshkin commented Sep 21, 2023

The current runc code treats PidsLimit value like this:

  • > 0 -- set to specific value
  • -1 (or, in some cases, any negative value) -- set to "max"
  • `== 0' -- unset

So, we have the "unset" case without using a pointer.

The only issue is, we should properly convert runtime-spec representation into internal representation, essentially meaning, if spec value is <=0, set PidsLimit to -1.

I.e. something like this might be sufficient to fix the bug:

--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go
@@ -772,7 +772,13 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi
                                c.Resources.CPUIdle = r.CPU.Idle
                        }
                        if r.Pids != nil {
-                               c.Resources.PidsLimit = r.Pids.Limit
+                               if r.Pids.Limit > 0 {
+                                       c.Resources.PidsLimit = r.Pids.Limit
+                               } else {
+                                       // Convert from runtime-spec (where a value <= 0 means "max")
+                                       // to internal (where -1 is "max", and 0 is "unset").
+                                       c.Resources.PidsLimit = -1
+                               }
                        }
                        if r.BlockIO != nil {
                                if r.BlockIO.Weight != nil {
--- a/libcontainer/configs/cgroup_linux.go
+++ b/libcontainer/configs/cgroup_linux.go
@@ -90,7 +90,7 @@ type Resources struct {
        // cgroup SCHED_IDLE
        CPUIdle *int64 `json:"cpu_idle,omitempty"`
 
-       // Process limit; set <= `0' to disable limit.
+       // Process limit; 0 is "unset", -1 is "max".
        PidsLimit int64 `json:"pids_limit"`
 
        // Specifies per cgroup weight, range is from 10 to 1000.

(alas, we probably also need the same code in update.go)

WDYT @lifubang @haircommander? I am more comfortable with a change like this merely because it is smaller and does not introduce a potential panic when dereferening a nil pointer.

@lifubang
Copy link
Member

because it is smaller

Yes, it is indeed a smaller change with your code. But I think github.com/opencontainers/runc/libcontainer/cgroups is used by some upstream projects, they must know this knowledge before they write the code, so I think maybe we should remain it as is?
WDYT @kolyshkin

@kolyshkin
Copy link
Contributor

Yes, it is indeed a smaller change with your code. But I think github.com/opencontainers/runc/libcontainer/cgroups is used by some upstream projects, they must know this knowledge before they write the code, so I think maybe we should remain it as is?

That reminds me, changing Limit field to be a pointer is a breaking change for all those projects. We said we do not guarantee that API will stay compatible, so if there is a reason to break it, I would rather do so, but in this case I don't see a reason. Currently we do support both "unlimited" and "unset" cases, and I guess existing users of libcontainer already know how to use these. So, nothing needs to be changed here.

I'll keep looking into it.

@lifubang
Copy link
Member

is a breaking change for all those projects

I think it can't be defined as a breaking change, we don't break any result, this is just only a change of type, when they update to the new version of libcontainer, they know this change, because the code can't be compiled successfully.

but in this case I don't see a reason

There is a reason, if keep it as a raw type int64, the value will be always 0, so we will always write max to pids.max when we call Manager.Set, it is unnecessary.

@kolyshkin
Copy link
Contributor

kolyshkin commented Sep 22, 2023

is a breaking change for all those projects

I think it can't be defined as a breaking change, we don't break any result, this is just only a change of type, when they update to the new version of libcontainer, they know this change, because the code can't be compiled successfully.

This is exactly what a breaking change is. Your code worked, you updated a dependency, and now it won't even compile.

Yes, we've done it in the past and we'll do it again, but this should be justified.

but in this case I don't see a reason

There is a reason, if keep it as a raw type int64, the value will be always 0, so we will always write max to pids.max when we call Manager.Set, it is unnecessary.

Not necessarily. Current code treats PidsLimit==0 as unset, which is the behavior I intend to keep. In other words, I think the fix should belong to specconv (this is what I'm doing in #4023 -- let me know what you think).

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As outlined above, I don't like the PidsLimit change to be a pointer. See #4023 which fixes the issue reported without so much changes.

@haircommander
Copy link
Contributor Author

I am happy either way as long as the behavior when setting 0 to pids limit is well defined :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined (and potentially incorrect) behavior when pids limit is set to 0
3 participants