Merge pull request #1082 from wongma7/nodestageidempotent

Search for nvme device path even if non-nvme exists

Makefile

@@ -50,12 +50,12 @@ word-hyphen = $(word $2,$(subst -, ,$1))
 
 .EXPORT_ALL_VARIABLES:
 
-.PHONY: linux/$(ARCH)
+.PHONY: linux/$(ARCH) bin/aws-ebs-csi-driver
 linux/$(ARCH): bin/aws-ebs-csi-driver
 bin/aws-ebs-csi-driver: | bin
 	CGO_ENABLED=0 GOOS=linux GOARCH=$(ARCH) go build -mod=vendor -ldflags ${LDFLAGS} -o bin/aws-ebs-csi-driver ./cmd/
 
-.PHONY: windows/$(ARCH)
+.PHONY: windows/$(ARCH) bin/aws-ebs-csi-driver.exe
 windows/$(ARCH): bin/aws-ebs-csi-driver.exe
 bin/aws-ebs-csi-driver.exe: | bin
 	CGO_ENABLED=0 GOOS=windows GOARCH=$(ARCH) go build -mod=vendor -ldflags ${LDFLAGS} -o bin/aws-ebs-csi-driver.exe ./cmd/

hack/e2e/run.sh

@@ -42,7 +42,7 @@ REGION=${AWS_REGION:-us-west-2}
 ZONES=${AWS_AVAILABILITY_ZONES:-us-west-2a,us-west-2b,us-west-2c}
 FIRST_ZONE=$(echo "${ZONES}" | cut -d, -f1)
 NODE_COUNT=${NODE_COUNT:-3}
-INSTANCE_TYPE=${INSTANCE_TYPE:-c4.large}
+INSTANCE_TYPE=${INSTANCE_TYPE:-c5.large}
 
 AWS_ACCOUNT_ID=$(aws sts get-caller-identity --query Account --output text)
 IMAGE_NAME=${IMAGE_NAME:-${AWS_ACCOUNT_ID}.dkr.ecr.${REGION}.amazonaws.com/${DRIVER_NAME}}

pkg/driver/mount.go

@@ -17,6 +17,9 @@ limitations under the License.
 package driver
 
 import (
+	"os"
+	"path/filepath"
+
 	"github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/mounter"
 	mountutils "k8s.io/mount-utils"
 )
@@ -54,3 +57,27 @@ func newNodeMounter() (Mounter, error) {
 	}
 	return &NodeMounter{safeMounter}, nil
 }
+
+// DeviceIdentifier is for mocking os io functions used for the driver to
+// identify an EBS volume's corresponding device (in Linux, the path under
+// /dev; in Windows, the volume number) so that it can mount it. For volumes
+// already mounted, see GetDeviceNameFromMount.
+// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/nvme-ebs-volumes.html#identify-nvme-ebs-device
+type DeviceIdentifier interface {
+	Lstat(name string) (os.FileInfo, error)
+	EvalSymlinks(path string) (string, error)
+}
+
+type nodeDeviceIdentifier struct{}
+
+func newNodeDeviceIdentifier() DeviceIdentifier {
+	return &nodeDeviceIdentifier{}
+}
+
+func (i *nodeDeviceIdentifier) Lstat(name string) (os.FileInfo, error) {
+	return os.Lstat(name)
+}
+
+func (i *nodeDeviceIdentifier) EvalSymlinks(path string) (string, error) {
+	return filepath.EvalSymlinks(path)
+}

pkg/driver/node.go

@@ -73,10 +73,11 @@ var (
 
 // nodeService represents the node service of CSI driver
 type nodeService struct {
-	metadata      cloud.MetadataService
-	mounter       Mounter
-	inFlight      *internal.InFlight
-	driverOptions *DriverOptions
+	metadata         cloud.MetadataService
+	mounter          Mounter
+	deviceIdentifier DeviceIdentifier
+	inFlight         *internal.InFlight
+	driverOptions    *DriverOptions
 }
 
 // newNodeService creates a new node service
@@ -94,10 +95,11 @@ func newNodeService(driverOptions *DriverOptions) nodeService {
 	}
 
 	return nodeService{
-		metadata:      metadata,
-		mounter:       nodeMounter,
-		inFlight:      internal.NewInFlight(),
-		driverOptions: driverOptions,
+		metadata:         metadata,
+		mounter:          nodeMounter,
+		deviceIdentifier: newNodeDeviceIdentifier(),
+		inFlight:         internal.NewInFlight(),
+		driverOptions:    driverOptions,
 	}
 }
 

