Merge pull request #46182 from FengyunPan/check_detach

Automatic merge from submit-queue

Check volume's status before detaching volume

When volume's status is 'detaching', controllermanager will detach
it again and return err. It is necessary to check volume's status
before detaching volume.

same issue: #44536

pkg/volume/cinder/BUILD

@@ -35,6 +35,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
     ],
 )
 

pkg/volume/cinder/attacher.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/golang/glog"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/util/exec"
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
@@ -41,7 +42,13 @@ var _ volume.Attacher = &cinderDiskAttacher{}
 var _ volume.AttachableVolumePlugin = &cinderPlugin{}
 
 const (
-	checkSleepDuration = time.Second
+	checkSleepDuration       = 1 * time.Second
+	operationFinishInitDealy = 1 * time.Second
+	operationFinishFactor    = 1.1
+	operationFinishSteps     = 10
+	diskDetachInitDealy      = 1 * time.Second
+	diskDetachFactor         = 1.2
+	diskDetachSteps          = 13
 )
 
 func (plugin *cinderPlugin) NewAttacher() (volume.Attacher, error) {
@@ -267,6 +274,63 @@ func (plugin *cinderPlugin) NewDetacher() (volume.Detacher, error) {
 	}, nil
 }
 
+func (detacher *cinderDiskDetacher) waitOperationFinished(volumeID string) error {
+	backoff := wait.Backoff{
+		Duration: operationFinishInitDealy,
+		Factor:   operationFinishFactor,
+		Steps:    operationFinishSteps,
+	}
+
+	var volumeStatus string
+	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
+		var pending bool
+		var err error
+		pending, volumeStatus, err = detacher.cinderProvider.OperationPending(volumeID)
+		if err == nil {
+			if pending == false {
+				return true, nil
+			} else {
+				return false, nil
+			}
+		} else {
+			return false, err
+		}
+	})
+
+	if err == wait.ErrWaitTimeout {
+		err = fmt.Errorf("Volume %q is %s, can't finish within the alloted time", volumeID, volumeStatus)
+	}
+
+	return err
+}
+
+func (detacher *cinderDiskDetacher) waitDiskDetached(instanceID, volumeID string) error {
+	backoff := wait.Backoff{
+		Duration: diskDetachInitDealy,
+		Factor:   diskDetachFactor,
+		Steps:    diskDetachSteps,
+	}
+
+	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
+		attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
+		if err == nil {
+			if attached == false {
+				return true, nil
+			} else {
+				return false, nil
+			}
+		} else {
+			return false, err
+		}
+	})
+
+	if err == wait.ErrWaitTimeout {
+		err = fmt.Errorf("Volume %q failed to detach within the alloted time", volumeID)
+	}
+
+	return err
+}
+
 func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName types.NodeName) error {
 	volumeID := path.Base(deviceMountPath)
 	instances, res := detacher.cinderProvider.Instances()
@@ -278,6 +342,10 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName type
 		instanceID = instanceID[(ind + 1):]
 	}
 
+	if err := detacher.waitOperationFinished(volumeID); err != nil {
+		return err
+	}
+
 	attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
 	if err != nil {
 		// Log error and continue with detach
@@ -293,10 +361,14 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName type
 	}
 
 	if err = detacher.cinderProvider.DetachDisk(instanceID, volumeID); err != nil {
-		glog.Errorf("Error detaching volume %q: %v", volumeID, err)
+		glog.Errorf("Error detaching volume %q from node %q: %v", volumeID, nodeName, err)
 		return err
 	}
-	glog.Infof("detached volume %q from instance %q", volumeID, instanceID)
+	if err = detacher.waitDiskDetached(instanceID, volumeID); err != nil {
+		glog.Errorf("Error waiting for volume %q to detach from node %q: %v", volumeID, nodeName, err)
+		return err
+	}
+	glog.Infof("detached volume %q from node %q", volumeID, nodeName)
 	return nil
 }
 

pkg/volume/cinder/attacher_test.go

