fix: the volume is not detached after the pod and PVC objects are deleted #116138

cvvz · 2023-02-28T17:19:36Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When vol_data.json file does not exist, skip UnmountDevice since it must succeed before, no need to do UnmountDevice multiple times.

Which issue(s) this PR fixes:

Fixes #114207

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix 1.24 regression: the volume is not detached after the pod and PVC objects are deleted

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

…eted

k8s-ci-robot · 2023-02-28T17:19:45Z

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-03-01T05:11:57Z

cc @andyzhangx

Chaunceyctx · 2023-03-01T08:15:34Z

Is there any better solution to fix a TOCTOU bug in unmountDetachDevices. In other words, kubelet will perform unmountDevice operation only once. @jingxu97 @cvvz

andyzhangx

/ok-to-test

cvvz · 2023-03-01T12:24:32Z

Is there any better solution to fix a TOCTOU bug in unmountDetachDevices. In other words, kubelet will perform unmountDevice operation only once. @jingxu97 @cvvz

@Chaunceyctx Thanks for you comment. After analyzing the logs and code, I do find there could be a TOCTOU bug in unmountDetachDevices process. I will explain more specificly later.

Anyway, I think it is worth to take a look at this PR first, since after there is no volume name in the global mount path, getDriverAndVolNameFromDeviceMountPath is useless and we can just rely on vol_data.json. The only risk will be vol_data.json file is corrupted for some reason, which should never happen IMO.

cvvz · 2023-03-01T14:15:01Z

Here is the time-series chart, the left side is the reconcile loop, the right side is the async UnmountDevice operation, the red box indicates the first time of unmountDetachDevices operation, and the green box indicates the second time of unmountDetachDevices operation, the timestamp is from the logs provided by @Chaunceyctx.

Hopefully I can make things clear.

jingxu97 · 2023-03-02T01:13:53Z

@cvvz thanks a lot for the diagram. Yes, this is the issue I was thinking about. There could be multiple places for fix this issue,

Avoid operation are called multiple times (by fixing the TOCTOU). However I don't think we can make a guarantee on this since operation might timeout etc.
Make sure operation is idempotent. I think this is necessary because previous one has no guarantee. There are two related operations here, one is the kubelet call for UnmountDevice, the other is CSI driver side to trigger UnstageVolume.

Should we consider add fixes for all those places?

/cc @msau42 @gnufied @jsafrane

msau42 · 2023-03-06T18:02:31Z

Thanks for putting together the diagram! What if after checking IsOperationPending, we try to get a fresh volume state? Will that solve the TOCTOU?

cc @sunnylovestiramisu

msau42 · 2023-03-06T18:08:31Z

pkg/volume/csi/csi_attacher.go

