🐛 Machine deletion skips waiting for volumes detached for unreachable Nodes

[typeid]

What this PR does / why we need it:

When MHC are remediating an unresponsive node, CAPI drains the node and gives up waiting for pods to be deleted after the SkipWaitForDeleteTimeoutSeconds. If one of the pods it gave up on has an attached volume, the machine deletion gets stuck.

This PR aligns the behavior for volume detachment during deletion to what we're doing with the drains: if the node is unreachable, we skip detaching volumes.

Which issue(s) this PR fixes
Fixes #10661

/area machine

[k8s-ci-robot]

Hi @typeid. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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-sigs/prow repository.

[mjlshen]

This is how OpenShift's handles this situation as well https://github.com/openshift/machine-api-operator/blob/dcf1387cb69f8257345b2062cff79a6aefb1f5d9/pkg/controller/machine/drain_controller.go#L164-L171. Furthermore, this allows pods to successfully be deleted, triggering CSI drivers to detach the relevant volumes, which allows the machine to be replaced

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 038bbf764ac6107032bcd184e1434415cc660f3d

[jackfrancis]

@willie-yao @Jont828 Could you sanity check the equivalent functionality for MachinePoolMachines? Do we need changes there as well?

[sbueringer]

@willie-yao @Jont828 Could you sanity check the equivalent functionality for MachinePoolMachines? Do we need changes there as well?

Do MachineHealthChecks work with MachinePool Machines? (I just don't remember that we implemented anything for it, at least in core CAPI)

[chrischdi]

/ok-to-test

[willie-yao]

Could you sanity check the equivalent functionality for MachinePoolMachines? Do we need changes there as well?

I know for a fact that MachineHealthCheck is not implemented for ClusterClass MachinePools, as stated in the "Later" section of issue #5991. @Jont828 are you able to verify if they are implemented for MachinePoolMachines?

[enxebre]

@typeid can you clarify where exactly in this https://github.com/kubernetes/kubectl/blob/master/pkg/drain/drain.go#L268-L360 flow this makes the difference?

[mjlshen]

@enxebre GracePeriodSeconds (the difference) is in https://github.com/kubernetes/kubectl/blob/d3ad75324522280fa8bccd7ad949b34336f5fc84/pkg/drain/drain.go#L312
--> https://github.com/kubernetes/kubectl/blob/d3ad75324522280fa8bccd7ad949b34336f5fc84/pkg/drain/drain.go#L150
--> https://github.com/kubernetes/kubectl/blob/d3ad75324522280fa8bccd7ad949b34336f5fc84/pkg/drain/drain.go#L133-L136

Skipping a couple steps here, but GracePeriodSeconds then takes effect in the kube-apiserver https://github.com/kubernetes/apiserver/blob/259cd1817cb1cc73bc33029df69934ac5c0d07ea/pkg/registry/generic/registry/store.go#L1116 where the graceful will eventually evaluate to false after the grace period has expired and the pods will be deleted by the api server instead of being stuck in a Terminating state forever.

That said, this is also behavior that could also be handled via https://kubernetes.io/docs/concepts/cluster-administration/node-shutdown/#non-graceful-node-shutdown instead. The description exactly describes the symptoms of this bug as well.

When a node is shutdown but not detected by kubelet's Node Shutdown Manager, the pods that are part of a StatefulSet will be stuck in terminating status on the shutdown node and cannot move to a new running node. This is because kubelet on the shutdown node is not available to delete the pods so the StatefulSet cannot create a new pod with the same name. If there are volumes used by the pods, the VolumeAttachments will not be deleted from the original shutdown node so the volumes used by these pods cannot be attached to a new running node. As a result, the application running on the StatefulSet cannot function properly. If the original shutdown node comes up, the pods will be deleted by kubelet and new pods will be created on a different running node. If the original shutdown node does not come up, these pods will be stuck in terminating status on the shutdown node forever.

[enxebre]

That said, this is also behavior that could also be handled via https://kubernetes.io/docs/concepts/cluster-administration/node-shutdown/#non-graceful-node-shutdown instead. The description exactly describes the symptoms of this bug as well.

I guess for us to take advantage of it the instance would need to be in the process of being shut down already, so it makes sense to me to mimic it earlier, at least for now.

Skipping a couple steps here, but GracePeriodSeconds then takes effect in the kube-apiserver https://github.com/kubernetes/apiserver/blob/259cd1817cb1cc73bc33029df69934ac5c0d07ea/pkg/registry/generic/registry/store.go#L1116 where the graceful will eventually evaluate to false after the grace period has expired and the pods will be deleted by the api server instead of being stuck in a Terminating state forever.

So does this effectively invalidates our SkipWaitForDeleteTimeoutSeconds, since all pods will match GracePeriodSeconds and get terminated first?

[typeid]

I initially had the same understanding as @mjlshen's explanation, but I did more digging and I'm no longer sure just setting the grace period to 1 is actually solving the issue here.

Reason:

• if no grace period or -1 (CAPI case) is specified, we use the default deleteOptions: see here
• we call the eviction api with these empty deleteOptions: see here
• this results in using the default grace period for the object (30 seconds for pods if unspecified):
  • If this value is nil, the default grace period for the specified type will be used. source
  • For pods, this Defaults to 30 seconds. , see terminationGracePeriodSeconds here

So I would expect the following to happen (current logic as well as with 1 second grace period - nothing differs in terms of pod deletion):

• we evict all pods via goroutines at the same time
• we then wait for these pods to be deleted, but timeout after 20 seconds due to our global timeout
• we keep on retrying until we reach the SkipWaitForDeleteTimeoutSeconds time we set here, then we ignore the pods not yet deleted and move on. This means we show the drain as completed, the pods are still around.

However, at this point, the eviction API has already been called (both cases, current vs PR), and the deletion grace period should be set on the pod, so the api server should take care of it.
There seems to be no difference when it comes to actual pod deletion between explicitly setting it and letting it default, it should only decide the sooner or later.

I re-tested draining with grace period 1, and it doesn't delete the pods for my reproducer. I thought I had tried that, but by not working I think that confirms the logic above. Setting the grace period to 0, however, results in the pods deleting.

Further digging in apiserver...

• With grace period 0 we run different logic on the apiserver side, return (graceful = false, gracefulPending =false , err = nil).
• With grace period 1 (or -1, which results in deleteGracePeriod = pod setting, default 30 sec), we update the deletion timestamp and grace period: https://github.com/kubernetes/apiserver/blob/4aef12dc73bd08c68036e1f8990c869806cbab58/pkg/registry/rest/delete.go#L134 and return (graceful=true, gracefulpending=false, err = nil)

The logic after that gets a bit complex, but it seems like setting it to 0 could be what want for immediate deletion.

I tried reproducing this with MAPI MHC, and it skips the pod deletion but doesn't get stuck replacing the node: I'm not sure what happens with the volume. Maybe it doesn't block the node deletion due to existing volumes? I'll have to dig further on that.

So does this effectively invalidates our SkipWaitForDeleteTimeoutSeconds, since all pods will match GracePeriodSeconds and get terminated first?

Not necessarily. I think in the case the pods still don't delete even with grace period = 1, we would still skip them after their deletionTimestamp is older than 5 minutes with that setting. MAPI actually sets SkipWaitForDeleteTimeoutSeconds to 1 as well, though (here and here). We might want to mimic that as well?

I'll sync up with @mjlshen to get the same understanding and see if I'm possibly missing something here. :)

