Repository navigation
fix(cloudhypervisor): stop create race spawning duplicate VMM - #1150
Merged
Merged
Conversation
CH State() returned Pending while the cloud-hypervisor process was already
alive: during the pre-socket-bind window and while Info() reports "Created"
(guest still booting, long under nested virt). The reconcile create-gate
(ShouldDo == Pending) then re-fired Create(), spawning a second
cloud-hypervisor on the same --api-socket ("Address in use" / tap
"Resource busy") and ensureState() unlinked the socket from under the first
healthy process, orphaning it until retries exhausted -> never CREATED.
The double-spawn requires State() to return exactly (Pending, nil). Fix:
once our pidfile process is alive, State() never returns Pending, mirroring
the firecracker provider:
- socket not yet bound -> Running
- Info() transport error -> Running (trust liveness, log warn)
- Info() Running/Created -> Running
- Info() Shutdown/other -> Unknown+err (post-boot only, safe)
Belt-and-suspenders: Create() early-returns when the pidfile PID is a live
process, so a stray re-create can neither double-spawn nor orphan the socket.
Also fixes two wrong-error-variable typos (startErr, delErr).
Adds provider/create tests covering both race windows.
Fixes #999
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for flintlock-docs canceled.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an intermittent Cloud Hypervisor create/reconcile race in flintlock where the create step could re-run while the VMM process was already alive, causing duplicate cloud-hypervisor instances to contend for the same API socket and leaving the “winning” process orphaned.
Changes:
- Adjust
cloudhypervisorproviderState()so that once the pidfile exists and the process is alive, it never reportsPending(mapping socket-not-bound and transientInfo()failures toRunning, and treatingCreatedasRunning). - Add a belt-and-suspenders guard in
Create()to no-op when the pidfile points to a live process, preventing double-spawn and avoiding socket deletion inensureState(). - Add focused unit tests covering the new
State()mappings and theCreate()guard behavior; also fix two wrong error-variable usages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| infrastructure/microvm/cloudhypervisor/provider.go | Prevents State() from returning Pending once the VMM pid is alive, closing the duplicate-spawn windows. |
| infrastructure/microvm/cloudhypervisor/create.go | Adds a live-process guard to skip re-create and fixes error-variable typos. |
| infrastructure/microvm/cloudhypervisor/provider_test.go | Adds State() test matrix covering pid/socket/Info() permutations and CH state mapping. |
| infrastructure/microvm/cloudhypervisor/create_test.go | Verifies Create() does not delete an existing socket when a live VMM is already running. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+44
| // Never re-create over a live cloud-hypervisor. The State()-based create guard | ||
| // should already prevent this, but this is a belt-and-suspenders check against a | ||
| // double-spawn (Address in use) or orphaning a running process by deleting its | ||
| // socket in ensureState. | ||
| if pidExists, _ := afero.Exists(p.fs, vmState.PIDPath()); pidExists { | ||
| if pid, perr := vmState.PID(); perr == nil { | ||
| if live, _ := process.Exists(pid); live { | ||
| logger.Debug("cloud-hypervisor already running for this microvm, skipping create") | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
| } |
The belt-and-suspenders create guard swallowed errors from afero.Exists, vmState.PID() and process.Exists. A transient FS error would fall through into ensureState(), which deletes an existing socket and risks orphaning a live VMM. Return early with context on each, matching State() and Delete(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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.
Fixes #999
Problem
With
provider: cloudhypervisor, VM creates intermittently (~1 in 3, worse under nested virt/load) never reachCREATED;cloudhypervisor.stderrfills withAPI socket "…/cloudhypervisor.sock" is already in use by another running instance. The winning guest boots and networks fine — it's a control-plane reconcile bug, not guest/boot.Root cause (traced in #999 (comment)):
startCloudHypervisordoescmd.Start()and returns before CH binds its API socket or finishes boot.ShouldDo()(gated onState() == Pending) right afterDo().State()returnedPendingduring two windows while the process was already alive: pre-socket-bind (!sockExists), and boot (Info() == "Created", long under nested virt).Create()→ a second cloud-hypervisor on the same--api-socket(Address in use/ tapResource busy), andensureState()unlinked the socket out from under the first healthy process → permanent orphan → retries exhaustMaximumRetry→FailedState.Firecracker is immune because its
State()treats "pidfile present + process alive" asRunning.Fix
The double-spawn requires
State()to return exactly(Pending, nil). So: once our pidfile process is alive,State()never returnsPending(mirrors firecracker), keepingInfo()for richer reporting:RunningInfo()transport errorRunning(trust liveness, log warn)Info=Running/CreatedRunningInfo=Shutdown/ otherUnknown+ err (post-boot only, safe —Unknown≠Pending)This closes both race windows.
Belt-and-suspenders:
Create()early-returns when the pidfile PID is a live process, so a stray re-create can neither double-spawn nor orphan the socket. Also fixes two pre-existing wrong-error-variable typos (startErr,delErr).Tests
New
provider_test.go— 7State()cases (real short-lived process for liveness, fake CH HTTP-over-unix server forInfostates) covering both race windows and theInfomapping. Newcreate_test.go— theCreate()guard leaves a live VM's socket intact.go build ./...,go vet, andgo test ./core/... ./infrastructure/...(159 passed) all green. End-to-end repro under nested KVM still needs a real host.🤖 Generated with Claude Code