@@ -595,14 +595,13 @@ func (c *csiAttacher) UnmountDevice(deviceMountPath string) error {
 driverName = data[volDataKey.driverName]
 volID = data[volDataKey.volHandle]
 } else {
- klog.Error(log("UnmountDevice failed to load volume data file [%s]: %v", dataDir, err))
-
- // The volume might have been mounted by old CSI volume plugin. Fall back to the old behavior: read PV from API server


Was this logic to handle a scenario where someone didn't drain the node before upgrading? Is this safe to remove now? cc @mattcary @saikat-royc

I think vol_data.json was brought in #64882 5 years ago, so now no need to fall back to the old behavior when it does not exist anymore, it should be safe to remove.

shall we move this PR forward since it really solves the problem per our large scale test, there is no disk detach issue any more. @mattcary @saikat-royc @msau42

#107065 is the PR where the new global path is introduced (1.24). The PR states that the node need to be drained explicitly in its release note. Change node staging path for csi driver to use a PV agnostic path. Nodes must be drained before updating the kubelet with this change.

So I think if vol_data.json does not exist, we can skip the else part of the code to parse the path. [Note: this also means that this PR can be cherry-picked to >= 1.24, since we already have a dependency to drain node when upgrading from < 1.24 to 1.24]. So +1 from my side to remove the fallback mountpath parsing

cvvz · 2023-03-08T03:32:01Z

Thanks for putting together the diagram! What if after checking IsOperationPending, we try to get a fresh volume state? Will that solve the TOCTOU?

How about making deviceMountState as a pointer? So we won't get the stale state.

sunnylovestiramisu · 2023-03-08T19:49:29Z

Thanks for the diagram. Is removing the vol_data.json file last step of unmount? Will there be any failures after that step?

cvvz · 2023-03-09T00:38:06Z

Yes it is. kubelet will only remove vol_data.json after NodeUnstageVolume succeed.https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/csi/csi_attacher.go#L635-L646

andyzhangx · 2023-03-13T03:11:31Z

This issue was consistently reproducible after ~5 runs of a 1K disk attach/detach load test. However, with this PR, 100 test runs of the same load test were conducted in k8s 1.25.5 and the issue no longer occurred.

andyzhangx · 2023-03-16T08:38:16Z

/priority important-soon
/sig storage

k8s-ci-robot · 2023-03-30T08:17:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, 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/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

andyzhangx · 2023-04-01T03:03:21Z

@jsafrane shall we merge this PR in milestone v1.27?

jsafrane · 2023-04-03T12:07:53Z

I'd wait for 1.27 GA and then open cherry-picks.

ialidzhikov · 2023-04-12T07:14:24Z

Can we now proceed with the backports? Thanks in advance!

ialidzhikov · 2023-04-14T07:39:41Z

@cvvz can we backport to all affected releases? Currently I see only a backport for the release-1.27 branch.

cvvz · 2023-04-14T08:25:28Z

I will backport to 1.25 and 1.26

humblec · 2023-04-14T10:54:35Z

@cvvz can we have the 1.24 backport too ?

dguendisch · 2023-04-14T11:00:44Z

While I cannot answer if you "have to", I can only second that we see this issue massively on 1.24 clusters and a back port would be super welcomed 😊

cvvz · 2023-04-14T13:34:28Z

sure, backport to 1.24

humblec · 2023-04-14T13:51:29Z

While I cannot answer if you "have to", I can only second that we see this issue massively on 1.24 clusters and a back port would be super welcomed 😊

Thanks for the info! , I was looking into 1.24 code and was trying to confirm :)

Thanks @cvvz for filing it ! 👍

…-origin-release-1.24 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

…-origin-release-1.26 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

…-origin-release-1.25 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

…-origin-release-1.27 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

andyzhangx · 2023-05-05T02:35:13Z

FYI. this issue is fixed in 1.24.14, 1.25.10, 1.26.5, and has been fixed in v1.27.0

shadowdsp · 2023-06-30T07:42:14Z

@cvvz can we have the 1.19 backport? Thanks

mattcary · 2023-06-30T16:15:26Z

@cvvz can we have the 1.19 backport? Thanks

It's only possible to backport to active k8s releases. Branches for end-of-life versions have been frozen in my understanding. So we can't backport past 1.24: https://kubernetes.io/releases/.

fix: the volume is not detached after the pod and PVC objects are del…

b0eeeac

…eted

k8s-ci-robot requested review from jingxu97 and mattcary February 28, 2023 17:20

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

msau42 reviewed Mar 6, 2023

View reviewed changes

cvvz mentioned this pull request Mar 7, 2023

REQUEST: New membership for cvvz kubernetes/org#4076

Closed

9 tasks

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

k8s-ci-robot merged commit de10997 into kubernetes:master 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 #116138: fix: the volume is not detached after the pod and PVC objects #117236

Merged

This was referenced Apr 14, 2023

Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects #117339

Merged

Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects #117340

Merged

cvvz mentioned this pull request Apr 14, 2023

Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects #117358

Merged

k8s-ci-robot added a commit that referenced this pull request May 4, 2023

Merge pull request #117358 from cvvz/automated-cherry-pick-of-#116138…

d6fc7f3

…-origin-release-1.24 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

k8s-ci-robot added a commit that referenced this pull request May 4, 2023

Merge pull request #117340 from cvvz/automated-cherry-pick-of-#116138…

a9e76b0

…-origin-release-1.26 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

k8s-ci-robot added a commit that referenced this pull request May 4, 2023

Merge pull request #117339 from cvvz/automated-cherry-pick-of-#116138…

7c65d2a

…-origin-release-1.25 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

k8s-ci-robot added a commit that referenced this pull request May 4, 2023

Merge pull request #117236 from cvvz/automated-cherry-pick-of-#116138…

7352d95

…-origin-release-1.27 Automated cherry pick of #116138: fix: the volume is not detached after the pod and PVC objects

