Skip to content

libcontainer/intelrdt: use moby/sys/mountinfo #2606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mrunalp merged 2 commits into from
Sep 30, 2020
Merged

libcontainer/intelrdt: use moby/sys/mountinfo #2606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1c1fdf
libcontainer/intelrdt: use moby/sys/mountinfo
kolyshkin Sep 29, 2020
2c70d23
libct/intelrdt: add TestFindIntelRdtMountpointDir
kolyshkin Sep 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 23 additions & 35 deletions libcontainer/intelrdt/intelrdt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ package intelrdt
import (
"bufio"
"fmt"
"io"
"io/ioutil"
"os"
"path/filepath"
"strconv"
"strings"
"sync"

"github.com/moby/sys/mountinfo"
"github.com/opencontainers/runc/libcontainer/configs"
)

Expand Down Expand Up @@ -240,46 +242,27 @@ func init() {
}

// Return the mount point path of Intel RDT "resource control" filesysem
func findIntelRdtMountpointDir() (string, error) {
f, err := os.Open("/proc/self/mountinfo")
func findIntelRdtMountpointDir(f io.Reader) (string, error) {
mi, err := mountinfo.GetMountsFromReader(f, func(m *mountinfo.Info) (bool, bool) {
// similar to mountinfo.FstypeFilter but stops after the first match
if m.Fstype == "resctrl" {
return false, true // don't skip, stop
}
return true, false // skip, keep going
})
if err != nil {
return "", err
}
defer f.Close()

s := bufio.NewScanner(f)
for s.Scan() {
text := s.Text()
fields := strings.Split(text, " ")
// Safe as mountinfo encodes mountpoints with spaces as \040.
index := strings.Index(text, " - ")
postSeparatorFields := strings.Fields(text[index+3:])
Copy link

@Creatone Creatone Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was working with this code because there was a possibility of accessing elements that not exist and that could cause a panic error.
Thanks, @kolyshkin for fixing it and improve code readability.

I adjusted the unit tests to your code. Could you consider using them?

kolyshkin/runc@intel-rdt...Creatone:creatone/intelrdt-moby-sys-mountinfo

Copy link
Contributor Author

@kolyshkin kolyshkin Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments then decided it's easier to rewrite the code. Hope it's OK with you.

Copy link

@Creatone Creatone Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this 👍

numPostFields := len(postSeparatorFields)

// This is an error as we can't detect if the mount is for "Intel RDT"
if numPostFields == 0 {
return "", fmt.Errorf("Found no fields post '-' in %q", text)
}

if postSeparatorFields[0] == "resctrl" {
// Check that the mount is properly formatted.
if numPostFields < 3 {
return "", fmt.Errorf("Error found less than 3 fields post '-' in %q", text)
}

// Check if MBA Software Controller is enabled through mount option "-o mba_MBps"
if strings.Contains(postSeparatorFields[2], "mba_MBps") {
isMbaScEnabled = true
}

return fields[4], nil
}
if len(mi) < 1 {
return "", NewNotFoundError("Intel RDT")
}
if err := s.Err(); err != nil {
return "", err

// Check if MBA Software Controller is enabled through mount option "-o mba_MBps"
if strings.Contains(","+mi[0].VfsOpts+",", ",mba_MBps,") {
isMbaScEnabled = true
}

return "", NewNotFoundError("Intel RDT")
return mi[0].Mountpoint, nil
}

// Gets the root path of Intel RDT "resource control" filesystem
Expand All @@ -291,7 +274,12 @@ func getIntelRdtRoot() (string, error) {
return intelRdtRoot, nil
}

root, err := findIntelRdtMountpointDir()
f, err := os.Open("/proc/self/mountinfo")
if err != nil {
return "", err
}
root, err := findIntelRdtMountpointDir(f)
f.Close()
if err != nil {
return "", err
}
Expand Down
139 changes: 139 additions & 0 deletions libcontainer/intelrdt/intelrdt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package intelrdt

import (
"io"
"strings"
"testing"
)
Expand Down Expand Up @@ -120,3 +121,141 @@ func TestIntelRdtSetMemBwScSchema(t *testing.T) {
t.Fatal("Got the wrong value, set 'schemata' failed.")
}
}

const (
mountinfoValid = `18 40 0:18 / /sys rw,nosuid,nodev,noexec,relatime shared:6 - sysfs sysfs rw
19 40 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
20 40 0:5 / /dev rw,nosuid shared:2 - devtmpfs devtmpfs rw,size=131927256k,nr_inodes=32981814,mode=755
21 18 0:17 / /sys/kernel/security rw,nosuid,nodev,noexec,relatime shared:7 - securityfs securityfs rw
22 20 0:19 / /dev/shm rw,nosuid,nodev shared:3 - tmpfs tmpfs rw
23 20 0:12 / /dev/pts rw,nosuid,noexec,relatime shared:4 - devpts devpts rw,gid=5,mode=620,ptmxmode=000
24 40 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,mode=755
25 18 0:21 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs ro,mode=755
26 25 0:22 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 - cgroup cgroup rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
27 18 0:23 / /sys/fs/pstore rw,nosuid,nodev,noexec,relatime shared:20 - pstore pstore rw
28 25 0:24 / /sys/fs/cgroup/perf_event rw,nosuid,nodev,noexec,relatime shared:10 - cgroup cgroup rw,perf_event
29 25 0:25 / /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:11 - cgroup cgroup rw,cpuacct,cpu
30 25 0:26 / /sys/fs/cgroup/memory rw,nosuid,nodev,noexec,relatime shared:12 - cgroup cgroup rw,memory
31 25 0:27 / /sys/fs/cgroup/devices rw,nosuid,nodev,noexec,relatime shared:13 - cgroup cgroup rw,devices
32 25 0:28 / /sys/fs/cgroup/hugetlb rw,nosuid,nodev,noexec,relatime shared:14 - cgroup cgroup rw,hugetlb
33 25 0:29 / /sys/fs/cgroup/blkio rw,nosuid,nodev,noexec,relatime shared:15 - cgroup cgroup rw,blkio
34 25 0:30 / /sys/fs/cgroup/pids rw,nosuid,nodev,noexec,relatime shared:16 - cgroup cgroup rw,pids
35 25 0:31 / /sys/fs/cgroup/cpuset rw,nosuid,nodev,noexec,relatime shared:17 - cgroup cgroup rw,cpuset
36 25 0:32 / /sys/fs/cgroup/freezer rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,freezer
37 25 0:33 / /sys/fs/cgroup/net_cls,net_prio rw,nosuid,nodev,noexec,relatime shared:19 - cgroup cgroup rw,net_prio,net_cls
38 18 0:34 / /sys/kernel/config rw,relatime shared:21 - configfs configfs rw
40 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
16 18 0:6 / /sys/kernel/debug rw,relatime shared:23 - debugfs debugfs rw
41 18 0:16 / /sys/fs/resctrl rw,relatime shared:24 - resctrl resctrl rw
42 20 0:36 / /dev/hugepages rw,relatime shared:25 - hugetlbfs hugetlbfs rw
43 19 0:37 / /proc/sys/fs/binfmt_misc rw,relatime shared:26 - autofs systemd-1 rw,fd=32,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=35492
44 20 0:15 / /dev/mqueue rw,relatime shared:27 - mqueue mqueue rw
45 40 8:1 / /boot rw,relatime shared:28 - ext4 /dev/sda1 rw,stripe=4,data=ordered
46 40 253:1 / /home rw,relatime shared:29 - ext4 /dev/mapper/vvhg-vvhg rw,data=ordered
47 40 0:38 / /var/lib/nfs/rpc_pipefs rw,relatime shared:30 - rpc_pipefs sunrpc rw
125 24 0:20 /mesos/containers /run/mesos/containers rw,nosuid shared:22 - tmpfs tmpfs rw,mode=755
123 40 253:0 /var/lib/docker/containers /var/lib/docker/containers rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
129 40 253:0 /var/lib/docker/overlay2 /var/lib/docker/overlay2 rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
119 24 0:39 / /run/user/1009 rw,nosuid,nodev,relatime shared:100 - tmpfs tmpfs rw,size=26387788k,mode=700,uid=1009,gid=1009`

mountinfoMbaSc = `18 40 0:18 / /sys rw,nosuid,nodev,noexec,relatime shared:6 - sysfs sysfs rw
19 40 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
20 40 0:5 / /dev rw,nosuid shared:2 - devtmpfs devtmpfs rw,size=131927256k,nr_inodes=32981814,mode=755
21 18 0:17 / /sys/kernel/security rw,nosuid,nodev,noexec,relatime shared:7 - securityfs securityfs rw
22 20 0:19 / /dev/shm rw,nosuid,nodev shared:3 - tmpfs tmpfs rw
23 20 0:12 / /dev/pts rw,nosuid,noexec,relatime shared:4 - devpts devpts rw,gid=5,mode=620,ptmxmode=000
24 40 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,mode=755
25 18 0:21 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs ro,mode=755
26 25 0:22 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 - cgroup cgroup rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
27 18 0:23 / /sys/fs/pstore rw,nosuid,nodev,noexec,relatime shared:20 - pstore pstore rw
28 25 0:24 / /sys/fs/cgroup/perf_event rw,nosuid,nodev,noexec,relatime shared:10 - cgroup cgroup rw,perf_event
29 25 0:25 / /sys/fs/cgroup/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:11 - cgroup cgroup rw,cpuacct,cpu
30 25 0:26 / /sys/fs/cgroup/memory rw,nosuid,nodev,noexec,relatime shared:12 - cgroup cgroup rw,memory
31 25 0:27 / /sys/fs/cgroup/devices rw,nosuid,nodev,noexec,relatime shared:13 - cgroup cgroup rw,devices
32 25 0:28 / /sys/fs/cgroup/hugetlb rw,nosuid,nodev,noexec,relatime shared:14 - cgroup cgroup rw,hugetlb
33 25 0:29 / /sys/fs/cgroup/blkio rw,nosuid,nodev,noexec,relatime shared:15 - cgroup cgroup rw,blkio
34 25 0:30 / /sys/fs/cgroup/pids rw,nosuid,nodev,noexec,relatime shared:16 - cgroup cgroup rw,pids
35 25 0:31 / /sys/fs/cgroup/cpuset rw,nosuid,nodev,noexec,relatime shared:17 - cgroup cgroup rw,cpuset
36 25 0:32 / /sys/fs/cgroup/freezer rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,freezer
37 25 0:33 / /sys/fs/cgroup/net_cls,net_prio rw,nosuid,nodev,noexec,relatime shared:19 - cgroup cgroup rw,net_prio,net_cls
38 18 0:34 / /sys/kernel/config rw,relatime shared:21 - configfs configfs rw
40 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
16 18 0:6 / /sys/kernel/debug rw,relatime shared:23 - debugfs debugfs rw
41 18 0:16 / /sys/fs/resctrl rw,relatime shared:24 - resctrl resctrl rw,mba_MBps
42 20 0:36 / /dev/hugepages rw,relatime shared:25 - hugetlbfs hugetlbfs rw
43 19 0:37 / /proc/sys/fs/binfmt_misc rw,relatime shared:26 - autofs systemd-1 rw,fd=32,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=35492
44 20 0:15 / /dev/mqueue rw,relatime shared:27 - mqueue mqueue rw
45 40 8:1 / /boot rw,relatime shared:28 - ext4 /dev/sda1 rw,stripe=4,data=ordered
46 40 253:1 / /home rw,relatime shared:29 - ext4 /dev/mapper/vvhg-vvhg rw,data=ordered
47 40 0:38 / /var/lib/nfs/rpc_pipefs rw,relatime shared:30 - rpc_pipefs sunrpc rw
125 24 0:20 /mesos/containers /run/mesos/containers rw,nosuid shared:22 - tmpfs tmpfs rw,mode=755
123 40 253:0 /var/lib/docker/containers /var/lib/docker/containers rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
129 40 253:0 /var/lib/docker/overlay2 /var/lib/docker/overlay2 rw,relatime - ext4 /dev/mapper/vvrg-vvrg rw,data=ordered
119 24 0:39 / /run/user/1009 rw,nosuid,nodev,relatime shared:100 - tmpfs tmpfs rw,size=26387788k,mode=700,uid=1009,gid=1009`
)

func TestFindIntelRdtMountpointDir(t *testing.T) {
testCases := []struct {
name string
input io.Reader
isNotFoundError bool
isError bool
isMbaScEnabled bool
mountpoint string
}{
{
name: "Valid mountinfo with MBA Software Controller disabled",
input: strings.NewReader(mountinfoValid),
mountpoint: "/sys/fs/resctrl",
},
{
name: "Valid mountinfo with MBA Software Controller enabled",
input: strings.NewReader(mountinfoMbaSc),
isMbaScEnabled: true,
mountpoint: "/sys/fs/resctrl",
},
{
name: "Empty mountinfo",
input: strings.NewReader(""),
isNotFoundError: true,
},
{
name: "Broken mountinfo",
input: strings.NewReader("baa"),
isError: true,
},
}

t.Parallel()
for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
mp, err := findIntelRdtMountpointDir(tc.input)
if tc.isNotFoundError {
if !IsNotFound(err) {
t.Errorf("expected IsNotFound error, got %+v", err)
}
return
}
if tc.isError {
if err == nil {
t.Error("expected error, got nil")
}
return
}
if err != nil {
t.Errorf("expected nil, got %+v", err)
return
}
// no errors, check the results
if tc.isMbaScEnabled != isMbaScEnabled {
t.Errorf("expected isMbaScEnabled=%v, got %v",
tc.isMbaScEnabled, isMbaScEnabled)
}
if tc.mountpoint != mp {
t.Errorf("expected mountpoint=%q, got %q",
tc.mountpoint, mp)
}
})
}
}