Repository: openebs/node-disk-manager (go)

Issue:
NVME mount points not being recognized on OpenSUSE 15.3
**What steps did you take and what happened:**
[A clear and concise description of what the bug is, and the sequence of operations performed / commands you ran.]
I have NVME disks, and they are not being detected properly for filtering out mount points.  The issue is in the
`getParentBlockDevice()` routine at [getParentBlockDevice](https://github.com/openebs/node-disk-manager/blob/develop/pkg/mount/mountutil.go#L238-L271)

The code is looking for a `/nvme/` component, like in the given example of:
```
/sys/devices/pci0000:00/0000:00:0e.0/nvme/nvme0/nvme0n1/nvme0n1p1
```

However, on my OpenSUSE 15.2 and 15.3 systems, the paths are different, for example from the NDM logs:
```
E0707 17:40:05.246852       7 osdiskexcludefilter.go:92] unable to configure os disk filter for mountpoint: /mnt/openebs-local, error: could not find parent device for /sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/nvme0n1p1
```
There is a directory for each NVME disk, not just a single `/nvme/` directory:
```
# ls /sys/devices/virtual/nvme-subsystem
nvme-subsys0  nvme-subsys1  nvme-subsys2  nvme-subsys3  nvme-subsys4
```

**What did you expect to happen:**
The NVME code needs to be expanded to handle `/nvme-subsysX/` in addition to just `/nvme/`

**Environment:**
- OpenEBS version: 3.2 (NDM 1.9.0)
- Kubernetes version (use `kubectl version`): 1.20.15
- Kubernetes installer & version: kubeadm
- Cloud provider or hardware configuration: baremetal
- Type of disks connected to the nodes: NVME
- OS (e.g. from `/etc/os-release`): openSUSE Leap 15.3


Pull request:
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 one
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>

## Pull Request template

**Why is this PR required? What issue does it fix?**:
NDM failed to filter out nvme disks on OpenSUSE 15.3 due a difference in the path name

**What this PR does?**:
Adds looking for `nvme-subsystem` in addition to `nvme`

**Does this PR require any upgrade changes?**:
No

**If the changes in this PR are manually verified, list down the scenarios covered:**:
I have one mount point to be filtered out, `/mnt/openebs-local` which is mounted on `/dev/nvme0n1p1`
I looked at the logs, and now see lines like:
```
I0812 17:54:15.820739       7 osdiskexcludefilter.go:131] applying os-filter regex ^/dev/nvme0n1(p[0-9]+)?$ on /dev/nvme2n1
```
whereas before it didn't have that regex filter.

For the logs on creating the filters, it now has:
```
E0812 17:54:15.695414       7 osdiskexcludefilter.go:108] unable to configure os disk filter for mountpoint: /boot, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0812 17:54:15.697012       7 osdiskexcludefilter.go:108] unable to configure os disk filter for mountpoint: /var/lib/etcd, error: could not get device mount attributes, Path/MountPoint not present in mounts file
```
whereas before it had:
```
E0707 17:40:05.246362       7 osdiskexcludefilter.go:92] unable to configure os disk filter for mountpoint: /etc/hosts, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0707 17:40:05.246588       7 osdiskexcludefilter.go:92] unable to configure os disk filter for mountpoint: /boot, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0707 17:40:05.246852       7 osdiskexcludefilter.go:92] unable to configure os disk filter for mountpoint: /mnt/openebs-local, error: could not find parent device for /sys/devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/nvme0n1p1
E0707 17:40:05.247076       7 osdiskexcludefilter.go:92] unable to configure os disk filter for mountpoint: /var/lib/etcd, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0707 17:40:05.247203       7 osdiskexcludefilter.go:104] unable to configure os disk filter for mountpoint: /, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0707 17:40:05.247429       7 osdiskexcludefilter.go:104] unable to configure os disk filter for mountpoint: /boot, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0707 17:40:05.247522       7 osdiskexcludefilter.go:104] unable to configure os disk filter for mountpoint: /mnt/openebs-local, error: could not get device mount attributes, Path/MountPoint not present in mounts file
E0707 17:40:05.247612       7 osdiskexcludefilter.go:104] unable to configure os disk filter for mountpoint: /var/lib/etcd, error: could not get device mount attributes, Path/MountPoint not present in mounts file
```

**Any additional information for your reviewer?** : 
No

**Checklist:**
- [x] Fixes #675
- [x] PR Title follows the convention of  `<type>(<scope>): <subject>`
- [x] Has the change log section been updated? N/A - minor bugfix
- [x] Commit has unit tests
- [x] Commit has integration tests - N/A


Patch:
diff --git a/cmd/ndm_daemonset/filter/osdiskexcludefilter.go b/cmd/ndm_daemonset/filter/osdiskexcludefilter.go
index c31c1425..7c4ba19b 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
+
+		// 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
diff --git a/pkg/mount/mountutil.go b/pkg/mount/mountutil.go
index e86eb158..e7cd7ba5 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"
+	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
 		}
diff --git a/pkg/sysfs/syspath.go b/pkg/sysfs/syspath.go
index 73555ce6..8f12a6f9 100644
--- a/pkg/sysfs/syspath.go
+++ b/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] {


Test patch:
diff --git a/build/test.sh b/build/test.sh
index 6c5f3f32..f3963e80 100755
--- a/build/test.sh
+++ b/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}
diff --git a/pkg/mount/mountutil_test.go b/pkg/mount/mountutil_test.go
index 80f11287..7c0d6408 100644
--- a/pkg/mount/mountutil_test.go
+++ b/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: "",
diff --git a/pkg/sysfs/syspath_test.go b/pkg/sysfs/syspath_test.go
index 478d173a..7f4341df 100644
--- a/pkg/sysfs/syspath_test.go
+++ b/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) {