fix(phase2): address review findings (log casing, reserved/resident split, tests) - #26
Merged
Merged
Conversation
…split, tests Standards: capitalize the 5 new log messages per repo convention. Spec (Task 4): make the reserved/resident split explicit. sumUsedResources now returns per-node resident (excludes Suspended — the overcommit) and reserved (includes Suspended, for oversubscription visibility). Schedule gates on resident only (a reserved gate would cancel overcommit) and logs reserved + an oversubscribed flag. Resolves the plan's contradictory 'filter on both'. Tests: sumUsedResources resident/reserved split; post-resume VTEP registration (the network-restore risk) asserting ImpNetwork.status.vtepTable after resume.
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
1 participant
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.
Addresses the two-axis review of the Phase 2 PRs (#23/#24/#25).
Standards
firecracker_driver.go,reconciler.go).Spec — Task 4 reserved/resident split (the WRONG finding)
The plan asked to split reserved vs resident; the original impl just excluded Suspended from all accounting. Now explicit:
sumUsedResourcesreturns per-node resident (excludes Suspended — memory freed) and reserved (includes Suspended — retains a node claim).Schedulegates on resident only (that IS the overcommit; a reserved gate would cancel it) and logsreserved+ anoversubscribedflag for visibility.Spec — missing tests
sumUsedResources: asserts resident = running-only, reserved = running+suspended, terminating excluded.ImpNetwork.status.vtepTablegets the entry (NodeIP + VMIP) — the ≥15 network-restore risk.Note (false positive)
The review's "NAT absent from resume" finding was a false positive: NAT is set up inside
Driver.Start'ssetupNetwork(EnsureNetwork+SetupVM+EnsureNAT), whichhandleResumingcalls — same as a cold boot. No change needed.