martysweet mentioned this pull request Jun 20, 2023

PVC attaching takes much time kubernetes-sigs/aws-ebs-csi-driver#1302

Closed

martysweet mentioned this pull request Jun 29, 2023

Could not unmount - unmount failed: exit status 32 kubernetes-sigs/aws-ebs-csi-driver#1663

Closed

diranged mentioned this pull request Aug 24, 2023

Upgrade EKS-D packages - resolve EBS PVC attaching for a long time bottlerocket-os/bottlerocket#3230

Closed

liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Sep 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: the volume is not detached after the pod and PVC objects are deleted #116138

fix: the volume is not detached after the pod and PVC objects are deleted #116138

cvvz commented Feb 28, 2023 •

edited by liggitt

k8s-ci-robot commented Feb 28, 2023

cvvz commented Mar 1, 2023

Chaunceyctx commented Mar 1, 2023 •

edited

andyzhangx left a comment

cvvz commented Mar 1, 2023 •

edited

cvvz commented Mar 1, 2023 •

edited

jingxu97 commented Mar 2, 2023 •

edited

msau42 commented Mar 6, 2023

msau42 Mar 6, 2023

cvvz Mar 8, 2023 •

edited

andyzhangx Mar 16, 2023

saikat-royc Mar 31, 2023

cvvz commented Mar 8, 2023 •

edited

sunnylovestiramisu commented Mar 8, 2023

cvvz commented Mar 9, 2023

andyzhangx commented Mar 13, 2023 •

edited

andyzhangx commented Mar 16, 2023

k8s-ci-robot commented Mar 30, 2023

andyzhangx commented Apr 1, 2023

jsafrane commented Apr 3, 2023

ialidzhikov commented Apr 12, 2023

ialidzhikov commented Apr 14, 2023

cvvz commented Apr 14, 2023

humblec commented Apr 14, 2023 •

edited

dguendisch commented Apr 14, 2023

cvvz commented Apr 14, 2023

humblec commented Apr 14, 2023

andyzhangx commented May 5, 2023

shadowdsp commented Jun 30, 2023

mattcary commented Jun 30, 2023

fix: the volume is not detached after the pod and PVC objects are deleted #116138

fix: the volume is not detached after the pod and PVC objects are deleted #116138

Conversation

cvvz commented Feb 28, 2023 • edited by liggitt

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 Mar 1, 2023

Chaunceyctx commented Mar 1, 2023 • edited

andyzhangx left a comment

Choose a reason for hiding this comment

cvvz commented Mar 1, 2023 • edited

cvvz commented Mar 1, 2023 • edited

jingxu97 commented Mar 2, 2023 • edited

msau42 commented Mar 6, 2023

msau42 Mar 6, 2023

Choose a reason for hiding this comment

cvvz Mar 8, 2023 • edited

Choose a reason for hiding this comment

andyzhangx Mar 16, 2023

Choose a reason for hiding this comment

saikat-royc Mar 31, 2023

Choose a reason for hiding this comment

cvvz commented Mar 8, 2023 • edited

sunnylovestiramisu commented Mar 8, 2023

cvvz commented Mar 9, 2023

andyzhangx commented Mar 13, 2023 • edited

andyzhangx commented Mar 16, 2023

k8s-ci-robot commented Mar 30, 2023

andyzhangx commented Apr 1, 2023

jsafrane commented Apr 3, 2023

ialidzhikov commented Apr 12, 2023

ialidzhikov commented Apr 14, 2023

cvvz commented Apr 14, 2023

humblec commented Apr 14, 2023 • edited

dguendisch commented Apr 14, 2023

cvvz commented Apr 14, 2023

humblec commented Apr 14, 2023

andyzhangx commented May 5, 2023

shadowdsp commented Jun 30, 2023

mattcary commented Jun 30, 2023

cvvz commented Feb 28, 2023 •

edited by liggitt

Chaunceyctx commented Mar 1, 2023 •

edited

cvvz commented Mar 1, 2023 •

edited

cvvz commented Mar 1, 2023 •

edited

jingxu97 commented Mar 2, 2023 •

edited

cvvz Mar 8, 2023 •

edited

cvvz commented Mar 8, 2023 •

edited

andyzhangx commented Mar 13, 2023 •

edited

humblec commented Apr 14, 2023 •

edited