fix(node-disk-manager): support nvme virtual paths (#678)

Enhance getParentBlockDevice() by looking not only
for /nvme/ in the path, but also /nvme-subystem/,
which is the name under /sys/devices/virtual on
OpenSUSE 15.3.

Clean up the osdiskexcludefilter.go Start()
routine so that for each mount point it will first
check /proc/1/mounts and if that fails, then check
/proc/self/mounts, and then if that also fails, finally
log an error message.  The observation is that things
will be found in one or the other, but not both, so
the old code would print out an error message in once
case, even when the mountpoint had been added to the
filter list by the other case.

Change "make test" so that it will run all the go
tests and not stop at the first failure.

Signed-off-by: David Borman <77121405+dborman-hpe@users.noreply.github.com>

Author: dborman-hpe

build/test.sh

@@ -30,12 +30,17 @@ if [ "$ARCH" != "amd64" ]; then
   exit 0
 fi
 
+exit_val=0
 echo "" > coverage.txt
 PACKAGES=$(go list ./... | grep -v '/vendor/\|/pkg/apis/\|/pkg/client/\|integration_test')
 for d in $PACKAGES; do
-	go test -coverprofile=profile.out -covermode=atomic "$d"
+	if ! go test -coverprofile=profile.out -covermode=atomic "$d"; then
+		exit_val=1
+	fi
 	if [ -f profile.out ]; then
 		cat profile.out >> coverage.txt
 		rm profile.out
 	fi
 done
+
+exit ${exit_val}

cmd/ndm_daemonset/filter/osdiskexcludefilter.go

@@ -80,33 +80,31 @@ func newNonOsDiskFilter(ctrl *controller.Controller) *oSDiskExcludeFilter {
 
 // Start set os disk devPath in nonOsDiskFilter pointer
 func (odf *oSDiskExcludeFilter) Start() {
-	/*
-		process1 check for mentioned mountpoint in host's /proc/1/mounts file
-		host's /proc/1/mounts file mounted inside container it checks for
-		every mentioned mountpoints if it is able to get parent disk's devpath
-		it adds it in Controller struct and make isOsDiskFilterSet true
-	*/
 	for _, mountPoint := range mountPoints {
+		var err error
+		var devPath string
+
+		// Check for mountpoints in both:
+		//    the host's /proc/1/mounts file
+		//    the /proc/self/mounts file
+		// If it is found in either one and we are able to get the
+		// disk's devpath, add it to the Controller struct.  Otherwise
+		// log an error.
+
 		mountPointUtil := mount.NewMountUtil(hostMountFilePath, "", mountPoint)
-		if devPath, err := mountPointUtil.GetDiskPath(); err != nil {
-			klog.Errorf("unable to configure os disk filter for mountpoint: %s, error: %v", mountPoint, err)
-		} else {
+		if devPath, err = mountPointUtil.GetDiskPath(); err == nil {
 			odf.excludeDevPaths = append(odf.excludeDevPaths, devPath)
+			continue
 		}
-	}
-	/*
-		process2 check for mountpoints in /proc/self/mounts file if it is able to get
-		disk's path of it adds it in Controller struct and make isOsDiskFilterSet true
-	*/
-	for _, mountPoint := range mountPoints {
-		mountPointUtil := mount.NewMountUtil(defaultMountFilePath, "", mountPoint)
-		if devPath, err := mountPointUtil.GetDiskPath(); err != nil {
-			klog.Errorf("unable to configure os disk filter for mountpoint: %s, error: %v", mountPoint, err)
-		} else {
+
+		mountPointUtil = mount.NewMountUtil(defaultMountFilePath, "", mountPoint)
+		if devPath, err = mountPointUtil.GetDiskPath(); err == nil {
 			odf.excludeDevPaths = append(odf.excludeDevPaths, devPath)
+			continue
 		}
-	}
 
+		klog.Errorf("unable to configure os disk filter for mountpoint: %s, error: %v", mountPoint, err)
+	}
 }
 
 // Include contains nothing by default it returns false

pkg/mount/mountutil.go

@@ -240,13 +240,18 @@ func parseRootDeviceLink(file io.Reader) (string, error) {
 // syspath looks like - /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda4
 // parent disk is present after block then partition of that disk
 //
-// for blockdevices that belong to the nvme subsystem, the symlink has a different format,
-// /sys/devices/pci0000:00/0000:00:0e.0/nvme/nvme0/nvme0n1/nvme0n1p1. So we search for the nvme subsystem
-// instead of `block`. The blockdevice will be available after the NVMe instance, nvme/instance/namespace.
+// for blockdevices that belong to the nvme subsystem, the symlink has different formats
+//     /sys/devices/pci0000:00/0000:00:0e.0/nvme/nvme0/nvme0n1/nvme0n1p1.
+//     /sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/nvme0n1p1
+// So we search for the "nvme" or "nvme-subsystem" subsystem instead of `block`. The
+// blockdevice will be available after the NVMe instance,
+//     nvme/instance/namespace
+//     nvme-subsystem/instance/namespace
 // The namespace will be the blockdevice.
 func getParentBlockDevice(sysPath string) (string, bool) {
 	blockSubsystem := "block"
 	nvmeSubsystem := "nvme"
+	nvmeSubsysClass := "nvme-subsystem"
 	parts := strings.Split(sysPath, "/")
 
 	// checking for block subsystem, return the next part after subsystem only
@@ -262,7 +267,7 @@ func getParentBlockDevice(sysPath string) (string, bool) {
 	// nvme namespace. Length checking is to avoid index out of range in case of malformed
 	// links (extremely rare case)
 	for i, part := range parts {
-		if part == nvmeSubsystem &&
+		if (part == nvmeSubsystem || part == nvmeSubsysClass) &&
 			len(parts)-1 >= i+2 {
 			return parts[i+2], true
 		}

pkg/mount/mountutil_test.go

@@ -291,6 +291,16 @@ func TestGetParentBlockDevice(t *testing.T) {
 			expectedParentBlockDevice: "nvme0n1",
 			expectedOk:                true,
 		},
+		"getting parent of main virtual NVMe blockdevice": {
+			syspath:                   "/sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1",
+			expectedParentBlockDevice: "nvme0n1",
+			expectedOk:                true,
+		},
+		"getting parent of partitioned virtual NVMe blockdevice": {
+			syspath:                   "/sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/nvme0n1p1",
+			expectedParentBlockDevice: "nvme0n1",
+			expectedOk:                true,
+		},
 		"getting parent of wrong disk": {
 			syspath:                   "/sys/devices/pci0000:00/0000:00:0e.0/nvme/nvme0",
 			expectedParentBlockDevice: "",

pkg/sysfs/syspath.go

@@ -31,6 +31,7 @@ const (
 	BlockSubSystem = "block"
 	// NVMeSubSystem is the key used to represent nvme subsystem in sysfs
 	NVMeSubSystem = "nvme"
+	NVMeSubSysClass = "nvme-subsystem"
 	// sectorSize is the sector size as understood by the unix systems.
 	// It is kept as 512 bytes. all entries in /sys/class/block/sda/size
 	// are in 512 byte blocks
@@ -86,7 +87,7 @@ func (s Device) getParent() (string, bool) {
 	// nvme namespace. Length checking is to avoid index out of range in case of malformed
 	// links (extremely rare case)
 	for i, part := range parts {
-		if part == NVMeSubSystem {
+		if part == NVMeSubSystem || part == NVMeSubSysClass {
 			// check if the length is greater to avoid panic. Also need to make sure that
 			// the same device is not returned if the given device is a parent.
 			if len(parts)-1 >= i+2 && s.deviceName != parts[i+2] {

pkg/sysfs/syspath_test.go

@@ -68,6 +68,24 @@ func TestGetParent(t *testing.T) {
 			wantedDeviceName: "nvme0n1",
 			wantOk:           true,
 		},
+		"[nvme-subsystem] given blockdevice is a parent": {
+			sysfsDevice: &Device{
+				deviceName: "nvme0n1",
+				path:       "/dev/nvme0n1",
+				sysPath:    "/sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/",
+			},
+			wantedDeviceName: "",
+			wantOk:           false,
+		},
+		"[nvme-subsystem] given blockdevice is a partition": {
+			sysfsDevice: &Device{
+				deviceName: "nvme0n1p1",
+				path:       "/dev/nvme0n1p1",
+				sysPath:    "/sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/nvme0n1p1/",
+			},
+			wantedDeviceName: "nvme0n1",
+			wantOk:           true,
+		},
 	}
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {