From cf275c85f259d6ca47586a823a2ca85e0afe13e2 Mon Sep 17 00:00:00 2001 From: Mario Macias Date: Mon, 23 Sep 2019 13:23:08 +0200 Subject: [PATCH 1/3] Allows ignoring memory modules Some hosts may have disabled some memory modules (e.g. `memsw` metrics are not available if the Kernel is built without the `CONFIG_MEMCG_SWAP_ENABLED` variable. This made the memory controller `Stat` function to not work properly, either because it returns error when a single memory entry is not available. Even if you run `cgroups.Stat(cgroups.IgnoreNotExist)` to ignore memory Stat errors, you won't get most of the memory metrics. This patch adds a configuration function parameter to the `NewMemory` constructor. This way, we don't introduce breaking changes in the current API. You can invoke the `NewMemory` constructor as usual or with a new optional configuration option: ```go mem := NewMemory(root, IgnoreModules("memsw")) ``` Signed-off-by: Mario Macias --- memory.go | 29 +++++++++++-- memory_test.go | 113 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 4 deletions(-) diff --git a/memory.go b/memory.go index aae14e0a..b5b46219 100644 --- a/memory.go +++ b/memory.go @@ -32,14 +32,32 @@ import ( "golang.org/x/sys/unix" ) -func NewMemory(root string) *memoryController { - return &memoryController{ - root: filepath.Join(root, string(Memory)), +// NewMemory returns a Memory controller given the root folder of cgroups. +// It may optionally accept other configuration options, such as IgnoreModules(...) +func NewMemory(root string, options ...func(*memoryController)) *memoryController { + mc := &memoryController{ + root: filepath.Join(root, string(Memory)), + ignored: map[string]struct{}{}, + } + for _, opt := range options { + opt(mc) + } + return mc +} + +// IgnoreModules configure the memory controller to not read memory metrics for some +// module names (e.g. passing "memsw" would avoid all the memory.memsw.* entries) +func IgnoreModules(names ...string) func(*memoryController) { + return func(mc *memoryController) { + for _, name := range names { + mc.ignored[name] = struct{}{} + } } } type memoryController struct { - root string + root string + ignored map[string]struct{} } func (m *memoryController) Name() Name { @@ -133,6 +151,9 @@ func (m *memoryController) Stat(path string, stats *v1.Metrics) error { entry: stats.Memory.KernelTCP, }, } { + if _, ok := m.ignored[t.module]; ok { + continue + } for _, tt := range []struct { name string value *uint64 diff --git a/memory_test.go b/memory_test.go index 6f5ef165..fb2e156e 100644 --- a/memory_test.go +++ b/memory_test.go @@ -17,6 +17,10 @@ package cgroups import ( + "fmt" + "io/ioutil" + "os" + "path" "strings" "testing" @@ -106,3 +110,112 @@ func TestParseMemoryStats(t *testing.T) { } } } + +func TestMemoryController_Stat(t *testing.T) { + modules := []string{"", "memsw", "kmem", "kmem.tcp"} + metrics := []string{"usage_in_bytes", "max_usage_in_bytes", "failcnt", "limit_in_bytes"} + tmpRoot := buildMemoryMetrics(t, modules, metrics) + + // checks that all the the cgroups memory entries are read + mc := NewMemory(tmpRoot) + stats := v1.Metrics{} + + if err := mc.Stat("", &stats); err != nil { + t.Errorf("can't get stats: %v", err) + } + + index := []uint64{ + stats.Memory.Usage.Usage, + stats.Memory.Usage.Max, + stats.Memory.Usage.Failcnt, + stats.Memory.Usage.Limit, + stats.Memory.Swap.Usage, + stats.Memory.Swap.Max, + stats.Memory.Swap.Failcnt, + stats.Memory.Swap.Limit, + stats.Memory.Kernel.Usage, + stats.Memory.Kernel.Max, + stats.Memory.Kernel.Failcnt, + stats.Memory.Kernel.Limit, + stats.Memory.KernelTCP.Usage, + stats.Memory.KernelTCP.Max, + stats.Memory.KernelTCP.Failcnt, + stats.Memory.KernelTCP.Limit, + } + for i, v := range index { + if v != uint64(i) { + t.Errorf("expected value at index %d to be %d but received %d", i, i, v) + } + } +} + +func TestMemoryController_Stat_Ignore(t *testing.T) { + modules := []string{"", "kmem", "kmem.tcp"} + metrics := []string{"usage_in_bytes", "max_usage_in_bytes", "failcnt", "limit_in_bytes"} + tmpRoot := buildMemoryMetrics(t, modules, metrics) + + // checks that the cgroups memory entry is parsed and the memsw is ignored + mc := NewMemory(tmpRoot, IgnoreModules("memsw")) + stats := v1.Metrics{} + + if err := mc.Stat("", &stats); err != nil { + t.Errorf("can't get stats: %v", err) + } + + mem := stats.Memory + if mem.Swap.Usage != 0 || mem.Swap.Limit != 0 || + mem.Swap.Max != 0 || mem.Swap.Failcnt != 0 { + t.Errorf("swap memory should have been ignored. Got: %+v", mem.Swap) + } + + index := []uint64{ + stats.Memory.Usage.Usage, + stats.Memory.Usage.Max, + stats.Memory.Usage.Failcnt, + stats.Memory.Usage.Limit, + stats.Memory.Kernel.Usage, + stats.Memory.Kernel.Max, + stats.Memory.Kernel.Failcnt, + stats.Memory.Kernel.Limit, + stats.Memory.KernelTCP.Usage, + stats.Memory.KernelTCP.Max, + stats.Memory.KernelTCP.Failcnt, + stats.Memory.KernelTCP.Limit, + } + for i, v := range index { + if v != uint64(i) { + t.Errorf("expected value at index %d to be %d but received %d", i, i, v) + } + } +} + +// buildMemoryMetrics creates fake cgroups memory entries in a temporary dir. Returns the fake cgroups root +func buildMemoryMetrics(t *testing.T, modules []string, metrics []string) string { + tmpRoot, err := ioutil.TempDir("", "memtests") + if err != nil { + t.Fatal(err) + } + tmpDir := path.Join(tmpRoot, string(Memory)) + if err := os.MkdirAll(tmpDir, os.ModeDir); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(path.Join(tmpDir, "memory.stat"), []byte(memoryData), 0666); err != nil { + t.Fatal(err) + } + cnt := 0 + for _, mod := range modules { + for _, metric := range metrics { + var fileName string + if mod == "" { + fileName = path.Join(tmpDir, strings.Join([]string{"memory", metric}, ".")) + } else { + fileName = path.Join(tmpDir, strings.Join([]string{"memory", mod, metric}, ".")) + } + if err := ioutil.WriteFile(fileName, []byte(fmt.Sprintln(cnt)), 0666); err != nil { + t.Fatal(err) + } + cnt++ + } + } + return tmpRoot +} From fa1a76b28faaff678184549171de676315645c09 Mon Sep 17 00:00:00 2001 From: Mario Macias Date: Mon, 23 Sep 2019 14:19:40 +0200 Subject: [PATCH 2/3] Fixed test file permissions Signed-off-by: Mario Macias --- memory_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/memory_test.go b/memory_test.go index fb2e156e..fad41c0b 100644 --- a/memory_test.go +++ b/memory_test.go @@ -196,10 +196,10 @@ func buildMemoryMetrics(t *testing.T, modules []string, metrics []string) string t.Fatal(err) } tmpDir := path.Join(tmpRoot, string(Memory)) - if err := os.MkdirAll(tmpDir, os.ModeDir); err != nil { + if err := os.MkdirAll(tmpDir, defaultDirPerm); err != nil { t.Fatal(err) } - if err := ioutil.WriteFile(path.Join(tmpDir, "memory.stat"), []byte(memoryData), 0666); err != nil { + if err := ioutil.WriteFile(path.Join(tmpDir, "memory.stat"), []byte(memoryData), defaultFilePerm); err != nil { t.Fatal(err) } cnt := 0 @@ -211,7 +211,7 @@ func buildMemoryMetrics(t *testing.T, modules []string, metrics []string) string } else { fileName = path.Join(tmpDir, strings.Join([]string{"memory", mod, metric}, ".")) } - if err := ioutil.WriteFile(fileName, []byte(fmt.Sprintln(cnt)), 0666); err != nil { + if err := ioutil.WriteFile(fileName, []byte(fmt.Sprintln(cnt)), defaultFilePerm); err != nil { t.Fatal(err) } cnt++ From ad1c4b91138ced79481a43fd94a4c123daad72f1 Mon Sep 17 00:00:00 2001 From: Mario Macias Date: Mon, 23 Sep 2019 17:50:07 +0200 Subject: [PATCH 3/3] Added OptionalSwap memory constructor option Signed-off-by: Mario Macias --- memory.go | 11 +++++ memory_test.go | 124 +++++++++++++++++++++++++++++++++---------------- 2 files changed, 94 insertions(+), 41 deletions(-) diff --git a/memory.go b/memory.go index b5b46219..9d55b4e5 100644 --- a/memory.go +++ b/memory.go @@ -55,6 +55,17 @@ func IgnoreModules(names ...string) func(*memoryController) { } } +// OptionalSwap allows the memory controller to not fail if cgroups is not accounting +// Swap memory (there are no memory.memsw.* entries) +func OptionalSwap() func(*memoryController) { + return func(mc *memoryController) { + _, err := os.Stat(filepath.Join(mc.root, "memory.memsw.usage_in_bytes")) + if os.IsNotExist(err) { + mc.ignored["memsw"] = struct{}{} + } + } +} + type memoryController struct { root string ignored map[string]struct{} diff --git a/memory_test.go b/memory_test.go index fad41c0b..16f3ba6a 100644 --- a/memory_test.go +++ b/memory_test.go @@ -112,75 +112,117 @@ func TestParseMemoryStats(t *testing.T) { } func TestMemoryController_Stat(t *testing.T) { + // GIVEN a cgroups folder with all the memory metrics modules := []string{"", "memsw", "kmem", "kmem.tcp"} metrics := []string{"usage_in_bytes", "max_usage_in_bytes", "failcnt", "limit_in_bytes"} tmpRoot := buildMemoryMetrics(t, modules, metrics) - // checks that all the the cgroups memory entries are read + // WHEN the memory controller reads the metrics stats mc := NewMemory(tmpRoot) stats := v1.Metrics{} - if err := mc.Stat("", &stats); err != nil { t.Errorf("can't get stats: %v", err) } - index := []uint64{ - stats.Memory.Usage.Usage, - stats.Memory.Usage.Max, - stats.Memory.Usage.Failcnt, - stats.Memory.Usage.Limit, - stats.Memory.Swap.Usage, - stats.Memory.Swap.Max, - stats.Memory.Swap.Failcnt, - stats.Memory.Swap.Limit, - stats.Memory.Kernel.Usage, - stats.Memory.Kernel.Max, - stats.Memory.Kernel.Failcnt, - stats.Memory.Kernel.Limit, - stats.Memory.KernelTCP.Usage, - stats.Memory.KernelTCP.Max, - stats.Memory.KernelTCP.Failcnt, - stats.Memory.KernelTCP.Limit, - } - for i, v := range index { - if v != uint64(i) { - t.Errorf("expected value at index %d to be %d but received %d", i, i, v) - } - } + // THEN all the memory stats have been completely loaded in memory + checkMemoryStatIsComplete(t, stats.Memory) } -func TestMemoryController_Stat_Ignore(t *testing.T) { +func TestMemoryController_Stat_IgnoreModules(t *testing.T) { + // GIVEN a cgroups folder that accounts for all the metrics BUT swap memory modules := []string{"", "kmem", "kmem.tcp"} metrics := []string{"usage_in_bytes", "max_usage_in_bytes", "failcnt", "limit_in_bytes"} tmpRoot := buildMemoryMetrics(t, modules, metrics) - // checks that the cgroups memory entry is parsed and the memsw is ignored + // WHEN the memory controller explicitly ignores memsw module and reads the data mc := NewMemory(tmpRoot, IgnoreModules("memsw")) stats := v1.Metrics{} + if err := mc.Stat("", &stats); err != nil { + t.Errorf("can't get stats: %v", err) + } + // THEN the swap memory stats are not loaded but all the other memory metrics are + checkMemoryStatHasNoSwap(t, stats.Memory) +} + +func TestMemoryController_Stat_OptionalSwap_HasSwap(t *testing.T) { + // GIVEN a cgroups folder with all the memory metrics + modules := []string{"", "memsw", "kmem", "kmem.tcp"} + metrics := []string{"usage_in_bytes", "max_usage_in_bytes", "failcnt", "limit_in_bytes"} + tmpRoot := buildMemoryMetrics(t, modules, metrics) + + // WHEN a memory controller that ignores swap only if it is missing reads stats + mc := NewMemory(tmpRoot, OptionalSwap()) + stats := v1.Metrics{} if err := mc.Stat("", &stats); err != nil { t.Errorf("can't get stats: %v", err) } - mem := stats.Memory + // THEN all the memory stats have been completely loaded in memory + checkMemoryStatIsComplete(t, stats.Memory) +} + +func TestMemoryController_Stat_OptionalSwap_NoSwap(t *testing.T) { + // GIVEN a cgroups folder that accounts for all the metrics BUT swap memory + modules := []string{"", "kmem", "kmem.tcp"} + metrics := []string{"usage_in_bytes", "max_usage_in_bytes", "failcnt", "limit_in_bytes"} + tmpRoot := buildMemoryMetrics(t, modules, metrics) + + // WHEN a memory controller that ignores swap only if it is missing reads stats + mc := NewMemory(tmpRoot, OptionalSwap()) + stats := v1.Metrics{} + if err := mc.Stat("", &stats); err != nil { + t.Errorf("can't get stats: %v", err) + } + + // THEN the swap memory stats are not loaded but all the other memory metrics are + checkMemoryStatHasNoSwap(t, stats.Memory) +} + +func checkMemoryStatIsComplete(t *testing.T, mem *v1.MemoryStat) { + index := []uint64{ + mem.Usage.Usage, + mem.Usage.Max, + mem.Usage.Failcnt, + mem.Usage.Limit, + mem.Swap.Usage, + mem.Swap.Max, + mem.Swap.Failcnt, + mem.Swap.Limit, + mem.Kernel.Usage, + mem.Kernel.Max, + mem.Kernel.Failcnt, + mem.Kernel.Limit, + mem.KernelTCP.Usage, + mem.KernelTCP.Max, + mem.KernelTCP.Failcnt, + mem.KernelTCP.Limit, + } + for i, v := range index { + if v != uint64(i) { + t.Errorf("expected value at index %d to be %d but received %d", i, i, v) + } + } +} + +func checkMemoryStatHasNoSwap(t *testing.T, mem *v1.MemoryStat) { if mem.Swap.Usage != 0 || mem.Swap.Limit != 0 || mem.Swap.Max != 0 || mem.Swap.Failcnt != 0 { t.Errorf("swap memory should have been ignored. Got: %+v", mem.Swap) } - index := []uint64{ - stats.Memory.Usage.Usage, - stats.Memory.Usage.Max, - stats.Memory.Usage.Failcnt, - stats.Memory.Usage.Limit, - stats.Memory.Kernel.Usage, - stats.Memory.Kernel.Max, - stats.Memory.Kernel.Failcnt, - stats.Memory.Kernel.Limit, - stats.Memory.KernelTCP.Usage, - stats.Memory.KernelTCP.Max, - stats.Memory.KernelTCP.Failcnt, - stats.Memory.KernelTCP.Limit, + mem.Usage.Usage, + mem.Usage.Max, + mem.Usage.Failcnt, + mem.Usage.Limit, + mem.Kernel.Usage, + mem.Kernel.Max, + mem.Kernel.Failcnt, + mem.Kernel.Limit, + mem.KernelTCP.Usage, + mem.KernelTCP.Max, + mem.KernelTCP.Failcnt, + mem.KernelTCP.Limit, } for i, v := range index { if v != uint64(i) {