Skip to content
247 changes: 208 additions & 39 deletions pkg/reconciler/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import (
"context"
"errors"
"fmt"
"slices"
"strconv"
"sync"
"time"

. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -1492,45 +1494,6 @@ var _ = Describe("Reconciler", func() {
})
})
})
When("label selector set", func() {
It("reconcile only matching CR", func() {
By("adding selector to the reconciler", func() {
selectorFoo := metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}}
Expect(WithSelector(selectorFoo)(r)).To(Succeed())
})

By("adding not matching label to the CR", func() {
Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed())
obj.SetLabels(map[string]string{"app": "bar"})
Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed())
})

By("reconciling skipped and no actions for the release", func() {
res, err := r.Reconcile(ctx, req)
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).ToNot(HaveOccurred())
})

By("verifying the release has not changed", func() {
rel, err := ac.Get(obj.GetName())
Expect(err).ToNot(HaveOccurred())
Expect(rel).NotTo(BeNil())
Expect(*rel).To(Equal(*currentRelease))
})

By("adding matching label to the CR", func() {
Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed())
obj.SetLabels(map[string]string{"app": "foo"})
Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed())
})

By("successfully reconciling with correct labels", func() {
res, err := r.Reconcile(ctx, req)
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).ToNot(HaveOccurred())
})
})
})
})
})
})
Expand All @@ -1545,6 +1508,181 @@ var _ = Describe("Reconciler", func() {
})
})

_ = Describe("WithSelector", func() {
var (
ctx context.Context
cancel context.CancelFunc
matchingLabels map[string]string
anotherMatchingLabels map[string]string
mu sync.Mutex
doneChans []chan struct{}
)

BeforeEach(func() {
matchingLabels = map[string]string{"app": "foo"}
anotherMatchingLabels = map[string]string{"app": "bar"}
doneChans = nil
ctx, cancel = context.WithCancel(context.Background())

cleanupMgr := getManagerOrFail()
cleanupClient := cleanupMgr.GetClient()
crList := &unstructured.UnstructuredList{}
crList.SetGroupVersionKind(gvk)
Expect(cleanupClient.List(ctx, crList)).To(Succeed())

for i := range crList.Items {
cr := &crList.Items[i]
// Remove all finalizers to allow CR deletion
cr.SetFinalizers([]string{})
Expect(cleanupClient.Update(ctx, cr)).To(Succeed())
Expect(cleanupClient.Delete(ctx, cr)).To(Succeed())
}

// Wait for all CRs to be deleted
Eventually(func() bool {
crList := &unstructured.UnstructuredList{}
crList.SetGroupVersionKind(gvk)
if err := cleanupClient.List(ctx, crList); err != nil {
return false
}
return len(crList.Items) == 0
}, "10s", "100ms").Should(BeTrue())
})

AfterEach(func() {
// Cancel the context to stop all managers
if cancel != nil {
cancel()
}
// Wait for all managers to shut down completely
for _, done := range doneChans {
select {
case <-done:
// Manager has shut down
case <-time.After(5 * time.Second):
// Timeout waiting for manager shutdown
}
}
})

It("should only reconcile CRs matching the label selector", func() {
labeledObj := testutil.BuildTestCR(gvk)
labeledObj.SetName("labeled-cr-test1")
labeledObj.SetLabels(matchingLabels)
labeledObjKey := types.NamespacedName{Namespace: labeledObj.GetNamespace(), Name: labeledObj.GetName()}

anotherObj := testutil.BuildTestCR(gvk)
anotherObj.SetName("another-cr-test1")
anotherObjKey := types.NamespacedName{Namespace: anotherObj.GetNamespace(), Name: anotherObj.GetName()}

var reconciledCRs []string
postHook := makePostHook(&mu, &reconciledCRs)
mgr, done := setupManagerWithSelectorAndPostHook(ctx, postHook, matchingLabels)
doneChans = append(doneChans, done)

By("creating a CR without matching labels", func() {
Expect(mgr.GetClient().Create(ctx, anotherObj)).To(Succeed())
})

By("verifying that the labeled reconciler does not reconcile CR without labels", func() {
Consistently(func() bool {
mu.Lock()
defer mu.Unlock()
return len(reconciledCRs) == 0
}, "2s", "100ms").Should(BeTrue())
})

By("creating a CR with matching labels", func() {
Expect(mgr.GetClient().Create(ctx, labeledObj)).To(Succeed())
})

By("verifying only the labeled CR was reconciled", func() {
Eventually(func() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kurlov kurlov Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that non-labeled object gets reconciled. I saw the thread Marcin shared but in this test there is a labeled manager so even if non-labeled object gets a reconciliation attempt it will not be in the reconciledCRs slice because of the filtering by watchers.
@porridge would you agree with that or I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurlov I think @perdasilva 's question was about the fact that if there is a bug which will cause the reconcile for the non-labeled object to run after all, but with a delay of, say, 3 seconds, then this test will not be able to catch this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be fine to just add a comment to that effect and we move forward with the PR. I don't think it would be be highly likely to happen, and I'm struggling to think of another test strategy to avoid this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment

mu.Lock()
defer mu.Unlock()
return len(reconciledCRs) == 1 && reconciledCRs[0] == labeledObjKey.Name
}).Should(BeTrue())
})

By("updating the unlabeled CR to have matching labels", func() {
Expect(mgr.GetClient().Get(ctx, anotherObjKey, anotherObj)).To(Succeed())
anotherObj.SetLabels(matchingLabels)
Expect(mgr.GetClient().Update(ctx, anotherObj)).To(Succeed())
})

By("verifying that both CRs were reconciled after setting label to the unlabeled CR", func() {
Eventually(func() bool {
mu.Lock()
defer mu.Unlock()
return len(reconciledCRs) == 2 &&
slices.Contains(reconciledCRs, labeledObjKey.Name) &&
slices.Contains(reconciledCRs, anotherObjKey.Name)
}, "10s", "100ms").Should(BeTrue())
})
})

It("should reconcile CRs independently when using two managers with different label selectors", func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

labeledObj := testutil.BuildTestCR(gvk)
labeledObj.SetName("labeled-cr-test2")
labeledObj.SetLabels(matchingLabels)
labeledObjKey := types.NamespacedName{Namespace: labeledObj.GetNamespace(), Name: labeledObj.GetName()}

anotherObj := testutil.BuildTestCR(gvk)
anotherObj.SetName("another-cr-test2")
anotherObjKey := types.NamespacedName{Namespace: anotherObj.GetNamespace(), Name: anotherObj.GetName()}

var reconciledCRs []string
var anotherReconciledCRs []string

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

postHook2 := makePostHook(&mu, &anotherReconciledCRs)
_, done2 := setupManagerWithSelectorAndPostHook(ctx, postHook2, anotherMatchingLabels)
doneChans = append(doneChans, done2)

By("creating a CR with matching labels for the first manager", func() {
Expect(mgr.GetClient().Create(ctx, labeledObj)).To(Succeed())
})

By("verifying that only the first manager reconciled the CR", func() {
Eventually(func() bool {
mu.Lock()
defer mu.Unlock()
return len(reconciledCRs) == 1 && reconciledCRs[0] == labeledObjKey.Name
}, "10s", "100ms").Should(BeTrue())

Consistently(func() bool {
mu.Lock()
defer mu.Unlock()
return len(anotherReconciledCRs) == 0
}, "2s", "100ms").Should(BeTrue())
})

By("creating a CR with matching labels for the second manager", func() {
Expect(mgr.GetClient().Create(ctx, anotherObj)).To(Succeed())
Expect(mgr.GetClient().Get(ctx, anotherObjKey, anotherObj)).To(Succeed())
anotherObj.SetLabels(anotherMatchingLabels)
Expect(mgr.GetClient().Update(ctx, anotherObj)).To(Succeed())
})

By("verifying that both managers reconcile only matching labels CRs", func() {
Eventually(func() bool {
mu.Lock()
defer mu.Unlock()
return len(reconciledCRs) == 1 && reconciledCRs[0] == labeledObjKey.Name
}, "10s", "100ms").Should(BeTrue())

Eventually(func() bool {
mu.Lock()
defer mu.Unlock()
return len(anotherReconciledCRs) == 1 && anotherReconciledCRs[0] == anotherObjKey.Name
}, "10s", "100ms").Should(BeTrue())
})
})
})

