Update VerifyDevicePath logic to run udevadm --trigger to fix scenarios where disk is not found or is wrong device

Author: davidz627

Dockerfile

@@ -20,11 +20,15 @@ ADD . .
 ARG TAG
 RUN CGO_ENABLED=0 GOOS=linux go build -a -ldflags '-X main.vendorVersion='"${TAG:-latest}"' -extldflags "-static"' -o bin/gce-pd-csi-driver ./cmd/
 
+# MAD HACKS: Build a version first so we can take the scsi_id bin and put it somewhere else in our real build
+FROM gcr.io/google-containers/debian-base-amd64:v2.0.0 as base
+RUN clean-install udev
+
 # Start from Google Debian base
 FROM gcr.io/google-containers/debian-base-amd64:v2.0.0
 COPY --from=builder /go/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/bin/gce-pd-csi-driver /gce-pd-csi-driver
-
 # Install necessary dependencies
 RUN clean-install util-linux e2fsprogs mount ca-certificates udev xfsprogs
+COPY --from=base /lib/udev/scsi_id /lib/udev_containerized/scsi_id
 
 ENTRYPOINT ["/gce-pd-csi-driver"]

pkg/gce-pd-csi-driver/node.go

@@ -528,7 +528,7 @@ func (ns *GCENodeServer) getDevicePath(volumeID string, partition string) (strin
 	}
 
 	devicePaths := ns.DeviceUtils.GetDiskByIdPaths(deviceName, partition)
