Stop propeller before admin on single-binary shutdown - #21
Merged
Merged
Conversation
Stop Propeller and release its leader lease before shutting down Admin in single-binary mode, while preserving standalone FlyteAdmin signal handling. Assisted-by: Devin:claude-sonnet-4.5 Co-Authored-By: benchan <ben@vervious.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Tracking issue
Related to #16 (HA mode for
flyte-binary)Why are the changes needed?
In single-binary mode, propeller publishes workflow/node/task events to admin over
localhost:8089— i.e. to the admin running inside its own pod.cmd/single/start.gostarts admin, propeller and datacatalog as unordered siblings of oneerrgroup, and admin installs its own process-level SIGTERM handler. So on pod termination admin closes its listeners immediately while propeller keeps reconciling and keeps renewing the leader lease for the rest of the grace period.That window is not cosmetic. A propeller round that emits an event during it fails to publish, and the failure escalates:
In-flight executions fail during an ordinary rolling restart. This only became reachable with #16: a
Recreatesingleton never had a terminating pod overlapping a live one.Holding the lease to expiry is the second half of the same bug — it is why leader handover takes ~46s rather than the ~25s the lease config implies.
What changes were proposed in this pull request?
Ordered shutdown, scoped to single-binary mode:
cmd/single/start.go— the root command owns SIGTERM/SIGINT viasignal.NotifyContextand drives shutdown explicitly: cancel propeller first, wait for it to exit (bounded at 5s so a wedged propeller cannot eat the pod's grace period), then cancel admin and datacatalog and wait for them to return. Theerrgroupis replaced by per-service contexts plus a result channel; a service failing outside shutdown still tears the process down as before.flyteadmin/pkg/server/service.go— the gateway waits on its context when given a cancellable one, and falls back to its own signal handler otherwise. Standalone flyteadmin passescontext.Background(), so its behaviour is unchanged; only the single binary takes the new path. HTTP shutdown now uses a live timeout context rather than the cancelled one, so graceful shutdown isn't skipped.flytepropeller— newleader-election.release-on-cancelconfig, plumbed to client-go'sReleaseOnCancel. The single binary enables it, so a terminating leader releases the lease instead of renewing until it expires. Losing leadership because our own context was cancelled is now an ordinary shutdown, notlogger.Fatal.Defaults for standalone deployments are untouched.
How was this patch tested?
Unit tests (
./cmd/single,./pkg/server,./pkg/controller/...,./pkg/leaderelection/...) plus a control-first A/B on a local 3-nodekindcluster. Control is clean master6bac6f6f3, fix is this branch; identical Helm values apart from the image tag. 2 replicas, 6–8 concurrent 15-task executions in flight, thenkubectl delete podon the current lease holder (only the leader reconciles, so killing a non-leader proves nothing).connection refusedto[::1]:8089EventSinkErrorOrdering is shown positively rather than only by absent errors — the terminating pod logs
with zero propeller reconcile lines after admin stopped. The handover improvement is mechanically visible too: the terminating leader emits no new
renewTimeafter termination under the fix, versus 16 (30s grace) and 61 (120s grace) on master.Regression check on the default path: single replica,
Recreate, no PDB, pod deleted mid-execution — clean shutdown, no panic, new pod Ready with 0 restarts, admin reachable ~1s later, in-flight and post-restart executions both succeeded.Caveats worth a reviewer's attention:
terminationGracePeriodSeconds=120, applied identically to control and fix. Both sets of numbers are above.leader-election.release-on-cancelwas only exercised via the programmatic default the single binary sets; the config/chart path itself is untested.localhost:8089finds nothing even while the bug fires:localhostresolves to IPv6, so the logs read[::1]:8089.Labels
fixed
Check all the applicable boxes
Related PRs
#16
Link to Devin session: https://app.devin.ai/sessions/137e980b425d43668660198d80b6e21d
Requested by: @Vervious