disk: fix Detaching disks treated as attached

We should reject to attach disk detaching from the requested node.

Author: huww98

pkg/disk/cloud.go

@@ -162,6 +162,51 @@ func (ad *DiskAttachDetach) findDevice(ctx context.Context, diskID, serial strin
 	return device, nil
 }
 
+type attachAction int
+
+const (
+	giveUp attachAction = iota
+	alreadyAttached
+	detachFirst
+	forceAttach
+	attachNormally
+)
+
+func chooseAttachAction(disk *ecs.Disk, instanceID string) (attachAction, error) {
+	if disk.MultiAttach == "Disabled" {
+		switch disk.Status {
+		case DiskStatusInuse:
+			if disk.InstanceId == instanceID {
+				return alreadyAttached, nil
+			}
+			return detachFirst, nil
+		case DiskStatusAttaching:
+			return giveUp, status.Errorf(codes.Aborted, "AttachDisk: Disk %s is attaching to %s", disk.DiskId, instanceID)
+		case DiskStatusDetaching:
+			if disk.InstanceId == instanceID {
+				return giveUp, status.Errorf(codes.Aborted, "AttachDisk: Disk %s is detaching from %s", disk.DiskId, instanceID)
+			}
+			// disk is detaching from another instance
+			return forceAttach, nil
+		}
+	} else {
+		switch disk.Status {
+		case DiskStatusInuse:
+			if waitstatus.IsInstanceAttached(disk, instanceID) {
+				return alreadyAttached, nil
+			}
+		case DiskStatusDetaching:
+			a := disk.Attachments.Attachment
+			if len(a) == 1 && a[0].InstanceId == instanceID {
+				return giveUp, status.Errorf(codes.Aborted, "AttachDisk: Disk %s is detaching from %s", disk.DiskId, instanceID)
+			}
+		}
+		// Not sure for status == "Attaching", maybe attaching to another node, but detaching from the requested node?
+	}
+	// Most likely status == "Available". But let AttachDisk OpenAPI to decide.
+	return attachNormally, nil
+}
+
 // Attach Alibaba Cloud disk.
 // Returns device path if fromNode, disk serial number otherwise.
 func (ad *DiskAttachDetach) attachDisk(ctx context.Context, diskID, nodeID string, fromNode bool) (string, error) {
@@ -207,35 +252,11 @@ func (ad *DiskAttachDetach) attachDisk(ctx context.Context, diskID, nodeID strin
 		}
 	}
 
-	tryForceAttach := false
-
-	// disk is attached, means disk_ad_controller env is true, disk must be created after 2020.06
-	switch disk.Status {
-	case DiskStatusInuse:
-		if disk.InstanceId == nodeID {
-			if !fromNode {
-				klog.Infof("AttachDisk: Disk %s is already attached to Instance %s, skipping", diskID, disk.InstanceId)
-				return disk.SerialNumber, nil
-			}
-			deviceName, err := GetVolumeDeviceName(diskID)
-			if err == nil && deviceName != "" && IsFileExisting(deviceName) {
-				klog.Infof("AttachDisk: Disk %s is already attached to self Instance %s, and device is: %s", diskID, disk.InstanceId, deviceName)
-				return deviceName, nil
-			} else if disk.SerialNumber != "" {
-				// wait for pci attach ready
-				time.Sleep(5 * time.Second)
-				klog.Infof("AttachDisk: find disk dev after 5 seconds")
-				deviceName, err := GetVolumeDeviceName(diskID)
-				if err == nil && deviceName != "" && IsFileExisting(deviceName) {
-					klog.Infof("AttachDisk: Disk %s is already attached to self Instance %s, and device is: %s", diskID, disk.InstanceId, deviceName)
-					return deviceName, nil
-				}
-				err = fmt.Errorf("AttachDisk: disk device cannot be found in node, diskid: %s, deviceName: %s, err: %+v", diskID, deviceName, err)
-				return "", err
-			}
-			klog.Warningf("AttachDisk: Disk (no serial) %s is already attached to instance %s, but device unknown, will be detached and try again", diskID, disk.InstanceId)
-		}
-
+	action, err := chooseAttachAction(disk, nodeID)
+	if err != nil {
+		return "", err
+	}
+	if action == detachFirst || action == forceAttach {
 		if GlobalConfigVar.DiskBdfEnable {
 			if allowed, err := forceDetachAllowed(GlobalConfigVar.EcsClient, disk); err != nil {
 				return "", status.Errorf(codes.Aborted, "forceDetachAllowed failed: %v", err)
@@ -247,31 +268,54 @@ func (ad *DiskAttachDetach) attachDisk(ctx context.Context, diskID, nodeID strin
 		if !GlobalConfigVar.DetachBeforeAttach {
 			return "", status.Errorf(codes.Aborted, "AttachDisk: Disk %s is already attached to instance %s, env DISK_FORCE_DETACHED is false reject force detach", diskID, disk.InstanceId)
 		}
-		if canForceAttach {
-			tryForceAttach = true
-		} else {
-			klog.Infof("AttachDisk: Disk %s is already attached to instance %s, will be detached", diskID, disk.InstanceId)
-			detachRequest := ecs.CreateDetachDiskRequest()
-			detachRequest.InstanceId = disk.InstanceId
-			detachRequest.DiskId = disk.DiskId
-			for key, value := range GlobalConfigVar.RequestBaseInfo {
-				detachRequest.AppendUserAgent(key, value)
-			}
-			_, err = ad.ecs.DetachDisk(detachRequest)
-			if err != nil {
-				klog.Errorf("AttachDisk: Can't Detach disk %s from instance %s: with error: %v", diskID, disk.InstanceId, err)
-				return "", status.Errorf(codes.Aborted, "AttachDisk: Can't Detach disk %s from instance %s: with error: %v", diskID, disk.InstanceId, err)
-			}
-			klog.Infof("AttachDisk: Wait for disk %s to be detached", diskID)
-			if err := ad.waitForDiskDetached(ctx, diskID, nodeID); err != nil {
-				return "", err
+	}
+	if canForceAttach && action == detachFirst {
+		action = forceAttach
+	}
+	if action == forceAttach && !canForceAttach {
+		action = attachNormally // should fail, but try it anyway
+	}
+
+	switch action {
+	case alreadyAttached:
+		if !fromNode {
+			klog.Infof("AttachDisk: Disk %s is already attached to Instance %s, skipping", diskID, disk.InstanceId)
+			return disk.SerialNumber, nil
+		}
+		deviceName, err := GetVolumeDeviceName(diskID)
+		if err == nil && deviceName != "" && IsFileExisting(deviceName) {
+			klog.Infof("AttachDisk: Disk %s is already attached to self Instance %s, and device is: %s", diskID, disk.InstanceId, deviceName)
+			return deviceName, nil
+		} else if disk.SerialNumber != "" {
+			// wait for pci attach ready
+			time.Sleep(5 * time.Second)
+			klog.Infof("AttachDisk: find disk dev after 5 seconds")
+			deviceName, err := GetVolumeDeviceName(diskID)
+			if err == nil && deviceName != "" && IsFileExisting(deviceName) {
+				klog.Infof("AttachDisk: Disk %s is already attached to self Instance %s, and device is: %s", diskID, disk.InstanceId, deviceName)
+				return deviceName, nil
 			}
+			err = fmt.Errorf("AttachDisk: disk device cannot be found in node, diskid: %s, deviceName: %s, err: %+v", diskID, deviceName, err)
+			return "", err
+		}
+		klog.Warningf("AttachDisk: Disk (no serial) %s is already attached to instance %s, but device unknown, will be detached and try again", diskID, disk.InstanceId)
+		fallthrough
+	case detachFirst:
+		klog.Infof("AttachDisk: Disk %s is already attached to instance %s, will be detached", diskID, disk.InstanceId)
+		detachRequest := ecs.CreateDetachDiskRequest()
+		detachRequest.InstanceId = disk.InstanceId
+		detachRequest.DiskId = disk.DiskId
+		for key, value := range GlobalConfigVar.RequestBaseInfo {
+			detachRequest.AppendUserAgent(key, value)
 		}
-	case DiskStatusAttaching:
-		return "", status.Errorf(codes.Aborted, "AttachDisk: Disk %s is attaching %v", diskID, disk)
-	case DiskStatusDetaching:
-		if canForceAttach {
-			tryForceAttach = true
+		_, err = ad.ecs.DetachDisk(detachRequest)
+		if err != nil {
+			klog.Errorf("AttachDisk: Can't Detach disk %s from instance %s: with error: %v", diskID, disk.InstanceId, err)
+			return "", status.Errorf(codes.Aborted, "AttachDisk: Can't Detach disk %s from instance %s: with error: %v", diskID, disk.InstanceId, err)
+		}
+		klog.Infof("AttachDisk: Wait for disk %s to be detached", diskID)
+		if err := ad.waitForDiskDetached(ctx, diskID, nodeID); err != nil {
+			return "", err
 		}
 	}
 
@@ -287,7 +331,7 @@ func (ad *DiskAttachDetach) attachDisk(ctx context.Context, diskID, nodeID strin
 	attachRequest := ecs.CreateAttachDiskRequest()
 	attachRequest.InstanceId = nodeID
 	attachRequest.DiskId = diskID
-	if tryForceAttach {
+	if action == forceAttach {
 		attachRequest.Force = requests.NewBoolean(true)
 		logger.V(1).Info("try force attach", "from", disk.InstanceId, "to", nodeID)
 	}

pkg/disk/cloud_test.go

@@ -687,9 +687,10 @@ func TestAttachDisk(t *testing.T) {
 			expectErr: true,
 		},
 		{
-			name:   "detaching from self",
-			before: disk("Detaching", "i-testinstanceid"),
-			after:  disk("In_use", "i-testinstanceid"), // FIXME
+			name:      "detaching from self",
+			before:    disk("Detaching", "i-testinstanceid"),
+			noAttach:  true,
+			expectErr: true,
 		},
 		{
 			// This not supported by ECS, but desired by us to speed up failover. Hopes they will support it someday.