• ✅login: ntnn / (b7916fb)
• ✅login: ntnn / (b7916fb, b2a7f79)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185, 1ead538)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185, 1ead538, 627ec95)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185, 1ead538, 627ec95, 717cd6d)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185, 1ead538, 627ec95, 717cd6d, 795324e)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185, 1ead538, 627ec95, 717cd6d, 795324e, 5392364)
• ✅login: ntnn / (b7916fb, b2a7f79, 5788185, 1ead538, 627ec95, 717cd6d, 795324e, 5392364, 8bdc98a)

Welcome @ntnn!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @ntnn. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

why is the context not cancelling the handlers?

why is the context not cancelling the handlers?

kubernetes/staging/src/k8s.io/client-go/tools/cache/shared_informer.go

Lines 884 to 909 in 195803c

	func (p *sharedProcessor) run(ctx context.Context) {
	func() {
	p.listenersLock.RLock()
	defer p.listenersLock.RUnlock()
	for listener := range p.listeners {
	p.wg.Start(listener.run)
	p.wg.Start(listener.pop)
	}
	p.listenersStarted = true
	}()
	<-ctx.Done()
	p.listenersLock.Lock()
	defer p.listenersLock.Unlock()
	for listener := range p.listeners {
	close(listener.addCh) // Tell .pop() to stop. .pop() will tell .run() to stop
	}
	// Wipe out list of listeners since they are now closed
	// (processorListener cannot be re-used)
	p.listeners = nil
	// Reset to false since no listeners are running
	p.listenersStarted = false
	p.wg.Wait() // Wait for all .pop() and .run() to stop

Because the context is for the informer, not the controllers.

Meaning if the informer / informer factory is shutdown the handlers would also be shut down.

However if a controller is stopped - but not the informer - then the goroutines for the handlers keep running.

However if a controller is stopped - but not the informer - then the goroutines for the handlers keep running.

when does this happen in the controller manager? I don't remember the controllers to be stopped , is not all context cancelled at the same time?

E.g. the QuotaMonitors leak goroutines whenever a monitors is stopped, as the handlers are registered but never cleaned up:

QuotaMonitors are regularly synced monitors to remove are removed by closing the stopCh:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/resourcequota/resource_quota_monitor.go#L238

Trough the wait.Until.. here:

That's calling .resyncMonitors and in turn .SyncMonitors here:

This happens whenever quota-relevant grvs are removed:

That is one case that likely happens in production.

As for the controller manager I'd have to check - likely there it doesn't matter because the controllers and informers are shutdown with everything else when the node is restarting.

For KCP this happens because KCP starts and stops controllers dynamically for workspaces - the documentation does a better job at explaining workspaces then I'd do: https://docs.kcp.io/kcp/v0.27/concepts/workspaces/

Imho it doesn't matter that the informer cancels any handlers still registered on shutdown as controllers should cleanup any resources they require when shutting down. Both cases make sense (e.g. the handlers being stopped by the informer on shutdown as the informer quits as well as the controller stopping the handlers as the controller shuts down).

As far as the controller manager goes - these leaks do affect it.

This is the function starting the controllers and informers:

Notice that StartControllers uses the passed ctx - the informer factory however gets stopCh, which is defined here:

This means that the controllers are also leaking goroutines in the controller manager every time the leader changes.

Edit: That should be fixed as well, because every time the leader changes the informer factories will leak their goroutines. I could add a commit to replace the channel with context cancellation unless there's arguments against it.

This means that the controllers are also leaking goroutines in the controller manager every time the leader changes.

when the leader changes the process exits

when the leader changes the process exits

Ah, missed that; so it doesn't matter for the controller manager.

when the leader changes the process exits

Ah, missed that; so it doesn't matter for the controller manager.

I'm just trying to understand the changes and the motivation, we went through a lot to avoid goroutines leakes on controllers in the apiserver, see #108483 , so we should not leak goroutines.

Said this, we need to understand the benefit of the change and the ROI, since touching these critical pieces can have subtle bugs and the blast radius is very large ... I do not see any risk on goroutine leaks if the context termination leads to stop the process

I more than understand the hesitation on a change of this impact, but I'd rather put the "best practice" solution forward for discussion - this could definitely happen for other projects and also kube itself if the lifecycle of controllers is ever changed.

I had another idea to keep going with the context cancelletion - the shared informer could get a method like this:

AddContextEventHandlerWithOptions(ctx context.Context, handler ResourceEventHandler, options HandlerOptions) error

Either with or without returning a handler - though if this handler is context-cancelled through the context provided by the controller the registration is superfluous.

A very simple implementation would just launch a goroutine that blocks until the context is cancelled and removes the handler:

func (s *sharedIndexInformer)  AddContextEventHandlerWithOptions(ctx context.Context, handler ResourceEventHandler, options HandlerOptions) error {
	handler, err := s.AddEventHandlerWithOptions(handler, options)
	if err != nil {
		return err
	}
	go func() {
		<-ctx.Done()
		utilruntime.HandleError(s.RemoveEventHandler(handler))
	}()
	return nil
}

And errors could still be handed off to utilruntime.HandleError to not change the function signatures of the controllers.

Said this, we need to understand the benefit of the change and the ROI, since touching these critical pieces can have subtle bugs and the blast radius is very large ... I do not see any risk on goroutine leaks if the context termination leads to stop the process

In the future, I would like to explore an option to transition leaders without stopping the process so fixing goroutine leaks would be important. Also have similar questions as @aojea though, I was also under the impression that shutting down an informer factory would clean up these handlers. Is there no way of making this fix without having to rewrite every controller?

Stopping the informer does clean up those handlers - and cleaning the handlers up can only be done by either updating the controllers or by shutting down the informer.

At least at the moment for kube it doesn't make a difference; even if you would go into leader migration without stopping the former leaders process you would likely shutdown the informer (e.g. by changing the stopCh here to also use ctx.Done()).

However if the informer should be kept running on non-leaders the handlers would leak on a leader migration.

Tbh I think the option with AddContextEventHandler.. would be the cleanest solution.

Ah thanks for the explanation. So this only happens for very specific controllers that can exit while keeping the informerFactory running. None of the KCM controllers should fall into that bucket then?

However if the informer should be kept running on non-leaders the handlers would leak on a leader migration.

Tbh I think the option with AddContextEventHandler.. would be the cleanest solution.

I recently wrote an apiserver controller that falls into the exact condition you described, must be able to stop and start without touching the informerFactory (https://github.com/kubernetes/kubernetes/blob/master/pkg/controlplane/controller/leaderelection/leaderelection_controller.go#L80-L89) and removing the handler does feel a bit hacky. AddContextEventHandler (or something similar) is a pattern we could benefit from.

None of the KCM controllers should fall into that bucket then?

Correct, everything started by the controller manager is not directly affected because the controller manager shuts down both the informers and controllers at the same time.

removing the handler does feel a bit hacky.

Yeah, that's what I felt as well also with the .ShutDown methods in this PR. Your solution is effectively the same as I did in this PR.

AddContextEventHandler (or something similar) is a pattern we could benefit from.

I'll prep a PR tomorrow

I'll prep a PR tomorrow

please don't make api incompatible changes on those informers methods, it is very painful for downstream projects to rewrite all the controllers just to adapt for a new field

/triage accepted

Its been ~two months since the last activity here, is this still being worked on?

Sortof - for controllers to unregister handlers upon shutdown they either need to retain the handler ref and unregister (like in this PR) or AddContextEventHandler is added to support context-aware handlers (as in the linked PR, after which I'd update this PR).
@aojea had also mentioned that he wanted a more holistic solution for controllers with lifecycles - which would also be great.

I'm basically just waiting for reviews and for PRs to be merged or closed.

@aojea had also mentioned that he wanted a more holistic solution for controllers with lifecycles - which would also be great.

my bad sorry, this slipped , regarding the changes in the controller manager those are not required as they do not leak goroutines #131199 (comment)

Regarding the holistic solution, I think there are lately similar request so I was wondering it will be better to build a better solution around all this problem or just this context cancellation is enough ... but you need to involve API machinery leads for that , @jpbetz and @deads2k (Joe is out at least a few weeks IIRC)