Merge pull request #63303 from jsafrane/fix-csi-attach-error

Automatic merge from submit-queue (batch tested with PRs 64102, 63303, 64150, 63841). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Return attach error to A/D controller.

The plugin should not wait with errors for WaitForAttach, WaitForAttach runs on nodes, not in A/D controller.

**Which issue(s) this PR fixes**
Fixes #63300

~~Work in progress, missing testing.~~

**Release note**:

```release-note
NONE
```

/sig storage
@saad-ali @vladimirvivien

pkg/volume/csi/csi_attacher.go

@@ -102,17 +102,11 @@ func (c *csiAttacher) Attach(spec *volume.Spec, nodeName types.NodeName) (string
 		glog.V(4).Info(log("attachment [%v] for volume [%v] created successfully", attachID, csiSource.VolumeHandle))
 	}
 
-	// probe for attachment update here
-	// NOTE: any error from waiting for attachment is logged only.  This is because
-	// the primary intent of the enclosing method is to create VolumeAttachment.
-	// DONOT return that error here as it is mitigated in attacher.WaitForAttach.
-	volAttachmentOK := true
 	if _, err := c.waitForVolumeAttachment(csiSource.VolumeHandle, attachID, csiTimeout); err != nil {
-		volAttachmentOK = false
-		glog.Error(log("attacher.Attach attempted to wait for attachment to be ready, but failed with: %v", err))
+		return "", err
 	}
 
-	glog.V(4).Info(log("attacher.Attach finished OK with VolumeAttachment verified=%t: attachment object [%s]", volAttachmentOK, attachID))
+	glog.V(4).Info(log("attacher.Attach finished OK with VolumeAttachment object [%s]", attachID))
 
 	return attachID, nil
 }

pkg/volume/csi/csi_attacher_test.go

@@ -60,12 +60,13 @@ func makeTestAttachment(attachID, nodeName, pvName string) *storage.VolumeAttach
 func TestAttacherAttach(t *testing.T) {
 
 	testCases := []struct {
-		name       string
-		nodeName   string
-		driverName string
-		volumeName string
-		attachID   string
-		shouldFail bool
+		name                string
+		nodeName            string
+		driverName          string
+		volumeName          string
+		attachID            string
+		injectAttacherError bool
+		shouldFail          bool
 	}{
 		{
 			name:       "test ok 1",
@@ -105,6 +106,15 @@ func TestAttacherAttach(t *testing.T) {
 			attachID:   getAttachmentName("vol02", "driver02", "node02"),
 			shouldFail: true,
 		},
+		{
+			name:                "attacher error",
+			nodeName:            "node02",
+			driverName:          "driver02",
+			volumeName:          "vol02",
+			attachID:            getAttachmentName("vol02", "driver02", "node02"),
+			injectAttacherError: true,
+			shouldFail:          true,
+		},
 	}
 
 	// attacher loop
@@ -128,6 +138,9 @@ func TestAttacherAttach(t *testing.T) {
 			if !fail && err != nil {
 				t.Errorf("expecting no failure, but got err: %v", err)
 			}
+			if fail && err == nil {
+				t.Errorf("expecting failure, but got no err")
+			}
 			if attachID != id && !fail {
 				t.Errorf("expecting attachID %v, got %v", id, attachID)
 			}
@@ -155,7 +168,14 @@ func TestAttacherAttach(t *testing.T) {
 		if attach == nil {
 			t.Logf("attachment not found for id:%v", tc.attachID)
 		} else {
-			attach.Status.Attached = true
+			if tc.injectAttacherError {
+				attach.Status.Attached = false
+				attach.Status.AttachError = &storage.VolumeError{
+					Message: "attacher error",
+				}
+			} else {
+				attach.Status.Attached = true
+			}
 			_, err = csiAttacher.k8s.StorageV1beta1().VolumeAttachments().Update(attach)
 			if err != nil {
 				t.Error(err)