Skip to content

fix(storages): use the CSI volume handle as NodePublishVolume volume_id - #783

Open
AnshulPatil2005 wants to merge 1 commit into
openkruise:masterfrom
AnshulPatil2005:fix/csi-deterministic-volume-id
Open

AnshulPatil2005 wants to merge 1 commit into
openkruise:masterfrom
AnshulPatil2005:fix/csi-deterministic-volume-id

Conversation

@AnshulPatil2005

Copy link
Copy Markdown

I. Describe what this PR does

Every publish built volume_id as the PV name plus six random characters, against the same target path:

VolumeId:   fmt.Sprintf("%v-%s", persistentVolumeObj.Name, generateRandomString(6)),
TargetPath: containerMountTarget,

The CSI spec makes NodePublishVolume idempotent on (volume_id, target_path), so a volume_id that changes per call defeats it and the driver sees each retry as a new publish. Nothing in the tree calls NodeUnpublishVolume, so the previous mount is never cleaned up. For FUSE drivers like JuiceFS and Ceph that forks a mount helper per publish, which is the stacking @zhulinwei describes on the issue.

Uses Spec.CSI.VolumeHandle instead. That identifies the volume rather than the call, and it is what kubelet sends for the same PV, so a driver can recognise a repeat. A PV with no handle is now rejected rather than publishing an empty volume_id.

generateRandomString had no other caller so it goes too.

This is only the volume_id half of the issue. The re-mount running outside the guard in EnsureSandboxResumed, and the missing unmount, are separate and I have left them alone.

II. Does this pull request fix one issue?

Refs #408

III. Describe how to verify it

go test ./pkg/agent-runtime/storages/ -count=1

TestMountProvider_GenerateNodePublishVolumeRequest_Idempotency already existed but asserted TargetPath, Readonly, VolumeContext, Secrets and VolumeCapability while skipping volume_id, which was the only field that was not idempotent. It now checks it and fails on master without this change.

IV. Special notes for reviews

Worth a check from someone with a real driver: kubelet passes the volume handle as volume_id, so this should match what drivers already expect, but the mounts here are issued by sandbox-runtime-storage rather than kubelet and I could only test it against the fakes in tree.

TestCreateSymlinkBranches and TestSystemMountReaderReadMountsWithFixture fail for me on both this branch and a clean master. They look like Windows-only failures in symlink creation and /proc mount parsing, unrelated to this.

@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign airanthem for approval by writing /assign @airanthem in a comment. For more information see:The Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.08%. Comparing base (1781b7c) to head (a5bd862).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #783   +/-   ##
=======================================
  Coverage   83.08%   83.08%           
=======================================
  Files         259      259           
  Lines       22555    22551    -4     
=======================================
- Hits        18739    18736    -3     
+ Misses       3093     3092    -1     
  Partials      723      723           
Flag Coverage Δ
unittests 83.08% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AnshulPatil2005
AnshulPatil2005 force-pushed the fix/csi-deterministic-volume-id branch from cd5f153 to 335e3e6 Compare August 13, 2026 11:05
@AnshulPatil2005
AnshulPatil2005 force-pushed the fix/csi-deterministic-volume-id branch from 335e3e6 to 155f070 Compare September 4, 2026 04:27
Every publish built a volume_id from the PV name plus six random characters
against the same target path. The CSI spec makes NodePublishVolume idempotent
on (volume_id, target_path), so a volume_id that changes per call defeats it
and a driver sees each retry as a new publish. Nothing calls
NodeUnpublishVolume, so those mounts are never cleaned up. FUSE drivers such
as JuiceFS and Ceph fork a mount helper per publish, so a retried mount stacks
bind mounts and leaks helpers.

Use Spec.CSI.VolumeHandle, which identifies the volume and is what kubelet
sends for the same PV, and reject a PV that has none.

The existing idempotency test asserted every field except volume_id; it now
covers it and fails without this change. generateRandomString had no other
caller and goes with it.

Refs openkruise#408

Signed-off-by: Anshul <anshulpatil1022@gmail.com>
@AnshulPatil2005
AnshulPatil2005 force-pushed the fix/csi-deterministic-volume-id branch from 155f070 to a5bd862 Compare September 5, 2026 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants