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

Fix review comments:

- Add test cases to mountutil_test.go
- Add test cases to syspath_test.go
- Change variable name for "nvme-subsystem"
- Clean up comments in oSDiskExcludeFilter()

Signed-off-by: David Borman <[email protected]>

cmd/ndm_daemonset/filter/osdiskexcludefilter.go

@@ -84,25 +84,25 @@ func (odf *oSDiskExcludeFilter) Start() {
 		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
+		// 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 {
 			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
 		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)
 	}
 }

pkg/mount/mountutil.go

@@ -251,7 +251,7 @@ func parseRootDeviceLink(file io.Reader) (string, error) {
 func getParentBlockDevice(sysPath string) (string, bool) {
 	blockSubsystem := "block"
 	nvmeSubsystem := "nvme"
-	altNvmeSubsystem := "nvme-subsystem"
+	nvmeSubsysClass := "nvme-subsystem"
 	parts := strings.Split(sysPath, "/")
 
 	// checking for block subsystem, return the next part after subsystem only
@@ -267,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 || part == altNvmeSubsystem) &&
+		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,7 +31,7 @@ const (
 	BlockSubSystem = "block"
 	// NVMeSubSystem is the key used to represent nvme subsystem in sysfs
 	NVMeSubSystem = "nvme"
-	altNVMeSubSystem = "nvme-subsystem"
+	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
@@ -87,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 || part == altNVMeSubSystem {
+		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) {