Skip to content
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.

Sign up for GitHub

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

ROX-19221: Fix reconciling with label selector for multiple reconcilers #42

Merged
merged 4 commits into from
Aug 25, 2023
Merged

ROX-19221: Fix reconciling with label selector for multiple reconcilers #42

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7126477
Add filter predicate to secret kind watch
kurlov Aug 22, 2023
3d7adc3
Change fix to filter resource in the reconcile function
kurlov Aug 24, 2023
3734571
Fix doc string typo
kurlov Aug 24, 2023
24b3290
Update description for setting broken action client
kurlov Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
errs "github.com/pkg/errors"
"sigs.k8s.io/controller-runtime/pkg/event"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -276,7 +277,7 @@ func WithOverrideValues(overrides map[string]string) Option {
}
}

// WithDependentWatchesEnabled is an Option that configures whether the
// SkipDependentWatches is an Option that configures whether the
// Reconciler will register watches for dependent objects in releases and
// trigger reconciliations when they change.
//
Expand Down Expand Up @@ -597,7 +598,7 @@ func WithControllerSetupFunc(f ControllerSetupFunc) Option {
}

// ControllerSetup allows restricted access to the Controller using the WithControllerSetupFunc option.
// Currently the only supposed configuration is adding additional watchers do the controller.
// Currently, the only supposed configuration is adding additional watchers do the controller.
type ControllerSetup interface {
// Watch takes events provided by a Source and uses the EventHandler to
// enqueue reconcile.Requests in response to the events.
Expand Down Expand Up @@ -650,6 +651,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
return ctrl.Result{}, err
}

if r.selectorPredicate != nil && !r.selectorPredicate.Generic(event.GenericEvent{Object: obj}) {
Copy link

Choose a reason for hiding this comment

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

Why don't put labels on secrets instead? I.e. do a k8s client customization as it is done for the ownerReference.
Adding correct label selector on secrets watch may also give a performance benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing predicate to the secret watch was initial approach. But the downside is that it hides filtering mechanism and it's pretty easy to overlook it in the future (e.g. adding additional watch). Filtering selector in the reconcile is straightforward and robust approach. But you are right, it's not the best in terms of CPU because Reconcile call is triggered

Copy link
Member

Choose a reason for hiding this comment

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

The CPU consumption is minimal here, a reconcile call in itself it is not computationally expensive - it starts a function with exits immediately.

log.V(1).Info("Label selector does not match, skipping reconcile")
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the affected object and labels to the log message?
As an engineer I would like to see more details why it was skipped and get a hint which label would reconcile the CR.

Copy link
Member Author

@kurlov kurlov Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Name and namespace is part of the logger
Log message will look like this:

2023-08-25T16:25:05+02:00	DEBUG	controllers.Central	Label selector does not match, skipping reconcile	{"central": {"name":"stackrox-central-services","namespace":"stackrox"}}

return ctrl.Result{}, nil
}

// The finalizer must be present on the CR before we can do anything. Otherwise, if the reconciliation fails,
// there might be resources created by the chart that will not be garbage-collected
// (cluster-scoped resources or resources in other namespaces, which are not bound by an owner reference).
Expand Down
39 changes: 39 additions & 0 deletions pkg/reconciler/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ var _ = Describe("Reconciler", func() {
Expect(mgr.GetCache().WaitForCacheSync(ctx)).To(BeTrue())

obj = testutil.BuildTestCR(gvk)
obj.SetLabels(map[string]string{"foo": "bar"})
objKey = types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
req = reconcile.Request{NamespacedName: objKey}

Expand Down Expand Up @@ -517,6 +518,8 @@ var _ = Describe("Reconciler", func() {
cancel()
})

selector := metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}

// After migration to Ginkgo v2 this can be rewritten using e.g. DescribeTable.
parameterizedReconcilerTests := func(opts reconcilerTestSuiteOpts) {
BeforeEach(func() {
Expand All @@ -535,6 +538,7 @@ var _ = Describe("Reconciler", func() {
WithUpgradeAnnotations(annotation.UpgradeDescription{}),
WithUninstallAnnotations(annotation.UninstallDescription{}),
WithPauseReconcileAnnotation("my.domain/pause-reconcile"),
WithSelector(selector),
WithOverrideValues(map[string]string{
"image.repository": "custom-nginx",
}),
Expand All @@ -550,6 +554,7 @@ var _ = Describe("Reconciler", func() {
WithUpgradeAnnotations(annotation.UpgradeDescription{}),
WithUninstallAnnotations(annotation.UninstallDescription{}),
WithPauseReconcileAnnotation("my.domain/pause-reconcile"),
WithSelector(selector),
WithOverrideValues(map[string]string{
"image.repository": "custom-nginx",
}),
Expand Down Expand Up @@ -1392,6 +1397,40 @@ var _ = Describe("Reconciler", func() {
})
})
})
When("label selector succeeds", func() {
It("reconciles only matching label", func() {
By("setting a broken action client getter for the reconciler", func() {
Copy link
Member

Choose a reason for hiding this comment

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

This describes the action, can you describe it's impact/reason why it is necessary to add a broken action client getter?
For example:
By returning an invalid action client getter to assert different reconcile results

Copy link
Member

Choose a reason for hiding this comment

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

If possible, the solution you've proposed using a custom reconcile.Result object for a skipped reconcile looks cleaner to me. Changing action client getter would have side-effects on this test, when using the reconcile.Result the test is side-effect free.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though introducing a new result type is a small change but it might effect many part and I would like to reduce scope to the bug fixing only

r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) {
fakeClient := helmfake.NewActionClient()
return &fakeClient, nil
})
})

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

By("reconciling is skipped, action client was not called and no error returned", func() {
res, err := r.Reconcile(ctx, req)
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).To(BeNil())
})

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

By("reconciling is not skipped and error returned because of broken action client", func() {
res, err := r.Reconcile(ctx, req)
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).To(MatchError("get not implemented"))
})
})
})
})
})
})
Expand Down