Repository: kubernetes-sigs/aws-ebs-csi-driver
Pull request #1141 (2021-12-16 03:24:02)

Fix canonical nvme device resolution in more cases
**Is this a bug fix or adding new feature?**
Bug fix further improving on https://github.com/kubernetes-sigs/aws-ebs-csi-driver/pull/1082

**What is this PR about? / Why do we need it?**
- It allows the `/dev/xvdXX` symlinks to continue to function, but properly identifies them as nvme devices based on the `/dev/nvme` prefix
- No matter what kind of symlinking is setup, it will always find the canonical block device because all symlinks are always resolved now. This means that if some crazy non-nitro system had `/dev/xvdb` symlinked to `/dev/sdb`, even that would work correctly.
- nvme devices resolved via the `/dev/xvdXX` symlinks will now correctly handle appending the partition string
- The `/dev/disk/by-id` fallback continues to work correctly
- It fixes logging for `node_linux.go`: this file was using klog v2 while the rest of the repo was using v1 so its logging was going nowhere

**What testing is done?** 
- Tested on:
  - a kubelet with just the `/dev/xvdXX` udev rules for nvme devices
  - a kubelet with just the `/dev/disk/by-id` udev rules for nvme devices
  - a kubelet with both sets of udev rules present
- On each of those, I confirmed that:
  - pods with PVCs can be created and deleted successfully
  - when kubelet in 1.21 is restarted, the CSI driver correctly recognizes that the volumes are already mounted and the operation is idempotent


Patch:
diff --git a/pkg/driver/node_linux.go b/pkg/driver/node_linux.go
index fa72ba16..9477eefe 100644
--- a/pkg/driver/node_linux.go
+++ b/pkg/driver/node_linux.go
@@ -27,9 +27,21 @@ import (
 	"strings"
 
 	"golang.org/x/sys/unix"
-	"k8s.io/klog/v2"
+	"k8s.io/klog"
 )
 
+func (d *nodeService) appendPartition(devicePath, partition string) string {
+	if partition == "" {
+		return devicePath
+	}
+
+	if strings.HasPrefix(devicePath, "/dev/nvme") {
+		return devicePath + nvmeDiskPartitionSuffix + partition
+	}
+
+	return devicePath + diskPartitionSuffix + partition
+}
+
 // findDevicePath finds path of device and verifies its existence
 // if the device is not nvme, return the path directly
 // if the device is nvme, finds and returns the nvme device path eg. /dev/nvme1n1
@@ -48,12 +60,26 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st
 	}
 
 	if exists {
-		if partition != "" {
-			devicePath = devicePath + diskPartitionSuffix + partition
+		stat, err := d.deviceIdentifier.Lstat(devicePath)
+		if err != nil {
+			return "", fmt.Errorf("failed to lstat %q: %v", devicePath, err)
 		}
-		canonicalDevicePath = devicePath
+
+		if stat.Mode()&os.ModeSymlink == os.ModeSymlink {
+			canonicalDevicePath, err = d.deviceIdentifier.EvalSymlinks(devicePath)
+			if err != nil {
+				return "", fmt.Errorf("failed to evaluate symlink %q: %v", devicePath, err)
+			}
+		} else {
+			canonicalDevicePath = devicePath
+		}
+
+		klog.V(5).Infof("[Debug] The canonical device path for %q was resolved to: %q", devicePath, canonicalDevicePath)
+		return d.appendPartition(canonicalDevicePath, partition), nil
 	}
 
+	klog.V(5).Infof("[Debug] Falling back to nvme volume ID lookup for: %q", devicePath)
+
 	// AWS recommends identifying devices by volume ID
 	// (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/nvme-ebs-volumes.html),
 	// so find the nvme device path using volume ID. This is the magic name on
@@ -65,9 +91,7 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st
 	nvmeDevicePath, err := findNvmeVolume(d.deviceIdentifier, nvmeName)
 
 	if err == nil {
-		if partition != "" {
-			nvmeDevicePath = nvmeDevicePath + nvmeDiskPartitionSuffix + partition
-		}
+		klog.V(5).Infof("[Debug] successfully resolved nvmeName=%q to %q", nvmeName, nvmeDevicePath)
 		canonicalDevicePath = nvmeDevicePath
 	} else {
 		klog.V(5).Infof("[Debug] error searching for nvme path %q: %v", nvmeName, err)
@@ -77,6 +101,7 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st
 		return "", errNoDevicePathFound(devicePath, volumeID)
 	}
 
+	canonicalDevicePath = d.appendPartition(canonicalDevicePath, partition)
 	return canonicalDevicePath, nil
 }
 


Test patch:
diff --git a/pkg/driver/node_linux_test.go b/pkg/driver/node_linux_test.go
index fa3a5900..a7a656eb 100644
--- a/pkg/driver/node_linux_test.go
+++ b/pkg/driver/node_linux_test.go
@@ -36,7 +36,9 @@ func TestFindDevicePath(t *testing.T) {
 	nvmeDevicePath := "/dev/nvme1n1"
 	volumeID := "vol-test"
 	nvmeName := "/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_voltest"
+	deviceFileInfo := fs.FileInfo(&fakeFileInfo{devicePath, os.ModeDevice})
 	symlinkFileInfo := fs.FileInfo(&fakeFileInfo{nvmeName, os.ModeSymlink})
+	nvmeDevicePathSymlinkFileInfo := fs.FileInfo(&fakeFileInfo{nvmeDevicePath, os.ModeSymlink})
 	type testCase struct {
 		name             string
 		devicePath       string
@@ -55,9 +57,8 @@ func TestFindDevicePath(t *testing.T) {
 			expectMock: func(mockMounter MockMounter, mockDeviceIdentifier MockDeviceIdentifier) {
 				gomock.InOrder(
 					mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
-
-					mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(symlinkFileInfo, nil),
-					mockDeviceIdentifier.EXPECT().EvalSymlinks(gomock.Eq(symlinkFileInfo.Name())).Return(nvmeDevicePath, nil),
+					mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(nvmeDevicePathSymlinkFileInfo, nil),
+					mockDeviceIdentifier.EXPECT().EvalSymlinks(gomock.Eq(devicePath)).Return(nvmeDevicePath, nil),
 				)
 			},
 			expectDevicePath: nvmeDevicePath,
@@ -70,8 +71,7 @@ func TestFindDevicePath(t *testing.T) {
 			expectMock: func(mockMounter MockMounter, mockDeviceIdentifier MockDeviceIdentifier) {
 				gomock.InOrder(
 					mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
-
-					mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist),
+					mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil),
 				)
 			},
 			expectDevicePath: devicePath,
diff --git a/pkg/driver/node_test.go b/pkg/driver/node_test.go
index e075b966..7b2096a6 100644
--- a/pkg/driver/node_test.go
+++ b/pkg/driver/node_test.go
@@ -45,8 +45,10 @@ func TestNodeStageVolume(t *testing.T) {
 	var (
 		targetPath     = "/test/path"
 		devicePath     = "/dev/fake"
-		nvmeDevicePath = "/dev/fakenvme1n1"
-		stdVolCap      = &csi.VolumeCapability{
+		nvmeDevicePath = "/dev/nvmefake1n1"
+		deviceFileInfo = fs.FileInfo(&fakeFileInfo{devicePath, os.ModeDevice})
+		//deviceSymlinkFileInfo = fs.FileInfo(&fakeFileInfo{nvmeDevicePath, os.ModeSymlink})
+		stdVolCap = &csi.VolumeCapability{
 			AccessType: &csi.VolumeCapability_Mount{
 				Mount: &csi.VolumeCapability_MountVolume{
 					FsType: FSTypeExt4,
@@ -67,7 +69,7 @@ func TestNodeStageVolume(t *testing.T) {
 			mockMounter.EXPECT().MakeDir(targetPath).Return(nil)
 			mockMounter.EXPECT().GetDeviceNameFromMount(targetPath).Return("", 1, nil)
 			mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil)
-			mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+			mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 			mockMounter.EXPECT().NeedResize(gomock.Eq(devicePath), gomock.Eq(targetPath)).Return(false, nil)
 		}
 	)
@@ -188,7 +190,7 @@ func TestNodeStageVolume(t *testing.T) {
 				mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, nil)
 				mockMounter.EXPECT().GetDeviceNameFromMount(targetPath).Return(devicePath, 1, nil)
 				mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 
 				mockMounter.EXPECT().FormatAndMount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
 			},
@@ -233,7 +235,7 @@ func TestNodeStageVolume(t *testing.T) {
 				mockMounter.EXPECT().MakeDir(targetPath).Return(nil)
 				mockMounter.EXPECT().GetDeviceNameFromMount(targetPath).Return("", 1, nil)
 				mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 
 				// The device path argument should be canonicalized to contain the
 				// partition
@@ -594,6 +596,7 @@ func TestNodePublishVolume(t *testing.T) {
 	targetPath := "/test/path"
 	stagingTargetPath := "/test/staging/path"
 	devicePath := "/dev/fake"
+	deviceFileInfo := fs.FileInfo(&fakeFileInfo{devicePath, os.ModeDevice})
 	stdVolCap := &csi.VolumeCapability{
 		AccessType: &csi.VolumeCapability_Mount{
 			Mount: &csi.VolumeCapability_MountVolume{},
@@ -904,7 +907,7 @@ func TestNodePublishVolume(t *testing.T) {
 				mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
 				mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil)
 				mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 
 				req := &csi.NodePublishVolumeRequest{
 					PublishContext:    map[string]string{DevicePathKey: "/dev/fake"},
@@ -951,7 +954,7 @@ func TestNodePublishVolume(t *testing.T) {
 				mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil)
 				mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
 				mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(false, nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 
 				req := &csi.NodePublishVolumeRequest{
 					PublishContext:    map[string]string{DevicePathKey: "/dev/fake"},
@@ -993,6 +996,7 @@ func TestNodePublishVolume(t *testing.T) {
 
 				gomock.InOrder(
 					mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
+					mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil),
 					mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil),
 					mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, nil),
 				)
@@ -1000,7 +1004,6 @@ func TestNodePublishVolume(t *testing.T) {
 				mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil)
 				mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
 				mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, errors.New("Internal System Error"))
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
 
 				req := &csi.NodePublishVolumeRequest{
 					PublishContext:    map[string]string{DevicePathKey: "/dev/fake"},
@@ -1040,6 +1043,7 @@ func TestNodePublishVolume(t *testing.T) {
 
 				gomock.InOrder(
 					mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
+					mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil),
 					mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil),
 					mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(true, errors.New("CorruptedMntError")),
 				)
@@ -1051,7 +1055,6 @@ func TestNodePublishVolume(t *testing.T) {
 				mockMounter.EXPECT().Unmount(gomock.Eq(targetPath)).Return(nil)
 				mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, errors.New("Internal System Error"))
 				mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Any(), gomock.Any()).Return(nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
 
 				req := &csi.NodePublishVolumeRequest{
 					PublishContext:    map[string]string{DevicePathKey: "/dev/fake"},
@@ -1099,7 +1102,7 @@ func TestNodePublishVolume(t *testing.T) {
 				mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
 				mockMounter.EXPECT().Mount(gomock.Eq(devicePathWithPartition), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil)
 				mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 
 				req := &csi.NodePublishVolumeRequest{
 					PublishContext:    map[string]string{DevicePathKey: "/dev/fake"},
@@ -1148,7 +1151,7 @@ func TestNodePublishVolume(t *testing.T) {
 				mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
 				mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil)
 				mockMounter.EXPECT().IsLikelyNotMountPoint(gomock.Eq(targetPath)).Return(true, nil)
-				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(nvmeName)).Return(nil, os.ErrNotExist)
+				mockDeviceIdentifier.EXPECT().Lstat(gomock.Eq(devicePath)).Return(deviceFileInfo, nil)
 
 				req := &csi.NodePublishVolumeRequest{
 					PublishContext:    map[string]string{DevicePathKey: "/dev/fake"},