ROX-30138: Add tests for WithSelector #496

kurlov · 2025-10-20T11:55:42Z

Follow up for #459

Add test for WithSelector using controller manager to verify the reconciler only processes CRs that match the configured selector.

codecov-commenter · 2025-10-23T08:18:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.71%. Comparing base (08ab7fb) to head (9149213).
⚠️ Report is 125 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (9149213). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (08ab7fb) HEAD (9149213)

2 1

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #496       +/-   ##
===========================================
- Coverage   85.06%   74.71%   -10.35%     
===========================================
  Files          19       31       +12     
  Lines        1346     2037      +691     
===========================================
+ Hits         1145     1522      +377     
- Misses        125      433      +308     
- Partials       76       82        +6

☔ View full report in Codecov by Sentry.
📢 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.

porridge

This is much better than the previous one, and it will actually fail when we accidentally stop passing the predicate, so it's useful.

However, I'm afraid that this test passing does not guarantee much. AFAICT there is no way to be sure whether the controller will never schedule the unwanted CR for reconciliation, or if it just did not get round to doing that yet. 🤔

Perhaps we can improve our confidence a bit by creating the unlabeled CR before the labeled one?

pkg/reconciler/reconciler_test.go

porridge · 2025-11-04T09:31:26Z

pkg/reconciler/reconciler_test.go

+			controllerName := fmt.Sprintf("%v-controller", strings.ToLower(gvk.Kind))
+			Expect(r.addDefaults(mgr, controllerName)).To(Succeed())
+			r.setupScheme(mgr)
+
+			c, err := controller.New(controllerName, mgr, controller.Options{
+				Reconciler:              reconciler,
+				MaxConcurrentReconciles: 1,
+			})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(r.setupWatches(mgr, c)).To(Succeed())


This duplicates a bunch of code from SetupWithManager. I don't think we should be doing this, it makes the test brittle. I guess the reason is that you want to append to reconciledCRs? Perhaps consider using a hook for this instead?

it's a great idea. I changed the test to use hooks

pkg/reconciler/reconciler_test.go

porridge · 2025-11-04T09:33:07Z

pkg/reconciler/reconciler_test.go

