-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
cgroups: add pids controller support #58
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
// +build linux | ||
|
||
package fs | ||
|
||
import ( | ||
"fmt" | ||
"strconv" | ||
|
||
"github.com/opencontainers/runc/libcontainer/cgroups" | ||
"github.com/opencontainers/runc/libcontainer/configs" | ||
) | ||
|
||
type PidsGroup struct { | ||
} | ||
|
||
func (s *PidsGroup) Name() string { | ||
return "pids" | ||
} | ||
|
||
func (s *PidsGroup) Apply(d *cgroupData) error { | ||
_, err := d.join("pids") | ||
if err != nil && !cgroups.IsNotFound(err) { | ||
return err | ||
} | ||
return nil | ||
} | ||
|
||
func (s *PidsGroup) Set(path string, cgroup *configs.Cgroup) error { | ||
if cgroup.Resources.PidsLimit != 0 { | ||
// "max" is the fallback value. | ||
limit := "max" | ||
|
||
if cgroup.Resources.PidsLimit > 0 { | ||
limit = strconv.FormatInt(cgroup.Resources.PidsLimit, 10) | ||
} | ||
|
||
if err := writeFile(path, "pids.max", limit); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (s *PidsGroup) Remove(d *cgroupData) error { | ||
return removePath(d.path("pids")) | ||
} | ||
|
||
func (s *PidsGroup) GetStats(path string, stats *cgroups.Stats) error { | ||
value, err := getCgroupParamUint(path, "pids.current") | ||
if err != nil { | ||
return fmt.Errorf("failed to parse pids.current - %s", err) | ||
} | ||
|
||
stats.PidsStats.Current = value | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
// +build linux | ||
|
||
package fs | ||
|
||
import ( | ||
"strconv" | ||
"testing" | ||
|
||
"github.com/opencontainers/runc/libcontainer/cgroups" | ||
) | ||
|
||
const ( | ||
maxUnlimited = -1 | ||
maxLimited = 1024 | ||
) | ||
|
||
func TestPidsSetMax(t *testing.T) { | ||
helper := NewCgroupTestUtil("pids", t) | ||
defer helper.cleanup() | ||
|
||
helper.writeFileContents(map[string]string{ | ||
"pids.max": "max", | ||
}) | ||
|
||
helper.CgroupData.config.Resources.PidsLimit = maxLimited | ||
pids := &PidsGroup{} | ||
if err := pids.Set(helper.CgroupPath, helper.CgroupData.config); err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
value, err := getCgroupParamUint(helper.CgroupPath, "pids.max") | ||
if err != nil { | ||
t.Fatalf("Failed to parse pids.max - %s", err) | ||
} | ||
|
||
if value != maxLimited { | ||
t.Fatalf("Expected %d, got %d for setting pids.max - limited", maxLimited, value) | ||
} | ||
} | ||
|
||
func TestPidsSetUnlimited(t *testing.T) { | ||
helper := NewCgroupTestUtil("pids", t) | ||
defer helper.cleanup() | ||
|
||
helper.writeFileContents(map[string]string{ | ||
"pids.max": strconv.Itoa(maxLimited), | ||
}) | ||
|
||
helper.CgroupData.config.Resources.PidsLimit = maxUnlimited | ||
pids := &PidsGroup{} | ||
if err := pids.Set(helper.CgroupPath, helper.CgroupData.config); err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
value, err := getCgroupParamString(helper.CgroupPath, "pids.max") | ||
if err != nil { | ||
t.Fatalf("Failed to parse pids.max - %s", err) | ||
} | ||
|
||
if value != "max" { | ||
t.Fatalf("Expected %s, got %s for setting pids.max - unlimited", "max", value) | ||
} | ||
} | ||
|
||
func TestPidsStats(t *testing.T) { | ||
helper := NewCgroupTestUtil("pids", t) | ||
defer helper.cleanup() | ||
|
||
helper.writeFileContents(map[string]string{ | ||
"pids.current": strconv.Itoa(1337), | ||
"pids.max": strconv.Itoa(maxLimited), | ||
}) | ||
|
||
pids := &PidsGroup{} | ||
stats := *cgroups.NewStats() | ||
if err := pids.GetStats(helper.CgroupPath, &stats); err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
if stats.PidsStats.Current != 1337 { | ||
t.Fatalf("Expected %d, got %d for pids.current", 1337, stats.PidsStats.Current) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,11 @@ type MemoryStats struct { | |
Stats map[string]uint64 `json:"stats,omitempty"` | ||
} | ||
|
||
type PidsStats struct { | ||
// number of pids in the cgroup | ||
Current uint64 `json:"current,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NumPids? We should also document that it is the number of pids in the cgroup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer it matches the name of the sysfs file. But it doesn't really matter I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this field matching the kernel API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This is the value of |
||
} | ||
|
||
type BlkioStatEntry struct { | ||
Major uint64 `json:"major,omitempty"` | ||
Minor uint64 `json:"minor,omitempty"` | ||
|
@@ -80,6 +85,7 @@ type HugetlbStats struct { | |
type Stats struct { | ||
CpuStats CpuStats `json:"cpu_stats,omitempty"` | ||
MemoryStats MemoryStats `json:"memory_stats,omitempty"` | ||
PidsStats PidsStats `json:"pids_stats,omitempty"` | ||
BlkioStats BlkioStats `json:"blkio_stats,omitempty"` | ||
// the map is in the format "size of hugepage: stats of the hugepage" | ||
HugetlbStats map[string]HugetlbStats `json:"hugetlb_stats,omitempty"` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this is true? Did it change in a kernel version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crosbymichael No, this hasn't changed (see 39279b1), it's a specific issue with kernel memory limiting. This is the case for
kmemcg
limits. Since thekmemcg
limits are integrated into the memory cgroup, I couldn't think of a "nice" way of only setting half of the options.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can make this change now or in this pr because updating memory limits, not kernel limits, is a big usecase and this just stops it. Maybe can we keep the changes small and only do the pid change in this pr and update the other things later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
On Fri, Dec 18, 2015 at 10:22 AM, Michael Crosby notifications@github.com
wrote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crosbymichael
This is how this code has always worked. The only change I've made is separating
.Set()
and.Apply()
. The previous code explicitly did this this way (I actually haven't modified theApply()
method forMemoryGroup
). In other words, I'm not making any change to how theMemoryGroup
works.Memory limits are still being set, they're just being set in
MemoryGroup.Apply()
rather thanMemoryGroup.Set()
.And we can't "only do the pid change in this PR" because we need to
Set()
the cgroup value as late as possible in order for us to use the PIDs cgroup for all legal values. Sure, we could only do late setting for only the PIDs cgroup, but that's just ugly and there are other problems that this solves for other cgroups (the late setting is a common requirement for cgroups).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @crosbymichael @vishh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info @cyphar. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially the workflow is as follows:
As of now
1
and3
are essentially together. We should clean that up soon.