Fix invalid handling of pids.limit=0 from runtime spec #4023

kolyshkin · 2023-09-22T02:58:43Z

This is an alternative to #4015 (smaller set of code changes, no libct API breakage).

It has been pointed out in #4014 that runc incorrectly ignores pids.limit=0 set
in the runtime spec. This happens because runtime-spec says "default is
unlimited" and also allows for Pids to not be set at all, thus
distinguishing unset (Resources.Pids == nil) from unlimited
(Resources.Pids.Limit <= 0).

Internally, runc also distinguishes unset from unlimited, but since we
do not use a pointer, we treat 0 as unset and -1 as unlimited.

Add a conversion code to libcontainer/specconv.

Add a test case to check that starting a container with pids.limit=0
results in no pids limit (the test fails before the fix when systemd
cgroup manager is used, as systemd apparently uses parent's TasksMax;
see #4022 for CI runs).

NOTE that runc update still treats 0 as "unset".

Finally, fix/update the documentation here and there.

Fixes: #4014.

kolyshkin · 2023-09-22T18:42:51Z

@haircommander PTAL

lifubang · 2023-09-23T02:10:15Z

update.go

 		},
 		cli.IntFlag{
 			Name:  "pids-limit",
-			Usage: "Maximum number of pids allowed in the container",
+			Usage: "Maximum number of tasks; use '-1' for unlimited",


Maybe some users will be confused with -r and runtime-spec?

runc update -r - containerid { "pids": { "limit": 0 } }

This is the way it was always working.

It seems that the behavior of runc update is not in spec.

lifubang · 2023-09-23T02:27:10Z

libcontainer/specconv/spec_linux.go

 			if r.Pids != nil {
-				c.Resources.PidsLimit = r.Pids.Limit
+				if r.Pids.Limit > 0 {


I don't know whether upstream projects should write such codes or not when they update the container's pids limit?

if r.Pids.Limit > 0 { c.Resources.PidsLimit = r.Pids.Limit } else { c.Resources.PidsLimit = -1 }

@AkihiroSuda Do you know what's the situation in nerdctl.

Maybe in kubelet they should know this information:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cm/cgroup_manager_linux.go#L355-L381

So I still think we should fix it in pids.go, because others always use Manager.Set directly without use specconv.
But we have to solve 2 problems if we choose solutions like #4015 :

Don't always write pids.max if PidsLimit == 0;

Don't introduce break changes of libcontainer API.

I took a look at git history, and it seems that libcontainer always treated 0 as unset for pids limit, since commit db3159c from January 2016 that added the initial pids limit support (alas, the very same commit incorrectly documents the field (db3159c#diff-b71b6973c045d22e41e802cf65cade82c573a844190bc0d06a6ade9f21cb2c5c), and this PR fixes that).

Thus, I am pretty sure libcontainer users are aware of what 0 means for pids limit in libcontainer; yet I am going to look into kubernetes and nerdctl to confirm.

Now, we have two problems to solve:

runc violates the runtime spec by treating pids.limit==0 as unset rather than unlimited (Undefined (and potentially incorrect) behavior when pids limit is set to 0 #4014);

libcontainer's handling of pids limit value differs from runtime spec.

I think that 2 is much less of a problem than 1, and can be solved by properly documenting the PidsLimit field (this is what this PR does).

FWIW: case 1 is the only case I care about fixing

haircommander

LGTM, thanks!

lifubang · 2023-09-28T00:08:17Z

If we say that we just only need to fix it in ‘runc create’ and ‘runc run’, for the uses using ‘runc update’ and ‘libcontainer’, they should fix this issue in their own projects according to runc’s doc. This one LGTM.

kolyshkin · 2023-09-29T17:47:15Z

for the uses using ‘runc update’ and ‘libcontainer’,

Yes, both runc update and libcontainer API is not actually covered by runtime-spec (which is sort of a problem, but out of scope for this).

lifubang

LGTM, to fix this issue completely, it needs to open PR by their own contributors in projects using ‘runc update’ or ‘runc libcontainer API’, for example, Kubernetes, containerd, and cri-o etc.

lifubang · 2023-10-07T10:06:14Z

@opencontainers/runc-maintainers PTAL
It's really a bug for runc violates the runtime spec by treating pids.limit==0 as unset rather than unlimited. (#4023 (comment))

After this one backported to 1.1, we can release 1.1.10.

lifubang · 2023-10-11T14:49:59Z

@AkihiroSuda PTAL
I have checked docker cli and nerdctl, they tell users to use -1 for unlimited pids limit.

thaJeztah · 2023-10-11T19:34:18Z

I'd yet have to have a close look, but would like @cyphar to have a look; I think there's still some ambiguity in the description of the PR that updated the specs opencontainers/runtime-spec#234 ("there is no limit: meaning: don't (set) a limit (and use the active max)" vs "explicitly set it to max/unlimited")

The mention of "Go's unset is zero value" in that description may indicate that 0 == unset and thus the former may have been the intent?

And it wouldn't be the first time that specs were update to reflect the implementation, and now (due to ambiguity on the wording), the implementation being update to reflect the spec (going full circle)

kolyshkin · 2023-10-20T04:30:06Z

Rebased

This test case checks that unified resources override those set by conventional means, but it does not set conventional pids limit. Fix this. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

It has been pointed out that runc incorrectly ignores pids.limit=0 set in the runtime spec. This happens because runtime-spec says "default is unlimited" and also allows for Pids to not be set at all, thus distinguishing unset (Resources.Pids == nil) from unlimited (Resources.Pids.Limit <= 0). Internally, runc also distinguishes unset from unlimited, but since we do not use a pointer, we treat 0 as unset and -1 as unlimited. Add a conversion code to libcontainer/specconv. Add a test case to check that starting a container with pids.limit=0 results in no pids limit (the test fails before the fix when systemd cgroup manager is used, as systemd apparently uses parent's TasksMax). NOTE that runc update still treats 0 as "unset". Finally, fix/update the documentation here and there. Should fix issue 4014. Reported-by: Peter Hunt <pehunt@redhat.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

cyphar · 2023-10-23T23:26:53Z

Reading through my (8-year-old 😰) comments, I think that there were three issues at play here:

A value of 0 being identical to 1 is a quirk of the way pids limits work (a process needs to be in the cgroup to fork into the cgroup, so a limit of 0 and 1 are identical in terms of behaviour -- though with CLONE_INTO_CGROUP this is no longer true).
When I wrote that PR, I had sent a patch to make 0 an invalid value of a pids cgroup (due to point 1). It was never merged, but I think the logic was that having two values that mean the same thing was not ideal (and the cgroup maintainer had made a similar comment before merging the cgroup).
At the time, we hadn't fully settled on the whole pointers-in-Go-JSON thing, and so I just used the zero value of int64 as a temporary stopgap until we did a tree-wide fixup. It seems we never did a proper tree-wide fixup (or we skipped PidsLimit).

Looking at it now, I think the ideal option would be to change the spec so that PidsLimit is an *int64 and then we can have the behaviour of most other cgroups (unset/nil means to not touch the limit, any value means "use that value" with -1 meaning unlimited). However, given the disparate behaviour of runtimes when dealing with 0, the spec should have a note saying that (prior to whatever release has the new change) the behaviour was not always consistent...

kolyshkin · 2023-10-24T00:03:31Z

Removing the backport/todo/1.1 as we're not sure how to move forward with this (maybe we'll change the spec instead, since runc never accepted 0 as unlimited.

lifubang · 2023-10-24T00:49:11Z

maybe we'll change the spec instead, since runc never accepted 0 as unlimited.

👍

haircommander · 2023-11-21T18:59:24Z

in the meantime, CRI-O can just set -1 where it originally set 0 cri-o/cri-o#7503

AkihiroSuda · 2024-03-27T23:50:43Z

What's the next step?

kolyshkin requested a review from lifubang September 22, 2023 02:59

kolyshkin added this to the 1.2.0 milestone Sep 22, 2023

kolyshkin added area/systemd kind/bug backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 labels Sep 22, 2023

kolyshkin mentioned this pull request Sep 22, 2023

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

Open

kolyshkin marked this pull request as ready for review September 22, 2023 03:07

lifubang reviewed Sep 23, 2023

View reviewed changes

kolyshkin requested review from lifubang and haircommander September 27, 2023 17:34

haircommander approved these changes Sep 27, 2023

View reviewed changes

lifubang approved these changes Sep 30, 2023

View reviewed changes

lifubang requested review from thaJeztah, cyphar and AkihiroSuda October 2, 2023 12:14

AkihiroSuda approved these changes Oct 11, 2023

View reviewed changes

kolyshkin force-pushed the pids-limit branch from 949432b to 7ec2582 Compare October 20, 2023 04:30

kolyshkin added 3 commits October 20, 2023 19:56

tests/int: fix "runc run (cgroup v2 resources.unified override)"

5cfd6e8

This test case checks that unified resources override those set by conventional means, but it does not set conventional pids limit. Fix this. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

tests/int/update: use systemd_version()

36e3e42

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

lifubang force-pushed the pids-limit branch from 7ec2582 to 4c61f68 Compare October 20, 2023 11:56

kolyshkin removed the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Oct 24, 2023

haircommander mentioned this pull request Nov 21, 2023

config: set default pids limit to -1 cri-o/cri-o#7503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid handling of pids.limit=0 from runtime spec #4023

Fix invalid handling of pids.limit=0 from runtime spec #4023

kolyshkin commented Sep 22, 2023 •

edited

kolyshkin commented Sep 22, 2023

lifubang Sep 23, 2023

kolyshkin Sep 27, 2023

lifubang Sep 23, 2023

lifubang Sep 23, 2023

lifubang Sep 24, 2023

kolyshkin Sep 26, 2023

haircommander Sep 27, 2023

haircommander left a comment

lifubang commented Sep 28, 2023

kolyshkin commented Sep 29, 2023

lifubang left a comment

lifubang commented Oct 7, 2023

lifubang commented Oct 11, 2023

thaJeztah commented Oct 11, 2023

kolyshkin commented Oct 20, 2023

cyphar commented Oct 23, 2023 •

edited

kolyshkin commented Oct 24, 2023

lifubang commented Oct 24, 2023

haircommander commented Nov 21, 2023 •

edited

AkihiroSuda commented Mar 27, 2024

Fix invalid handling of pids.limit=0 from runtime spec #4023

Are you sure you want to change the base?

Fix invalid handling of pids.limit=0 from runtime spec #4023

Conversation

kolyshkin commented Sep 22, 2023 • edited

kolyshkin commented Sep 22, 2023

lifubang Sep 23, 2023

Choose a reason for hiding this comment

kolyshkin Sep 27, 2023

Choose a reason for hiding this comment

lifubang Sep 23, 2023

Choose a reason for hiding this comment

lifubang Sep 23, 2023

Choose a reason for hiding this comment

lifubang Sep 24, 2023

Choose a reason for hiding this comment

kolyshkin Sep 26, 2023

Choose a reason for hiding this comment

haircommander Sep 27, 2023

Choose a reason for hiding this comment

haircommander left a comment

Choose a reason for hiding this comment

lifubang commented Sep 28, 2023

kolyshkin commented Sep 29, 2023

lifubang left a comment

Choose a reason for hiding this comment

lifubang commented Oct 7, 2023

lifubang commented Oct 11, 2023

thaJeztah commented Oct 11, 2023

kolyshkin commented Oct 20, 2023

cyphar commented Oct 23, 2023 • edited

kolyshkin commented Oct 24, 2023

lifubang commented Oct 24, 2023

haircommander commented Nov 21, 2023 • edited

AkihiroSuda commented Mar 27, 2024

kolyshkin commented Sep 22, 2023 •

edited

cyphar commented Oct 23, 2023 •

edited

haircommander commented Nov 21, 2023 •

edited