diff --git a/cmd/cliutil/cliutil.go b/cmd/cliutil/cliutil.go index 73dd90c5..378ac197 100644 --- a/cmd/cliutil/cliutil.go +++ b/cmd/cliutil/cliutil.go @@ -12,6 +12,9 @@ import ( "github.com/spf13/cobra" ) +// TableFunc renders the table body; the writer is flushed by the caller. +type TableFunc func(w *tabwriter.Writer) + func CommandContext(cmd *cobra.Command) context.Context { if cmd != nil && cmd.Context() != nil { return cmd.Context() @@ -52,13 +55,13 @@ func MaybeOutputJSON(cmd *cobra.Command, v any) (bool, error) { return true, OutputJSON(v) } -func OutputFormatted(cmd *cobra.Command, data any, tableFn func(w *tabwriter.Writer)) error { +func OutputFormatted(cmd *cobra.Command, data any, tableFn TableFunc) error { format, _ := cmd.Flags().GetString("format") return OutputFormattedStr(format, data, tableFn) } // OutputFormattedStr renders data as JSON when format=="json", else as a table via tableFn. -func OutputFormattedStr(format string, data any, tableFn func(w *tabwriter.Writer)) error { +func OutputFormattedStr(format string, data any, tableFn TableFunc) error { if format == "json" { return OutputJSON(data) } diff --git a/cmd/core/metastore.go b/cmd/core/metastore.go index 4d0f2839..c5f81fb5 100644 --- a/cmd/core/metastore.go +++ b/cmd/core/metastore.go @@ -3,6 +3,7 @@ package core import ( "context" "path/filepath" + "slices" "sync" "time" @@ -111,12 +112,9 @@ func ResolveMetaBackend(conf *config.Config) string { // LegacyJSONPresent reports whether any json-engine namespace file exists // under the root — data a fresh sqlite store must never shadow. func LegacyJSONPresent(conf *config.Config) bool { - for _, ns := range MetaJSONNamespaces(conf) { - if utils.FileExists(ns.FilePath) { - return true - } - } - return false + return slices.ContainsFunc(MetaJSONNamespaces(conf), func(ns metajson.Namespace) bool { + return utils.FileExists(ns.FilePath) + }) } // MetaStore builds the process-wide meta store once — one store, every diff --git a/cmd/core/utils.go b/cmd/core/utils.go index b994e808..c2301e76 100644 --- a/cmd/core/utils.go +++ b/cmd/core/utils.go @@ -167,6 +167,19 @@ func EnsureSnapshotNameFree(ctx context.Context, snapBackend snapshot.Snapshot, if name == "" { return nil } + // The name index, not Inspect: a save killed mid-flight leaves a pending record + // still holding the name, which Inspect reports as not-found. Passing that + // preflight means the whole capture is written before the insert rejects it. + if nh, ok := snapBackend.(snapshot.NameHolder); ok { + id, held, err := nh.NameOwner(ctx, name) + if err != nil { + return fmt.Errorf("check snapshot name: %w", err) + } + if held { + return fmt.Errorf("snapshot name %q already exists (held by %s)", name, id) + } + return nil + } if _, err := snapBackend.Inspect(ctx, name); err == nil { return fmt.Errorf("snapshot name %q already exists", name) } else if !errors.Is(err, snapshot.ErrNotFound) { diff --git a/cmd/snapshot/handler.go b/cmd/snapshot/handler.go index 54fc6857..0bfac2a8 100644 --- a/cmd/snapshot/handler.go +++ b/cmd/snapshot/handler.go @@ -28,7 +28,7 @@ func (h Handler) Save(cmd *cobra.Command, args []string) error { if err != nil { return err } - logger := log.WithFunc("cmd.snapshot.save") + logger := log.WithFunc("cmd.snapshot.Save") vmRef := args[0] hyper, err := cmdcore.FindHypervisor(ctx, conf, vmRef) @@ -136,7 +136,7 @@ func (h Handler) Export(cmd *cobra.Command, args []string) (err error) { if err != nil { return err } - logger := log.WithFunc("cmd.snapshot.export") + logger := log.WithFunc("cmd.snapshot.Export") snapBackend, err := cmdcore.InitSnapshot(ctx, conf) if err != nil { return err @@ -222,7 +222,7 @@ func (h Handler) Import(cmd *cobra.Command, args []string) error { if err != nil { return err } - logger := log.WithFunc("cmd.snapshot.import") + logger := log.WithFunc("cmd.snapshot.Import") snapBackend, err := cmdcore.InitSnapshot(ctx, conf) if err != nil { return err @@ -263,7 +263,7 @@ func (h Handler) RM(cmd *cobra.Command, args []string) error { if err != nil { return err } - logger := log.WithFunc("cmd.snapshot.rm") + logger := log.WithFunc("cmd.snapshot.RM") snapBackend, err := cmdcore.InitSnapshot(ctx, conf) if err != nil { return err diff --git a/cmd/storebench/main.go b/cmd/storebench/main.go index c397cf02..360dda31 100644 --- a/cmd/storebench/main.go +++ b/cmd/storebench/main.go @@ -1,7 +1,4 @@ -// storebench times meta-engine operations through the hypervisor backend: -// `update`/`get` are single-process loops at resident N (P0 paired gate and -// §9 anchors); `create` fans out worker PROCESSES doing reserve+finalize on -// one shared store — the concurrent VM-creation shape (§9). +// storebench times meta-engine operations through the hypervisor backend: update/get loop in-process at resident N, create fans out worker processes doing reserve+finalize on one shared store (§9's concurrent VM-creation shape). package main import ( diff --git a/cmd/vm/reseed_test.go b/cmd/vm/reseed_test.go index 7bda5a1d..60771d9c 100644 --- a/cmd/vm/reseed_test.go +++ b/cmd/vm/reseed_test.go @@ -13,7 +13,6 @@ import ( "testing" "github.com/cocoonstack/cocoon-agent/agent" - "github.com/cocoonstack/cocoon/types" ) diff --git a/cmd/vm/run.go b/cmd/vm/run.go index 9a57b0f6..6089046b 100644 --- a/cmd/vm/run.go +++ b/cmd/vm/run.go @@ -115,10 +115,12 @@ func (h Handler) Clone(cmd *cobra.Command, args []string) error { defer stream.Close() //nolint:errcheck defer cmdcore.CloseOnCancel(ctx, stream)() - vmCfg, vmID, rollbackReserve, unlock, netProvider, netSetup, err := h.prepareClone(ctx, cmd, conf, hyper, cfg) + cs, err := h.prepareClone(ctx, cmd, conf, hyper, cfg) if err != nil { return err } + vmCfg, vmID, rollbackReserve, unlock := cs.vmCfg, cs.vmID, cs.rollback, cs.unlock + netProvider, netSetup := cs.netProvider, cs.netSetup defer unlock() logger.Infof(ctx, "cloning VM from snapshot %s ...", snapID) @@ -283,10 +285,12 @@ func (h Handler) cloneFromDir(ctx context.Context, cmd *cobra.Command, conf *con } func (h Handler) cloneFromSrcDir(ctx context.Context, cmd *cobra.Command, conf *config.Config, hyper hypervisor.Hypervisor, dcr hypervisor.Direct, cfg types.SnapshotConfig, srcDir, sourceLabel string, logger *log.Fields) error { - vmCfg, vmID, rollbackReserve, unlock, netProvider, netSetup, err := h.prepareClone(ctx, cmd, conf, hyper, cfg) + cs, err := h.prepareClone(ctx, cmd, conf, hyper, cfg) if err != nil { return err } + vmCfg, vmID, rollbackReserve, unlock := cs.vmCfg, cs.vmID, cs.rollback, cs.unlock + netProvider, netSetup := cs.netProvider, cs.netSetup defer unlock() wantJSON := cliutil.WantJSON(cmd) @@ -310,32 +314,42 @@ func (h Handler) cloneFromSrcDir(ctx context.Context, cmd *cobra.Command, conf * return nil } -func (h Handler) prepareClone(ctx context.Context, cmd *cobra.Command, conf *config.Config, hyper hypervisor.Hypervisor, cfg types.SnapshotConfig) (*types.VMConfig, string, func(), func(), network.Network, types.NetSetup, error) { +// cloneSetup is prepareClone's result: the reserved clone's identity and network plus the rollback/unlock pair the caller owes until finalize. +type cloneSetup struct { + vmCfg *types.VMConfig + vmID string + rollback func() + unlock func() + netProvider network.Network + netSetup types.NetSetup +} + +func (h Handler) prepareClone(ctx context.Context, cmd *cobra.Command, conf *config.Config, hyper hypervisor.Hypervisor, cfg types.SnapshotConfig) (cloneSetup, error) { vmCfg, err := cmdcore.CloneVMConfigFromFlags(cmd, cfg) if err != nil { - return nil, "", nil, nil, nil, types.NetSetup{}, err + return cloneSetup{}, err } vmID := utils.GenerateID() if vmCfg.Name == "" { vmCfg.Name = "cocoon-clone-" + network.VMIDPrefix(vmID) } if err = vmCfg.Validate(); err != nil { - return nil, "", nil, nil, nil, types.NetSetup{}, err + return cloneSetup{}, err } // Envelope pins share create's digest-lock window; a record-backed clone's source pin already protects these. releasePins, err := cmdcore.PinEnvelopeBlobs(ctx, conf, cfg.ImageBlobIDs) if err != nil { - return nil, "", nil, nil, nil, types.NetSetup{}, err + return cloneSetup{}, err } rollbackReserve, unlock, err := prereserveVM(ctx, hyper, vmID, vmCfg, cfg.ImageBlobIDs) releasePins() if err != nil { - return nil, "", nil, nil, nil, types.NetSetup{}, err + return cloneSetup{}, err } - fail := func(err error) (*types.VMConfig, string, func(), func(), network.Network, types.NetSetup, error) { + fail := func(err error) (cloneSetup, error) { rollbackReserve() unlock() - return nil, "", nil, nil, nil, types.NetSetup{}, err + return cloneSetup{}, err } if pull, _ := cmd.Flags().GetBool("pull"); pull && vmCfg.Image != "" && vmCfg.ImageType != "" { @@ -363,7 +377,7 @@ func (h Handler) prepareClone(ctx context.Context, cmd *cobra.Command, conf *con return fail(err) } - return vmCfg, vmID, rollbackReserve, unlock, netProvider, netSetup, nil + return cloneSetup{vmCfg: vmCfg, vmID: vmID, rollback: rollbackReserve, unlock: unlock, netProvider: netProvider, netSetup: netSetup}, nil } func (h Handler) restoreDirect(ctx context.Context, cmd *cobra.Command, conf *config.Config, snapRef, vmRef string, vmCfg *types.VMConfig, snapBackend snapshot.Snapshot, hyper hypervisor.Hypervisor, logger *log.Fields) (bool, error) { diff --git a/docs/gc.md b/docs/gc.md index de5fabea..bbc5ae0a 100644 --- a/docs/gc.md +++ b/docs/gc.md @@ -35,8 +35,8 @@ journalctl -u cocoon-gc.service --since today | awk '/gc.Run completed/' ``` Reasons: -- **snapshot**: `orphan` (dataDir without DB record), `stale-pending` (Create crashed >24h ago), `lru-all` / `lru-age` / `lru-keep` / `lru-size` (multi-criterion uses `+` joiner) -- **cloud-hypervisor / firecracker**: `orphan-runDir`, `orphan-logDir`, `stale-creating` +- **snapshot**: `orphan` (dataDir without DB record), `stale-pending` (a dead save's pending record — its build lease is free), `lru-all` / `lru-age` / `lru-keep` / `lru-size` (multi-criterion uses `+` joiner) +- **cloud-hypervisor / firecracker**: `orphan-runDir`, `orphan-logDir`, `stale-creating` (a dead create/clone's placeholder — its ops lock is free, no age wait) - **images (oci, cloudimg)**: `unreferenced` - **cni**: `orphan` (netns without active VM) - **bridge**: `orphan-tap` @@ -44,7 +44,7 @@ Reasons: ### Snapshot LRU Eviction -Bare `cocoon gc` only reclaims **orphans** (on-disk data with no DB record) and **stale pending** records (crashed mid-Create, older than 24h). To also evict healthy snapshots by access recency, pass `--snapshot`: +Bare `cocoon gc` only reclaims **orphans** (on-disk data with no DB record) and **stale pending** records (a save died mid-flight; every save holds its snapshot's build lease from placeholder to finalize, so a pending record whose lease GC can acquire is provably ownerless — no age wait). To also evict healthy snapshots by access recency, pass `--snapshot`: | Flag | Effect | | -------------------- | ----------------------------------------------------------------------------------------------- | diff --git a/hypervisor/backend.go b/hypervisor/backend.go index fc600da9..25b7f411 100644 --- a/hypervisor/backend.go +++ b/hypervisor/backend.go @@ -28,7 +28,7 @@ const ( // COWRawFileName is the raw COW disk's file name in the run dir (single owner so snapshot matchers and clone path rewrites can't drift). COWRawFileName = "cow.raw" - // CreatingStateGCGrace bounds how long GC tolerates a "creating" VM. + // CreatingStateGCGrace ages leftover capture/staging dirs and clone locks, where no held lock proves the owner died; creating records need no age — their ops lock is the proof. CreatingStateGCGrace = 24 * time.Hour // VMMemTransferTimeout is the single-shot timeout for snapshot/restore API calls. diff --git a/hypervisor/choreo_trace_test.go b/hypervisor/choreo_trace_test.go index 525c9526..7fa6a755 100644 --- a/hypervisor/choreo_trace_test.go +++ b/hypervisor/choreo_trace_test.go @@ -13,10 +13,7 @@ import ( "github.com/cocoonstack/cocoon/types" ) -// TestLegacyChoreographyTrace is the P0 differential gate (design §10): the -// legacy implementation ran this exact op sequence and recorded returned -// values, error identities and final bytes; the migrated boundary must -// reproduce every step. +// TestLegacyChoreographyTrace is the P0 differential gate (design §10): the migrated boundary must reproduce the legacy run's returned values, error identities and final bytes step for step. func TestLegacyChoreographyTrace(t *testing.T) { raw, err := os.ReadFile(filepath.Join("testdata", "legacy-choreo-trace.json")) if err != nil { diff --git a/hypervisor/clone.go b/hypervisor/clone.go index 4b3300fb..64cd8471 100644 --- a/hypervisor/clone.go +++ b/hypervisor/clone.go @@ -14,14 +14,17 @@ import ( // AfterExtractFn finalizes a cloned VM after snapshot files are in place; sourceSnapshotID flows through for metering lineage. type AfterExtractFn func(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, runDir, logDir string, now time.Time, sourceSnapshotID string) (*types.VM, error) +// CloneSpec carries one clone's inputs through the shared placeholder→populate→finalize skeleton. +type CloneSpec struct { + VMCfg *types.VMConfig + Net types.NetSetup + SnapshotConfig *types.SnapshotConfig + AfterExtract AfterExtractFn +} + // DirectCloneBase clones from a local snapshot directory. Used when the snapshot lives on the same host (no tar streaming needed). -func (b *Backend) DirectCloneBase( - ctx context.Context, vmID string, vmCfg *types.VMConfig, - net types.NetSetup, snapshotConfig *types.SnapshotConfig, srcDir string, - cloneFiles func(dstDir, srcDir string) error, - afterExtract AfterExtractFn, -) (*types.VM, error) { - return b.cloneBase(ctx, vmID, vmCfg, net, snapshotConfig, afterExtract, func(runDir string) error { +func (b *Backend) DirectCloneBase(ctx context.Context, vmID string, spec CloneSpec, srcDir string, cloneFiles func(dstDir, srcDir string) error) (*types.VM, error) { + return b.cloneBase(ctx, vmID, spec, func(runDir string) error { if err := cloneFiles(runDir, srcDir); err != nil { return fmt.Errorf("clone snapshot files: %w", err) } @@ -30,12 +33,8 @@ func (b *Backend) DirectCloneBase( } // CloneFromStream clones from a tar stream into a fresh runDir. Used when the snapshot arrives over the network (cross-node clone). -func (b *Backend) CloneFromStream( - ctx context.Context, vmID string, vmCfg *types.VMConfig, - net types.NetSetup, snapshotConfig *types.SnapshotConfig, snapshot io.Reader, - afterExtract AfterExtractFn, -) (*types.VM, error) { - return b.cloneBase(ctx, vmID, vmCfg, net, snapshotConfig, afterExtract, func(runDir string) error { +func (b *Backend) CloneFromStream(ctx context.Context, vmID string, spec CloneSpec, snapshot io.Reader) (*types.VM, error) { + return b.cloneBase(ctx, vmID, spec, func(runDir string) error { if err := utils.ExtractTar(runDir, snapshot, isLockFile); err != nil { return fmt.Errorf("extract snapshot: %w", err) } @@ -62,12 +61,8 @@ func (b *Backend) FinalizeClone(ctx context.Context, vmID string, info *types.VM return nil } -func (b *Backend) cloneBase( - ctx context.Context, vmID string, vmCfg *types.VMConfig, - net types.NetSetup, snapshotConfig *types.SnapshotConfig, - afterExtract AfterExtractFn, populate func(runDir string) error, -) (_ *types.VM, err error) { - runDir, logDir, now, cleanup, err := b.reservePlaceholder(ctx, vmID, vmCfg, snapshotConfig.ImageBlobIDs) +func (b *Backend) cloneBase(ctx context.Context, vmID string, spec CloneSpec, populate func(runDir string) error) (_ *types.VM, err error) { + runDir, logDir, now, cleanup, err := b.reservePlaceholder(ctx, vmID, spec.VMCfg, spec.SnapshotConfig.ImageBlobIDs) if err != nil { return nil, err } @@ -79,5 +74,5 @@ func (b *Backend) cloneBase( if err = populate(runDir); err != nil { return nil, err } - return afterExtract(ctx, vmID, vmCfg, net, runDir, logDir, now, snapshotConfig.ID) + return spec.AfterExtract(ctx, vmID, spec.VMCfg, spec.Net, runDir, logDir, now, spec.SnapshotConfig.ID) } diff --git a/hypervisor/cloudhypervisor/clone.go b/hypervisor/cloudhypervisor/clone.go index c394fa6e..66df5884 100644 --- a/hypervisor/cloudhypervisor/clone.go +++ b/hypervisor/cloudhypervisor/clone.go @@ -31,7 +31,7 @@ type cloneResumeOpts struct { } func (ch *CloudHypervisor) Clone(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, snapshotConfig *types.SnapshotConfig, snapshot io.Reader) (*types.VM, error) { - return ch.CloneFromStream(ctx, vmID, vmCfg, net, snapshotConfig, snapshot, ch.cloneAfterExtract) + return ch.CloneFromStream(ctx, vmID, hypervisor.CloneSpec{VMCfg: vmCfg, Net: net, SnapshotConfig: snapshotConfig, AfterExtract: ch.cloneAfterExtract}, snapshot) } func (ch *CloudHypervisor) cloneAfterExtract(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, runDir, logDir string, now time.Time, sourceSnapshotID string) (*types.VM, error) { @@ -45,7 +45,7 @@ func (ch *CloudHypervisor) cloneAfterExtract(ctx context.Context, vmID string, v // cloneAfterExtractParsed is cloneAfterExtract minus the config.json parse; DirectClone passes the copy step's parse of the verbatim-copied source. func (ch *CloudHypervisor) cloneAfterExtractParsed(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, runDir, logDir string, now time.Time, sourceSnapshotID string, chCfg *chVMConfig) (*types.VM, error) { networkConfigs := net.NetworkConfigs - logger := log.WithFunc("cloudhypervisor.cloneAfterExtract") + logger := log.WithFunc("cloudhypervisor.cloneAfterExtractParsed") chConfigPath := filepath.Join(runDir, configJSONName) vmCfg.RestoreMode = resolveRestoreMode(ctx, vmCfg.RestoreMode, chCfg.Memory) @@ -158,12 +158,7 @@ func (ch *CloudHypervisor) cloneAfterExtractParsed(ctx context.Context, vmID str return info, nil } -func (ch *CloudHypervisor) restoreAndResumeClone( - ctx context.Context, - pid int, - sockPath, runDir string, - opts *cloneResumeOpts, -) (err error) { +func (ch *CloudHypervisor) restoreAndResumeClone(ctx context.Context, pid int, sockPath, runDir string, opts *cloneResumeOpts) (err error) { defer func() { if err != nil { ch.AbortLaunch(ctx, pid, sockPath, runDir, runtimeFiles) diff --git a/hypervisor/cloudhypervisor/direct.go b/hypervisor/cloudhypervisor/direct.go index 2d739b1c..d06594a2 100644 --- a/hypervisor/cloudhypervisor/direct.go +++ b/hypervisor/cloudhypervisor/direct.go @@ -15,12 +15,16 @@ import ( func (ch *CloudHypervisor) DirectClone(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, snapshotConfig *types.SnapshotConfig, srcDir string) (*types.VM, error) { // The copy step's parse feeds cloneAfterExtractParsed: config.json is copied verbatim, so re-parsing the runDir copy would decode identical bytes. var srcCfg *chVMConfig - return ch.DirectCloneBase(ctx, vmID, vmCfg, net, snapshotConfig, srcDir, func(dstDir, srcDir string) error { + spec := hypervisor.CloneSpec{ + VMCfg: vmCfg, Net: net, SnapshotConfig: snapshotConfig, + AfterExtract: func(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, runDir, logDir string, now time.Time, sourceSnapshotID string) (*types.VM, error) { + return ch.cloneAfterExtractParsed(ctx, vmID, vmCfg, net, runDir, logDir, now, sourceSnapshotID, srcCfg) + }, + } + return ch.DirectCloneBase(ctx, vmID, spec, srcDir, func(dstDir, srcDir string) error { var err error srcCfg, err = cloneSnapshotFiles(ctx, dstDir, srcDir) return err - }, func(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, runDir, logDir string, now time.Time, sourceSnapshotID string) (*types.VM, error) { - return ch.cloneAfterExtractParsed(ctx, vmID, vmCfg, net, runDir, logDir, now, sourceSnapshotID, srcCfg) }) } diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index baada1f0..0e95473f 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/hypervisor/cloudhypervisor/extend.go @@ -23,6 +23,15 @@ import ( const chStatePaused = "Paused" +// makeBodyFn builds a device endpoint's request body from the record reloaded under the ops lock. +type makeBodyFn func(rec *hypervisor.VMRecord) any + +// preCheckFn vets the under-lock vm.info snapshot before a device mutation. +type preCheckFn func(*chVMInfoResponse) error + +// findIDFn resolves the device id to detach from the under-lock vm.info snapshot. +type findIDFn func(*chVMInfoResponse) (string, error) + func (ch *CloudHypervisor) DiskAttach(ctx context.Context, vmRef string, spec disk.Spec) (string, error) { if err := spec.Normalize(); err != nil { return "", err @@ -230,11 +239,7 @@ func (ch *CloudHypervisor) lockedDeviceOp(ctx context.Context, vmRef string) (*h return hc, rec, info, unlock, nil } -func (ch *CloudHypervisor) attachWith( - ctx context.Context, vmRef, endpoint string, - makeBody func(rec *hypervisor.VMRecord) any, fallbackID string, - preCheck func(*chVMInfoResponse) error, -) (string, error) { +func (ch *CloudHypervisor) attachWith(ctx context.Context, vmRef, endpoint string, makeBody makeBodyFn, fallbackID string, preCheck preCheckFn) (string, error) { hc, rec, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) if err != nil { return "", err @@ -269,10 +274,7 @@ func (ch *CloudHypervisor) attachWith( return fallbackID, nil } -func (ch *CloudHypervisor) detachWith( - ctx context.Context, vmRef string, - findID func(*chVMInfoResponse) (string, error), -) error { +func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID findIDFn) error { hc, rec, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) if err != nil { return err diff --git a/hypervisor/cloudhypervisor/netresize.go b/hypervisor/cloudhypervisor/netresize.go index f062a272..9864dcd2 100644 --- a/hypervisor/cloudhypervisor/netresize.go +++ b/hypervisor/cloudhypervisor/netresize.go @@ -100,10 +100,7 @@ func (ch *CloudHypervisor) netResizeAdd(ctx context.Context, hc *http.Client, vm return res, nil } -// resolveFailedPersist re-reads the record after a failed NIC persist (fsync -// can fail after the rename landed) and tears down only on a conclusive miss: -// removing a committed NIC would strand record-without-device, unhealable by a -// same-target retry. Lockless read so a GC index lock can't fake a miss. +// resolveFailedPersist tears down a failed NIC persist only on a conclusive lockless re-read miss: fsync can fail after the rename landed, and removing a committed NIC strands record-without-device beyond any retry. func (ch *CloudHypervisor) resolveFailedPersist(ctx context.Context, hc *http.Client, plumbing netresize.Plumbing, vmID string, nc *types.NetworkConfig, chID string, i int) (bool, error) { rec, err := ch.PeekRecord(ctx, vmID) if err != nil { @@ -211,10 +208,7 @@ func reconcileOrphanNICs(ctx context.Context, hc *http.Client, info *chVMInfoRes if err := removeDeviceVM(ctx, hc, n.ID); err != nil { return fmt.Errorf("eject orphan NIC %s: %w", n.ID, err) } - // Reclaim the host slot even when the eject wait times out (same - // pattern as resolveFailedPersist's rollback): once the guest finishes - // a late eject the device vanishes from vm.info, so no later reconcile - // would ever see this TAP again and it would wedge retries at CreateTAP. + // Reclaim the host slot even on eject-wait timeout: a late guest eject removes the device from vm.info, so no later reconcile would see this TAP and retries wedge at CreateTAP. ejectErr := waitDeviceEjected(ctx, hc, n.ID) if idx, ok := network.TAPIndex(n.TAP); ok { if rmErr := plumbing.Remove(ctx, vmID, idx); rmErr != nil { diff --git a/hypervisor/cloudhypervisor/netresize_test.go b/hypervisor/cloudhypervisor/netresize_test.go index 59536344..0e97f27a 100644 --- a/hypervisor/cloudhypervisor/netresize_test.go +++ b/hypervisor/cloudhypervisor/netresize_test.go @@ -21,10 +21,7 @@ import ( "github.com/cocoonstack/cocoon/types" ) -// TestReconcileOrphanNICs covers the interrupted-resize fault window: a CH -// device whose MAC the VM record does not know must be ejected AND its host -// TAP slot reclaimed before a retry adds alongside it (a leftover bridge TAP -// wedges every retry); recorded and boot-time (_net*) devices must survive. +// TestReconcileOrphanNICs covers the interrupted-resize window: an unrecorded-MAC device must be ejected and its host TAP slot reclaimed (a leftover bridge TAP wedges every retry); recorded and boot-time (_net*) devices survive. func TestReconcileOrphanNICs(t *testing.T) { hc, removed := newCHStubClient(t, []chNet{ {ID: "cocoon-net-aabbccddee01", MAC: "aa:bb:cc:dd:ee:01", TAP: "tapvm1beef-0"}, @@ -49,10 +46,7 @@ func TestReconcileOrphanNICs(t *testing.T) { } } -// TestReconcileOrphanNICsReclaimsSlotOnEjectTimeout pins the slow-guest path: -// a timed-out B0EJ wait must still reclaim the orphan's host TAP slot — after -// a late eject the device vanishes from vm.info and no later reconcile could -// ever see this TAP again. +// TestReconcileOrphanNICsReclaimsSlotOnEjectTimeout pins the slow-guest path: a timed-out B0EJ wait still reclaims the orphan's TAP slot, since after a late eject no later reconcile could ever see it. func TestReconcileOrphanNICsReclaimsSlotOnEjectTimeout(t *testing.T) { hc, removed := newCHStubClient(t, []chNet{ {ID: "cocoon-net-aabbccddee02", MAC: "aa:bb:cc:dd:ee:02", TAP: "tapvm1beef-1"}, diff --git a/hypervisor/firecracker/clone.go b/hypervisor/firecracker/clone.go index 141276b4..516c6fdc 100644 --- a/hypervisor/firecracker/clone.go +++ b/hypervisor/firecracker/clone.go @@ -42,8 +42,19 @@ func (p *bindRedirectPlan) files() []*os.File { func (p *bindRedirectPlan) close() { closeLeases(p.leases) } +// launchCloneFn starts the FC process over the plan's redirected drive FDs. +type launchCloneFn func([]*os.File) (int, *cloneLeaseControl, error) + +// cloneLaunch carries one clone launch's anchoring inputs down the startCloneVM chain; src/dst are the snapshot's recorded drives and the clone's rebuilt ones, index-aligned. +type cloneLaunch struct { + launch launchCloneFn + sockPath, runDir string + networkConfigs []*types.NetworkConfig + src, dst []*types.StorageConfig +} + func (fc *Firecracker) Clone(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, snapshotConfig *types.SnapshotConfig, snapshot io.Reader) (*types.VM, error) { - return fc.CloneFromStream(ctx, vmID, vmCfg, net, snapshotConfig, snapshot, fc.cloneAfterExtract) + return fc.CloneFromStream(ctx, vmID, hypervisor.CloneSpec{VMCfg: vmCfg, Net: net, SnapshotConfig: snapshotConfig, AfterExtract: fc.cloneAfterExtract}, snapshot) } func (fc *Firecracker) cloneAfterExtract(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, runDir, logDir string, now time.Time, sourceSnapshotID string) (*types.VM, error) { @@ -92,7 +103,10 @@ func (fc *Firecracker) cloneAfterExtract(ctx context.Context, vmID string, vmCfg LogDir: logDir, }, sockPath, net.NetnsPath, leaseFiles) } - pid, leaseControl, plan, cloneErr := fc.startCloneVM(ctx, launch, sockPath, runDir, networkConfigs, meta.StorageConfigs, storageConfigs) + pid, leaseControl, plan, cloneErr := fc.startCloneVM(ctx, cloneLaunch{ + launch: launch, sockPath: sockPath, runDir: runDir, + networkConfigs: networkConfigs, src: meta.StorageConfigs, dst: storageConfigs, + }) if cloneErr != nil { fc.MarkError(ctx, vmID) return nil, cloneErr @@ -121,18 +135,12 @@ func (fc *Firecracker) cloneAfterExtract(ctx context.Context, vmID string, vmCfg } // startCloneVM launches FC, drives snapshot/load, then resumes and re-anchors. Clone disks are bind-mounted over source-absolute paths in a private mount namespace, so siblings load in parallel without replacing host paths with symlinks. -func (fc *Firecracker) startCloneVM( - ctx context.Context, - launch func([]*os.File) (int, *cloneLeaseControl, error), - sockPath, runDir string, - networkConfigs []*types.NetworkConfig, - srcConfigs, dstConfigs []*types.StorageConfig, -) (int, *cloneLeaseControl, *bindRedirectPlan, error) { - pid, leaseControl, plan, err := fc.loadCloneSnapshot(ctx, launch, sockPath, runDir, networkConfigs, srcConfigs, dstConfigs) +func (fc *Firecracker) startCloneVM(ctx context.Context, cl cloneLaunch) (int, *cloneLeaseControl, *bindRedirectPlan, error) { + pid, leaseControl, plan, err := fc.loadCloneSnapshot(ctx, cl) if err != nil { return 0, nil, nil, err } - if err := fc.resumeAndReanchorClone(ctx, pid, sockPath, runDir, srcConfigs, dstConfigs); err != nil { + if err := fc.resumeAndReanchorClone(ctx, pid, cl); err != nil { leaseControl.close() plan.close() return 0, nil, nil, err @@ -140,17 +148,11 @@ func (fc *Firecracker) startCloneVM( return pid, leaseControl, plan, nil } -func (fc *Firecracker) loadCloneSnapshot( - ctx context.Context, - launch func([]*os.File) (int, *cloneLeaseControl, error), - sockPath, runDir string, - networkConfigs []*types.NetworkConfig, - srcConfigs, dstConfigs []*types.StorageConfig, -) (int, *cloneLeaseControl, *bindRedirectPlan, error) { - netOverrides := buildNetworkOverrides(networkConfigs) - vsockPath := hypervisor.VsockSockPath(runDir) - - plan, err := holdBindableRedirects(ctx, fc.Conf.RootDirPath(), fc.Conf.RunDir(), srcConfigs, dstConfigs, func(id string) (bool, error) { +func (fc *Firecracker) loadCloneSnapshot(ctx context.Context, cl cloneLaunch) (int, *cloneLeaseControl, *bindRedirectPlan, error) { + netOverrides := buildNetworkOverrides(cl.networkConfigs) + vsockPath := hypervisor.VsockSockPath(cl.runDir) + + plan, err := holdBindableRedirects(ctx, fc.Conf.RootDirPath(), fc.Conf.RunDir(), cl.src, cl.dst, func(id string) (bool, error) { rec, peekErr := fc.PeekRecord(ctx, id) return rec != nil, peekErr }) @@ -163,11 +165,11 @@ func (fc *Firecracker) loadCloneSnapshot( leaseControl *cloneLeaseControl ) if len(plan.binds) == 0 { - pid, leaseControl, err = launch(nil) + pid, leaseControl, err = cl.launch(nil) } else { pid, err = launchWithBinds(plan.binds, func() (int, error) { var launchErr error - pid, leaseControl, launchErr = launch(plan.files()) + pid, leaseControl, launchErr = cl.launch(plan.files()) return pid, launchErr }) } @@ -176,40 +178,35 @@ func (fc *Firecracker) loadCloneSnapshot( plan.close() return 0, nil, nil, fmt.Errorf("launch FC: %w", err) } - if err := loadSnapshotFC(ctx, sockPath, runDir, netOverrides, vsockPath); err != nil { + if err := loadSnapshotFC(ctx, cl.sockPath, cl.runDir, netOverrides, vsockPath); err != nil { leaseControl.close() - fc.AbortLaunch(ctx, pid, sockPath, runDir, runtimeFiles) + fc.AbortLaunch(ctx, pid, cl.sockPath, cl.runDir, runtimeFiles) plan.close() return 0, nil, nil, fmt.Errorf("snapshot/load: %w", err) } if err := verifyDriveFDs(pid, plan.binds); err != nil { leaseControl.close() - fc.AbortLaunch(ctx, pid, sockPath, runDir, runtimeFiles) + fc.AbortLaunch(ctx, pid, cl.sockPath, cl.runDir, runtimeFiles) plan.close() return 0, nil, nil, fmt.Errorf("bind redirect lost during load (source mutated concurrently): %w", err) } return pid, leaseControl, plan, nil } -func (fc *Firecracker) resumeAndReanchorClone( - ctx context.Context, - pid int, - sockPath, runDir string, - srcConfigs, dstConfigs []*types.StorageConfig, -) (err error) { +func (fc *Firecracker) resumeAndReanchorClone(ctx context.Context, pid int, cl cloneLaunch) (err error) { defer func() { if err != nil { - fc.AbortLaunch(ctx, pid, sockPath, runDir, runtimeFiles) + fc.AbortLaunch(ctx, pid, cl.sockPath, cl.runDir, runtimeFiles) } }() - hc := utils.NewSocketHTTPClient(sockPath) + hc := utils.NewSocketHTTPClient(cl.sockPath) if err = resumeVM(ctx, hc); err != nil { return fmt.Errorf("resume: %w", err) } // Re-anchor redirected drives at the clone's own paths: the loaded vmstate still names the source's, and a future snapshot would embed those dangling paths, breaking its restore. - for _, i := range redirectedDriveIndices(srcConfigs, dstConfigs) { - if err = patchDrivePath(ctx, hc, fmt.Sprintf(driveIDFmt, i), dstConfigs[i].Path); err != nil { + for _, i := range redirectedDriveIndices(cl.src, cl.dst) { + if err = patchDrivePath(ctx, hc, fmt.Sprintf(driveIDFmt, i), cl.dst[i].Path); err != nil { return fmt.Errorf("re-anchor drive %d: %w", i, err) } } diff --git a/hypervisor/firecracker/direct.go b/hypervisor/firecracker/direct.go index 4ca68237..e9e21c8c 100644 --- a/hypervisor/firecracker/direct.go +++ b/hypervisor/firecracker/direct.go @@ -10,9 +10,10 @@ import ( // DirectClone clones from a local snapshot dir. Per-type: hardlink mem, reflink/copy COW, plain copy metadata. func (fc *Firecracker) DirectClone(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, snapshotConfig *types.SnapshotConfig, srcDir string) (*types.VM, error) { - return fc.DirectCloneBase(ctx, vmID, vmCfg, net, snapshotConfig, srcDir, func(dstDir, srcDir string) error { + spec := hypervisor.CloneSpec{VMCfg: vmCfg, Net: net, SnapshotConfig: snapshotConfig, AfterExtract: fc.cloneAfterExtract} + return fc.DirectCloneBase(ctx, vmID, spec, srcDir, func(dstDir, srcDir string) error { return cloneSnapshotFiles(ctx, dstDir, srcDir) - }, fc.cloneAfterExtract) + }) } // DirectRestore restores a VM in place from a local snapshot dir. diff --git a/hypervisor/gc.go b/hypervisor/gc.go index f4d9ccfb..c9424e09 100644 --- a/hypervisor/gc.go +++ b/hypervisor/gc.go @@ -68,7 +68,6 @@ func (b *Backend) BuildGCModule() gc.Module[VMGCSnapshot] { Recover: b.gcRecover, ReadDB: func(ctx context.Context) (VMGCSnapshot, error) { snap := VMGCSnapshot{reasons: make(map[string]string)} - cutoff := timeNow().Add(-CreatingStateGCGrace) if err := b.view(ctx, func(t *vmTx) error { snap.blobIDs = make(map[string]struct{}) snap.vmIDs = make(map[string]struct{}) @@ -82,7 +81,7 @@ func (b *Backend) BuildGCModule() gc.Module[VMGCSnapshot] { snap.recRunDirs = append(snap.recRunDirs, rec.RunDir) } maps.Copy(snap.blobIDs, rec.ImageBlobIDs) - if rec.State == types.VMStateCreating && rec.UpdatedAt.Before(cutoff) { + if rec.State == types.VMStateCreating { snap.staleCreate = append(snap.staleCreate, id) } return nil @@ -156,7 +155,6 @@ func (b *Backend) gcCollect(ctx context.Context, ids []string, snap VMGCSnapshot errs := b.sweepStaleCaptureDirs(ctx, snap.sweepDirs(b.Conf.RunDir())) errs = append(errs, b.sweepOrphanDirs(ctx, snap.orphanDirs)...) errs = append(errs, b.sweepStaleCloneLocks(ctx)...) - cutoff := timeNow().Add(-CreatingStateGCGrace) for _, id := range ids { // Ops lock excludes in-flight owners: a create pre-locks and mkdirs before its DB record lands, so an unlocked "orphan" may be seconds old. ok := b.withOpsTryLock(ctx, id, func() { @@ -184,8 +182,8 @@ func (b *Backend) gcCollect(ctx context.Context, ids []string, snap VMGCSnapshot logger.Infof(ctx, "collected id=%s reason=%s", id, snap.reasons[id]) return } - // Revalidate under the lock: only a still-stale creating record qualifies. - if rec.State != types.VMStateCreating || !rec.UpdatedAt.Before(cutoff) { + // Revalidate under the lock: the held ops lock is the ownerless proof — create and clone hold it from prereserve through the final record commit. + if rec.State != types.VMStateCreating { return } if err := b.collectStaleCreate(ctx, id, rec); err != nil { diff --git a/hypervisor/gc_test.go b/hypervisor/gc_test.go index 29e7b34d..d70cbe97 100644 --- a/hypervisor/gc_test.go +++ b/hypervisor/gc_test.go @@ -18,9 +18,9 @@ func TestGCCollectKeepsLockedVM(t *testing.T) { const id = "vm-gc-lock" runDir, logDir := t.TempDir(), t.TempDir() seedVMRecord(t, b, id, 1, 512, 1024, false) + // A fresh creating record: the free ops lock alone is the reclaim proof, no age gate. if err := b.dbUpdate(ctx, func(idx *VMIndex) error { idx.VMs[id].State = types.VMStateCreating - idx.VMs[id].UpdatedAt = time.Now().Add(-25 * time.Hour) idx.VMs[id].RunDir = runDir idx.VMs[id].LogDir = logDir return nil diff --git a/hypervisor/restore.go b/hypervisor/restore.go index c28bfd2a..c81daedf 100644 --- a/hypervisor/restore.go +++ b/hypervisor/restore.go @@ -97,7 +97,7 @@ func (b *Backend) RestoreSequence(ctx context.Context, vmRef string, spec Restor } return nil } - return b.restoreCore(ctx, vmID, rec, spec.VMCfg, spec.SourceSnapshotID, spec.Kill, apply, spec.AfterExtract) + return b.restoreCore(ctx, restoreRun{vmID: vmID, rec: rec, vmCfg: spec.VMCfg, sourceSnapshotID: spec.SourceSnapshotID, kill: spec.Kill, apply: apply, afterExtract: spec.AfterExtract}) } // DirectRestoreSequence restores from a local snapshot directory. @@ -122,7 +122,7 @@ func (b *Backend) DirectRestoreSequence(ctx context.Context, vmRef string, spec } return nil } - return b.restoreCore(ctx, vmID, rec, spec.VMCfg, spec.SourceSnapshotID, spec.Kill, apply, spec.AfterExtract) + return b.restoreCore(ctx, restoreRun{vmID: vmID, rec: rec, vmCfg: spec.VMCfg, sourceSnapshotID: spec.SourceSnapshotID, kill: spec.Kill, apply: apply, afterExtract: spec.AfterExtract}) } // failRestore marks the VM error after a restore failure; a stopped origin (the pre-kill state) is spared so hibernate wake stays retryable — run-dir-mutating steps quarantine at their own site. @@ -133,40 +133,48 @@ func (b *Backend) failRestore(ctx context.Context, vmID string, origin types.VMS b.MarkError(ctx, vmID) } +// restoreRun is restoreCore's locked input set, common to both restore sequences. +type restoreRun struct { + vmID string + rec *VMRecord + vmCfg *types.VMConfig + sourceSnapshotID string + kill KillHook + apply func(*VMRecord) error + afterExtract AfterExtractHook +} + // restoreCore is the shared kill→emit→apply→finalize tail of both restore sequences. -func (b *Backend) restoreCore( - ctx context.Context, vmID string, rec *VMRecord, vmCfg *types.VMConfig, sourceSnapshotID string, - kill KillHook, apply func(*VMRecord) error, afterExtract AfterExtractHook, -) (*types.VM, error) { - oldShape := shapeFromConfig(rec.Config) - if killErr := kill(ctx, vmID, rec); killErr != nil { +func (b *Backend) restoreCore(ctx context.Context, run restoreRun) (*types.VM, error) { + oldShape := shapeFromConfig(run.rec.Config) + if killErr := run.kill(ctx, run.vmID, run.rec); killErr != nil { return nil, killErr } - b.emitRestoreComputeStop(ctx, vmID, oldShape, sourceSnapshotID) + b.emitRestoreComputeStop(ctx, run.vmID, oldShape, run.sourceSnapshotID) var result *types.VM inner := func() error { // Tombstone before the destructive phase: it survives lost quarantine writes and process death; only FinalizeRestore clears it. - if err := markRestoreDirty(rec.RunDir); err != nil { + if err := markRestoreDirty(run.rec.RunDir); err != nil { return err } - if err := apply(rec); err != nil { + if err := run.apply(run.rec); err != nil { return err } // Inside the ops lock, like StartSequence: a hibernate resume after a host reboot finds no plumbing left to un-quiesce. - if err := b.RecoverNetwork(ctx, rec); err != nil { + if err := b.RecoverNetwork(ctx, run.rec); err != nil { return fmt.Errorf("recover network: %w", err) } var afterErr error - result, afterErr = afterExtract(ctx, vmID, vmCfg, rec) + result, afterErr = run.afterExtract(ctx, run.vmID, run.vmCfg, run.rec) return afterErr } if err := inner(); err != nil { // The kill already ran, so networking left up stays durable retry work even for a retryable stopped origin. - b.markFailedOperation(ctx, vmID, rec.State != types.VMStateStopped) + b.markFailedOperation(ctx, run.vmID, run.rec.State != types.VMStateStopped) return nil, err } - b.emitRestoreSuccess(ctx, result, oldShape, sourceSnapshotID) + b.emitRestoreSuccess(ctx, result, oldShape, run.sourceSnapshotID) return result, nil } diff --git a/hypervisor/teardown.go b/hypervisor/teardown.go index 442cd75a..822c8962 100644 --- a/hypervisor/teardown.go +++ b/hypervisor/teardown.go @@ -11,10 +11,6 @@ import ( "github.com/cocoonstack/cocoon/meta/tombstone" ) -// ErrTombstoned reports an entrypoint that met a deleting-phase entity: -// resources are partially gone and the operation must not touch the record. -var ErrTombstoned = errors.New("vm is being deleted") - // vmCleanup is the vms-namespace tombstone payload: everything teardown // needs once the record is gone. type vmCleanup struct { @@ -23,10 +19,7 @@ type vmCleanup struct { LogDir string `json:"log_dir,omitempty"` } -// EntryGuardLoad runs the tombstone entry guard under the caller's ops lock — -// roll a leased tombstone back, drive a deleting one to completion and refuse — -// returning the record from the guard's own transaction, sparing lock-held -// entry paths a second whole-namespace read. +// EntryGuardLoad runs the tombstone entry guard under the caller's ops lock: a leased tombstone rolls back, a deleting one is driven to completion and refused; the record returns from the guard's own transaction so entry paths skip a second namespace read. func (b *Backend) EntryGuardLoad(ctx context.Context, id string) (VMRecord, error) { rec, err := b.entryGuard(ctx, id) if err != nil { @@ -42,10 +35,7 @@ func (b *Backend) tombstones() *tombstone.Table { return tombstone.NewTable(b.NS) } -// deleteVMProtocol runs the §5 phase protocol for one VM under its held ops -// lock: lease with full payload → deleting → slow teardown → fenced finalize. -// A crash at any point leaves a phase-directed tombstone a later worker -// resumes. +// deleteVMProtocol runs the §5 phase protocol (lease → deleting → teardown → fenced finalize) under the held ops lock; a crash at any point leaves a phase-directed tombstone a later worker resumes. func (b *Backend) deleteVMProtocol(ctx context.Context, id string, rec *VMRecord) error { ts := b.tombstones() cl := vmCleanup{Name: rec.Config.Name, RunDir: rec.RunDir, LogDir: rec.LogDir} @@ -137,39 +127,44 @@ func (b *Backend) recoverVMTombstone(ctx context.Context, id string) (done bool, return true, nil } +// entryGuard peeks tombstone and record in a read transaction — the caller's ops lock freezes both — escalating to a write only to roll a leased tombstone back, keeping the common no-tombstone path off the single sqlite writer connection. func (b *Backend) entryGuard(ctx context.Context, id string) (*VMRecord, error) { - var rec *VMRecord - err := b.update(ctx, func(t *vmTx) error { - if err := b.guardVMTombstone(ctx, t, id); err != nil { + ts := b.tombstones() + var ( + rec *VMRecord + tsRec *tombstone.Record + ) + if err := b.view(ctx, func(t *vmTx) error { + var err error + if tsRec, err = ts.Get(ctx, t.r, id); err != nil || tsRec != nil { return err } - var getErr error - rec, getErr = t.Get(id) - return getErr - }) - if !errors.Is(err, ErrTombstoned) { - return rec, err + rec, err = t.Get(id) + return err + }); err != nil { + return nil, err } - if _, rerr := b.recoverVMTombstone(ctx, id); rerr != nil { - return nil, fmt.Errorf("vm %s: recover interrupted delete: %w", id, rerr) + if tsRec == nil { + return rec, nil } - return nil, fmt.Errorf("vm %s was partially deleted; recovery finished the removal: %w", id, ErrNotFound) -} - -// guardVMTombstone refuses tombstoned VMs inside the entrypoint's own -// transaction: leased rolls back in place, deleting reports ErrTombstoned. -func (b *Backend) guardVMTombstone(ctx context.Context, t *vmTx, id string) error { - ts := b.tombstones() - rec, err := ts.Get(ctx, t.w, id) - if err != nil || rec == nil { - return err + if tsRec.Phase != tombstone.PhaseLeased { + if _, rerr := b.recoverVMTombstone(ctx, id); rerr != nil { + return nil, fmt.Errorf("vm %s: recover interrupted delete: %w", id, rerr) + } + return nil, fmt.Errorf("vm %s was partially deleted; recovery finished the removal: %w", id, ErrNotFound) } - if rec.Phase == tombstone.PhaseLeased { + if err := b.update(ctx, func(t *vmTx) error { taken, err := ts.TakeOver(ctx, t.w, id) if err != nil { return err } - return ts.Rollback(ctx, t.w, id, taken.LeaseID) + if rbErr := ts.Rollback(ctx, t.w, id, taken.LeaseID); rbErr != nil { + return rbErr + } + rec, err = t.Get(id) + return err + }); err != nil { + return nil, err } - return fmt.Errorf("vm %s: %w", id, ErrTombstoned) + return rec, nil } diff --git a/images/baseconfig.go b/images/baseconfig.go index d6628c91..65d26340 100644 --- a/images/baseconfig.go +++ b/images/baseconfig.go @@ -54,7 +54,7 @@ func (c *BaseConfig) OwnsBlob(hex string) bool { return err == nil } -func (c *BaseConfig) EnsureBaseDirs() error { +func (c *BaseConfig) EnsureDirs() error { if c.RootDir == "" { return fmt.Errorf("root dir must not be empty") } diff --git a/images/cloudimg/cloudimg.go b/images/cloudimg/cloudimg.go index f87fd3f5..ed2ea4b3 100644 --- a/images/cloudimg/cloudimg.go +++ b/images/cloudimg/cloudimg.go @@ -21,10 +21,11 @@ var _ images.Images = (*CloudImg)(nil) // CloudImg stores cloud image blobs for UEFI boot under Cloud Hypervisor. type CloudImg struct { + images.Ops[imageEntry] + conf *Config store *images.Store[imageEntry] pullGroup singleflight.Group - ops images.Ops[imageEntry] pinnedElsewhere func(context.Context) (map[string]struct{}, error) } @@ -41,7 +42,7 @@ func New(ctx context.Context, rootDir string, pullConns int, metaStore meta.Stor c := &CloudImg{ conf: cfg, store: store, - ops: images.Ops[imageEntry]{ + Ops: images.Ops[imageEntry]{ Store: store, Type: typ, LookupRefs: func(m map[string]*imageEntry, id string) []string { return images.LookupRefs(m, id) }, @@ -76,19 +77,6 @@ func (c *CloudImg) ImportFromReader(ctx context.Context, name string, tracker pr return importQcow2Reader(ctx, c.conf, c.store, name, tracker, r) } -// Inspect returns (nil, nil) if not found. -func (c *CloudImg) Inspect(ctx context.Context, id string) (*types.Image, error) { - return c.ops.Inspect(ctx, id) -} - -func (c *CloudImg) List(ctx context.Context) ([]*types.Image, error) { - return c.ops.List(ctx) -} - -func (c *CloudImg) Delete(ctx context.Context, ids []string) ([]string, error) { - return c.ops.Delete(ctx, ids) -} - // Config resolves cloud images to qcow2 storage plus firmware boot config. func (c *CloudImg) Config(ctx context.Context, vms []*types.VMConfig) (result [][]*types.StorageConfig, boot []*types.BootConfig, err error) { err = c.store.View(ctx, func(idx *imageIndex) error { diff --git a/images/cloudimg/commit.go b/images/cloudimg/commit.go index b5c34d0c..dfa113da 100644 --- a/images/cloudimg/commit.go +++ b/images/cloudimg/commit.go @@ -15,15 +15,7 @@ import ( "github.com/cocoonstack/cocoon/utils" ) -func commit( - ctx context.Context, - conf *Config, - store *images.Store[imageEntry], - ref string, - tracker progress.Tracker, - sourcePath string, - digestHex string, -) error { +func commit(ctx context.Context, conf *Config, store *images.Store[imageEntry], ref string, tracker progress.Tracker, sourcePath, digestHex string) error { logger := log.WithFunc("cloudimg.commit") blobPath := conf.BlobPath(digestHex) @@ -70,13 +62,7 @@ func commit( return nil } -func prepareTmpBlob( - ctx context.Context, - conf *Config, - tracker progress.Tracker, - sourcePath string, - digestHex string, -) (string, error) { +func prepareTmpBlob(ctx context.Context, conf *Config, tracker progress.Tracker, sourcePath, digestHex string) (string, error) { logger := log.WithFunc("cloudimg.prepareTmpBlob") info, err := inspectImage(ctx, sourcePath) diff --git a/images/cloudimg/config.go b/images/cloudimg/config.go index 334ac9e4..a357d408 100644 --- a/images/cloudimg/config.go +++ b/images/cloudimg/config.go @@ -26,10 +26,6 @@ func NewConfig(rootDir string, pullConns int) *Config { } } -func (c *Config) EnsureDirs() error { - return c.EnsureBaseDirs() -} - // tmpBlobPath uses a hidden prefix so a partial write is safe under last-writer-wins. func (c *Config) tmpBlobPath(digestHex string) string { return filepath.Join(c.TempDir(), ".tmp-"+digestHex+".qcow2") diff --git a/images/cloudimg/pull.go b/images/cloudimg/pull.go index 3b110668..dfcd4568 100644 --- a/images/cloudimg/pull.go +++ b/images/cloudimg/pull.go @@ -112,13 +112,7 @@ func pull(ctx context.Context, conf *Config, store *images.Store[imageEntry], ur }) } -func withDownload( - ctx context.Context, - conf *Config, - url string, - tracker progress.Tracker, - fn func(f *os.File, tmpPath, digestHex string) error, -) error { +func withDownload(ctx context.Context, conf *Config, url string, tracker progress.Tracker, fn func(f *os.File, tmpPath, digestHex string) error) error { tmpFile, tmpPath, cleanup, err := newTempImage(conf, "pull-*.img") if err != nil { return err diff --git a/images/gc_test.go b/images/gc_test.go index bdb33879..1ae5775d 100644 --- a/images/gc_test.go +++ b/images/gc_test.go @@ -180,7 +180,7 @@ func TestGCCollectRespectsExternalPins(t *testing.T) { // taker while held, releases cleanly, and a missing blob fails the pin. func TestPinBlobs(t *testing.T) { cfg := &BaseConfig{RootDir: t.TempDir(), Subdir: "oci", BlobExt: ".erofs"} - if err := cfg.EnsureBaseDirs(); err != nil { + if err := cfg.EnsureDirs(); err != nil { t.Fatal(err) } if err := os.WriteFile(cfg.BlobPath("deadbeef"), []byte("x"), 0o644); err != nil { diff --git a/images/oci/config.go b/images/oci/config.go index c9d4cff9..752757bf 100644 --- a/images/oci/config.go +++ b/images/oci/config.go @@ -24,7 +24,7 @@ func NewConfig(rootDir string, poolSize int) *Config { } func (c *Config) EnsureDirs() error { - if err := c.EnsureBaseDirs(); err != nil { + if err := c.BaseConfig.EnsureDirs(); err != nil { return err } return utils.EnsureDirs(c.BootBaseDir()) diff --git a/images/oci/oci.go b/images/oci/oci.go index d3fbd949..1429e59d 100644 --- a/images/oci/oci.go +++ b/images/oci/oci.go @@ -25,10 +25,11 @@ var _ images.Images = (*OCI)(nil) // OCI converts OCI container layers to EROFS for Cloud Hypervisor. type OCI struct { + images.Ops[imageEntry] + conf *Config store *images.Store[imageEntry] pullGroup singleflight.Group - ops images.Ops[imageEntry] pinnedElsewhere func(context.Context) (map[string]struct{}, error) } @@ -47,7 +48,7 @@ func New(ctx context.Context, rootDir string, poolSize int, metaStore meta.Store o := &OCI{ conf: cfg, store: store, - ops: images.Ops[imageEntry]{ + Ops: images.Ops[imageEntry]{ Store: store, Type: typ, LookupRefs: func(m map[string]*imageEntry, id string) []string { return images.LookupRefs(m, id, normalizeRef) }, @@ -75,20 +76,6 @@ func (o *OCI) ImportFromReader(ctx context.Context, name string, tracker progres return importTarFromReader(ctx, o.conf, o.store, name, tracker, r) } -// Inspect returns (nil, nil) if not found. -func (o *OCI) Inspect(ctx context.Context, id string) (*types.Image, error) { - return o.ops.Inspect(ctx, id) -} - -func (o *OCI) List(ctx context.Context) ([]*types.Image, error) { - return o.ops.List(ctx) -} - -// Delete returns actually-deleted refs; not-found ids are logged and skipped. -func (o *OCI) Delete(ctx context.Context, ids []string) ([]string, error) { - return o.ops.Delete(ctx, ids) -} - // Config builds StorageConfig + BootConfig from layer digests; errors if any blob is missing. func (o *OCI) Config(ctx context.Context, vms []*types.VMConfig) (result [][]*types.StorageConfig, boot []*types.BootConfig, err error) { err = o.store.View(ctx, func(idx *imageIndex) error { diff --git a/images/op.go b/images/op.go index b320a590..b9186eba 100644 --- a/images/op.go +++ b/images/op.go @@ -39,7 +39,7 @@ func (ops Ops[E]) List(ctx context.Context) (result []*types.Image, err error) { return result, err } -// Delete deletes entries from an index by ids and returns removed refs. +// Delete deletes entries from an index by ids and returns actually-removed refs; not-found ids are logged and skipped. func (ops Ops[E]) Delete(ctx context.Context, ids []string) (deleted []string, err error) { err = ops.Store.Update(ctx, func(idx *Index[E]) error { var delErr error diff --git a/meta/json/codec.go b/meta/json/codec.go index a9e55f9e..8b91ccc1 100644 --- a/meta/json/codec.go +++ b/meta/json/codec.go @@ -18,6 +18,10 @@ type Codec interface { var _ Codec = GenericCodec{} +type genericFile struct { + Tables map[string]map[string]json.RawMessage `json:"tables"` +} + // GenericCodec persists a Model as {"tables":{...}} for // namespaces with no legacy format (contract tests, future additions). // Insertion order is not preserved across reload: tables refill sorted by id. @@ -59,7 +63,3 @@ func (GenericCodec) Encode(m *Model) ([]byte, error) { } return append(data, '\n'), nil } - -type genericFile struct { - Tables map[string]map[string]json.RawMessage `json:"tables"` -} diff --git a/meta/json/model.go b/meta/json/model.go index 644dd9c2..64f184bb 100644 --- a/meta/json/model.go +++ b/meta/json/model.go @@ -8,6 +8,11 @@ import ( "slices" ) +type table struct { + ids []string + recs map[string]json.RawMessage +} + // Model is one namespace's decoded state: named tables preserving insertion // order — loaded file order first, new ids appended — which legacy codecs // rely on for order-sensitive fields. @@ -92,8 +97,3 @@ func (m *Model) TableNames() []string { // markClean resets Dirty after decode — loading populates via Put. func (m *Model) markClean() { m.dirty = false } - -type table struct { - ids []string - recs map[string]json.RawMessage -} diff --git a/snapshot/localfile/gc.go b/snapshot/localfile/gc.go index 1920e738..e87449a9 100644 --- a/snapshot/localfile/gc.go +++ b/snapshot/localfile/gc.go @@ -18,12 +18,7 @@ import ( "github.com/cocoonstack/cocoon/utils" ) -const ( - // pendingGCGrace lets a slow-storage snapshot finish before GC reclaims a pending record. - pendingGCGrace = 24 * time.Hour - - reasonOrphan = "orphan" -) +const reasonOrphan = "orphan" // EvictionPolicy controls LRU snapshot eviction; Enabled with zero criteria evicts all non-pending. type EvictionPolicy struct { @@ -98,7 +93,6 @@ func gcModule(lf *LocalFile, policy EvictionPolicy) gc.Module[snapshotGCSnapshot Recover: lf.gcRecover, ReadDB: func(ctx context.Context) (snapshotGCSnapshot, error) { snap := snapshotGCSnapshot{policy: policy, reasons: make(map[string]string)} - cutoff := time.Now().Add(-pendingGCGrace) if err := lf.view(ctx, func(t *snapTx) error { snap.blobIDs = make(map[string]struct{}) snap.snapshotIDs = make(map[string]struct{}) @@ -107,9 +101,7 @@ func gcModule(lf *LocalFile, policy EvictionPolicy) gc.Module[snapshotGCSnapshot snap.snapshotIDs[id] = struct{}{} maps.Copy(snap.blobIDs, rec.ImageBlobIDs) if rec.Pending { - if rec.CreatedAt.Before(cutoff) { - snap.stalePending = append(snap.stalePending, id) - } + snap.stalePending = append(snap.stalePending, id) return nil } if _, statErr := os.Stat(cmp.Or(rec.DataDir, conf.SnapshotDataDir(id))); errors.Is(statErr, fs.ErrNotExist) { @@ -163,21 +155,20 @@ func gcModule(lf *LocalFile, policy EvictionPolicy) gc.Module[snapshotGCSnapshot }, Collect: func(ctx context.Context, ids []string, snap snapshotGCSnapshot) error { logger := log.WithFunc("gc.snapshot") - cutoff := time.Now().Add(-pendingGCGrace) var errs []error for _, id := range ids { if err := ctx.Err(); err != nil { errs = append(errs, err) break } - // Exclusive lease: an active clone/restore/export holds it shared; skip and retry next cycle. + // Exclusive lease: an active save holds it exclusively (build lease) and readers (clone/restore/export) hold it shared, so acquiring it is the proof a pending record's owner died — no age gate needed. fl, ok, lockErr := lf.tryExclusiveLease(id) if lockErr != nil { errs = append(errs, lockErr) continue } if !ok { - logger.Warnf(ctx, "skip %s: leased by an active reader", id) + logger.Warnf(ctx, "skip %s: leased by an active holder", id) continue } // Candidacy revalidation under the lease: the reason picked at ReadDB must still hold, or a create/touch that landed in the window evicts the wrong snapshot. @@ -186,7 +177,7 @@ func gcModule(lf *LocalFile, policy EvictionPolicy) gc.Module[snapshotGCSnapshot pending, sawRecord = rec.Pending, true switch snap.reasons[id] { case "stale-pending": - return rec.Pending && rec.CreatedAt.Before(cutoff) + return rec.Pending case "missing-dir": _, statErr := os.Stat(cmp.Or(rec.DataDir, conf.SnapshotDataDir(id))) return errors.Is(statErr, fs.ErrNotExist) diff --git a/snapshot/localfile/gc_test.go b/snapshot/localfile/gc_test.go index 4d28ba28..e66e63a8 100644 --- a/snapshot/localfile/gc_test.go +++ b/snapshot/localfile/gc_test.go @@ -1,7 +1,9 @@ package localfile import ( + "errors" "fmt" + "io/fs" "maps" "os" "path/filepath" @@ -484,6 +486,68 @@ func TestGCModule_OrphanAndStalePendingDoNotEmit(t *testing.T) { } } +// TestGCCollectsFreshPendingWithFreeLease pins the lease-free ownerless proof: +// a dead save's pending record is reclaimed on the next pass, however young — +// the free build lease is the proof, not an age gate. +func TestGCCollectsFreshPendingWithFreeLease(t *testing.T) { + lf := newTestLF(t) + ctx := t.Context() + id := testID(t) + if _, err := lf.beginCreate(ctx, &types.SnapshotConfig{ID: id, Name: "fresh-pending"}); err != nil { + t.Fatalf("beginCreate: %v", err) + } + if err := os.MkdirAll(lf.conf.SnapshotDataDir(id), 0o750); err != nil { + t.Fatal(err) + } + + mod := gcModule(lf, EvictionPolicy{}) + snap, err := mod.ReadDB(ctx) + if err != nil { + t.Fatal(err) + } + ids := mod.Resolve(ctx, snap, map[string]any{}) + if !slices.Contains(ids, id) { + t.Fatalf("candidates = %v, want the fresh pending record picked", ids) + } + if err := mod.Collect(ctx, ids, snap); err != nil { + t.Fatal(err) + } + if _, held, err := lf.NameOwner(ctx, "fresh-pending"); err != nil || held { + t.Fatalf("NameOwner = (%v, %v), want the dead save's name freed", err, held) + } + if _, err := os.Stat(lf.conf.SnapshotDataDir(id)); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("data dir survives: %v", err) + } +} + +// TestGCSkipsPendingHeldByLiveBuild pins the other half of the proof: a save +// still holding its build lease must not lose its pending record to GC. +func TestGCSkipsPendingHeldByLiveBuild(t *testing.T) { + lf := newTestLF(t) + ctx := t.Context() + id := testID(t) + release, err := lf.acquireBuildLease(id) + if err != nil { + t.Fatalf("acquireBuildLease: %v", err) + } + defer release() + if _, err := lf.beginCreate(ctx, &types.SnapshotConfig{ID: id, Name: "live-build"}); err != nil { + t.Fatalf("beginCreate: %v", err) + } + + mod := gcModule(lf, EvictionPolicy{}) + snap, err := mod.ReadDB(ctx) + if err != nil { + t.Fatal(err) + } + if err := mod.Collect(ctx, mod.Resolve(ctx, snap, map[string]any{}), snap); err != nil { + t.Fatal(err) + } + if owner, held, err := lf.NameOwner(ctx, "live-build"); err != nil || !held || owner != id { + t.Fatalf("NameOwner = (%q, %v, %v), want the live build's record untouched", owner, held, err) + } +} + func agedMeta(ageHours int, size int64) snapshotMeta { accessedAt := time.Now().Add(-time.Duration(ageHours) * time.Hour) return snapshotMeta{lastAccessed: accessedAt, sizeBytes: size} diff --git a/snapshot/localfile/localfile.go b/snapshot/localfile/localfile.go index a07af226..98ce39a0 100644 --- a/snapshot/localfile/localfile.go +++ b/snapshot/localfile/localfile.go @@ -232,6 +232,25 @@ func (lf *LocalFile) RegisterGC(orch *gc.Orchestrator) { gc.Register(orch, gcModule(lf, lf.gcPolicy)) } +// NameOwner reports the record holding name in the index, pending or not — this is the reservation insertRecord enforces, so the save preflight sees the same thing the insert will. +func (lf *LocalFile) NameOwner(ctx context.Context, name string) (string, bool, error) { + if name == "" { + return "", false, nil + } + var ( + id string + held bool + ) + if err := lf.view(ctx, func(t *snapTx) error { + var err error + id, held, err = t.NameGet(name) + return err + }); err != nil { + return "", false, err + } + return id, held, nil +} + // tryExclusiveLease TryLocks id's lease file; ok=false means an active holder. The caller owns fl.Close() when ok. func (lf *LocalFile) tryExclusiveLease(id string) (fl *gofrsflock.Flock, ok bool, err error) { fl = gofrsflock.New(lf.conf.LeasePath(id)) diff --git a/snapshot/localfile/localfile_test.go b/snapshot/localfile/localfile_test.go index d7f5a16f..8b5fe850 100644 --- a/snapshot/localfile/localfile_test.go +++ b/snapshot/localfile/localfile_test.go @@ -270,6 +270,38 @@ func TestRollbackCreateSurvivesCanceledContext(t *testing.T) { } } +// TestNameOwnerSeesPendingReservation pins what a killed save leaves behind: the +// pending record still holds the name in the index, so Inspect reports not-found +// while insertRecord rejects the reuse. The save preflight resolves through the +// index for exactly this reason — otherwise the capture runs to completion first. +func TestNameOwnerSeesPendingReservation(t *testing.T) { + lf := newTestLF(t) + ctx := t.Context() + id := testID(t) + if _, err := lf.beginCreate(ctx, &types.SnapshotConfig{ID: id, Name: "pending-snap"}); err != nil { + t.Fatalf("beginCreate: %v", err) + } + + if _, err := lf.Inspect(ctx, "pending-snap"); !errors.Is(err, snapshot.ErrNotFound) { + t.Fatalf("Inspect on a pending record = %v, want ErrNotFound", err) + } + owner, held, err := lf.NameOwner(ctx, "pending-snap") + if err != nil { + t.Fatalf("NameOwner: %v", err) + } + if !held || owner != id { + t.Fatalf("NameOwner = (%q, %v), want (%q, true)", owner, held, id) + } + if _, held, err = lf.NameOwner(ctx, "never-taken"); err != nil || held { + t.Fatalf("NameOwner on a free name = (%v, %v), want (nil, false)", err, held) + } + + lf.rollbackCreate(ctx, id, "pending-snap") + if _, held, err = lf.NameOwner(ctx, "pending-snap"); err != nil || held { + t.Fatalf("NameOwner after rollback = (%v, %v), want the name released", err, held) + } +} + func TestCreate(t *testing.T) { lf := newTestLF(t) ctx := t.Context() diff --git a/snapshot/snapshot.go b/snapshot/snapshot.go index 2524f97b..1e3b4c81 100644 --- a/snapshot/snapshot.go +++ b/snapshot/snapshot.go @@ -18,6 +18,11 @@ type DirectCreator interface { CreateFromDir(ctx context.Context, cfg *types.SnapshotConfig, srcDir string) (id string, ok bool, err error) } +// NameHolder is an optional interface for backends whose name index can be held by a record Inspect does not resolve — a pending one left by a save that died before it could roll back. The save preflight consults it so a taken name is rejected up front instead of after the whole capture has been written. +type NameHolder interface { + NameOwner(ctx context.Context, name string) (id string, held bool, err error) +} + // CompressedExporter is an optional interface for backends that support exporting with compression (e.g. gzip). The default Export produces raw tar. type CompressedExporter interface { ExportCompressed(ctx context.Context, ref string) (io.ReadCloser, error)