pkg/topology: get sysfs parsing from containers/nri-plugins

A proper upstream for the sysfs topology parsing is
containers/nri-plugins se we can drop the duplicated code.

At the same time, we no longer need to carry scripts/ttar
and the sys.ttar unpacking for the tests because the remaining
testdata becomes easier to maintain (the original testdata
contained PCI BDF paths that trigged issues).

Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>

Author: mythi

Makefile

@@ -26,8 +26,6 @@ BUNDLE_METADATA_OPTS ?= $(BUNDLE_CHANNELS) $(BUNDLE_DEFAULT_CHANNEL)
 OLM_MANIFESTS = deployments/operator/manifests
 BUNDLE_DIR = community-operators/operators/intel-device-plugins-operator/$(TAG)
 
-TESTDATA_DIR = pkg/topology/testdata
-
 EXTRA_BUILD_ARGS += --build-arg GOLICENSES_VERSION=$(GOLICENSES_VERSION)
 
 pkgs  = $(shell $(GO) list ./... | grep -v vendor | grep -v e2e | grep -v envtest)
@@ -50,13 +48,7 @@ go-mod-tidy:
 	$(GO) mod download all
 	@report=`$(GO) mod tidy -v 2>&1` ; if [ -n "$$report" ]; then echo "$$report"; exit 1; fi
 
-update-fixture:
-	@scripts/ttar -C $(TESTDATA_DIR) -c -f $(TESTDATA_DIR)/sys.ttar sys/
-
-fixture:
-	@scripts/ttar --recursive-unlink -C $(TESTDATA_DIR) -x -f $(TESTDATA_DIR)/sys.ttar
-
-test: fixture
+test:
 ifndef WHAT
 	@$(GO) test -tags $(BUILDTAGS) -race -coverprofile=coverage.txt -covermode=atomic $(pkgs)
 else
@@ -68,7 +60,7 @@ else
 	    exit $$rc
 endif
 
-test-with-kind: fixture intel-sgx-admissionwebhook intel-fpga-admissionwebhook intel-deviceplugin-operator install-tools
+test-with-kind: intel-sgx-admissionwebhook intel-fpga-admissionwebhook intel-deviceplugin-operator install-tools
 	# Build a Cluster with KinD & Load Images & Install Cert-Manager
 	kind create cluster
 	kind load docker-image $(REG)intel-sgx-admissionwebhook:$(TAG)
@@ -241,7 +233,7 @@ check-github-actions:
 	jq -e '$(images_json) - [$(skip_images)] - .jobs.image.strategy.matrix.image == []' > /dev/null || \
 	(echo "Make sure all images are listed in .github/workflows/ci.yaml"; exit 1)
 
-.PHONY: all format test lint build images $(cmds) $(images) lock-images vendor pre-pull set-version check-github-actions envtest fixture update-fixture install-tools test-image-base-layer
+.PHONY: all format test lint build images $(cmds) $(images) lock-images vendor pre-pull set-version check-github-actions envtest install-tools test-image-base-layer
 
 SPHINXOPTS    =
 SPHINXBUILD   = sphinx-build

go.mod

