fixes for multi access mode attachment condition

pkg/controller/csi_handler.go

@@ -24,7 +24,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/kubernetes-csi/csi-lib-utils/connection"
 	"github.com/kubernetes-csi/external-attacher/pkg/attacher"
 	v1 "k8s.io/api/core/v1"
@@ -255,10 +254,11 @@ func (h *csiHandler) syncAttach(va *storage.VolumeAttachment) error {
 			// Just log it, propagate the attach error.
 			klog.V(2).Infof("Failed to save attach error to %q: %s", va.Name, saveErr.Error())
 		}
-		// Add context to the error for logging
 		err := fmt.Errorf("failed to attach: %s", err)
+
 		return err
 	}
+
 	klog.V(2).Infof("Attached %q", va.Name)
 
 	// Mark as attached
@@ -491,6 +491,13 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt
 	if err != nil {
 		return va, nil, err
 	}
+
+	mount, err := h.checkMountAvailability(va, readOnly)
+	if err != nil {
+		return va, nil, err
+	}
+	readOnly = mount
+
 	if !h.supportsPublishReadOnly {
 		// "CO MUST set this field to false if SP does not have the
 		// PUBLISH_READONLY controller capability"
@@ -502,25 +509,6 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt
 		return va, nil, err
 	}
 
-	if volumeCapabilities.AccessMode.Mode == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY {
-		rox, err := h.checkIfROXMount(va)
-		if err != nil {
-			return va, nil, err
-		}
-		if !rox {
-			return va, nil, errors.New("volume may be attached to another node read/write already, can not be attached read only anymore")
-		}
-	}
-	if volumeCapabilities.AccessMode.Mode == csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER {
-		attached, err := h.checkIfAttachedToOtherNodes(va)
-		if err != nil {
-			return va, nil, err
-		}
-		if attached {
-			return va, nil, errors.New("volume is already attached to another node, can not be attached r/w anymore")
-		}
-	}
-
 	secrets, err := h.getCredentialsFromPV(csiSource)
 	if err != nil {
 		return va, nil, err
@@ -546,15 +534,19 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt
 	defer cancel()
 	// We're not interested in `detached` return value, the controller will
 	// issue Detach to be sure the volume is really detached.
-	publishInfo, _, err := h.attacher.Attach(ctx, volumeHandle, readOnly, nodeID, volumeCapabilities, attributes, secrets)
+	attachResponse, _, err := h.attacher.Attach(ctx, volumeHandle, readOnly, nodeID, volumeCapabilities, attributes, secrets)
 	if err != nil {
 		return va, nil, err
 	}
-	if _, ok := publishInfo[readonlyAttachmentKey]; !ok {
+	if _, ok := attachResponse[readonlyAttachmentKey]; !ok {
 		return va, nil, fmt.Errorf("controllerPublishVolume failed to proceed")
 	}
 
-	return va, publishInfo, nil
+	// attachResponse[pod] = "readOnly"
+	// if !podRO {
+	// 	attachResponse[pod] = "readWrite"
+	// }
+	return va, attachResponse, nil
 }
 
 func (h *csiHandler) csiDetach(va *storage.VolumeAttachment) (*storage.VolumeAttachment, error) {
@@ -637,6 +629,19 @@ func (h *csiHandler) saveAttachError(va *storage.VolumeAttachment, err error) (*
 	return newVa, nil
 }
 
+func (h *csiHandler) saveMetaData(va *storage.VolumeAttachment, metadata map[string]string) (*storage.VolumeAttachment, error) {
+	clone := va.DeepCopy()
+	clone.Status.AttachmentMetadata = metadata
+
+	var newVa *storage.VolumeAttachment
+	var err error
+	if newVa, err = h.patchVA(va, clone, "status"); err != nil {
+		return va, err
+	}
+	klog.V(4).Infof("Saved attach metadata to %q", va.Name)
+	return newVa, nil
+}
+
 func (h *csiHandler) saveDetachError(va *storage.VolumeAttachment, err error) (*storage.VolumeAttachment, error) {
 	klog.V(4).Infof("Saving detach error to %q", va.Name)
 	clone := va.DeepCopy()

pkg/controller/readonly.go

@@ -59,41 +59,17 @@ func (h *csiHandler) checkIfReadonlyMount(va *storage.VolumeAttachment) (bool, e
 						}
 						return false, nil
 					}
+					return true, nil
 				}
 			}
 		}
 	}
 
-	return true, nil
-}
-
-func (h *csiHandler) checkIfROXMount(va *storage.VolumeAttachment) (bool, error) {
-	vas, err := h.vaLister.List(labels.Everything())
-	if err != nil {
-		return false, fmt.Errorf("list volume attachments, %w", err)
-	}
-
-	node := va.Spec.NodeName
-
-	for _, target := range vas {
-		if *target.Spec.Source.PersistentVolumeName != *va.Spec.Source.PersistentVolumeName {
-			continue
-		}
-		// exclude current va itself
-		if target.Spec.NodeName == node {
-			continue
-		}
-
-		if target.Status.Attached && target.Status.AttachmentMetadata[readonlyAttachmentKey] != "true" {
-			return false, nil
-		}
-
-	}
+	return false, fmt.Errorf("no matching conditions")
 
-	return true, nil
 }
 
-func (h *csiHandler) checkIfAttachedToOtherNodes(va *storage.VolumeAttachment) (bool, error) {
+func (h *csiHandler) checkMountAvailability(va *storage.VolumeAttachment, readOnly bool) (bool, error) {
 	vas, err := h.vaLister.List(labels.Everything())
 	if err != nil {
 		return false, fmt.Errorf("list volume attachments, %w", err)
@@ -106,17 +82,26 @@ func (h *csiHandler) checkIfAttachedToOtherNodes(va *storage.VolumeAttachment) (
 			if *target.Spec.Source.PersistentVolumeName != *va.Spec.Source.PersistentVolumeName {
 				continue
 			}
-		}
+			// exclude current va itself
+			if target.Spec.NodeName == node {
+				continue
+			}
 
-		// // exclude current va itself
-		if target.Spec.NodeName == node {
-			continue
-		}
-		// // fixme: there could be some race condition when another attaching (r/w or ro) is in progress and has not set metadata yet.
-		if target.Status.Attached {
-			return true, nil
+			if target.Status.Attached {
+				if target.Status.AttachmentMetadata[readonlyAttachmentKey] == "true" {
+					return true, nil
+				}
+				if _, ok := target.Status.AttachmentMetadata[readonlyAttachmentKey]; !ok {
+					return true, nil
+				}
+				// attached as RWO, cannot be attached by other node anymore
+				return false, errors.New("volume may be attached to another node read/write already, can not be attached anymore")
+
+			}
 		}
+
 	}
+
 	return false, nil
 }