fix: stop reporting restarts that never happened - #217
Merged
Conversation
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.
restart_countno longer reports restarts that never happened.The bug
apply_vm_start_failureassignedrestart_count = MAX_RESTART_COUNTfor every terminal error, using the counter as a "stop reconciling" marker rather than a tally.So a deployment refused by host-memory admission — checked before anything is spawned — displayed:
Five restarts, for a workload where no process was ever created. Visible in the CLI, the API and the dashboard, sending anyone diagnosing it to look for an instability that never existed.
Why the marker was there, and why it is not needed everywhere
For statuses the scheduler still reconciles (
ConfigError,ImagePullBackOff,CreateContainerError), exhausting the budget genuinely is what stops the retries — remove it and a permanent failure retries forever.But a few terminal statuses are already excluded from the scheduler's filter by status alone (
InsufficientResources,Failed,CrashLoopBackOff,Completed). For those the write bought nothing and only made the field lie.scheduler_skips_by_status()now draws that line, and a test asserts it stays in sync with the scheduler's filter — a status added back to that filter without updating the helper would otherwise retry forever with no budget to stop it.The same one-line change applies to containerd's
handle_create_error, narrowed toInsufficientResourcessince its other terminal statuses are still reconciled.Behaviour, verified on real microVMs
The distinction the field is supposed to express is restored.
Also
tests/e2e/cloud-hypervisor/packer/*.logis now gitignored — a regenerated 1200-line serial console dump that kept getting swept into unrelated commits.Testing
827 unit tests, clippy clean. Three existing classifier tests encoded the old behaviour and were updated, including
every_start_failure_moves_the_restart_counter, whose invariant is now stated correctly: a failure must either move the counter or land outside the reconcile filter. Verified end to end witht14_crashloop(still 5) andt16_insufficient_memory(now 0).