[kubernetes] Graceful TaskManager termination + fix wrong-signal TODO in ResourceManager#28729
Open
gangavhi wants to merge 1 commit into
Open
[kubernetes] Graceful TaskManager termination + fix wrong-signal TODO in ResourceManager#28729gangavhi wants to merge 1 commit into
gangavhi wants to merge 1 commit into
Conversation
… in ResourceManager Adds kubernetes.taskmanager.termination-grace-period: when set, a new TerminationGracePeriodDecorator sets terminationGracePeriodSeconds and a preStop hook (SIGUSR2) on the TaskManager pod, never overriding a user-supplied pod template. SIGUSR2 triggers TaskExecutor#prepareForTermination(), which proactively disconnects from the ResourceManager instead of waiting to be heartbeat-timed-out, so the slot is freed immediately. The preStop hook then sleeps for most of the remaining grace period before returning, so already-running tasks keep making progress instead of being killed the instant the pod is scheduled for deletion (SIGTERM only follows once preStop returns). Separately, ResourceManager#closeTaskManagerConnection carried a "// TODO :: suggest failed task executor to stop itself" comment. Tracing the actual behavior showed the ResourceManager already calls back into the presumed-dead TaskExecutor here (TaskExecutorGateway#disconnectResourceManager) but that RPC tells it to reconnect, which is the wrong instruction for a TaskExecutor the ResourceManager has already unregistered and told every JobMaster to fail. Adds TaskExecutorGateway#fenceAndStop, which the ResourceManager now calls instead: it tells the TaskExecutor to stop reconnecting and to proactively fail its own currently-running tasks. Both changes are additive: the pod-spec change is opt-in via the new config option, and fenceAndStop only changes the RPC sent on a teardown path the ResourceManager has already committed to - no change to failure-detection timing or semantics. JobManager-side graceful shutdown is deliberately out of scope here: it would need its own SIGUSR2 handler first, since sending that signal unhandled terminates a process by POSIX default. Verified with unit tests and a live kind cluster test: kubectl delete pod on a TaskManager now keeps the pod's task alive for the configured grace window before SIGTERM arrives (previously ~0s with no preStop hook), and the ResourceManager's proactive-disconnect log line arrives within ~1s of the signal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Contributor
|
Can you file a JIRA for this change and work on fixing the CI failures before opening the PR for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
On Kubernetes, a TaskManager pod is handled the same way whether it's lost voluntarily (node
drain, spot/preemptible reclaim, rolling upgrade) or crashes outright: the ResourceManager only
learns it's gone once the heartbeat times out (50s by default), and
SIGTERMtears the processdown immediately with no chance for running tasks to make further progress.
This PR adds an opt-in
kubernetes.taskmanager.termination-grace-periodoption that gives aTaskManager pod a real, bounded grace window on voluntary eviction, and separately fixes a
long-standing
TODOinResourceManagerwhere the "notify the TaskExecutor to stop" step existedbut sent the wrong instruction.
Not a Jira ticket yet — happy to file one (or have this PR converted into the trigger for one)
based on maintainer feedback on scope/approach; wanted to put the code up for review first rather
than guess at a title before there's been any discussion.
Brief change log
kubernetes.taskmanager.termination-grace-period(newConfigOption<Duration>) +TerminationGracePeriodDecorator: setsterminationGracePeriodSecondsand apreStophook onthe TaskManager pod, never overriding a value already present in the user's pod template.
preStophook sendsSIGUSR2to the main process, whichTaskManagerRunnerroutes to thenew
TaskExecutor#prepareForTermination(): disconnects from the ResourceManager proactivelyinstead of waiting to be timed out, so the slot is freed immediately. The hook then sleeps for
most of the remaining grace period before returning (Kubernetes only sends
SIGTERMoncepreStopreturns), so already-running tasks keep making progress in the meantime.ResourceManager#closeTaskManagerConnectioncarried// TODO :: suggest failed task executor to stop itself. It already calls back into thepresumed-dead TaskExecutor via
TaskExecutorGateway#disconnectResourceManager— but that RPCtells the TaskExecutor to reconnect, which is the wrong instruction for a TaskExecutor the
ResourceManager has already unregistered and told every JobMaster to fail. Adds
TaskExecutorGateway#fenceAndStop, called here instead: tells the TaskExecutor to stop tryingto reconnect and to proactively fail its own currently-running tasks.
native_kubernetes.md; regeneratedkubernetes_config_configuration.html.Deliberately out of scope, to keep this reviewable: equivalent JobManager-side graceful
shutdown (needs its own
SIGUSR2handler first — sending it unhandled terminates a process byPOSIX default, the opposite of graceful), and a TaskManager-side proactive self-fencing/isolation
timeout for the case where the ResourceManager's own notification can't be delivered at all (a
true network partition) — that changes failure-detection timing, not just teardown messaging,
and seems like it deserves its own dev@ discussion rather than being bundled here.
Verifying this change
TerminationGracePeriodDecoratorTest(pod-spec assertions: option unset is ano-op, grace period +
preStopset correctly, neither overrides an existing pod-template value,sleep duration clamps to zero for very short grace periods), plus two new tests in
TaskExecutorTestcoveringprepareForTermination()(disconnects and does not reconnect) andfenceAndStop(fails locally-running tasks and disconnects the JobMaster connection, reusingthe existing
runJobManagerHeartbeatTestharness).flink-kubernetesmodule test suite and the fullTaskExecutorTest/TaskManagerRunnerTestclasses pass (checked for regressions since a shared gateway interface,TaskExecutorGateway, gained a new method)../mvnw spotless:checkand./mvnw checkstyle:checkclean on touched modules.kindcluster with a build of this branch: submitted acheckpointed streaming job, then
kubectl delete podon the TaskManager. Confirmed viatimestamped logs that the ResourceManager's proactive-disconnect log line lands within ~1s of
the
SIGUSR2signal (well before the 50s heartbeat timeout would fire), and that the pod (andits task) stays alive for the configured grace window before
SIGTERMarrives — comparedagainst a baseline session cluster without the option set, where
SIGTERMfires the instantkubectl delete podis issued today.Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes) — addsone new
ConfigOptiontoKubernetesConfigOptions(@PublicEvolving); purely additive,defaults to today's behavior when unset.
Documentation
Was generative AI tooling used to co-author this PR?
I worked with Claude Code through the design, implementation, and verification of this change,
including tracing the existing heartbeat/RM/TM code paths to scope it, and reviewing the kind
cluster test results (which surfaced and required fixing two real bugs during development: a
preStopscript that fired the signal but didn't actually block for the grace period, and a racecondition in one of the new unit tests). I'm responsible for the correctness of everything in this
PR regardless of tooling used.