[enxebre]

With grace period 0 we run different logic on the apiserver side, return (graceful = false, gracefulPending =false , err = nil).
With grace period 1 (or -1, which results in deleteGracePeriod = pod setting, default 30 sec), we update the deletion timestamp and grace period: https://github.com/kubernetes/apiserver/blob/4aef12dc73bd08c68036e1f8990c869806cbab58/pkg/registry/rest/delete.go#L134 and return (graceful=true, gracefulpending=false, err = nil)

With a pending source code verification I assume the second case won't help in this scenario because it requires kubelet coordination via pod status, which is not possible because the kubelet is unreachable.

I tried reproducing this with MAPI MHC, and it skips the pod deletion but doesn't get stuck replacing the node: I'm not sure what happens with the volume. Maybe it doesn't block the node deletion due to existing volumes? I'll have to dig further on that.

Yes, the volume detaching decoupling form draining it's not implemented in mapi.

So to summarise, the erratic behaviour is that we have a semantic to ack an unreachable Node and an opinion on how react to it (We Assume that giving up on those pods after 5min is safe as the eviction request is satisfied by pdbs) but we are not consistent on that opinion for volumes dettachment.
So my preference would be that we add that semantic consistently to the volume detachment process. I.e I propose we add additional logic within isNodeVolumeDetachingAllowed / nodeVolumeDetachTimeoutExceeded to account for unreachable Nodes and give up after the same SkipWaitForDeleteTimeoutSeconds we have for drain.

FWIW see related old discussion when we introduced this semantic #2165 (comment)

[typeid]

That sounds good to me.

I think we should still make the drain consistent to what MAPI does for unreachable nodes: setting GracePeriod and the PodDeletionTimeout both to 1 second. While this still would not fix issue #10661 (to my current understanding), it would at least allow faster replacement of unreachable nodes. I will open a new issue for that and link a new PR to not cause extra confusion by remapping this PR.

[enxebre]

For history context this where we introduced permanent wait on detachment:
#4707

This is where we exposed it as API input:
#6285.

Now this PR aligns the behaviour with drain for unreachable Nodes.
/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: d99ffbe32ca8dfdfe705b34e26485170293b14c8

[enxebre]

ptal @chrischdi @sbueringer

[sbueringer]

I'll take a look, but I need some time to get a full picture

[chrischdi]

Implementation makes sense to me and matches the issue

/lgtm

[sbueringer]

Just a quick update, I'm looking into this. Just have to double check something with folks downstream. I'll get back to you ASAP.

[sbueringer]

/lgtm

Slightly renamed the PR, because this affects more than just MHC.

/hold
I want to check with some other folks, will get back to you ASAP

[sbueringer]

/hold cancel

[sbueringer]

Thx folks!

[sbueringer]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

• OWNERS [sbueringer]

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

[sbueringer]

/cherry-pick release-1.7

[k8s-infra-cherrypick-robot]

@sbueringer: new pull request created: #10765

In response to this:

/cherry-pick release-1.7

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-sigs/prow repository.