-
Notifications
You must be signed in to change notification settings - Fork 53
ROX-30138: Add tests for WithSelector #496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
kurlov
wants to merge
10
commits into
operator-framework:main
Choose a base branch
from
kurlov:akurlov/ROX-30138-imporve-withselector-test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
24bd217
Add tests for WithSelector
kurlov 25d253a
Fix lint
kurlov d486710
Small fix
kurlov 23cd0b6
Apply suggestions from code review
kurlov 0a43969
Fix indentation
kurlov 072866a
Use hooks and add another test with two managers
kurlov 25d4810
Fix linting
kurlov ae66a85
Improve assertion
kurlov bd631b6
Add managers cleanup and change Eventually asserting for mutex objects
kurlov 9149213
return protobuf Size() method for label setup check
kurlov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,9 @@ import ( | |
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "slices" | ||
| "strconv" | ||
| "sync" | ||
| "time" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
|
|
@@ -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()) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
@@ -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 { | ||
| 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() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
perdasilva marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| _, 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 | ||
|
|
@@ -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 | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :-(
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
reconciledCRsslice because of the filtering by watchers.@porridge would you agree with that or I missing something?