Uncertain handling for attach

[msau42]

It looks like we check for finalError on ControllerPublishVolume:

external-attacher/pkg/attacher/attacher.go

Line 70 in c4cfca3

return nil, isFinalError(err), err

But we ignore it later on:

external-attacher/pkg/controller/csi_handler.go

Line 513 in c4cfca3

// We're not interested in `detached` return value, the controller will

And we rely on the driver's implementation of ControllerUnpublishVolume to be able to properly detect if there is still an attach operation in progress.

This is different than how we handle other uncertain states for volume operations like provision and mount. Should we consider adding uncertain handling for attach?

[msau42]

cc @jsafrane @gnufied

[jsafrane]

IIRC, the external-attacher always calls ControllerUnpublish when a VolumeAttachment gets DeletionTimestamp, regardless if the previous attach was successful or not or if it got a final error or temporary one. In this sense, an attachment is always "uncertain" until fully attached.

It could be possible to optimize and mark attachments as "not attached" after a final ControllerPublish error and skip ControllerUnpublish in this case. I am not sure it's worth the effort.

[bswartz]

I agree it sounds like it should not cause problems, but one could imagine that some plugin depends on the current Kubernetes behavior to avoid leaking resources.

My reading of the spec on the requirements for the CO are ambiguous in this area. There is a lot of "the CO may choose to" language that suggests the CO has little if any obligation here.

[msau42]

The main challenge is that the current behavior relies on the plugin to potentially keep state about pending attaches requests, which may be difficult. You can have a sequence like:

1. Attach disk started
2. Attach disk timed out on client side, but is still queued in the backend
3. Pod is deleted and we trigger detach
4. Detach returns success because disk is not attached to the node yet.
5. Queued attach operation proceeds and attaches the disk to the old node.

Some ways that a plugin could address this are:

• During step 4, check for pending attach operations
• During step 4, issue a detach operation regardless and wait for the operation to complete

However, keeping track of pending operations may be difficult. For example, in GCP, operation metadata only provides the instance id, not the disk id. So the CSI driver would have to potentially 1) serialize operations per instance, which would not be good for performance or 2) cache operations in memory and only serialize on restart 3) keep operations state in some CRD

[msau42]

For reference, here is a prototype of adding operation caching in the GCP driver: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#923

[xing-yang]

Discussed at the triage meeting. We'll work on a design that is similar to how provisioner works and the attach/detach operations will be synchronized with an operations cache.

[xing-yang]

/priority important-soon

[xing-yang]

/triage accepted

[Sneha-at]

I will work on this issue, unable to assign to myself.

[k8s-triage-robot]

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged.
Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Deprioritize it with /priority important-longterm or /priority backlog
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted