feat(disk): vm disk attach and detach are idempotent

docs/devices.md

@@ -62,6 +62,12 @@ The backing file may live anywhere outside cocoon's managed directories;
 attach resolves symlinks and refuses a path under the root/run/log dirs
 because `vm rm` would delete it along with them.
 
+Both calls are idempotent. Attaching a disk that is already attached under
+the same name, path and mode succeeds and returns its existing id; the same
+name with a different path or mode is refused. Detaching a name that is not
+attached succeeds and logs a warning; on Cloud Hypervisor, a retry while the
+guest has not yet ejected an earlier removal waits for that eject.
+
 Flags:
 
 | Flag | Default | Description |

extend/disk/disk.go

@@ -49,7 +49,7 @@ type Attached struct {
 	ReadOnly bool   `json:"readonly,omitzero"`
 }
 
-// Attacher hot-plugs and removes virtio-blk data disks on a running VM.
+// Attacher hot-plugs and removes virtio-blk data disks on a running VM; both calls are idempotent.
 type Attacher interface {
 	DiskAttach(ctx context.Context, vmRef string, spec Spec) (deviceID string, err error)
 	DiskDetach(ctx context.Context, vmRef, name string) error

hypervisor/cloudhypervisor/extend.go

@@ -28,10 +28,10 @@ const (
 type makeBodyFn func(rec *hypervisor.VMRecord) any
 
 // preCheckFn vets the under-lock vm.info snapshot before a device mutation.
-type preCheckFn func(*chVMInfoResponse) error
+type preCheckFn func(*chVMInfoResponse) (existingID string, err error)
 
 // findIDFn resolves the device id to detach from the under-lock vm.info snapshot.
-type findIDFn func(*chVMInfoResponse) (string, error)
+type findIDFn func(*chVMInfoResponse) (id string, remove bool, err error)
 
 func (ch *CloudHypervisor) DiskAttach(ctx context.Context, vmRef string, spec disk.Spec) (string, error) {
 	if err := spec.Normalize(); err != nil {
@@ -50,20 +50,8 @@ func (ch *CloudHypervisor) DiskAttach(ctx context.Context, vmRef string, spec di
 		d.ID = id
 		return d
 	}
-	return ch.attachWith(ctx, vmRef, "vm.add-disk", makeBody, id, func(info *chVMInfoResponse) error {
-		for _, ex := range info.Config.Disks {
-			if ex.ID == id {
-				return fmt.Errorf("disk name %q already attached", spec.Name)
-			}
-			// Record data disks carry CH auto ids — match serials too, else two devices race for one /dev/disk/by-id/virtio-<name>.
-			if ex.Serial == spec.Name {
-				return fmt.Errorf("disk serial %q already used by disk %q", spec.Name, ex.ID)
-			}
-			if ex.Path == path {
-				return fmt.Errorf("disk path %q already attached as %q", path, ex.ID)
-			}
-		}
-		return nil
+	return ch.attachWith(ctx, vmRef, "vm.add-disk", makeBody, id, func(info *chVMInfoResponse) (string, error) {
+		return diskAttached(info, id, spec.Name, path, spec.ReadOnly)
 	})
 }
 
@@ -72,11 +60,15 @@ func (ch *CloudHypervisor) DiskDetach(ctx context.Context, vmRef, name string) e
 		return fmt.Errorf("name is required")
 	}
 	id := disk.DeriveID(name)
-	return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, error) {
+	return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, bool, error) {
 		if slices.ContainsFunc(info.Config.Disks, func(d chDisk) bool { return d.ID == id }) {
-			return id, nil
+			return id, true, nil
 		}
-		return "", fmt.Errorf("disk %q not attached", name)
+		if _, pending := info.DeviceTree[id]; pending {
+			return id, false, nil
+		}
+		log.WithFunc("cloudhypervisor.DiskDetach").Warnf(ctx, "disk %q is not attached to vm %s, nothing to detach", name, vmRef)
+		return "", false, nil
 	})
 }
 
@@ -106,33 +98,33 @@ func (ch *CloudHypervisor) FsAttach(ctx context.Context, vmRef string, spec fs.S
 			QueueSize: spec.QueueSize,
 		}
 	}
