:sparkles: add logging to Enqueue_owner event handler

[varshaprasad96]

This PR adds logging to Enqueue owner event handler, that is helpful in logging the GVK of the owner that caused reconciliation.

Fixes: #1186

In Operator SDK we have a LoggingEnqueueRequestForOwnerHandler (ref) that used to add logging to the existing EnqueueRequestForOwnerHandler. In C-R 0.15.0 this was made internal, making it difficult to create a wrapper around it. However, adding logging to the existing handler available in c-r would be useful to other consumers also in identifying the cause of reconcile.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: varshaprasad96 Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[alvaroaleman]

@varshaprasad96 sorry, I don't understand the problem you are solving. How does the implementation being internal prevent you from adding logging? Your wrapper can and should wrap the handler.Handler interface, not the concrete implementation.

I am not a huge fan of this because it opens a can of worms:

• Maybe the next person wants some additional information to be logged, like what requests actually came out of the handler
• Maybe the next person wants that for some other handler
• Maybe the third person would also like to have metrics for this

I'd prefer it if we made it easy to wrap a handler, possibly even provide a generic logging implementation but not adjust all the handlers.

[erikgb]

I agree with @alvaroaleman, and to get a bit more familiar with the c-r codebase, I drafted something that might allow users to wrap event handlers. Feel free to use it in a new PR if you like the idea: https://github.com/kubernetes-sigs/controller-runtime/commit/2f012137f407fe9a3d8055d889e79a3d80f2ca92

[varshaprasad96]

Your wrapper can and should wrap the handler.Handler interface, not the concrete implementation.

@alvaroaleman Agreed that we need to implement the handler interface. However, enqueueRequestForOwner is a commonly used handler, which is what we are looking to re-use, just with additional logging on who the owner was that caused reconcile.

How does the implementation being internal prevent you from adding logging? Based on the current state, if we want to customize and reuse a particular implementation of handler we would have to re-write the same piece of code that implements the interface.

I would like to explore the options before closing the PR here:

1. Agreed on @erikgb's implementation, implementing a logging handler and providing an option to wrap it with existing one's is useful.
2. Injecting a logger into the handler implementation (similar to how it was done in previous implementations with inject pkg), where if provided we would log the details on the events. Or have the current implementation and allow users to enable/disable it using Options. (Con: we cannot control the log message for each event).
3. Provide an option to enable passing of multiple event handlers. Each event handler would create a listener (https://github.com/kubernetes/kubernetes/blob/2c6c4566eff972d6c1320b5f8ad795f88c822d09/staging/src/k8s.io/client-go/tools/cache/shared_informer.go#L556), which would then be enabled here.

I'm not super familiar with the client-go side of code, but the question which I have with (1) and (3) is while wrapping or enabling multiple handlers, there is no guarantee on the order in which listeners will be distributed. Which is something we would want to maintain at least while logging for a particular event?

[alvaroaleman]

Agreed that we need to implement the handler interface. However, enqueueRequestForOwner is a commonly used handler, which is what we are looking to re-use, just with additional logging on who the owner was that caused reconcile.

Yeah, you can do that as a wraper.

Injecting a logger into the handler implementation (similar to how it was done in previous implementations with inject pkg), where if provided we would log the details on the events. Or have the current implementation and allow users to enable/disable it using Options. (Con: we cannot control the log message for each event).

The approach of injecting things is deprecated. You can get a logger from the manager or anywhere else.

Provide an option to enable passing of multiple event handlers. Each event handler would create a listener [..]

I don't understand this part. The source registers the eventhandler in the cache and then passed the event onto the predicate.predicate and finally handler.Handler (the package here). Why do you want multiple handlers?

[sbueringer]

I'm inclined to close this PR given no recent activity and the alternative suggested by @alvaroaleman. @varshaprasad96 WDYT?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[vincepri]

/close

per comment above

[k8s-ci-robot]

@vincepri: Closed this PR.

In response to this:

/close

per comment above

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.