Add a flag to disable force detach behavior in kube-controller-manager

[rohitssingh]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR adds a flag to allow the 6-minute force detach window to be disabled.

With this flag enabled, ControllerUnpublishVolume RPCs will not be dispatched if a given pod is stuck unmounting for 6-minutes and the node is deemed to be Unhealthy in that window.

Which issue(s) this PR fixes:

Fixes #120328

Special notes for your reviewer:

Does this PR introduce a user-facing change?

This change adds the following CLI option for `kube-controller-manager`:
* `disable-force-detach` (defaults to `false`): Prevent force detaching volumes based on maximum unmount time and node status. If enabled, the non-graceful node shutdown feature must be used to recover from node failure (see https://kubernetes.io/blog/2023/08/16/kubernetes-1-28-non-graceful-node-shutdown-ga/). If enabled and a pod must be forcibly terminated at the risk of corruption, then the appropriate VolumeAttachment object (see here: https://kubernetes.io/docs/reference/kubernetes-api/config-and-storage-resources/volume-attachment-v1/) must be deleted.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @rohitssingh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rohitssingh]

@bswartz @jingxu97 @msau42 @saad-ali

[alexzielenski]

/remove-sig api-machinery
/sig apps
/sig storage

[carlory]

/kind feature
/ok-to-test

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 10e35089f92ad5267d73ba3a9c4ce3f520fa3773

[msau42]

/assign @jpbetz

[bswartz]

/lgtm

[jpbetz]

/approve

[rohitssingh]

@jpbetz: Thanks for the feedback! I think I've addressed your comments. Please feel free to take another look.
@msau42 & @bswartz: Looks like the lgtm tag has been removed because I uploaded a new patch? Please feel free to take another look.

[jpbetz]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: a069353479a4fdd71897bdb60a40777182ba61c2

[jpbetz]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, msau42, rohitssingh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• cmd/kube-controller-manager/OWNERS [jpbetz]
• pkg/controller/volume/OWNERS [msau42]
• pkg/generated/openapi/OWNERS [msau42]
• staging/src/k8s.io/kube-controller-manager/config/OWNERS [msau42]
• test/OWNERS [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rohitssingh]

Thank you for your kindly detailed explanation. I agree with your chage.
It's just out of interest, in your team, the unmount of a pod takes too long time in spite of node broken? Approximately how many minutes does it take to unmount?

Hi @YuikoTakada,

I think we've encountered a number of different scenarios with force detach:

• In some cases, we have had misbehaving pods on otherwise healthy nodes that were being migrated and just ignored shutdown signals. These pods would never shutdown on their own, and often resulted in Zombie LUNs and data corruption.
• In other cases, we have had problems with the CSI driver not being able to process NodeUnpublishVolume or NodeUnstageVolume in a timely fashion. In cases where kubelet was restarted in the wrong window, or the load on a node was too high, the node would fall into an unhealthy state, leading to Zombie LUNs and corruption.

I don't think in any case we would have been able to unmount within any given time window. IMHO, We would very much prefer shutdown to hang indefinitely so that we can easily find & fix these problems (as opposed to discovering data corruption days or weeks later).

[sftim]

BTW, we can use Markdown in the changelog, including actual hyperlinks.