@@ -36,6 +36,8 @@ import (
 const VolumeStatusPending = "pending"
 const VolumeStatusDone = "done"
 
+var VolumeIsNotAttached = true
+
 func TestGetDeviceName_Volume(t *testing.T) {
 	plugin := newPlugin()
 	name := "my-cinder-volume"
@@ -96,6 +98,7 @@ type testcase struct {
 	disksAreAttached disksAreAttachedCall
 	diskPath         diskPathCall
 	t                *testing.T
+	notAttached      *bool
 
 	instanceID string
 	// Actual test to run
@@ -118,6 +121,7 @@ func TestAttachDetach(t *testing.T) {
 	diskCheckError := errors.New("Fake DiskIsAttached error")
 	diskPathError := errors.New("Fake GetAttachmentDiskPath error")
 	disksCheckError := errors.New("Fake DisksAreAttached error")
+	operationFinishTimeout := errors.New("Fake waitOperationFinished error")
 	tests := []testcase{
 		// Successful Attach call
 		{
@@ -247,10 +251,11 @@ func TestAttachDetach(t *testing.T) {
 
 		// Detach succeeds
 		{
-			name:           "Detach_Positive",
+			name:             "Detach_Positive",
-			instanceID:     instanceID,
+			instanceID:       instanceID,
-			diskIsAttached: diskIsAttachedCall{instanceID, volumeID, true, nil},
+			operationPending: operationPendingCall{volumeID, false, done, nil},
-			detach:         detachCall{instanceID, volumeID, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, true, nil},
+			detach:           detachCall{instanceID, volumeID, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
 				return "", detacher.Detach(volumeID, nodeName)
@@ -259,9 +264,10 @@ func TestAttachDetach(t *testing.T) {
 
 		// Disk is already detached
 		{
-			name:           "Detach_Positive_AlreadyDetached",
+			name:             "Detach_Positive_AlreadyDetached",
-			instanceID:     instanceID,
+			instanceID:       instanceID,
-			diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil},
+			operationPending: operationPendingCall{volumeID, false, done, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
 				return "", detacher.Detach(volumeID, nodeName)
@@ -270,10 +276,11 @@ func TestAttachDetach(t *testing.T) {
 
 		// Detach succeeds when DiskIsAttached fails
 		{
-			name:           "Detach_Positive_CheckFails",
+			name:             "Detach_Positive_CheckFails",
-			instanceID:     instanceID,
+			instanceID:       instanceID,
-			diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			operationPending: operationPendingCall{volumeID, false, done, nil},
-			detach:         detachCall{instanceID, volumeID, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			detach:           detachCall{instanceID, volumeID, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
 				return "", detacher.Detach(volumeID, nodeName)
@@ -282,20 +289,36 @@ func TestAttachDetach(t *testing.T) {
 
 		// Detach fails
 		{
-			name:           "Detach_Negative",
+			name:             "Detach_Negative",
-			instanceID:     instanceID,
+			instanceID:       instanceID,
-			diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			operationPending: operationPendingCall{volumeID, false, done, nil},
-			detach:         detachCall{instanceID, volumeID, detachError},
+			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			detach:           detachCall{instanceID, volumeID, detachError},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
 				return "", detacher.Detach(volumeID, nodeName)
 			},
 			expectedError: detachError,
 		},
+
+		// // Disk is detaching
+		{
+			name:             "Detach_Is_Detaching",
+			instanceID:       instanceID,
+			operationPending: operationPendingCall{volumeID, true, pending, operationFinishTimeout},
+			test: func(testcase *testcase) (string, error) {
+				detacher := newDetacher(testcase)
+				return "", detacher.Detach(volumeID, nodeName)
+			},
+			expectedError: operationFinishTimeout,
+		},
 	}
 
 	for _, testcase := range tests {
 		testcase.t = t
+		// set VolumeIsNotAttached to test detach case, attach case ignore it
+		VolumeIsNotAttached = false
+		testcase.notAttached = &VolumeIsNotAttached
 		result, err := testcase.test(&testcase)
 		if err != testcase.expectedError {
 			t.Errorf("%s failed: expected err=%q, got %q", testcase.name, testcase.expectedError, err)
@@ -457,6 +480,8 @@ func (testcase *testcase) AttachDisk(instanceID, volumeID string) (string, error
 
 	glog.V(4).Infof("AttachDisk call: %s, %s, returning %q, %v", volumeID, instanceID, expected.retDeviceName, expected.ret)
 
+	VolumeIsNotAttached = false
+	testcase.notAttached = &VolumeIsNotAttached
 	return expected.retDeviceName, expected.ret
 }
 
@@ -482,6 +507,8 @@ func (testcase *testcase) DetachDisk(instanceID, volumeID string) error {
 
 	glog.V(4).Infof("DetachDisk call: %s, %s, returning %v", volumeID, instanceID, expected.ret)
 
+	VolumeIsNotAttached = true
+	testcase.notAttached = &VolumeIsNotAttached
 	return expected.ret
 }
 
@@ -500,6 +527,10 @@ func (testcase *testcase) OperationPending(diskName string) (bool, string, error
 
 func (testcase *testcase) DiskIsAttached(instanceID, volumeID string) (bool, error) {
 	expected := &testcase.diskIsAttached
+	// If testcase call DetachDisk*, return false
+	if *testcase.notAttached == true {
+		return false, nil
+	}
 
 	if expected.volumeID == "" && expected.instanceID == "" {
 		// testcase.diskIsAttached looks uninitialized, test did not expect to