Resolve disk name collisions for attached regional disks

davidz627 · davidz627 · commit 3dec96ecb6be · 2018-09-13T16:59:06.000-07:00
diff --git a/pkg/common/utils.go b/pkg/common/utils.go
@@ -38,6 +38,8 @@ const (
 	nodeIDZoneValue     = 0
 	nodeIDNameValue     = 1
 	nodeIDTotalElements = 2
+
+	regionalDeviceNameSuffix = "_regional"
 )
 
 func BytesToGb(bytes int64) int64 {
@@ -90,3 +92,14 @@ func GetRegionFromZones(zones []string) (string, error) {
 	}
 	return regions.UnsortedList()[0], nil
 }
+
+func GetDeviceName(volKey *meta.Key) (string, error) {
+	switch volKey.Type() {
+	case meta.Zonal:
+		return volKey.Name, nil
+	case meta.Regional:
+		return volKey.Name + regionalDeviceNameSuffix, nil
+	default:
+		return "", fmt.Errorf("volume key %v neither zonal nor regional", volKey.Name)
+	}
+}
diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go
@@ -300,8 +300,12 @@ func (cloud *CloudProvider) deleteRegionalDisk(ctx context.Context, region, name
 func (cloud *CloudProvider) AttachDisk(ctx context.Context, disk *CloudDisk, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string) error {
 	source := cloud.GetDiskSourceURI(disk, volKey)
 
+	deviceName, err := common.GetDeviceName(volKey)
+	if err != nil {
+		return fmt.Errorf("failed to get device name: %v", err)
+	}
 	attachedDiskV1 := &compute.AttachedDisk{
-		DeviceName: disk.GetName(),
+		DeviceName: deviceName,
 		Kind:       disk.GetKind(),
 		Mode:       readWrite,
 		Source:     source,
@@ -320,7 +324,12 @@ func (cloud *CloudProvider) AttachDisk(ctx context.Context, disk *CloudDisk, vol
 }
 
 func (cloud *CloudProvider) DetachDisk(ctx context.Context, volKey *meta.Key, instanceZone, instanceName string) error {
-	op, err := cloud.service.Instances.DetachDisk(cloud.project, instanceZone, instanceName, volKey.Name).Context(ctx).Do()
+	deviceName, err := common.GetDeviceName(volKey)
+	if err != nil {
+		return err
+	}
+
+	op, err := cloud.service.Instances.DetachDisk(cloud.project, instanceZone, instanceName, deviceName).Context(ctx).Do()
 	if err != nil {
 		return err
 	}
diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go
@@ -251,7 +251,12 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
 		readWrite = "READ_ONLY"
 	}
 
-	attached, err := diskIsAttachedAndCompatible(disk, instance, volumeCapability, readWrite)
+	deviceName, err := common.GetDeviceName(volKey)
+	if err != nil {
+		return nil, status.Error(codes.Internal, fmt.Sprintf("error getting device name: %v", err))
+	}
+
+	attached, err := diskIsAttachedAndCompatible(deviceName, instance, volumeCapability, readWrite)
 	if err != nil {
 		return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("Disk %v already published to node %v but incompatbile: %v", volKey.Name, nodeID, err))
 	}
@@ -298,10 +303,6 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
 		return nil, err
 	}
 
-	disk, err := gceCS.CloudProvider.GetDisk(ctx, volKey)
-	if err != nil {
-		return nil, err
-	}
 	instanceZone, instanceName, err := common.NodeIDToZoneAndName(nodeID)
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("could not split nodeID: %v", err))
@@ -311,7 +312,12 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
 		return nil, err
 	}
 