-	return ch.attachWith(ctx, vmRef, "vm.add-fs", makeBody, id, func(info *chVMInfoResponse) error {
+	return ch.attachWith(ctx, vmRef, "vm.add-fs", makeBody, id, func(info *chVMInfoResponse) (string, error) {
 		if !info.Config.Memory.Shared {
-			return fmt.Errorf("fs attach requires the VM to be created with --shared-memory (current memory shared=off; cannot be flipped on a running VM)")
+			return "", fmt.Errorf("fs attach requires the VM to be created with --shared-memory (current memory shared=off; cannot be flipped on a running VM)")
 		}
 		for _, ex := range info.Config.Fs {
 			if ex.Tag == spec.Tag {
-				return fmt.Errorf("fs tag %q already attached", spec.Tag)
+				return "", fmt.Errorf("fs tag %q already attached", spec.Tag)
 			}
 			if ex.ID == id {
-				return fmt.Errorf("fs id %q already attached", id)
+				return "", fmt.Errorf("fs id %q already attached", id)
 			}
 		}
-		return nil
+		return "", nil
 	})
 }
 
 func (ch *CloudHypervisor) FsDetach(ctx context.Context, vmRef, tag string) error {
 	if tag == "" {
 		return fmt.Errorf("tag is required")
 	}
-	return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, error) {
+	return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, bool, error) {
 		for _, ex := range info.Config.Fs {
 			if ex.Tag == tag {
-				return ex.ID, nil
+				return ex.ID, true, nil
 			}
 		}
-		return "", fmt.Errorf("fs tag %q not attached", tag)
+		return "", false, fmt.Errorf("fs tag %q not attached", tag)
 	})
 }
 
@@ -154,36 +146,36 @@ func (ch *CloudHypervisor) DeviceAttach(ctx context.Context, vmRef string, spec
 	makeBody := func(*hypervisor.VMRecord) any {
 		return chDevice{ID: spec.ID, Path: path}
 	}
-	return ch.attachWith(ctx, vmRef, "vm.add-device", makeBody, spec.ID, func(info *chVMInfoResponse) error {
+	return ch.attachWith(ctx, vmRef, "vm.add-device", makeBody, spec.ID, func(info *chVMInfoResponse) (string, error) {
 		// stat is gated behind the running-VM check so stopped VMs surface the state error, not a host-path one.
 		st, statErr := os.Stat(path)
 		if statErr != nil {
-			return fmt.Errorf("pci path %s: %w", path, statErr)
+			return "", fmt.Errorf("pci path %s: %w", path, statErr)
 		}
 		if !st.IsDir() {
-			return fmt.Errorf("pci path %s: not a directory", path)
+			return "", fmt.Errorf("pci path %s: not a directory", path)
 		}
 		for _, ex := range info.Config.Devices {
 			if ex.Path == path {
-				return fmt.Errorf("device %s already attached (id=%s)", path, ex.ID)
+				return "", fmt.Errorf("device %s already attached (id=%s)", path, ex.ID)
 			}
 			if spec.ID != "" && ex.ID == spec.ID {
-				return fmt.Errorf("device id %q already in use", spec.ID)
+				return "", fmt.Errorf("device id %q already in use", spec.ID)
 			}
 		}
-		return nil
+		return "", nil
 	})
 }
 
 func (ch *CloudHypervisor) DeviceDetach(ctx context.Context, vmRef, id string) error {
 	if id == "" {
 		return fmt.Errorf("id is required")
 	}
-	return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, error) {
+	return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, bool, error) {
 		if slices.ContainsFunc(info.Config.Devices, func(d chDevice) bool { return d.ID == id }) {
-			return id, nil
+			return id, true, nil
 		}
-		return "", fmt.Errorf("device id %q not attached", id)
+		return "", false, fmt.Errorf("device id %q not attached", id)
 	})
 }
 
@@ -220,8 +212,9 @@ func (ch *CloudHypervisor) attachWith(ctx context.Context, vmRef, endpoint strin
 		return "", err
 	}
 	defer unlock()
