capacity: fix duplicate topology (attempt #2) by huww98 · Pull Request #1450 · kubernetes-csi/external-provisioner

huww98 · 2025-11-26T15:14:19Z

When the controller starts, 2 sync() call will run simultaneously, one from HasSynced(), another from processNextWorkItem(). Each will produce an instance for the same topology segment, and pass it to callbacks.

This will result in duplicated entries in capacities map, resulting in: either

Two CSIStorageCapacity object get created for the same topology, or
The same CSIStorageCapacity object get assigned to two keys in capacities map. When one of them is updated, the other one will hold an outdated object and all subsequent update will fail with conflict.

What type of PR is this?

/kind bug

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Please see also #1435

Does this PR introduce a user-facing change?:

Fixed possible duplicated CSIStorageCapacity and constantly failing update request.

/cc @pohly

huww98 · 2025-11-26T15:25:41Z

+func TestHasSynced(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		client := fakeclientset.NewSimpleClientset()
+		informerFactory := informers.NewSharedInformerFactory(client, 1*time.Hour)


For why not setting resync period to 0, see kubernetes/kubernetes#133500

I don't see the connection to kubernetes/kubernetes#133500. Can you explain here why a resync period of one hour is useful?

I'm guessing it's because without kubernetes/kubernetes#133500, a zero resync period would cause reads from neverExitWatch, which isn't valid in a synctest. But it would be nice to explicitly state that.

Now we have Kubernetes v0.35 included in the go.mod. We actually can set resync period to 0. So no need special comment now.

huww98 · 2025-11-26T15:32:48Z

+	go func() {
+		<-ctx.Done()
+		nt.queue.ShutDown()
+	}()


synctest checks for leaked goroutines. So I have to clean it up.

huww98 · 2025-11-26T16:19:12Z

We need go 1.25 to use the synctest package :(

k8s-triage-robot · 2026-03-15T11:50:33Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2026-04-14T12:18:49Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

pohly · 2026-04-17T08:40:33Z

/remove-lifecycle rotten

@huww98: shall we finish this PR?

See:

huww98 · 2026-04-17T08:45:46Z

Yes, I was waiting for go 1.25 update for synctest. Now we have it, let me rebase and resolve the comments.

When the controller starts, 2 sync() call will run simultaneously, one from HasSynced(), another from processNextWorkItem(). Each will produce an instance for the same topology segment, and pass it to callbacks. This will result in duplicated entries in capacities map, resulting in: either - Two CSIStorageCapacity object get created for the same topology, or - The same CSIStorageCapacity object get assigned to two keys in capacities map. When one of them is updated, the other one will hold an outdated object and all subsequent update will fail with conflict.

pohly · 2026-04-21T12:27:11Z

 			// wait for sync.
 			factoryForNamespace.Start(ctx.Done())
 		}
+		if topologyInformer != nil {


Why is important that topologyInformer gets started here vs. where it was started before?

If it's important, then let's add a comment explainining why. If it's not important, then let's not change it.

All other informers are started here. It is nature to start it here.
I think we should only start topologyInformer after factoryForNamespace.Start, to avoid cache.WaitForCacheSync waiting for not-started informers. Wasting a little CPU when waiting for leader election.

There is already a comment just before:

// Starting is enough, the capacityController and topologyInformer will // wait for sync.

Do you think this is enough?

I think both approaches would be fine and in general I prefer to avoid drive-by enhancements that aren't related to what a PR is primarily trying to do (in this case "fix duplicate topology"). It causes churn and makes reviews harder.

But we can keep it.

pohly

/lgtm
/approve

pohly · 2026-04-22T09:11:10Z

 			// wait for sync.
 			factoryForNamespace.Start(ctx.Done())
 		}
+		if topologyInformer != nil {


I think both approaches would be fine and in general I prefer to avoid drive-by enhancements that aren't related to what a PR is primarily trying to do (in this case "fix duplicate topology"). It causes churn and makes reviews harder.

But we can keep it.

k8s-ci-robot · 2026-04-22T09:11:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huww98, pohly

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [pohly]

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 requested a review from pohly November 26, 2025 15:14

huww98 mentioned this pull request Nov 26, 2025

capacity: fix duplicate topology #1435

Closed

huww98 commented Nov 26, 2025

View reviewed changes

pohly reviewed Nov 26, 2025

View reviewed changes

Comment thread cmd/csi-provisioner/csi-provisioner.go

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 15, 2026

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 14, 2026

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 17, 2026

huww98 force-pushed the fix-duplicate-capacity-2 branch from 1b66f2e to 5f11dc4 Compare April 17, 2026 09:14

pohly reviewed Apr 21, 2026

View reviewed changes

pohly reviewed Apr 22, 2026

View reviewed changes

k8s-ci-robot assigned pohly Apr 22, 2026

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

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

k8s-ci-robot merged commit 2311410 into kubernetes-csi:master Apr 22, 2026
8 checks passed

Conversation

huww98 commented Nov 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huww98 commented Nov 26, 2025

Uh oh!

Uh oh!

k8s-triage-robot commented Mar 15, 2026

Uh oh!

k8s-triage-robot commented Apr 14, 2026

Uh oh!

pohly commented Apr 17, 2026

Uh oh!

huww98 commented Apr 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huww98 Apr 22, 2026 • 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!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huww98 Apr 22, 2026 •

edited

Loading