@@ -3,6 +3,7 @@ module github.com/intel/intel-device-plugins-for-kubernetes
 go 1.20
 
 require (
+	github.com/containers/nri-plugins/pkg/topology v0.0.0-20230417061637-0847843000f8
 	github.com/fsnotify/fsnotify v1.6.0
 	github.com/go-ini/ini v1.67.0
 	github.com/go-logr/logr v1.2.4

go.sum

@@ -72,6 +72,8 @@ github.com/cncf/xds/go v0.0.0-20210805033703-aa0b78936158/go.mod h1:eXthEFrGJvWH
 github.com/cncf/xds/go v0.0.0-20210922020428-25de7278fc84/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/cncf/xds/go v0.0.0-20230607035331-e9ce68804cb4 h1:/inchEIKaYC1Akx+H+gqO04wryn5h75LSazbRlnya1k=
+github.com/containers/nri-plugins/pkg/topology v0.0.0-20230417061637-0847843000f8 h1:fcv6mtBBQd+woQSqZN1QJ1g91TRwUNNijj1WKjqub5Q=
+github.com/containers/nri-plugins/pkg/topology v0.0.0-20230417061637-0847843000f8/go.mod h1:XHuUl2t6TVLTfGsy8+w3MdxOjSn2hN0lc1PuaxOEFEc=
 github.com/coreos/go-semver v0.3.0 h1:wkHLiw0WNATZnSG7epLsujiMCgPAc9xhjJ4tgnAxmfM=
 github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk=
 github.com/coreos/go-systemd/v22 v22.4.0 h1:y9YHcjnjynCd/DVbg5j9L/33jQM3MxJlbj/zWskzfGU=

pkg/topology/testdata/sys.ttar

@@ -0,0 +1 @@
+0-7

pkg/topology/testdata/sys/devices/virtual/mem/null/local_cpulist

@@ -0,0 +1 @@
+1,2,3

pkg/topology/testdata/sys/devices/virtual/mem/null/numa_node

@@ -0,0 +1 @@
+0-7

pkg/topology/testdata/sys/devices/virtual/mem/random/local_cpulist

@@ -0,0 +1 @@
+-1

pkg/topology/testdata/sys/devices/virtual/mem/random/numa_node

@@ -0,0 +1 @@
+0-7

pkg/topology/testdata/sys/devices/virtual/tty/tty/local_cpulist

@@ -0,0 +1 @@
+4,5,6

pkg/topology/testdata/sys/devices/virtual/tty/tty/numa_node

@@ -15,288 +15,23 @@
 package topology
 
 import (
-	"fmt"
-	"os"
-	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
-	"syscall"
 
+	topo "github.com/containers/nri-plugins/pkg/topology"
 	"github.com/pkg/errors"
-	"golang.org/x/sys/unix"
 	pluginapi "k8s.io/kubelet/pkg/apis/deviceplugin/v1beta1"
 )
 
-// Path to the directory to mock in tests.
-var (
-	mockRoot = ""
-)
-
-const (
-	// ProviderKubelet is a constant to distinguish that topology hint comes
-	// from parameters passed to CRI create/update requests from Kubelet.
-	ProviderKubelet = "kubelet"
-)
-
-// Hint represents various hints that can be detected from sysfs for the device.
-type Hint struct {
-	Provider string
-	CPUs     string
-	NUMAs    string
-	Sockets  string
-}
-
-// Hints represents set of hints collected from multiple providers.
-type Hints map[string]Hint
-
-func getDevicesFromVirtual(realDevPath string) (devs []string, err error) {
-	relPath, err := filepath.Rel("/sys/devices/virtual", realDevPath)
-	if err != nil {
-		return nil, errors.Wrap(err, "unable to find relative path")
-	}
-
-	if strings.HasPrefix(relPath, "..") {
-		return nil, errors.Errorf("%s is not a virtual device", realDevPath)
-	}
-
-	dir, file := filepath.Split(relPath)
-	switch dir {
-	case "vfio/":
-		iommuGroup := filepath.Join(mockRoot, "/sys/kernel/iommu_groups", file, "devices")
-
-		files, err := os.ReadDir(iommuGroup)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to read IOMMU group %s", iommuGroup)
-		}
-
-		for _, file := range files {
-			realDev, err := filepath.EvalSymlinks(filepath.Join(iommuGroup, file.Name()))
-			if err != nil {
-				return nil, errors.Wrapf(err, "failed to get real path for %s", file.Name())
-			}
-
-			devs = append(devs, realDev)
-		}
-
-		return devs, nil
-	default:
-		return nil, nil
-	}
-}
-
-func getTopologyHint(sysFSPath string) (*Hint, error) {
-	hint := Hint{Provider: sysFSPath}
-	fileMap := map[string]*string{
-		"local_cpulist": &hint.CPUs,
-		"numa_node":     &hint.NUMAs,
-	}
-
-	if err := readFilesInDirectory(fileMap, sysFSPath); err != nil {
-		return nil, err
-	}
-
-	// Workarounds for broken information provided by kernel
-	if hint.NUMAs == "-1" {
-		// non-NUMA aware device or system, ignore it
-		hint.NUMAs = ""
-	}
-
-	if hint.NUMAs != "" && hint.CPUs == "" {
-		// broken topology hint. BIOS reports socket id as NUMA node
-		// First, try to get hints from parent device or bus.
-		parentHints, er := NewTopologyHints(filepath.Dir(sysFSPath))
-		if er == nil {
-			cpulist := map[string]bool{}
-			numalist := map[string]bool{}
-
-			for _, h := range parentHints {
-				if h.CPUs != "" {
-					cpulist[h.CPUs] = true
-				}
-
-				if h.NUMAs != "" {
-					numalist[h.NUMAs] = true
-				}
-			}
-
-			if cpus := strings.Join(mapKeys(cpulist), ","); cpus != "" {
-				hint.CPUs = cpus
-			}
-
-			if numas := strings.Join(mapKeys(numalist), ","); numas != "" {
-				hint.NUMAs = numas
-			}
-		}
-		// if after parent hints we still don't have CPUs hints, use numa hint as sockets.
-		if hint.CPUs == "" && hint.NUMAs != "" {
-			hint.Sockets = hint.NUMAs
-			hint.NUMAs = ""
-		}
-	}
-
-	return &hint, nil
-}
-
-// NewTopologyHints return array of hints for the main device and its
-// dependend devices (e.g. RAID).
-func NewTopologyHints(devPath string) (hints Hints, err error) {
-	hints = make(Hints)
-
-	realDevPath, err := filepath.EvalSymlinks(devPath)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed get realpath for %s", devPath)
-	}
-
-	for p := realDevPath; strings.HasPrefix(p, mockRoot+"/sys/devices/"); p = filepath.Dir(p) {
-		hint, er := getTopologyHint(p)
-		if er != nil {
-			return nil, er
-		}
-
-		if hint.CPUs != "" || hint.NUMAs != "" || hint.Sockets != "" {
-			hints[hint.Provider] = *hint
-			break
-		}
-	}
-
-	fromVirtual, _ := getDevicesFromVirtual(realDevPath)
-	deps, _ := filepath.Glob(filepath.Join(realDevPath, "slaves/*"))
-
-	for _, device := range append(deps, fromVirtual...) {
-		deviceHints, er := NewTopologyHints(device)
-		if er != nil {
-			return nil, er
-		}
-
-		hints = MergeTopologyHints(hints, deviceHints)
-	}
-
-	return hints, err
-}
-
-// MergeTopologyHints combines org and hints.
-func MergeTopologyHints(org, hints Hints) (res Hints) {
-	if org != nil {
-		res = org
-	} else {
-		res = make(Hints)
-	}
-
-	for k, v := range hints {
-		if _, ok := res[k]; ok {
-			continue
-		}
-
-		res[k] = v
-	}
-
-	return
-}
-
-// String returns the hints as a string.
-func (h *Hint) String() string {
-	cpus, nodes, sockets, sep := "", "", "", ""
-
-	if h.CPUs != "" {
-		cpus = "CPUs:" + h.CPUs
-		sep = ", "
-	}
-
-	if h.NUMAs != "" {
-		nodes = sep + "NUMAs:" + h.NUMAs
-		sep = ", "
-	}
-
-	if h.Sockets != "" {
-		sockets = sep + "sockets:" + h.Sockets
-	}
-
-	return "<hints " + cpus + nodes + sockets + " (from " + h.Provider + ")>"
-}
-
-// FindSysFsDevice for given argument returns physical device where it is linked to.
-// For device nodes it will return path for device itself. For regular files or directories
-// this function returns physical device where this inode resides (storage device).
-// If result device is a virtual one (e.g. tmpfs), error will be returned.
-// For non-existing path, no error returned and path is empty.
-func FindSysFsDevice(dev string) (string, error) {
-	fi, err := os.Stat(dev)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return "", nil
-		}
-
-		return "", errors.Wrapf(err, "unable to get stat for %s", dev)
-	}
-
-	devType := "block"
-	rdev := fi.Sys().(*syscall.Stat_t).Dev
-
-	if mode := fi.Mode(); mode&os.ModeDevice != 0 {
-		rdev = fi.Sys().(*syscall.Stat_t).Rdev
-
-		if mode&os.ModeCharDevice != 0 {
-			devType = "char"
-		}
-	}
-
-	major := unix.Major(rdev)
-	minor := unix.Minor(rdev)
-
-	if major == 0 {
-		return "", errors.Errorf("%s is a virtual device node", dev)
-	}
-
-	devPath := fmt.Sprintf("/sys/dev/%s/%d:%d", devType, major, minor)
-
-	realDevPath, err := filepath.EvalSymlinks(devPath)
-	if err != nil {
-		return "", errors.Wrapf(err, "failed get realpath for %s", devPath)
-	}
-
-	return filepath.Join(mockRoot, realDevPath), nil
-}
-
-// readFilesInDirectory small helper to fill struct with content from sysfs entry.
-func readFilesInDirectory(fileMap map[string]*string, dir string) error {
-	for k, v := range fileMap {
-		b, err := os.ReadFile(filepath.Join(dir, k))
-		if err != nil {
-			if os.IsNotExist(err) {
-				continue
-			}
-
-			return errors.Wrapf(err, "%s: unable to read file %q", dir, k)
-		}
-
-		*v = strings.TrimSpace(string(b))
-	}
-
-	return nil
-}
-
-// mapKeys is a small helper that returns slice of keys for a given map.
-func mapKeys(m map[string]bool) []string {
-	ret := make([]string, len(m))
-	i := 0
-
-	for k := range m {
-		ret[i] = k
-		i++
-	}
-
-	return ret
-}
-
 // GetTopologyInfo returns topology information for the list of device nodes.
 func GetTopologyInfo(devs []string) (*pluginapi.TopologyInfo, error) {
 	var result pluginapi.TopologyInfo
 
 	nodeIDs := map[int64]struct{}{}
 
 	for _, dev := range devs {
-		sysfsDevice, err := FindSysFsDevice(dev)
+		sysfsDevice, err := topo.FindSysFsDevice(dev)
 		if err != nil {
 			return nil, err
 		}
@@ -305,7 +40,7 @@ func GetTopologyInfo(devs []string) (*pluginapi.TopologyInfo, error) {
 			return nil, errors.Errorf("device %s doesn't exist", dev)
 		}
 
-		hints, err := NewTopologyHints(sysfsDevice)
+		hints, err := topo.NewTopologyHints(sysfsDevice)
 		if err != nil {
 			return nil, err
 		}

pkg/topology/topology.go

@@ -18,9 +18,9 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
-	"sort"
 	"testing"
 
+	topo "github.com/containers/nri-plugins/pkg/topology"
 	pluginapi "k8s.io/kubelet/pkg/apis/deviceplugin/v1beta1"
 )
 
@@ -34,300 +34,15 @@ func setupTestEnv(t *testing.T) func() {
 		pwd = path
 	}
 
-	mockRoot = pwd + "/testdata"
+	topo.SetSysRoot(pwd + "/testdata")
+
 	teardown := func() {
-		mockRoot = ""
+		topo.SetSysRoot("")
 	}
 
 	return teardown
 }
 
