fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. #116134

cvvz · 2023-02-28T15:39:35Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. (#111933) Meanwhile Kubelet will keep printing the log Orphaned pod "xxx" found, but error not a directory occurred when trying to remove the volumes dir every 2 seconds. (#105536)

Although we have KEP 3756 to refactor the process of reconstructing ASW and DSW, which aims to fix these problems totally and some of the work has been done by introducing SELinuxMountReadWriteOncePod feature gate and adding new reconcile function, we still need to fix them under old implementation before the new feature is stable.

Which issue(s) this PR fixes:

Fixes #111933 #105536

Special notes for your reviewer:

No need to check volume existence during reconstruction and cleanup if mount point does not exist.

After the evicted Pods are reconstructed and added to ASW or skippedDuringReconstruction, the mount point will be finally cleaned up in the subsequence reconcile loop, since these Pods are not in DSW.

Does this PR introduce a user-facing change?

fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. (#111933) Meanwhile Kubelet will print the log `Orphaned pod "xxx" found, but error not a directory occurred when trying to remove the volumes dir` every 2 seconds. (#105536)

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

k8s-ci-robot · 2023-02-28T15:39:44Z

Hi @cvvz. 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.

cvvz · 2023-02-28T15:48:08Z

@dobsonj @gnufied @msau42 @jingxu97 @jsafrane @xing-yang
Could you please take a look? Thanks.

cvvz · 2023-03-01T05:12:37Z

cc @andyzhangx

andyzhangx

/ok-to-test

bart0sh · 2023-03-02T10:29:45Z

/triage accepted
/priority important-longterm
/cc @kubernetes/sig-storage-pr-reviews

k8s-ci-robot · 2023-03-02T10:29:47Z

@bart0sh: GitHub didn't allow me to request PR reviews from the following users: kubernetes/sig-storage-pr-reviews.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/triage accepted
/priority important-longterm
/cc @kubernetes/sig-storage-pr-reviews

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.

jsafrane · 2023-03-07T13:52:03Z

@cvvz can you please squash the commits, so we have a clean history?

pkg/kubelet/volumemanager/reconciler/reconciler_test.go

gnufied · 2023-03-07T17:37:43Z

pkg/kubelet/volumemanager/reconciler/reconstruct_common.go

- if !isExist {
- return nil, fmt.Errorf("volume: %q is not mounted", uniqueVolumeName)
- }
-
 reconstructedVolume := &reconstructedVolume{


Should we at least check for existence of mount or map path? If they don't exist (but may be pod directory exists) - is there a point in doing reconstruction?

Reconstruction is still needed when mount or map path doesn't exist, since in this situation, we can rely on unmountDevice and unmountVolume to clean up both the global mount path and pod mount path whereas cleanupMounts can only cleanup the mount path in Pod directory.

+1 to @cvvz, at this point, operationExecutor.ReconstructVolumeOperation has succeeded , so the volume plugin should have enough information to clean up the volume by itself.

but if pod still running, we add it to asw directly, volume manage will not remount this volume forever, container will use local path instead of remote path.

If Pod is still running, then the volume should exist in DSW, then it won't be added to ASW:

kubernetes/pkg/kubelet/volumemanager/reconciler/reconstruct.go

Lines 87 to 97 in e575e60

if volumeInDSW {

// Some pod needs the volume. And it exists on disk. Some previous

// kubelet must have created the directory, therefore it must have

// reported the volume as in use. Mark the volume as in use also in

// this new kubelet so reconcile() calls SetUp and re-mounts the

// volume if it's necessary.

volumeNeedReport = append(volumeNeedReport, reconstructedVolume.volumeName)

rc.skippedDuringReconstruction[reconstructedVolume.volumeName] = gvl

klog.V(4).InfoS("Volume exists in desired state, marking as InUse", "podName", volume.podName, "volumeSpecName", volume.volumeSpecName)

continue

}

commit 1b3ae27e7af577372d5aaaf28ea401eb33d1c4df Author: weizhichen <[email protected]> Date: Thu Mar 9 08:39:04 2023 +0000 fix commit 566e139308e3cec4c9d4765eb4ccc3a735346c2e Author: weizhichen <[email protected]> Date: Thu Mar 9 08:36:32 2023 +0000 fix unit test commit 13a58ebd25b824dcf854a132e9ac474c8296f0bf Author: weizhichen <[email protected]> Date: Thu Mar 2 03:32:39 2023 +0000 add unit test commit c984e36e37c41bbef8aec46fe3fe81ab1c6a2521 Author: weizhichen <[email protected]> Date: Tue Feb 28 15:25:56 2023 +0000 fix imports commit 58ec617e0ff1fbd209ca0af3237017679c3c0ad7 Author: weizhichen <[email protected]> Date: Tue Feb 28 15:24:21 2023 +0000 delete CheckVolumeExistenceOperation commit 0d8cf0caa78bdf1f1f84ce011c4cc0e0de0e8707 Author: weizhichen <[email protected]> Date: Tue Feb 28 14:29:37 2023 +0000 fix 111933

cvvz · 2023-03-09T12:36:26Z

/retest

cvvz · 2023-03-10T00:03:20Z

/retest

cvvz · 2023-03-31T10:34:18Z

Hi, @gnufied @jsafrane Shall we move this pr on?

jsafrane · 2023-04-05T12:03:19Z

/lgtm
/approve

k8s-ci-robot · 2023-04-05T12:03:26Z

LGTM label has been added.

Git tree hash: 02acc88fcce5d632091a150161a97f8dd1081dce

k8s-ci-robot · 2023-04-05T12:03:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cvvz, jsafrane

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~~ [jsafrane]
~~pkg/volume/OWNERS~~ [jsafrane]

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

xing-yang · 2023-04-05T17:41:21Z

It should be safe to backport to 1.27, but backporting to older releases needs more investigation.

jsafrane · 2023-04-06T09:47:26Z

I checked 1.26 with the old-style volume reconstruction.

If reconstructVolume() succeeds:

if the volume is already in DSW, continue here skips adding it to ASW (that's good):

kubernetes/pkg/kubelet/volumemanager/reconciler/reconstruct.go

Lines 87 to 97 in e575e60

 if volumeInDSW { 

 // Some pod needs the volume. And it exists on disk. Some previous 

 // kubelet must have created the directory, therefore it must have 

 // reported the volume as in use. Mark the volume as in use also in 

 // this new kubelet so reconcile() calls SetUp and re-mounts the 

 // volume if it's necessary. 

 volumeNeedReport = append(volumeNeedReport, reconstructedVolume.volumeName) 

 rc.skippedDuringReconstruction[reconstructedVolume.volumeName] = gvl 

 klog.V(4).InfoS("Volume exists in desired state, marking as InUse", "podName", volume.podName, "volumeSpecName", volume.volumeSpecName) 

 continue 

 }

If the volume is not in DSW, it's added to ASW as Mounted (that's bad):

kubernetes/pkg/kubelet/volumemanager/reconciler/reconstruct.go

Line 149 in e575e60

err = rc.markVolumeState(volume, operationexecutor.VolumeMounted)

So if kubelet gets a new Pod that uses the volume, then it will see it as mounted, while it could have been already unmounted.

Therefore this PR is not suitable for the old-style volume reconstruction + backports to 1.25-1.26. I don't think it is safe to mark the volumes as uncertain, that's why we reworked the volume reconstruction and put it behind a feature gate.

cvvz · 2023-04-08T01:41:19Z

If the volume is not in DSW, it's added to ASW as Mounted (that's bad)
So if kubelet gets a new Pod that uses the volume, then it will see it as mounted, while it could have been already unmounted.

I think it would be ok. If kubelet gets a new Pod that uses the volume, the followings are going to happen:

Since the new Pod and old Pod uid is not the same, so rc.unmountVolumes() for old pod Volumes should succeed.
rc.unmountDetachDevices() will be skipped since DSW has the same volumeName in ASW.
rc.mountOrAttachVolumes() for new Pod will try to mount device if the plugin is DeviceMountable and NOT DeviceGloballyMounted: it will first make mount dir and save metadata to vol_data.json, I think the operations won't be failed even if the mount dir and vol_data.json already exist. And then call NodeStageVolume to mount device, after that mount new Pod volumes.

So, I think we need to get the mount state of device mount path during reconstruction, if the device mount path is not mounted, then set the state as DeviceMountUncertain.

WDYT? @jsafrane

andyzhangx · 2023-04-12T02:22:57Z

not sure whether it's suitable to backports to 1.25-1.26, @cvvz could you back port to 1.27 first? thanks.

…-origin-release-1.27 Automated cherry pick of #116134: fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully.

migs35323 · 2023-12-19T09:57:31Z

How about backport to 1.26, or even previous ones?

k8s-ci-robot requested review from chrishenzie and xing-yang February 28, 2023 15:40

andyzhangx reviewed Mar 1, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2023

bart0sh added this to Triage in SIG Node PR Triage Mar 1, 2023

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2023

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Mar 2, 2023

jsafrane reviewed Mar 7, 2023

View reviewed changes

pkg/kubelet/volumemanager/reconciler/reconciler_test.go Outdated Show resolved Hide resolved

gnufied reviewed Mar 7, 2023

View reviewed changes

cvvz force-pushed the fix-111933 branch from 1b3ae27 to 81fe919 Compare March 9, 2023 08:43

cvvz force-pushed the fix-111933 branch from 81fe919 to a6ffbb4 Compare March 9, 2023 09:54

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2023

bart0sh moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Apr 7, 2023

k8s-ci-robot merged commit 4893c66 into kubernetes:master Apr 11, 2023

SIG Node PR Triage automation moved this from Needs Approver to Done Apr 11, 2023

k8s-ci-robot added this to the v1.28 milestone Apr 11, 2023

cvvz mentioned this pull request Apr 12, 2023

Automated cherry pick of #116134: fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. #117235

Merged

cvvz mentioned this pull request Apr 21, 2023

Mount point will be local volume without calling NodeStageVolume if mount point was left over before node rebooting #117513

Open

msau42 mentioned this pull request May 3, 2023

Disable NewVolumeManagerReconstruction and SELinuxMountReadWriteOncePod feature gate #117751

Merged

ejweber mentioned this pull request Jul 12, 2023

docs: Add KB about failure to delete orphaned pod volume directory longhorn/website#680

Merged

roger-ryao mentioned this pull request Aug 31, 2023

[BUG] orphaned pod pod_id found, but error not a directory occurred when trying to remove the volumes dir longhorn/longhorn#3207

Closed

yardenshoham mentioned this pull request Sep 6, 2023

Orphaned pod found, but error not a directory occurred when trying to remove the volumes dir #105536

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. #116134

fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. #116134

cvvz commented Feb 28, 2023 •

edited

k8s-ci-robot commented Feb 28, 2023

cvvz commented Feb 28, 2023 •

edited

cvvz commented Mar 1, 2023

andyzhangx left a comment

bart0sh commented Mar 2, 2023

k8s-ci-robot commented Mar 2, 2023

jsafrane commented Mar 7, 2023

gnufied Mar 7, 2023

cvvz Mar 8, 2023 •

edited

jsafrane Apr 5, 2023

Pingan2017 Apr 14, 2023

cvvz Apr 20, 2023

cvvz commented Mar 9, 2023

cvvz commented Mar 10, 2023

cvvz commented Mar 31, 2023

jsafrane commented Apr 5, 2023

k8s-ci-robot commented Apr 5, 2023

k8s-ci-robot commented Apr 5, 2023

xing-yang commented Apr 5, 2023

jsafrane commented Apr 6, 2023

cvvz commented Apr 8, 2023 •

edited

andyzhangx commented Apr 12, 2023

migs35323 commented Dec 19, 2023

	if volumeInDSW {
	// Some pod needs the volume. And it exists on disk. Some previous
	// kubelet must have created the directory, therefore it must have
	// reported the volume as in use. Mark the volume as in use also in
	// this new kubelet so reconcile() calls SetUp and re-mounts the
	// volume if it's necessary.
	volumeNeedReport = append(volumeNeedReport, reconstructedVolume.volumeName)
	rc.skippedDuringReconstruction[reconstructedVolume.volumeName] = gvl
	klog.V(4).InfoS("Volume exists in desired state, marking as InUse", "podName", volume.podName, "volumeSpecName", volume.volumeSpecName)
	continue
	}

fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. #116134

fix: After a Node is down and take some time to get back to up again, the mount point of the evicted Pods cannot be cleaned up successfully. #116134

Conversation

cvvz commented Feb 28, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Feb 28, 2023

cvvz commented Feb 28, 2023 • edited

cvvz commented Mar 1, 2023

andyzhangx left a comment

Choose a reason for hiding this comment

bart0sh commented Mar 2, 2023

k8s-ci-robot commented Mar 2, 2023

jsafrane commented Mar 7, 2023

gnufied Mar 7, 2023

Choose a reason for hiding this comment

cvvz Mar 8, 2023 • edited

Choose a reason for hiding this comment

jsafrane Apr 5, 2023

Choose a reason for hiding this comment

Pingan2017 Apr 14, 2023

Choose a reason for hiding this comment

cvvz Apr 20, 2023

Choose a reason for hiding this comment

cvvz commented Mar 9, 2023

cvvz commented Mar 10, 2023

cvvz commented Mar 31, 2023

jsafrane commented Apr 5, 2023

k8s-ci-robot commented Apr 5, 2023

k8s-ci-robot commented Apr 5, 2023

xing-yang commented Apr 5, 2023

jsafrane commented Apr 6, 2023

cvvz commented Apr 8, 2023 • edited

andyzhangx commented Apr 12, 2023

migs35323 commented Dec 19, 2023

cvvz commented Feb 28, 2023 •

edited

cvvz commented Feb 28, 2023 •

edited

cvvz Mar 8, 2023 •

edited

cvvz commented Apr 8, 2023 •

edited