feat(xapi): add an XCP-ng/XenServer backend over XAPI JSON-RPC - #1
Merged
Conversation
clawk runs sandboxes on local hypervisors: vz on macOS, firecracker on Linux. Neither reaches a pool, so anyone whose compute already lives on XCP-ng has to stand up separate hardware to use it. This adds a machine.Backend that talks to a pool instead. Three constraints shaped it, and each rules out an easier design: Nothing is installed in dom0 — not a binary, not a kernel, not a config file. A tool that needs files placed in dom0 is unshippable to XCP-ng users no matter how much simpler it makes the code, so DirectKernel specs are honoured by synthesising a read-only boot VDI (ESP + GRUB + vmlinux + initrd) as xvda with the OCI rootfs as xvdb, rather than by pointing PV_kernel at a dom0 path. Egress filtering stays out of this process. gvproxy runs in a per-host gateway VM with a VIF on the sandbox's private no-PIF network, so the guest's only route out passes through it. That keeps clawk's actual security property — root in the guest cannot reconfigure the filter — without a dom0-resident daemon. Caps.UserModeNet, TAPNet and UnixgramNet are consequently false and Spec.Net variants are rejected. Xen has no virtio-vsock, so Machine.VSock returns ErrVSockUnsupported and the control path is TCP over a management VIF via ControlDialer. This is the backend's largest divergence from the machine contract and the part most likely to need discussion. No virtio-fs either, so Spec.Shares is rejected; firecracker already reports VirtioFS: false, so there is precedent. The transport is XAPI's own JSON-RPC endpoint spoken with net/http and encoding/json, which adds no dependency to go.mod. A generated XenAPI binding would drag in thousands of types to serve the ~20 calls in Client, and the raw VDI import is a separate HTTP PUT outside the RPC surface either way, so the binding saves nothing on the hard part. Xen Orchestra's API is nicer and brings its ACLs, but puts XO in the path of a tool that otherwise talks to any pool unaided. The Client interface keeps that choice reversible. Live so far: session.login_with_password, VM.get_power_state, VM.get_by_name_label, session.logout. Every other Client method returns errNotImplemented, as do Create, Destroy and checkpoint Restore. They fail loudly rather than returning a zero value that reads as success — a failing Create is the honest current state. Suspend and snapshot diverge from the documented contract: it says state is written into a caller-owned directory, but XAPI keeps memory images in the SR, so this writes a small JSON pointer file there instead. Whether that reading is acceptable, or the interface should grow an opaque-handle variant, is a question for upstream. Verified against an XCP-ng 8.3 pool: login, logout, name-label lookup and power-state read all round-trip, with the enum confirmed in both Running and Halted. Those calls live in manual_test.go and run only when TEST_XAPI_POOL is set; the httptest wire tests cover the same paths in CI, where no pool exists. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds an XAPI client and JSON-RPC transport, registers an XAPI machine backend with lifecycle and guest-control operations, adds snapshot pointer handling and tests, and introduces a configurable ChangesXAPI backend
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant SmokeXAPI
participant XAPIBackend
participant JSONRPCClient
participant XCPPool
participant GuestAgent
SmokeXAPI->>XAPIBackend: Configure and create machine
XAPIBackend->>JSONRPCClient: Create VM and query power state
JSONRPCClient->>XCPPool: Send JSON-RPC request
XCPPool-->>JSONRPCClient: Return session or VM state
JSONRPCClient-->>XAPIBackend: Return XAPI result
XAPIBackend-->>SmokeXAPI: Return machine state
SmokeXAPI->>GuestAgent: Open TCP control connection
GuestAgent-->>SmokeXAPI: Accept connection
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
8 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
machine/xapi/client.go (2)
125-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueWrite the pointer file atomically.
writePointerwrites in place. If the process stops during the write, the file is truncated andreadPointerfails, so the suspended VM in the SR becomes unreachable through this directory. Write to a temporary file in the same directory, then rename it.♻️ Proposed change
func writePointer(dir string, p pointer) error { if err := os.MkdirAll(dir, 0o700); err != nil { return err } b, err := json.Marshal(p) if err != nil { return err } - return os.WriteFile(filepath.Join(dir, pointerName), b, 0o600) + final := filepath.Join(dir, pointerName) + tmp := final + ".tmp" + if err := os.WriteFile(tmp, b, 0o600); err != nil { + return err + } + return os.Rename(tmp, final) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/client.go` around lines 125 - 134, Update writePointer to write the marshaled pointer to a temporary file created in the same directory, then atomically rename it to pointerName. Preserve the existing directory creation, permissions, and error propagation, and clean up the temporary file if writing or renaming fails.
110-112: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn an explicit nil
Clienton error.
newJSONRPCClientreturns a*jsonrpcClient. When it fails, the returned pointer is nil, but theClientinterface value thatNewClientproduces is non-nil. Callers that checkcl != nilinstead oferr != nilthen dereference a nil pointer. The current caller inmachine/xapi/xapi.gocheckserr, so this is latent only.♻️ Proposed change
func NewClient(ctx context.Context, c Config) (Client, error) { - return newJSONRPCClient(ctx, c) + cl, err := newJSONRPCClient(ctx, c) + if err != nil { + return nil, err + } + return cl, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/client.go` around lines 110 - 112, Update NewClient to capture the result of newJSONRPCClient and, when it returns an error, explicitly return a nil Client interface alongside that error; preserve the successful client and nil error path.machine/xapi/manual_test.go (1)
3-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the example command path.
The package lives at
machine/xapi. The example runsgo test ./xapi, which fails from the repository root. Use the module-relative path.♻️ Proposed change
-// go test ./xapi -run TestPool -v +// go test ./machine/xapi -run TestPool -vIf
machine/is a separate module, state that the command runs from insidemachine/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/manual_test.go` around lines 3 - 18, Update the manual test command in the harness comment to use the module-relative package path for machine/xapi, or explicitly state that it must be run from inside the machine module if machine is separate. Keep the remaining environment variables and test flags unchanged.machine/cmd/smoke-xapi/main.go (1)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport all missing flags, in a stable order.
The loop iterates a map, and Go randomizes map iteration order. When two required flags are missing,
log.Fatalfnames a different one on each run. Use an ordered slice and collect every missing name.♻️ Proposed change
- for name, v := range map[string]string{"url": *url, "sr": *srUUID, "mgmt-net": *mgmtNet, "kernel": *kernel} { - if v == "" { - log.Fatalf("-%s is required", name) - } - } + required := []struct { + name string + value string + }{ + {"url", *url}, {"sr", *srUUID}, {"mgmt-net", *mgmtNet}, {"kernel", *kernel}, + } + var missing []string + for _, r := range required { + if r.value == "" { + missing = append(missing, "-"+r.name) + } + } + if len(missing) > 0 { + log.Fatalf("required flags missing: %s", strings.Join(missing, ", ")) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/cmd/smoke-xapi/main.go` around lines 60 - 64, Replace the map iteration in the required-flag validation with an ordered slice containing url, sr, mgmt-net, and kernel; collect every flag whose value is empty, then report all missing flag names together in that stable order instead of exiting on the first missing value.machine/xapi/xapi_test.go (1)
153-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack client closure in the fake pool.
fakePool.Closereturns nil and records nothing. No test asserts that the backend logs out. That gap hides the missingv.cl.Close()call flagged inmachine/xapi/xapi.go. Add aclosed boolfield, set it inClose, and assert it afterDestroyonceDestroylands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/xapi_test.go` around lines 153 - 162, Update fakePool and its test helper: add a closed bool field, set it in fakePool.Close, and assert it is true after Destroy completes. Extend the relevant test around withFakePool to verify Destroy invokes the client closure.machine/xapi/client_jsonrpc.go (2)
182-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPlan for session expiry.
XAPI sessions expire after an idle or absolute timeout, and the pool master invalidates all sessions on restart.
sessionCallthen fails withSESSION_INVALIDfor the rest of the client's life, because nothing re-logs in. A sandbox VM can outlive the session. Store the credentials onjsonrpcClientand retry one time after aSESSION_INVALIDfailure, or document that callers must recreate the client.I can generate the re-login wrapper if you want it in this PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/client_jsonrpc.go` around lines 182 - 190, Update jsonrpcClient and sessionCall to retain the credentials needed for authentication, detect a SESSION_INVALID response from c.call, re-authenticate once, refresh the stored session, and retry the original request exactly once. Preserve existing behavior for other errors and avoid retrying repeatedly; if re-login cannot succeed, return the resulting authentication error.
74-87: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
MinVersionexplicitly on the TLS config.The Go client default minimum is TLS 1.2, so this is not an exploitable gap. An explicit
MinVersiondocuments the floor and protects against a future default change.♻️ Proposed change
TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: c.InsecureTLS, //nolint:gosec // opt-in; see Config.InsecureTLS },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/client_jsonrpc.go` around lines 74 - 87, Update the TLS configuration in the HTTP client construction to set TLS MinVersion explicitly to TLS 1.2, while preserving the existing InsecureSkipVerify behavior and transport timeouts.Source: Linters/SAST tools
machine/xapi/xapi.go (1)
385-399: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider restoring into
v.refinstead of the pointer's VM ref.
Restoreresumes the VM named in the pointer file. That ref belongs to the VM that wrote the file, which is not necessarily thisvminstance. A caller that restores a snapshot into a freshMachinethen drives a VM that this instance does not own, becausev.refstays empty andStart,State, andPausekeep using it. Either adoptp.VMintov.refand setcreated, or reject a pointer whoseVMdoes not matchv.ref.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/xapi.go` around lines 385 - 399, Update vm.Restore to ensure the restored VM is owned by this instance: adopt p.VM into v.ref and mark created before resuming, or reject pointers whose VM ref differs from an already-set v.ref. Keep subsequent Start, State, and Pause operations aligned with the ref established by Restore.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machine/cmd/smoke-xapi/main.go`:
- Around line 124-190: Move the current smoke-test body, including the
Create/Start/waitState/control-channel and optional-interface operations, into a
run() error function so its deferred Destroy and work-directory cleanup execute
on failures. Replace every log.Fatalf within run() with a wrapped error return
using the existing contextual messages, and have main invoke run() through
log.Fatal(run()).
- Line 47: Change the password flag initialization in main so its default is
empty rather than os.Getenv("XAPI_PASSWORD"), preventing flag.PrintDefaults from
exposing the secret. After flag.Parse(), resolve XAPI_PASSWORD into the password
value only when no command-line password was provided, preserving explicit flag
precedence.
In `@machine/xapi/client_jsonrpc_test.go`:
- Around line 38-61: Update fakeRPC.serve to use assert rather than require,
return an HTTP error when request decoding fails, and remove handler-side fatal
assertions. Add mutex protection for f.reqs, including appends and reads through
methods() or other test accessors, then move request validation assertions to
the test goroutine after the handler completes.
In `@machine/xapi/xapi.go`:
- Around line 169-174: Update the VM lifecycle around New, Create, and Destroy
so every client returned by dialClient is closed through v.cl.Close(). Ensure
Destroy performs the logout for normally torn-down machines, and ensure Create
invokes the same cleanup when creation fails, including partial-failure paths,
while preserving the existing error handling.
- Around line 344-382: Use the existing liveRef helper in Control, Pause,
Resume, Suspend, Snapshot, and Stop to read v.ref under v.mu and enforce the
created/destroyed lifecycle checks before invoking XAPI operations; preserve
each method’s existing behavior after obtaining the validated reference,
including pointer writing for Suspend and Snapshot.
- Around line 295-306: Update vm.Destroy so v.destroyed is assigned only after
teardown completes successfully; preserve the existing early return for
already-destroyed VMs, but leave the flag false when teardown returns an error
so retries remain possible and State does not report a live VM as destroyed.
---
Nitpick comments:
In `@machine/cmd/smoke-xapi/main.go`:
- Around line 60-64: Replace the map iteration in the required-flag validation
with an ordered slice containing url, sr, mgmt-net, and kernel; collect every
flag whose value is empty, then report all missing flag names together in that
stable order instead of exiting on the first missing value.
In `@machine/xapi/client_jsonrpc.go`:
- Around line 182-190: Update jsonrpcClient and sessionCall to retain the
credentials needed for authentication, detect a SESSION_INVALID response from
c.call, re-authenticate once, refresh the stored session, and retry the original
request exactly once. Preserve existing behavior for other errors and avoid
retrying repeatedly; if re-login cannot succeed, return the resulting
authentication error.
- Around line 74-87: Update the TLS configuration in the HTTP client
construction to set TLS MinVersion explicitly to TLS 1.2, while preserving the
existing InsecureSkipVerify behavior and transport timeouts.
In `@machine/xapi/client.go`:
- Around line 125-134: Update writePointer to write the marshaled pointer to a
temporary file created in the same directory, then atomically rename it to
pointerName. Preserve the existing directory creation, permissions, and error
propagation, and clean up the temporary file if writing or renaming fails.
- Around line 110-112: Update NewClient to capture the result of
newJSONRPCClient and, when it returns an error, explicitly return a nil Client
interface alongside that error; preserve the successful client and nil error
path.
In `@machine/xapi/manual_test.go`:
- Around line 3-18: Update the manual test command in the harness comment to use
the module-relative package path for machine/xapi, or explicitly state that it
must be run from inside the machine module if machine is separate. Keep the
remaining environment variables and test flags unchanged.
In `@machine/xapi/xapi_test.go`:
- Around line 153-162: Update fakePool and its test helper: add a closed bool
field, set it in fakePool.Close, and assert it is true after Destroy completes.
Extend the relevant test around withFakePool to verify Destroy invokes the
client closure.
In `@machine/xapi/xapi.go`:
- Around line 385-399: Update vm.Restore to ensure the restored VM is owned by
this instance: adopt p.VM into v.ref and mark created before resuming, or reject
pointers whose VM ref differs from an already-set v.ref. Keep subsequent Start,
State, and Pause operations aligned with the ref established by Restore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 467bc5a4-efad-460e-ab51-3835a7878393
📒 Files selected for processing (7)
machine/cmd/smoke-xapi/main.gomachine/xapi/client.gomachine/xapi/client_jsonrpc.gomachine/xapi/client_jsonrpc_test.gomachine/xapi/manual_test.gomachine/xapi/xapi.gomachine/xapi/xapi_test.go
Six defects, none of which the fake-pool tests could have caught, because every one of them lives in a path no test drove. Destroy marked the machine destroyed and then returned an error. A second Destroy therefore returned nil having torn nothing down, and State reported StateDestroyed for a VM still running on the pool. That is precisely the plausible-looking lie the rest of this package exists to refuse, so it cut deeper than a stray assignment. Teardown is split into its own method to give Destroy a real success path — mark destroyed, then log out — rather than a hypothetical one. Nothing ever called Client.Close, so every Machine held a pool session until it timed out. XAPI caps concurrent sessions per pool, which makes this a shared resource consumed rather than a local leak a restart clears. The logout is on Destroy's success path only: once teardown is implemented a failed Destroy is worth retrying, and closing first would hand the retry a dead session and bury the original error. Until teardown lands, Destroy always fails and so never reaches the logout — the session still leaks. That is the honest consequence of an unimplemented Destroy, recorded in the doc comment rather than hidden. Create wrote v.ref under the mutex while Control, Pause, Resume, Suspend and Snapshot read it without one, and those five also skipped the created/destroyed checks Start performed, so calling them before Create sent the empty ref to the pool and got HANDLE_INVALID back instead of ErrInvalidState. A liveRef helper now does both jobs in one place and every ref-touching call goes through it. In the smoke tool, the password flag defaulted to $XAPI_PASSWORD, and flag.PrintDefaults prints any default that is not the zero value — so -h wrote the password to stderr. The lookup moves after Parse. Separately log.Fatalf reached os.Exit, skipping the Destroy and RemoveAll defers; the body moves into run() error so failure paths clean up. Inert today only because Create fails before the Destroy defer is registered. The wire tests asserted with require on the httptest handler goroutine. require.FailNow calls runtime.Goexit, which unwinds that goroutine mid-response, so a mismatch surfaced as a client-side decode error rather than the assertion that explains it. They use assert now, the recorded requests are mutex-guarded behind an accessor, and the name-label test builds one pool per case instead of reassigning a captured slice the handler reads. Both regressions are covered and both were confirmed to fail against the reintroduced defects before being kept. Verified with go test -race. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
This was referenced Aug 3, 2026
NOTES.md gains the six defects the review on PR #1 found, why each mattered, and what is still open as issues #3-#8. The point of writing them down is the pattern rather than the list: every one lived on a path no test drove, which is why a fake-pool suite that passes says less about this backend than it appears to. It also records the caveat that is easiest to lose. The session leak is not gone — closeClient runs on Destroy's success path and Destroy is still a stub that always fails, so the session still leaks. The fix is correct for when teardown lands. Its review thread has been resolved, which removed the last visible marker, so the note is now the only thing carrying it. Build-order step 2 gains the conditions it needs rather than just a warning. It is the first code that writes to a pool, and it wants an empty, uncontended, file-based SR: VDI.clone full-copies on LVM, which defeats the per-sandbox copy-on-write model the whole design rests on. Which host to use, and who else is using it, stays out of this file. .gitignore gains CONTEXT.md and .env. This repo is public, and the session state names lab hosts, SR uuids and who owns which VM. Another session pushes to this branch, so an accidental `git add -A` is a real way for that to become public rather than a hypothetical one. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
machine/xapi/xapi_test.go (1)
295-326: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd a post-
Createteardown-failure test.This test never calls
Create, so it does not exercise teardown for a live VM.Statefollows the!v.createdbranch, and no remote VM exists to retry.When
Createis implemented, inject a teardown failure after VM creation. Assert that the firstDestroypreserves the VM state and session. Then clear the failure and verify that a retry tears down the VM and closes the client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/xapi_test.go` around lines 295 - 326, Update TestFailedDestroyDoesNotMarkDestroyed to call Create successfully, inject a teardown failure after the remote VM is created, and assert the first Destroy preserves the VM state and pool session. Clear the injected failure, retry Destroy, and verify the VM is removed and the client/session is closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@machine/xapi/xapi_test.go`:
- Around line 295-326: Update TestFailedDestroyDoesNotMarkDestroyed to call
Create successfully, inject a teardown failure after the remote VM is created,
and assert the first Destroy preserves the VM state and pool session. Clear the
injected failure, retry Destroy, and verify the VM is removed and the
client/session is closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8f7bc83-08c3-4a8c-bf6d-78ad75c1d25e
📒 Files selected for processing (6)
.gitignoreNOTES.mdmachine/cmd/smoke-xapi/main.gomachine/xapi/client_jsonrpc_test.gomachine/xapi/xapi.gomachine/xapi/xapi_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- machine/cmd/smoke-xapi/main.go
- machine/xapi/client_jsonrpc_test.go
- machine/xapi/xapi.go
Three review findings, issues #4, #5 and #6. The one that changes behaviour is the session. XAPI expires sessions, and the transport logged in once and held that ref for its lifetime, so a pool that timed one out left every subsequent call failing forever. That matters more here than on a local hypervisor: a Machine outlives the operations performed on it, and an idle sandbox will sit past the pool's timeout as a matter of course rather than as an edge case. The issue offered documenting the limitation instead, which reads the normal case as the exception. sessionCall now detects SESSION_INVALID, logs in again and retries once. Once, not in a loop, so a pool refusing every login produces an error rather than a spin. A generation counter behind a dedicated login mutex keeps concurrent callers to one re-login between them instead of one each, and Close marks the client closed so a re-login racing with it cannot revive a dead one; it ends the fresh ref rather than leaking it on the pool. The credentials are kept for this. Config already held them for the life of the process, so nothing is newly exposed. What a fake pool cannot settle is whether XAPI really names the failure SESSION_INVALID over JSON-RPC. The unit tests assert the retry against a fake returning the name this package expects, so they would pass just as green if the match were wrong and the retry never fired. TestPoolSessionRecovery_Manual ends its own session behind the client's back and watches the recovery. It is read-only apart from that and needs no write authorisation on the pool. Not yet run: both lab hosts are unreachable from here. NewClient returned newJSONRPCClient's pair straight through, which converts a nil *jsonrpcClient into a non-nil Client interface holding it. Latent, since the only caller checks the error, but a trap for the next one. The test compares against nil directly rather than through testify, which sees the nil pointer inside and would pass either way. writePointer wrote in place, so a crash mid-write left a truncated pointer to a snapshot the pool had taken successfully: pool state intact, the only handle to it gone. It now writes a temp file in the same directory, syncs, and renames over the target. By hand rather than pulling renameio into this module, since machine/ is separate and this is the one place that needs it, which is the reasoning that kept the transport on net/http. TLS MinVersion is now explicit. Go's floor is already 1.2, so this changes nothing today; it is documentation, and insurance against that default moving under the session token this connection carries. Fixes #4 Fixes #5 Fixes #6 Signed-off-by: Bruno Verachten <gounthar@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
machine/xapi/client.go (1)
174-183: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider fsyncing the directory after the rename.
f.Sync()makes the temp file's contents durable. The rename itself stays in the directory's metadata cache. If the host crashes right afteros.Rename, the directory entry can be lost andreadPointerreturns the previous pointer instead of this one. The truncated-file case this function targets is still prevented, so this is hardening only.♻️ Proposed directory sync
if err = os.Rename(tmp, filepath.Join(dir, pointerName)); err != nil { return err } - return nil + // Sync the directory so the rename itself survives a crash, not just + // the contents of the file it points at. + d, err := os.Open(dir) + if err != nil { + return err + } + defer d.Close() + return d.Sync() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machine/xapi/client.go` around lines 174 - 183, Update the pointer-write flow after os.Rename in the relevant function to open the containing directory and fsync it before returning, propagating any open or sync errors. Preserve the existing file sync, close, rename ordering and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@machine/xapi/client.go`:
- Around line 174-183: Update the pointer-write flow after os.Rename in the
relevant function to open the containing directory and fsync it before
returning, propagating any open or sync errors. Preserve the existing file sync,
close, rename ordering and error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dede966-165c-47d1-a3a2-6676a5d08bfb
📒 Files selected for processing (5)
machine/xapi/client.gomachine/xapi/client_jsonrpc.gomachine/xapi/client_jsonrpc_test.gomachine/xapi/manual_test.gomachine/xapi/xapi_test.go
This was referenced Aug 3, 2026
Two review findings, both small, both about not letting a plausible configuration mean something other than what it says. newJSONRPCClient accepted any URL. On an http:// pool the login puts the password on the wire in the clear, and every call after it puts the session ref there too. InsecureTLS does not cover this: waiving certificate verification is a much smaller concession than dropping TLS, and an operator who set the one has not agreed to the other. The URL is now parsed and a non-https scheme, or a missing host, is refused. Parsing also means a URL with no scheme fails naming the field, rather than surfacing later out of http.NewRequestWithContext as an error that mentions neither Config nor URL. That has a knock-on in the tests: the fake pool served plain HTTP, so it now serves TLS with a self-signed certificate and the tests opt into InsecureTLS. That is the shape of a stock XCP-ng install, so the suite exercises a slightly more honest path than it did. poolConfigFromEnv read TEST_XAPI_INSECURE as != "0", so only that exact string turned verification on. TEST_XAPI_INSECURE=false and =no both left it off, which is the opposite of what they say, on the one setting that decides whether the pool's certificate is checked. It is parsed as a boolean now, and an unparseable value fails the test rather than falling back to a default, because the default here is the insecure one and guessing about TLS is not a thing to do quietly. Verified against XCP-ng 8.3.0 / XAPI 26.1 on the lab pool: the manual suite still logs in, recovers an ended session and refuses the unimplemented calls with the scheme check in place. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
8 tasks
gounthar
added a commit
that referenced
this pull request
Aug 4, 2026
Restore resumed the VM named in the pointer file but never wrote the ref back to v.ref, so a machine restored rather than created in this process was left with an empty ref. Every later call — Start, State, Stop, Pause — then drove a different VM from the one that had actually been resumed. The liveRef check added in #1 turns that into ErrInvalidState rather than a silent wrong-VM call, which is better but still wrong: a restored machine ought to be usable. Restore now takes the ref and marks the machine created, but only after the pool confirms the resume. Marking it created off the back of a failed resume would leave it answering for a VM that is not running. Issue #3 asked to settle the adjacent question at the same time, so: Restore is legal only on a machine that has been neither created nor destroyed. Both refusals fall out of adoption. Repointing a created machine would orphan the VM it already holds, with nothing left to tear it down; restoring after Destroy would resurrect a machine the caller has finished with, which Create already refuses for the same reason. rootVDI and bootVDI stay empty on a restored machine — the pointer file records the VM ref only. XAPI can be asked for the VBDs when teardown needs them, which is noted where teardown will be written. Fixes #3 Signed-off-by: Bruno Verachten <gounthar@gmail.com>
8 tasks
gounthar
added a commit
that referenced
this pull request
Aug 4, 2026
fix(xapi): three lifecycle defects from the PR #1 review
12 tasks
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.
What / why
clawk runs sandboxes on local hypervisors — vz on macOS, firecracker on Linux. Neither
reaches a pool, so anyone whose compute already lives on XCP-ng has to stand up separate
hardware to use it. This adds
machine/xapi, amachine.Backendthat drives anXCP-ng / XenServer pool over XAPI.
machine/is its own Go module, so nothing ininternal/orcmd/clawkis touched.Three constraints, each ruling out an easier design
Nothing is installed in dom0 — not a binary, not a kernel, not a config file. A tool
that needs files placed in dom0 is unshippable to XCP-ng users no matter how much simpler
it makes the code. So
DirectKernelspecs are honoured by synthesising a read-only bootVDI (ESP + GRUB + vmlinux + initrd) as
xvda, with the OCI rootfs asxvdbandroot=/dev/xvdb, rather than by pointingPV_kernelat a dom0 path. One boot VDI per(kernel, cmdline), cached in the SR.
Egress filtering stays out of this process. gvproxy runs in a per-host gateway VM with
a VIF on the sandbox's private (no-PIF) network, so the guest's only route out passes
through it. That preserves clawk's actual security property — root in the guest cannot
reconfigure the filter — without a dom0-resident daemon.
Caps.UserModeNet,TAPNetandUnixgramNetare consequently all false, andSpec.Netvariants are rejected. That isthe design, not an omission.
Xen has no virtio-vsock.
Machine.VSockreturnsErrVSockUnsupported; the controlpath is TCP over a management VIF via the package's
ControlDialer. This is the backend'slargest divergence from the machine contract and the part most worth arguing about. No
virtio-fs either, so
Spec.Sharesis rejected — firecracker already reportsVirtioFS: false, so there is precedent.Why plain XAPI JSON-RPC, and not a binding or XO
The transport is XAPI's own JSON-RPC endpoint spoken with
net/httpandencoding/json.No new dependency in
go.mod.A generated XenAPI binding over XML-RPC would drag in thousands of generated types to
serve the ~20 calls in
Client, and the raw VDI import is a separate HTTP PUT outside theRPC surface either way — so the binding saves nothing on the one hard part.
Xen Orchestra's API is nicer and brings XO's ACLs, which would matter for a multi-user
agent fleet. It also puts XO in the path of a tool that otherwise talks to any pool
unaided, which is the larger claim on an operator's setup. The
Clientinterface exists sothis stays reversible: an XO transport can land later as a second implementation, judged on
its own merits.
What actually works
Live:
session.login_with_password,VM.get_power_state,VM.get_by_name_label,session.logout.Everything else — the other
Clientmethods, plusCreate,Destroyand checkpointRestore— returns an error. Deliberately: they fail loudly at the pool boundary ratherthan returning a zero value that reads as success. A failing
Createis the honest stateof this branch.
Platform(s) built and tested on
macos-14job covers it./dev/kvmhere).Against the pool, confirmed by hand and by
manual_test.go:/jsonrpcendpoint pathlogin_with_passwordsignaturecode/message/data)VM.get_by_name_label→VM.get_power_stateRunningandHaltedChecklist
gofmt -l .is cleango build ./... && go vet ./... && go test ./...pass from the repo root — build andvet pass;
go test ./...has one failure that is not from this change.TestSandboxNetNS/topology_exists_inside_the_namespacefails looking for/sys/class/net/br0/flagsinside the netns. It reproduces identically on a cleanmainwith none of this branch's code, and nothing outside
machine/referencesmachine/xapi,so it is a WSL2 environment limitation rather than a regression. Flagged rather than
ticked, because a checked box here would be false.
go build ./... && go vet ./... && go test ./...pass frommachine/Open questions for review
These are genuinely undecided, and I'd rather raise them than pick unilaterally:
machine.Spechas no extension point. A local hypervisor has nothing to configure;a remote one has a pool address, credentials, an SR and two networks. This package takes
them via
Configure()before the firstNew(), which works but is process-global. ABackendconstructor alongside the registry would be cleaner — worth doing?internal/vsockclientconstructs its own transport. For this backend to work as aninternal/sandbox.Provider, it needs to accept a dialer instead. That is the only changereaching outside
machine/xapi, and whether it is welcome decides the shape of the restof this work. Not attempted here.
The snapshot contract.
Suspendable/Snapshottableare documented as writing stateinto a caller-owned directory, but XAPI keeps memory images in the SR. This writes a small
JSON pointer file into that directory instead. Is that reading acceptable, or should the
interface grow an opaque-handle variant?
Notes for the reviewer
Config.InsecureTLSis new public API, off by default. A stock XCP-ng install has aself-signed cert, so a lab pool is otherwise unreachable. The doc comment says why it
matters: the session token authenticating every later call crosses that connection.
VMByNameLabelis deliberately not on theClientinterface — it is on the concreteJSON-RPC type. The backend addresses VMs by the ref
VMCreatereturned and never searches;only a human needs lookup-by-name. It errors on duplicate name-labels rather than guessing,
since XAPI does not make them unique.
client_jsonrpc_test.gotests the wire format againsthttptest, so CI exercises thetransport with no pool. Without it, the gated
manual_test.gowould skip and nothingwould cover this code upstream.
manual_test.gogated ona
TEST_*variable, matchingkernel/manual_test.goandoci/manual_test.go.No linked issue — this work was not tracked in one.
Summary by CodeRabbit