-	if checkErr := preCheck(info); checkErr != nil {
-		return "", checkErr
+	existing, checkErr := preCheck(info)
+	if checkErr != nil || existing != "" {
+		return existing, checkErr
 	}
 	bodyBytes, err := json.Marshal(makeBody(&rec))
 	if err != nil {
@@ -251,12 +244,14 @@ func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID
 		return err
 	}
 	defer unlock()
-	deviceID, err := findID(info)
-	if err != nil {
+	deviceID, remove, err := findID(info)
+	if err != nil || deviceID == "" {
 		return err
 	}
-	if err := removeDeviceVM(ctx, hc, deviceID); err != nil {
-		return fmt.Errorf("vm.remove-device %s: %w", deviceID, err)
+	if remove {
+		if err := removeDeviceVM(ctx, hc, deviceID); err != nil {
+			return fmt.Errorf("vm.remove-device %s: %w", deviceID, err)
+		}
 	}
 	// CH frees the slot, id and backing file only after the guest's eject ack.
 	if err := waitDeviceEjected(ctx, hc, deviceID); err != nil {
@@ -281,6 +276,25 @@ func (ch *CloudHypervisor) listWith[A any](ctx context.Context, vmRef string, ex
 	return extract(info), nil
 }
 
+func diskAttached(info *chVMInfoResponse, id, name, path string, readOnly bool) (string, error) {
+	for _, ex := range info.Config.Disks {
+		if ex.ID == id {
+			if ex.Path == path && ex.ReadOnly == readOnly {
+				return id, nil
+			}
+			return "", fmt.Errorf("disk name %q already attached with a different path or mode", name)
+		}
+		// Record data disks carry CH auto ids — match serials too, else two devices race for one /dev/disk/by-id/virtio-<name>.
+		if ex.Serial == name {
+			return "", fmt.Errorf("disk serial %q already used by disk %q", name, ex.ID)
+		}
+		if ex.Path == path {
+			return "", fmt.Errorf("disk path %q already attached as %q", path, ex.ID)
+		}
+	}
+	return "", nil
+}
+
 // the ops lock plus the Running gate exclude every capture window, so a pause seen here is ownerless.
 func convergeOrphanedPause(ctx context.Context, hc *http.Client, vmID string, info *chVMInfoResponse) (*chVMInfoResponse, error) {
 	if info.State != chStatePaused {

hypervisor/cloudhypervisor/extend_test.go

@@ -0,0 +1,43 @@
+package cloudhypervisor
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/cocoonstack/cocoon/extend/disk"
+)
+
+func TestDiskAttached(t *testing.T) {
+	id := disk.DeriveID("db")
+	info := &chVMInfoResponse{}
+	info.Config.Disks = []chDisk{
+		{ID: "_disk0", Path: "/vm/cow.raw", Serial: "root"},
+		{ID: id, Path: "/vols/db.raw", ReadOnly: true, Serial: "db"},
+	}
+	tests := []struct {
+		name, diskName, path string
+		readOnly             bool
+		want, wantErr        string
+	}{
+		{"free", "cache", "/vols/cache.raw", false, "", ""},
+		{"identical", "db", "/vols/db.raw", true, id, ""},
+		{"same name other path", "db", "/vols/other.raw", true, "", "different path or mode"},
+		{"same name other mode", "db", "/vols/db.raw", false, "", "different path or mode"},
+		{"serial in use", "root", "/vols/root.raw", false, "", "serial"},
+		{"same path other name", "db2", "/vols/db.raw", true, "", "already attached as"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := diskAttached(info, disk.DeriveID(tt.diskName), tt.diskName, tt.path, tt.readOnly)
+			if got != tt.want {
+				t.Errorf("existing id = %q, want %q", got, tt.want)
+			}
+			if tt.wantErr == "" && err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if tt.wantErr != "" && (err == nil || !strings.Contains(err.Error(), tt.wantErr)) {
+				t.Fatalf("err = %v, want containing %q", err, tt.wantErr)
+			}
+		})
+	}
+}

hypervisor/firecracker/extend.go

@@ -83,8 +83,8 @@ func (fc *Firecracker) DiskAttach(ctx context.Context, vmRef string, spec disk.S
 	if err != nil {
 		return "", err
 	}
-	if err := checkDriveFree(cfg, id, path); err != nil {
-		return "", err
+	if existing, err := driveAttached(cfg, id, path, spec.ReadOnly); err != nil || existing != "" {
+		return existing, err
 	}
 	if spec.DirectIO != nil {
 		log.WithFunc("firecracker.DiskAttach").Warnf(ctx, directIOIgnoredMsg, spec.Name)
@@ -117,7 +117,8 @@ func (fc *Firecracker) DiskDetach(ctx context.Context, vmRef, name string) error
 		return err
 	}
 	if !slices.ContainsFunc(cfg.Drives, func(d fcDrive) bool { return d.DriveID == id }) {
-		return fmt.Errorf("disk %q not attached", name)
+		log.WithFunc("firecracker.DiskDetach").Warnf(ctx, "disk %q is not attached to vm %s, nothing to detach", name, vmRef)
+		return nil
 	}
 	return deleteDevice(ctx, hc, "/drives/"+id)
 }
@@ -198,16 +199,19 @@ func hotDiskName(id string) string {
 	return ""
 }
 
-func checkDriveFree(cfg *fcVMConfig, id, path string) error {
+func driveAttached(cfg *fcVMConfig, id, path string, readOnly bool) (string, error) {
 	for _, d := range cfg.Drives {
 		if d.DriveID == id {
-			return fmt.Errorf("disk %q already attached", hotDiskName(id))
+			if d.PathOnHost == path && d.IsReadOnly == readOnly {
+				return id, nil
+			}
+			return "", fmt.Errorf("disk %q already attached with a different path or mode", hotDiskName(id))
 		}
 		if d.PathOnHost == path {
-			return fmt.Errorf("disk path %q already attached as %q", path, d.DriveID)
+			return "", fmt.Errorf("disk path %q already attached as %q", path, d.DriveID)
 		}
 	}
-	return nil
+	return "", nil
 }
 
 func hotAttachedDisks(cfg *fcVMConfig) []disk.Attached {

hypervisor/firecracker/extend_test.go

@@ -56,21 +56,28 @@ func TestRequirePCI(t *testing.T) {
 	}
 }
 
-func TestCheckDriveFree(t *testing.T) {
+func TestDriveAttached(t *testing.T) {
 	cfg := &fcVMConfig{Drives: []fcDrive{
 		{DriveID: "drive_0", PathOnHost: "/layer.erofs"},
-		{DriveID: hotDiskIDPrefix + "db", PathOnHost: "/vols/db.raw"},
+		{DriveID: hotDiskIDPrefix + "db", PathOnHost: "/vols/db.raw", IsReadOnly: true},
 	}}
 	tests := []struct {
-		name, id, path, wantErr string
+		name, id, path string
+		readOnly       bool
+		want, wantErr  string
 	}{
-		{"free", hotDiskIDPrefix + "cache", "/vols/cache.raw", ""},
-		{"same name", hotDiskIDPrefix + "db", "/vols/other.raw", "already attached"},
-		{"same path", hotDiskIDPrefix + "db2", "/vols/db.raw", "already attached as"},
+		{"free", hotDiskIDPrefix + "cache", "/vols/cache.raw", false, "", ""},
+		{"identical", hotDiskIDPrefix + "db", "/vols/db.raw", true, hotDiskIDPrefix + "db", ""},
+		{"same name other path", hotDiskIDPrefix + "db", "/vols/other.raw", true, "", "different path or mode"},
+		{"same name other mode", hotDiskIDPrefix + "db", "/vols/db.raw", false, "", "different path or mode"},
+		{"same path other name", hotDiskIDPrefix + "db2", "/vols/db.raw", true, "", "already attached as"},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := checkDriveFree(cfg, tt.id, tt.path)
+			got, err := driveAttached(cfg, tt.id, tt.path, tt.readOnly)
+			if got != tt.want {
+				t.Errorf("existing id = %q, want %q", got, tt.want)
+			}
 			if tt.wantErr == "" && err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}