Consolidate controllers implementation to use scope objects

[fabriziopandini]

User Story

As a developer, I would like to have a simple way to pass objects throw the chain of reconcile funcs inside each controller
As a developer, I would like to have a consistent implementation approach in different controllers

Detailed Description

If a look at controllers, as of today all the reconcile funcs are getting in input the same list of parameters, and in most cases, this list is composed by 3 or more objects.

• As of today, adding a new parameter to this list has a domino effect, because you have to change all the signatures up to the point where the value is used.
• In order to implement conditions, there is an increasing need of issue Patch command in the middle of the reconcile logic, but given the above limitation, we are recreating the patch helper/reimplementing the Patch call in many places instead of re-using that exists in the main reconcile loop, and this is risky due to the increasing number of patch options.
• In most cases, this list is already missing of the log parameter, and as a consequence, we are recreating the same log.WithValues in many places

In order to address this problem I would like to discuss the option to introduce a scope object with the goal to provide a single object for passing along the reconcile funcs chain the following objects:

• the operation context (ctx)
• the object to reconcile + all the related objects (e.g. machine, infraMachine, bootstrapConfig)
• the log instance with all the WithValues keys for ^^
• the patchHelper for ^^

The scope object should provide access to the above objects, expose the Patch method (thus ensuring the various patch options are defined in a single place).

Additionally, the scope can implement some utility methods to handle the contained objects, like e.g. filters; by moving such details of object handling detail out of the reconcile loop, there is a concrete opportunity to clean up and make more readable reconcile logic in the main controller file.

WDYT?

Anything else you would like to add:

Scope objects are already used in CAPA
KCP has a controlplane object that acts as a scope object

/kind cleanup

[vincepri]

@randomvariable can speak more for CAPA, mostly a personal opinion, but I'd rather find a better solution in controller-runtime that is generic and can setup things properly, rather than introduce scoped reconcilers which are hard to debug.

the operation context (ctx)

This is already being tackled in controller-runtime v0.7.x, each Reconciler will now accept a context.

the log instance with all the WithValues keys for ^^

Same as above, the context now contains a logger that reconcilers can retrieve and use throughout the lifetime of the reconciliation loop.

Scope objects that we find today in CAPA/CAPV/CAPZ have definitely fulfilled their purpose, although they're one of those things that try to do too much and are hard to test.

[fabriziopandini]

rather than introduce scoped reconcilers which are hard to debug

I'm not willing to introduce scoped reconcilers but to extend to CAPI the pattern of using scope object like in CAPA

I'd rather find a better solution in controller-runtime

That's interesting, but TBH, given that Log and ctx are already addressed, all the remaining bits are really CAPI/controllers specific, so I don't find something that can be generalized.
Do you have something specific in mind?

[vincepri]

Do you have something specific in mind?

Not yet, need to think through a little more :D

I'm not willing to introduce scoped reconcilers but to extend to CAPI the pattern of using scope object like in CAPA

That's what I meant as well, I don't know if that's the best way forward, I like the simplicity of our controllers today

/milestone Next

[randomvariable]

Being able to add more keys to the logger and having them passed downstream would be great. I do have a branch of KCP that attempts to thread a logger all the way through everything. If ctx is going to have a logger, I guess that's fine.

Otherwise, main benefits in CAPA have been having the well-formed AWS service clients and utility functions, but otherwise they've not lead to the cleanest code. CAPA's scopes have also been broken up into much smaller ones to accommodate the EKS implementation, so atm.

[vincepri]

If ctx is going to have a logger, I guess that's fine.

Not only that, but you'll be able to enrich the logger when passing it down 😄

Otherwise, main benefits in CAPA have been having the well-formed AWS service clients and utility functions, but otherwise they've not lead to the cleanest cod

Do we now have tests for the scope objects as well? IIRC they were untested for a while

[detiber]

I too would like to avoid extending the "scope" pattern further.

It makes debugging more difficult, makes testing more difficult (what all do I need to populate in the scope as a bare minimum for testing this thing), and it tends to leak much further than the controller code and down into libraries as well.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fabriziopandini]

/remove-lifecycle stale

[vincepri]

Now that we have the logger passed in with the context, we should be able to close this one out for now. As mentioned above, I don't think expanding the scope pattern found in CAPA is actually something we might want to do.

[fabriziopandini]

/close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.