_ = Describe("Test custom controller setup", func() {
var (
mgr manager.Manager
Expand Down Expand Up @@ -1743,3 +1881,34 @@ func verifyEvent(ctx context.Context, cl client.Reader, obj metav1.Object, event
Reason: %q
Message: %q`, eventType, reason, message))
}

func makePostHook(mu *sync.Mutex, reconciledCRs *[]string) hook.PostHook {
return hook.PostHookFunc(func(obj *unstructured.Unstructured, _ release.Release, _ logr.Logger) error {
mu.Lock()
defer mu.Unlock()
if !slices.Contains(*reconciledCRs, obj.GetName()) {
*reconciledCRs = append(*reconciledCRs, obj.GetName())
}
return nil
})
}

func setupManagerWithSelectorAndPostHook(ctx context.Context, postHook hook.PostHook, matchingLabels map[string]string) (manager.Manager, chan struct{}) {
mgr := getManagerOrFail()
r, err := New(
WithGroupVersionKind(gvk),
WithChart(chrt),
WithSelector(metav1.LabelSelector{MatchLabels: matchingLabels}),
WithPostHook(postHook),
)
Expect(err).ToNot(HaveOccurred())
Expect(r.SetupWithManager(mgr)).To(Succeed())

done := make(chan struct{})
go func() {
defer close(done)
Expect(mgr.Start(ctx)).To(Succeed())
}()
Expect(mgr.GetCache().WaitForCacheSync(ctx)).To(BeTrue())
return mgr, done
}