Add In Memory Cache of Node/CSINode Info for Long Provisioning to Prevent Volume Leak #1413

sunnylovestiramisu · 2025-09-08T20:42:28Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This is the option 4 discussed in Leaked Volume Fix in external-provisioner Google doc.

The short term fix does not solve the issue of csi-provisioner restarting during the long provisioning time, however it is still an improvement. Also the short term fix does not have conflict with other long term fixes if we want to revisit in the future.

Which issue(s) this PR fixes:

kubernetes-sigs/sig-storage-lib-external-provisioner#152

Special notes for your reviewer:
Making changes in vendor/sigs.k8s.io/sig-storage-lib-external-provisioner/v11 to get initial review, I will move the change to sig-storage-lib-external-provisioner and bump up the sig-storage-lib-external-provisioner major version once this review is in good shape.

Tested via the following steps.

kubetest --build --up from a k/k repo
Add wait time to PDCSI driver similar to this branch
Make the PDCSI driver consuming the external-provisioner change in this PR and deploy PDCSI driver
Find the node of the csi-gce-pd-controller, for example

k get pod -n gce-pd-csi-driver 
NAME                                    READY   STATUS             RESTARTS        AGE
csi-gce-pd-controller-8dffb78ff-2xzbq   5/5     Running            0               27m
csi-gce-pd-node-54b8l                   2/2     Running            0               27m
csi-gce-pd-node-5fcwm                   2/2     Running            0               27m
csi-gce-pd-node-7g72v                   0/2     CrashLoopBackOff   15 (100s ago)   27m

csi-gce-pd-controller-8dffb78ff-2xzbq is on node e2e-test-songsunny-minion-group-kncr, for the following testing pod, specify a nodeSelector that is NOT on node e2e-test-songsunny-minion-group-kncr
5. Apply the following objects:

kubectl apply -f - <<EOF
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: pd-storage
provisioner: pd.csi.storage.gke.io
volumeBindingMode: WaitForFirstConsumer
allowVolumeExpansion: true
parameters:
  type: pd-balanced
EOF

kubectl apply -f - <<EOF
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: pd-pvc
spec:
  storageClassName: pd-storage
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 67Gi
EOF

kubectl apply -f - <<EOF
apiVersion: v1
kind: Pod
metadata:
  name: test-pod
spec:
  containers:
  - name: nginx
    image: nginx
    volumeMounts:
    - name: pd-volume
      mountPath: /data/pd
  nodeSelector:
    kubernetes.io/hostname: e2e-test-songsunny-minion-group-302x
  volumes:
  - name: pd-volume
    persistentVolumeClaim:
      claimName: pd-pvc
      readOnly: false
EOF

Delete the node e2e-test-songsunny-minion-group-302x, and then the Pod
Make sure the pv object can still be created even node is deleted

Does this PR introduce a user-facing change?:

Add In Memory Cache of Node/CSINode Info for Long Provisioning to Prevent Volume Leak

sunnylovestiramisu · 2025-09-08T20:44:43Z

/assign @msau42

sunnylovestiramisu · 2025-09-08T23:09:54Z

vendor/sigs.k8s.io/sig-storage-lib-external-provisioner/v11/controller/controller.go

@@ -1486,31 +1486,19 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
 		return ProvisioningFinished, errStopProvision
 	}

-	var selectedNode *v1.Node
+	var selectedNodeName string


@jsafrane Should we just make a new major version instead of making it an option for other provisioners?

This is a breaking change, so yes, it will require a new major version.
I don't see where you make it an option, this PR looks very backward incompatible. And I am not against.

The alternative, if we think non-csi provisioners still want the old behavior, is to add the new changes as a feature gate. I'm not aware of any non-csi provisioners that support topology though and they would still run into the topology bug if we keep the old way.

So I am also in favor of just making a new major.

pkg/controller/controller.go

pkg/controller/cache.go

msau42 · 2025-09-09T00:34:24Z

cmd/csi-provisioner/csi-provisioner.go

