fix(disk): undelete a disk through mount-then-ready ordering (MIR-1469)#970
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughDisk undelete now creates restored volumes in Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
This is a draft, so I'm judging whether it's ready for human review — and it is, with one minor gap worth noting.
What this does
The fix closes a race in disk undelete: previously, the command wrote DV_READY and DiskStatusProvisioned immediately on creation, which meant a lease could bind against a volume that wasn't yet registered in the runner's in-memory state. That produced a terminal FAILED lease. The fix introduces a proper ordering:
disk_volumeis created inDV_PENDING(notDV_READY), so theDiskVolumeControllerdrives it through the normal mount-then-ready sequence.- The disk entity is set to
PROVISIONING(notPROVISIONED), so the disk isn't advertised as leasable until the volume controller promotes it after a real mount. DiskVolumeController.createVolumenow skipsCreateDiskImagewhendisk.imgis already present, so the restored data isn't truncated.
The causal chain is clear and the comments in each changed file explain the invariant well.
What I checked
disk_volume_controller.go — the VolumePathExists(imagePath) guard is the right call. The imagePath is constructed inside createVolume as filepath.Join(volumePath, "disk.img"), so it correctly targets the image file, not just the directory. VolumePathExists is already used in several other places against directory paths (where a directory also satisfies the check), but here it's targeting a file — I verified the real implementation at disk_ops_linux.go line 49 uses os.Stat, which works on both files and directories. ✓
The PROVISIONING → PROVISIONED promotion chain — I traced through the codebase to confirm this path exists. The normal debug disk create flow also starts with PROVISIONING, and the DiskVolumeController promotes the disk entity once DV_READY is reached via updateVolumeState. The undelete path now uses the same sequence. ✓
Test: t.Fatalf in the poll callback — harness.Poll calls the condition in the same goroutine (no go keyword, no t.Run), so t.Fatalf is safe there. The change also replaces a silent 60-second timeout with a fast, diagnostic failure message — a concrete improvement to the test's debuggability. ✓
One gap
There's no unit test for the "disk image already present, skip CreateDiskImage" branch in createVolume. The existing test TestDiskVolumeControllerReconcileVolumePresent exercises the create-from-scratch path and verifies ops.createdImages has one entry. A restore scenario would need ops.existingPaths to include the imagePath so that VolumePathExists returns true, and then the assertion would be assert.Empty(t, ops.createdImages). This is the only newly added runtime behaviour without a unit test. It's not a blocker — the blackbox test covers the end-to-end scenario — but a unit test here would make regressions very cheap to catch.
🍪 full review note · comment /biscuit review to run biscuit again.
`disk undelete` was writing the restored volume straight to DV_READY and
the disk straight to PROVISIONED, both up front. But those are just entity
attributes: the runner's DiskVolumeController still has to register the
volume in its in-memory state and mount it, asynchronously. A lease created
in that window found the volume PROVISIONED-and-READY-on-paper but not yet
in the runner's state, so the mount controller latched a terminal DM_ERROR
("volume not found in state") and the lease went permanently FAILED. Since
the reconcile framework does not requeue failed items and FAILED leases are
never re-driven, it never self-healed: the volume mounted a beat later, too
late. That is the TestDiskUndelete flake, lease created then instantly
failed, green on re-run.
Fix the ordering at the source. Undelete now creates the volume DV_PENDING
and leaves the disk PROVISIONING, so it flows through the same
mount-then-DV_READY path a fresh create uses, and the DiskController only
promotes the disk to PROVISIONED once the volume genuinely reaches DV_READY
(registered and mounted). The disk therefore never reports leasable ahead
of a real mount.
To let the restored volume reuse that create path safely, createVolume now
skips reimaging when a disk.img is already present instead of truncating
it. Volume entity IDs are unique, so a genuine fresh create never collides,
only the restore path lands there idempotently. This also closes a latent
data-loss hole where the DV_ERROR-recreation path would truncate an
existing image on any error.
Also make the blackbox bind-poll fail fast on a failed lease with the
status output, so a future regression is an instant legible failure rather
than an opaque 60s hang.
a23529b to
be8ff82
Compare
|
/biscuit review |
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
The gap I flagged in my last review — no unit test for the "skip CreateDiskImage when disk.img already exists" branch — has been filled. TestDiskVolumeControllerCreateSkipsExistingImage (lines 91–144 of disk_volume_controller_test.go) does exactly what I asked: it pre-marks both the volume directory and disk.img as existing via ops.existingPaths, runs reconcile, and asserts ops.createdImages is empty while also confirming the entity reaches DV_READY. That's the right shape for this test.
The rest of the change is unmodified from my previous review, and my analysis of it still holds: the DV_PENDING / PROVISIONING ordering fix is correct, the VolumePathExists guard targets the file correctly, and the blackbox test improvement is a genuine win. No new concerns from this pass.
Ready to graduate to human review.
🍪 full review note · comment /biscuit review to run biscuit again.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
TestDiskUndeletewas flaking: the lease it puts on a just-undeleted disk would sometimes go straight tofailedand never recover, timing out after 60s, then pass on re-run.Turns out
disk undeletewas getting ahead of itself. It wrote the volumeDV_READYand the diskPROVISIONEDup front, but those are just entity attributes. The runner still has to register and mount the volume asynchronously, and in that window the disk looks leasable while nothing's actually mounted. A lease created then finds no volume in the runner's state, latches a terminalDM_ERROR, and gets stuckFAILEDforever (failed leases never get re-driven). Reproduced it locally at iteration 44, and the runner log showed the volume mounting ~18s too late.Fix is to stop lying about readiness. Undelete now creates the volume
DV_PENDINGand leaves the diskPROVISIONING, so it flows through the same mount-then-DV_READYpath a normal create uses, and the disk only reports provisioned once the volume is genuinely mounted. To reuse that path safely,createVolumeskips reimaging when adisk.imgis already there instead of truncating it (also closes a latent data-loss hole on the error-recreation path). And the test now fails fast on afailedlease so a regression is legible instead of a 60s hang.Verified: 150 clean iterations (flaked at 44 before), normal disk create/persist still green, lint clean.
Closes MIR-1469