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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cfd6e8
tests/int: fix "runc run (cgroup v2 resources.unified override)"
kolyshkin Sep 22, 2023
36e3e42
tests/int/update: use systemd_version()
kolyshkin Sep 22, 2023
4c61f68
runc run: treat pids.limit=0 as unlimited
kolyshkin Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion libcontainer/configs/cgroup_linux.go
View file
Open in desktop
Expand Up @@ -90,7 +90,7 @@ type Resources struct {
// cgroup SCHED_IDLE
CPUIdle *int64 `json:"cpu_idle,omitempty"`

// Process limit; set <= `0' to disable limit.
// Maximum number of tasks; 0 for unset, -1 for max/unlimited.
PidsLimit int64 `json:"pids_limit"`

// Specifies per cgroup weight, range is from 10 to 1000.
Expand Down
8 changes: 7 additions & 1 deletion libcontainer/specconv/spec_linux.go
View file
Open in desktop
Expand Up @@ -775,8 +775,14 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi
c.Resources.CpusetMems = r.CPU.Mems
c.Resources.CPUIdle = r.CPU.Idle
}
// Convert pids limit from the runtime-spec value (where any value <= 0 means "unlimited")
// to internal runc value (where -1 is "unlimited", and 0 is "unset").
if r.Pids != nil {
c.Resources.PidsLimit = r.Pids.Limit
if r.Pids.Limit > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :

  1. Don't always write pids.max if PidsLimit == 0;
  2. Don't introduce break changes of libcontainer API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. 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);
  2. 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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

c.Resources.PidsLimit = r.Pids.Limit
} else {
c.Resources.PidsLimit = -1
}
}
if r.BlockIO != nil {
if r.BlockIO.Weight != nil {
Expand Down
2 changes: 1 addition & 1 deletion man/runc-update.8.md
View file
Open in desktop
Expand Up @@ -85,7 +85,7 @@ stdin. If this option is used, all other options are ignored.
(i.e. use unlimited swap).

**--pids-limit** _num_
: Set the maximum number of processes allowed in the container.
: Set the maximum number of tasks. Use **-1** for unlimited.

**--l3-cache-schema** _value_
: Set the value for Intel RDT/CAT L3 cache schema.
Expand Down
15 changes: 15 additions & 0 deletions tests/integration/cgroups.bats
View file
Open in desktop
Expand Up @@ -263,6 +263,20 @@ convert_hugetlb_size() {
done
}

# https://github.com/opencontainers/runc/issues/4014.
@test "runc run (pids.limit=0 means unlimited)" {
[ $EUID -ne 0 ] && requires rootless_cgroup

set_cgroups_path
update_config '.linux.resources.pids.limit |= 0'

runc run -d --console-socket "$CONSOLE_SOCKET" test_pids
[ "$status" -eq 0 ]
check_cgroup_value "pids.max" "max"
# systemd < v227 shows UINT64_MAX instead of "infinity".
check_systemd_value "TasksMax" "infinity" "18446744073709551615"
}

@test "runc run (cgroup v2 resources.unified only)" {
requires root cgroups_v2

Expand Down Expand Up @@ -309,6 +323,7 @@ convert_hugetlb_size() {
set_cgroups_path
# CPU shares of 3333 corresponds to CPU weight of 128.
update_config ' .linux.resources.memory |= {"limit": 33554432}
| .linux.resources.pids.limit |= 100
| .linux.resources.cpu |= {
"shares": 3333,
"quota": 40000,
Expand Down
3 changes: 1 addition & 2 deletions tests/integration/update.bats
View file
Open in desktop
Expand Up @@ -54,8 +54,7 @@ function setup() {
fi

SD_UNLIMITED="infinity"
SD_VERSION=$(systemctl --version | awk '{print $2; exit}')
if [ "$SD_VERSION" -lt 227 ]; then
if [ "$(systemd_version)" -lt 227 ]; then
SD_UNLIMITED="18446744073709551615"
fi

Expand Down
4 changes: 2 additions & 2 deletions update.go
View file
Open in desktop
Expand Up @@ -122,11 +122,11 @@ other options are ignored.
},
cli.StringFlag{
Name: "memory-swap",
Usage: "Total memory usage (memory + swap); set '-1' to enable unlimited swap",
Usage: "Total memory usage (memory + swap); use '-1' to enable unlimited swap",
},
cli.IntFlag{
Name: "pids-limit",
Usage: "Maximum number of pids allowed in the container",
Usage: "Maximum number of tasks; use '-1' for unlimited",
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. This is the way it was always working.
  2. It seems that the behavior of runc update is not in spec.

},
cli.StringFlag{
Name: "l3-cache-schema",
Expand Down