Prune logging cannot be set

[fgiloux]

Bug Report

What did you do?

Wanting to configure the Prune structure but the log field is not exported and never set.

What did you expect to see?

Either that the field is automatically populated or that it is exported.

What did you see instead? Under which circumstances?

A field that is not exported and not automatically set.

Possible Solution

Although, I am not in favor of using global variables for loggers things should be consistent inside a library. The other features provided by operator-lib for instance event handler and leader election use a preset variable:

var log = logf.Log.WithName("event_handler")
var log = logf.Log.WithName("leader")

logf is a package of the controller-runtime library, which contains logr.Logger Log. This assumes that SetLogger was called earlier, which is the same expectation sets by Kubebuilder/controller-runtime.
This makes this library less suitable for clients or other apps not based on controller-runtime but again things should be consistent.

Another thing I would like to mention and a second issue could get open for it is that the naming/structure could be improved. It seems a bit unconventional to have a method like Execute on a structure named Config. I would expect that more on a structure called Pruner. Config being what is provided by users to get Pruners that fits their needs.

[fgiloux]

cc @camilamacedo86 @Sijoma @ryantking

[camilamacedo86]

The codes shared from operator-lib came from old implementations.
The C+R implementation for the logs changed if I am not wrong here afterwords.

What exactly is required to do here? Just change the lib imported?

It seems a bit unconventional to have a method like Execute on a structure named Config
I think it is about: https://github.com/operator-framework/operator-lib/blob/main/prune/prune.go#L77

This shows that the idea is to apply/run the prune configuration. Would pruneconfig.ApplyWith(ctx) solve your concern?

[jberkhahn]

this (and the linked PR) seems good. I don't seem to have permissions over in operator-lib, though.
/lgtm

[joelanford]

@fgiloux instead of using a new global logger, would it be possible to pull the logger from the context.Context as shown here: https://github.com/kubernetes-sigs/kubebuilder/blob/5ef7c83ec2f843b3d74f864eead8200756c7b6a6/testdata/project-v3/controllers/admiral_controller.go#L50?

[fgiloux]

@joelanford isn't it making the assumption that the library functions would be called from the reconcile loop? This may not be the case. The initial author mentioned that it may be called from a cron package: https://sdk.operatorframework.io/docs/best-practices/resource-pruning/#pruning-execution
Obviously we could still require to have the logger passed through the context but see Dave Cheney's concerns on the approach
Generally this PR only aims at unlocking people wanting to use the prune package of the library, which is documented in operator-framework and downstream. There is more to do and Ryan has done some work on the design side, which has unfortunately not been merged yet: operator-framework/enhancements#109
Let me know how you see things.

[joelanford]

Yeah, I'm generally always in the Dave Cheney camp, and I would typically be on this topic as well. I think the reasons I favor the context approach over the global approach in this case are:

• I think I'm against globals more than I'm against context values.
• I'd imagine most usage of this will be in a reconcile function with the ctx that's pased in, in which case the caller will get the extra logger k/v name/namespace pairs for free.
• With the context approach, there's an opportunity for the library user to provide their own implementation or extra customization that's potentially separate from the global logger they set with log.SetLogger (e.g. by changing the base log level, adding more k/v pairs for better context about the pruning, using a null logger, or using a completely different implementation altogether). There's a function in both logr and controller-runtime to configure a context with the logger to make this easy.
• Folks get cranky when a library uses a logger they can't control.

Perhaps in Execute we could try to get a logger from the context, and if it's nil, we could use the default global logger? In fact, it looks like that's pretty much what log.FromContext does.

Another option would just be to make Log an exported field of the Config struct.

Having said all that. I don't feel so strongly that the PR should be blocked. If I've convinced you, great! If not, feel free to unhold the PR.

[fgiloux]

@joelanford Thanks for the input. I have reworked the pull request. I am not fully happy with the result but I hope that it is an acceptable state till some redesign comes from Ryan's enhancement proposal.
Here is what I have done:

• removed the global var (I like it but it makes it inconsistent with the way it is done in other modules of the library)
• supporting that the logger is passed through the context
• defaulting to the logger set as part of the config if no logger is passed by the context.

One consequence of the context approach is that there is still no compile time check. The lack of key validation is mitigated by the fact that there is a canonical way provided by logr to set and retrieve the logger from context:
logr.FromContext(ctx) and logr.NewContext(ctx, log)
There is a fallback (the config field) when the logger is not available in the context for use of the library from controllers not based on controller-runtime. I am not sure whether it is a good idea. For log and other things we may achieve a cleaner API if the scope of the lib was limited to controller-runtime.