Manager does not wait for Runnables to stop

[grantr]

Currently Manager.Start immediately returns when stop channel is closed without waiting for Runnables to stop:

controller-runtime/pkg/manager/internal.go

Lines 216 to 219 in 6649bdb

	select {
	case <-stop:
	// We are done
	return nil

This makes writing a Runnable with blocking stop logic difficult or impossible. For example, the Manager's internal serveMetrics is basically a Runnable (a blocking method that takes a stop channel). When the stop channel is closed, serveMetrics tries to shut down its http server:

controller-runtime/pkg/manager/internal.go

Lines 188 to 192 in 6649bdb

	select {
	case <-stop:
	if err := server.Shutdown(context.Background()); err != nil {
	cm.errChan <- err
	}

But in normal usage, the process immediately exits when Manager.Start returns so the Shutdown call is unlikely to complete.

Adding sync.WaitGroup accounting inside the Runnable wrapper goroutines would allow the addition of a Wait() method to the Manager interface that blocks until all Runnables have returned (or WaitWithTimeout(time.Duration), Shutdown(context.Context) for a termination timeout), without changing either the Runnable contract or the Manager contract:

if err := mgr.Start(signals.SetupSignalHandler()); err != nil {
    log.Error(err, "unable to run the manager")
    os.Exit(1)
}
mgr.Wait()

[DirectXMan12]

/kind bug
/priority important-soon

good catch

[rajathagasthya]

I'll take this!

[DirectXMan12]

awesome!

[grantr]

While investigating this I discovered another issue: the Manager's errChan is unbuffered, so only one Runnable goroutine (including the metrics server) will be able to terminate correctly. Others will block indefinitely. An easy fix for this is making the errChan buffered, but that's not a great long-term solution.

Anyone working on solutions in this area might want to keep #330 in mind too.

[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

[rajathagasthya]

/remove-lifecycle stale

Sorry, haven't had a chance to look at this. Will do shortly. Might need your guidance a bit @DirectXMan12

[DirectXMan12]

ack, lmk if/when you need my guidance

[rajathagasthya]

@DirectXMan12 I'm starting to take a look at this. Could you guide me on what a solution could look like?

[DirectXMan12]

off the top of my head, we'd need some way to check that the runnables each terminate. A wait group, as suggested, seems reasonable. Just rolling it into Start instead of a separate wait method could be reasonable, but I'd imaging that'd have some issues too (like delayed error reporting). Need to weigh the pros and cons there.

[DirectXMan12]

probably at least log immediately

[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

[seh]

I’m bitten by this problem with my use of an instrumentation package that I try to shut down cleanly before the process exits. My predicates and reconcilers are making calls into this package, and there’s no way I can see that I can wait for them all to finish before trying to close the connection they’re all using.

It’s one thing to know that the manager observed the “stop” channel becoming ready, when it returns from Start, but that says nothing about the current state of the Runnables—even those that ask for the “stop” channel to be injected.

Using async.WaitGroup sounds like the way to go. Whether delaying the return from Start should be something you can opt into or out of warrants consideration.

[alexeldeib]

/remove-lifecycle stale

[vincepri]

Closing in favor of #764

/close

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

Closing in favor of #764

/close

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.