Merge pull request #47018 from FengyunPan/fix-attach

Automatic merge from submit-queue (batch tested with PRs 46977, 47005, 47018, 47061, 46809)

Waiting attach operation to be finished rather than returning nil

Fixes #46882

pkg/volume/cinder/attacher.go

@@ -46,6 +46,9 @@ const (
 	operationFinishInitDealy = 1 * time.Second
 	operationFinishFactor    = 1.1
 	operationFinishSteps     = 10
+	diskAttachInitDealy      = 1 * time.Second
+	diskAttachFactor         = 1.2
+	diskAttachSteps          = 15
 	diskDetachInitDealy      = 1 * time.Second
 	diskDetachFactor         = 1.2
 	diskDetachSteps          = 13
@@ -67,6 +70,53 @@ func (plugin *cinderPlugin) GetDeviceMountRefs(deviceMountPath string) ([]string
 	return mount.GetMountRefs(mounter, deviceMountPath)
 }
 
+func (attacher *cinderDiskAttacher) 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 = attacher.cinderProvider.OperationPending(volumeID)
+		if err != nil {
+			return false, err
+		}
+		return !pending, nil
+	})
+
+	if err == wait.ErrWaitTimeout {
+		err = fmt.Errorf("Volume %q is %s, can't finish within the alloted time", volumeID, volumeStatus)
+	}
+
+	return err
+}
+
+func (attacher *cinderDiskAttacher) waitDiskAttached(instanceID, volumeID string) error {
+	backoff := wait.Backoff{
+		Duration: diskAttachInitDealy,
+		Factor:   diskAttachFactor,
+		Steps:    diskAttachSteps,
+	}
+
+	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
+		attached, err := attacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
+		if err != nil {
+			return false, err
+		}
+		return attached, nil
+	})
+
+	if err == wait.ErrWaitTimeout {
+		err = fmt.Errorf("Volume %q failed to be attached within the alloted time", volumeID)
+	}
+
+	return err
+}
+
 func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.NodeName) (string, error) {
 	volumeSource, _, err := getVolumeSource(spec)
 	if err != nil {
@@ -80,14 +130,9 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod
 		return "", err
 	}
 
-	pending, volumeStatus, err := attacher.cinderProvider.OperationPending(volumeID)
+	if err := attacher.waitOperationFinished(volumeID); err != nil {
-	if err != nil {
 		return "", err
 	}
-	if pending {
-		glog.Infof("status of volume %q is %s, wait it to be finished", volumeID, volumeStatus)
-		return "", nil
-	}
 
 	attached, err := attacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
 	if err != nil {
@@ -103,6 +148,10 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod
 	} else {
 		_, err = attacher.cinderProvider.AttachDisk(instanceID, volumeID)
 		if err == nil {
+			if err = attacher.waitDiskAttached(instanceID, volumeID); err != nil {
+				glog.Errorf("Error waiting for volume %q to be attached from node %q: %v", volumeID, nodeName, err)
+				return "", err
+			}
 			glog.Infof("Attach operation successful: volume %q attached to instance %q.", volumeID, instanceID)
 		} else {
 			glog.Infof("Attach volume %q to instance %q failed with: %v", volumeID, instanceID, err)
@@ -110,20 +159,11 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod
 		}
 	}
 
-	// When volume's status is 'attaching', DiskIsAttached() return <false, nil>, and can't get device path.
+	devicePath, err := attacher.cinderProvider.GetAttachmentDiskPath(instanceID, volumeID)
-	// We should get device path next time and do not return err.
-	devicePath := ""
-	pending, _, err = attacher.cinderProvider.OperationPending(volumeID)
 	if err != nil {
+		glog.Infof("Can not get device path of volume %q which be attached to instance %q, failed with: %v", volumeID, instanceID, err)
 		return "", err
 	}
-	if !pending {
-		devicePath, err = attacher.cinderProvider.GetAttachmentDiskPath(instanceID, volumeID)
-		if err != nil {
-			glog.Infof("Can not get device path of volume %q which be attached to instance %q, failed with: %v", volumeID, instanceID, err)
-			return "", err
-		}
-	}
 
 	return devicePath, nil
 }
@@ -286,15 +326,10 @@ func (detacher *cinderDiskDetacher) waitOperationFinished(volumeID string) error
 		var pending bool
 		var err error
 		pending, volumeStatus, err = detacher.cinderProvider.OperationPending(volumeID)
-		if err == nil {
+		if err != nil {
-			if pending == false {
-				return true, nil
-			} else {
-				return false, nil
-			}
-		} else {
 			return false, err
 		}
+		return !pending, nil
 	})
 
 	if err == wait.ErrWaitTimeout {
@@ -313,15 +348,10 @@ func (detacher *cinderDiskDetacher) waitDiskDetached(instanceID, volumeID string
 
 	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
 		attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
-		if err == nil {
+		if err != nil {
-			if attached == false {
-				return true, nil
-			} else {
-				return false, nil
-			}
-		} else {
 			return false, err
 		}
+		return !attached, nil
 	})
 
 	if err == wait.ErrWaitTimeout {

pkg/volume/cinder/attacher_test.go

@@ -33,10 +33,13 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
-const VolumeStatusPending = "pending"
+const (
-const VolumeStatusDone = "done"
+	VolumeStatusPending = "pending"
+	VolumeStatusDone    = "done"
+)
 
-var VolumeIsNotAttached = true
+var attachStatus = "Attach"
+var detachStatus = "Detach"
 
 func TestGetDeviceName_Volume(t *testing.T) {
 	plugin := newPlugin()
@@ -98,7 +101,7 @@ type testcase struct {
 	disksAreAttached disksAreAttachedCall
 	diskPath         diskPathCall
 	t                *testing.T
-	notAttached      *bool
+	attachOrDetach   *string
 
 	instanceID string
 	// Actual test to run
@@ -156,28 +159,12 @@ func TestAttachDetach(t *testing.T) {
 		{
 			name:             "Attach_is_attaching",
 			instanceID:       instanceID,
-			operationPending: operationPendingCall{volumeID, true, pending, nil},
+			operationPending: operationPendingCall{volumeID, true, pending, operationFinishTimeout},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
 			test: func(testcase *testcase) (string, error) {
 				attacher := newAttacher(testcase)
 				return attacher.Attach(spec, nodeName)
 			},
-			expectedResult: "",
+			expectedError: operationFinishTimeout,
-		},
-
-		// DiskIsAttached fails and Attach succeeds
-		{
-			name:             "Attach_Positive_CheckFails",
-			instanceID:       instanceID,
-			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
-			attach:           attachCall{instanceID, volumeID, "", nil},
-			diskPath:         diskPathCall{instanceID, volumeID, "/dev/sda", nil},
-			test: func(testcase *testcase) (string, error) {
-				attacher := newAttacher(testcase)
-				return attacher.Attach(spec, nodeName)
-			},
-			expectedResult: "/dev/sda",
 		},
 
 		// Attach call fails
@@ -199,7 +186,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Attach_Negative_DiskPatchFails",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, nil},
 			attach:           attachCall{instanceID, volumeID, "", nil},
 			diskPath:         diskPathCall{instanceID, volumeID, "", diskPathError},
 			test: func(testcase *testcase) (string, error) {
@@ -316,9 +303,8 @@ func TestAttachDetach(t *testing.T) {
 
 	for _, testcase := range tests {
 		testcase.t = t
-		// set VolumeIsNotAttached to test detach case, attach case ignore it
+		attachOrDetach := ""
-		VolumeIsNotAttached = false
+		testcase.attachOrDetach = &attachOrDetach
-		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)
@@ -480,8 +466,7 @@ 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.attachOrDetach = &attachStatus
-	testcase.notAttached = &VolumeIsNotAttached
 	return expected.retDeviceName, expected.ret
 }
 
@@ -507,8 +492,7 @@ 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.attachOrDetach = &detachStatus
-	testcase.notAttached = &VolumeIsNotAttached
 	return expected.ret
 }
 
@@ -528,10 +512,15 @@ 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 {
+	if *testcase.attachOrDetach == detachStatus {
 		return false, nil
 	}
 
+	// If testcase call AttachDisk*, return true
+	if *testcase.attachOrDetach == attachStatus {
+		return true, nil
+	}
+
 	if expected.volumeID == "" && expected.instanceID == "" {
 		// testcase.diskIsAttached looks uninitialized, test did not expect to
 		// call DiskIsAttached

pkg/volume/util/operationexecutor/operation_generator.go

@@ -266,9 +266,6 @@ func (og *operationGenerator) GenerateAttachVolumeFunc(
 				og.recorder.Eventf(pod, v1.EventTypeWarning, kevents.FailedMountVolume, eventErr.Error())
 			}
 			return detailedErr
-		} else if devicePath == "" {
-			// do nothing and get device path next time.
-			return nil
 		}
 
 		glog.Infof(volumeToAttach.GenerateMsgDetailed("AttachVolume.Attach succeeded", ""))