@@ -417,7 +417,7 @@ func main() {
 	if supportsMigrationFromInTreePluginName != "" {
 		provisionerOptions = append(provisionerOptions, controller.AdditionalProvisionerNames([]string{supportsMigrationFromInTreePluginName}))
 	}
-
+	pvcNodeStore := ctrl.NewInMemoryStore()


If the driver doesn't support topology, then I think the cache is not needed.

So we need to add Topology check for all the cache code.

pkg/controller/cache.go

msau42 · 2025-09-09T00:37:17Z

pkg/controller/cache.go

+// a TopologyInfo object by its name.
+// The name is pvcNamespace/pvcName
+type TopologyProvider interface {
+	GetByName(name string) (*TopologyInfo, error)


To make it very clear what the key is, what if we make it v1.PersistentVolumeClaim* instead of a string?

What happens if PVC is deleted? Also currently we only pass in pvcName and pvcNamespace in the GenerateAccessibilityRequirements. Can changing to a pointer to PVC cause problems?

pkg/controller/cache.go

msau42 · 2025-09-09T00:42:38Z

pkg/controller/cache.go

+	// First, check if the key exists to provide a helpful error.
+	_, found := s.data[name]
+	if !found {
+		return fmt.Errorf("cannot delete: object with name '%s' not found", name)


Do we want to error if it doesn't exist? I don't think the caller will do anything other than ignore the error. If you do want to return an error, I would make an explicit error type for NotFoundError, so that the caller can distinguish it from other errors.

+1 for returning no error.

Do we want to just log a warning and not returning error?

msau42 · 2025-09-09T00:58:54Z

pkg/controller/topology.go

+		return nil, err
+	}
+	if len(cacheInfo.TopologyKeys) == 0 {
+		return nil, fmt.Errorf("no topology key found in the CSINode in memory cache %s", nodeName)


Do we need a special error message just for no keys/label from cache? We already have a warning message that we're going to lookup the cache.

We do not need the special error message but we should return an error. Maybe errors.New("")?

msau42 · 2025-09-09T01:07:58Z

pkg/controller/topology.go

-	if selectedNode != nil {
-		selectedCSINode, err = getSelectedCSINode(csiNodeLister, selectedNode)
+	var topologyKeys []string
+	if len(selectedNodeName) > 0 {


This logic is very hard to follow with the cache lookups and updates being interspersed with condition checks. I think it would be easier to read if we wrapped the informer/cache lookup logic in one call, and kept the core GenerateAccessibility function clean with just business logic. Something like:

func topologyLookup(selectedNodeName) { csiNode, err1 := getSelectedCSINode(csiNodeLister, selectedNodeName) node,err2 := getNode(nodeLister, selectedNodeName) if csiNode == nil || node == nil { topologyInfo = cache.getTopologyInfo(pvc) } else { topologyInfo = { getTopologyKeys(csiNode), getNodeLabels(node, topologyKeys)} cache.addToplogyInfo(topologyInfo) } return topologyInfo }

We can also consider looking up cache first, then if it doesn't exist in cache, then lookup in informers. I think switching that logic will solve the immediate binding scenario I outlined in #1413 (comment)

I would prefer checking the informers first, the node might have gotten new labels as the CSI driver just got installed there.

msau42 · 2025-09-09T01:20:20Z

vendor/sigs.k8s.io/sig-storage-lib-external-provisioner/v11/controller/controller.go

-		}
+		selectedNodeName = nodeName
+	} else {
+		logger.Info("No selected node annotation")


This will spam for drivers that don't support topology or use Immediate binding

Okay so we just do not log anything

msau42 · 2025-09-09T01:23:26Z

pkg/controller/topology.go

+			return term, node.Labels, false
+		}
+	} else {
+		// nodeLister is nil, read from the cache directly


Is this possible? I think nodeLister is required.

The original logic in the lib is:

if ctrl.nodeLister != nil { selectedNode, err = ctrl.nodeLister.Get(nodeName) } else { selectedNode, err = ctrl.client.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) // TODO (verult) cache Nodes }

So I was thinking nodeLister can be nil? I am not sure when will it occur though.

In the csi-provisioner it is mandatory as long as the plugin supports topology: https://github.com/kubernetes-csi/external-provisioner/blob/master/cmd/csi-provisioner/csi-provisioner.go#L339

Okay then we can remove the nodeLister nil check

msau42 · 2025-09-09T01:25:35Z

pkg/controller/topology.go

+			klog.Warningf("no node labels for node %s found in memory cache", nodeName)
+			return nil, nil, true
+		}
+		for _, key := range topologyKeys {


This logic looks repeated 3 times in this function. It would be better to share it across all 3 branches.

Removed one for the nodeLister is nil code. However the rest two, the first one read nodeLabels from Cache, the other one read nodeLabels from the node. I think we can make them to a processTerm func.

msau42 · 2025-09-09T01:46:12Z

For your e2e test case steps, can you also test deleting the PVC object along with the Pod and Node to trigger the leak? I guess the leak is triggered by 2 scenarios:

PVC is deleted along with Pod and Node
PVC is not deleted, but Pod and Node are deleted. AND the scheduler retries in a different zone.

Can you verify you can repro the issue with both those scenarios, and that the fix works in both those scenarios?

I'm trying to think if AllowedTopologies or Immediate binding mode would be impacted. AllowedTopologies shouldn't be impacted because we grab the topology directly from it. I think Immediate binding could be impacted in a scenario like:

Start provisioning in zone A.
During provisioning, scale down all the Nodes in zone A.
Future retries will either error if no zones are found, or it will retry with a zone B, leaking the volume being provisioned in zone A.

msau42 · 2025-09-09T01:52:05Z

Also in the testing steps, with the fix, verify that the PV object is created and then deleted when the disk gets deleted.

jsafrane · 2025-09-09T11:12:56Z

pkg/controller/cache.go

+
+// TopologyProvider is an interface that defines the behavior for looking up
+// a TopologyInfo object by its name.
+// The name is pvcNamespace/pvcName


IMHO the key should be PVC UID.

Consider this scenario:

User creates a PVC

The provisioning takes a long time, i.e. many timeouts + retries.

User deletes the PVC and creates a new one, with the same name, but different StorageClass.

The original PVC is still being provisioned. Despite it has been deleted from the API server, the provisioner waits for a final error.

The new PVC should be provisioned too.
-> Two PVCs have the same name and are being provisioned, but they have a different StorageClass and thus different topology cache.

PVC UID will be unique for these PVCs.

And PVC UID guarantees uniqueness as well.

jsafrane · 2025-09-09T11:15:55Z

pkg/controller/cache.go

+	// First, check if the key exists to provide a helpful error.
+	_, found := s.data[name]
+	if !found {
+		return fmt.Errorf("cannot delete: object with name '%s' not found", name)


+1 for returning no error.

jsafrane · 2025-09-09T11:19:53Z

vendor/sigs.k8s.io/sig-storage-lib-external-provisioner/v11/controller/controller.go

@@ -1486,31 +1486,19 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
 		return ProvisioningFinished, errStopProvision
 	}

-	var selectedNode *v1.Node
+	var selectedNodeName string


This is a breaking change, so yes, it will require a new major version.
I don't see where you make it an option, this PR looks very backward incompatible. And I am not against.

jsafrane · 2025-09-09T11:29:22Z

pkg/controller/controller.go

 			mayReschedule,
 			state,
 			err)
+		if IsFinalError(err) {


If I understand it correctly, as long as the PVC exists and the provisioner lib calls Provision(), the external-provisioner re-loads the topology from the API server, adds it to the cache and deletes it from the cache on every final error.

It's actually a smart idea that simplifies the cache cleanup. It's just not very obvious from the cache name and its comments. I somehow expected it caches the topology forever and deletes it after PVC is deleted. A comment on the right place (the struct definition?) would help.

jsafrane · 2025-09-09T11:35:06Z

pkg/controller/topology.go

-	if selectedNode != nil {
-		selectedCSINode, err = getSelectedCSINode(csiNodeLister, selectedNode)
+	var topologyKeys []string
+	if len(selectedNodeName) > 0 {


I would prefer checking the informers first, the node might have gotten new labels as the CSI driver just got installed there.

k8s-ci-robot · 2025-09-09T22:11:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sunnylovestiramisu
Once this PR has been reviewed and has the lgtm label, please ask for approval from msau42. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2025-09-09T22:40:48Z

@sunnylovestiramisu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-csi-external-provisioner-unit	`98536bb`	link	true	`/test pull-kubernetes-csi-external-provisioner-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

msau42 · 2025-09-10T00:56:21Z

pkg/controller/cache.go

@@ -50,22 +51,24 @@ func (s *InMemoryStore) Add(name string, info *TopologyInfo) {

 // Delete implements the TopologyProvider interface.
 // It uses the built-in delete() function to remove the item from the map.
+// The entry is deleted when provision succeeds or returns a final error.


This is tue regardless of implementation. I would probably put it as a comment in the interface

msau42 · 2025-09-10T00:57:32Z

pkg/controller/controller.go

@@ -692,7 +692,7 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
 		requirements, err := GenerateAccessibilityRequirements(
 			p.client,
 			p.driverName,
-			claim.Namespace,
+			string(claim.UID),


do we need to convert the key to a string? Can we use the direct type as the key?

msau42 · 2025-09-10T01:09:10Z

pkg/controller/topology.go

@@ -105,6 +106,78 @@ func SupportsTopology(pluginCapabilities rpc.PluginCapabilitySet) bool {
 		utilfeature.DefaultFeatureGate.Enabled(features.Topology)
 }

+func topologyLookup(


This is not quite what I meant about the wrapper function.

I think it would be simpler to understand the code if things like obj lookup (whether from informer or cache) is abstracted from business logic (how to behave when there are no keys, or strictTopology mode is set, etc).

So I think a better structure for GenerateAccessiblityRequirements would be something like:

func GenerateAccessibilityRequirements(...) (...) { // This returns the information from informer or cache, the caller doesn't care topologyInfo = topologyLookup(selectedNode) // Nothing in the logic after this should care if the information came from the cache or informer if len(topologyKeys) == 0) { // business logic to handle it } if strictTopology { // business logic to handle it } // everything else }

sunnylovestiramisu added 3 commits September 8, 2025 20:23

Update sig-storage-lib-external-provisioner

6d5c359

Add In Memory Cache for PVC and Node/CSINode Info

9c6bb48

Refactor

e870615

sunnylovestiramisu requested a review from jsafrane September 8, 2025 20:42

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Sep 8, 2025

k8s-ci-robot requested review from gnufied and xing-yang September 8, 2025 20:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 8, 2025

Remove logging

4fdda4f

sunnylovestiramisu force-pushed the leakedVolumeCache branch from 8e951c6 to 4fdda4f Compare September 8, 2025 20:44

k8s-ci-robot assigned msau42 Sep 8, 2025

sunnylovestiramisu commented Sep 8, 2025

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

sunnylovestiramisu commented Sep 8, 2025

View reviewed changes

pkg/controller/cache.go Outdated Show resolved Hide resolved

msau42 reviewed Sep 9, 2025

View reviewed changes

jsafrane reviewed Sep 9, 2025

View reviewed changes

Update cache.go based on comments

98536bb

sunnylovestiramisu force-pushed the leakedVolumeCache branch from abab6ad to 98536bb Compare September 9, 2025 22:13

msau42 reviewed Sep 10, 2025

View reviewed changes

Add In Memory Cache of Node/CSINode Info for Long Provisioning to Prevent Volume Leak #1413

Are you sure you want to change the base?

Add In Memory Cache of Node/CSINode Info for Long Provisioning to Prevent Volume Leak #1413

Uh oh!

Conversation

sunnylovestiramisu commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunnylovestiramisu commented Sep 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunnylovestiramisu Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msau42 Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msau42 commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msau42 commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunnylovestiramisu commented Sep 8, 2025 •

edited

Loading

sunnylovestiramisu Sep 9, 2025 •

edited

Loading

msau42 Sep 9, 2025 •

edited

Loading

msau42 commented Sep 9, 2025 •

edited

Loading

msau42 commented Sep 9, 2025 •

edited

Loading