From 78750bc70ffee5746e5220c662180c428e9e3ed6 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 12 Sep 2023 16:27:19 -0400 Subject: [PATCH] libcontainer: set pids limit to max when set to 0 Signed-off-by: Peter Hunt --- libcontainer/cgroups/devices/systemd_test.go | 5 ++- libcontainer/cgroups/fs/pids.go | 19 ++++----- libcontainer/cgroups/fs/pids_test.go | 44 ++++++++++++++++---- libcontainer/cgroups/fs2/create.go | 2 +- libcontainer/cgroups/fs2/pids.go | 8 +--- libcontainer/cgroups/systemd/v1.go | 4 +- libcontainer/cgroups/systemd/v2.go | 4 +- libcontainer/configs/cgroup_linux.go | 2 +- libcontainer/integration/exec_test.go | 7 ++-- libcontainer/specconv/spec_linux.go | 3 +- update.go | 3 +- 11 files changed, 62 insertions(+), 39 deletions(-) diff --git a/libcontainer/cgroups/devices/systemd_test.go b/libcontainer/cgroups/devices/systemd_test.go index b6e81dcd827..8bb9c97967b 100644 --- a/libcontainer/cgroups/devices/systemd_test.go +++ b/libcontainer/cgroups/devices/systemd_test.go @@ -28,12 +28,13 @@ func TestPodSkipDevicesUpdate(t *testing.T) { } podName := "system-runc_test_pod" + t.Name() + ".slice" + l := int64(42) podConfig := &configs.Cgroup{ Systemd: true, Parent: "system.slice", Name: podName, Resources: &configs.Resources{ - PidsLimit: 42, + PidsLimit: &l, Memory: 32 * 1024 * 1024, SkipDevices: true, }, @@ -97,7 +98,7 @@ func TestPodSkipDevicesUpdate(t *testing.T) { // Now update the pod a few times. for i := 0; i < 42; i++ { - podConfig.Resources.PidsLimit++ + *podConfig.Resources.PidsLimit++ podConfig.Resources.Memory += 1024 * 1024 if err := pm.Set(podConfig.Resources); err != nil { t.Fatal(err) diff --git a/libcontainer/cgroups/fs/pids.go b/libcontainer/cgroups/fs/pids.go index 1f13532a5ad..0978f4a0ee8 100644 --- a/libcontainer/cgroups/fs/pids.go +++ b/libcontainer/cgroups/fs/pids.go @@ -20,20 +20,17 @@ func (s *PidsGroup) Apply(path string, _ *configs.Resources, pid int) error { } func (s *PidsGroup) Set(path string, r *configs.Resources) error { - if r.PidsLimit != 0 { - // "max" is the fallback value. - limit := "max" - - if r.PidsLimit > 0 { - limit = strconv.FormatInt(r.PidsLimit, 10) - } + if r.PidsLimit == nil { + return nil + } + // "max" is the fallback value. + limit := "max" - if err := cgroups.WriteFile(path, "pids.max", limit); err != nil { - return err - } + if *r.PidsLimit > 0 { + limit = strconv.FormatInt(*r.PidsLimit, 10) } - return nil + return cgroups.WriteFile(path, "pids.max", limit) } func (s *PidsGroup) GetStats(path string, stats *cgroups.Stats) error { diff --git a/libcontainer/cgroups/fs/pids_test.go b/libcontainer/cgroups/fs/pids_test.go index 9d9a7ce8f18..be0d1a9a6ef 100644 --- a/libcontainer/cgroups/fs/pids_test.go +++ b/libcontainer/cgroups/fs/pids_test.go @@ -9,9 +9,10 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) -const ( - maxUnlimited = -1 - maxLimited = 1024 +var ( + maxUnlimited int64 = -1 + maxZero int64 + maxLimited int64 = 1024 ) func TestPidsSetMax(t *testing.T) { @@ -22,7 +23,7 @@ func TestPidsSetMax(t *testing.T) { }) r := &configs.Resources{ - PidsLimit: maxLimited, + PidsLimit: &maxLimited, } pids := &PidsGroup{} if err := pids.Set(path, r); err != nil { @@ -33,20 +34,45 @@ func TestPidsSetMax(t *testing.T) { if err != nil { t.Fatal(err) } - if value != maxLimited { + // Only done for comparison + if value != uint64(maxLimited) { t.Fatalf("Expected %d, got %d for setting pids.max - limited", maxLimited, value) } } +func TestPidsSetUnlimitedWhenZero(t *testing.T) { + path := tempDir(t, "pids") + + writeFileContents(t, path, map[string]string{ + "pids.max": "max", + }) + + r := &configs.Resources{ + PidsLimit: &maxZero, + } + pids := &PidsGroup{} + if err := pids.Set(path, r); err != nil { + t.Fatal(err) + } + + value, err := fscommon.GetCgroupParamString(path, "pids.max") + if err != nil { + t.Fatal(err) + } + if value != "max" { + t.Fatalf("Expected %s, got %s for setting pids.max - unlimited", "max", value) + } +} + func TestPidsSetUnlimited(t *testing.T) { path := tempDir(t, "pids") writeFileContents(t, path, map[string]string{ - "pids.max": strconv.Itoa(maxLimited), + "pids.max": strconv.FormatInt(maxLimited, 10), }) r := &configs.Resources{ - PidsLimit: maxUnlimited, + PidsLimit: &maxUnlimited, } pids := &PidsGroup{} if err := pids.Set(path, r); err != nil { @@ -67,7 +93,7 @@ func TestPidsStats(t *testing.T) { writeFileContents(t, path, map[string]string{ "pids.current": strconv.Itoa(1337), - "pids.max": strconv.Itoa(maxLimited), + "pids.max": strconv.FormatInt(maxLimited, 10), }) pids := &PidsGroup{} @@ -80,7 +106,7 @@ func TestPidsStats(t *testing.T) { t.Fatalf("Expected %d, got %d for pids.current", 1337, stats.PidsStats.Current) } - if stats.PidsStats.Limit != maxLimited { + if stats.PidsStats.Limit != uint64(maxLimited) { t.Fatalf("Expected %d, got %d for pids.max", maxLimited, stats.PidsStats.Limit) } } diff --git a/libcontainer/cgroups/fs2/create.go b/libcontainer/cgroups/fs2/create.go index 641123a4d84..72312b251d2 100644 --- a/libcontainer/cgroups/fs2/create.go +++ b/libcontainer/cgroups/fs2/create.go @@ -39,7 +39,7 @@ func needAnyControllers(r *configs.Resources) (bool, error) { return ok } - if isPidsSet(r) && have("pids") { + if r.PidsLimit != nil && have("pids") { return true, nil } if isMemorySet(r) && have("memory") { diff --git a/libcontainer/cgroups/fs2/pids.go b/libcontainer/cgroups/fs2/pids.go index c8c4a365894..4616fdf0b46 100644 --- a/libcontainer/cgroups/fs2/pids.go +++ b/libcontainer/cgroups/fs2/pids.go @@ -13,15 +13,11 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) -func isPidsSet(r *configs.Resources) bool { - return r.PidsLimit != 0 -} - func setPids(dirPath string, r *configs.Resources) error { - if !isPidsSet(r) { + if r.PidsLimit == nil { return nil } - if val := numToStr(r.PidsLimit); val != "" { + if val := numToStr(*r.PidsLimit); val != "" { if err := cgroups.WriteFile(dirPath, "pids.max", val); err != nil { return err } diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 8c64a5887a9..19a19622d91 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -98,9 +98,9 @@ func genV1ResourcesProperties(r *configs.Resources, cm *dbusConnManager) ([]syst newProp("BlockIOWeight", uint64(r.BlkioWeight))) } - if r.PidsLimit > 0 || r.PidsLimit == -1 { + if r.PidsLimit != nil && (*r.PidsLimit > 0 || *r.PidsLimit == -1) { properties = append(properties, - newProp("TasksMax", uint64(r.PidsLimit))) + newProp("TasksMax", uint64(*r.PidsLimit))) } err = addCpuset(cm, &properties, r.CpusetCpus, r.CpusetMems) diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index b28ec6b22f2..64176a28be4 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -257,9 +257,9 @@ func genV2ResourcesProperties(dirPath string, r *configs.Resources, cm *dbusConn addCpuQuota(cm, &properties, r.CpuQuota, r.CpuPeriod) - if r.PidsLimit > 0 || r.PidsLimit == -1 { + if r.PidsLimit != nil && (*r.PidsLimit > 0 || *r.PidsLimit == -1) { properties = append(properties, - newProp("TasksMax", uint64(r.PidsLimit))) + newProp("TasksMax", uint64(*r.PidsLimit))) } err = addCpuset(cm, &properties, r.CpusetCpus, r.CpusetMems) diff --git a/libcontainer/configs/cgroup_linux.go b/libcontainer/configs/cgroup_linux.go index 1664304be21..8e09c7406d6 100644 --- a/libcontainer/configs/cgroup_linux.go +++ b/libcontainer/configs/cgroup_linux.go @@ -88,7 +88,7 @@ type Resources struct { CPUIdle *int64 `json:"cpu_idle,omitempty"` // Process limit; set <= `0' to disable limit. - PidsLimit int64 `json:"pids_limit"` + PidsLimit *int64 `json:"pids_limit"` // Specifies per cgroup weight, range is from 10 to 1000. BlkioWeight uint16 `json:"blkio_weight"` diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index 9795964caf2..b6cb05f97a4 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -535,14 +535,15 @@ func testPids(t *testing.T, systemd bool) { } config := newTemplateConfig(t, &tParam{systemd: systemd}) - config.Cgroups.Resources.PidsLimit = -1 + l := int64(-1) + config.Cgroups.Resources.PidsLimit = &l // Running multiple processes, expecting it to succeed with no pids limit. _ = runContainerOk(t, config, "/bin/sh", "-c", "/bin/true | /bin/true | /bin/true | /bin/true") // Enforce a permissive limit. This needs to be fairly hand-wavey due to the // issues with running Go binaries with pids restrictions (see below). - config.Cgroups.Resources.PidsLimit = 64 + *config.Cgroups.Resources.PidsLimit = 64 _ = runContainerOk(t, config, "/bin/sh", "-c", ` /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | @@ -551,7 +552,7 @@ func testPids(t *testing.T, systemd bool) { // Enforce a restrictive limit. 64 * /bin/true + 1 * shell should cause // this to fail reliably. - config.Cgroups.Resources.PidsLimit = 64 + *config.Cgroups.Resources.PidsLimit = 64 out, _, err := runContainer(t, config, "/bin/sh", "-c", ` /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | /bin/true | bin/true | /bin/true | diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index d3938da516c..5975476da23 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -768,7 +768,8 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi c.Resources.CPUIdle = r.CPU.Idle } if r.Pids != nil { - c.Resources.PidsLimit = r.Pids.Limit + l := r.Pids.Limit + c.Resources.PidsLimit = &l } if r.BlockIO != nil { if r.BlockIO.Weight != nil { diff --git a/update.go b/update.go index 425ce135397..ea9ad1b825b 100644 --- a/update.go +++ b/update.go @@ -310,8 +310,9 @@ other options are ignored. config.Cgroups.Resources.MemoryReservation = *r.Memory.Reservation config.Cgroups.Resources.MemorySwap = *r.Memory.Swap config.Cgroups.Resources.MemoryCheckBeforeUpdate = *r.Memory.CheckBeforeUpdate - config.Cgroups.Resources.PidsLimit = r.Pids.Limit config.Cgroups.Resources.Unified = r.Unified + l := r.Pids.Limit + config.Cgroups.Resources.PidsLimit = &l // Update Intel RDT l3CacheSchema := context.String("l3-cache-schema")