-func TestMapKeys(t *testing.T) {
-	cases := []struct {
-		name   string
-		input  map[string]bool
-		output []string
-	}{
-		{
-			name:   "empty",
-			input:  map[string]bool{},
-			output: []string{},
-		},
-		{
-			name:   "one",
-			input:  map[string]bool{"a": false},
-			output: []string{"a"},
-		},
-		{
-			name:   "multiple",
-			input:  map[string]bool{"a": false, "b": true, "c": false},
-			output: []string{"a", "b", "c"},
-		},
-	}
-	for _, tc := range cases {
-		test := tc
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-			output := mapKeys(test.input)
-			sort.Strings(output)
-			if !reflect.DeepEqual(output, test.output) {
-				t.Fatalf("expected output: %+v got: %+v", test.output, output)
-			}
-		})
-	}
-}
-
-func TestFindSysFsDevice(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping test in short mode.")
-	}
-
-	teardown := setupTestEnv(t)
-	defer teardown()
-
-	cases := []struct {
-		name        string
-		input       string
-		output      string
-		expectedErr bool
-	}{
-		{
-			name:        "empty",
-			input:       "",
-			output:      "",
-			expectedErr: false,
-		},
-		{
-			name:        "null",
-			input:       "/dev/null",
-			output:      "/sys/devices/virtual/mem/null",
-			expectedErr: false,
-		},
-		{
-			name:        "proc",
-			input:       "/proc/self",
-			output:      "",
-			expectedErr: true,
-		},
-	}
-	for _, tc := range cases {
-		test := tc
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-			output, err := FindSysFsDevice(test.input)
-			switch {
-			case err != nil && !test.expectedErr:
-				t.Fatalf("unexpected error returned: %+v", err)
-			case err == nil && test.expectedErr:
-				t.Fatalf("unexpected success: %+v", output)
-			case output != test.output:
-				t.Fatalf("expected: %q got: %q", test.output, output)
-			}
-		})
-	}
-}
-
-func TestReadFilesInDirectory(t *testing.T) {
-	var file, empty string
-
-	fname := "test-a"
-	content := []byte(" something\n")
-	expectedContent := "something"
-
-	fileMap := map[string]*string{
-		fname:          &file,
-		"non_existing": &empty,
-	}
-
-	dir, err := os.MkdirTemp("", "readFilesInDirectory")
-	if err != nil {
-		t.Fatalf("unable to create test directory: %+v", err)
-	}
-
-	defer os.RemoveAll(dir)
-
-	if err = os.WriteFile(filepath.Join(dir, fname), content, 0600); err != nil {
-		t.Fatalf("unexpected failure: %v", err)
-	}
-
-	if err = readFilesInDirectory(fileMap, dir); err != nil {
-		t.Fatalf("unexpected failure: %v", err)
-	}
-
-	if empty != "" {
-		t.Fatalf("unexpected content: %q", empty)
-	}
-
-	if file != expectedContent {
-		t.Fatalf("unexpected content: %q expected: %q", file, expectedContent)
-	}
-}
-
-func TestGetDevicesFromVirtual(t *testing.T) {
-	teardown := setupTestEnv(t)
-	defer teardown()
-
-	cases := []struct {
-		name        string
-		input       string
-		output      []string
-		expectedErr bool
-	}{
-		{
-			name:        "vfio",
-			input:       "/sys/devices/virtual/vfio/42",
-			output:      []string{mockRoot + "/sys/devices/pci0000:00/0000:00:02.0"},
-			expectedErr: false,
-		},
-		{
-			name:        "misc",
-			input:       "/sys/devices/virtual/misc/vfio",
-			output:      nil,
-			expectedErr: false,
-		},
-		{
-			name:        "missing-iommu-group",
-			input:       "/sys/devices/virtual/vfio/84",
-			output:      nil,
-			expectedErr: true,
-		},
-		{
-			name:        "non-virtual",
-			input:       "/sys/devices/pci0000:00/0000:00:02.0",
-			output:      nil,
-			expectedErr: true,
-		},
-		{
-			name:        "garbage",
-			input:       "./sys/devices/virtual/vfio/42",
-			output:      nil,
-			expectedErr: true,
-		},
-	}
-
-	for _, tc := range cases {
-		test := tc
-		t.Run(test.name, func(t *testing.T) {
-			output, err := getDevicesFromVirtual(test.input)
-			switch {
-			case err != nil && !test.expectedErr:
-				t.Fatalf("unexpected error returned: %+v", err)
-			case err == nil && test.expectedErr:
-				t.Fatalf("unexpected success: %+v", output)
-			case len(output) != len(test.output):
-				t.Fatalf("expected: %q got: %q", len(test.output), len(output))
-			}
-			for i, p := range test.output {
-				if test.output[i] != p {
-					t.Fatalf("expected: %q got: %q", test.output[i], p)
-				}
-			}
-		})
-	}
-}
-
-func TestMergeTopologyHints(t *testing.T) {
-	cases := []struct {
-		inputA         Hints
-		inputB         Hints
-		expectedOutput Hints
-		name           string
-		expectedErr    bool
-	}{
-		{
-			name:           "empty",
-			inputA:         nil,
-			inputB:         nil,
-			expectedOutput: Hints{},
-		},
-		{
-			name:           "one,nil",
-			inputA:         Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-			inputB:         nil,
-			expectedOutput: Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-		},
-		{
-			name:           "nil, one",
-			inputA:         nil,
-			inputB:         Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-			expectedOutput: Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-		},
-		{
-			name:           "duplicate",
-			inputA:         Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-			inputB:         Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-			expectedOutput: Hints{"test": Hint{Provider: "test", CPUs: "0"}},
-		},
-		{
-			name:   "two",
-			inputA: Hints{"test1": Hint{Provider: "test1", CPUs: "0"}},
-			inputB: Hints{"test2": Hint{Provider: "test2", CPUs: "1"}},
-			expectedOutput: Hints{
-				"test1": Hint{Provider: "test1", CPUs: "0"},
-				"test2": Hint{Provider: "test2", CPUs: "1"},
-			},
-		},
-	}
-	for _, tc := range cases {
-		test := tc
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-			output := MergeTopologyHints(test.inputA, test.inputB)
-			if !reflect.DeepEqual(output, test.expectedOutput) {
-				t.Fatalf("expected output: %+v got: %+v", test.expectedOutput, output)
-			}
-		})
-	}
-}
-
-func TestNewTopologyHints(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping test in short mode.")
-	}
-
-	teardown := setupTestEnv(t)
-	defer teardown()
-
-	cases := []struct {
-		output      Hints
-		name        string
-		input       string
-		expectedErr bool
-	}{
-		{
-			name:        "empty",
-			input:       "non-existing",
-			output:      nil,
-			expectedErr: true,
-		},
-		{
-			name:  "pci card1",
-			input: mockRoot + "/sys/devices/pci0000:00/0000:00:02.0/drm/card1",
-			output: Hints{
-				mockRoot + "/sys/devices/pci0000:00/0000:00:02.0": Hint{
-					Provider: mockRoot + "/sys/devices/pci0000:00/0000:00:02.0",
-					CPUs:     "0-7",
-					NUMAs:    "",
-					Sockets:  ""},
-			},
-			expectedErr: false,
-		},
-	}
-	for _, test := range cases {
-		t.Run(test.name, func(t *testing.T) {
-			output, err := NewTopologyHints(test.input)
-			switch {
-			case err != nil && !test.expectedErr:
-				t.Fatalf("unexpected error returned: %+v", err)
-			case err == nil && test.expectedErr:
-				t.Fatalf("unexpected success: %+v", output)
-			case !reflect.DeepEqual(output, test.output):
-				t.Fatalf("expected: %q got: %q", test.output, output)
-			}
-		})
-	}
-}
-
 func TestGetTopologyInfo(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping test in short mode.")