Introduce an interface for controllers to improve consistency #3263

mbarnes · 2023-11-02T23:09:56Z

Which issue this PR addresses:

This is loosely related to the epic [ARO-1627] Propagate Errors and Statuses of ARO Controllers to ARO Cluster Operator. It's an enhancement to the AROController base type that was developed for that epic.

What this PR does / why we need it:

All of ARO's controllers can be disabled through operator flags in the cluster object, and so every Reconcile function has a common preamble: fetch the cluster object, check the appropriate operator flag, and short-circuit reconciliation if the flag is set to disabled.

This pull request moves that preamble into AROController for consistent handling of disabled controllers, and allows individual controllers to focus exclusively on their business logic. It also introduces a common interface for controllers to simplify registration during startup.

Test plan for issue:

Add unit tests for the Reconcile logic in AROController, otherwise rely on existing unit and E2E tests.

I verified the controllers using AROController start up by running the operator locally with -loglevel=debug and saw lots of messages like

pkg/operator/controllers/base/aro_controller.go:53 base.(*AROController).Reconcile() running

Is there any documentation that needs to be updated for this PR?

No, this is strictly an internal refactoring.

Additional notes

For reviewers, I suggest reviewing one commit at a time. I structured the commits as an incremental progression.

Also please note the commit messages, especially in the last commit where I tried to leave a detailed explanation.

Progressing toward not exporting controller name constants.

Initial methods are geared toward controller registration: GetName() string SetupWithManager(ctrl.Manager) error All AROController subtypes must implement SetupWithManager.

Use AROReconciler.GetName instead.

This introduces two new methods in AROReconciler: ReconcileEnabled(ctx, request, cluster) -> (result, error) ReconcileDisabled(ctx, request, cluster) -> (result, error) One or the other method will be called based on the controller's "enabled" operator flag. AROController itself now implements the Reconcile method in order to consistently handle disabled controllers. Subtypes need only implement the ReconcileEnabled method, called when the controller is enabled. The Reconcile method uses the "template method" design pattern from object-oriented programming. This warrants some explanation since the pattern looks a little odd in Golang. Some background reading: https://golangbyexample.com/template-method-design-pattern-golang/ AROController provides ReconcileDisabled but leaves ReconcileEnabled unimplemented, so ReconcileEnabled is effectively an abstract method. Therefore AROController itself is not an AROReconciler according to Golang's interface semantics, but its subtypes are. Because AROController's Reconcile method calls ReconcileEnabled, which is abstract, it needs a way to cast itself to an AROReconciler. This is the purpose of the Reconciler field in AROController. The field is set during subtype instantiation: r := ExampleReconciler{ AROController: base.AROController { Log: ... Client: ... Name: ... EnabledFlag: ... } } r.Reconciler = r return r The Reconciler field is essentially a virtual method table like those inherent in polymorphic languages like C++ and Python. It allows AROController to call abstract AROReconciler methods that are implemented by a subtype.

mbarnes · 2023-11-09T08:20:19Z

With #3221 merged I incorporated the GenevaLogging controller into these commits.

tsatam · 2023-11-09T21:19:07Z

pkg/operator/controllers/base/aro_controller.go

 	GetName() string
 	SetupWithManager(ctrl.Manager) error
+	ReconcileEnabled(context.Context, ctrl.Request, *arov1alpha1.Cluster) (ctrl.Result, error)


nit: these names may convey that these functions return a boolean (e.g. isReconcileEnabled). I can't think of an immediately better name, but this function and ReconcileDisabled may need a better name.

Best I can think of is flip-flop it to EnabledReconcile but I'm not sure if that's any clearer.
Naming is hard. I'll submit to the consensus of opinion.

Java uses is as a standard prefix ☕ https://www.comp.nus.edu.sg/~cs2103/AY1516S2/contents/coding-standards-java.html - since that was my first language, it's my preference too. It looks like the standard lib also uses is: https://go.dev/src/go/types/predicates.go. Although, since we don't have a coding standard, I'd say stick with what feels right to you.

I take that to mean the method name is clear enough as is then, if I'm understanding you right, because the expectation for a simple "is reconcile enabled?" test is that it would be named IsReconcileEnabled. Whereas this method is what the controller implements for "reconcile when enabled flag is true". (Note also the companion "reconcile when enabled flag is false" method on the next line down).

We can always rename it later if it proves confusing to others, but I think in the proper context the confusion @tsatam was calling out should be avoided.

I think that we should keep Reconcile as is and use context.WithValue instead because it seems to me that it fits this use case. In fact one could use context.WithValue to pass along whether or not the Reconcile function should be acting in an enabled/disabled mode too.

Imho this would simplify the implementation a bit and return control of how the enabled/disabled logic worked back to each reconcile function.

tsatam