-	devicePath, err := ns.DeviceUtils.VerifyDevicePath(devicePaths)
+	devicePath, err := ns.DeviceUtils.VerifyDevicePath(devicePaths, volumeKey.Name)
 
 	if err != nil {
 		return "", fmt.Errorf("error verifying GCE PD (%q) is attached: %v", volumeKey.Name, err)

pkg/mount-manager/device-utils.go

@@ -20,10 +20,11 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"regexp"
 	"strings"
 
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog"
+	pathutils "k8s.io/utils/path"
 )
 
 const (
@@ -46,6 +47,14 @@ const (
 	// 'fsck' found errors but exited without correcting them
 	fsckErrorsUncorrected = 4
 	defaultMountCommand   = "mount"
+	// scsi_id output should be in the form of:
+	// 0Google PersistentDisk <disk name>
+	scsiPattern = `^0Google\s+PersistentDisk\s+([\S]+)\s*$`
+)
+
+var (
+	// regex to parse scsi_id output and extract the serial
+	scsiRegex = regexp.MustCompile(scsiPattern)
 )
 
 // DeviceUtils are a collection of methods that act on the devices attached
@@ -57,7 +66,7 @@ type DeviceUtils interface {
 
 	// VerifyDevicePath returns the first of the list of device paths that
 	// exists on the machine, or an empty string if none exists
-	VerifyDevicePath(devicePaths []string) (string, error)
+	VerifyDevicePath(devicePaths []string, diskName string) (string, error)
 }
 
 type deviceUtils struct {
@@ -85,74 +94,129 @@ func (m *deviceUtils) GetDiskByIdPaths(deviceName string, partition string) []st
 	return devicePaths
 }
 
-// Returns the first path that exists, or empty string if none exist.
-func (m *deviceUtils) VerifyDevicePath(devicePaths []string) (string, error) {
-	sdBefore, err := filepath.Glob(diskSDPattern)
-	if err != nil {
-		// Seeing this error means that the diskSDPattern is malformed.
-		klog.Errorf("Error filepath.Glob(\"%s\"): %v\r\n", diskSDPattern, err)
-	}
-	sdBeforeSet := sets.NewString(sdBefore...)
-	// TODO(#69): Verify udevadm works as intended in driver
-	if err := udevadmChangeToNewDrives(sdBeforeSet); err != nil {
-		// udevadm errors should not block disk detachment, log and continue
-		klog.Errorf("udevadmChangeToNewDrives failed with: %v", err)
-	}
-
-	for _, path := range devicePaths {
-		if pathExists, err := pathExists(path); err != nil {
-			return "", fmt.Errorf("Error checking if path exists: %v", err)
+func existingDevicePath(devicePaths []string) (string, error) {
+	for _, devicePath := range devicePaths {
+		if pathExists, err := pathExists(devicePath); err != nil {
+			return "", fmt.Errorf("error checking if path exists: %v", err)
 		} else if pathExists {
-			return path, nil
+			return devicePath, nil
 		}
 	}
-
 	return "", nil
 }
 
-// Triggers the application of udev rules by calling "udevadm trigger
-// --action=change" for newly created "/dev/sd*" drives (exist only in
-// after set). This is workaround for Issue #7972. Once the underlying
-// issue has been resolved, this may be removed.
-func udevadmChangeToNewDrives(sdBeforeSet sets.String) error {
-	sdAfter, err := filepath.Glob(diskSDPattern)
+// getScsiSerial assumes that /lib/udev/scsi_id exists and will error if it
+// doesnt. It is the callers responsibility to verify the existence of this
+// tool. Calls scsi_id on the given devicePath to get the serial number reported
+// by that device.
+func getScsiSerial(devicePath string) (string, error) {
+	out, err := exec.Command(
+		"/lib/udev_containerized/scsi_id",
+		"--page=0x83",
+		"--whitelisted",
+		fmt.Sprintf("--device=%v", devicePath)).CombinedOutput()
 	if err != nil {
-		return fmt.Errorf("Error filepath.Glob(\"%s\"): %v\r\n", diskSDPattern, err)
+		return "", fmt.Errorf("scsi_id failed for device %q with output %s: %v", devicePath, string(out), err)
 	}
 
-	for _, sd := range sdAfter {
-		if !sdBeforeSet.Has(sd) {
-			return udevadmChangeToDrive(sd)
-		}
+	return parseScsiSerial(string(out))
+}
+
+// Parse the output returned by scsi_id and extract the serial number
+func parseScsiSerial(output string) (string, error) {
+	substrings := scsiRegex.FindStringSubmatch(output)
+	if substrings == nil {
+		return "", fmt.Errorf("scsi_id output cannot be parsed: %q", output)
 	}
 
-	return nil
+	return substrings[1], nil
 }
 
-// Calls "udevadm trigger --action=change" on the specified drive.
-// drivePath must be the block device path to trigger on, in the format "/dev/sd*", or a symlink to it.
-// This is workaround for Issue #7972. Once the underlying issue has been resolved, this may be removed.
-func udevadmChangeToDrive(drivePath string) error {
-	klog.V(4).Infof("udevadmChangeToDrive: drive=%q", drivePath)
+// VerifyDevicePath returns the first devicePath that maps to a real disk in the
+// candidate devicePaths or an empty string if none is found. If
+// /lib/udev/scsi_id exists it will attempt to fix any issues caused by missing
+// paths or mismatched devices by running a udevadm --trigger.
+func (m *deviceUtils) VerifyDevicePath(devicePaths []string, diskName string) (string, error) {
+	devicePath, err := existingDevicePath(devicePaths)
+	if err != nil {
+		return "", fmt.Errorf("failed to check for existing device path: %v", err)
+	}
 
-	// Evaluate symlink, if any
-	drive, err := filepath.EvalSymlinks(drivePath)
+	exists, err := pathutils.Exists(pathutils.CheckFollowSymlink, "/lib/udev_containerized/scsi_id")
 	if err != nil {
-		return fmt.Errorf("udevadmChangeToDrive: filepath.EvalSymlinks(%q) failed with %v.", drivePath, err)
+		return "", fmt.Errorf("failed to check scsi_id existence: %v", err)
+	}
+	if !exists {
+		// No SCSI ID tool, no udevadm fixing is possible so just return best
+		// effort devicePath.
+		return devicePath, nil
+	}
+
+	if len(devicePath) == 0 {
+		// Couldn't find the path so we need to find a /dev/sdx with the SCSI
+		// serial that matches diskName. Then we run udevadm trigger on that
+		// device to get the device to show up in /dev/by-id/
+		sds, err := filepath.Glob(diskSDPattern)
+		if err != nil {
+			return "", fmt.Errorf("failed to filepath.Glob(\"%s\"): %v", diskSDPattern, err)
+		}
+		for _, devSDX := range sds {
+			scsiSerial, err := getScsiSerial(devSDX)
+			if err != nil {
+				return "", fmt.Errorf("failed to get SCSI Serial num for %s: %v", devSDX, err)
+			}
+			if scsiSerial == diskName {
+				// Found the disk that we're looking for so run a trigger on it
+				// to resolve its /dev/by-id/ path
+				err := udevadmChangeToDrive(devSDX)
+				if err != nil {
+					return "", fmt.Errorf("failed to fix disk at path %s which has SCSI ID %s when expecting %s: %v", devicePath, scsiSerial, diskName, err)
+				}
+				break
+			}
+		}
+		// Try get the deviceName again after potentially fixing it with the
+		// udev command
+		return existingDevicePath(devicePaths)
+	}
+
+	// If there exists a devicePath we make sure disk at /dev/sdx matches the
+	// expected disk at devicePath by matching SCSI Serial to the disk name
+	devSDX, err := filepath.EvalSymlinks(devicePath)
+	if err != nil {
+		return "", fmt.Errorf("filepath.EvalSymlinks(%q) failed with %v.", devicePath, err)
 	}
 	// Check to make sure input is "/dev/sd*"
-	if !strings.Contains(drive, diskSDPath) {
-		return fmt.Errorf("udevadmChangeToDrive: expected input in the form \"%s\" but drive is %q.", diskSDPattern, drive)
+	if !strings.Contains(devSDX, diskSDPath) {
+		return "", fmt.Errorf("expected input in the form \"%s\" but drive is %q.", diskSDPattern, devSDX)
 	}
+	scsiSerial, err := getScsiSerial(devSDX)
+	if err != nil {
+		return "", fmt.Errorf("couldn't get SCSI serial number for disk %s: %v", diskName, err)
+	}
+	if scsiSerial != diskName {
+		klog.Warningf("Disk found at path %s has SCSI serial ID %s but expected %s. Running udevadm trigger to resolve", devicePath, scsiSerial, diskName)
+		err := udevadmChangeToDrive(devSDX)
+		if err != nil {
+			return "", fmt.Errorf("failed to fix disk at path %s which has SCSI ID %s when expecting %s: %v", devicePath, scsiSerial, diskName, err)
+		}
+	}
+	return devicePath, nil
+}
 
+// Calls "udevadm trigger --action=change" on the specified drive.
+// drivePath must be the block device path to trigger on, in the format "/dev/sd*", or a symlink to it.
+// This is workaround for Issue #7972. Once the underlying issue has been resolved, this may be removed.
+func udevadmChangeToDrive(devSDX string) error {
+	klog.V(4).Infof("udevadmChangeToDrive: drive=%q", devSDX)
 	// Call "udevadm trigger --action=change --property-match=DEVNAME=/dev/sd..."
-	_, err = exec.Command(
+	_, err := exec.Command(
 		"udevadm",
 		"trigger",
 		"--action=change",
-		fmt.Sprintf("--property-match=DEVNAME=%s", drive)).CombinedOutput()
+		fmt.Sprintf("--property-match=DEVNAME=%s", devSDX)).CombinedOutput()
 	if err != nil {
-		return fmt.Errorf("udevadmChangeToDrive: udevadm trigger failed for drive %q with %v.", drive, err)
+		return fmt.Errorf("udevadmChangeToDrive: udevadm trigger failed for drive %q with %v.", devSDX, err)
 	}
 	return nil
 }

pkg/mount-manager/fake-device-utils.go

@@ -30,7 +30,7 @@ func (m *fakeDeviceUtils) GetDiskByIdPaths(pdName string, partition string) []st
 }
 
 // Returns the first path that exists, or empty string if none exist.
-func (m *fakeDeviceUtils) VerifyDevicePath(devicePaths []string) (string, error) {
+func (m *fakeDeviceUtils) VerifyDevicePath(devicePaths []string, diskName string) (string, error) {
 	// Return any random device path to use as mount source
 	return "/dev/disk/fake-path", nil
 }