From 313df572d7b6ef399073eb50372943c75ac06f40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Wed, 28 May 2025 13:56:56 +0200 Subject: [PATCH 01/12] Add benchmark for cgroup.SubsystemMountpoints --- metric/system/cgroup/util_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/metric/system/cgroup/util_test.go b/metric/system/cgroup/util_test.go index 4ed0adc175..db5e479092 100644 --- a/metric/system/cgroup/util_test.go +++ b/metric/system/cgroup/util_test.go @@ -30,6 +30,7 @@ import ( "github.com/stretchr/testify/require" "github.com/elastic/elastic-agent-libs/logp" + "github.com/elastic/elastic-agent-system-metrics/metric/system/cgroup/testhelpers" "github.com/elastic/elastic-agent-system-metrics/metric/system/resolve" ) @@ -155,6 +156,32 @@ func TestSubsystemMountpoints(t *testing.T) { assert.Equal(t, "testdata/docker/sys/fs/cgroup/perf_event", mountpoints.V1Mounts["perf_event"]) } +func BenchmarkSubsystemMountpoints(b *testing.B) { + subsystems := map[string]struct{}{ + "blkio": {}, + "cpu": {}, + "cpuacct": {}, + "cpuset": {}, + "devices": {}, + "freezer": {}, + "hugetlb": {}, + "memory": {}, + "perf_event": {}, + } + + resolver := resolve.NewTestResolver("testdata/docker") + + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + _, err := SubsystemMountpoints(resolver, subsystems) + if err != nil { + b.Fatalf("error in SubsystemMountpoints: %s", err) + } + } +} + func TestProcessCgroupPaths(t *testing.T) { reader, err := NewReader(resolve.NewTestResolver("testdata/docker"), false) if err != nil { From cc9437052b2c7a894818582bfb40147978ae7455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Wed, 28 May 2025 18:17:23 +0200 Subject: [PATCH 02/12] Reduce heap allocations in SubsystemMountpoints() --- metric/system/cgroup/stringutil/stringutil.go | 107 ++++++++++++++++++ metric/system/cgroup/util.go | 68 +++++++---- 2 files changed, 153 insertions(+), 22 deletions(-) create mode 100644 metric/system/cgroup/stringutil/stringutil.go diff --git a/metric/system/cgroup/stringutil/stringutil.go b/metric/system/cgroup/stringutil/stringutil.go new file mode 100644 index 0000000000..7669f21ffe --- /dev/null +++ b/metric/system/cgroup/stringutil/stringutil.go @@ -0,0 +1,107 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package stringutil + +import ( + "strings" + "unsafe" +) + +var asciiSpace = [256]uint8{'\t': 1, '\n': 1, '\v': 1, '\f': 1, '\r': 1, ' ': 1} + +// FieldsN splits the string s around each instance of one or more consecutive space +// characters, filling f with substrings of s. +// If s contains more fields than n, the last element of f is set to the +// unparsed remainder of s starting with the first non-space character. +// f will stay untouched if s is empty or contains only white space. +// if n is greater than len(f), 0 is returned without doing any parsing. +// +// Apart from the mentioned differences, FieldsN is like an allocation-free strings.Fields. +func FieldsN(s string, f []string) int { + n := len(f) + si := 0 + for i := 0; i < n-1; i++ { + // Find the start of the next field. + for si < len(s) && asciiSpace[s[si]] != 0 { + si++ + } + fieldStart := si + + // Find the end of the field. + for si < len(s) && asciiSpace[s[si]] == 0 { + si++ + } + if fieldStart >= si { + return i + } + + f[i] = s[fieldStart:si] + } + + // Find the start of the next field. + for si < len(s) && asciiSpace[s[si]] != 0 { + si++ + } + + // Put the remainder of s as last element of f. + if si < len(s) { + f[n-1] = s[si:] + return n + } + + return n - 1 +} + +// SplitN splits the string around each instance of sep, filling f with substrings of s. +// If s contains more fields than n, the last element of f is set to the +// unparsed remainder of s starting with the first non-space character. +// f will stay untouched if s is empty or contains only white space. +// if n is greater than len(f), 0 is returned without doing any parsing. +// +// Apart from the mentioned differences, SplitN is like an allocation-free strings.SplitN. +func SplitN(s, sep string, f []string) int { + n := len(f) + i := 0 + for ; i < n-1 && s != ""; i++ { + fieldEnd := strings.Index(s, sep) + if fieldEnd < 0 { + f[i] = s + return i + 1 + } + f[i] = s[:fieldEnd] + s = s[fieldEnd+len(sep):] + } + + // Put the remainder of s as last element of f. + f[i] = s + return i + 1 +} + +// ByteSlice2String converts a byte slice into a string without a heap allocation. +// Be aware that the byte slice and the string share the same memory - which makes +// the string mutable. +func ByteSlice2String(b []byte) string { + return *(*string)(unsafe.Pointer(&b)) +} + +func SplitInline(s, sep string) []string { + // Use a fixed-size slice to avoid allocations. + fields := make([]string, strings.Count(s, sep)+1) + SplitN(s, sep, fields) + return fields +} diff --git a/metric/system/cgroup/util.go b/metric/system/cgroup/util.go index 0e31fd6ad3..8aba8e5b45 100644 --- a/metric/system/cgroup/util.go +++ b/metric/system/cgroup/util.go @@ -30,6 +30,8 @@ import ( "time" "github.com/elastic/elastic-agent-libs/logp" + + "github.com/elastic/elastic-agent-system-metrics/metric/system/cgroup/stringutil" "github.com/elastic/elastic-agent-system-metrics/metric/system/resolve" ) @@ -120,35 +122,36 @@ func (pl PathList) Flatten() []ControllerPath { func parseMountinfoLine(line string) (mountinfo, error) { mount := mountinfo{} - fields := strings.Fields(line) - if len(fields) < 10 { + var fields [10 + 6]string // support up to 6 optional fields + + nFields := stringutil.FieldsN(line, fields[:]) + if nFields < 10 { return mount, fmt.Errorf("invalid mountinfo line, expected at least "+ - "10 fields but got %d from line='%s'", len(fields), line) + "10 fields but got %d from line='%s'", nFields, line) } - mount.mountpoint = fields[4] - var separatorIndex int - for i, value := range fields { - if value == "-" { + for i := 6; i < nFields; i++ { + if fields[i] == "-" { separatorIndex = i break } } - if fields[separatorIndex] != "-" { + if separatorIndex == 0 { return mount, fmt.Errorf("invalid mountinfo line, separator ('-') not "+ "found in line='%s'", line) } - - if len(fields)-separatorIndex-1 < 3 { + if nFields-separatorIndex-1 < 3 { return mount, fmt.Errorf("invalid mountinfo line, expected at least "+ "3 fields after separator but got %d from line='%s'", - len(fields)-separatorIndex-1, line) + nFields-separatorIndex-1, line) } - fields = fields[separatorIndex+1:] - mount.filesystemType = fields[0] - mount.superOptions = strings.Split(fields[2], ",") + mount.mountpoint = fields[4] + mount.filesystemType = fields[separatorIndex+1] + if mount.filesystemType == "cgroup" { + mount.superOptions = stringutil.SplitInline(fields[separatorIndex+3], ",") + } return mount, nil } @@ -210,6 +213,7 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ } defer mountinfo.Close() + hostFS := rootfs.ResolveHostFS("") mounts := map[string]string{} mountInfo := Mountpoints{} sc := bufio.NewScanner(mountinfo) @@ -218,7 +222,11 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ // https://www.kernel.org/doc/Documentation/filesystems/proc.txt // Example: // 25 21 0:20 / /cgroup/cpu rw,relatime - cgroup cgroup rw,cpu - line := strings.TrimSpace(sc.Text()) + + // Avoid heap allocation by not using scanner.Text(). + // NOTE: The underlying bytes will change with the next call to scanner.Scan(), + // so make sure to not keep any references after the end of the loop iteration. + line := strings.TrimSpace(stringutil.ByteSlice2String(sc.Bytes())) if line == "" { continue } @@ -229,7 +237,7 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ } // if the mountpoint from the subsystem has a different root than ours, it probably belongs to something else. - if !strings.HasPrefix(mount.mountpoint, rootfs.ResolveHostFS("")) { + if !strings.HasPrefix(mount.mountpoint, hostFS) { continue } @@ -237,8 +245,8 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ if mount.filesystemType == "cgroup" { for _, opt := range mount.superOptions { // Sometimes the subsystem name is written like "name=blkio". - fields := strings.SplitN(opt, "=", 2) - if len(fields) > 1 { + var fields [2]string + if n := stringutil.SplitN(opt, "=", fields[:]); n > 1 { opt = fields[1] } @@ -246,7 +254,7 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ if _, found := subsystems[opt]; found { // Add the subsystem mount if it does not already exist. if _, exists := mounts[opt]; !exists { - mounts[opt] = mount.mountpoint + mounts[opt] = strings.Clone(mount.mountpoint) } } } @@ -254,7 +262,7 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ // V2 option if mount.filesystemType == "cgroup2" { - possibleV2Paths = append(possibleV2Paths, mount.mountpoint) + possibleV2Paths = append(possibleV2Paths, strings.Clone(mount.mountpoint)) } } @@ -290,9 +298,25 @@ func isCgroupNSPrivate() bool { } // if we have a path of just "/" that means we're in our own private namespace // if it's something else, we're probably in a host namespace - segments := strings.Split(strings.TrimSpace(string(raw)), ":") - return segments[len(segments)-1] == "/" + colons := 2 + for i := range trimBytes(raw) { + if raw[i] == ':' { + colons++ + if colons == 2 { + // if we have a second colon, check if the next character is a slash + return len(raw) == i+2 && raw[i+1] == '/' + } + } + } + return false +} +func trimBytes(b []byte) []byte { + // trim the trailing newlines + for i := len(b) - 1; i >= 0 && b[i] == '\n'; i-- { + b = b[:i] + } + return b } // tries to find the cgroup path for the currently-running container, From c8a5c497373def9405217e1003d7e912230c5ac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Sat, 31 May 2025 12:39:49 +0200 Subject: [PATCH 03/12] Switch to []bytes and iterators --- metric/system/cgroup/bytesutil/bytesutil.go | 70 ++++++++++++ .../system/cgroup/bytesutil/bytesutil_test.go | 79 +++++++++++++ metric/system/cgroup/stringutil/stringutil.go | 107 ------------------ metric/system/cgroup/util.go | 87 +++++++------- metric/system/cgroup/util_test.go | 9 +- 5 files changed, 194 insertions(+), 158 deletions(-) create mode 100644 metric/system/cgroup/bytesutil/bytesutil.go create mode 100644 metric/system/cgroup/bytesutil/bytesutil_test.go delete mode 100644 metric/system/cgroup/stringutil/stringutil.go diff --git a/metric/system/cgroup/bytesutil/bytesutil.go b/metric/system/cgroup/bytesutil/bytesutil.go new file mode 100644 index 0000000000..7305105caa --- /dev/null +++ b/metric/system/cgroup/bytesutil/bytesutil.go @@ -0,0 +1,70 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package bytesutil + +import ( + "iter" +) + +var asciiSpace = [256]bool{'\t': true, '\n': true, '\v': true, '\f': true, '\r': true, ' ': true} + +// Fields returns an iterator that yields the fields of byte array b split around each single +// ASCII white space character (space, tab, newline, vertical tab, form feed, carriage return). +// It can be used with range loops like this: +// +// for i, field := range stringutil.Fields(b) { +// fmt.Printf("Field %d: %v\n", i, field) +// } +func Fields(b []byte) iter.Seq2[int, []byte] { + return func(yield func(int, []byte) bool) { + for i, bi := 0, 0; bi < len(b); i++ { + fieldStart := bi + // Find the end of the field + for bi < len(b) && !asciiSpace[b[bi]] { + bi++ + } + if !yield(i, b[fieldStart:bi]) { + return + } + bi++ + } + } +} + +// Split returns an iterator that yields the fields of byte array b split around +// the given character. +// It can be used with range loops like this: +// +// for i, field := range stringutil.Split(b, ',') { +// fmt.Printf("Field %d: %v\n", i, field) +// } +func Split(b []byte, sep byte) iter.Seq2[int, []byte] { + return func(yield func(int, []byte) bool) { + for i, bi := 0, 0; bi < len(b); i++ { + fieldStart := bi + // Find the end of the field + for bi < len(b) && b[bi] != sep { + bi++ + } + if !yield(i, b[fieldStart:bi]) { + return + } + bi++ + } + } +} diff --git a/metric/system/cgroup/bytesutil/bytesutil_test.go b/metric/system/cgroup/bytesutil/bytesutil_test.go new file mode 100644 index 0000000000..15a3e48bca --- /dev/null +++ b/metric/system/cgroup/bytesutil/bytesutil_test.go @@ -0,0 +1,79 @@ +package bytesutil + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFields(t *testing.T) { + tests := []struct { + name string + input []byte + want []string + }{ + { + name: "empty array", + input: []byte(""), + want: []string{}, + }, + { + name: "single white space", + input: []byte(" "), + want: []string{"", ""}, + }, + { + name: "single word", + input: []byte("hello"), + want: []string{"hello"}, + }, + { + name: "multiple words", + input: []byte("hello world this is a test"), + want: []string{"hello", "world", "this", "is", "a", "test"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for i, f := range Fields(tt.input) { + require.Equal(t, tt.want[i], string(f), "Fields() mismatch at index %d", i) + } + }) + } +} + +func TestSplit(t *testing.T) { + tests := []struct { + name string + input []byte + want []string + }{ + { + name: "empty array", + input: []byte(""), + want: []string{}, + }, + { + name: "single separator", + input: []byte(","), + want: []string{"", ""}, + }, + { + name: "single word", + input: []byte("hello"), + want: []string{"hello"}, + }, + { + name: "multiple words", + input: []byte("hello,world,this,is,a,test"), + want: []string{"hello", "world", "this", "is", "a", "test"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for i, f := range Split(tt.input, ',') { + require.Equal(t, tt.want[i], string(f), "Split() mismatch at index %d", i) + } + }) + } +} diff --git a/metric/system/cgroup/stringutil/stringutil.go b/metric/system/cgroup/stringutil/stringutil.go deleted file mode 100644 index 7669f21ffe..0000000000 --- a/metric/system/cgroup/stringutil/stringutil.go +++ /dev/null @@ -1,107 +0,0 @@ -// Licensed to Elasticsearch B.V. under one or more contributor -// license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright -// ownership. Elasticsearch B.V. licenses this file to you under -// the Apache License, Version 2.0 (the "License"); you may -// not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package stringutil - -import ( - "strings" - "unsafe" -) - -var asciiSpace = [256]uint8{'\t': 1, '\n': 1, '\v': 1, '\f': 1, '\r': 1, ' ': 1} - -// FieldsN splits the string s around each instance of one or more consecutive space -// characters, filling f with substrings of s. -// If s contains more fields than n, the last element of f is set to the -// unparsed remainder of s starting with the first non-space character. -// f will stay untouched if s is empty or contains only white space. -// if n is greater than len(f), 0 is returned without doing any parsing. -// -// Apart from the mentioned differences, FieldsN is like an allocation-free strings.Fields. -func FieldsN(s string, f []string) int { - n := len(f) - si := 0 - for i := 0; i < n-1; i++ { - // Find the start of the next field. - for si < len(s) && asciiSpace[s[si]] != 0 { - si++ - } - fieldStart := si - - // Find the end of the field. - for si < len(s) && asciiSpace[s[si]] == 0 { - si++ - } - if fieldStart >= si { - return i - } - - f[i] = s[fieldStart:si] - } - - // Find the start of the next field. - for si < len(s) && asciiSpace[s[si]] != 0 { - si++ - } - - // Put the remainder of s as last element of f. - if si < len(s) { - f[n-1] = s[si:] - return n - } - - return n - 1 -} - -// SplitN splits the string around each instance of sep, filling f with substrings of s. -// If s contains more fields than n, the last element of f is set to the -// unparsed remainder of s starting with the first non-space character. -// f will stay untouched if s is empty or contains only white space. -// if n is greater than len(f), 0 is returned without doing any parsing. -// -// Apart from the mentioned differences, SplitN is like an allocation-free strings.SplitN. -func SplitN(s, sep string, f []string) int { - n := len(f) - i := 0 - for ; i < n-1 && s != ""; i++ { - fieldEnd := strings.Index(s, sep) - if fieldEnd < 0 { - f[i] = s - return i + 1 - } - f[i] = s[:fieldEnd] - s = s[fieldEnd+len(sep):] - } - - // Put the remainder of s as last element of f. - f[i] = s - return i + 1 -} - -// ByteSlice2String converts a byte slice into a string without a heap allocation. -// Be aware that the byte slice and the string share the same memory - which makes -// the string mutable. -func ByteSlice2String(b []byte) string { - return *(*string)(unsafe.Pointer(&b)) -} - -func SplitInline(s, sep string) []string { - // Use a fixed-size slice to avoid allocations. - fields := make([]string, strings.Count(s, sep)+1) - SplitN(s, sep, fields) - return fields -} diff --git a/metric/system/cgroup/util.go b/metric/system/cgroup/util.go index 8aba8e5b45..ad3219718e 100644 --- a/metric/system/cgroup/util.go +++ b/metric/system/cgroup/util.go @@ -19,6 +19,7 @@ package cgroup import ( "bufio" + "bytes" "errors" "fmt" "io/fs" @@ -31,7 +32,7 @@ import ( "github.com/elastic/elastic-agent-libs/logp" - "github.com/elastic/elastic-agent-system-metrics/metric/system/cgroup/stringutil" + "github.com/elastic/elastic-agent-system-metrics/metric/system/cgroup/bytesutil" "github.com/elastic/elastic-agent-system-metrics/metric/system/resolve" ) @@ -73,9 +74,9 @@ var ( // mountinfo represents a subset of the fields containing /proc/[pid]/mountinfo. type mountinfo struct { - mountpoint string - filesystemType string - superOptions []string + mountpoint []byte + filesystemType []byte + superOptions []byte } // Mountpoints organizes info about V1 and V2 cgroup mountpoints @@ -119,12 +120,19 @@ func (pl PathList) Flatten() []ControllerPath { // parseMountinfoLine parses a line from the /proc/[pid]/mountinfo file on // Linux. The format of the line is specified in section 3.5 of // https://www.kernel.org/doc/Documentation/filesystems/proc.txt. -func parseMountinfoLine(line string) (mountinfo, error) { +func parseMountinfoLine(line []byte) (mountinfo, error) { mount := mountinfo{} - var fields [10 + 6]string // support up to 6 optional fields + var fields [10 + 6][]byte // support up to 6 optional fields + nFields := 0 + for i, field := range bytesutil.Fields(line) { + fields[i] = field + nFields = i + 1 + if nFields >= len(fields) { + break // avoid out of bounds + } + } - nFields := stringutil.FieldsN(line, fields[:]) if nFields < 10 { return mount, fmt.Errorf("invalid mountinfo line, expected at least "+ "10 fields but got %d from line='%s'", nFields, line) @@ -132,7 +140,7 @@ func parseMountinfoLine(line string) (mountinfo, error) { var separatorIndex int for i := 6; i < nFields; i++ { - if fields[i] == "-" { + if len(fields[i]) == 1 && fields[i][0] == '-' { separatorIndex = i break } @@ -149,9 +157,7 @@ func parseMountinfoLine(line string) (mountinfo, error) { mount.mountpoint = fields[4] mount.filesystemType = fields[separatorIndex+1] - if mount.filesystemType == "cgroup" { - mount.superOptions = stringutil.SplitInline(fields[separatorIndex+3], ",") - } + mount.superOptions = fields[separatorIndex+3] return mount, nil } @@ -214,20 +220,21 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ defer mountinfo.Close() hostFS := rootfs.ResolveHostFS("") - mounts := map[string]string{} + mounts := make(map[string]string, len(subsystems)) // preallocate map with expected size mountInfo := Mountpoints{} - sc := bufio.NewScanner(mountinfo) possibleV2Paths := []string{} + + sc := bufio.NewScanner(mountinfo) for sc.Scan() { // https://www.kernel.org/doc/Documentation/filesystems/proc.txt // Example: // 25 21 0:20 / /cgroup/cpu rw,relatime - cgroup cgroup rw,cpu + line := bytes.TrimSpace(sc.Bytes()) + if len(line) == 0 { + continue + } - // Avoid heap allocation by not using scanner.Text(). - // NOTE: The underlying bytes will change with the next call to scanner.Scan(), - // so make sure to not keep any references after the end of the loop iteration. - line := strings.TrimSpace(stringutil.ByteSlice2String(sc.Bytes())) - if line == "" { + if !bytes.Contains(line, []byte("cgroup")) { continue } @@ -237,34 +244,33 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ } // if the mountpoint from the subsystem has a different root than ours, it probably belongs to something else. - if !strings.HasPrefix(mount.mountpoint, hostFS) { + if !bytes.HasPrefix(mount.mountpoint, []byte(hostFS)) { continue } // cgroupv1 option - if mount.filesystemType == "cgroup" { - for _, opt := range mount.superOptions { + if bytes.Equal(mount.filesystemType, []byte("cgroup")) { + for _, opt := range bytesutil.Split(mount.superOptions, ',') { // Sometimes the subsystem name is written like "name=blkio". - var fields [2]string - if n := stringutil.SplitN(opt, "=", fields[:]); n > 1 { - opt = fields[1] + if _, v, found := bytes.Cut(opt, []byte{'='}); found { + opt = v } // Test if option is a subsystem name. - if _, found := subsystems[opt]; found { + if _, found := subsystems[string(opt)]; found { // Add the subsystem mount if it does not already exist. - if _, exists := mounts[opt]; !exists { - mounts[opt] = strings.Clone(mount.mountpoint) + if _, exists := mounts[string(opt)]; !exists { + mounts[string(opt)] = string(mount.mountpoint) } } } + continue } // V2 option - if mount.filesystemType == "cgroup2" { - possibleV2Paths = append(possibleV2Paths, strings.Clone(mount.mountpoint)) + if bytes.Equal(mount.filesystemType, []byte("cgroup2")) { + possibleV2Paths = append(possibleV2Paths, string(mount.mountpoint)) } - } mountInfo.V2Loc = getProperV2Paths(rootfs, possibleV2Paths) @@ -285,7 +291,7 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ return mountInfo, sc.Err() } -// isCgroupNSHost returns true if we're running inside a container with a +// isCgroupNSPrivate returns true if we're running inside a container with a // private cgroup namespace. Will return true if we're in a public namespace, or there's an error // Note that this function only makes sense *inside* a container. Outside it will probably always return false. func isCgroupNSPrivate() bool { @@ -298,27 +304,14 @@ func isCgroupNSPrivate() bool { } // if we have a path of just "/" that means we're in our own private namespace // if it's something else, we're probably in a host namespace - colons := 2 - for i := range trimBytes(raw) { - if raw[i] == ':' { - colons++ - if colons == 2 { - // if we have a second colon, check if the next character is a slash - return len(raw) == i+2 && raw[i+1] == '/' - } + for i, field := range bytesutil.Split(bytes.TrimSpace(raw), ':') { + if i == 2 { + return bytes.Equal(field, []byte{'/'}) } } return false } -func trimBytes(b []byte) []byte { - // trim the trailing newlines - for i := len(b) - 1; i >= 0 && b[i] == '\n'; i-- { - b = b[:i] - } - return b -} - // tries to find the cgroup path for the currently-running container, // assuming we are running in a container. // see https://docs.docker.com/config/containers/runmetrics/#find-the-cgroup-for-a-given-container diff --git a/metric/system/cgroup/util_test.go b/metric/system/cgroup/util_test.go index db5e479092..a7c85b606d 100644 --- a/metric/system/cgroup/util_test.go +++ b/metric/system/cgroup/util_test.go @@ -21,6 +21,7 @@ package cgroup import ( + "bytes" "errors" "fmt" "os" @@ -289,14 +290,14 @@ func TestParseMountinfoLine(t *testing.T) { } for _, line := range lines { - mount, err := parseMountinfoLine(line) + mount, err := parseMountinfoLine([]byte(line)) if err != nil { t.Fatal(err) } - assert.Equal(t, "/sys/fs/cgroup/blkio", mount.mountpoint) - assert.Equal(t, "cgroup", mount.filesystemType) - assert.Len(t, mount.superOptions, 2) + assert.Equal(t, "/sys/fs/cgroup/blkio", string(mount.mountpoint)) + assert.Equal(t, "cgroup", string(mount.filesystemType)) + assert.Equal(t, bytes.Count(mount.superOptions, []byte{','}), 1) } } From f8ed77612ab08a4f7e295e5baa462f5f115d3ebe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Fri, 20 Jun 2025 13:48:26 +0200 Subject: [PATCH 04/12] Silence staticcheck deprecation warning --- metric/system/cgroup/util_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/system/cgroup/util_test.go b/metric/system/cgroup/util_test.go index a7c85b606d..b1a22cd314 100644 --- a/metric/system/cgroup/util_test.go +++ b/metric/system/cgroup/util_test.go @@ -260,7 +260,7 @@ func TestMountpointsV2(t *testing.T) { []byte(pidFmt), 0o744) require.NoError(t, err) - _ = logp.DevelopmentSetup() + _ = logp.DevelopmentSetup() //nolint:staticcheck // Use logp.NewDevelopmentLogger reader, err := NewReader(resolve.NewTestResolver("testdata/docker2"), false) require.NoError(t, err) From f52dba4a65fd30f817488154cff55cc8821628df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Fri, 20 Jun 2025 13:53:12 +0200 Subject: [PATCH 05/12] Add license header to bytesutil_test.go --- .../system/cgroup/bytesutil/bytesutil_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/metric/system/cgroup/bytesutil/bytesutil_test.go b/metric/system/cgroup/bytesutil/bytesutil_test.go index 15a3e48bca..25e13d8fcf 100644 --- a/metric/system/cgroup/bytesutil/bytesutil_test.go +++ b/metric/system/cgroup/bytesutil/bytesutil_test.go @@ -1,3 +1,20 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + package bytesutil import ( From 406bd5d92fe09713eeaa9eabf865eee18a67d8d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Fri, 20 Jun 2025 16:30:09 +0200 Subject: [PATCH 06/12] debug tests --- metric/system/cgroup/reader.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/metric/system/cgroup/reader.go b/metric/system/cgroup/reader.go index 6bacae3199..66052be7f0 100644 --- a/metric/system/cgroup/reader.go +++ b/metric/system/cgroup/reader.go @@ -27,6 +27,7 @@ import ( "time" "github.com/elastic/elastic-agent-libs/logp" + "github.com/elastic/elastic-agent-system-metrics/metric/system/cgroup/cgv1" "github.com/elastic/elastic-agent-system-metrics/metric/system/cgroup/cgv2" "github.com/elastic/elastic-agent-system-metrics/metric/system/resolve" @@ -361,6 +362,7 @@ func (r *Reader) readControllerList(cgroupsFile string) ([]string, error) { controllers := strings.Split(cgroupsFile, "\n") var cgpath string for _, controller := range controllers { + logp.L().Infof("controller: %s", controller) if strings.Contains(controller, "0::/") { fields := strings.Split(controller, ":") cgpath = fields[2] @@ -372,7 +374,10 @@ func (r *Reader) readControllerList(cgroupsFile string) ([]string, error) { } cgFilePath := filepath.Join(r.cgroupMountpoints.V2Loc, cgpath, "cgroup.controllers") if cgroupNSStateFetch() && r.rootfsMountpoint.IsSet() { + logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) cgFilePath = filepath.Join(r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount, cgpath, "cgroup.controllers") + } else { + logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc) } controllersRaw, err := os.ReadFile(cgFilePath) From 8669d66fa19a038e3f30ae98d309fb944617abde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 23 Jun 2025 13:01:49 +0200 Subject: [PATCH 07/12] Split function for testing --- metric/system/cgroup/util.go | 6 +++++- metric/system/cgroup/util_test.go | 5 +++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/metric/system/cgroup/util.go b/metric/system/cgroup/util.go index ad3219718e..646e9fbb8b 100644 --- a/metric/system/cgroup/util.go +++ b/metric/system/cgroup/util.go @@ -234,7 +234,7 @@ func SubsystemMountpoints(rootfs resolve.Resolver, subsystems map[string]struct{ continue } - if !bytes.Contains(line, []byte("cgroup")) { + if !bytes.Contains(line, []byte(" cgroup")) { continue } @@ -304,6 +304,10 @@ func isCgroupNSPrivate() bool { } // if we have a path of just "/" that means we're in our own private namespace // if it's something else, we're probably in a host namespace + return isCgroupPathSlash(raw) +} + +func isCgroupPathSlash(raw []byte) bool { for i, field := range bytesutil.Split(bytes.TrimSpace(raw), ':') { if i == 2 { return bytes.Equal(field, []byte{'/'}) diff --git a/metric/system/cgroup/util_test.go b/metric/system/cgroup/util_test.go index b1a22cd314..0f1e368690 100644 --- a/metric/system/cgroup/util_test.go +++ b/metric/system/cgroup/util_test.go @@ -363,3 +363,8 @@ func TestFetchV2Paths(t *testing.T) { }) } } + +func TestIsCgroupPathSlash(t *testing.T) { + require.False(t, isCgroupPathSlash([]byte("0::/user.slice/user-1000.slice/session-520.scope"))) + require.True(t, isCgroupPathSlash([]byte("0::/"))) +} From 448ac916180c2b41346802b6afce488c55924298 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 23 Jun 2025 13:26:09 +0200 Subject: [PATCH 08/12] test --- metric/system/cgroup/reader.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/metric/system/cgroup/reader.go b/metric/system/cgroup/reader.go index 66052be7f0..c25d027f34 100644 --- a/metric/system/cgroup/reader.go +++ b/metric/system/cgroup/reader.go @@ -377,9 +377,10 @@ func (r *Reader) readControllerList(cgroupsFile string) ([]string, error) { logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) cgFilePath = filepath.Join(r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount, cgpath, "cgroup.controllers") } else { - logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc) + logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) } + logp.L().Infof("reading %s", cgFilePath) controllersRaw, err := os.ReadFile(cgFilePath) if err != nil { return nil, fmt.Errorf("error reading cgroup '%s': file %s: %w", cgpath, cgFilePath, err) From d0f0ddadc5f59a17b91874e1b38d03265fea36f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 23 Jun 2025 15:56:53 +0200 Subject: [PATCH 09/12] test --- metric/system/cgroup/reader.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/metric/system/cgroup/reader.go b/metric/system/cgroup/reader.go index c25d027f34..c9eba4b57a 100644 --- a/metric/system/cgroup/reader.go +++ b/metric/system/cgroup/reader.go @@ -372,12 +372,13 @@ func (r *Reader) readControllerList(cgroupsFile string) ([]string, error) { if cgpath == "" { return []string{}, nil } + cgpath = filepath.Join(cgpath) // The path may have a relative prefix like "/../..`, which effectively is "/". cgFilePath := filepath.Join(r.cgroupMountpoints.V2Loc, cgpath, "cgroup.controllers") if cgroupNSStateFetch() && r.rootfsMountpoint.IsSet() { - logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) + logp.L().Infof("a) V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) cgFilePath = filepath.Join(r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount, cgpath, "cgroup.controllers") } else { - logp.L().Infof("V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) + logp.L().Infof("b) V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) } logp.L().Infof("reading %s", cgFilePath) From 344b10664abcc38b86fd589c68fbf41b203b8ae7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 23 Jun 2025 16:22:19 +0200 Subject: [PATCH 10/12] test --- metric/system/cgroup/reader.go | 1 + metric/system/cgroup/util.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/metric/system/cgroup/reader.go b/metric/system/cgroup/reader.go index c9eba4b57a..f25152183e 100644 --- a/metric/system/cgroup/reader.go +++ b/metric/system/cgroup/reader.go @@ -387,6 +387,7 @@ func (r *Reader) readControllerList(cgroupsFile string) ([]string, error) { return nil, fmt.Errorf("error reading cgroup '%s': file %s: %w", cgpath, cgFilePath, err) } + logp.L().Infof("controllersRaw: %s", controllersRaw) if len(controllersRaw) == 0 { return []string{}, nil } diff --git a/metric/system/cgroup/util.go b/metric/system/cgroup/util.go index 646e9fbb8b..bdc1019d94 100644 --- a/metric/system/cgroup/util.go +++ b/metric/system/cgroup/util.go @@ -304,6 +304,7 @@ func isCgroupNSPrivate() bool { } // if we have a path of just "/" that means we're in our own private namespace // if it's something else, we're probably in a host namespace + logp.L().Infof("isCgroupPathSlash(%s) = %v", string(raw), isCgroupPathSlash(raw)) return isCgroupPathSlash(raw) } @@ -482,6 +483,7 @@ func (r *Reader) ProcessCgroupPaths(pid int) (PathList, error) { if r.cgroupsHierarchyOverride != "" { path = r.cgroupsHierarchyOverride } + path = filepath.Join(path) //on newer docker versions (1.41+?), docker will do namespacing with cgroups // such that we'll get a cgroup path like `0::/../../user.slice/user-1000.slice/session-520.scope` From e8045a3416576eb6ad261e99ebf865cb90864658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Tue, 24 Jun 2025 08:09:14 +0200 Subject: [PATCH 11/12] clean filepath --- metric/system/cgroup/reader.go | 2 +- metric/system/cgroup/util.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/metric/system/cgroup/reader.go b/metric/system/cgroup/reader.go index f25152183e..b86ca005ed 100644 --- a/metric/system/cgroup/reader.go +++ b/metric/system/cgroup/reader.go @@ -372,7 +372,7 @@ func (r *Reader) readControllerList(cgroupsFile string) ([]string, error) { if cgpath == "" { return []string{}, nil } - cgpath = filepath.Join(cgpath) // The path may have a relative prefix like "/../..`, which effectively is "/". + cgpath = filepath.Clean(cgpath) // The path may have a relative prefix like "/../..`, which effectively is "/". cgFilePath := filepath.Join(r.cgroupMountpoints.V2Loc, cgpath, "cgroup.controllers") if cgroupNSStateFetch() && r.rootfsMountpoint.IsSet() { logp.L().Infof("a) V2Loc: %s, ContainerizedRootMount %s", r.cgroupMountpoints.V2Loc, r.cgroupMountpoints.ContainerizedRootMount) diff --git a/metric/system/cgroup/util.go b/metric/system/cgroup/util.go index bdc1019d94..a9aa9e0e17 100644 --- a/metric/system/cgroup/util.go +++ b/metric/system/cgroup/util.go @@ -483,7 +483,7 @@ func (r *Reader) ProcessCgroupPaths(pid int) (PathList, error) { if r.cgroupsHierarchyOverride != "" { path = r.cgroupsHierarchyOverride } - path = filepath.Join(path) + path = filepath.Clean(path) //on newer docker versions (1.41+?), docker will do namespacing with cgroups // such that we'll get a cgroup path like `0::/../../user.slice/user-1000.slice/session-520.scope` From f7ba9e269d71614db0575f646a270a37a9c3c5ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Fri, 25 Jul 2025 15:59:17 +0200 Subject: [PATCH 12/12] Silence linter --- metric/system/process/process_container_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/metric/system/process/process_container_test.go b/metric/system/process/process_container_test.go index 39fc8baead..0273c788ea 100644 --- a/metric/system/process/process_container_test.go +++ b/metric/system/process/process_container_test.go @@ -38,7 +38,7 @@ import ( // However, they are designed so that `go test` can run them normally as well func TestContainerMonitoringFromInsideContainer(t *testing.T) { - _ = logp.DevelopmentSetup() + _ = logp.DevelopmentSetup() //nolint:staticcheck // Use logp.NewDevelopmentLogger testStats := Stats{CPUTicks: true, EnableCgroups: true, @@ -64,7 +64,7 @@ func TestContainerMonitoringFromInsideContainer(t *testing.T) { } func TestSelfMonitoringFromInsideContainer(t *testing.T) { - _ = logp.DevelopmentSetup() + _ = logp.DevelopmentSetup() //nolint:staticcheck // Use logp.NewDevelopmentLogger testStats := Stats{CPUTicks: true, EnableCgroups: true, @@ -89,7 +89,7 @@ func TestSelfMonitoringFromInsideContainer(t *testing.T) { } func TestSystemHostFromContainer(t *testing.T) { - _ = logp.DevelopmentSetup() + _ = logp.DevelopmentSetup() //nolint:staticcheck // Use logp.NewDevelopmentLogger testStats := Stats{CPUTicks: true, EnableCgroups: true,