LGTM - one comment on the names for the ReconcileEnabled and ReconcileDisabled functions, but the structure of the change itself looks great, thank you!

github-actions · 2024-01-04T05:01:41Z

Please rebase pull request.

jaitaiwan

I have a couple of thoughts on this PR, one of which talks about another way of implementing which would take advantage of the existing call structure and put control back into the hands of each reconcile function.

jaitaiwan · 2024-02-02T01:01:41Z

pkg/operator/controllers/base/aro_controller.go

+}
+
+func (c *AROController) ReconcileDisabled(ctx context.Context, request ctrl.Request, cluster *arov1alpha1.Cluster) (ctrl.Result, error) {
+	c.ClearConditions(ctx)


Do we want to clear out the current conditions when disabling? I'm wondering if that could cause issues with debugging, in that we might want to keep the conditions around after it's disabled. I don't know of any reason why we couldn't leave the conditions as they are if there's an enabled/disabled condition?

jaitaiwan · 2024-02-02T01:21:01Z

pkg/operator/controllers/base/aro_controller.go

 	GetName() string
 	SetupWithManager(ctrl.Manager) error
+	ReconcileEnabled(context.Context, ctrl.Request, *arov1alpha1.Cluster) (ctrl.Result, error)


I think that we should keep Reconcile as is and use context.WithValue instead because it seems to me that it fits this use case. In fact one could use context.WithValue to pass along whether or not the Reconcile function should be acting in an enabled/disabled mode too.

Imho this would simplify the implementation a bit and return control of how the enabled/disabled logic worked back to each reconcile function.

mbarnes force-pushed the aro-reconciler branch from 789f8e9 to 05a82d6 Compare November 2, 2023 23:17

mbarnes added the loki label Nov 3, 2023

mbarnes force-pushed the aro-reconciler branch 3 times, most recently from b3f3c50 to 066d3b7 Compare November 7, 2023 13:16

Matthew Barnes added 6 commits November 9, 2023 02:18

operator: Let AROControllers configure their own loggers

e397ee6

Progressing toward not exporting controller name constants.

operator: Introduce AROReconciler interface

76934a5

Initial methods are geared toward controller registration: GetName() string SetupWithManager(ctrl.Manager) error All AROController subtypes must implement SetupWithManager.

operator: Do not export controller name constants

9660fb4

Use AROReconciler.GetName instead.

operator: Consistency tweaks

03642e4

operator: Add EnabledFlag field to AROController

f1b40b0

mbarnes force-pushed the aro-reconciler branch from 066d3b7 to 499680a Compare November 9, 2023 08:18

mbarnes marked this pull request as ready for review November 9, 2023 13:48

mbarnes requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, cadenmarchese, UlrichSchlueter, s-amann, SudoBrendan, Shivkumar13, yjst2012, anshulvermapatel and hlipsig as code owners November 9, 2023 13:48

operator: Add unit tests for AROController's Reconcile

3c6c252

mbarnes added the ready-for-review label Nov 9, 2023

tsatam reviewed Nov 9, 2023

View reviewed changes

tsatam approved these changes Nov 9, 2023

View reviewed changes

github-actions bot added needs-rebase branch needs a rebase and removed ready-for-review labels Jan 4, 2024

jaitaiwan reviewed Feb 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce an interface for controllers to improve consistency #3263

Introduce an interface for controllers to improve consistency #3263

mbarnes commented Nov 2, 2023 •

edited

Loading

mbarnes commented Nov 9, 2023

tsatam Nov 9, 2023

mbarnes Nov 9, 2023

SudoBrendan Nov 20, 2023 •

edited

Loading

mbarnes Nov 21, 2023

jaitaiwan Feb 2, 2024

tsatam left a comment

github-actions bot commented Jan 4, 2024

jaitaiwan left a comment

jaitaiwan Feb 2, 2024

jaitaiwan Feb 2, 2024

Introduce an interface for controllers to improve consistency #3263

Are you sure you want to change the base?

Introduce an interface for controllers to improve consistency #3263

Conversation

mbarnes commented Nov 2, 2023 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

Additional notes

mbarnes commented Nov 9, 2023

tsatam Nov 9, 2023

Choose a reason for hiding this comment

mbarnes Nov 9, 2023

Choose a reason for hiding this comment

SudoBrendan Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

mbarnes Nov 21, 2023

Choose a reason for hiding this comment

jaitaiwan Feb 2, 2024

Choose a reason for hiding this comment

tsatam left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2024

jaitaiwan left a comment

Choose a reason for hiding this comment

jaitaiwan Feb 2, 2024

Choose a reason for hiding this comment

jaitaiwan Feb 2, 2024

Choose a reason for hiding this comment

mbarnes commented Nov 2, 2023 •

edited

Loading

SudoBrendan Nov 20, 2023 •

edited

Loading