From 0b5581fd28b81cfddfa8da8cc73fccea90dd1f94 Mon Sep 17 00:00:00 2001 From: Phil Estes Date: Sat, 20 Feb 2016 19:29:53 -0600 Subject: [PATCH] Handle memory swappiness as a pointer to handle default/unset case This prior fix to set "-1" explicitly was lost, and it is simpler to use the same pointer type from the OCI spec to handle nil pointer == -1 == unset case. Also, as a nearly humorous aside, there was a test for MemorySwappiness that was actually setting Memory, and it was passing because of this bug (as it was always setting everyone's MemorySwappiness to zero!) Docker-DCO-1.1-Signed-off-by: Phil Estes (github: estesp) --- libcontainer/cgroups/fs/memory.go | 12 ++++++------ libcontainer/cgroups/fs/memory_test.go | 12 +++++------- libcontainer/configs/cgroup_unix.go | 2 +- libcontainer/integration/template_test.go | 2 +- spec.go | 3 ++- 5 files changed, 15 insertions(+), 16 deletions(-) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 2121f6d4..e3fd327e 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -86,14 +86,14 @@ func (s *MemoryGroup) Set(path string, cgroup *configs.Cgroup) error { return err } } - if cgroup.Resources.MemorySwappiness >= 0 && cgroup.Resources.MemorySwappiness <= 100 { - if err := writeFile(path, "memory.swappiness", strconv.FormatInt(cgroup.Resources.MemorySwappiness, 10)); err != nil { + if cgroup.Resources.MemorySwappiness == nil || int64(*cgroup.Resources.MemorySwappiness) == -1 { + return nil + } else if int64(*cgroup.Resources.MemorySwappiness) >= 0 && int64(*cgroup.Resources.MemorySwappiness) <= 100 { + if err := writeFile(path, "memory.swappiness", strconv.FormatInt(*cgroup.Resources.MemorySwappiness, 10)); err != nil { return err } - } else if cgroup.Resources.MemorySwappiness == -1 { - return nil } else { - return fmt.Errorf("invalid value:%d. valid memory swappiness range is 0-100", cgroup.Resources.MemorySwappiness) + return fmt.Errorf("invalid value:%d. valid memory swappiness range is 0-100", int64(*cgroup.Resources.MemorySwappiness)) } return nil @@ -149,7 +149,7 @@ func memoryAssigned(cgroup *configs.Cgroup) bool { cgroup.Resources.MemorySwap > 0 || cgroup.Resources.KernelMemory > 0 || cgroup.Resources.OomKillDisable || - cgroup.Resources.MemorySwappiness != -1 + (cgroup.Resources.MemorySwappiness != nil && *cgroup.Resources.MemorySwappiness != -1) } func getMemoryData(path, name string) (cgroups.MemoryData, error) { diff --git a/libcontainer/cgroups/fs/memory_test.go b/libcontainer/cgroups/fs/memory_test.go index 6dc4ae7c..8321ce97 100644 --- a/libcontainer/cgroups/fs/memory_test.go +++ b/libcontainer/cgroups/fs/memory_test.go @@ -118,16 +118,14 @@ func TestMemorySetMemorySwappinessDefault(t *testing.T) { helper := NewCgroupTestUtil("memory", t) defer helper.cleanup() - const ( - swappinessBefore = 60 //deafult is 60 - swappinessAfter = 0 - ) + swappinessBefore := 60 //default is 60 + swappinessAfter := int64(0) helper.writeFileContents(map[string]string{ "memory.swappiness": strconv.Itoa(swappinessBefore), }) - helper.CgroupData.config.Resources.Memory = swappinessAfter + helper.CgroupData.config.Resources.MemorySwappiness = &swappinessAfter memory := &MemoryGroup{} if err := memory.Set(helper.CgroupPath, helper.CgroupData.config); err != nil { t.Fatal(err) @@ -137,8 +135,8 @@ func TestMemorySetMemorySwappinessDefault(t *testing.T) { if err != nil { t.Fatalf("Failed to parse memory.swappiness - %s", err) } - if value != swappinessAfter { - t.Fatal("Got the wrong value, set memory.swappiness failed.") + if int64(value) != swappinessAfter { + t.Fatalf("Got the wrong value (%d), set memory.swappiness = %d failed.", value, swappinessAfter) } } diff --git a/libcontainer/configs/cgroup_unix.go b/libcontainer/configs/cgroup_unix.go index 40a033f3..2ea00658 100644 --- a/libcontainer/configs/cgroup_unix.go +++ b/libcontainer/configs/cgroup_unix.go @@ -111,7 +111,7 @@ type Resources struct { OomKillDisable bool `json:"oom_kill_disable"` // Tuning swappiness behaviour per cgroup - MemorySwappiness int64 `json:"memory_swappiness"` + MemorySwappiness *int64 `json:"memory_swappiness"` // Set priority of network traffic for container NetPrioIfpriomap []*IfPrioMap `json:"net_prio_ifpriomap"` diff --git a/libcontainer/integration/template_test.go b/libcontainer/integration/template_test.go index 047a5f1e..f54a849a 100644 --- a/libcontainer/integration/template_test.go +++ b/libcontainer/integration/template_test.go @@ -48,7 +48,7 @@ func newTemplateConfig(rootfs string) *configs.Config { Cgroups: &configs.Cgroup{ Path: "integration/test", Resources: &configs.Resources{ - MemorySwappiness: -1, + MemorySwappiness: nil, AllowAllDevices: false, AllowedDevices: configs.DefaultAllowedDevices, }, diff --git a/spec.go b/spec.go index 955678ff..70ccdd81 100644 --- a/spec.go +++ b/spec.go @@ -398,7 +398,8 @@ func createCgroupConfig(name string, spec *specs.LinuxSpec) (*configs.Cgroup, er c.Resources.KernelMemory = int64(*r.Memory.Kernel) } if r.Memory.Swappiness != nil { - c.Resources.MemorySwappiness = int64(*r.Memory.Swappiness) + swappiness := int64(*r.Memory.Swappiness) + c.Resources.MemorySwappiness = &swappiness } } if r.CPU != nil {