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

[vincepri]

/lifecycle frozen

[luxas]

@vincepri any update on this? How did the community meeting discussion go?

Add two parameters to Reconcile, one for context, one for logger. (Simple solution, quick to implement).

I think this could actually be a fair and straightforward alternative. It's easy for the user to see that "here's my pre-defined logger, which I can use if I want to". The decision I think depends on if we know of anything else we'd like to provide the controllers.
Three arguments is managable (especially as ctx as the first argument is "a standard") IMO, but 4 would not be.

#798 would be nice to solve in some way, to be able to set a global deadline. The manager could also e.g. close the ctx given to the reconciler loop upon a Ctrl-C signal. We could just say "if Reconcile doesn't respect the context and deadlocks, you're violating the contract", with the tradeoff that if a reconcile loop doesn't follow the contract, there might be multiple executing reconcile loops at the same time (optionally; guaranteed to never run two Reconcile loops twice for the same NamespacedName, for the example of faulty resources)

[vincepri]

There was some consensus on getting this implemented with a simple breaking change to the current API, given that's a breaking change we'd have to wait for the next release. Happy to contribute it (I'll probably be available for this ~2 week), or review it if someone wants to tackle it.

[luxas]

simple breaking change to the current API

So, in other words, the first option with just adding context and logger to the signature?

given that's a breaking change we'd have to wait for the next release

that is, v0.6.0 or v0.7.0?

[vincepri]

So, in other words, the first option with just adding context and logger to the signature?

Yeah, either that or a single struct that has multiple fields (like Log) and implements context.Context interface, that's something we should think about it a little more.

that is, v0.6.0 or v0.7.0?

It'd be for v0.7, given that v0.6 has already been released

[luxas]

I could take a stab at a PoC PR of this if nobody else can.

[alvaroaleman]

@luxas vince already did: #1054

[luxas]

Ah cool 👍

[vincepri]

/close

This has been implemented

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

/close

This has been implemented

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.