-	attached := diskIsAttached(disk, instance)
+	deviceName, err := common.GetDeviceName(volKey)
+	if err != nil {
+		return nil, status.Error(codes.Internal, fmt.Sprintf("error getting device name: %v", err))
+	}
+
+	attached := diskIsAttached(deviceName, instance)
 
 	if !attached {
 		// Volume is not attached to node. Success!
@@ -473,19 +479,19 @@ func getRequestCapacity(capRange *csi.CapacityRange) (int64, error) {
 	return capBytes, nil
 }
 
-func diskIsAttached(volume *gce.CloudDisk, instance *compute.Instance) bool {
+func diskIsAttached(deviceName string, instance *compute.Instance) bool {
 	for _, disk := range instance.Disks {
-		if disk.DeviceName == volume.GetName() {
+		if disk.DeviceName == deviceName {
 			// Disk is attached to node
 			return true
 		}
 	}
 	return false
 }
 
-func diskIsAttachedAndCompatible(volume *gce.CloudDisk, instance *compute.Instance, volumeCapability *csi.VolumeCapability, readWrite string) (bool, error) {
+func diskIsAttachedAndCompatible(deviceName string, instance *compute.Instance, volumeCapability *csi.VolumeCapability, readWrite string) (bool, error) {
 	for _, disk := range instance.Disks {
-		if disk.DeviceName == volume.GetName() {
+		if disk.DeviceName == deviceName {
 			// Disk is attached to node
 			if disk.Mode != readWrite {
 				return true, fmt.Errorf("disk mode does not match. Got %v. Want %v", disk.Mode, readWrite)
diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go
@@ -604,15 +604,13 @@ func TestGetRequestCapacity(t *testing.T) {
 func TestDiskIsAttached(t *testing.T) {
 	testCases := []struct {
 		name        string
-		disk        *compute.Disk
+		deviceName  string
 		instance    *compute.Instance
 		expAttached bool
 	}{
 		{
-			name: "normal-attached",
-			disk: &compute.Disk{
-				Name: "test-disk",
-			},
+			name:       "normal-attached",
+			deviceName: "test-disk",
 			instance: &compute.Instance{
 				Disks: []*compute.AttachedDisk{
 					{
@@ -623,10 +621,8 @@ func TestDiskIsAttached(t *testing.T) {
 			expAttached: true,
 		},
 		{
-			name: "normal-not-attached",
-			disk: &compute.Disk{
-				Name: "test-disk",
-			},
+			name:       "normal-not-attached",
+			deviceName: "test-disk",
 			instance: &compute.Instance{
 				Disks: []*compute.AttachedDisk{
 					{
@@ -639,7 +635,7 @@ func TestDiskIsAttached(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Logf("test case: %s", tc.name)
-		if attached := diskIsAttached(gce.ZonalCloudDisk(tc.disk), tc.instance); attached != tc.expAttached {
+		if attached := diskIsAttached(tc.deviceName, tc.instance); attached != tc.expAttached {
 			t.Errorf("Expected disk attached to be %v, but got %v", tc.expAttached, attached)
 		}
 	}
@@ -648,17 +644,15 @@ func TestDiskIsAttached(t *testing.T) {
 func TestDiskIsAttachedAndCompatible(t *testing.T) {
 	testCases := []struct {
 		name        string
-		disk        *compute.Disk
+		deviceName  string
 		instance    *compute.Instance
 		mode        string
 		expAttached bool
 		expErr      bool
 	}{
 		{
-			name: "normal-attached",
-			disk: &compute.Disk{
-				Name: "test-disk",
-			},
+			name:       "normal-attached",
+			deviceName: "test-disk",
 			instance: &compute.Instance{
 				Disks: []*compute.AttachedDisk{
 					{
@@ -671,10 +665,8 @@ func TestDiskIsAttachedAndCompatible(t *testing.T) {
 			expAttached: true,
 		},
 		{
-			name: "normal-not-attached",
-			disk: &compute.Disk{
-				Name: "test-disk",
-			},
+			name:       "normal-not-attached",
+			deviceName: "test-disk",
 			instance: &compute.Instance{
 				Disks: []*compute.AttachedDisk{
 					{
@@ -687,10 +679,8 @@ func TestDiskIsAttachedAndCompatible(t *testing.T) {
 			expAttached: false,
 		},
 		{
-			name: "incompatible mode",
-			disk: &compute.Disk{
-				Name: "test-disk",
-			},
+			name:       "incompatible mode",
+			deviceName: "test-disk",
 			instance: &compute.Instance{
 				Disks: []*compute.AttachedDisk{
 					{
@@ -706,7 +696,7 @@ func TestDiskIsAttachedAndCompatible(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Logf("test case: %s", tc.name)
-		attached, err := diskIsAttachedAndCompatible(gce.ZonalCloudDisk(tc.disk), tc.instance, nil, tc.mode)
+		attached, err := diskIsAttachedAndCompatible(tc.deviceName, tc.instance, nil, tc.mode)
 		if err != nil && !tc.expErr {
 			t.Errorf("Did not expect error but got: %v", err)
 		}
diff --git a/pkg/gce-pd-csi-driver/node.go b/pkg/gce-pd-csi-driver/node.go
@@ -177,7 +177,12 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 	// TODO(#83): Get real partitions
 	partition := ""
 
-	devicePaths := ns.DeviceUtils.GetDiskByIdPaths(volumeKey.Name, partition)
+	deviceName, err := common.GetDeviceName(volumeKey)
+	if err != nil {
+		status.Error(codes.Internal, fmt.Sprintf("error getting device name: %v", err))
+	}
+
+	devicePaths := ns.DeviceUtils.GetDiskByIdPaths(deviceName, partition)
 	devicePath, err := ns.DeviceUtils.VerifyDevicePath(devicePaths)
 
 	if err != nil {
diff --git a/pkg/mount-manager/device-utils.go b/pkg/mount-manager/device-utils.go
@@ -54,7 +54,7 @@ const (
 type DeviceUtils interface {
 	// GetDiskByIdPaths returns a list of all possible paths for a
 	// given Persistent Disk
-	GetDiskByIdPaths(pdName string, partition string) []string
+	GetDiskByIdPaths(deviceName string, partition string) []string
 
 	// VerifyDevicePath returns the first of the list of device paths that
 	// exists on the machine, or an empty string if none exists
@@ -71,12 +71,12 @@ func NewDeviceUtils() *deviceUtils {
 }
 
 // Returns list of all /dev/disk/by-id/* paths for given PD.
-func (m *deviceUtils) GetDiskByIdPaths(pdName string, partition string) []string {
+func (m *deviceUtils) GetDiskByIdPaths(deviceName string, partition string) []string {
 	devicePaths := []string{
-		path.Join(diskByIdPath, diskGooglePrefix+pdName),
-		path.Join(diskByIdPath, diskScsiGooglePrefix+pdName),
-		path.Join(diskByHostIdPath, diskScsiGooglePrefix+pdName),
-		path.Join(diskByHostIdPath, diskScsiGooglePrefix+pdName),
+		path.Join(diskByIdPath, diskGooglePrefix+deviceName),
+		path.Join(diskByIdPath, diskScsiGooglePrefix+deviceName),
+		path.Join(diskByHostIdPath, diskScsiGooglePrefix+deviceName),
+		path.Join(diskByHostIdPath, diskScsiGooglePrefix+deviceName),
 	}
 
 	if partition != "" {
diff --git a/test/run-e2e-local.sh b/test/run-e2e-local.sh
@@ -1,9 +1,8 @@
 #!/bin/bash
 
-#set -o nounset
+set -o nounset
 set -o errexit
 
 readonly PKGDIR=sigs.k8s.io/gcp-compute-persistent-disk-csi-driver
 
-
 ginkgo -v "test/e2e/tests" --logtostderr -- --project ${PROJECT} --service-account ${IAM_NAME}
