feat(xapi): implement the VDI calls so an image can reach an SR and be cloned - #13
Conversation
…be cloned Build-order step 3. VDIImportRaw, VDIFindByName, VDIClone and VDIDestroy were all errNotImplemented; all four are needed together, because a test that creates disks on someone's SR and cannot remove them is not one worth having. The import is the only call in this transport that is not an RPC. The data goes to XAPI's /import_raw_vdi as an HTTP PUT, and the outcome is not in the HTTP response: XAPI writes its 200 header before the transfer completes, so the status line means "accepted", not "the disk is on the SR". A Task is therefore created and waited on rather than letting XAPI make its own and discarding the handle, and task.get_error_info is surfaced so a failure says what the pool said. A failed import destroys the VDI it created. Leaving it would be worse than useless: it keeps the name-label, so the next run's VDIFindByName would adopt a half-written disk as a valid golden image. Cleanup uses WithoutCancel, since the usual way to get here is a cancelled context and that is exactly when it still has to happen. poolEndpoint became poolBase plus a JoinPath, so both endpoints derive from the same validated parts. Assembling the import URL separately would have left the https guarantee, the credential rejection and the query rejection covering RPC and quietly not covering the disk upload. virtual_size goes on the wire as a JSON number, confirmed against the pool rather than assumed: SR.get_physical_size returned 207028920320, not a string. A string is accepted by encoding/json and rejected by XAPI, which is an unpleasant place to learn it, so a unit test pins the encoding. Duplicate name-labels in one SR are an error rather than a coin toss, the rule VMByNameLabel already follows. These names are keyed on an image digest, so a duplicate means something else is writing under our scheme, and guessing would attach an unknown disk to a sandbox. Verified against XCP-ng 8.3.0 / XAPI 26.1: 8 MiB imported, virtual size read back exactly, found by name, cloned, both destroyed, SR left with zero VDIs. The unit tests are mutation-checked — dropping the task wait or the failure cleanup makes them fail. Worth knowing for step 4: VDI.clone on an ext SR leaves a third VDI named "base copy", the read-only shared parent. It cannot be destroyed directly and the SR garbage collector reclaims it once both children are gone. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
Warning Review limit reached
Next review available in: 6 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)
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 |
All three were in code that had already passed against a real pool, which is the uncomfortable part: the round trip worked and the bugs were still there. **The session ref was read before task.create.** taskCreate goes through sessionCall, which re-logs-in and retries on SESSION_INVALID, so a ref captured before it can be dead by the time it reaches the import URL. Nothing would have caught that: the import is not an RPC, so sessionCall's retry does not cover it, and XAPI would simply refuse the PUT. Reading the ref after task.create closes the window every preceding RPC opens. **A failed import never cancelled its task.** The client stopped watching; the task kept running on the pool and kept writing into the VDI. **And the destroy that follows raced XAPI's release.** This is the one the fake could not have taught us. Adding the task cancel above was not enough: against XCP-ng 8.3 the destroy came back VDI_IN_USE and the half-written disk survived under the name the next VDIFindByName would adopt — exactly the failure the cleanup exists to prevent, reintroduced by fixing something else. XAPI frees the disk asynchronously, and the same destroy succeeds moments later, so destroyAfterFailedImport retries for a bounded six seconds. The pool test that caught it went from 2.9s to 6.3s, which is the retry working. Also: GetBody is now set when the source is seekable. XAPI redirects this endpoint to the host the SR is attached to, which on any multi-host pool is routinely not the master, and Go can only follow a redirect whose body it can replay. net/http sets GetBody itself for *bytes.Reader, *bytes.Buffer and *strings.Reader, so the first version of that test passed with and without the fix; it now uses a bare io.ReadSeeker, which is what an *os.File is and what a real image will be. Every test here fails against the previous commit. The first two were found by agy and gh copilot independently, reviewing the same diff — CodeRabbit was rate-limited on #13 and did not review it at all. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
Pushed 0fdd1b9. CodeRabbit was rate limited on this PR, so I ran the local reviewers over the diff instead, and they earned their keep: agy and gh copilot independently found the same two bugs, both in code that had already round-tripped against a real pool. The session ref was read before task.create. taskCreate goes through sessionCall, which re-logs-in on SESSION_INVALID, so the ref could be dead by the time it reached the import URL. The import is not an RPC, so sessionCall retry never covered it. Reading it after task.create closes the window. A failed import never cancelled its XAPI task, so it kept running server-side and kept writing into the VDI. Then fixing that second one exposed a third that neither reviewer predicted and the fake could not have shown. With the cancel in place, VDI.destroy came back VDI_IN_USE and the half-written disk survived under the name the next VDIFindByName would adopt, which is exactly the failure the cleanup exists to prevent. XAPI frees a disk asynchronously after an import ends: the identical destroy succeeds a few seconds later. destroyAfterFailedImport now retries for a bounded six seconds, and the pool test that caught it went from 2.9s to 6.3s. So the honest sequence is that the review was right and my fix for it made a latent bug reachable. One VDI leaked onto the lab host in the process; our own test caught it and it was removed within a minute. Also set GetBody for seekable sources. XAPI redirects /import_raw_vdi to the host the SR is attached to, and Go can only follow a redirect whose body it can replay. Worth noting the first version of that test was useless: net/http sets GetBody itself for *bytes.Reader, so it passed with and without the fix. It now uses a bare io.ReadSeeker, which is what an *os.File is and what a real image will be. Every test added here fails against 49e5746. Re-verified against XCP-ng 8.3.0 / XAPI 26.1, SR left with zero VDIs. linux and macos green. |
1 participant
What / why
Build-order step 3: prove a disk image can reach an SR and be cloned.
VDIImportRaw,VDIFindByName,VDICloneandVDIDestroywere allerrNotImplemented.Four calls rather than the two the build order names.
VDIDestroyis what lets the pool test clean up after itself, and a test that creates disks on someone's SR and cannot remove them is not one worth having. That turned out to be less theoretical than I meant it, see the confession below.The import is the only call here that is not an RPC. Data goes to XAPI's
/import_raw_vdias an HTTP PUT, and the outcome is not in the HTTP response: XAPI writes its 200 header before the transfer completes, so the status line means "accepted", not "the disk is on the SR". So the transport creates its own Task, passes it astask_id, waits for it to leavepending, and surfacestask.get_error_infoon failure. Letting XAPI make its own task and discarding the handle would have made every import report success, including the ones that ran the SR out of space.A failed import destroys the VDI it created. Leaving it behind would be worse than useless, because it keeps the name-label, so the next run's
VDIFindByNamewould adopt a half-written disk as a valid golden image. Cleanup goes throughcontext.WithoutCancel, since the usual way to reach that path is a cancelled context and that is exactly when it still has to happen.poolEndpointbecamepoolBaseplus aJoinPath. Both endpoints now derive from the same validated parts. Building the import URL separately would have left the https guarantee, the credential rejection and the query rejection covering RPC and quietly not covering the disk upload, which is the larger of the two transfers and the one carrying a session ref in its query string.Duplicate name-labels in one SR are an error, not a coin toss, the rule
VMByNameLabelalready follows. These names are keyed on an image digest, so a duplicate means something else is writing under our scheme, and guessing would attach an unknown disk to a sandbox.Two things checked against a pool rather than assumed
virtual_sizegoes on the wire as a JSON number. I checked before writing the code:SR.get_physical_sizecame back207028920320, not"207028920320". A string is accepted byencoding/jsonand rejected by XAPI, which is an unpleasant place to learn it, so a unit test pins the encoding.VDI.cloneon an ext SR leaves a third VDI namedbase copy, the read-only shared parent, with the original and the clone as its children. It cannot be destroyed directly and the SR garbage collector reclaims it once both children are gone. That matters for step 4's teardown, and it is noted in the commit for whoever writes it.Testing
Unit tests run against an httptest server that now also serves
/import_raw_vdi. They are mutation-checked: dropping the task wait, or dropping the failure cleanup, makes them fail. I did that because most of this file is ordering, and a test written after the code has a habit of asserting the author's assumptions back at them.The writing pool tests take
TEST_XAPI_WRITE=1and an explicitTEST_XAPI_SR, rather than riding on the existingTEST_XAPI_POOL. Someone pointing the read-only suite at a production pool should not discover that the variable they already set has started creating disks, and naming the SR explicitly means nothing can default to writing somewhere nobody chose.The bit where I broke something
The first pool run leaked two VDIs onto the lab host. The test registered the pool logout with
deferand the VDI destroys witht.Cleanup, and Go runs deferred calls when the test function returns butt.Cleanupafter that, so the session was already closed when the destroys fired and both failed with "no session".I removed them by hand, verified the SR back to zero VDIs, fixed the ordering (register the close via
t.Cleanupfirst, so LIFO runs it last) and re-ran clean. The comment explaining it is in the test, becausedefer cl.Close()looks equivalent to a cleanup and is not.Platform(s) built and tested on
Against a real pool, XCP-ng 8.3.0 / XAPI 26.1: 8 MiB imported, virtual size read back exactly, found by name, cloned, both destroyed, SR left with zero VDIs.
physical_utilisationends 4096 bytes above where it started, one block of filesystem metadata rather than a leak.Not run on macOS.
vzis build-gated behind cgo on darwin and CI covers it.Checklist
gofmt -l .is cleango build ./... && go vet ./... && go test ./...pass from the repo rootgo build ./... && go vet ./... && go test ./...pass frommachine/go test -race ./xapi/is clean too.The part I would most like a second opinion on is the Task handling. Waiting on a task the transport created is more machinery than the endpoint strictly requires, and if there is a simpler way to tell a finished import from an accepted one, I would rather use it.