Fix pod stuck in termination state when mount fails or gets skipped after kubelet restart #110670

gnufied · 2022-06-21T01:31:06Z

This PR partially uses changes from - #110575

This PR:

Keeps tracks of volumes which were skipped during reconstruction with the assumption that since they were present in DSOW, they will make it to ASOW.
Enhances the check that makes volumes uncertain on unmounted - https://github.com/kubernetes/kubernetes/pull/110670/files#diff-3e8f8a6e5eca57db8e84bcc6c896f0a3b10289fe55d8e999b4bdf1cfdc0de394L785 to ensure that, we are not accidentally marking volumes which were added via reconsturction as unmounted because mount is failing after kubelet restart.

There are two important things in this PR:

It adds volumes skipped during reconstruction as uncertain in ASOW.
But above change alone is not enough to fix the bug (or unmount the volume), because - when kubelet will retry to mount the volume, mount will fail and it will mark the volume we added as uncertain in previous step as "unmounted" - https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/operationexecutor/operation_generator.go#L788 . So basically if mount operation fails, kubelet will undo our hard work of adding volumes which we added as uncertain in step#1.

Now it is clear that - we need to add volumes we read from disk as uncertain, so as kubelet can unmount them properly. But there are two ways to fix the second problem (i.e where kubelet is marking mount point as unmounted, if mount fails for a volume which was marked uncertain before).

In this PR - I am keeping track of such reconstructed volumes in ASOW, so as even if mount fails for the volume it must not mark the volume was unmounted, if volume was added via reconstruction.
There is another easier way - which is to make all volumes uncertain in ASOW if mount fails. That is what - Make mount errors uncertain #110650 PR does. This code however breaks an existing behaviour where if kubelet starts mounting a volume and may be operation timed-out (marked uncertain) and kubelet retries and mount fails then volume is marked as unmounted and a pod that was using this volume can be deleted without unmounting the volume. If we treat every mount failure as uncertain mount, then volume must be unmounted for pod to be deleted.

Unmount volumes correctly for reconstructed volumes even if mount operation fails after kubelet restart

k8s-ci-robot · 2022-06-21T01:32:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied

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:

~~pkg/kubelet/volumemanager/OWNERS~~ [gnufied]
~~pkg/volume/OWNERS~~ [gnufied]

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

aojea · 2022-06-21T08:40:41Z

/assign @smarterclayton @bobbypage

pkg/kubelet/volumemanager/reconciler/reconciler.go

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

pkg/volume/util/operationexecutor/operation_executor.go

pkg/volume/util/operationexecutor/operation_generator.go

jsafrane · 2022-06-23T12:47:37Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

+		}
+
+		if uncertainVolumeCount > 0 {
+			// If the volume has device to mount, we mark its device as mounted.


"mark its device as uncertain"

jingxu97 · 2022-06-30T00:51:53Z

How about during reconstruction, we put all reconstructed volumes into actual state with uncertain mode even if desired state has this information (we might need to sync between desired state and actual state which already there)?

gnufied · 2022-06-30T02:05:50Z

@jingxu97 we could and that is what #108180 PR by @jsafrane does. But adding all volumes to ASW in uncertain state during reconstruction brings new challenges, such as if we add a volume to ASW before it gets added to DSW, then we could accidently unmount the volume. So it has all these additional checks to safeguard against similar failure modes - https://github.com/kubernetes/kubernetes/pull/108180/files#diff-2acbd2d6c088f99f9c7e9042b3449418b4824675d2692350c332d0f276489268R40 ( @jsafrane correct me if I am wrong).

But that alone will not fix the issue, because we still need to avoid marking volumes which we added via recontruction to ASW as "unmounted" (from previous "uncertan" state if mount fails) - f291291#diff-3e8f8a6e5eca57db8e84bcc6c896f0a3b10289fe55d8e999b4bdf1cfdc0de394R787

So we have this middle ground PR of mine, which avoids those problems and still fixes issues we set out to fix.

gnufied · 2022-06-30T02:13:39Z

/retest

jsafrane · 2022-06-30T12:16:33Z

As Hemant pointed out. will need all reconstructed volumes in ASW for my SELinux work. But I can have it behind a feature gate + we don't need to worry about backporting it as a bugfix to older releases. (And yes, I need to update it with the latest Hemant's fixes from this PR.) This PR can be somewhere in middle and could be backported to older releases.

gnufied · 2022-06-30T14:17:38Z

Yeah eventually once jan's PR gets merged and enabled by default - we could delete the separate processReconstructedVolumes function and have reconstruction add all volumes. We will still need part#2 of this PR - which tracks reconstructed volumes in ASOW, so as we are not accidently flipping mount state of uncertain volumes which were added to ASOW via reconstruction on mount failures.

Add unit tests for removing reconstructed volumes from ASOW

Also only remove volumes from skippedDuringReconstruction only if volume was marked as attached.

gnufied · 2022-07-24T01:44:09Z

/retest

jsafrane · 2022-07-27T11:40:20Z

/lgtm

gnufied · 2022-07-27T12:14:35Z

/hold cancel

rphillips · 2022-08-02T13:22:42Z

Created a cherry pick for 1.24 here.

…10670-upstream-release-1.24 Automated cherry pick of #110670: Keep track of each pod that uses a volume during

To preserve fix in kubernetes#110670, add an unit test that check a volume is *uncertain* even after final mount error when it was reconstructed. And actually fix a regression introduced in the previous patch.

