From efca32c799d02a7606a6233b0800500971a6399c Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 28 May 2021 14:48:25 +1000 Subject: [PATCH 1/3] cgroup2: io: map io.stats to v1 blkio.stats correctly Kubelet and cAdvisor depend on the metrics having the same values as in cgroupv1, but we didn't correctly map the number of read and write IOs to the correct cgroupv1 stats table (blkio.io_serviced). In addition, don't leak any extra stats in our output -- if users need that information we can always add a new field for it. Reported-by: Yashpal Choudhary Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs2/io.go | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/libcontainer/cgroups/fs2/io.go b/libcontainer/cgroups/fs2/io.go index bd6f2088f..166b05582 100644 --- a/libcontainer/cgroups/fs2/io.go +++ b/libcontainer/cgroups/fs2/io.go @@ -8,6 +8,8 @@ import ( "strconv" "strings" + "github.com/sirupsen/logrus" + "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" @@ -88,12 +90,12 @@ func readCgroup2MapFile(dirPath string, name string) (map[string][]string, error } func statIo(dirPath string, stats *cgroups.Stats) error { - // more details on the io.stat file format: https://www.kernel.org/doc/Documentation/cgroup-v2.txt - var ioServiceBytesRecursive []cgroups.BlkioStatEntry values, err := readCgroup2MapFile(dirPath, "io.stat") if err != nil { return err } + // more details on the io.stat file format: https://www.kernel.org/doc/Documentation/cgroup-v2.txt + var parsedStats cgroups.BlkioStats for k, v := range values { d := strings.Split(k, ":") if len(d) != 2 { @@ -115,12 +117,29 @@ func statIo(dirPath string, stats *cgroups.Stats) error { } op := d[0] - // Accommodate the cgroup v1 naming + // Map to the cgroupv1 naming and layout (in separate tables). + var targetTable *[]cgroups.BlkioStatEntry switch op { + // Equivalent to cgroupv1's blkio.io_service_bytes. case "rbytes": op = "Read" + targetTable = &parsedStats.IoServiceBytesRecursive case "wbytes": op = "Write" + targetTable = &parsedStats.IoServiceBytesRecursive + // Equivalent to cgroupv1's blkio.io_serviced. + case "rios": + op = "Read" + targetTable = &parsedStats.IoServicedRecursive + case "wios": + op = "Write" + targetTable = &parsedStats.IoServicedRecursive + default: + // Skip over entries we cannot map to cgroupv1 stats for now. + // In the future we should expand the stats struct to include + // them. + logrus.Debugf("cgroupv2 io stats: skipping over unmappable %s entry", item) + continue } value, err := strconv.ParseUint(d[1], 10, 0) @@ -134,9 +153,9 @@ func statIo(dirPath string, stats *cgroups.Stats) error { Minor: minor, Value: value, } - ioServiceBytesRecursive = append(ioServiceBytesRecursive, entry) + *targetTable = append(*targetTable, entry) } } - stats.BlkioStats = cgroups.BlkioStats{IoServiceBytesRecursive: ioServiceBytesRecursive} + stats.BlkioStats = parsedStats return nil } From 0fef122f87f804037f4a16bbf4f41044d4d4e92c Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 1 Jun 2021 10:32:47 +1000 Subject: [PATCH 2/3] cgroup2: io: handle 64-bit values correctly on 32-bit architectures strconv.ParseUint(..., 0) is not really safe, because on 32-bit architectures it will trigger runtime errors when trying to parse large numbers (which in the case of the cgroupv2 io controller, is almost certainly going to happen). Fixes: 1932917b716d ("libcontainer: add initial support for cgroups v2") Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs2/io.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fs2/io.go b/libcontainer/cgroups/fs2/io.go index 166b05582..77174f437 100644 --- a/libcontainer/cgroups/fs2/io.go +++ b/libcontainer/cgroups/fs2/io.go @@ -101,11 +101,11 @@ func statIo(dirPath string, stats *cgroups.Stats) error { if len(d) != 2 { continue } - major, err := strconv.ParseUint(d[0], 10, 0) + major, err := strconv.ParseUint(d[0], 10, 64) if err != nil { return err } - minor, err := strconv.ParseUint(d[1], 10, 0) + minor, err := strconv.ParseUint(d[1], 10, 64) if err != nil { return err } @@ -142,7 +142,7 @@ func statIo(dirPath string, stats *cgroups.Stats) error { continue } - value, err := strconv.ParseUint(d[1], 10, 0) + value, err := strconv.ParseUint(d[1], 10, 64) if err != nil { return err } From 1eea9253a13cf53b9e8d3de880ec7edd2f743350 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Mon, 31 May 2021 13:01:15 +1000 Subject: [PATCH 3/3] cgroup2: io: add io.stats parsing test Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs2/io_test.go | 87 +++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 libcontainer/cgroups/fs2/io_test.go diff --git a/libcontainer/cgroups/fs2/io_test.go b/libcontainer/cgroups/fs2/io_test.go new file mode 100644 index 000000000..c6cb7a63a --- /dev/null +++ b/libcontainer/cgroups/fs2/io_test.go @@ -0,0 +1,87 @@ +package fs2 + +import ( + "io/ioutil" + "os" + "path/filepath" + "reflect" + "sort" + "testing" + + "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" +) + +const exampleIoStatData = `254:1 rbytes=6901432320 wbytes=14245535744 rios=263278 wios=248603 dbytes=0 dios=0 +254:0 rbytes=2702336 wbytes=0 rios=97 wios=0 dbytes=0 dios=0 +259:0 rbytes=6911345664 wbytes=14245536256 rios=264538 wios=244914 dbytes=530485248 dios=2` + +var exampleIoStatsParsed = cgroups.BlkioStats{ + IoServiceBytesRecursive: []cgroups.BlkioStatEntry{ + {Major: 254, Minor: 1, Value: 6901432320, Op: "Read"}, + {Major: 254, Minor: 1, Value: 14245535744, Op: "Write"}, + {Major: 254, Minor: 0, Value: 2702336, Op: "Read"}, + {Major: 254, Minor: 0, Value: 0, Op: "Write"}, + {Major: 259, Minor: 0, Value: 6911345664, Op: "Read"}, + {Major: 259, Minor: 0, Value: 14245536256, Op: "Write"}, + }, + IoServicedRecursive: []cgroups.BlkioStatEntry{ + {Major: 254, Minor: 1, Value: 263278, Op: "Read"}, + {Major: 254, Minor: 1, Value: 248603, Op: "Write"}, + {Major: 254, Minor: 0, Value: 97, Op: "Read"}, + {Major: 254, Minor: 0, Value: 0, Op: "Write"}, + {Major: 259, Minor: 0, Value: 264538, Op: "Read"}, + {Major: 259, Minor: 0, Value: 244914, Op: "Write"}, + }, +} + +func lessBlkioStatEntry(a, b cgroups.BlkioStatEntry) bool { + if a.Major != b.Major { + return a.Major < b.Major + } + if a.Minor != b.Minor { + return a.Minor < b.Minor + } + if a.Op != b.Op { + return a.Op < b.Op + } + return a.Value < b.Value +} + +func sortBlkioStats(stats *cgroups.BlkioStats) { + for _, table := range []*[]cgroups.BlkioStatEntry{ + &stats.IoServicedRecursive, + &stats.IoServiceBytesRecursive, + } { + sort.SliceStable(*table, func(i, j int) bool { return lessBlkioStatEntry((*table)[i], (*table)[j]) }) + } +} + +func TestStatIo(t *testing.T) { + // We're using a fake cgroupfs. + fscommon.TestMode = true + + fakeCgroupDir, err := ioutil.TempDir("", "runc-stat-io-test.*") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(fakeCgroupDir) + statPath := filepath.Join(fakeCgroupDir, "io.stat") + + if err := ioutil.WriteFile(statPath, []byte(exampleIoStatData), 0644); err != nil { + t.Fatal(err) + } + + var gotStats cgroups.Stats + if err := statIo(fakeCgroupDir, &gotStats); err != nil { + t.Error(err) + } + + // Sort the output since statIo uses a map internally. + sortBlkioStats(&gotStats.BlkioStats) + sortBlkioStats(&exampleIoStatsParsed) + + if !reflect.DeepEqual(gotStats.BlkioStats, exampleIoStatsParsed) { + t.Errorf("parsed cgroupv2 io.stat doesn't match expected result: \ngot %#v\nexpected %#v\n", gotStats.BlkioStats, exampleIoStatsParsed) + } +}