Repository navigation
fc-agent times the container-state read and bounds the reap after a timeout - #1313
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
67ab9d3 to
5af8809
Compare
ejc3
changed the title
…imeout Before the restore hook runs, fc-agent reads whether fcvm-container runs (`podman inspect`) with a 10 s budget, and a read that runs out fails the restore closed. Two clones of a 128 GiB www snapshot restored at once on a 72-CPU host held the run queue at 82 to 123, and 4 of the 5 second and third clones failed there; clones restored one at a time passed. Nothing recorded how long the read takes, so neither the healthy distribution nor how far a starved read is from the budget was known. The read is now timed: when podman returns, a log line gives its exit code and elapsed_ms, and a failure carries elapsed_ms in its error. A bounded command that runs out (run_bounded, which also runs the hook) now says it hit its budget, how long the reap after the kill took (or that the wait failed, or that the pid was not reaped), fc-agent's vitals line, and the guest's blocked threads and per-CPU ticks with steal, which tell a host that is not running the vCPUs from a busy guest. The vitals are read off the tokio worker. The reap has its own 5 s bound: a child stuck in uninterruptible sleep does not die on SIGKILL, and the unbounded wait would have left the restore hanging with ingress gated instead of failing closed. Tested: make test-agent-unit (274 passed); make clippy; cargo check -p fc-agent for both musl targets. Red: with the old one-line timeout error, a_hook_past_its_budget_is_killed fails on its new assertions; a_timeout_names_a_child_that_was_not_reaped pins the other two messages.
5af8809 to
d68940f
Compare
|
@codex review |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
fc-agent now times its container-state read before the restore hook, so the 10 s budget on that read can be judged against measurements. It also bounds the reap after a bounded command times out.
Why
Before the restore hook runs, fc-agent asks
podman inspectwhetherfcvm-containerruns. The read has a 10 s budget, and running out fails the restore closed. On a 72-CPU host, I restored clones of a 128 GiB www snapshot at the same moment, and the run queue sat at 82 to 123. Four of the five second and third clones failed at this read; clones restored one at a time passed. Nothing recorded how long the read takes, either in a healthy restore or a starved one. A budget change without those numbers would be a guess.The change
elapsed_ms, and a failure carrieselapsed_msin its error.run_bounded, which also runs the hook) now says it hit its budget and how long the reap after the kill took, or that the wait failed, or that the pid was not reaped. It also carries fc-agent's vitals line and the guest's blocked threads and per-CPU ticks, steal included, which tell a host that is not running the vCPUs from a busy guest. The vitals are read off the tokio worker.Evidence
Red: with the old one-line timeout error,
a_hook_past_its_budget_is_killedfails on its new assertions.a_timeout_names_a_child_that_was_not_reapedpins the other two messages.Measured on a test host (72 CPUs) with this commit applied to main: an nginx:alpine VM booted with the new fc-agent, snapshotted, and restored five times with
fcvm snapshot run --snapshot <tag> --on-restore true:So the healthy read takes about a third of a second, against the 10 s budget. A restored clone runs the fc-agent its snapshot was booted with, so the www snapshots on that host, all booted before this change, do not log the line. www numbers come with the first snapshot prepared after it merges.