Detect plugin process exit and reflect it in piped's /healthz - #7113
Open
vipulpandey21 wants to merge 1 commit into
Open
Detect plugin process exit and reflect it in piped's /healthz#7113vipulpandey21 wants to merge 1 commit into
vipulpandey21 wants to merge 1 commit into
Conversation
Piped launches each plugin as a separate process and never checks on it again until its own shutdown. If a plugin process dies on its own (crash, OOM kill, etc.), piped keeps running, /healthz keeps saying ok, and every deployment needing that plugin fails until someone notices and restarts the pod by hand. lifecycle.Command already tracks process exit internally through stoppedCh, it was just never exposed or watched outside of shutdown. This adds Done()/Err() accessors to read that signal, watches each plugin for an unexpected exit alongside the existing shutdown path, and lets /healthz report unhealthy plugins so the livenessProbe and readinessProbe already pointed at /healthz in the shipped manifests can actually restart the pod. Signed-off-by: Vipul Subhash Pandey <vipulpandey7917@gmail.com>
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 this PR does:
Makes piped actually notice when a plugin's process dies on its own, and makes
/healthzsay so, instead of only reacting when piped itself is shutting down.Why we need it:
Right now, if a plugin process crashes (or gets OOM-killed, or anything else that kills it unexpectedly), piped does not notice at all. It keeps running,
/healthzkeeps sayingok, and any deployment that needs that plugin just keeps failing quietly. Kubernetes is already set up to restart piped whenever/healthzsays something is wrong, so this fix mostly just makes that existing health check tell the truth. Full explanation with code references is in #7112.What changed, in plain terms
pkg/lifecycle/binary.go— piped already had a way to know when a plugin process exits (a channel that closes when it happens), it just was not exposed outside that file. I added two small getters:Done()— a channel you can wait on, closes when the process exits, for any reason.Err()— tells you the error it exited with, if any.Nothing existing changes, this just exposes something that was already being tracked.
pkg/app/pipedv1/cmd/piped/pluginhealth.go(new file) — a very small helper that keeps a list of "plugins that are currently known to be dead". Nothing fancy, just a map with a lock around it, and its own tests.pkg/app/pipedv1/cmd/piped/piped.go:/healthznow checks that list. If any plugin is dead, it returns a503and says which plugin(s) are down, instead of always sayingok.I did not make piped try to automatically restart a crashed plugin. If a plugin keeps crashing, silently restarting it over and over could hide a real bug from whoever is running it. Instead, this just makes
/healthzhonest, and lets Kubernetes do what it is already set up to do: restart the whole pod when something is wrong.How I tested it
Added tests for both new pieces:
pkg/lifecycle/binary_test.go— checks that the newDone()/Err()correctly reflect a normal exit, a crash (non-zero exit code), and a graceful stop, using real short-lived processes the same way the existing tests in that file already do.pkg/app/pipedv1/cmd/piped/pluginhealth_test.go— checks marking a plugin unhealthy, marking the same one twice (should not duplicate), multiple plugins, and that it is safe if many goroutines touch it at once.Also ran:
go test -raceon both packages — passedgo veton both packages — cleango build ./pkg/app/pipedv1/... ./cmd/...— to make sure nothing else brokegofmt— cleanI did not add a full end-to-end test that starts a real plugin binary, kills it, and checks
/healthzover HTTP. The function that wires up the whole piped agent (run()inpiped.go) did not have any test coverage before this change either, so instead of trying to add the first big integration test for it in this same PR, I kept all the new logic in small pieces that are fully testable on their own (Command.Done/Err,pluginHealth).Which issue(s) this PR fixes:
Fixes #7112
Does this PR introduce a user-facing change?:
/healthznow returns503(and lists which plugin) if a configured plugin's process has died unexpectedly, instead of always returning200 ok. Anyone using the standard Helm chart or quickstart manifests already has a liveness/readiness probe pointed at/healthz, so this means the pod now actually gets restarted automatically in that situation, instead of silently staying broken.