From be8ff82ea70e931233270062b36a8ce0ff6e3b1b Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 24 Jul 2026 11:35:48 -0500 Subject: [PATCH] fix(disk): undelete a disk through mount-then-ready ordering (MIR-1469) `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. --- blackbox/disk_undelete_test.go | 8 ++- cli/commands/disk_undelete.go | 20 +++++-- components/diskio/disk_volume_controller.go | 13 ++++- .../diskio/disk_volume_controller_test.go | 55 +++++++++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/blackbox/disk_undelete_test.go b/blackbox/disk_undelete_test.go index 6ac146a82..d96dd022c 100644 --- a/blackbox/disk_undelete_test.go +++ b/blackbox/disk_undelete_test.go @@ -105,7 +105,11 @@ func TestDiskUndelete(t *testing.T) { leaseID := extractLeaseID(t, r) t.Logf("Lease ID: %s", leaseID) - // Wait for lease to become bound + // Wait for lease to become bound. A lease that lands in "failed" is + // terminal (the controller never re-drives it), so treat it as fatal and + // fail fast with the status output rather than spinning until timeout — + // otherwise a regression reads as an opaque 60s hang instead of a lease + // that failed to bind (MIR-1469). t.Log("Waiting for lease to bind...") harness.Poll(t, "lease bound", 60*time.Second, 2*time.Second, func() (bool, string) { @@ -117,7 +121,7 @@ func TestDiskUndelete(t *testing.T) { return true, "" } if r.OutputContains("failed") { - return false, "lease failed" + t.Fatalf("lease %s went to failed instead of binding:\n%s", leaseID, r.Stdout+r.Stderr) } return false, "lease not yet bound" }, diff --git a/cli/commands/disk_undelete.go b/cli/commands/disk_undelete.go index 3c863a59d..e40674848 100644 --- a/cli/commands/disk_undelete.go +++ b/cli/commands/disk_undelete.go @@ -163,9 +163,15 @@ func DiskUndelete(ctx *Context, opts struct { Filesystem: filesystem, VolumeMode: storage_v1alpha.DiskVolumeVolumeMode(meta.VolumeMode), DesiredState: storage_v1alpha.DV_PRESENT, - ActualState: storage_v1alpha.DV_READY, - ImagePath: imagePath, - NodeId: nodeId, + // Start PENDING, not READY. The runner's DiskVolumeController drives + // the restored volume through the same mount-then-READY ordering as a + // fresh create (skipping the reimage, since disk.img is already in + // place). Advertising DV_READY here would let a lease bind against a + // volume that isn't registered in the runner's in-memory state yet, + // which lands the lease in a terminal FAILED (MIR-1469). + ActualState: storage_v1alpha.DV_PENDING, + ImagePath: imagePath, + NodeId: nodeId, } _, err = eac.Create(context.Background(), entity.New( @@ -176,10 +182,14 @@ func DiskUndelete(ctx *Context, opts struct { return fmt.Errorf("creating disk_volume entity: %w", err) } - // Transition disk to PROVISIONED + // Transition disk to PROVISIONING (not PROVISIONED). The DiskController + // promotes it to PROVISIONED only once the disk_volume actually reaches + // DV_READY — i.e. after the volume is registered and mounted on the node — + // so the disk never reports "provisioned" (and thus leasable) ahead of a + // real mount. _, err = eac.Patch(context.Background(), []entity.Attr{ entity.Ref(entity.DBId, diskEntityId), - entity.Ref(storage_v1alpha.DiskStatusId, storage_v1alpha.DiskStatusProvisionedId), + entity.Ref(storage_v1alpha.DiskStatusId, storage_v1alpha.DiskStatusProvisioningId), entity.String(storage_v1alpha.DiskVolumeIdId, volId), }, 0) if err != nil { diff --git a/components/diskio/disk_volume_controller.go b/components/diskio/disk_volume_controller.go index 029e01825..d6445499d 100644 --- a/components/diskio/disk_volume_controller.go +++ b/components/diskio/disk_volume_controller.go @@ -240,11 +240,20 @@ func (c *DiskVolumeController) createVolume(ctx context.Context, volume *storage return fmt.Errorf("failed to create volume directory: %w", err) } - // Create sparse disk image + // Create sparse disk image. Skip when one is already present: a restored + // volume (moved back into place by `disk undelete`) already carries its + // disk.img, and reimaging it here would truncate away the recovered data. + // Volume entity IDs are unique, so a genuine fresh create never collides + // with an existing image — only the restore path lands here idempotently. imagePath := filepath.Join(volumePath, "disk.img") sizeBytes := units.GigaBytes(volume.SizeGb).Bytes().Int64() - if err := c.ops.CreateDiskImage(imagePath, sizeBytes); err != nil { + if c.ops.VolumePathExists(imagePath) { + c.log.Info("disk image already present, preserving it (restore/recovery)", + "entity_id", entityId, + "image_path", imagePath, + ) + } else if err := c.ops.CreateDiskImage(imagePath, sizeBytes); err != nil { c.setVolumeError(ctx, volume.ID, fmt.Sprintf("failed to create disk image: %v", err)) return fmt.Errorf("failed to create disk image: %w", err) } diff --git a/components/diskio/disk_volume_controller_test.go b/components/diskio/disk_volume_controller_test.go index e1fd47d19..3caeb219e 100644 --- a/components/diskio/disk_volume_controller_test.go +++ b/components/diskio/disk_volume_controller_test.go @@ -88,6 +88,61 @@ func TestDiskVolumeControllerReconcileVolumePresent(t *testing.T) { assert.Equal(t, filepath.Join(expectedVolPath, "disk.img"), updated.ImagePath) } +// TestDiskVolumeControllerCreateSkipsExistingImage covers the restore path: +// when a disk.img is already present (moved back into place by `disk +// undelete`), createVolume must preserve it rather than reimaging, so the +// recovered data survives. Reaching this branch is what lets undelete route a +// restored volume through the normal mount-then-READY sequence (MIR-1469). +func TestDiskVolumeControllerCreateSkipsExistingImage(t *testing.T) { + ctx := t.Context() + log := testutils.TestLogger(t) + + es, cleanup := testutils.NewInMemEntityServer(t) + defer cleanup() + + dataPath := t.TempDir() + nodeId := "test-node-1" + state := NewState() + ops := newMockDiskVolumeOps() + + vc := newTestDiskVolumeController(log, dataPath, nodeId, es.EAC, state, ops) + + // Simulate a restored volume: the directory and its disk.img already + // exist on disk before reconcile runs. + expectedVolPath := filepath.Join(dataPath, "volumes", "vol-123") + imagePath := filepath.Join(expectedVolPath, "disk.img") + ops.existingPaths[expectedVolPath] = true + ops.existingPaths[imagePath] = true + + vol := &storage_v1alpha.DiskVolume{ + ID: "disk_volume/vol-123", + NodeId: compute.NewNodeId(nodeId).Id(), + SizeGb: 10, + Filesystem: "ext4", + DesiredState: storage_v1alpha.DV_PRESENT, + ActualState: storage_v1alpha.DV_PENDING, + } + createDiskVolumeEntity(ctx, t, es, vol) + + err := vc.ReconcileWithEntities(ctx) + require.NoError(t, err) + + // The existing image must be preserved, not reimaged. + assert.Empty(t, ops.createdImages, "restored disk.img should not be recreated") + + // State is still registered and the entity still reaches READY, so the + // restored volume becomes leasable through the normal promotion path. + volState := state.GetVolume("disk_volume/vol-123") + require.NotNil(t, volState) + assert.Equal(t, "vol-123", volState.VolumeId) + + resp, err := es.EAC.Get(ctx, "disk_volume/vol-123") + require.NoError(t, err) + var updated storage_v1alpha.DiskVolume + updated.Decode(resp.Entity().Entity()) + assert.Equal(t, storage_v1alpha.DV_READY, updated.ActualState) +} + func TestDiskVolumeControllerReconcileVolumeAbsent(t *testing.T) { ctx := t.Context() log := testutils.TestLogger(t)