refactor(controller): collapse fork reconcile status round-trips off the hot path#859
refactor(controller): collapse fork reconcile status round-trips off the hot path#859stubbi wants to merge 2 commits into
Conversation
…the hot path
The husk fork reconcile persisted ForkSnapshotTaken in its OWN r.Status().Update
before the child loop, on every path, so a fork spent an apiserver plus etcd
round-trip the client waits on purely to record intermediate progress.
Fold that standalone write into the unconditional pass-boundary write on the
NEW-POD path, where it is redundant: no child is ACTIVATED in the snapshot pass
there (the child pods are only CREATED, and activated a LATER pass after the
pass-boundary write has already persisted the flag), so the snapshot pass drops
from 2 status round-trips to 1.
The CO-LOCATION write is KEPT because it is load-bearing: spawnForkChildInSourcePod
ACTIVATES the child inside the source pod in the same pass, before its per-spawn
status write commits, so ForkSnapshotTaken must be durable BEFORE that first
activation. Folding it there would let a failed per-spawn write or a crash in that
window leave an active child while the flag is not durable, and a later pass would
re-snapshot under an already-restored child and split a multi-replica fork into an
incoherent point. The write is now made only when the pass will actually co-locate
a child (spawnInSourcePod and remaining budget), preserving the exact pre-change
ordering on that path.
Tests (in-package, via a status-write counting client):
- the new-pod snapshot pass writes status exactly once and still reaches Ready
with the flag and timing anchors durable and the snapshot taken once;
- the co-located path has ForkSnapshotTaken durable in the store at the first
spawn-vm activation and still reaches Ready (the crash-coherence guard).
Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChanges reconcileHuskFork's handling of the ForkSnapshotTaken status flag from an unconditional Status().Update immediately after a successful snapshot to a conditional update triggered only when co-location activation will occur in the current pass. Adds unit tests verifying status-write counts and durability across reconcile passes. ChangesFork Status Write Consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant reconcileHuskFork
participant StatusWriter
participant spawnVM
Test->>reconcileHuskFork: pass 1 (NEW-POD, snapshot succeeds)
reconcileHuskFork->>reconcileHuskFork: set ForkSnapshotTaken=true
reconcileHuskFork->>StatusWriter: Status().Update (pass-boundary write)
Test->>reconcileHuskFork: pass 2 (child ready)
reconcileHuskFork->>StatusWriter: Status().Update (single additional write)
Test->>reconcileHuskFork: reconcile (CO-LOCATION path)
reconcileHuskFork->>reconcileHuskFork: set ForkSnapshotTaken=true
alt spawnInSourcePod and budget remaining
reconcileHuskFork->>StatusWriter: Status().Update (durable before activation)
reconcileHuskFork->>spawnVM: spawn co-located child
else
reconcileHuskFork->>StatusWriter: deferred pass-boundary Status().Update
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
CI status: all checks this diff can affect are green (go-test, go-lint, python-test, sdk-conformance/examples, typescript-sdk, docker-build, CodeQL, CodeRabbit, dco, title/body/no-dashes). firecracker-test is RED, but it is a pre-existing failure on main unrelated to this change:
Nothing in this PR needs to change for firecracker-test; it will go green once the pre-existing internal/husk KVM prewarm issue is resolved on main. |
|
Review verdict: HOLD, and honestly the simplest outcome is closing this one. The fold breaks the crash-coherence invariant the standalone write existed for, on the new-pod recovery path. Trace: pass 1 takes snapshot S1, sets ForkSnapshotTaken in memory only, creates child pods, then the end-of-pass status write fails (status conflicts on the Sandbox CR are realistic) or the controller crashes or loses leadership. The flag is not durable but child pods now exist. Pass 2 sees !ForkSnapshotTaken and re-snapshots the source (re-pausing a pauseSource=false source a second time and overwriting mem/vmstate), and a pass-1 child can be Running+Ready by then, so it activates in the same pass as the re-snapshot: the exact configuration the hazard comment at sandboxfork_controller.go:814-821 exists to prevent. With a second boundary failure you get children active on different snapshot generations. The pre-change code was immune by construction: the flag was durable before any child pod existed. The new tests cover only the clean pass, not this recovery window. The trade also buys little: the saved write is on the cold new-pod path, and the #868 measurements show the client-visible latency lives elsewhere. On top of that the branch conflicts with main exactly in the rewritten hunk (the #860 per-stage instrumentation wraps this very write, and #874's RequireMemFile landed in the same block), and the govulncheck red is just the pre-#862 toolchain on the branch. To make it mergeable it would need: the invariant restored (refuse to re-snapshot when child pods already exist, or only fold when no child artifacts precede the durable flag), a failed-final-write recovery test, and a rebase re-woven around #860/#874. Given the first point likely collapses it back to the status quo, recommend closing. |
1 participant
Thinking Path
Linked Issues or Issue Description
No tracked issue. Problem: the husk fork reconcile persisted
ForkSnapshotTakenin its ownr.Status().Updatebefore the child loop on every path, adding an apiserver plus etcd round-trip the fork client waits on purely to record intermediate progress. On the new-pod path that write is redundant with the pass-boundary write.What Changed
reconcileHuskFork: the standaloneForkSnapshotTakenwrite is no longer unconditional. It is folded into the pass-boundary write on the new-pod path (where the child pods are only CREATED in the snapshot pass and activated a later pass), and KEPT on the co-location path (where the child is ACTIVATED in the same pass before its per-spawn write commits, so the flag must be durable first). The write now fires only whenspawnInSourcePod && coLocationBudgetRemaining > 0, preserving the exact pre-change ordering on that path.fork_reconcile_roundtrips_test.go) drivingreconcileHuskForkthrough a status-write counting client:TestHuskForkNewPodPathFoldsSnapshotWrite: the new-pod snapshot pass writes status EXACTLY ONCE (was twice), the flag and timing anchors stay durable, the snapshot is taken once, and the fork reaches Ready on the next pass.TestHuskForkCoLocatedSnapshotDurableBeforeActivation:ForkSnapshotTakenis durable in the store at the FIRST co-locatedspawn-vmactivation (the crash-coherence guard), and the fork reaches Ready.Verification
go build ./...go test ./internal/controller/(full envtest suite green on the pre-image; the two new tests pass; setup-envtest 1.31)TestHuskForkNewPodPathFoldsSnapshotWriteandTestHuskForkCoLocatedSnapshotDurableBeforeActivationpassgolangci-lint run --timeout=5m ./internal/controller/cleanGOOS=linux golangci-lint run --timeout=5m ./internal/controller/cleanRisks
Low risk. The change is confined to K8s status bookkeeping; it does not touch the husk RPC calls (ForkSnapshot / SpawnVM / Activate), fork correctness, isolation, or the leak-prevention paths. The co-location (hot) path keeps its pre-activation flag write, so its crash coherence is byte-for-byte the pre-change behavior; the only behavioral change is on the new-pod path, where no child is activated in the snapshot pass so the deferred flag is always durable before any later-pass activation. Status.Phase transitions and observed generation semantics for watchers are unchanged; fewer status writes reduce, not increase, conflict pressure. The saving is one round-trip on the new-pod snapshot pass (not the co-located hot path), so no wall-clock number is claimed; the operator measures on prod.
Model Used
Claude Opus 4.8 (claude-opus-4-8), extended thinking. Agent-assisted implementation and verification.
Checklist
Summary by CodeRabbit