Reconcile methods should take a context and logger

[vincepri]

We can have different solutions to this issue, but it's going to be indeed a breaking change.

A few ideas to discuss:

• Add two parameters to Reconcile, one for context, one for logger. (Simple solution, quick to implement).
• Create a new Context type in controller-runtime and expand it for our own use cases (More complicated, needs a design document / proposal).
  • Importable using ctrl.Context, should always satisfy context.Context interface.
  • Contains a pre-populated logger with name/namespace, and maybe gvk?

[alvaroaleman]

@vincepri This sounds like it's already part of a solution, could you elaborate a bit on the problem/use case?
Also:

• How should the controller construct the context, should it have e. G. a timeout or do you just want to have any context?
• Same for the logger, should it have any fields preconfigured?
• How do we handle ppl wanting to use their own loggers?

As an implementation detail comment, this could be implemented backwards compatible by adding a new interface, checking via type assertion if the reconciler implements it and if yes use that rather than the existing reconcile.

[vincepri]

could you elaborate a bit on the problem/use case?

Was planning on discussing at the next community meeting, the main use case is to ease adoption and reduce boilerplate. If we look at a sample of controller-runtime implementations, pretty much all of them define a logger and context at the very top of the function.

How should the controller construct the context, should it have e. G. a timeout or do you just want to have any context?

There is another issue around a global timeout (#798), which could be useful in this case. In alternative, we can start with a simple context that has no deadline, and see how things go from there. I'd like to hear more feedback from the wild on this.

Same for the logger, should it have any fields preconfigured?

I'd like to start with a logger that has the reconciled object type, name, and namespace.

How do we handle ppl wanting to use their own loggers?

This doesn't change from what we have today, which is the ability to set the global klogr logger in CR's log package.

As an implementation detail comment, this could be implemented backwards compatible by adding a new interface, checking via type assertion if the reconciler implements it and if yes use that rather than the existing reconcile.

Definitely a possibility. We should definitely explore more solutions. That said, having one interface to maintain is probably easier, cleaner, and more consistent.

[vincepri]

/milestone breaking-next

[vincepri]

/kind design
/priority important-longterm
/help

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/kind design
/priority important-longterm
/help

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.

[djzager]

I'm a little more keen on the Reconciles method taking a context than I am with it taking the logger.

This doesn't change from what we have today, which is the ability to set the global klogr logger in CR's log package.

I didn't know about this. In the case that logger was included in the Reconcile methods this particular aspect (being able to choose your own logger) should be called out as best as possible in docs.

[djzager]

Looking through a couple of controllers that I've been a part of writing and I'm seeing a lot of ensureSomeResource(instance, reqLogger) and I'm suddenly less opposed.

I'm looking forward to the discussion in the next community meeting to better understand the value proposition.

[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