UMBRELLA: design and refactor graceful termination

[tapih]

To shutdown controller gracefully, containers should be terminated after endpoints deletion.

If time.Sleep(someSeconds) is added between these lines, applications made with controller runtime can wait for endpoint deletion as default.
What do you think?

[alexeldeib]

See some of the previous discussions about graceful termination:

• Stopping individual controllers in a manager #730
• Support for stoppable managers #695
• Manager does not wait for Runnables to stop #350

I think a better approach than sleeping (and one that's been discussed a few times) is properly wiring up all the contexts (well, currently stop channels) and using either a wait group (for manager shutdown) or some sort of map + lock + shutdown mechanism (for manager shutdown + dynamically (un)loading controllers).

There's a tentative desire to replace all the stop channels with context plumbed all the way through -- graceful manager termination could plausibly be done in tandem with that change (or not, but seems natural).

[tapih]

Sorry for my poor explanation. What I meant to propose is that, by inserting sleep just after the signal handler, we can omit /bin/sleep in preStop hook of Kubernetes Pods.

When a Pod is shutting down, pod termination and endpoint deletion are executed at the same time.
The problem is that some requests cannot be accepted if the endpoint deletion does not finish while the pod termination finishes.
I meant by the word "graceful shutdown" in the previous comment to avoid this situation by ensuring that the pod is terminated after the endpoint is deleted.

One way to wait for endpoint deletion is to add the sleep command into preStop, but if a base image does not have the sleep command, this approach cannot be taken.
So, I would like to add time.Sleep(someSeconds) to resolve this problem at the framework level.

[vincepri]

/kind design

We'd need a design document in form of PR to the controller-runtime repository.

/help
/priority important-soon

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/kind design

We'd need a design document in form of PR to the controller-runtime repository.

/help
/priority important-soon

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.

[seh]

Presumably the duration to sleep there would need to be configurable, with zero meaning not to sleep at all. But sleeping like this is a brittle cover for the lack of proper coordination. The chosen duration always turns out to be wrong.

[alexeldeib]

Based on the discussion today I think ideally we see one design sweep out all of the related context issues:

• dynamically adding/removing controllers
• stopping the manager and gracefully waiting for termination
• replacing stop channels with context everywhere (maybe?)

if the manager and all dependencies respect context behavior properly, the ask here becomes a matter of usage -- wire up your signal handler to wait for the manager to cleanly exit.

I don't think the design will be anything crazy, but having poked at this a bit I think it will take some time + thought to cleanly flesh out the corner cases.

[negz]

It seems a small handful of issues around stopping managers and controllers (including #730, which I am fairly invested in) just got deduplicated into this one. Could we consider renaming this issue to reflect the scope? Perhaps a new issue is warranted to track all of this? The title (and to some extent content) of this issue don't immediately reflect the scope it seems to have taken on.

[vincepri]

@negz fair point! re-titled this issue, let me know if you have further feedback

[answer1991]

consider my solution at PR #805 , which was used by kube-controller-manager.

[answer1991]

The controller program exit step may including these:

1. close controller stop channel to notify controller and workers exit
2. wait for controller's queue closed, which means no new reconcile action will be invoked by controller works. And then wait for all workers exit will be better
3. close lease stop channel to notify lease to quit leading
4. wait for leading stoped
5. exit program

[negz]

There's a tentative desire to replace all the stop channels with context plumbed all the way through -- graceful manager termination could plausibly be done in tandem with that change (or not, but seems natural).

Is it common to use contexts for this purpose, given that contexts are intended to be "request scoped"? It feels like using a context to replace a stop channel could be a misuse, depending on how you interpret a "request".

[negz]

Is it common to use contexts for this purpose

Apparently it is - kubernetes/kubernetes#57932 is an example of leader election being migrated from stop channels to contexts.

[alexeldeib]

Making incremental notes on context changes required:

• source should accept context -> ⚠ make Start on Source interface cancellable #903
• cache/multinamespace cache start and wait for sync should accept context
• webhooks should accept context
• dynamic rest mapper should wrap the underlying requests in a context to be non-blocking
• runnables should accept a context
• manager start should accept a context
• contollers start should accept a context
• informers map should accept a context

[alvaroaleman]

@alexeldeib just for my understanding, maybe I am missing something. How is graceful termination depending on having plugged through the usage of context everywhere? Graceful termination just means that we have to redesign some interfaces to allow Runnables to signal that they are done shutting down and then wait for that or a timeout or not?

[alexeldeib]

Yeah, I think this issue arguably re-conflated two things:

• our usage of stop channels is very inconsistent and messy, and it'd be preferable to cleanly wire context
• there's no way to do graceful shutdown properly

[alexeldeib]

• things that should be non-blocking are actually non-blocking