Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver
Pull request #1633 (2024-03-07 00:12:21)

Fix nvme path filtering logic for udevadm trigger
**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
This PR correctly filters out partition paths in `/dev/nvme`. The search path was updated in PR#1485, however this introduced logic which filtered out all `/dev/nvme` paths. The result was that the logic to trigger a `udev` event for a NVMe device did not work as expected.

**Which issue(s) this PR fixes**:
Fixes regression of search path https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1485/files

**Special notes for your reviewer**:
A follow-up PR will introduce an e2e test for a machine type that uses NVMe (current tests use SCSI), to prevent this type of regression in the future.

**Does this PR introduce a user-facing change?**:
```release-note
Fixes a regression in the NVMe device search path
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1633#" title="Author self-approved">pwschuurman</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fgcp-compute-persistent-disk-csi-driver).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/OWNERS)~~ [pwschuurman]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
pwschuurman: /assign @msau42 @Sneha-at 

Patch:
diff --git a/CHANGELOG/CHANGELOG-1.13.md b/CHANGELOG/CHANGELOG-1.13.md
index 4934dbb7..cc9cbacf 100644
--- a/CHANGELOG/CHANGELOG-1.13.md
+++ b/CHANGELOG/CHANGELOG-1.13.md
@@ -1,25 +1,3 @@
-# v1.13.3 - Changelog since v1.13.2
-
-## Changes by Kind
-
-### Uncategorized
-
-- Flag --use-instance-api-to-poll-attachment-disk-types uses instances.get API when polling for disk attachment in ControllerPublish for passed in disk types ([#1630](https://github.com/kube
-rnetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1630), [@k8s-infra-cherrypick-robot](https://github.com/k8s-infra-cherrypick-robot))
-- Support for read_ahead_kb mount flag to change block device readahead ([#1631](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1631), [@k8s-infra-cherrypick-
-robot](https://github.com/k8s-infra-cherrypick-robot))
-
-## Dependencies
-
-### Added
-_Nothing has changed._
-
-### Changed
-_Nothing has changed._
-
-### Removed
-_Nothing has changed._
-
 # v1.13.2 - Changelog since v1.13.1
 
 ## Changes by Kind
diff --git a/deploy/kubernetes/images/prow-stable-sidecar-rc-master/image.yaml b/deploy/kubernetes/images/prow-stable-sidecar-rc-master/image.yaml
index b8ce6237..77a8146f 100644
--- a/deploy/kubernetes/images/prow-stable-sidecar-rc-master/image.yaml
+++ b/deploy/kubernetes/images/prow-stable-sidecar-rc-master/image.yaml
@@ -48,6 +48,6 @@ metadata:
 imageTag:
   name: registry.k8s.io/cloud-provider-gcp/gcp-compute-persistent-disk-csi-driver
   newName: gcr.io/k8s-staging-cloud-provider-gcp/gcp-compute-persistent-disk-csi-driver
-  newTag: "v1.13.3-rc1"
+  newTag: "v1.13.2-rc1"
 ---
 
diff --git a/pkg/deviceutils/device-utils.go b/pkg/deviceutils/device-utils.go
index bdae2e2f..90214a3f 100644
--- a/pkg/deviceutils/device-utils.go
+++ b/pkg/deviceutils/device-utils.go
@@ -36,9 +36,10 @@ const (
 	diskScsiGooglePrefix = "scsi-0Google_PersistentDisk_"
 	diskPartitionSuffix  = "-part"
 	diskSDPath           = "/dev/sd"
-	diskSDPattern        = "/dev/sd*"
+	diskSDGlob           = "/dev/sd*"
 	diskNvmePath         = "/dev/nvme"
 	diskNvmePattern      = "^/dev/nvme[0-9]+n[0-9]+$"
+	diskNvmeGlob         = "/dev/nvme*"
 	// How many times to retry for a consistent read of /proc/mounts.
 	maxListTries = 3
 	// Number of fields per line in /proc/mounts as per the fstab man page.
@@ -69,6 +70,8 @@ var (
 	scsiRegex = regexp.MustCompile(scsiPattern)
 	// regex to parse google_nvme_id output and extract the serial
 	nvmeRegex = regexp.MustCompile(nvmePattern)
+	// regex to filter for disk drive paths from filepath.Glob output of diskNvmeGlob
+	diskNvmeRegex = regexp.MustCompile(diskNvmePattern)
 )
 
 // DeviceUtils are a collection of methods that act on the devices attached
@@ -306,15 +309,28 @@ func getDevFsSerial(devFsPath string) (string, error) {
 	}
 }
 
+func filterAvailableNvmeDevFsPaths(devNvmePaths []string) []string {
+	// Devices under /dev/nvme need to be filtered for disk drive paths only.
+	diskNvmePaths := []string{}
+	for _, devNvmePath := range devNvmePaths {
+		if diskNvmeRegex.MatchString(devNvmePath) {
+			diskNvmePaths = append(diskNvmePaths, devNvmePath)
+		}
+	}
+	return diskNvmePaths
+}
+
 func findAvailableDevFsPaths() ([]string, error) {
-	diskSDPaths, err := filepath.Glob(diskSDPattern)
+	diskSDPaths, err := filepath.Glob(diskSDGlob)
 	if err != nil {
-		return nil, fmt.Errorf("failed to filepath.Glob(\"%s\"): %w", diskSDPattern, err)
+		return nil, fmt.Errorf("failed to filepath.Glob(\"%s\"): %w", diskSDGlob, err)
 	}
-	diskNvmePaths, err := filepath.Glob(diskNvmePattern)
+	devNvmePaths, err := filepath.Glob(diskNvmeGlob)
 	if err != nil {
-		return nil, fmt.Errorf("failed to filepath.Glob(\"%s\"): %w", diskNvmePattern, err)
+		return nil, fmt.Errorf("failed to filepath.Glob(\"%s\"): %w", diskNvmeGlob, err)
 	}
+	// Devices under /dev/nvme need to be filtered for disk drive paths only.
+	diskNvmePaths := filterAvailableNvmeDevFsPaths(devNvmePaths)
 	return append(diskSDPaths, diskNvmePaths...), nil
 }
 


Test patch:
diff --git a/pkg/deviceutils/device-utils_test.go b/pkg/deviceutils/device-utils_test.go
index bf686af3..1026e3c1 100644
--- a/pkg/deviceutils/device-utils_test.go
+++ b/pkg/deviceutils/device-utils_test.go
@@ -2,7 +2,7 @@ package deviceutils
 
 import (
 	"fmt"
-	"regexp"
+	"path/filepath"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -64,8 +64,6 @@ func less(a, b fmt.Stringer) bool {
 // and may prevent our tests from running on all platforms. See the following test for an example:
 // https://github.com/golang/go/blob/d33548d178016122726342911f8e15016a691472/src/syscall/exec_linux_test.go#L250
 func TestDiskNvmePattern(t *testing.T) {
-	nvmeDiskRegex := regexp.MustCompile(diskNvmePattern)
-
 	testCases := []struct {
 		paths     []string
 		wantPaths []string
@@ -97,12 +95,19 @@ func TestDiskNvmePattern(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		gotPaths := []string{}
+		devPaths := []string{}
+
 		for _, path := range tc.paths {
-			if nvmeDiskRegex.MatchString(path) {
-				gotPaths = append(gotPaths, path)
+			ok, err := filepath.Match(diskNvmeGlob, path)
+			if err != nil {
+				t.Errorf("Error encountered matching path: %v", path)
+			}
+
+			if ok {
+				devPaths = append(devPaths, path)
 			}
 		}
+		gotPaths := filterAvailableNvmeDevFsPaths(devPaths)
 		if diff := cmp.Diff(gotPaths, tc.wantPaths, cmpopts.SortSlices(less)); diff != "" {
 			t.Errorf("Unexpected NVMe device paths (-got, +want):\n%s", diff)
 		}