Fix controller setup in storage version unit testcases #134469

vikasbolla · 2025-10-08T09:19:14Z

What type of PR is this?

/kind flake

What this PR does / why we need it:

This PR tries to fix the test flakes
There is an issue during the controller setup in the test file. Controller depends on the informer cache and in this case I can see that the controller call is not controlled based on the informer cache sync, due to which, when the controller is called even before the informer execution, then the worker queue on which the controller is depended on will be empty and the reconciliation logic will timeout.

So added the cache check before calling the controller to make sure the informer cache is properly updated with latest objects state.

Which issue(s) this PR is related to:

Fixes #131745

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

linux-foundation-easycla · 2025-10-08T09:19:21Z

The committers listed above are authorized under a signed CLA.

✅ login: vikasbolla / name: Vikas Bolla (caea56f)

k8s-ci-robot · 2025-10-08T09:19:23Z

Welcome @vikasbolla!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-10-08T09:19:24Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2025-10-08T09:19:24Z

Hi @vikasbolla. 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-sigs/prow repository.

k8s-ci-robot · 2025-10-08T09:19:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vikasbolla
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. 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:

pkg/controller/OWNERS

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

Karthik-K-N · 2025-10-08T09:47:13Z

/ok-to-test

Jefftree · 2025-10-08T14:29:46Z

pkg/controller/storageversiongc/gc_controller_test.go

 	controller := NewStorageVersionGC(ctx, clientset, leaseInformer, storageVersionInformer)
+	informerFactory.Start(ctx.Done())
+	// Using this ensure informer caches are fully populated before starting the controller.
+	if !cache.WaitForCacheSync(ctx.Done(), controller.leasesSynced, controller.storageVersionSynced) {


Don't think this is strictly necessary. We wait for the sync in controller.Run() already. https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/storageversiongc/gc_controller.go#L103-L106

+1 to polling instead of checking condition once though.

Thanks @Jefftree, but controller.Run() is sometimes called even before the informer cache is synced with the object state as controller.Run() is a goroutine, so in that case I think the flakiness is observed

The PollWithContextTimeout addition should catch those flakes though? Overall I don't have a strong opinion so will defer to the approvers.

actually I have tried only with polling and saw that it alone does not solve issue, when I future investigated got to know that since informer cache never get updates and controller never knows the lease deletion, reconciliation never happens and polling eventually time out

Jefftree · 2025-10-08T14:41:17Z

pkg/controller/storageversiongc/gc_controller_test.go

 		},
 	}
+	// Wait up to 5 seconds, checking every 100ms to ensure controller had a chance to reconcile
+	err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (bool, error) {


imo it seems to make more sense to wrap the entire reflection check from L126-148 within the wait.PollUntilContextTimeout. Feels a bit weird that we're just checking the CommonEncodingVersion

Thanks @Jefftree , made the changes, PTAL

Jefftree · 2025-10-08T16:55:36Z

/lgtm

Thanks!

k8s-ci-robot · 2025-10-08T16:55:45Z

LGTM label has been added.

Git tree hash: 04d9c8ae9ffb9bec71ba0d1c6880a2321147f0ad

vikasbolla · 2025-10-09T03:24:34Z

/retest

k8s-ci-robot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 8, 2025

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 8, 2025

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 8, 2025

k8s-ci-robot requested review from mortent and soltysh October 8, 2025 09:19

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 8, 2025

github-project-automation bot added this to SIG Apps Oct 8, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Oct 8, 2025

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 8, 2025

vikasbolla marked this pull request as ready for review October 8, 2025 09:44

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2025

k8s-ci-robot requested a review from smarterclayton October 8, 2025 09:44

Karthik-K-N mentioned this pull request Oct 8, 2025

Fix storage version test flake #134432

Open

Jefftree reviewed Oct 8, 2025

View reviewed changes

Fix controller setup in storage version unit testcases

caea56f

vikasbolla force-pushed the fix_131745_UT branch from e5918f3 to caea56f Compare October 8, 2025 16:43

k8s-ci-robot assigned Jefftree Oct 8, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2025

Fix controller setup in storage version unit testcases #134469

Are you sure you want to change the base?

Fix controller setup in storage version unit testcases #134469

Conversation

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

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

linux-foundation-easycla bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

Karthik-K-N commented Oct 8, 2025

Uh oh!

Jefftree Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

vikasbolla Oct 8, 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!

Jefftree Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

vikasbolla Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Jefftree Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

vikasbolla Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Jefftree commented Oct 8, 2025

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

vikasbolla commented Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vikasbolla commented Oct 8, 2025 •

edited

Loading

linux-foundation-easycla bot commented Oct 8, 2025 •

edited

Loading

vikasbolla Oct 8, 2025 •

edited

Loading