pkg/driver/node_linux.go

@@ -33,41 +33,62 @@ import (
 // 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
-func (d *nodeService) findDevicePath(devicePath, volumeID string, partition string) (string, error) {
+func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (string, error) {
+	canonicalDevicePath := ""
+
+	// If the given path exists, the device MAY be nvme. Further, it MAY be a
+	// symlink to the nvme device path like:
+	// | $ stat /dev/xvdba
+	// | File: ‘/dev/xvdba’ -> ‘nvme1n1’
+	// Since these are maybes, not guarantees, the search for the nvme device
+	// path below must happen and must rely on volume ID
 	exists, err := d.mounter.PathExists(devicePath)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("failed to check if path %q exists: %v", devicePath, err)
 	}
 
-	// If the path exists, assume it is not nvme device
 	if exists {
 		if partition != "" {
 			devicePath = devicePath + diskPartitionSuffix + partition
 		}
-		return devicePath, nil
+		canonicalDevicePath = devicePath
 	}
 
-	// Else find the nvme device path using volume ID
-	// This is the magic name on which AWS presents NVME devices under /dev/disk/by-id/
-	// For example, vol-0fab1d5e3f72a5e23 creates a symlink at
+	// 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
+	// which AWS presents NVME devices under /dev/disk/by-id/. For example,
+	// vol-0fab1d5e3f72a5e23 creates a symlink at
 	// /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol0fab1d5e3f72a5e23
 	nvmeName := "nvme-Amazon_Elastic_Block_Store_" + strings.Replace(volumeID, "-", "", -1)
 
-	nvmeDevicePath, err := findNvmeVolume(nvmeName)
-	if err != nil {
-		return "", err
+	nvmeDevicePath, err := findNvmeVolume(d.deviceIdentifier, nvmeName)
+
+	if err == nil {
+		if partition != "" {
+			nvmeDevicePath = nvmeDevicePath + nvmeDiskPartitionSuffix + partition
+		}
+		canonicalDevicePath = nvmeDevicePath
+	} else {
+		klog.V(5).Infof("[Debug] error searching for nvme path %q: %v", nvmeName, err)
 	}
-	if partition != "" {
-		nvmeDevicePath = nvmeDevicePath + nvmeDiskPartitionSuffix + partition
+
+	if canonicalDevicePath == "" {
+		return "", errNoDevicePathFound(devicePath, volumeID)
 	}
-	return nvmeDevicePath, nil
+
+	return canonicalDevicePath, nil
+}
+
+func errNoDevicePathFound(devicePath, volumeID string) error {
+	return fmt.Errorf("no device path for device %q volume %q found!", devicePath, volumeID)
 }
 
 // findNvmeVolume looks for the nvme volume with the specified name
 // It follows the symlink (if it exists) and returns the absolute path to the device
-func findNvmeVolume(findName string) (device string, err error) {
+func findNvmeVolume(deviceIdentifier DeviceIdentifier, findName string) (device string, err error) {
 	p := filepath.Join("/dev/disk/by-id/", findName)
-	stat, err := os.Lstat(p)
+	stat, err := deviceIdentifier.Lstat(p)
 	if err != nil {
 		if os.IsNotExist(err) {
 			klog.V(5).Infof("[Debug] nvme path %q not found", p)
@@ -82,7 +103,7 @@ func findNvmeVolume(findName string) (device string, err error) {
 	}
 	// Find the target, resolving to an absolute path
 	// For example, /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol0fab1d5e3f72a5e23 -> ../../nvme2n1
-	resolved, err := filepath.EvalSymlinks(p)
+	resolved, err := deviceIdentifier.EvalSymlinks(p)
 	if err != nil {
 		return "", fmt.Errorf("error reading target of symlink %q: %v", p, err)
 	}