From f04089a6d0bb009d4ed226fd485e79202da4b0dc Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:41:04 +0800 Subject: [PATCH 1/9] feat(disk): add the snapshot attach contract and default snapshot disk name --- extend/disk/disk.go | 50 +++++++++++++++++++++++++++++++++------- extend/disk/disk_test.go | 39 +++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 8 deletions(-) diff --git a/extend/disk/disk.go b/extend/disk/disk.go index c0c9aa13..d38eab2d 100644 --- a/extend/disk/disk.go +++ b/extend/disk/disk.go @@ -1,4 +1,4 @@ -// Package disk is the runtime attach interface for extra virtio-blk disks backed by existing raw files; attach is runtime-only (snapshot/hibernate refuse while one is attached) and detach never deletes the backing file. +// Package disk is the runtime attach interface for extra virtio-blk disks backed by existing raw files or a snapshot's COW copy; attach is runtime-only (snapshot/hibernate refuse while one is attached) and detach deletes only cocoon's own copy. package disk import ( @@ -11,7 +11,11 @@ import ( "github.com/cocoonstack/cocoon/types" ) -const diskIDPrefix = "cocoon-disk-" +const ( + diskIDPrefix = "cocoon-disk-" + snapshotNamePrefix = "snap-" + snapshotNameIDLen = 8 +) // ErrUnsupportedBackend signals the backend cannot hot-plug virtio-blk disks (Firecracker on the MMIO transport). var ErrUnsupportedBackend = errors.New("backend does not support disk attach") @@ -32,13 +36,22 @@ func (s *Spec) Normalize() error { if !filepath.IsAbs(s.Path) { return fmt.Errorf("path must be absolute, got %q", s.Path) } - if s.Name == "" { - return fmt.Errorf("name is required") - } - if !types.ValidDataDiskName(s.Name) { - return fmt.Errorf("name %q invalid: must match ^[a-z][a-z0-9_-]{0,19}$", s.Name) + return ValidateName(s.Name) +} + +// SnapshotSource names a snapshot dir whose COW disk is copied and attached; BasePath is the local qcow2 base blob, empty for OCI. +type SnapshotSource struct { + Dir string + Name string + BlobIDs map[string]struct{} + BasePath string +} + +func (s *SnapshotSource) Normalize() error { + if s.Dir == "" { + return fmt.Errorf("snapshot dir is required") } - return nil + return ValidateName(s.Name) } // Attached is the inspect-time view of one hot-added disk read from the running VMM's config. @@ -55,11 +68,32 @@ type Attacher interface { DiskDetach(ctx context.Context, vmRef, name string) error } +// SnapshotAttacher hot-plugs a private writable copy of a snapshot's COW disk; repeating the call returns the attached disk. +type SnapshotAttacher interface { + DiskAttachSnapshot(ctx context.Context, vmRef string, src SnapshotSource) (deviceID, format string, err error) +} + // Lister enumerates hot-added disks from running VM state. type Lister interface { DiskList(ctx context.Context, vmRef string) ([]Attached, error) } +// ValidateName checks a disk name, which doubles as the guest serial and a file name component. +func ValidateName(name string) error { + if name == "" { + return fmt.Errorf("name is required") + } + if !types.ValidDataDiskName(name) { + return fmt.Errorf("name %q invalid: must match ^[a-z][a-z0-9_-]{0,19}$", name) + } + return nil +} + +// SnapshotName is the default disk name for a snapshot-sourced attach. +func SnapshotName(snapID string) string { + return snapshotNamePrefix + strings.ToLower(snapID[:min(len(snapID), snapshotNameIDLen)]) +} + // DeriveID returns the deterministic Cloud Hypervisor device id for a disk name (attach and detach share it so concurrent attaches collide on CH's id check); Firecracker derives its own underscore form. func DeriveID(name string) string { return diskIDPrefix + name diff --git a/extend/disk/disk_test.go b/extend/disk/disk_test.go index a8ac0dd4..f06977a9 100644 --- a/extend/disk/disk_test.go +++ b/extend/disk/disk_test.go @@ -25,6 +25,45 @@ func TestSpecNormalize(t *testing.T) { } } +func TestSnapshotSourceNormalize(t *testing.T) { + tests := []struct { + name string + src SnapshotSource + wantErr bool + }{ + {"valid", SnapshotSource{Dir: "/snap", Name: "snap-abcd"}, false}, + {"missing dir", SnapshotSource{Name: "snap-abcd"}, true}, + {"missing name", SnapshotSource{Dir: "/snap"}, true}, + {"path in name", SnapshotSource{Dir: "/snap", Name: "a/../b"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.src.Normalize() + if (err != nil) != tt.wantErr { + t.Errorf("Normalize() = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestSnapshotName(t *testing.T) { + tests := []struct { + snapID, want string + }{ + {"7QXKZ2MVB4N5LTRWYHC3DAEFGJ", "snap-7qxkz2mv"}, + {"ABC", "snap-abc"}, + } + for _, tt := range tests { + got := SnapshotName(tt.snapID) + if got != tt.want { + t.Errorf("SnapshotName(%q) = %q, want %q", tt.snapID, got, tt.want) + } + if err := ValidateName(got); err != nil { + t.Errorf("SnapshotName(%q) = %q is not a valid disk name: %v", tt.snapID, got, err) + } + } +} + func TestDeriveIDRoundTrip(t *testing.T) { id := DeriveID("vol1") if id != "cocoon-disk-vol1" { From fe9877c33be924776f061924d9400ebe08e1be47 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:41:04 +0800 Subject: [PATCH 2/9] feat(hypervisor): resolve a sidecar's cow file and let runtime cleanup take glob patterns The snapshot disk attach copies only the sidecar's cow entry, by basename, so another node's sidecar works. Stop needs to sweep per-name copies, which fixed runtime names cannot express; non-pattern names keep the plain remove so dangling symlinks still go. --- hypervisor/snapshot.go | 14 ++++++++++++++ hypervisor/snapshot_test.go | 30 ++++++++++++++++++++++++++++++ hypervisor/utils.go | 15 +++++++++++---- hypervisor/utils_test.go | 26 ++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/hypervisor/snapshot.go b/hypervisor/snapshot.go index d2716f73..d119cce2 100644 --- a/hypervisor/snapshot.go +++ b/hypervisor/snapshot.go @@ -6,6 +6,7 @@ import ( "maps" "os" "path/filepath" + "slices" "strings" "github.com/projecteru2/core/log" @@ -214,6 +215,19 @@ func LoadSnapshotMeta(dir string) (*SnapshotMeta, error) { return &meta, nil } +// SnapshotCOWFile returns the basename of the sidecar's COW disk; only the basename is trusted, so another node's sidecar works. +func SnapshotCOWFile(meta *SnapshotMeta) (string, error) { + i := slices.IndexFunc(meta.StorageConfigs, func(sc *types.StorageConfig) bool { return sc != nil && sc.Role == types.StorageRoleCOW }) + if i < 0 { + return "", fmt.Errorf("no cow disk in snapshot metadata") + } + base := filepath.Base(meta.StorageConfigs[i].Path) + if base == "." || base == ".." || base == string(filepath.Separator) { + return "", fmt.Errorf("invalid cow disk path in snapshot metadata: %q", meta.StorageConfigs[i].Path) + } + return base, nil +} + func LoadAndValidateMeta(dir, rootDir, runDir string) (*SnapshotMeta, error) { meta, err := LoadSnapshotMeta(dir) if err != nil { diff --git a/hypervisor/snapshot_test.go b/hypervisor/snapshot_test.go index 8b750642..adcdfdad 100644 --- a/hypervisor/snapshot_test.go +++ b/hypervisor/snapshot_test.go @@ -212,3 +212,33 @@ func TestPrepareSnapshotRejectsNilNIC(t *testing.T) { t.Fatalf("err = %v, want network invariants violation", err) } } + +func TestSnapshotCOWFile(t *testing.T) { + tests := []struct { + name string + configs []*types.StorageConfig + want string + wantErr bool + }{ + {"oci raw cow", []*types.StorageConfig{ + {Path: "/other/root/blobs/a.erofs", Role: types.StorageRoleLayer}, + {Path: "/other/run/vm/cow.raw", Role: types.StorageRoleCOW}, + }, "cow.raw", false}, + {"cloudimg overlay", []*types.StorageConfig{ + {Path: "/other/run/vm/overlay.qcow2", Role: types.StorageRoleCOW}, + {Path: "/other/run/vm/cidata.img", RO: true, Role: types.StorageRoleCidata}, + }, "overlay.qcow2", false}, + {"nil entry skipped", []*types.StorageConfig{nil, {Path: "/r/cow.raw", Role: types.StorageRoleCOW}}, "cow.raw", false}, + {"no cow", []*types.StorageConfig{{Path: "/r/a.erofs", Role: types.StorageRoleLayer}}, "", true}, + {"empty path", []*types.StorageConfig{{Role: types.StorageRoleCOW}}, "", true}, + {"root path", []*types.StorageConfig{{Path: "/", Role: types.StorageRoleCOW}}, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := SnapshotCOWFile(&SnapshotMeta{StorageConfigs: tt.configs}) + if (err != nil) != tt.wantErr || got != tt.want { + t.Errorf("SnapshotCOWFile = (%q, %v), want (%q, wantErr %v)", got, err, tt.want, tt.wantErr) + } + }) + } +} diff --git a/hypervisor/utils.go b/hypervisor/utils.go index 5b1bdd8d..450bcf4e 100644 --- a/hypervisor/utils.go +++ b/hypervisor/utils.go @@ -151,11 +151,18 @@ func RemoveVMDirs(runDir, logDir string) error { ) } +// CleanupRuntimeFiles removes runDir entries by name; a name with glob metacharacters matches entries of runDir only. func CleanupRuntimeFiles(ctx context.Context, runDir string, files []string) { - for _, name := range files { - p := filepath.Join(runDir, name) - if err := os.Remove(p); err != nil && !errors.Is(err, fs.ErrNotExist) { - log.WithFunc("hypervisor.CleanupRuntimeFiles").Warnf(ctx, "cleanup %s: %v", p, err) + for _, pattern := range files { + names := []string{pattern} + if strings.ContainsAny(pattern, "*?[") { + names, _ = fs.Glob(os.DirFS(runDir), pattern) + } + for _, name := range names { + p := filepath.Join(runDir, name) + if err := os.Remove(p); err != nil && !errors.Is(err, fs.ErrNotExist) { + log.WithFunc("hypervisor.CleanupRuntimeFiles").Warnf(ctx, "cleanup %s: %v", p, err) + } } } } diff --git a/hypervisor/utils_test.go b/hypervisor/utils_test.go index ef3bfcf7..6bb604e5 100644 --- a/hypervisor/utils_test.go +++ b/hypervisor/utils_test.go @@ -364,6 +364,32 @@ func TestDiskPathByRole(t *testing.T) { } } +func TestCleanupRuntimeFilesExpandsPatterns(t *testing.T) { + runDir := t.TempDir() + for _, name := range []string{"api.sock", "snapdisk-a.raw", "snapdisk-b.qcow2", "cow.raw"} { + if err := os.WriteFile(filepath.Join(runDir, name), nil, 0o600); err != nil { + t.Fatalf("setup: %v", err) + } + } + if err := os.Symlink("/nonexistent/pts/9", filepath.Join(runDir, "console.pty")); err != nil { + t.Fatalf("setup: %v", err) + } + + CleanupRuntimeFiles(t.Context(), runDir, []string{"api.sock", "console.pty", "missing.pid", "snapdisk-*"}) + + entries, err := os.ReadDir(runDir) + if err != nil { + t.Fatalf("read run dir: %v", err) + } + var left []string + for _, e := range entries { + left = append(left, e.Name()) + } + if !slices.Equal(left, []string{"cow.raw"}) { + t.Errorf("left in run dir = %v, want [cow.raw]", left) + } +} + func TestBalloonSize(t *testing.T) { tests := []struct { name string From 091a910ef62e6f3457d569400357439f17867a79 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:41:04 +0800 Subject: [PATCH 3/9] feat(cloudhypervisor): attach a snapshot's cow copy to a running vm Identity check, pre-check, copy and vm.add-disk all run under one ops lock, so a retry after a lost result returns the attached disk instead of truncating its file. Detach removes the copy after the eject; stop, restore and start sweep snapdisk-* with the runtime files. --- hypervisor/cloudhypervisor/extend.go | 180 ++++++++++++++++++---- hypervisor/cloudhypervisor/extend_test.go | 123 +++++++++++++++ hypervisor/cloudhypervisor/utils.go | 2 +- 3 files changed, 272 insertions(+), 33 deletions(-) diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index c05e9eb1..113c2804 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/hypervisor/cloudhypervisor/extend.go @@ -5,8 +5,10 @@ import ( "encoding/json/v2" "errors" "fmt" + "maps" "net/http" "os" + "path/filepath" "slices" "strings" @@ -17,11 +19,16 @@ import ( "github.com/cocoonstack/cocoon/extend/vfio" "github.com/cocoonstack/cocoon/hypervisor" "github.com/cocoonstack/cocoon/types" + "github.com/cocoonstack/cocoon/utils" ) const ( chStateRunning = "Running" chStatePaused = "Paused" + + snapDiskPrefix = "snapdisk-" + formatRaw = "raw" + formatQcow2 = "qcow2" ) // makeBodyFn builds a device endpoint's request body from the record reloaded under the ops lock. @@ -42,22 +49,60 @@ func (ch *CloudHypervisor) DiskAttach(ctx context.Context, vmRef string, spec di return "", err } id := disk.DeriveID(spec.Name) - // The CH fork refuses disks without an explicit image_type; build from the record reloaded under the ops lock so DirectIO/queue semantics match create-path data disks (restore rewrites Config). makeBody := func(rec *hypervisor.VMRecord) any { - d := storageConfigToDisk(&types.StorageConfig{ + return hotDisk(rec, id, &types.StorageConfig{ Role: types.StorageRoleData, Path: path, Serial: spec.Name, RO: spec.ReadOnly, DirectIO: spec.DirectIO, - }, rec.Config.CPU, rec.Config.DiskQueueSize, rec.Config.NoDirectIO, hypervisor.QueueCPUs(rec)) - d.ID = id - return d + }) } return ch.attachWith(ctx, vmRef, "vm.add-disk", makeBody, id, func(info *chVMInfoResponse) (string, error) { return diskAttached(info, id, spec.Name, path, spec.ReadOnly) }) } +func (ch *CloudHypervisor) DiskAttachSnapshot(ctx context.Context, vmRef string, src disk.SnapshotSource) (string, string, error) { + if err := src.Normalize(); err != nil { + return "", "", err + } + meta, err := hypervisor.LoadSnapshotMeta(src.Dir) + if err != nil { + return "", "", fmt.Errorf("load snapshot metadata: %w", err) + } + cowFile, err := hypervisor.SnapshotCOWFile(meta) + if err != nil { + return "", "", err + } + format := snapshotDiskFormat(cowFile) + id := disk.DeriveID(src.Name) + hc, rec, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) + if err != nil { + return "", "", err + } + defer unlock() + if err = checkSnapshotImage(meta.StorageConfigs, src.BlobIDs, &rec); err != nil { + return "", "", err + } + path := snapshotDiskPath(rec.RunDir, src.Name, format) + existing, err := diskAttached(info, id, src.Name, path, false) + if err != nil || existing != "" { + return existing, format, err + } + if err = copySnapshotDisk(ctx, path, filepath.Join(src.Dir, cowFile), src.BasePath); err != nil { + os.Remove(path) //nolint:errcheck,gosec + return "", "", err + } + deviceID, err := addDevice(ctx, hc, "vm.add-disk", hotDisk(&rec, id, &types.StorageConfig{ + Role: types.StorageRoleData, Path: path, Serial: src.Name, + }), id) + if err != nil { + os.Remove(path) //nolint:errcheck,gosec + return "", "", err + } + return deviceID, format, nil +} + func (ch *CloudHypervisor) DiskDetach(ctx context.Context, vmRef, name string) error { - if name == "" { - return fmt.Errorf("name is required") + if err := disk.ValidateName(name); err != nil { + return err } id := disk.DeriveID(name) return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, bool, error) { @@ -69,6 +114,8 @@ func (ch *CloudHypervisor) DiskDetach(ctx context.Context, vmRef, name string) e } log.WithFunc("cloudhypervisor.DiskDetach").Warnf(ctx, "disk %q is not attached to vm %s, nothing to detach", name, vmRef) return "", false, nil + }, func(rec *hypervisor.VMRecord) error { + return removeSnapshotDisks(rec.RunDir, name) }) } @@ -125,7 +172,7 @@ func (ch *CloudHypervisor) FsDetach(ctx context.Context, vmRef, tag string) erro } } return "", false, fmt.Errorf("fs tag %q not attached", tag) - }) + }, nil) } func (ch *CloudHypervisor) FsList(ctx context.Context, vmRef string) ([]fs.Attached, error) { @@ -176,7 +223,7 @@ func (ch *CloudHypervisor) DeviceDetach(ctx context.Context, vmRef, id string) e return id, true, nil } return "", false, fmt.Errorf("device id %q not attached", id) - }) + }, nil) } func (ch *CloudHypervisor) DeviceList(ctx context.Context, vmRef string) ([]vfio.Attached, error) { @@ -216,30 +263,11 @@ func (ch *CloudHypervisor) attachWith(ctx context.Context, vmRef, endpoint strin if checkErr != nil || existing != "" { return existing, checkErr } - bodyBytes, err := json.Marshal(makeBody(&rec)) - if err != nil { - return "", fmt.Errorf("marshal %s: %w", endpoint, err) - } - resp, err := vmAPIOnce(ctx, hc, endpoint, bodyBytes, http.StatusOK, http.StatusNoContent) - if err != nil { - return "", fmt.Errorf("%s: %w", endpoint, err) - } - pci, err := decodePciDeviceInfo(resp) - if err != nil { - return "", err - } - if pci.ID != "" { - return pci.ID, nil - } - // VFIO without --id has no detach key, so an empty fallback must fail. - if fallbackID == "" { - return "", fmt.Errorf("%s: empty response body and no fallback id (CH returned no PciDeviceInfo)", endpoint) - } - return fallbackID, nil + return addDevice(ctx, hc, endpoint, makeBody(&rec), fallbackID) } -func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID findIDFn) error { - hc, _, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) +func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID findIDFn, afterEject func(*hypervisor.VMRecord) error) error { + hc, rec, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) if err != nil { return err } @@ -257,7 +285,10 @@ func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID if err := waitDeviceEjected(ctx, hc, deviceID); err != nil { return fmt.Errorf("device %s removal initiated but the guest has not ejected it: %w", deviceID, err) } - return nil + if afterEject == nil { + return nil + } + return afterEject(&rec) } // listWith returns nil (not error) for stopped VMs so inspect can omit the field. @@ -276,6 +307,36 @@ func (ch *CloudHypervisor) listWith[A any](ctx context.Context, vmRef string, ex return extract(info), nil } +func addDevice(ctx context.Context, hc *http.Client, endpoint string, body any, fallbackID string) (string, error) { + bodyBytes, err := json.Marshal(body) + if err != nil { + return "", fmt.Errorf("marshal %s: %w", endpoint, err) + } + resp, err := vmAPIOnce(ctx, hc, endpoint, bodyBytes, http.StatusOK, http.StatusNoContent) + if err != nil { + return "", fmt.Errorf("%s: %w", endpoint, err) + } + pci, err := decodePciDeviceInfo(resp) + if err != nil { + return "", err + } + if pci.ID != "" { + return pci.ID, nil + } + // VFIO without --id has no detach key, so an empty fallback must fail. + if fallbackID == "" { + return "", fmt.Errorf("%s: empty response body and no fallback id (CH returned no PciDeviceInfo)", endpoint) + } + return fallbackID, nil +} + +// The CH fork refuses disks without an explicit image_type; build from the record reloaded under the ops lock so DirectIO/queue semantics match create-path data disks (restore rewrites Config). +func hotDisk(rec *hypervisor.VMRecord, id string, sc *types.StorageConfig) chDisk { + d := storageConfigToDisk(sc, rec.Config.CPU, rec.Config.DiskQueueSize, rec.Config.NoDirectIO, hypervisor.QueueCPUs(rec)) + d.ID = id + return d +} + func diskAttached(info *chVMInfoResponse, id, name, path string, readOnly bool) (string, error) { for _, ex := range info.Config.Disks { if ex.ID == id { @@ -316,3 +377,58 @@ func bdfFromSysfsPath(p string) string { } return bdf } + +// checkSnapshotImage compares digests only, so a sidecar written under another node's root still matches. +func checkSnapshotImage(sidecar []*types.StorageConfig, blobIDs map[string]struct{}, rec *hypervisor.VMRecord) error { + snapLayers, vmLayers := layerHexes(sidecar), layerHexes(rec.StorageConfigs) + if slices.Equal(snapLayers, vmLayers) && maps.Equal(blobIDs, rec.ImageBlobIDs) { + return nil + } + return fmt.Errorf("snapshot image (layers %v, blobs %v) does not match vm %s (layers %v, blobs %v)", + snapLayers, slices.Sorted(maps.Keys(blobIDs)), rec.ID, vmLayers, slices.Sorted(maps.Keys(rec.ImageBlobIDs))) +} + +func layerHexes(configs []*types.StorageConfig) []string { + var out []string + for _, sc := range configs { + if sc != nil && sc.Role == types.StorageRoleLayer { + out = append(out, hypervisor.BlobHexFromPath(sc.Path)) + } + } + return out +} + +func snapshotDiskFormat(cowFile string) string { + if filepath.Ext(cowFile) == "."+formatQcow2 { + return formatQcow2 + } + return formatRaw +} + +func snapshotDiskPath(runDir, name, format string) string { + return filepath.Join(runDir, snapDiskPrefix+name+"."+format) +} + +// The copy is throwaway, so it skips fsync; the rebase replaces a backing path that may name another node's root. +func copySnapshotDisk(ctx context.Context, dst, src, basePath string) error { + if err := utils.ReflinkCopy(ctx, dst, src, utils.NoSync); err != nil { + return fmt.Errorf("copy snapshot disk %s: %w", filepath.Base(src), err) + } + if basePath == "" { + return nil + } + if err := utils.RunQemuImg(ctx, "rebase", "-u", "-F", formatQcow2, "-b", basePath, dst); err != nil { + return fmt.Errorf("rebase snapshot disk onto %s: %w", basePath, err) + } + return nil +} + +func removeSnapshotDisks(runDir, name string) error { + var errs []error + for _, format := range []string{formatRaw, formatQcow2} { + if err := os.Remove(snapshotDiskPath(runDir, name, format)); err != nil && !errors.Is(err, os.ErrNotExist) { + errs = append(errs, fmt.Errorf("remove snapshot disk copy: %w", err)) + } + } + return errors.Join(errs...) +} diff --git a/hypervisor/cloudhypervisor/extend_test.go b/hypervisor/cloudhypervisor/extend_test.go index d6624a42..128b2c29 100644 --- a/hypervisor/cloudhypervisor/extend_test.go +++ b/hypervisor/cloudhypervisor/extend_test.go @@ -1,10 +1,15 @@ package cloudhypervisor import ( + "os" + "path/filepath" + "slices" "strings" "testing" "github.com/cocoonstack/cocoon/extend/disk" + "github.com/cocoonstack/cocoon/hypervisor" + "github.com/cocoonstack/cocoon/types" ) func TestDiskAttached(t *testing.T) { @@ -41,3 +46,121 @@ func TestDiskAttached(t *testing.T) { }) } } + +func TestCheckSnapshotImage(t *testing.T) { + rec := &hypervisor.VMRecord{ + StorageConfigs: []*types.StorageConfig{ + {Path: "/local/blobs/aaa.erofs", RO: true, Role: types.StorageRoleLayer}, + {Path: "/local/blobs/bbb.erofs", RO: true, Role: types.StorageRoleLayer}, + {Path: "/local/run/vm/cow.raw", Role: types.StorageRoleCOW}, + }, + ImageBlobIDs: map[string]struct{}{"aaa": {}, "bbb": {}, "kkk": {}}, + } + rec.ID = "target-vm" + tests := []struct { + name string + layers []string + blobIDs map[string]struct{} + wantErr bool + }{ + {"same image on another root", []string{"/remote/blobs/aaa.erofs", "/remote/blobs/bbb.erofs"}, map[string]struct{}{"aaa": {}, "bbb": {}, "kkk": {}}, false}, + {"layers in another order", []string{"/remote/blobs/bbb.erofs", "/remote/blobs/aaa.erofs"}, map[string]struct{}{"aaa": {}, "bbb": {}, "kkk": {}}, true}, + {"another blob set", []string{"/remote/blobs/aaa.erofs", "/remote/blobs/bbb.erofs"}, map[string]struct{}{"aaa": {}, "bbb": {}, "zzz": {}}, true}, + {"another image", []string{"/remote/blobs/ccc.erofs"}, map[string]struct{}{"ccc": {}, "kkk": {}}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sidecar := []*types.StorageConfig{{Path: "/remote/run/vm/cow.raw", Role: types.StorageRoleCOW}} + for _, p := range tt.layers { + sidecar = append(sidecar, &types.StorageConfig{Path: p, RO: true, Role: types.StorageRoleLayer}) + } + err := checkSnapshotImage(sidecar, tt.blobIDs, rec) + if (err != nil) != tt.wantErr { + t.Fatalf("checkSnapshotImage = %v, wantErr %v", err, tt.wantErr) + } + if err != nil && (!strings.Contains(err.Error(), "target-vm") || !strings.Contains(err.Error(), "[aaa bbb]") || !strings.Contains(err.Error(), "[aaa bbb kkk]")) { + t.Errorf("error %q does not name both sides", err) + } + }) + } +} + +func TestCheckSnapshotImageCloudimg(t *testing.T) { + rec := &hypervisor.VMRecord{ + StorageConfigs: []*types.StorageConfig{{Path: "/local/run/vm/overlay.qcow2", Role: types.StorageRoleCOW}}, + ImageBlobIDs: map[string]struct{}{"base": {}}, + } + sidecar := []*types.StorageConfig{{Path: "/remote/run/vm/overlay.qcow2", Role: types.StorageRoleCOW}} + if err := checkSnapshotImage(sidecar, map[string]struct{}{"base": {}}, rec); err != nil { + t.Errorf("same base rejected: %v", err) + } + if err := checkSnapshotImage(sidecar, map[string]struct{}{"other": {}}, rec); err == nil { + t.Error("another base accepted") + } +} + +func TestSnapshotDiskPathKeepsTheFormat(t *testing.T) { + tests := []struct { + cowFile, wantFormat, wantPath string + }{ + {"cow.raw", formatRaw, "/run/vm/snapdisk-snap-ab.raw"}, + {"overlay.qcow2", formatQcow2, "/run/vm/snapdisk-snap-ab.qcow2"}, + } + for _, tt := range tests { + format := snapshotDiskFormat(tt.cowFile) + if format != tt.wantFormat { + t.Errorf("snapshotDiskFormat(%q) = %q, want %q", tt.cowFile, format, tt.wantFormat) + } + if got := snapshotDiskPath("/run/vm", "snap-ab", format); got != tt.wantPath { + t.Errorf("snapshotDiskPath = %q, want %q", got, tt.wantPath) + } + } +} + +func TestRemoveSnapshotDisks(t *testing.T) { + runDir := t.TempDir() + writeRunFiles(t, runDir, "snapdisk-a.raw", "snapdisk-a.qcow2", "snapdisk-b.raw", "cow.raw") + + if err := removeSnapshotDisks(runDir, "a"); err != nil { + t.Fatalf("removeSnapshotDisks: %v", err) + } + if err := removeSnapshotDisks(runDir, "a"); err != nil { + t.Fatalf("repeat removeSnapshotDisks: %v", err) + } + if got := runFiles(t, runDir); !slices.Equal(got, []string{"cow.raw", "snapdisk-b.raw"}) { + t.Errorf("left in run dir = %v, want [cow.raw snapdisk-b.raw]", got) + } +} + +func TestRuntimeFilesSweepSnapshotDisks(t *testing.T) { + runDir := t.TempDir() + writeRunFiles(t, runDir, "snapdisk-a.raw", "snapdisk-b.qcow2", pidFileName, "cow.raw", "overlay.qcow2") + + hypervisor.CleanupRuntimeFiles(t.Context(), runDir, runtimeFiles) + + if got := runFiles(t, runDir); !slices.Equal(got, []string{"cow.raw", "overlay.qcow2"}) { + t.Errorf("left in run dir = %v, want [cow.raw overlay.qcow2]", got) + } +} + +func writeRunFiles(t *testing.T, dir string, names ...string) { + t.Helper() + for _, name := range names { + if err := os.WriteFile(filepath.Join(dir, name), nil, 0o600); err != nil { + t.Fatalf("setup: %v", err) + } + } +} + +func runFiles(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + return names +} diff --git a/hypervisor/cloudhypervisor/utils.go b/hypervisor/cloudhypervisor/utils.go index 3a7b2042..8ffa09ef 100644 --- a/hypervisor/cloudhypervisor/utils.go +++ b/hypervisor/cloudhypervisor/utils.go @@ -44,7 +44,7 @@ const ( chMemoryRestoreMmap chMemoryRestoreMode = "CopyOnWrite" ) -var runtimeFiles = []string{hypervisor.APISocketName, pidFileName, hypervisor.ConsoleSockName, hypervisor.ConsolePTYName, cmdlineFileName, hypervisor.VsockSockName} +var runtimeFiles = []string{hypervisor.APISocketName, pidFileName, hypervisor.ConsoleSockName, hypervisor.ConsolePTYName, cmdlineFileName, hypervisor.VsockSockName, snapDiskPrefix + "*"} type chMemoryRestoreMode string From cc5bc8d4886625b21e4dece66da8f5e0f1753e1b Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:41:04 +0800 Subject: [PATCH 4/9] feat(snapshot): list the files a disk attach reads in the export envelope Both export paths fill disk_attach_files from the sidecar, so an archive consumer fetches only those. A snapshot without a sidecar exports as before, without the field. --- snapshot/envelope.go | 10 ++--- snapshot/envelope_test.go | 2 +- snapshot/localfile/export.go | 31 ++++++++++++- snapshot/localfile/localfile_test.go | 67 +++++++++++++++++++++++++++- types/snapshot.go | 2 + 5 files changed, 103 insertions(+), 9 deletions(-) diff --git a/snapshot/envelope.go b/snapshot/envelope.go index d76347e1..10da0648 100644 --- a/snapshot/envelope.go +++ b/snapshot/envelope.go @@ -37,9 +37,9 @@ func ReadSnapshotEnvelope(dir string) (types.SnapshotExport, error) { return envelope, nil } -// MarshalEnvelope returns the indented snapshot.json bytes for cfg and the files exported beside it. -func MarshalEnvelope(cfg types.SnapshotConfig, files []string) ([]byte, error) { - data, err := json.Marshal(types.SnapshotExport{Version: EnvelopeVersion, Config: cfg, Files: files}, json.Deterministic(true), jsontext.WithIndent(" ")) +// MarshalEnvelope returns the indented snapshot.json bytes for cfg, the files exported beside it, and the subset a disk attach reads. +func MarshalEnvelope(cfg types.SnapshotConfig, files, diskAttachFiles []string) ([]byte, error) { + data, err := json.Marshal(types.SnapshotExport{Version: EnvelopeVersion, Config: cfg, Files: files, DiskAttachFiles: diskAttachFiles}, json.Deterministic(true), jsontext.WithIndent(" ")) if err != nil { return nil, fmt.Errorf("marshal snapshot envelope: %w", err) } @@ -47,8 +47,8 @@ func MarshalEnvelope(cfg types.SnapshotConfig, files []string) ([]byte, error) { } // WriteSnapshotEnvelope writes /snapshot.json atomically so a concurrent reader can't see a partial write. -func WriteSnapshotEnvelope(dir string, cfg types.SnapshotConfig, files []string) error { - data, err := MarshalEnvelope(cfg, files) +func WriteSnapshotEnvelope(dir string, cfg types.SnapshotConfig, files, diskAttachFiles []string) error { + data, err := MarshalEnvelope(cfg, files, diskAttachFiles) if err != nil { return err } diff --git a/snapshot/envelope_test.go b/snapshot/envelope_test.go index bfe94d6e..0ca8acc6 100644 --- a/snapshot/envelope_test.go +++ b/snapshot/envelope_test.go @@ -61,7 +61,7 @@ func TestReadSnapshotEnvelope_RoundTrip(t *testing.T) { Hypervisor: "cloud-hypervisor", NICs: 1, } - if err := WriteSnapshotEnvelope(dir, cfg, []string{"cow.raw", "vmstate"}); err != nil { + if err := WriteSnapshotEnvelope(dir, cfg, []string{"cow.raw", "vmstate"}, nil); err != nil { t.Fatalf("write: %v", err) } got, err := ReadSnapshotEnvelope(dir) diff --git a/snapshot/localfile/export.go b/snapshot/localfile/export.go index 1c925a03..36587f23 100644 --- a/snapshot/localfile/export.go +++ b/snapshot/localfile/export.go @@ -4,12 +4,15 @@ import ( "archive/tar" "compress/gzip" "context" + "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "time" + "github.com/cocoonstack/cocoon/hypervisor" "github.com/cocoonstack/cocoon/snapshot" "github.com/cocoonstack/cocoon/utils" ) @@ -45,6 +48,10 @@ func (lf *LocalFile) ExportToDir(ctx context.Context, ref, dir string) (err erro if err != nil { return err } + attachFiles, err := diskAttachFiles(dataDir) + if err != nil { + return err + } // a partial tree would make the retry refuse the non-empty target defer func() { if err != nil { @@ -62,7 +69,7 @@ func (lf *LocalFile) ExportToDir(ctx context.Context, ref, dir string) (err erro }); err != nil { return err } - if err = snapshot.WriteSnapshotEnvelope(dir, cfg, names); err != nil { + if err = snapshot.WriteSnapshotEnvelope(dir, cfg, names, attachFiles); err != nil { return fmt.Errorf("write envelope: %w", err) } return nil @@ -79,7 +86,12 @@ func (lf *LocalFile) export(ctx context.Context, ref string, compress bool) (io. release() return nil, err } - jsonData, err := snapshot.MarshalEnvelope(cfg, files) + attachFiles, err := diskAttachFiles(dataDir) + if err != nil { + release() + return nil, err + } + jsonData, err := snapshot.MarshalEnvelope(cfg, files, attachFiles) if err != nil { release() return nil, err @@ -118,3 +130,18 @@ func (lf *LocalFile) export(ctx context.Context, ref string, compress bool) (io. return nil }), nil } + +func diskAttachFiles(dataDir string) ([]string, error) { + meta, err := hypervisor.LoadSnapshotMeta(dataDir) + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("load snapshot metadata: %w", err) + } + cowFile, err := hypervisor.SnapshotCOWFile(meta) + if err != nil { + return nil, err + } + return []string{hypervisor.SnapshotMetaFile, cowFile}, nil +} diff --git a/snapshot/localfile/localfile_test.go b/snapshot/localfile/localfile_test.go index ae1f5306..e2afd92c 100644 --- a/snapshot/localfile/localfile_test.go +++ b/snapshot/localfile/localfile_test.go @@ -217,7 +217,7 @@ func TestImportEmitsSnapStorageStart(t *testing.T) { ID: "src-snap", Name: "src-name", Hypervisor: "cloud-hypervisor", - }, nil) + }, nil, nil) if err != nil { t.Fatalf("MarshalEnvelope: %v", err) } @@ -1466,6 +1466,54 @@ func TestImportRejectsEveryTruncatedExport(t *testing.T) { } } +func TestExportEnvelopeListsDiskAttachFiles(t *testing.T) { + tests := []struct { + name string + files map[string][]byte + want []string + }{ + {"oci", map[string][]byte{ + "cocoon.json": []byte(`{"storage_configs":[{"path":"/other/root/blobs/a.erofs","role":"layer"},{"path":"/other/run/vm/cow.raw","role":"cow"}]}`), + "cow.raw": []byte("disk"), + "memory": []byte("mem"), + }, []string{"cocoon.json", "cow.raw"}}, + {"cloudimg", map[string][]byte{ + "cocoon.json": []byte(`{"storage_configs":[{"path":"/other/run/vm/overlay.qcow2","role":"cow"}]}`), + "overlay.qcow2": []byte("qcow"), + }, []string{"cocoon.json", "overlay.qcow2"}}, + {"no sidecar", map[string][]byte{"cow.raw": []byte("disk")}, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lf := newTestLF(t) + ctx := t.Context() + id := makeExportableSnapshot(t, lf, "attach-src", tt.files) + + dst := filepath.Join(t.TempDir(), "exported") + if err := lf.ExportToDir(ctx, id, dst); err != nil { + t.Fatalf("ExportToDir: %v", err) + } + dirEnvelope, err := snapshot.ReadSnapshotEnvelope(dst) + if err != nil { + t.Fatalf("ReadSnapshotEnvelope: %v", err) + } + stream, err := lf.Export(ctx, id) + if err != nil { + t.Fatalf("Export: %v", err) + } + defer stream.Close() + tarEnvelope := readTarEnvelope(t, stream) + + if !slices.Equal(dirEnvelope.DiskAttachFiles, tt.want) { + t.Errorf("to-dir disk_attach_files = %v, want %v", dirEnvelope.DiskAttachFiles, tt.want) + } + if !slices.Equal(tarEnvelope.DiskAttachFiles, tt.want) { + t.Errorf("tar disk_attach_files = %v, want %v", tarEnvelope.DiskAttachFiles, tt.want) + } + }) + } +} + func testID(t *testing.T) string { t.Helper() return utils.GenerateID() @@ -1589,3 +1637,20 @@ func newTestMetaStore(t *testing.T, conf *config.Config) *metajson.Store { t.Cleanup(func() { _ = store.Close() }) return store } + +func readTarEnvelope(t *testing.T, r io.Reader) types.SnapshotExport { + t.Helper() + tr := tar.NewReader(r) + hdr, err := tr.Next() + if err != nil { + t.Fatalf("read tar: %v", err) + } + if hdr.Name != snapshot.SnapshotJSONName { + t.Fatalf("first tar entry = %q, want %s", hdr.Name, snapshot.SnapshotJSONName) + } + var envelope types.SnapshotExport + if err := json.UnmarshalRead(tr, &envelope); err != nil { + t.Fatalf("decode envelope: %v", err) + } + return envelope +} diff --git a/types/snapshot.go b/types/snapshot.go index e99107f0..1f099cf0 100644 --- a/types/snapshot.go +++ b/types/snapshot.go @@ -37,4 +37,6 @@ type SnapshotExport struct { Config SnapshotConfig `json:"config"` Version int `json:"version"` Files []string `json:"files,omitempty"` // data-dir files the export carries; absent on exports older than the manifest + + DiskAttachFiles []string `json:"disk_attach_files,omitempty"` // the subset `vm disk attach --from-dir` reads } From c20d5848d4a3813c810103f57e9ea0600a85b773 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:42:39 +0800 Subject: [PATCH 5/9] feat(cmd): attach a snapshot's cow copy with vm disk attach --from-snapshot/--from-dir --from-snapshot holds the snapshot's read lease across the attach; --from-dir reads the envelope. The cmd layer resolves the local cloudimg base so the hypervisor stays free of image-store knowledge. --name defaults to snap-; --readonly and --directio do not apply to the copy. --- cmd/vm/commands.go | 18 ++++++--- cmd/vm/disk.go | 86 +++++++++++++++++++++++++++++++++++++++++-- cmd/vm/disk_test.go | 47 +++++++++++++++++++++++ images/cloudimg/gc.go | 2 + 4 files changed, 143 insertions(+), 10 deletions(-) create mode 100644 cmd/vm/disk_test.go diff --git a/cmd/vm/commands.go b/cmd/vm/commands.go index e752844d..7b947aa3 100644 --- a/cmd/vm/commands.go +++ b/cmd/vm/commands.go @@ -222,21 +222,27 @@ func buildDiskCommand(h Handler) *cobra.Command { attach := &cobra.Command{ Use: useAttachVM, - Short: "Hot-attach an existing raw disk file to a running VM", + Short: "Hot-attach an existing raw disk file, or a private copy of a snapshot's COW disk (CH only), to a running VM", Args: cobra.ExactArgs(1), RunE: h.DiskAttach, } - attach.Flags().String("path", "", "absolute path to an existing raw disk file (required; never deleted by cocoon)") - attach.Flags().String("name", "", "disk name: guest /dev/disk/by-id/virtio- and the detach key (required)") + attach.Flags().String("path", "", "absolute path to an existing raw disk file (never deleted by cocoon)") + attach.Flags().String("from-snapshot", "", "attach a read-write copy of this local snapshot's COW disk; the VM must run the snapshot's image") + attach.Flags().String("from-dir", "", "attach a read-write copy of the COW disk in this export dir (snapshot.json + its disk_attach_files)") + attach.Flags().String("name", "", "disk name: guest /dev/disk/by-id/virtio- and the detach key (required with --path; default snap-)") attach.Flags().Bool("readonly", false, "attach read-only") attach.Flags().String("directio", "auto", "O_DIRECT for the disk: on|off|auto (off for files on tmpfs)") - _ = attach.MarkFlagRequired("path") - _ = attach.MarkFlagRequired("name") + attach.MarkFlagsOneRequired("path", "from-snapshot", "from-dir") + attach.MarkFlagsMutuallyExclusive("path", "from-snapshot", "from-dir") + attach.MarkFlagsMutuallyExclusive("readonly", "from-snapshot") + attach.MarkFlagsMutuallyExclusive("readonly", "from-dir") + attach.MarkFlagsMutuallyExclusive("directio", "from-snapshot") + attach.MarkFlagsMutuallyExclusive("directio", "from-dir") cliutil.AddOutputFlag(attach) detach := &cobra.Command{ Use: useDetachVM, - Short: "Detach a hot-attached disk from a running VM (keeps the backing file)", + Short: "Detach a hot-attached disk from a running VM (keeps a --path backing file, removes a snapshot copy)", Args: cobra.ExactArgs(1), RunE: h.DiskDetach, } diff --git a/cmd/vm/disk.go b/cmd/vm/disk.go index b379ffd3..4cd04144 100644 --- a/cmd/vm/disk.go +++ b/cmd/vm/disk.go @@ -1,21 +1,34 @@ package vm import ( + "cmp" + "context" + "fmt" + "github.com/projecteru2/core/log" "github.com/spf13/cobra" + cmdcore "github.com/cocoonstack/cocoon/cmd/core" + "github.com/cocoonstack/cocoon/config" "github.com/cocoonstack/cocoon/extend/disk" + "github.com/cocoonstack/cocoon/snapshot" "github.com/cocoonstack/cocoon/types" ) type diskResult struct { - VM string `json:"vm"` - Name string `json:"name"` - ID string `json:"id,omitempty"` - Hints []string `json:"hints,omitempty"` + VM string `json:"vm"` + Name string `json:"name"` + ID string `json:"id,omitempty"` + Format string `json:"format,omitempty"` + Hints []string `json:"hints,omitempty"` } func (h Handler) DiskAttach(cmd *cobra.Command, args []string) error { + fromSnapshot, _ := cmd.Flags().GetString("from-snapshot") + fromDir, _ := cmd.Flags().GetString("from-dir") + if fromSnapshot != "" || fromDir != "" { + return h.diskAttachSnapshot(cmd, args, fromSnapshot, fromDir) + } ctx, _, hyper, a, err := h.resolveAttacher[disk.Attacher](cmd, args, "disk attach", disk.ErrUnsupportedBackend) if err != nil { return err @@ -60,3 +73,68 @@ func (h Handler) DiskDetach(cmd *cobra.Command, args []string) error { printGuestHints(out.Hints) }) } + +func (h Handler) diskAttachSnapshot(cmd *cobra.Command, args []string, fromSnapshot, fromDir string) error { + ctx, conf, _, a, err := h.resolveAttacher[disk.SnapshotAttacher](cmd, args, "snapshot disk attach", disk.ErrUnsupportedBackend) + if err != nil { + return err + } + dir, cfg, release, err := openSnapshotSource(ctx, conf, fromSnapshot, fromDir) + if err != nil { + return err + } + defer release() + basePath, err := cloudimgBasePath(ctx, conf, cfg.ImageBlobIDs) + if err != nil { + return err + } + name, _ := cmd.Flags().GetString("name") + name = cmp.Or(name, disk.SnapshotName(cfg.ID)) + id, format, err := a.DiskAttachSnapshot(ctx, args[0], disk.SnapshotSource{Dir: dir, Name: name, BlobIDs: cfg.ImageBlobIDs, BasePath: basePath}) + if err != nil { + return classifyAttachErr(err) + } + return outputOrLog(cmd, diskResult{VM: args[0], Name: name, ID: id, Format: format}, func() { + log.WithFunc("cmd.vm.disk.attach").Infof(ctx, "attached snapshot disk name=%s id=%s format=%s vm=%s", name, id, format, args[0]) + }) +} + +// openSnapshotSource holds a --from-snapshot read lease until release, so delete/GC cannot reap the dir mid-copy. +func openSnapshotSource(ctx context.Context, conf *config.Config, fromSnapshot, fromDir string) (string, types.SnapshotConfig, func(), error) { + if fromDir != "" { + envelope, err := snapshot.ReadSnapshotEnvelope(fromDir) + if err != nil { + return "", types.SnapshotConfig{}, nil, fmt.Errorf("load envelope: %w", err) + } + if err = snapshot.VerifyCOWSize(fromDir, envelope.Config); err != nil { + return "", types.SnapshotConfig{}, nil, err + } + return fromDir, envelope.Config, func() {}, nil + } + snapBackend, err := cmdcore.InitSnapshot(ctx, conf) + if err != nil { + return "", types.SnapshotConfig{}, nil, err + } + da, ok := snapBackend.(snapshot.Direct) + if !ok { + return "", types.SnapshotConfig{}, nil, fmt.Errorf("snapshot backend %s has no local data dir", snapBackend.Type()) + } + dir, cfg, release, err := da.DataDir(ctx, fromSnapshot) + if err != nil { + return "", types.SnapshotConfig{}, nil, fmt.Errorf("open snapshot %s: %w", fromSnapshot, err) + } + return dir, cfg, release, nil +} + +func cloudimgBasePath(ctx context.Context, conf *config.Config, blobIDs map[string]struct{}) (string, error) { + _, cloudimgStore, err := cmdcore.InitImageBackendsForPull(ctx, conf) + if err != nil { + return "", err + } + for hex := range blobIDs { + if cloudimgStore.OwnsBlob(hex) { + return cloudimgStore.BlobPath(hex), nil + } + } + return "", nil +} diff --git a/cmd/vm/disk_test.go b/cmd/vm/disk_test.go new file mode 100644 index 00000000..c0646f78 --- /dev/null +++ b/cmd/vm/disk_test.go @@ -0,0 +1,47 @@ +package vm + +import ( + "slices" + "testing" + + "github.com/spf13/cobra" +) + +func TestDiskAttachFlagGroups(t *testing.T) { + tests := []struct { + name string + args []string + wantErr bool + }{ + {"path", []string{"--path", "/vols/d.raw", "--name", "d", "--readonly", "--directio", "off"}, false}, + {"from snapshot", []string{"--from-snapshot", "snap"}, false}, + {"from dir with name", []string{"--from-dir", "/exports/snap", "--name", "reader"}, false}, + {"no source", []string{"--name", "d"}, true}, + {"path and snapshot", []string{"--path", "/vols/d.raw", "--from-snapshot", "snap"}, true}, + {"snapshot and dir", []string{"--from-snapshot", "snap", "--from-dir", "/exports/snap"}, true}, + {"readonly snapshot", []string{"--from-snapshot", "snap", "--readonly"}, true}, + {"directio dir", []string{"--from-dir", "/exports/snap", "--directio", "off"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + attach := diskSubcommand(t, "attach") + if err := attach.ParseFlags(tt.args); err != nil { + t.Fatalf("parse flags: %v", err) + } + err := attach.ValidateFlagGroups() + if (err != nil) != tt.wantErr { + t.Errorf("ValidateFlagGroups() = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func diskSubcommand(t *testing.T, name string) *cobra.Command { + t.Helper() + subs := buildDiskCommand(Handler{}).Commands() + i := slices.IndexFunc(subs, func(c *cobra.Command) bool { return c.Name() == name }) + if i < 0 { + t.Fatalf("disk %s subcommand missing", name) + } + return subs[i] +} diff --git a/images/cloudimg/gc.go b/images/cloudimg/gc.go index fe891c8b..7bb7987d 100644 --- a/images/cloudimg/gc.go +++ b/images/cloudimg/gc.go @@ -38,3 +38,5 @@ func (c *CloudImg) PinBlobs(_ context.Context, blobIDs map[string]struct{}) (fun } func (c *CloudImg) OwnsBlob(hex string) bool { return c.conf.OwnsBlob(hex) } + +func (c *CloudImg) BlobPath(hex string) string { return c.conf.BlobPath(hex) } From 9f4046a4121883093614e4d5f110a3dfbb524a85 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:43:07 +0800 Subject: [PATCH 6/9] docs: list the vm disk attach --from-snapshot/--from-dir flags --- docs/devices.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/devices.md b/docs/devices.md index d3c18524..578d32bb 100644 --- a/docs/devices.md +++ b/docs/devices.md @@ -72,10 +72,12 @@ Flags: | Flag | Default | Description | |------|---------|-------------| -| `--path` | required | Absolute path to an existing raw disk file | -| `--name` | required | Guest serial and detach key (`^[a-z][a-z0-9_-]{0,19}$`; the `cocoon-` prefix is reserved) | -| `--readonly` | `false` | Attach read-only | -| `--directio` | `auto` | O_DIRECT for the disk: `on`/`off`/`auto` (use `off` for files on tmpfs); Cloud Hypervisor only, Firecracker ignores it with a warning | +| `--path` | empty | Absolute path to an existing raw disk file; exactly one of `--path`, `--from-snapshot`, `--from-dir` is required | +| `--from-snapshot` | empty | Cloud Hypervisor only: attach a private read-write copy of this local snapshot's COW disk (`raw` for OCI, `qcow2` for cloudimg, reported as `format`); the VM must run the snapshot's image, layers in the same order; detach, stop and `vm rm` remove the copy | +| `--from-dir` | empty | As `--from-snapshot`, from an export dir that holds `snapshot.json` and the files its `disk_attach_files` lists, possibly exported on another node | +| `--name` | required with `--path` | Guest serial and detach key (`^[a-z][a-z0-9_-]{0,19}$`; the `cocoon-` prefix is reserved); defaults to `snap-` plus the first 8 characters of the snapshot id with `--from-snapshot`/`--from-dir` | +| `--readonly` | `false` | Attach read-only; not with `--from-snapshot`/`--from-dir` | +| `--directio` | `auto` | O_DIRECT for the disk: `on`/`off`/`auto` (use `off` for files on tmpfs); Cloud Hypervisor only, Firecracker ignores it with a warning; not with `--from-snapshot`/`--from-dir` | On Cloud Hypervisor, detach blocks until the guest acks the ACPI eject (up to 30s — Windows can take 10–20s), so when it returns the slot, the name, and the From f506db37dedac47c7f706b520f3fbe20642f8b33 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:53:18 +0800 Subject: [PATCH 7/9] cut: drop the cow path shape guard and fold the attach flag exclusions The sidecar is cocoon-written and a degenerate cow path already fails the copy loudly, so the basename guard defended nothing reachable. --- cmd/vm/commands.go | 6 ++---- hypervisor/snapshot.go | 6 +----- hypervisor/snapshot_test.go | 2 -- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/cmd/vm/commands.go b/cmd/vm/commands.go index 7b947aa3..15f55d58 100644 --- a/cmd/vm/commands.go +++ b/cmd/vm/commands.go @@ -234,10 +234,8 @@ func buildDiskCommand(h Handler) *cobra.Command { attach.Flags().String("directio", "auto", "O_DIRECT for the disk: on|off|auto (off for files on tmpfs)") attach.MarkFlagsOneRequired("path", "from-snapshot", "from-dir") attach.MarkFlagsMutuallyExclusive("path", "from-snapshot", "from-dir") - attach.MarkFlagsMutuallyExclusive("readonly", "from-snapshot") - attach.MarkFlagsMutuallyExclusive("readonly", "from-dir") - attach.MarkFlagsMutuallyExclusive("directio", "from-snapshot") - attach.MarkFlagsMutuallyExclusive("directio", "from-dir") + attach.MarkFlagsMutuallyExclusive("readonly", "from-snapshot", "from-dir") + attach.MarkFlagsMutuallyExclusive("directio", "from-snapshot", "from-dir") cliutil.AddOutputFlag(attach) detach := &cobra.Command{ diff --git a/hypervisor/snapshot.go b/hypervisor/snapshot.go index d119cce2..0edac375 100644 --- a/hypervisor/snapshot.go +++ b/hypervisor/snapshot.go @@ -221,11 +221,7 @@ func SnapshotCOWFile(meta *SnapshotMeta) (string, error) { if i < 0 { return "", fmt.Errorf("no cow disk in snapshot metadata") } - base := filepath.Base(meta.StorageConfigs[i].Path) - if base == "." || base == ".." || base == string(filepath.Separator) { - return "", fmt.Errorf("invalid cow disk path in snapshot metadata: %q", meta.StorageConfigs[i].Path) - } - return base, nil + return filepath.Base(meta.StorageConfigs[i].Path), nil } func LoadAndValidateMeta(dir, rootDir, runDir string) (*SnapshotMeta, error) { diff --git a/hypervisor/snapshot_test.go b/hypervisor/snapshot_test.go index adcdfdad..c74b34fb 100644 --- a/hypervisor/snapshot_test.go +++ b/hypervisor/snapshot_test.go @@ -230,8 +230,6 @@ func TestSnapshotCOWFile(t *testing.T) { }, "overlay.qcow2", false}, {"nil entry skipped", []*types.StorageConfig{nil, {Path: "/r/cow.raw", Role: types.StorageRoleCOW}}, "cow.raw", false}, {"no cow", []*types.StorageConfig{{Path: "/r/a.erofs", Role: types.StorageRoleLayer}}, "", true}, - {"empty path", []*types.StorageConfig{{Role: types.StorageRoleCOW}}, "", true}, - {"root path", []*types.StorageConfig{{Path: "/", Role: types.StorageRoleCOW}}, "", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From a4855b83d187f72d8985941f33e0aea066bd8b00 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 11:57:53 +0800 Subject: [PATCH 8/9] fix(cloudhypervisor): converge the snapshot disk copy across lost add and remove responses A lost vm.add-disk response may have attached the copy, so the error path no longer unlinks it; a retry, detach or stop reclaims it. A detach whose vm.remove-device response was lost finds no device, so the copy cleanup now also runs on that path. --- hypervisor/cloudhypervisor/extend.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index 113c2804..a78fbf9c 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/hypervisor/cloudhypervisor/extend.go @@ -90,11 +90,11 @@ func (ch *CloudHypervisor) DiskAttachSnapshot(ctx context.Context, vmRef string, os.Remove(path) //nolint:errcheck,gosec return "", "", err } + // a lost add-disk response may have attached the copy, so only a retry, detach or stop reclaims it deviceID, err := addDevice(ctx, hc, "vm.add-disk", hotDisk(&rec, id, &types.StorageConfig{ Role: types.StorageRoleData, Path: path, Serial: src.Name, }), id) if err != nil { - os.Remove(path) //nolint:errcheck,gosec return "", "", err } return deviceID, format, nil @@ -273,18 +273,21 @@ func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID } defer unlock() deviceID, remove, err := findID(info) - if err != nil || deviceID == "" { + if err != nil { return err } - if remove { - if err := removeDeviceVM(ctx, hc, deviceID); err != nil { - return fmt.Errorf("vm.remove-device %s: %w", deviceID, err) + if deviceID != "" { + 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 { + return fmt.Errorf("device %s removal initiated but the guest has not ejected it: %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 { - return fmt.Errorf("device %s removal initiated but the guest has not ejected it: %w", deviceID, err) - } + // a detach whose remove-device response was lost finds no device, so the copy is reclaimed here too if afterEject == nil { return nil } From 099b4684c3637c10fef5d6b8cc4aedb5bf7270f0 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 18 Oct 2026 12:11:37 +0800 Subject: [PATCH 9/9] fix(cmd): name a snapshot disk copy only after its snapshot With a caller-chosen name two snapshots of one image could share a copy path, and the idempotent pre-check then returned the first snapshot's disk for the second. Deriving the name from the snapshot id makes a repeat of the same snapshot a no-op and gives different snapshots different disks. --- cmd/vm/commands.go | 3 ++- cmd/vm/disk.go | 4 +--- cmd/vm/disk_test.go | 3 ++- docs/devices.md | 2 +- extend/disk/disk.go | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/vm/commands.go b/cmd/vm/commands.go index 15f55d58..0e1d4eb6 100644 --- a/cmd/vm/commands.go +++ b/cmd/vm/commands.go @@ -229,11 +229,12 @@ func buildDiskCommand(h Handler) *cobra.Command { attach.Flags().String("path", "", "absolute path to an existing raw disk file (never deleted by cocoon)") attach.Flags().String("from-snapshot", "", "attach a read-write copy of this local snapshot's COW disk; the VM must run the snapshot's image") attach.Flags().String("from-dir", "", "attach a read-write copy of the COW disk in this export dir (snapshot.json + its disk_attach_files)") - attach.Flags().String("name", "", "disk name: guest /dev/disk/by-id/virtio- and the detach key (required with --path; default snap-)") + attach.Flags().String("name", "", "disk name: guest /dev/disk/by-id/virtio- and the detach key (required with --path; a snapshot copy is named snap-)") attach.Flags().Bool("readonly", false, "attach read-only") attach.Flags().String("directio", "auto", "O_DIRECT for the disk: on|off|auto (off for files on tmpfs)") attach.MarkFlagsOneRequired("path", "from-snapshot", "from-dir") attach.MarkFlagsMutuallyExclusive("path", "from-snapshot", "from-dir") + attach.MarkFlagsMutuallyExclusive("name", "from-snapshot", "from-dir") attach.MarkFlagsMutuallyExclusive("readonly", "from-snapshot", "from-dir") attach.MarkFlagsMutuallyExclusive("directio", "from-snapshot", "from-dir") cliutil.AddOutputFlag(attach) diff --git a/cmd/vm/disk.go b/cmd/vm/disk.go index 4cd04144..be2c5c95 100644 --- a/cmd/vm/disk.go +++ b/cmd/vm/disk.go @@ -1,7 +1,6 @@ package vm import ( - "cmp" "context" "fmt" @@ -88,8 +87,7 @@ func (h Handler) diskAttachSnapshot(cmd *cobra.Command, args []string, fromSnaps if err != nil { return err } - name, _ := cmd.Flags().GetString("name") - name = cmp.Or(name, disk.SnapshotName(cfg.ID)) + name := disk.SnapshotName(cfg.ID) id, format, err := a.DiskAttachSnapshot(ctx, args[0], disk.SnapshotSource{Dir: dir, Name: name, BlobIDs: cfg.ImageBlobIDs, BasePath: basePath}) if err != nil { return classifyAttachErr(err) diff --git a/cmd/vm/disk_test.go b/cmd/vm/disk_test.go index c0646f78..85745efe 100644 --- a/cmd/vm/disk_test.go +++ b/cmd/vm/disk_test.go @@ -15,7 +15,8 @@ func TestDiskAttachFlagGroups(t *testing.T) { }{ {"path", []string{"--path", "/vols/d.raw", "--name", "d", "--readonly", "--directio", "off"}, false}, {"from snapshot", []string{"--from-snapshot", "snap"}, false}, - {"from dir with name", []string{"--from-dir", "/exports/snap", "--name", "reader"}, false}, + {"from dir", []string{"--from-dir", "/exports/snap"}, false}, + {"name with dir", []string{"--from-dir", "/exports/snap", "--name", "reader"}, true}, {"no source", []string{"--name", "d"}, true}, {"path and snapshot", []string{"--path", "/vols/d.raw", "--from-snapshot", "snap"}, true}, {"snapshot and dir", []string{"--from-snapshot", "snap", "--from-dir", "/exports/snap"}, true}, diff --git a/docs/devices.md b/docs/devices.md index 578d32bb..b56fd76d 100644 --- a/docs/devices.md +++ b/docs/devices.md @@ -75,7 +75,7 @@ Flags: | `--path` | empty | Absolute path to an existing raw disk file; exactly one of `--path`, `--from-snapshot`, `--from-dir` is required | | `--from-snapshot` | empty | Cloud Hypervisor only: attach a private read-write copy of this local snapshot's COW disk (`raw` for OCI, `qcow2` for cloudimg, reported as `format`); the VM must run the snapshot's image, layers in the same order; detach, stop and `vm rm` remove the copy | | `--from-dir` | empty | As `--from-snapshot`, from an export dir that holds `snapshot.json` and the files its `disk_attach_files` lists, possibly exported on another node | -| `--name` | required with `--path` | Guest serial and detach key (`^[a-z][a-z0-9_-]{0,19}$`; the `cocoon-` prefix is reserved); defaults to `snap-` plus the first 8 characters of the snapshot id with `--from-snapshot`/`--from-dir` | +| `--name` | required with `--path` | Guest serial and detach key (`^[a-z][a-z0-9_-]{0,19}$`; the `cocoon-` prefix is reserved); not with `--from-snapshot`/`--from-dir`, whose copy is always named `snap-` plus the first 8 characters of the snapshot id, so two snapshots never share a name and a repeat of the same snapshot is a no-op | | `--readonly` | `false` | Attach read-only; not with `--from-snapshot`/`--from-dir` | | `--directio` | `auto` | O_DIRECT for the disk: `on`/`off`/`auto` (use `off` for files on tmpfs); Cloud Hypervisor only, Firecracker ignores it with a warning; not with `--from-snapshot`/`--from-dir` | diff --git a/extend/disk/disk.go b/extend/disk/disk.go index d38eab2d..7451bcba 100644 --- a/extend/disk/disk.go +++ b/extend/disk/disk.go @@ -89,7 +89,7 @@ func ValidateName(name string) error { return nil } -// SnapshotName is the default disk name for a snapshot-sourced attach. +// SnapshotName is the disk name of a snapshot-sourced attach; derived from the snapshot so two snapshots never collide on one name. func SnapshotName(snapID string) string { return snapshotNamePrefix + strings.ToLower(snapID[:min(len(snapID), snapshotNameIDLen)]) }