• controllerName = "service-cidr-controller" https://github.com/kubernetes/kubernetes/pull/127550/files#diff-37ce781520331481491143bc84a058962599acdc003edf2e905cfa60fb52d8b9L57
• c.name = "service-cidr-controller" https://github.com/carlory/kubernetes/blob/ctrl-name/cmd/kube-controller-manager/names/controller_names.go#L85

FieldManager is not changed.

This is unnecessary because each controller gets instantiated with a context where the controller name is already included via WithName.

/lgtm cancel

// nameKey is used to log the `WithName` values as an additional attribute.
nameKey = "logger"

The key is different when printing the controller name.

	logger := klog.FromContext(ctx)
	logger.Info("Starting daemon sets controller")
	defer logger.Info("Shutting down daemon sets controller")

When I look at the code, I see that there're some comments.

// In StartController, an explicit "controller" key is used instead, for two reasons:
// - while contextual logging is alpha, klog.LoggerWithName is still a no-op,
//   so we cannot rely on it yet to add the name
// - it allows distinguishing between log entries emitted by the controller
//   and those emitted for it - this is a bit debatable and could be revised.

@pohly What's current state of the contextual logging feature? And What's the desired key name for controller name, logger or controller?

Where should other todos get the controller name?

//  2. used anywhere inside the controller itself:
//     2.1. [TODO] logging should use a canonical controller name when referencing a controller (Eg. Starting X, Shutting down X)
//     2.2. [TODO] emitted events should have an EventSource.Component set to the controller name (usually when initializing an EventRecorder)
//     2.3. [TODO] registering ControllerManagerMetrics with ControllerStarted and ControllerStopped
//     2.4. [TODO] calling WaitForNamedCacheSync

• 2.2 may need to move initializing an EventRecorder into Run func. it's tracked in The problem with the event recorder is that it runs (sic!) goroutines. #128282
• 2.3 may need to define a new func for each controller to run, i.e. (c *Controller) RunWithNamedMetircs(ctx, controllerName, metrics)
• 2.4 maybe able to get logger from ctx. It requires a new function. i.e. WaitForCacheSyncWithContext?

I'm unsure if my idea is correct.

What's current state of the contextual logging feature?

It's enabled by default, but further work is needed on helper packages, in particular client-go.

And What's the desired key name for controller name, logger or controller?

I don't think it matters, and it is not configurable, so it's "logger".

Where should other todos get the controller name?

The best approach would be to complete as much of the client-go changes as possible, then proceed as described in #126379. I don't think it makes sense to create PRs for individual APIs.

maybe able to get logger from ctx. It requires a new function. i.e. WaitForCacheSyncWithContext?

I'm working on it - see #126387

Great work! This PR can be closed. Thanks for your review!

For the first to-do, I think the remaining thing is to remove the TODO and (Eg. Starting X, Shutting down X) words.

replaced by #128537

/close

@pohly @carlory

I think it is still useful to have the name passed separately:

• There is no way how to get the controllerName from the context. And it is also more readable to pass it directly.
• The logger name can be prefixed multiple times, and it is good to have a stable identifier for a controller:

The stable identifier is mainly needed for:

• clienBuilder (service account)
• eventSource.component
• queue names
• metrics / controllerManagerMetrics
• as a fieldManager

We do not have to duplicate the name in Starting/Shutting down, but IMO it is still useful to have when debugging the controllers. Because there may be a longer name path or an incorrect/incomplete path - depending under what context the controller is run.

For example in https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/certificates.go we run multiple controllers under the same context.

I think it would be good to reopen this PR and at least unify the messages and change the Apply calls.

The stable identifier is mainly needed for: [...]

This seems reasonable to me. Some of these cases might go away when making more of those helpers context-aware, but that's further down the road. How was that handled before? Each controller hard-coding its own name and potentially disagreeing with its caller on what that name is? I vaguely remember something along those lines.

Would it make sense to pass the controller name in the ControllerContext? It's less explicit, but also less intrusive.

Whatever way the name is passed, let's clearly document that logging doesn't need to replicate the name because it is already in the contextual logger.

Each controller hard-coding its own name and potentially disagreeing with its caller on what that name is?

Yeah, most of the controllers have inconsistent hardcoded names for everything and it is difficult for us to change most of the stuff. But the new controllers should name everything consistently.

Would it make sense to pass the controller name in the ControllerContext?

There is only one instance of ControllerContext per KCM and is used to pass SharedInformerFactory, etc. to the init func. And not passed to each controller. So it is not well suited for that use case.

Whatever way the name is passed, let's clearly document that logging doesn't need to replicate the name because it is already in the contextual logger.

I am okay with that. The users will have to adapt to this change though.

There is only one instance of ControllerContext per KCM

It could get copied and modified specifically for each controller. But it's not important.