Merge pull request #137220 from ffromani/dra-helper-sysfs-repleaceable

DRA: Make GetPCIeRootAttributeByPCIBusID filesystem-independent

staging/src/k8s.io/dynamic-resource-allocation/deviceattribute/machine.go

@@ -0,0 +1,47 @@
+/*
+Copyright The Kubernetes Authors.
+
+Licensed 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 deviceattribute
+
+import (
+	"io/fs"
+	"os"
+)
+
+// machine is the bare minimum machine/host abstraction we need.
+// we use a wrapping struct to enable the functional option pattern
+type machine struct {
+	sysfs fs.ReadLinkFS
+}
+
+// MachineModifier is the type of functional options
+type MachineModifier func(*machine)
+
+// WithFS replaces the host sysfs with a custom FS
+func WithFS(sysfs fs.ReadLinkFS) MachineModifier {
+	return func(mc *machine) {
+		mc.sysfs = sysfs
+	}
+}
+
+// WithFSFromRoot replaces the host sysfs with a FS whose root
+// is the provided path. Like chroot, this is for convenience
+// and testability and for usage within containers, not for security.
+func WithFSFromRoot(root string) MachineModifier {
+	return func(mc *machine) {
+		mc.sysfs = os.DirFS(root).(fs.ReadLinkFS)
+	}
+}

staging/src/k8s.io/dynamic-resource-allocation/deviceattribute/machine_linux.go

@@ -16,25 +16,15 @@ limitations under the License.
 
 package deviceattribute
 
-import "path/filepath"
+import (
+	"io/fs"
+	"os"
+)
 
 const (
-	sysfsRoot = "/sys"
+	SysfsRoot = "/sys"
 )
 
-var (
-	sysfs sysfsPath = sysfsPath(sysfsRoot)
-)
-
-// sysfsPath provides methods to construct sysfs paths for various subsystems.
-// It is used to abstract the sysfs path construction
-// and can be replaced with a mock in tests.
-type sysfsPath string
-
-func (s sysfsPath) devices(path string) string {
-	return filepath.Join(string(s), "devices", path)
-}
-
-func (s sysfsPath) bus(path string) string {
-	return filepath.Join(string(s), "bus", path)
+func initDefaultMachine(mc *machine) {
+	mc.sysfs = os.DirFS(SysfsRoot).(fs.ReadLinkFS)
 }

staging/src/k8s.io/dynamic-resource-allocation/deviceattribute/machine_test.go

@@ -0,0 +1,68 @@
+/*
+Copyright The Kubernetes Authors.
+
+Licensed 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 deviceattribute
+
+import (
+	"io/fs"
+	"path/filepath"
+	"testing"
+	"testing/fstest"
+)
+
+func TestWithFS(t *testing.T) {
+	mc := machine{}
+	path := "dra.test"
+	content := "DRA deviceattribute"
+	mode := fs.FileMode(0o640) // any random mode pattern which is not the default
+	fakefs := fstest.MapFS{
+		path: &fstest.MapFile{
+			Data: []byte(content),
+			Mode: mode,
+		},
+	}
+
+	mod := WithFS(fakefs)
+	mod(&mc)
+
+	got, err := mc.sysfs.Lstat(path)
+	if err != nil {
+		t.Fatalf("cannot Lstat %q: %v", path, err)
+	}
+	if got.Size() != int64(len(content)) {
+		t.Fatalf("did not found content: got %v expected %v", got.Size(), content)
+	}
+	if got.Mode() != mode {
+		t.Fatalf("found mismatched file mode: got %v expected %v", got.Mode(), mode)
+	}
+}
+
+func TestWithFSFromRoot(t *testing.T) {
+	mc := machine{}
+	// we are gonna using real files from sysfs pseudofs, but we are using files which
+	// are standard since pretty much the inception of sysfs
+	mod := WithFSFromRoot("/sys")
+	mod(&mc)
+
+	srcPath := filepath.Join("devices", "system", "cpu", "online")
+	got, err := fs.ReadFile(mc.sysfs, srcPath)
+	if err != nil {
+		t.Fatalf("cannot read %q: %v", srcPath, err)
+	}
+	if len(got) == 0 {
+		t.Fatalf("empty content in %q", srcPath)
+	}
+}

staging/src/k8s.io/dynamic-resource-allocation/deviceattribute/pci_linux.go

@@ -18,7 +18,7 @@ package deviceattribute
 
 import (
 	"fmt"
-	"os"
+	"io/fs"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -33,12 +33,18 @@ import (
 // as a string value or an error if the PCI Bus ID is invalid or the root complex cannot be determined.
 //
 // ref: https://wiki.xenproject.org/wiki/Bus:Device.Function_(BDF)_Notation
-func GetPCIeRootAttributeByPCIBusID(pciBusID string) (DeviceAttribute, error) {
+func GetPCIeRootAttributeByPCIBusID(pciBusID string, mods ...MachineModifier) (DeviceAttribute, error) {
+	var mc machine
+	initDefaultMachine(&mc)
+	for _, mod := range mods {
+		mod(&mc)
+	}
+
 	if err := verifyPCIBDFFormat(pciBusID); err != nil {
 		return DeviceAttribute{}, err
 	}
 
-	pcieRoot, err := resolvePCIeRoot(pciBusID)
+	pcieRoot, err := resolvePCIeRoot(mc, pciBusID)
 	if err != nil {
 		return DeviceAttribute{}, fmt.Errorf("failed to resolve PCIe Root Complex for PCI Bus ID %s: %w", pciBusID, err)
 	}
@@ -67,22 +73,22 @@ func GetPCIeRootAttributeByPCIBusID(pciBusID string) (DeviceAttribute, error) {
 // /sys/bus/pci/devices/0000:04:1f.0 points to
 // /sys/devices/pci0000:00/...<intermediate PCI devices>.../0000:04:1f.0,
 // where "pci0000:00" is the PCIe Root.
-func resolvePCIeRoot(pciBusID string) (string, error) {
+func resolvePCIeRoot(mc machine, pciBusID string) (string, error) {
 	// e.g. /sys/bus/pci/devices/0000:04:1f.0
-	sysBusPath := sysfs.bus(filepath.Join("pci", "devices", pciBusID))
+	sysBusPath := filepath.Join("bus", "pci", "devices", pciBusID)
 
-	target, err := os.Readlink(sysBusPath)
+	target, err := fs.ReadLink(mc.sysfs, sysBusPath)
 	if err != nil {
 		return "", fmt.Errorf("failed to read symlink for PCI Bus ID %s: %w", sysBusPath, err)
 	}
 
 	// If the target is a relative path, we need to resolve it relative to the symlink's directory.
 	if !filepath.IsAbs(target) {
-		target = filepath.Join(filepath.Dir(sysBusPath), target)
+		target = filepath.Clean(filepath.Join(filepath.Dir(sysBusPath), target))
 	}
 
-	// targetAbs must be /sys/devices/pci0000:00/...<intermediate PCI devices>.../0000:04:1f.0
-	devicePathPrefix := sysfs.devices("pci")
+	// Once the symlink was resolved, we must have a path like /sys/devices/pci0000:00/...<intermediate PCI devices>.../0000:04:1f.0
+	devicePathPrefix := filepath.Join("devices", "pci")
 	if !strings.HasPrefix(target, devicePathPrefix) {
 		return "", fmt.Errorf("symlink target for PCI Bus ID %s is invalid: it must start with %s: %s", pciBusID, devicePathPrefix, target)
 	}
@@ -91,9 +97,9 @@ func resolvePCIeRoot(pciBusID string) (string, error) {
 	}
 
 	// We need to extract the PCIe Root part, which is the first part of the path after /sys/devices/.
-	pcieRootPart := strings.Split(strings.TrimPrefix(target, sysfs.devices("")+"/"), "/")[0]
-
-	return pcieRootPart, nil
+	target = strings.TrimPrefix(target, "devices"+string(filepath.Separator))
+	pcieRootParts := strings.Split(target, string(filepath.Separator))
+	return pcieRootParts[0], nil
 }
 
 // GetPCIBusIDAttribute returns a DeviceAttribute with the PCI Bus Address("<domain>:<bus>:<device>.<function>")

staging/src/k8s.io/dynamic-resource-allocation/deviceattribute/pci_linux_test.go

@@ -39,18 +39,22 @@ func TestGetPCIeRootAttributePCIBusID(t *testing.T) {
 	}
 
 	tests := map[string]struct {
-		mockSysfsSetup    func(t *testing.T, mockSysfs sysfsPath)
+		testMachineSetup  func(t *testing.T, testRootPath string)
 		address           string
 		expectedAttribute *DeviceAttribute
 		expectsError      bool
 		expectedErrMsg    string
 	}{
 		"valid": {
-			mockSysfsSetup: func(t *testing.T, mockSysfs sysfsPath) {
-				devicePath := mockSysfs.devices(filepath.Join(pcieRoot, intermediateBusID, pciBusID))
-				touchFile(t, devicePath)
-				busPath := mockSysfs.bus(filepath.Join("pci", "devices", pciBusID))
-				createSymlink(t, devicePath, busPath)
+			testMachineSetup: func(t *testing.T, testRootPath string) {
+				relDevicePath := filepath.Join("devices", pcieRoot, intermediateBusID, pciBusID)
+				touchFile(t, filepath.Join(testRootPath, relDevicePath))
+				busDir := filepath.Join(testRootPath, "bus", "pci", "devices")
+				mkDirAll(t, busDir)
+				createSymlink(t,
+					filepath.Join("..", "..", "..", relDevicePath),
+					filepath.Join(busDir, pciBusID),
+				)
 			},
 			address:           pciBusID,
 			expectedAttribute: &expectedAttribute,
@@ -75,11 +79,15 @@ func TestGetPCIeRootAttributePCIBusID(t *testing.T) {
 			expectedErrMsg:    "no such file or directory",
 		},
 		"invalid symlink (invalid prefix)": {
-			mockSysfsSetup: func(t *testing.T, mockSysfs sysfsPath) {
-				devicePath := mockSysfs.devices(filepath.Join("invalid-pci-root", intermediateBusID, pciBusID))
-				touchFile(t, devicePath)
-				busPath := mockSysfs.bus(filepath.Join("pci", "devices", pciBusID))
-				createSymlink(t, devicePath, busPath)
+			testMachineSetup: func(t *testing.T, testRootPath string) {
+				relDevicePath := filepath.Join("devices", "invalid-pci-root", intermediateBusID, pciBusID)
+				touchFile(t, filepath.Join(testRootPath, relDevicePath))
+				busDir := filepath.Join(testRootPath, "bus", "pci", "devices")
+				mkDirAll(t, busDir)
+				createSymlink(t,
+					filepath.Join("..", "..", "..", relDevicePath),
+					filepath.Join(busDir, pciBusID),
+				)
 			},
 			address:           pciBusID,
 			expectedAttribute: nil,
@@ -87,11 +95,15 @@ func TestGetPCIeRootAttributePCIBusID(t *testing.T) {
 			expectedErrMsg:    fmt.Sprintf("symlink target for PCI Bus ID %s is invalid: it must start with", pciBusID),
 		},
 		"invalid symlink (invalid suffix)": {
-			mockSysfsSetup: func(t *testing.T, mockSysfs sysfsPath) {
-				devicePath := mockSysfs.devices(filepath.Join(pcieRoot, intermediateBusID, "0000:00:13.1")) // different PCI Bus ID
-				touchFile(t, devicePath)
-				busPath := mockSysfs.bus(filepath.Join("pci", "devices", pciBusID))
-				createSymlink(t, devicePath, busPath)
+			testMachineSetup: func(t *testing.T, testRootPath string) {
+				relDevicePath := filepath.Join("devices", pcieRoot, intermediateBusID, "0000:00:13.1") // different PCI Bus ID
+				touchFile(t, filepath.Join(testRootPath, relDevicePath))
+				busDir := filepath.Join(testRootPath, "bus", "pci", "devices")
+				mkDirAll(t, busDir)
+				createSymlink(t,
+					filepath.Join("..", "..", "..", relDevicePath),
+					filepath.Join(busDir, pciBusID),
+				)
 			},
 			address:           pciBusID,
 			expectedAttribute: nil,
@@ -101,20 +113,12 @@ func TestGetPCIeRootAttributePCIBusID(t *testing.T) {
 	}
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			mockSysfsPath := t.TempDir()
-			mockSysfs := sysfsPath(mockSysfsPath)
-			if test.mockSysfsSetup != nil {
-				test.mockSysfsSetup(t, mockSysfs)
+			// per docs, the testing package ensures cleanup, so no need to do that ourselves
+			testMachinePath := t.TempDir()
+			if test.testMachineSetup != nil {
+				test.testMachineSetup(t, testMachinePath)
 			}
-			sysfs = mockSysfs
-			t.Cleanup(func() {
-				sysfs = sysfsPath(sysfsRoot)
-				if err := os.RemoveAll(mockSysfsPath); err != nil {
-					t.Errorf("Failed to clean up mock sysfs path %s: %v", mockSysfsPath, err)
-				}
-			})
-
-			got, err := GetPCIeRootAttributeByPCIBusID(test.address)
+			got, err := GetPCIeRootAttributeByPCIBusID(test.address, WithFSFromRoot(testMachinePath))
 			if test.expectsError {
 				if err == nil {
 					t.Errorf("Expected error but got none")
@@ -191,11 +195,16 @@ func TestGetPCIBusIDAttribute(t *testing.T) {
 	}
 }
 
+func mkDirAll(t *testing.T, path string) {
+	t.Helper()
+	if err := os.MkdirAll(path, 0755); err != nil {
+		t.Fatalf("Failed to create directory %s: %v", path, err)
+	}
+}
+
 func touchFile(t *testing.T, path string) {
 	t.Helper()
-	if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
-		t.Fatalf("Failed to create directory %s: %v", filepath.Dir(path), err)
-	}
+	mkDirAll(t, filepath.Dir(path))
 	if _, err := os.Create(path); err != nil {
 		t.Fatalf("Failed to create file %s: %v", path, err)
 	}
@@ -203,9 +212,6 @@ func touchFile(t *testing.T, path string) {
 
 func createSymlink(t *testing.T, target, link string) {
 	t.Helper()
-	if err := os.MkdirAll(filepath.Dir(link), 0755); err != nil {
-		t.Fatalf("Failed to create directory for symlink %s: %v", filepath.Dir(link), err)
-	}
 	if err := os.Symlink(target, link); err != nil {
 		t.Fatalf("Failed to create symlink from %s to %s: %v", target, link, err)
 	}