From 5455461b9e089cff389397967ce70a7ff98f644c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B2=88=E6=87=BF=E6=9D=B0?= Date: Tue, 15 Mar 2022 15:39:35 +0800 Subject: [PATCH 1/4] fixes for multi access mode attachment condition --- pkg/controller/csi_handler.go | 37 ++++++------------ pkg/controller/readonly.go | 74 +++++++++++++++++------------------ 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go index 83e480d2f..0ef1803ca 100644 --- a/pkg/controller/csi_handler.go +++ b/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 @@ -497,28 +497,17 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt readOnly = false } - volumeCapabilities, err := GetVolumeCapabilities(pvSpec, readOnly) + mount, info, err := h.checkMountAvailability(va, readOnly) if err != nil { 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 !mount { + readOnly = true } - 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") - } + + volumeCapabilities, err := GetVolumeCapabilities(pvSpec, readOnly) + if err != nil { + return va, nil, err } secrets, err := h.getCredentialsFromPV(csiSource) @@ -546,15 +535,15 @@ 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["attachInfo"] = info + return va, attachResponse, nil } func (h *csiHandler) csiDetach(va *storage.VolumeAttachment) (*storage.VolumeAttachment, error) { diff --git a/pkg/controller/readonly.go b/pkg/controller/readonly.go index a81587913..29fb74d4b 100644 --- a/pkg/controller/readonly.go +++ b/pkg/controller/readonly.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -43,6 +44,10 @@ func (h *csiHandler) checkIfReadonlyMount(va *storage.VolumeAttachment) (bool, e continue } + if po.Status.Phase != v1.PodRunning { + continue + } + for _, vol := range po.Spec.Volumes { if vol.PersistentVolumeClaim != nil { if vol.PersistentVolumeClaim.ClaimName == claim.Name { @@ -59,65 +64,58 @@ 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, string, error) { vas, err := h.vaLister.List(labels.Everything()) if err != nil { - return false, fmt.Errorf("list volume attachments, %w", err) + return false, "", fmt.Errorf("list volume attachments, %w", err) } node := va.Spec.NodeName - + attachInfo := "" for _, target := range vas { if va.Spec.Source.PersistentVolumeName != nil { 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" { + attachInfo = "Already mounted as readonly by other node, can only be mounted with readonly." + if readOnly { + // ROX + return true, attachInfo, nil + } + // any write attempt change to ROX or trigger error for release pod info + return false, attachInfo, nil + } + // attached as RWO, cannot be attached by other node anymore + return false, attachInfo, errors.New("volume may be attached to another node read/write already, can not be attached anymore") + + } } + } - return false, nil + if readOnly { + attachInfo = "mounted as readonly" + } else { + attachInfo = "mounted as readwrite" + } + + return true, attachInfo, nil } func (h *csiHandler) getClaimName(pvName string) (*types.NamespacedName, error) { From 7f489097519fd0ac608c97387caaea9281fc7f07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B2=88=E6=87=BF=E6=9D=B0?= Date: Wed, 12 Oct 2022 12:42:48 +1100 Subject: [PATCH 2/4] logic fix --- pkg/controller/csi_handler.go | 34 ++++++++++++++++++++++++++++++++-- pkg/controller/readonly.go | 22 ++++++++-------------- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go index 0ef1803ca..d51a3b389 100644 --- a/pkg/controller/csi_handler.go +++ b/pkg/controller/csi_handler.go @@ -491,13 +491,26 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt if err != nil { return va, nil, err } + + // podRO := readOnly + // if va.Status.AttachmentMetadata != nil { + // metadata := make(map[string]string) + // if pod != "" { + // metadata[pod] = "readOnly" + // if !podRO { + // metadata[pod] = "readWrite" + // } + // } + // return va, metadata, nil + // } + if !h.supportsPublishReadOnly { // "CO MUST set this field to false if SP does not have the // PUBLISH_READONLY controller capability" readOnly = false } - mount, info, err := h.checkMountAvailability(va, readOnly) + mount, err := h.checkMountAvailability(va, readOnly) if err != nil { return va, nil, err } @@ -542,7 +555,11 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt if _, ok := attachResponse[readonlyAttachmentKey]; !ok { return va, nil, fmt.Errorf("controllerPublishVolume failed to proceed") } - attachResponse["attachInfo"] = info + + // attachResponse[pod] = "readOnly" + // if !podRO { + // attachResponse[pod] = "readWrite" + // } return va, attachResponse, nil } @@ -626,6 +643,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() diff --git a/pkg/controller/readonly.go b/pkg/controller/readonly.go index 29fb74d4b..022623fbe 100644 --- a/pkg/controller/readonly.go +++ b/pkg/controller/readonly.go @@ -44,7 +44,7 @@ func (h *csiHandler) checkIfReadonlyMount(va *storage.VolumeAttachment) (bool, e continue } - if po.Status.Phase != v1.PodRunning { + if po.Status.Phase == v1.PodRunning || po.Status.Phase == v1.PodFailed { continue } @@ -74,14 +74,14 @@ func (h *csiHandler) checkIfReadonlyMount(va *storage.VolumeAttachment) (bool, e } -func (h *csiHandler) checkMountAvailability(va *storage.VolumeAttachment, readOnly bool) (bool, string, 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) + return false, fmt.Errorf("list volume attachments, %w", err) } node := va.Spec.NodeName - attachInfo := "" + for _, target := range vas { if va.Spec.Source.PersistentVolumeName != nil { if *target.Spec.Source.PersistentVolumeName != *va.Spec.Source.PersistentVolumeName { @@ -94,28 +94,22 @@ func (h *csiHandler) checkMountAvailability(va *storage.VolumeAttachment, readOn if target.Status.Attached { if target.Status.AttachmentMetadata[readonlyAttachmentKey] == "true" { - attachInfo = "Already mounted as readonly by other node, can only be mounted with readonly." if readOnly { // ROX - return true, attachInfo, nil + return true, nil } // any write attempt change to ROX or trigger error for release pod info - return false, attachInfo, nil + return false, nil } // attached as RWO, cannot be attached by other node anymore - return false, attachInfo, errors.New("volume may be attached to another node read/write already, can not be attached anymore") + return false, errors.New("volume may be attached to another node read/write already, can not be attached anymore") } } } - if readOnly { - attachInfo = "mounted as readonly" - } else { - attachInfo = "mounted as readwrite" - } - return true, attachInfo, nil + return true, nil } func (h *csiHandler) getClaimName(pvName string) (*types.NamespacedName, error) { From c6a2accd92e560f5ef30994fba9b8ca1e327114b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B2=88=E6=87=BF=E6=9D=B0?= Date: Fri, 21 Oct 2022 23:54:53 +1100 Subject: [PATCH 3/4] fix error --- pkg/controller/csi_handler.go | 12 ------------ pkg/controller/readonly.go | 4 ---- 2 files changed, 16 deletions(-) diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go index d51a3b389..3bab33a72 100644 --- a/pkg/controller/csi_handler.go +++ b/pkg/controller/csi_handler.go @@ -492,18 +492,6 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt return va, nil, err } - // podRO := readOnly - // if va.Status.AttachmentMetadata != nil { - // metadata := make(map[string]string) - // if pod != "" { - // metadata[pod] = "readOnly" - // if !podRO { - // metadata[pod] = "readWrite" - // } - // } - // return va, metadata, nil - // } - if !h.supportsPublishReadOnly { // "CO MUST set this field to false if SP does not have the // PUBLISH_READONLY controller capability" diff --git a/pkg/controller/readonly.go b/pkg/controller/readonly.go index 022623fbe..5731f4d14 100644 --- a/pkg/controller/readonly.go +++ b/pkg/controller/readonly.go @@ -44,10 +44,6 @@ func (h *csiHandler) checkIfReadonlyMount(va *storage.VolumeAttachment) (bool, e continue } - if po.Status.Phase == v1.PodRunning || po.Status.Phase == v1.PodFailed { - continue - } - for _, vol := range po.Spec.Volumes { if vol.PersistentVolumeClaim != nil { if vol.PersistentVolumeClaim.ClaimName == claim.Name { From e57b24cc98fd7534a60a159cc5c219c75cc3e55d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B2=88=E6=87=BF=E6=9D=B0?= Date: Tue, 6 Dec 2022 20:38:12 +1100 Subject: [PATCH 4/4] logic fixed --- pkg/controller/csi_handler.go | 14 ++++++-------- pkg/controller/readonly.go | 13 +++++-------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go index 3bab33a72..f3fce8fec 100644 --- a/pkg/controller/csi_handler.go +++ b/pkg/controller/csi_handler.go @@ -492,18 +492,16 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt return va, nil, err } - if !h.supportsPublishReadOnly { - // "CO MUST set this field to false if SP does not have the - // PUBLISH_READONLY controller capability" - readOnly = false - } - mount, err := h.checkMountAvailability(va, readOnly) if err != nil { return va, nil, err } - if !mount { - readOnly = true + readOnly = mount + + if !h.supportsPublishReadOnly { + // "CO MUST set this field to false if SP does not have the + // PUBLISH_READONLY controller capability" + readOnly = false } volumeCapabilities, err := GetVolumeCapabilities(pvSpec, readOnly) diff --git a/pkg/controller/readonly.go b/pkg/controller/readonly.go index 5731f4d14..bee152cc4 100644 --- a/pkg/controller/readonly.go +++ b/pkg/controller/readonly.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" - v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -90,12 +89,10 @@ func (h *csiHandler) checkMountAvailability(va *storage.VolumeAttachment, readOn if target.Status.Attached { if target.Status.AttachmentMetadata[readonlyAttachmentKey] == "true" { - if readOnly { - // ROX - return true, nil - } - // any write attempt change to ROX or trigger error for release pod info - return false, nil + 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") @@ -105,7 +102,7 @@ func (h *csiHandler) checkMountAvailability(va *storage.VolumeAttachment, readOn } - return true, nil + return false, nil } func (h *csiHandler) getClaimName(pvName string) (*types.NamespacedName, error) {