From e9248dd5e6de56e619a1528d14baa1a70b664cc6 Mon Sep 17 00:00:00 2001 From: acetang Date: Tue, 19 Jan 2021 14:56:15 +0800 Subject: [PATCH 1/3] cgroup: fix panic in parse memory.numa_stat strings.SplitN not always return N fields if not staify, sometimes cgroup interface add some custom fields make parse memory.numa_stat fails, it will case panic Signed-off-by: acetang --- libcontainer/cgroups/fs/memory.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 7fde663d9..22dff1a47 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -294,6 +294,9 @@ func getPageUsageByNUMA(cgroupPath string) (cgroups.PageUsageByNUMA, error) { for _, column := range columns { pagesByNode := strings.SplitN(column, numaStatKeyValueSeparator, numaStatColumnSliceLength) + if len(pagesByNode) != numaStatColumnSliceLength { + continue + } if strings.HasPrefix(pagesByNode[numaStatTypeIndex], numaNodeSymbol) { nodeID, err := strconv.ParseUint(pagesByNode[numaStatTypeIndex][1:], 10, 8) From 63c44e27f604c4f685c83c25b462330ba23822e0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 20 Jan 2021 14:06:47 -0800 Subject: [PATCH 2/3] libct/cg/fs: getPageUsageByNUMA: rewrite/optimize Rewrite getPageUsageByNUMA 1. Be less strict to unknown contents, i.e. skip it. This makes the function more future-proof. Before this commit, if a line like "a=b" is encountered, the function returns an error, which is propagated all the way up to and returned by (CgroupManager).GetStats. 2. Be more strict to contents it recognizes, i.e. return an error. In case the first field in the line is recognized (e.g. "total=123", the rest of the line should be in format "N= ...". 3. Optimize. Before this commit, addNUMAStatsByType was called for every item in the line, which is excessive and might even be slow in case there are many NUMA nodes. It is enough to look up the field once. 4. Remove a bunch of global numaNode* and numaStat* constants. Those were used by only one function, and it does not make sense to have them defined globally. Some were moved to the function, some were eliminated entirely. 5. Improve readability and added code comments. Finally, add some test cases for good and bad contents. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/memory.go | 107 +++++++++++++++---------- libcontainer/cgroups/fs/memory_test.go | 58 +++++++++++++- 2 files changed, 118 insertions(+), 47 deletions(-) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 22dff1a47..61be6d7e9 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -17,16 +17,8 @@ import ( ) const ( - numaNodeSymbol = "N" - numaStatColumnSeparator = " " - numaStatKeyValueSeparator = "=" - numaStatMaxColumns = math.MaxUint8 + 1 - numaStatValueIndex = 1 - numaStatTypeIndex = 0 - numaStatColumnSliceLength = 2 - cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes" - cgroupMemoryLimit = "memory.limit_in_bytes" - cgroupMemoryPagesByNuma = "memory.numa_stat" + cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes" + cgroupMemoryLimit = "memory.limit_in_bytes" ) type MemoryGroup struct { @@ -277,50 +269,81 @@ func getMemoryData(path, name string) (cgroups.MemoryData, error) { } func getPageUsageByNUMA(cgroupPath string) (cgroups.PageUsageByNUMA, error) { + const ( + maxColumns = math.MaxUint8 + 1 + filename = "memory.numa_stat" + ) stats := cgroups.PageUsageByNUMA{} - file, err := fscommon.OpenFile(cgroupPath, cgroupMemoryPagesByNuma, os.O_RDONLY) + file, err := fscommon.OpenFile(cgroupPath, filename, os.O_RDONLY) if os.IsNotExist(err) { return stats, nil } else if err != nil { return stats, err } + // File format is documented in linux/Documentation/cgroup-v1/memory.txt + // and it looks like this: + // + // total= N0= N1= ... + // file= N0= N1= ... + // anon= N0= N1= ... + // unevictable= N0= N1= ... + // hierarchical_= N0= N1= ... + scanner := bufio.NewScanner(file) for scanner.Scan() { - var statsType string - statsByType := cgroups.PageStats{Nodes: map[uint8]uint64{}} - columns := strings.SplitN(scanner.Text(), numaStatColumnSeparator, numaStatMaxColumns) + var field *cgroups.PageStats - for _, column := range columns { - pagesByNode := strings.SplitN(column, numaStatKeyValueSeparator, numaStatColumnSliceLength) - if len(pagesByNode) != numaStatColumnSliceLength { - continue + line := scanner.Text() + columns := strings.SplitN(line, " ", maxColumns) + for i, column := range columns { + byNode := strings.SplitN(column, "=", 2) + // Some custom kernels have non-standard fields, like + // numa_locality 0 0 0 0 0 0 0 0 0 0 + // numa_exectime 0 + if len(byNode) < 2 { + if i == 0 { + // Ignore/skip those. + break + } else { + // The first column was already validated, + // so be strict to the rest. + return stats, fmt.Errorf("malformed line %q in %s", + line, filename) + } } + key, val := byNode[0], byNode[1] + if i == 0 { // First column: key is name, val is total. + field = getNUMAField(&stats, key) + if field == nil { // unknown field (new kernel?) + break + } + field.Total, err = strconv.ParseUint(val, 0, 64) + if err != nil { + return stats, err + } + field.Nodes = map[uint8]uint64{} + } else { // Subsequent columns: key is N, val is usage. + if len(key) < 2 || key[0] != 'N' { + // This is definitely an error. + return stats, fmt.Errorf("malformed line %q in %s", + line, filename) + } - if strings.HasPrefix(pagesByNode[numaStatTypeIndex], numaNodeSymbol) { - nodeID, err := strconv.ParseUint(pagesByNode[numaStatTypeIndex][1:], 10, 8) + n, err := strconv.ParseUint(key[1:], 10, 8) if err != nil { return cgroups.PageUsageByNUMA{}, err } - statsByType.Nodes[uint8(nodeID)], err = strconv.ParseUint(pagesByNode[numaStatValueIndex], 0, 64) - if err != nil { - return cgroups.PageUsageByNUMA{}, err - } - } else { - statsByType.Total, err = strconv.ParseUint(pagesByNode[numaStatValueIndex], 0, 64) + usage, err := strconv.ParseUint(val, 10, 64) if err != nil { return cgroups.PageUsageByNUMA{}, err } - statsType = pagesByNode[numaStatTypeIndex] + field.Nodes[uint8(n)] = usage } - err := addNUMAStatsByType(&stats, statsByType, statsType) - if err != nil { - return cgroups.PageUsageByNUMA{}, err - } } } err = scanner.Err() @@ -331,26 +354,24 @@ func getPageUsageByNUMA(cgroupPath string) (cgroups.PageUsageByNUMA, error) { return stats, nil } -func addNUMAStatsByType(stats *cgroups.PageUsageByNUMA, byTypeStats cgroups.PageStats, statsType string) error { - switch statsType { +func getNUMAField(stats *cgroups.PageUsageByNUMA, name string) *cgroups.PageStats { + switch name { case "total": - stats.Total = byTypeStats + return &stats.Total case "file": - stats.File = byTypeStats + return &stats.File case "anon": - stats.Anon = byTypeStats + return &stats.Anon case "unevictable": - stats.Unevictable = byTypeStats + return &stats.Unevictable case "hierarchical_total": - stats.Hierarchical.Total = byTypeStats + return &stats.Hierarchical.Total case "hierarchical_file": - stats.Hierarchical.File = byTypeStats + return &stats.Hierarchical.File case "hierarchical_anon": - stats.Hierarchical.Anon = byTypeStats + return &stats.Hierarchical.Anon case "hierarchical_unevictable": - stats.Hierarchical.Unevictable = byTypeStats - default: - return fmt.Errorf("unsupported NUMA page type found: %s", statsType) + return &stats.Hierarchical.Unevictable } return nil } diff --git a/libcontainer/cgroups/fs/memory_test.go b/libcontainer/cgroups/fs/memory_test.go index 52244fc7b..00fb338b7 100644 --- a/libcontainer/cgroups/fs/memory_test.go +++ b/libcontainer/cgroups/fs/memory_test.go @@ -25,11 +25,18 @@ unevictable=0 N0=0 N1=0 N2=0 N3=0 hierarchical_total=768133 N0=509113 N1=138887 N2=20464 N3=99669 hierarchical_file=722017 N0=496516 N1=119997 N2=20181 N3=85323 hierarchical_anon=46096 N0=12597 N1=18890 N2=283 N3=14326 -hierarchical_unevictable=20 N0=0 N1=0 N2=0 N3=20` +hierarchical_unevictable=20 N0=0 N1=0 N2=0 N3=20 +` memoryNUMAStatNoHierarchyContents = `total=44611 N0=32631 N1=7501 N2=1982 N3=2497 file=44428 N0=32614 N1=7335 N2=1982 N3=2497 anon=183 N0=17 N1=166 N2=0 N3=0 -unevictable=0 N0=0 N1=0 N2=0 N3=0` +unevictable=0 N0=0 N1=0 N2=0 N3=0 +` + // Some custom kernels has extra fields that should be ignored + memoryNUMAStatExtraContents = `numa_locality 0 0 0 0 0 0 0 0 0 0 +numa_exectime 0 +whatever=100 N0=0 +` ) func TestMemorySetMemory(t *testing.T) { @@ -288,7 +295,7 @@ func TestMemoryStats(t *testing.T) { "memory.kmem.failcnt": memoryFailcnt, "memory.kmem.limit_in_bytes": memoryLimitContents, "memory.use_hierarchy": memoryUseHierarchyContents, - "memory.numa_stat": memoryNUMAStatContents, + "memory.numa_stat": memoryNUMAStatContents + memoryNUMAStatExtraContents, }) memory := &MemoryGroup{} @@ -486,7 +493,7 @@ func TestNoHierarchicalNumaStat(t *testing.T) { helper := NewCgroupTestUtil("memory", t) defer helper.cleanup() helper.writeFileContents(map[string]string{ - "memory.numa_stat": memoryNUMAStatNoHierarchyContents, + "memory.numa_stat": memoryNUMAStatNoHierarchyContents + memoryNUMAStatExtraContents, }) actualStats, err := getPageUsageByNUMA(helper.CgroupPath) @@ -505,6 +512,49 @@ func TestNoHierarchicalNumaStat(t *testing.T) { expectPageUsageByNUMAEquals(t, pageUsageByNUMA, actualStats) } +func TestBadNumaStat(t *testing.T) { + memoryNUMAStatBadContents := []struct { + desc, contents string + }{ + { + desc: "Nx where x is not a number", + contents: `total=44611 N0=44611, +file=44428 Nx=0 +`, + }, { + desc: "Nx where x > 255", + contents: `total=44611 N333=444`, + }, { + desc: "Nx argument missing", + contents: `total=44611 N0=123 N1=`, + }, { + desc: "Nx argument is not a number", + contents: `total=44611 N0=123 N1=a`, + }, { + desc: "Missing = after Nx", + contents: `total=44611 N0=123 N1`, + }, { + desc: "No Nx at non-first position", + contents: `total=44611 N0=32631 +file=44428 N0=32614 +anon=183 N0=12 badone +`, + }, + } + helper := NewCgroupTestUtil("memory", t) + defer helper.cleanup() + for _, c := range memoryNUMAStatBadContents { + helper.writeFileContents(map[string]string{ + "memory.numa_stat": c.contents, + }) + + _, err := getPageUsageByNUMA(helper.CgroupPath) + if err == nil { + t.Errorf("case %q: expected error, got nil", c.desc) + } + } +} + func TestWithoutNumaStat(t *testing.T) { helper := NewCgroupTestUtil("memory", t) defer helper.cleanup() From 6a9f5ac9d4d9116290d08d0cec7d81b0c463a6fe Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 20 Jan 2021 14:19:33 -0800 Subject: [PATCH 3/3] libct/cg/fs: fix a linter warning It was already explained why we ignore the error, so let's ignore this deliberately. This fixes > name.go:22:7: Error return value of `join` is not checked (errcheck) Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/name.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/cgroups/fs/name.go b/libcontainer/cgroups/fs/name.go index 9a5af709d..7cada9149 100644 --- a/libcontainer/cgroups/fs/name.go +++ b/libcontainer/cgroups/fs/name.go @@ -19,7 +19,7 @@ func (s *NameGroup) Name() string { func (s *NameGroup) Apply(path string, d *cgroupData) error { if s.Join { // ignore errors if the named cgroup does not exist - join(path, d.pid) + _ = join(path, d.pid) } return nil }