search

kubernetes-csi / external-attacher

Sidecar container that watches Kubernetes VolumeAttachment objects and triggers ControllerPublish/Unpublish against a CSI endpoint
Apache License 2.0
169 stars 186 forks source link

Uncertain handling for attach #400

Open msau42 opened 1 year ago

msau42 commented 1 year ago

It looks like we check for finalError on ControllerPublishVolume: https://github.com/kubernetes-csi/external-attacher/blob/c4cfca3cd22d7437f0a1d712e3fac30211d6ec09/pkg/attacher/attacher.go#L70

But we ignore it later on: https://github.com/kubernetes-csi/external-attacher/blob/c4cfca3cd22d7437f0a1d712e3fac30211d6ec09/pkg/controller/csi_handler.go#L513

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 commented 1 year ago

cc @jsafrane @gnufied

jsafrane commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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

xing-yang commented 1 year ago

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 commented 1 year ago

/priority important-soon

xing-yang commented 1 year ago

/triage accepted

Sneha-at commented 1 year ago

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

k8s-triage-robot commented 1 year ago

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:

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

/remove-triage accepted

msau42 commented 1 year ago

/priority important-longterm /triage accepted

k8s-triage-robot commented 10 months ago

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:

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

/remove-triage accepted

msau42 commented 8 months ago

/priority important-longterm /triage accepted

k8s-triage-robot commented 5 months ago

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:

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

/remove-triage accepted

msau42 commented 3 months ago

/remove-priority important-soon

k8s-triage-robot commented 2 weeks ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale