Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix canonical nvme device resolution in more cases #1141

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Fix canonical nvme device resolution in more cases #1141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee7162a
Fix canonical nvme device resolution in more cases
PaulFurtado Dec 16, 2021
9ac900a
Fix tests
PaulFurtado Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions pkg/driver/node_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -48,12 +60,26 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st
}

if exists {
Copy link
Contributor

@wongma7 wongma7 Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we still try to fallback to the findNvmeVolume method in case there is an error in this block (if lstat or evalsymlinks somehow fails)? Is there any situation where we receive a devicePath that exists but cannot* be lstatted or evalsymlink'd? If not then it is not necessary to fallback and this PR is good as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wongma7 I cannot think of a situation where that could be possible.

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
Expand All @@ -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)
Expand All @@ -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
}

Expand Down
10 changes: 5 additions & 5 deletions pkg/driver/node_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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,
Expand Down
25 changes: 14 additions & 11 deletions pkg/driver/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
)
Expand Down Expand Up @@ -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)
},
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -993,14 +996,14 @@ 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),
)

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"},
Expand Down Expand Up @@ -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")),
)
Expand All @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down