+				Eventually(func() []string {
+					reconciledCRsMutex.Lock()
+					defer reconciledCRsMutex.Unlock()
+					return reconciledCRs


I think that since this returns a slice pointing at the same backing array, the caller will race with other accesses 🤔
Perhaps instead check the existence of desired element in the Eventually, in the scope of the lock, and then make Should just check for true return value?

I might not understand the comment correctly but there is mutex in write/read to prevent data race. And the Eventually() matches synchronously after the function returns but before the mutex is unlocked. Also tests run with -race flag and during the development I had a few data race issues but all of them resolved. So I believe it should be fine.

In addition the using of slice/map is more accurate (not perfect though) than bool var because bool in Eventually() will match even when reconciler reconciled two different label selectors

the Eventually() matches synchronously after the function returns but before the mutex is unlocked.

I don't think this is true. Mutex is released when the func completes, and then the Eventually works on a copy of the returned slice with the same backing array as the mutex-protected slice. This program shows the data corruption that can happen. After running it a few times I got it to produce:

I don't think I get the point behind slice vs bool.. you can have the bool mean anything you need inside the func...

I think I got your point and moved assertions inside Eventually() and just check the predicate with .Should(BeTrue())

Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>

kurlov · 2025-11-27T07:45:08Z

pkg/reconciler/reconciler.go

 	var preds []predicate.Predicate

-	if r.labelSelector.Size() > 0 {
+	if len(r.labelSelector.MatchLabels) > 0 || len(r.labelSelector.MatchExpressions) > 0 {


Minor thing, I guess it's a bit better instead of pb generated method

Better in what sense? I don't think computational performance is important in this (initialization) phase. The goal of using Size() was to make it robust against addition of new fields to the metav1.LabelSelector size.

I thought that Size() will be > 0 for empty LabelSelector{} struct but it's not the case. Changed back to Size()

kurlov · 2025-11-27T07:46:03Z

pkg/reconciler/reconciler_test.go

+			})
+		})
+
+		It("should reconcile CRs independently when using two managers with different label selectors", func() {


Also added a test with two managers to simulate the original bug when two managers with different label selectors reconciles each other CRs

porridge · 2025-11-27T10:09:17Z

pkg/reconciler/reconciler.go

 	var preds []predicate.Predicate

-	if r.labelSelector.Size() > 0 {
+	if len(r.labelSelector.MatchLabels) > 0 || len(r.labelSelector.MatchExpressions) > 0 {


Better in what sense? I don't think computational performance is important in this (initialization) phase. The goal of using Size() was to make it robust against addition of new fields to the metav1.LabelSelector size.

porridge · 2025-11-27T10:44:35Z

pkg/reconciler/reconciler_test.go

+				Eventually(func() []string {
+					reconciledCRsMutex.Lock()
+					defer reconciledCRsMutex.Unlock()
+					return reconciledCRs


the Eventually() matches synchronously after the function returns but before the mutex is unlocked.

I don't think this is true. Mutex is released when the func completes, and then the Eventually works on a copy of the returned slice with the same backing array as the mutex-protected slice. This program shows the data corruption that can happen. After running it a few times I got it to produce:

I don't think I get the point behind slice vs bool.. you can have the bool mean anything you need inside the func...

porridge · 2025-11-27T10:56:18Z

pkg/reconciler/reconciler_test.go

+					}
+					return nil
+				})
+				_ = setupManagerWithSelectorAndPostHook(ctx, postHook, anotherMatchingLabels)


I'm wondering... now that we have all these managers and reconcilers, how do we make sure they do not affect other Describes? Or that we don't get affected by other ones? I cannot see them being stopped directly anywhere. Do they run indefinitely? 🤔

I added managers shut down. Also after experiencing a few issues with cache/leftover CRs I decided to not share CRs between tests and even name them differently (there is CR cleanup but still it's really easy to get not obvious issues). It is a few repeated lines in the tests but I believe it is worth it. It will make life a bit easier for person who will touch these tests in the future.

porridge

👍🏻

perdasilva · 2025-12-04T09:25:10Z

pkg/reconciler/reconciler_test.go

+			})
+
+			By("verifying only the labeled CR was reconciled", func() {
+				Eventually(func() bool {


is it possible that the non-labeled object gets reconciled straight after and we don't catch it?

Yes, unfortunately there does not seem to be a way to know whether controller-runtime will never do something, or just didn't get round to doing it yet :-(

It might be worth calling this out in a comment somewhere. In case we hit a flake, we can get the context.

perdasilva · 2025-12-05T09:49:38Z

pkg/reconciler/reconciler_test.go

+			mgr, done := setupManagerWithSelectorAndPostHook(ctx, postHook, matchingLabels)
+			doneChans = append(doneChans, done)
+
+			postHook2 := makePostHook(&mu, &anotherReconciledCRs)


we're using the same mutex across the two managers. Could this synchronization cause problems? would it be better to use different mutexes?

Add tests for WithSelector

24bd217

openshift-ci bot requested review from SimonBaeumer and everettraven October 20, 2025 11:55

github-actions bot added area/sdk area/testing labels Oct 20, 2025

Fix lint

25d253a

Small fix

d486710

porridge suggested changes Nov 4, 2025

View reviewed changes

kurlov and others added 5 commits November 4, 2025 15:19

Apply suggestions from code review

23cd0b6

Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>

Fix indentation

0a43969

Use hooks and add another test with two managers

072866a

Fix linting

25d4810

Improve assertion

ae66a85

kurlov commented Nov 27, 2025

View reviewed changes

kurlov requested a review from porridge November 27, 2025 07:49

porridge reviewed Nov 27, 2025

View reviewed changes

kurlov added 2 commits December 1, 2025 10:34

Add managers cleanup and change Eventually asserting for mutex objects

bd631b6

return protobuf Size() method for label setup check

9149213

kurlov requested a review from porridge December 1, 2025 09:44

porridge approved these changes Dec 1, 2025

View reviewed changes

perdasilva reviewed Dec 4, 2025

View reviewed changes

grokspawn mentioned this pull request Dec 4, 2025

📖 Clarify single controller assumption in OLM v1 design operator-framework/operator-controller#2372

Merged

4 tasks

perdasilva reviewed Dec 5, 2025

View reviewed changes

ROX-30138: Add tests for WithSelector #496

Are you sure you want to change the base?

ROX-30138: Add tests for WithSelector #496

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov-commenter commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

kurlov Dec 1, 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!

kurlov Dec 1, 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!

porridge left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kurlov commented Oct 20, 2025 •

edited

Loading

codecov-commenter commented Oct 23, 2025 •

edited

Loading

kurlov Dec 1, 2025 •

edited

Loading

kurlov Dec 1, 2025 •

edited

Loading