From e10254c5b33e3643b42359500e9cbbbfd93d9225 Mon Sep 17 00:00:00 2001
From: David Borman <77121405+dborman-hpe@users.noreply.github.com>
Date: Fri, 12 Aug 2022 19:28:26 +0000
Subject: [PATCH] fix(node-disk-manager): support nvme virtual paths

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.

Also 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.

Signed-off-by: David Borman <77121405+dborman-hpe@users.noreply.github.com>
---
 .../filter/osdiskexcludefilter.go             | 38 +++++++++----------
 pkg/mount/mountutil.go                        | 13 +++++--
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/cmd/ndm_daemonset/filter/osdiskexcludefilter.go b/cmd/ndm_daemonset/filter/osdiskexcludefilter.go
index c31c14253..d1ed776c6 100644
--- a/cmd/ndm_daemonset/filter/osdiskexcludefilter.go
+++ b/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
+
+		// 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
 		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 {
+
+		// 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
+		mountPointUtil = mount.NewMountUtil(defaultMountFilePath, "", mountPoint)
+		if devPath, err = mountPointUtil.GetDiskPath(); err == nil {
 			odf.excludeDevPaths = append(odf.excludeDevPaths, devPath)
+			continue
 		}
-	}
 
+		// Neither one succeeded, log the error
+		klog.Errorf("unable to configure os disk filter for mountpoint: %s, error: %v", mountPoint, err)
+	}
 }
 
 // Include contains nothing by default it returns false
diff --git a/pkg/mount/mountutil.go b/pkg/mount/mountutil.go
index e86eb158a..29ffbf463 100644
--- a/pkg/mount/mountutil.go
+++ b/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"
+	altNvmeSubsystem := "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 == altNvmeSubsystem) &&
 			len(parts)-1 >= i+2 {
 			return parts[i+2], true
 		}