diff --git a/docs/devices.md b/docs/devices.md index 917b7769..d3c18524 100644 --- a/docs/devices.md +++ b/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 | diff --git a/extend/disk/disk.go b/extend/disk/disk.go index 42ebed20..c0c9aa13 100644 --- a/extend/disk/disk.go +++ b/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 diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index 62c4568a..c05e9eb1 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/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-. - 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,19 +98,19 @@ 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 }) } @@ -126,13 +118,13 @@ func (ch *CloudHypervisor) FsDetach(ctx context.Context, vmRef, tag string) erro 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,24 +146,24 @@ 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 }) } @@ -179,11 +171,11 @@ func (ch *CloudHypervisor) DeviceDetach(ctx context.Context, vmRef, id string) e 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-. + 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 { diff --git a/hypervisor/cloudhypervisor/extend_test.go b/hypervisor/cloudhypervisor/extend_test.go new file mode 100644 index 00000000..d6624a42 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/hypervisor/firecracker/extend.go b/hypervisor/firecracker/extend.go index f34afc13..f03078ea 100644 --- a/hypervisor/firecracker/extend.go +++ b/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 { diff --git a/hypervisor/firecracker/extend_test.go b/hypervisor/firecracker/extend_test.go index 4135950a..a71b33ce 100644 --- a/hypervisor/firecracker/extend_test.go +++ b/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) }