k8s-ci-robot requested review from jingxu97 and xing-yang June 21, 2022 01:31

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 21, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2022

This was referenced Jun 21, 2022

Make mount errors uncertain #110650

Closed

Attempt to reconstruct the volume for a single pod #109991

Closed

k8s-ci-robot assigned bobbypage and smarterclayton Jun 21, 2022

gnufied force-pushed the fix-pod-deletion-terminating branch from 286a06e to f291291 Compare June 21, 2022 22:17

jsafrane reviewed Jun 23, 2022

View reviewed changes

pacoxu added this to Needs Reviewer in SIG Node PR Triage Jun 24, 2022

pacoxu moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Jun 24, 2022

pacoxu moved this from Waiting on Author to Triage in SIG Node PR Triage Jun 24, 2022

gnufied force-pushed the fix-pod-deletion-terminating branch from c1623ee to 3801ad1 Compare June 29, 2022 20:21

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 30, 2022

gnufied added 2 commits July 22, 2022 20:04

Remove volume from found during reconstruction if mounted

6d43345

Add unit tests for removing reconstructed volumes from ASOW

Use CheckAndMarkAsUncertainViaReconstruction for uncertain volumes

835e8cc

Also only remove volumes from skippedDuringReconstruction only if volume was marked as attached.

gnufied force-pushed the fix-pod-deletion-terminating branch from ee28639 to 835e8cc Compare July 23, 2022 11:36

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2022

k8s-ci-robot merged commit 9ad4c5c into kubernetes:master Jul 27, 2022

SIG Node PR Triage automation moved this from Triage to Done Jul 27, 2022

k8s-ci-robot added this to the v1.25 milestone Jul 27, 2022

This was referenced Aug 2, 2022

Bug 2113977: UPSTREAM: 110670: Fix pod stuck in termination state when mount fails… openshift/kubernetes#1336

Closed

Automated cherry pick of #110670: Keep track of each pod that uses a volume during #111636

Merged

This was referenced Aug 2, 2022

Pods stuck terminating (MountVolume.SetUp failed) #70044

Closed

Keep track of each pod that uses a volume during reconstruction #110575

Closed

innobead mentioned this pull request Aug 4, 2022

[BUG] Volumes are not properly mounted/unmounted when kubelet restarts longhorn/longhorn#2629

Closed

k8s-ci-robot added a commit that referenced this pull request Aug 10, 2022

Merge pull request #111636 from rphillips/automated-cherry-pick-of-#1…

e3f808e

…10670-upstream-release-1.24 Automated cherry pick of #110670: Keep track of each pod that uses a volume during

rphillips mentioned this pull request Oct 24, 2022

Automated cherry pick of #110670: Keep track of each pod that uses a volume during #113300

Closed

gnufied mentioned this pull request Oct 27, 2022

Reconstruction only cleans one path of shared volume between two pods leaving other one uncleaned #110547

Closed

cjohnston1158 mentioned this pull request Oct 28, 2022

Known-issues bug fixed charmed-kubernetes/kubernetes-docs#730

Closed

This was referenced Nov 3, 2022

Fix pod stuck in termination state when mount fails or gets skipped after kubelet restart #113589

Merged

Rework volume reconstruction #113262

Merged

qiliRedHat mentioned this pull request Apr 12, 2023

kubelet fails with UnmountVolume.NewUnmounter failed for volume and vol_data.json: no such file or directory for CSI volumes and floods the logs #116847

Closed

cvvz mentioned this pull request Aug 14, 2023

fix: mount point may become local volume without calling NodeStageVolume after node rebooting. #119827

Closed

jsafrane mentioned this pull request Aug 16, 2023

fix: Mount point may become local without calling NodePublishVolume after node rebooting #119923

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pod stuck in termination state when mount fails or gets skipped after kubelet restart #110670

Fix pod stuck in termination state when mount fails or gets skipped after kubelet restart #110670

gnufied commented Jun 21, 2022 •

edited

k8s-ci-robot commented Jun 21, 2022

aojea commented Jun 21, 2022

jsafrane Jun 23, 2022

gnufied Jun 29, 2022

jingxu97 commented Jun 30, 2022

gnufied commented Jun 30, 2022

gnufied commented Jun 30, 2022

jsafrane commented Jun 30, 2022

gnufied commented Jun 30, 2022 •

edited

gnufied commented Jul 24, 2022

jsafrane commented Jul 27, 2022

gnufied commented Jul 27, 2022

rphillips commented Aug 2, 2022

Fix pod stuck in termination state when mount fails or gets skipped after kubelet restart #110670

Fix pod stuck in termination state when mount fails or gets skipped after kubelet restart #110670

Conversation

gnufied commented Jun 21, 2022 • edited

k8s-ci-robot commented Jun 21, 2022

aojea commented Jun 21, 2022

jsafrane Jun 23, 2022

Choose a reason for hiding this comment

gnufied Jun 29, 2022

Choose a reason for hiding this comment

jingxu97 commented Jun 30, 2022

gnufied commented Jun 30, 2022

gnufied commented Jun 30, 2022

jsafrane commented Jun 30, 2022

gnufied commented Jun 30, 2022 • edited

gnufied commented Jul 24, 2022

jsafrane commented Jul 27, 2022

gnufied commented Jul 27, 2022

rphillips commented Aug 2, 2022

gnufied commented Jun 21, 2022 •

edited

gnufied commented Jun 30, 2022 •

edited