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

cgroups: add pids controller support #58

Merged
merged 4 commits into from
Dec 19, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions libcontainer/cgroups/fs/apply_raw.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var (
&MemoryGroup{},
&CpuGroup{},
&CpuacctGroup{},
&PidsGroup{},
&BlkioGroup{},
&HugetlbGroup{},
&NetClsGroup{},
Expand Down Expand Up @@ -179,11 +180,24 @@ func (m *Manager) GetStats() (*cgroups.Stats, error) {
}

func (m *Manager) Set(container *configs.Config) error {
for name, path := range m.Paths {
sys, err := subsystems.Get(name)
if err == errSubsystemDoesNotExist || !cgroups.PathExists(path) {
for _, sys := range subsystems {
// We can't set this here, because after being applied, memcg doesn't
// allow a non-empty cgroup from having its limits changed.
if sys.Name() == "memory" {
Copy link
Member

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?

Copy link
Member Author

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 the kmemcg limits are integrated into the memory cgroup, I couldn't think of a "nice" way of only setting half of the options.

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 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?

Copy link
Contributor

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:

In libcontainer/cgroups/fs/apply_raw.go
#58 (comment):

@@ -179,11 +180,24 @@ func (m _Manager) GetStats() (_cgroups.Stats, error) {
}

func (m *Manager) Set(container *configs.Config) error {

  • for name, path := range m.Paths {
  •   sys, err := subsystems.Get(name)
    
  •   if err == errSubsystemDoesNotExist || !cgroups.PathExists(path) {
    
  • for _, sys := range subsystems {
  •   // We can't set this here, because after being applied, memcg doesn't
    
  •   // allow a non-empty cgroup from having its limits changed.
    
  •   if sys.Name() == "memory" {
    

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?


Reply to this email directly or view it on GitHub
https://github.com/opencontainers/runc/pull/58/files#r48053452.

Copy link
Member Author

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 the Apply() method for MemoryGroup). In other words, I'm not making any change to how the MemoryGroup works.

Memory limits are still being set, they're just being set in MemoryGroup.Apply() rather than MemoryGroup.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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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

Copy link
Contributor

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:

  1. Create cgroups
  2. Set the appropriate limits
  3. Apply cgroups to the init process.

As of now 1 and 3 are essentially together. We should clean that up soon.

continue
}

// Generate fake cgroup data.
d, err := getCgroupData(container.Cgroups, -1)
if err != nil {
return err
}
// Get the path, but don't error out if the cgroup wasn't found.
path, err := d.path(sys.Name())
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := sys.Set(path, container.Cgroups); err != nil {
return err
}
Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/blkio.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,10 @@ func (s *BlkioGroup) Name() string {
}

func (s *BlkioGroup) Apply(d *cgroupData) error {
dir, err := d.join("blkio")
_, err := d.join("blkio")
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/cpu.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,10 @@ func (s *CpuGroup) Name() string {
func (s *CpuGroup) Apply(d *cgroupData) error {
// We always want to join the cpu group, to allow fair cpu scheduling
// on a container basis
dir, err := d.join("cpu")
_, err := d.join("cpu")
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
5 changes: 0 additions & 5 deletions libcontainer/cgroups/fs/cpuset.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,6 @@ func (s *CpusetGroup) ApplyDir(dir string, cgroup *configs.Cgroup, pid int) erro
if err := s.ensureParent(dir, root); err != nil {
return err
}
// the default values inherit from parent cgroup are already set in
// s.ensureParent, cover these if we have our own
if err := s.Set(dir, cgroup); err != nil {
return err
}
// because we are not using d.join we need to place the pid into the procs file
// unlike the other subsystems
if err := writeFile(dir, "cgroup.procs", strconv.Itoa(pid)); err != nil {
Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/devices.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,12 @@ func (s *DevicesGroup) Name() string {
}

func (s *DevicesGroup) Apply(d *cgroupData) error {
dir, err := d.join("devices")
_, err := d.join("devices")
if err != nil {
// We will return error even it's `not found` error, devices
// cgroup is hard requirement for container's security.
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/freezer.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,10 @@ func (s *FreezerGroup) Name() string {
}

func (s *FreezerGroup) Apply(d *cgroupData) error {
dir, err := d.join("freezer")
_, err := d.join("freezer")
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/hugetlb.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,10 @@ func (s *HugetlbGroup) Name() string {
}

func (s *HugetlbGroup) Apply(d *cgroupData) error {
dir, err := d.join("hugetlb")
_, err := d.join("hugetlb")
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
1 change: 0 additions & 1 deletion libcontainer/cgroups/fs/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) {
return err
}
}

if err := s.Set(path, d.config); err != nil {
return err
}
Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/net_cls.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,10 @@ func (s *NetClsGroup) Name() string {
}

func (s *NetClsGroup) Apply(d *cgroupData) error {
dir, err := d.join("net_cls")
_, err := d.join("net_cls")
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
7 changes: 1 addition & 6 deletions libcontainer/cgroups/fs/net_prio.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,10 @@ func (s *NetPrioGroup) Name() string {
}

func (s *NetPrioGroup) Apply(d *cgroupData) error {
dir, err := d.join("net_prio")
_, err := d.join("net_prio")
if err != nil && !cgroups.IsNotFound(err) {
return err
}

if err := s.Set(dir, d.config); err != nil {
return err
}

return nil
}

Expand Down
57 changes: 57 additions & 0 deletions libcontainer/cgroups/fs/pids.go
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
}
83 changes: 83 additions & 0 deletions libcontainer/cgroups/fs/pids_test.go
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)
}
}
6 changes: 6 additions & 0 deletions libcontainer/cgroups/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this field matching the kernel API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is the value of pids.current.

}

type BlkioStatEntry struct {
Major uint64 `json:"major,omitempty"`
Minor uint64 `json:"minor,omitempty"`
Expand Down Expand Up @@ -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"`
Expand Down
Loading