diff --git a/.github/workflows/build-os-images.yml b/.github/workflows/build-os-images.yml index a4355b90..7e293b2f 100644 --- a/.github/workflows/build-os-images.yml +++ b/.github/workflows/build-os-images.yml @@ -5,7 +5,6 @@ on: branches: [master] paths: - "os-image/**" - - "!os-image/windows/**" - "!os-image/**/*.md" workflow_dispatch: inputs: @@ -43,7 +42,6 @@ jobs: declare -A names while IFS= read -r dockerfile; do [[ -z "$dockerfile" ]] && continue - [[ -f "$(dirname "$dockerfile")/.skip-generic-build" ]] && continue name=$(echo "$dockerfile" | cut -d'/' -f2) names[$name]=1 done <<< "$(find os-image -name Dockerfile -type f)" @@ -55,7 +53,6 @@ jobs: declare -A names while IFS= read -r dockerfile; do [[ -z "$dockerfile" ]] && continue - [[ -f "$(dirname "$dockerfile")/.skip-generic-build" ]] && continue name=$(echo "$dockerfile" | cut -d'/' -f2) names[$name]=1 done <<< "$(find os-image -name Dockerfile -type f)" @@ -65,7 +62,6 @@ jobs: declare -A names for f in $all_changed; do tag_dir=$(echo "$f" | cut -d'/' -f1-3) - [[ -f "$tag_dir/.skip-generic-build" ]] && continue name=$(echo "$f" | cut -d'/' -f2) if [[ -n "$name" ]]; then names[$name]=1 @@ -104,7 +100,6 @@ jobs: # Find all Dockerfiles for this name while IFS= read -r dockerfile; do [[ -z "$dockerfile" ]] && continue - [[ -f "$(dirname "$dockerfile")/.skip-generic-build" ]] && continue tag=$(echo "$dockerfile" | cut -d'/' -f3) [[ -n "$tag" ]] || continue tag_no_cache="$needs_no_cache" diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 2bdcd052..34c7a5cd 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -7,6 +7,9 @@ on: paths-ignore: - 'os-image/**' +permissions: + contents: read + jobs: build: runs-on: ubuntu-latest diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 89c606c4..d7e1ffa7 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -12,6 +12,9 @@ on: paths-ignore: - 'os-image/**' +permissions: + contents: read + jobs: lint: runs-on: ubuntu-latest diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5810f1bb..dcb7d989 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,6 +12,9 @@ on: paths-ignore: - 'os-image/**' +permissions: + contents: read + jobs: test: runs-on: ubuntu-latest diff --git a/README.md b/README.md index aa8531ee..4b838b5f 100644 --- a/README.md +++ b/README.md @@ -20,12 +20,12 @@ Lightweight MicroVM engine with dual hypervisor backends: [Cloud Hypervisor](htt ## Quick Start ```bash -# Install the binary (Linux amd64; arm64: cocoon_0.7.6_Linux_arm64.tar.gz) -curl -fsSL -o cocoon.tar.gz https://github.com/cocoonstack/cocoon/releases/download/v0.7.6/cocoon_0.7.6_Linux_x86_64.tar.gz +# Install the binary (Linux amd64; arm64: cocoon_0.7.7_Linux_arm64.tar.gz) +curl -fsSL -o cocoon.tar.gz https://github.com/cocoonstack/cocoon/releases/download/v0.7.7/cocoon_0.7.7_Linux_x86_64.tar.gz tar -xzf cocoon.tar.gz && install -m 0755 cocoon /usr/local/bin/ # One-time environment setup (installs CH, firmware, CNI plugins) -curl -fsSL -o cocoon-check https://raw.githubusercontent.com/cocoonstack/cocoon/refs/tags/v0.7.6/doctor/check.sh +curl -fsSL -o cocoon-check https://raw.githubusercontent.com/cocoonstack/cocoon/refs/tags/v0.7.7/doctor/check.sh install -m 0755 cocoon-check /usr/local/bin/ && sudo cocoon-check --upgrade # Pull an image and run a VM diff --git a/cgroup/cgroup.go b/cgroup/cgroup.go index d743aff1..6c7567cc 100644 --- a/cgroup/cgroup.go +++ b/cgroup/cgroup.go @@ -317,9 +317,6 @@ func placeScope(parentDir, dir, cpuset string) error { if cpuset == "" { return nil } - if current, err := readControl(dir, cpusetName); err == nil && sameCPUList(current, cpuset) { - return nil - } if err := checkSubset(cpuset, parentDir, "cpuset_cpus placement"); err != nil { return err } @@ -384,7 +381,7 @@ func explainFenceWrite(parentDir, current, fence string, err error) error { return fmt.Errorf("cgroup_cpus %q -> %q rejected with %d VM(s) running; the fence is machine-wide, drain them first: %w", current, fence, len(ids), err) } -// enableControllers reads before writing: subtree_control writes take the kernel's hierarchy-wide cgroup_mutex, so steady-state launches must not contend on a no-op write. Missing controllers go in one combined write. +// enableControllers reads before writing: subtree_control writes take the kernel's hierarchy-wide cgroup_mutex, so steady-state launches must not contend on a no-op write. func enableControllers(dir string, ctrls []string) error { have, _ := readControl(dir, subtreeControlName) enabled := strings.Fields(have) diff --git a/cgroup/cgroup_test.go b/cgroup/cgroup_test.go index 87ebfbd9..7f8215fc 100644 --- a/cgroup/cgroup_test.go +++ b/cgroup/cgroup_test.go @@ -278,20 +278,6 @@ func TestExplainFenceWriteNamesLiveVMs(t *testing.T) { } } -func TestPlaceScopeReadGate(t *testing.T) { - parent := t.TempDir() - dir := ScopeDir(parent, "X") - if err := os.Mkdir(dir, 0o755); err != nil { - t.Fatalf("setup: %v", err) - } - if err := os.WriteFile(filepath.Join(dir, "cpuset.cpus"), []byte("2-3\n"), 0o600); err != nil { - t.Fatalf("setup: %v", err) - } - if err := placeScope(parent, dir, "2,3"); err != nil { - t.Errorf("equal placement rewrote: %v", err) - } -} - func TestArmWritesQuotaAndBurst(t *testing.T) { parent := t.TempDir() dir := ScopeDir(parent, "A") diff --git a/cmd/core/metastore.go b/cmd/core/metastore.go index 339a8c7c..8ad1456f 100644 --- a/cmd/core/metastore.go +++ b/cmd/core/metastore.go @@ -123,7 +123,7 @@ func MetaStore(conf *config.Config) (meta.Store, error) { // Bootstrap owns its context: the store outlives any single caller, and a canceled first caller must not poison the Once for everyone. ctx, cancel := context.WithTimeout(context.Background(), metaBootstrapTimeout) defer cancel() - // Ordinary opens of EITHER engine refuse while a conversion is in flight (§6); the json engine cannot see the manifest itself. + // Ordinary opens of EITHER engine refuse while a conversion is in flight; the json engine cannot see the manifest itself. dbPath := MetaDBPath(conf) if err := metasqlite.RefuseManifest(dbPath); err != nil { metaErr = err diff --git a/cmd/core/utils.go b/cmd/core/utils.go index 9650de7e..d16d4757 100644 --- a/cmd/core/utils.go +++ b/cmd/core/utils.go @@ -234,7 +234,6 @@ func resolveOwner[T typed](backends []T, ref string, found func(T) (bool, error) } } -// resolveVMOwner returns the owning hypervisor and the resolved VM. func resolveVMOwner(ctx context.Context, hypers []hypervisor.Hypervisor, ref string) (hypervisor.Hypervisor, *types.VM, error) { var resolved *types.VM owner, err := resolveOwner( diff --git a/cmd/daemon/commands.go b/cmd/daemon/commands.go index 19ff83bd..5ab24776 100644 --- a/cmd/daemon/commands.go +++ b/cmd/daemon/commands.go @@ -19,6 +19,6 @@ func Command(h Handler) *cobra.Command { cmd.Flags().Duration("gc-interval", 0, "sweep on this cadence in a background goroutine; 0 (default) leaves gc a CLI verb") cmd.Flags().String("api-socket", "", "read-only API socket path (default: /cocoond.sock)") cmd.Flags().Bool("no-api", false, "disable the read-only API") - cmd.Flags().Uint32("api-socket-mode", cocoond.DefaultAPISockMode, "read-only API socket permission bits") + cmd.Flags().Uint32("api-socket-mode", 0, "read-only API socket permission bits, octal with a leading 0 (default 0660)") return cmd } diff --git a/cmd/images/manage.go b/cmd/images/manage.go index 0fa90b60..5fc55e32 100644 --- a/cmd/images/manage.go +++ b/cmd/images/manage.go @@ -68,18 +68,18 @@ func (h Handler) RM(cmd *cobra.Command, args []string) error { refsByBackend[owner] = append(refsByBackend[owner], ref) } - var allDeleted []string + total := 0 for backend, refs := range refsByBackend { deleted, err := backend.Delete(ctx, refs) if err != nil { return fmt.Errorf("delete %s: %w", backend.Type(), err) } - allDeleted = append(allDeleted, deleted...) - } - for _, ref := range allDeleted { - logger.Infof(ctx, "deleted: %s", ref) + for _, ref := range deleted { + logger.Infof(ctx, "deleted: %s", ref) + } + total += len(deleted) } - if len(allDeleted) == 0 { + if total == 0 { logger.Info(ctx, "no matching images found") } return nil diff --git a/cmd/meta/convert/convert.go b/cmd/meta/convert/convert.go index ae21c9df..89d6c6b4 100644 --- a/cmd/meta/convert/convert.go +++ b/cmd/meta/convert/convert.go @@ -1,4 +1,4 @@ -// Package convert runs the offline meta engine cutover (design §6). +// Package convert runs the offline meta engine cutover. package convert import ( @@ -25,7 +25,7 @@ import ( const asideSuffix = ".converted-" -// testCrashStep injects crashes between protocol steps (§9 gate); nil in prod. +// testCrashStep injects crashes between protocol steps; nil in prod. var testCrashStep func(step string) error // Spec carries both engines' declarations from the composition root. @@ -121,7 +121,7 @@ func convertAll(ctx context.Context, spec Spec, target string, m *Manifest) erro if m, err = newManifest(ctx, spec, target, src); err != nil { return err } - // The manifest is durable before the first target byte (§6). + // The manifest is durable before the first target byte. if serr := saveManifest(spec.MetaRoot, m); serr != nil { return serr } @@ -174,7 +174,7 @@ func newManifest(ctx context.Context, spec Spec, target string, src meta.Store) return m, nil } -// checkQuiesced refuses conversion while a live process holds the source (§6). +// checkQuiesced refuses conversion while a live process holds the source. func checkQuiesced(ctx context.Context, spec Spec, target string, src meta.Store) error { if target == config.MetaBackendJSON { // sqlite source: an empty durable transaction fails ErrBusy while a writer is mid-flight. @@ -238,7 +238,7 @@ func openTarget(ctx context.Context, spec Spec, target string) (meta.Store, erro } func convertNamespace(ctx context.Context, src, dst meta.Store, ns metasqlite.Namespace, rec *NSRecord, m *Manifest, spec Spec) error { - // A committed-but-unmarked target (crash window) is re-verified and claimed, never redone (§6). + // A committed-but-unmarked target (crash window) is re-verified and claimed, never redone. dstDigest, dstCount, err := canonicalDigest(ctx, dst, ns) if err != nil { return err @@ -268,7 +268,7 @@ func convertNamespace(ctx context.Context, src, dst meta.Store, ns metasqlite.Na return crashStep("ns-done") } -// duplicateGeneration writes the json target's .prev so imported data survives a torn main (§9). +// duplicateGeneration writes the json target's .prev so imported data survives a torn main. func duplicateGeneration(spec Spec, nsName string) error { jns, ok := findJSON(spec, nsName) if !ok { @@ -345,7 +345,7 @@ func copyNamespace(ctx context.Context, src, dst meta.Store, ns metasqlite.Names }) } -// verifyNames checks every name entry points at an existing record (§6). +// verifyNames checks every name entry points at an existing record. func verifyNames(ctx context.Context, s meta.Store, ns metasqlite.Namespace) error { if !slices.Contains(ns.Tables, "names") { return nil @@ -368,7 +368,7 @@ func verifyNames(ctx context.Context, s meta.Store, ns metasqlite.Namespace) err }) } -// canonicalDigest is the engine-neutral namespace fingerprint (§6): rows sorted by id per table, tables in declaration order; count is records only. +// canonicalDigest is the engine-neutral namespace fingerprint: rows sorted by id per table, tables in declaration order; count is records only. func canonicalDigest(ctx context.Context, s meta.Store, ns metasqlite.Namespace) (string, int, error) { h := sha256.New() count := 0 @@ -431,7 +431,7 @@ func sourceFiles(spec Spec, target, nsName string) []string { return []string{spec.DBPath} } if jns, ok := findJSON(spec, nsName); ok { - // Both json generations are authority (§6). + // Both json generations are authority. return []string{jns.FilePath, jns.FilePath + ".prev"} } return nil diff --git a/cmd/meta/handler.go b/cmd/meta/handler.go index c2ee7004..aa2871ff 100644 --- a/cmd/meta/handler.go +++ b/cmd/meta/handler.go @@ -36,7 +36,7 @@ func (h Handler) InitStore(cmd *cobra.Command, _ []string) error { func (h Handler) Convert(cmd *cobra.Command, _ []string) error { ctx, conf := h.Init(cmd) - // The configured backend is the sole target authority (§6): convert always moves the OTHER engine's data into the effective backend. + // The configured backend is the sole target authority: convert always moves the OTHER engine's data into the effective backend. target := cmp.Or(conf.MetaBackend, config.MetaBackendSQLite) dbPath := cmdcore.MetaDBPath(conf) spec := convert.Spec{ diff --git a/cmd/others/commands.go b/cmd/others/commands.go index ba6fcd03..b0bf2abe 100644 --- a/cmd/others/commands.go +++ b/cmd/others/commands.go @@ -1,12 +1,7 @@ -// Package others defines the top-level gc, version, and completion commands. +// Package others defines the top-level gc and version commands. package others -import ( - "fmt" - "os" - - "github.com/spf13/cobra" -) +import "github.com/spf13/cobra" func Commands(h Handler) []*cobra.Command { gcCmd := &cobra.Command{ @@ -28,26 +23,5 @@ func Commands(h Handler) []*cobra.Command { Args: cobra.NoArgs, RunE: h.Version, }, - { - Use: "completion [bash|zsh|fish|powershell]", - Short: "Generate shell completion script", - Args: cobra.ExactArgs(1), - ValidArgs: []string{"bash", "zsh", "fish", "powershell"}, - RunE: func(cmd *cobra.Command, args []string) error { - root := cmd.Root() - switch args[0] { - case "bash": - return root.GenBashCompletion(os.Stdout) - case "zsh": - return root.GenZshCompletion(os.Stdout) - case "fish": - return root.GenFishCompletion(os.Stdout, true) - case "powershell": - return root.GenPowerShellCompletionWithDesc(os.Stdout) - default: - return fmt.Errorf("unsupported shell: %s", args[0]) - } - }, - }, } } diff --git a/cmd/root.go b/cmd/root.go index 3fc8173d..f36a243f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -115,7 +115,6 @@ func initConfig(ctx context.Context) error { viper.SetConfigFile(cfgFile) } if err := viper.ReadInConfig(); err != nil { - // No config file is OK; a corrupt/unreadable one is not. if _, ok := errors.AsType[viper.ConfigFileNotFoundError](err); !ok { return fmt.Errorf("read config: %w", err) } diff --git a/cmd/snapshot/handler.go b/cmd/snapshot/handler.go index f668e709..9c19f5a0 100644 --- a/cmd/snapshot/handler.go +++ b/cmd/snapshot/handler.go @@ -216,7 +216,7 @@ func (h Handler) Import(cmd *cobra.Command, args []string) error { return err } - var r io.Reader + var r *os.File if len(args) > 0 { f, openErr := os.Open(args[0]) if openErr != nil { @@ -229,6 +229,7 @@ func (h Handler) Import(cmd *cobra.Command, args []string) error { r = os.Stdin logger.Info(ctx, "importing from stdin ...") } + defer cmdcore.CloseOnCancel(ctx, r)() snapID, err := snapBackend.Import(ctx, r, name, description) if err != nil { diff --git a/cmd/vm/commands.go b/cmd/vm/commands.go index f5bc98a7..79b8f5ba 100644 --- a/cmd/vm/commands.go +++ b/cmd/vm/commands.go @@ -162,7 +162,7 @@ func Command(h Handler) *cobra.Command { } addVMFlags(debugCmd) debugCmd.Flags().Int("max-cpu", 8, "max CPUs") - debugCmd.Flags().Int("balloon", 0, "balloon size in MB") + debugCmd.Flags().Int("balloon", 0, "balloon size in MB (0 = auto)") debugCmd.Flags().String("cow", "", "COW disk path") debugCmd.Flags().String("ch", "", "cloud-hypervisor binary path (default: ch_binary from the config)") @@ -356,7 +356,7 @@ func addVMFlags(cmd *cobra.Command) { cmd.Flags().String("memory", "1G", "memory size") cmd.Flags().String("storage", "10G", "COW disk size") cmd.Flags().Int("nics", 1, "number of network interfaces (0 = no network); OCI images up to v0.7.7 configure only the last NIC") - cmd.Flags().Int("queue-size", 0, "virtio-net ring depth per queue (0 = default 512; tradeoff: larger improves download throughput, smaller improves RPC latency)") + cmd.Flags().Int("queue-size", 0, "virtio-net ring depth per queue (0 = default 512; larger improves download throughput, smaller improves RPC latency; CH only, ignored by FC)") cmd.Flags().Int("disk-queue-size", 0, "virtio-blk ring depth per device (0 = default 512; CH only, ignored by FC)") cmd.Flags().Int("cpu-weight", 0, "cgroup cpu.weight, 1..10000 (0 = vCPU count)") addCPUBandwidthFlags(cmd) diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index 025a9263..47c3943e 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -108,8 +108,11 @@ func (h Handler) Console(cmd *cobra.Command, args []string) error { fmt.Fprintf(os.Stderr, "Connected to %s (escape sequence: %s.)\r\n", ref, escapeDisplay) if f, ok := conn.(*os.File); ok { - cleanup := console.HandleResize(inFd, f.Fd()) - defer cleanup() + if rc, rcErr := f.SyscallConn(); rcErr == nil { + var ptyFd uintptr + _ = rc.Control(func(fd uintptr) { ptyFd = fd }) + defer console.HandleResize(inFd, ptyFd)() + } } escapeKeys := []byte{escapeChar, '.'} diff --git a/cmd/vm/logstream.go b/cmd/vm/logstream.go index 3c4e7acf..1968c4ae 100644 --- a/cmd/vm/logstream.go +++ b/cmd/vm/logstream.go @@ -64,12 +64,12 @@ func streamLog(ctx context.Context, path string, follow bool, tail int) error { } // Sig mismatch catches O_TRUNC re-opens. newSig, _ := utils.FileHead(f, logHeadSigLen) - if !bytes.Equal(newSig, sig) { + if !bytes.HasPrefix(newSig, sig) { if _, err := f.Seek(0, io.SeekStart); err != nil { return fmt.Errorf("rewind log: %w", err) } - sig = newSig } + sig = newSig if _, err := io.Copy(os.Stdout, f); err != nil { return fmt.Errorf("read log: %w", err) } @@ -111,7 +111,6 @@ func seekToLastNLines(f *os.File, n int) error { return err } -// watchFile watches the parent dir because an atomic rename changes the inode. func watchFile(ctx context.Context, filePath string, debounce time.Duration) (<-chan struct{}, error) { watcher, err := fsnotify.NewWatcher() if err != nil { diff --git a/cmd/vm/logstream_test.go b/cmd/vm/logstream_test.go index 63142406..672944c3 100644 --- a/cmd/vm/logstream_test.go +++ b/cmd/vm/logstream_test.go @@ -8,9 +8,36 @@ import ( "testing" "time" + "github.com/cocoonstack/cocoon/cmd/cliutil/clitest" "github.com/cocoonstack/cocoon/utils" ) +func TestStreamLogFollowsAShortHeadWithoutReprinting(t *testing.T) { + path := filepath.Join(t.TempDir(), "vm.log") + if err := os.WriteFile(path, []byte("ab\n"), 0o644); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(t.Context()) + out := clitest.CaptureStdout(t, func() { + go func() { + time.Sleep(200 * time.Millisecond) + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0) + if err == nil { + _, _ = f.WriteString("cd\n") + _ = f.Close() + } + time.Sleep(500 * time.Millisecond) + cancel() + }() + if err := streamLog(ctx, path, true, 0); err != nil { + t.Errorf("streamLog: %v", err) + } + }) + if out != "ab\ncd\n" { + t.Fatalf("output = %q, want the appended line once, not the head again", out) + } +} + func TestWatchFile(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "data.json") diff --git a/cmd/vm/reseed.go b/cmd/vm/reseed.go index 6cfdb38b..3c364920 100644 --- a/cmd/vm/reseed.go +++ b/cmd/vm/reseed.go @@ -40,9 +40,6 @@ func (h Handler) Reseed(cmd *cobra.Command, args []string) error { // reseedAfterResume detaches by default: the post-resume vsock dial would otherwise sit on every clone/restore critical path. func (h Handler) reseedAfterResume(ctx context.Context, conf *config.Config, hyper hypervisor.Hypervisor, vm *types.VM, regenMachineID bool) { - if vm.VsockSocket == "" { - vm = refreshVM(ctx, hyper, vm) - } if vm.VsockSocket == "" || vm.Config.Windows || !detachReseed(ctx, conf, hyper, vm, regenMachineID) { signalReseed(ctx, vm, regenMachineID) } diff --git a/cmd/vm/status.go b/cmd/vm/status.go index 9b881594..4b32fa3e 100644 --- a/cmd/vm/status.go +++ b/cmd/vm/status.go @@ -175,9 +175,8 @@ func statusRefreshLoop(ctx context.Context, hypers []hypervisor.Hypervisor, filt } func statusEventLoop(ctx context.Context, hypers []hypervisor.Hypervisor, filters []string, watchCh <-chan struct{}, tick <-chan time.Time) { - fmt.Println("EVENT\tID\tNAME\tSTATE\tCPU\tMEMORY\tIP\tIMAGE") - w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) + fmt.Fprintln(w, "EVENT\tID\tNAME\tSTATE\tCPU\tMEMORY\tIP\tIMAGE") //nolint:errcheck statusEventDiffLoop(ctx, hypers, filters, watchCh, tick, eventEmitter{ emit: func(event string, snap vmSnapshot, _ vmOutput) { printEventRow(w, event, snap) }, end: func() { _ = w.Flush() }, diff --git a/daemon/api.go b/daemon/api.go index d153c015..37680677 100644 --- a/daemon/api.go +++ b/daemon/api.go @@ -49,7 +49,7 @@ func (d *Daemon) serveAPI(ctx context.Context) (func(), error) { return nil, err } logger := log.WithFunc("daemon.serveAPI") - srv := &http.Server{Handler: d.routes(), ReadHeaderTimeout: apiReadHeaderTimeout} + srv := &http.Server{Handler: d.routes(), ReadHeaderTimeout: apiReadHeaderTimeout, BaseContext: func(net.Listener) context.Context { return ctx }} go func() { if err := srv.Serve(ln); err != nil && !errors.Is(err, http.ErrServerClosed) { logger.Error(ctx, err, "api server") diff --git a/daemon/api_test.go b/daemon/api_test.go index 25f59fa2..27a9a5d8 100644 --- a/daemon/api_test.go +++ b/daemon/api_test.go @@ -2,8 +2,11 @@ package daemon import ( "context" + "net" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "testing/synctest" @@ -52,6 +55,36 @@ func TestReadsWaitForTheFirstPass(t *testing.T) { } } +func TestAPIShutdownEndsAnEventsStream(t *testing.T) { + dir, err := os.MkdirTemp("/tmp", "cocoond-api-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + sock := filepath.Join(dir, "api.sock") + d := &Daemon{conf: Config{APIAddr: sock}, state: newCache()} + d.state.publish(nil, true, 0, time.Now()) + ctx, cancel := context.WithCancel(t.Context()) + stop, err := d.serveAPI(ctx) + if err != nil { + t.Fatal(err) + } + hc := &http.Client{Transport: &http.Transport{DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, "unix", sock) + }}} + resp, err := hc.Get("http://cocoond/v1/events") + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + cancel() + start := time.Now() + stop() + if took := time.Since(start); took >= apiShutdownTimeout { + t.Fatalf("shutdown took %s, want the stream ended by the daemon ctx", took) + } +} + func TestReadsUnblockOnAFailedFirstPass(t *testing.T) { synctest.Test(t, func(t *testing.T) { d := &Daemon{state: newCache()} diff --git a/daemon/reconcile_test.go b/daemon/reconcile_test.go index 745eb76f..d9eb3d3c 100644 --- a/daemon/reconcile_test.go +++ b/daemon/reconcile_test.go @@ -202,7 +202,7 @@ func TestFailingRollForwardDoesNotWakeItsOwnDaemon(t *testing.T) { defer store.Close() ts := tombstone.NewTable(ns) if err := store.Update(ctx, meta.Scope{Write: ns}, meta.CommitDurable, func(w meta.Writer) error { - lease, err := ts.Lease(ctx, w, "vm1", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: []byte(`{}`)}) + lease, err := ts.Lease(ctx, w, "vm1", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: []byte(`{}`)}) if err != nil { return err } diff --git a/docs/cli.md b/docs/cli.md index c511d653..cf850868 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -28,7 +28,7 @@ cocoon │ ├── logs [-f] [--tail N] VM Print the per-VM hypervisor log file │ ├── rm [flags] VM [VM...] Delete VM(s) (--force kills running VMs immediately) │ ├── reconcile-stale-create VM Reclaim an ownerless creating placeholder (JSON outcome) -│ ├── restore [flags] VM [SNAP] Restore a VM (running or stopped) to a snapshot (or --from-dir) +│ ├── restore [flags] VM [SNAP] Restore a VM (running, stopped or error) to a snapshot (or --from-dir) │ ├── hibernate [flags] VM Atomically snapshot a running VM and stop it │ ├── status [VM...] Show VM status (--watch refreshes, --event streams) │ ├── fs @@ -71,14 +71,14 @@ The meta engine is selected by `meta_backend` in the config; unset auto-resolves | Flag | Env Variable | Default | Description | | ----------------- | ------------------------------ | ------------------ | -------------------------------------- | -| `--config` | | | Config file path | +| `--config` | | | Config file path (none is read by default; a missing explicit path is an error) | | `--root-dir` | `COCOON_ROOT_DIR` | `/var/lib/cocoon` | Root directory for persistent data | | `--run-dir` | `COCOON_RUN_DIR` | `/var/lib/cocoon/run` | Runtime directory for sockets and PIDs | | `--log-dir` | `COCOON_LOG_DIR` | `/var/log/cocoon` | Log directory for VM and process logs | | `--log-level` | `COCOON_LOG_LEVEL` | `info` | Log level: debug, info, warn, error | | `--cni-conf-dir` | `COCOON_CNI_CONF_DIR` | `/etc/cni/net.d` | CNI plugin config directory | | `--cni-bin-dir` | `COCOON_CNI_BIN_DIR` | `/opt/cni/bin` | CNI plugin binary directory | -| `--dns` | `COCOON_DNS` | `8.8.8.8,1.1.1.1` | DNS servers for VMs (comma separated) | +| `--dns` | `COCOON_DNS` | `8.8.8.8,1.1.1.1` | DNS servers for VMs (comma or semicolon separated) | Config-file / env-only keys (no CLI flag): @@ -86,7 +86,7 @@ Config-file / env-only keys (no CLI flag): | ------------ | ------------------- | ------- | ---------------------------------------------------------------------- | | `pull_conns` | `COCOON_PULL_CONNS` | `8` | Concurrent HTTP Range connections per cloud-image download (`image pull`); raise for fat pipes, lower to be gentle on the registry; capped at 64 | | `cgroup_parent` | `COCOON_CGROUP_PARENT` | `cocoon.slice` | cgroup v2 slice under `/sys/fs/cgroup` holding the per-VM CPU scopes; GC removes any empty `vm-.scope` under it that none of its own VMs owns, so co-hosted installations need distinct slices, as with `net_scope`; see [CPU Isolation](vm.md#cpu-isolation-cgroup-v2) | -| `cgroup_cpus` | `COCOON_CGROUP_CPUS` | empty (all cores) | Host cpu list fencing the whole VM population (e.g. `0-14` reserves core 15 for the host); kernel cpu-list syntax | +| `cgroup_cpus` | `COCOON_CGROUP_CPUS` | empty (all cores) | Host cpu list fencing the whole VM population (e.g. `0-14` reserves core 15 for the host); ranges and commas, e.g. `0-3,8` | | `net_scope` | `COCOON_NET_SCOPE` | empty (legacy names) | Two alphanumerics keying this installation's host network families — bridge TAPs `-`, CNI netns `-` — so co-hosted installations never GC each other's; see [Host device namespaces](networking.md#host-device-namespaces) | | `ch_binary` | `COCOON_CH_BINARY` | `cloud-hypervisor` | cloud-hypervisor executable, path or `$PATH` name | | `fc_binary` | `COCOON_FC_BINARY` | `firecracker` | firecracker executable, path or `$PATH` name | @@ -142,7 +142,7 @@ Applies to `cocoon vm create`, `cocoon vm run`, and `cocoon vm debug`: | `--cpu-quota-us` | `0` (= vCPU count × period) | cgroup `cpu.max` quota in µs per period — caps the long-run average; add `--cpu-burst-us -1` for a strict per-period ceiling | | `--cpu-period-us` | `0` (= 100000) | cgroup `cpu.max` period in µs | | `--cpu-burst-us` | `0` (= quota) | cgroup `cpu.max.burst` credit in µs; `-1` = none; kernel requires burst ≤ quota | -| `--cpuset-cpus` | empty (anywhere in fence) | Pin the VM to specific host cpus (kernel cpu-list, e.g. `0-3`) or `auto` (the least-loaded last-level-cache domain at launch); non-work-conserving, explicit opt-in. Writable disk queue threads are pinned on every Cloud Hypervisor launch of a multi-vCPU VM, cpuset or not — see [CPU Isolation](vm.md#cpu-isolation-cgroup-v2) | +| `--cpuset-cpus` | empty (anywhere in fence) | Pin the VM to specific host cpus (ranges and commas, e.g. `0-3`) or `auto` (the least-loaded last-level-cache domain at launch); non-work-conserving, explicit opt-in. Writable disk queue threads are pinned on every Cloud Hypervisor launch of a multi-vCPU VM, cpuset or not — see [CPU Isolation](vm.md#cpu-isolation-cgroup-v2) | Every size flag (`--memory`, `--storage`, `--data-disk size=`, `gc --snapshot-size`) accepts the Docker spelling (`20G`, `20GiB`) and the Kubernetes spelling (`20Gi`); all units are binary, so `20G` is 20 GiB. @@ -277,7 +277,7 @@ All four outcomes exit 0; a non-zero exit is a real failure (store I/O, an orpha ### Reseed Flags -Applies to `cocoon vm reseed` (forces fresh guest entropy over vsock; runs automatically best-effort after clone/restore, invoke manually to retry a failed auto-reseed, which reports to `cocoon vm logs`): +Applies to `cocoon vm reseed` (forces fresh guest entropy over vsock; runs automatically best-effort after clone/restore, skipped for Windows guests and VMs without a vsock; invoke manually to retry a failed auto-reseed, which reports to `cocoon vm logs`): | Flag | Default | Description | |------|---------|-------------| @@ -324,7 +324,7 @@ the printed command carries no `queue_affinity` whatever `--cpuset-cpus` says. | ---------------- | -------- | ------------------------------------------------- | | `--escape-char` | `^]` | Escape character (single char or `^X` caret notation) | -For a running VM, `cocoon vm inspect` reports the console resolved at boot (start, clone, restore) as `console_path`: the `console.sock` UDS (UEFI serial, Firecracker relay) or the Cloud Hypervisor-allocated PTY (`/dev/pts/N`, direct-boot OCI). External supervisors can read the console from there without opening the owner-only API socket. A direct-boot VM booted by an older cocoon reports it from its next start. It also reports the resolved placement as `cpuset` and `queue_cpus`, both absent once the VM is not running. +For a running VM, `cocoon vm inspect` reports the console resolved at boot (start, clone, restore) as `console_path`: the `console.sock` UDS (UEFI serial, Firecracker relay) or the Cloud Hypervisor-allocated PTY (`/dev/pts/N`, direct-boot OCI). External supervisors can read the console from there without opening the owner-only API socket. A direct-boot VM booted by an older cocoon reports it from its next start. The Firecracker relay serves one console client at a time. It also reports the resolved placement as `cpuset` and `queue_cpus`, both absent once the VM is stopped. ### Exec Flags diff --git a/docs/daemon.md b/docs/daemon.md index fdb8bf2f..6789e1ff 100644 --- a/docs/daemon.md +++ b/docs/daemon.md @@ -93,9 +93,9 @@ record once and applies the same idempotent rules: | `creating` | any | ownerless only if the ops lock is free; then run the stale-create cleanup at once, freeing the name | | `running` | live | watch that exact process generation | | committed, not `running` | live | repair to `running` under the lock, then watch it | -| any | dead | close the interval, mark `unexpected-exit`, quiesce the network | +| `running`, or an open compute interval | dead | close the interval, mark `unexpected-exit`, quiesce the network | | `quiesce_pending` | dead | retry the quiesce | -| unfinished delete tombstone | any | finish the delete — supervision starts deletes of its own, so it resumes them | +| unfinished delete tombstone | any | finish a delete whose tombstone reached the deleting phase, roll back one still leased — supervision starts deletes of its own, so it resumes them | Exits are detected with `pidfd` + `epoll`, so the normal path costs no polling; the ticker is the correctness floor for anything the watcher missed, including @@ -138,9 +138,10 @@ the daemon does not claim an exit time it cannot know. ## Read-only API -The daemon serves HTTP/JSON over `/cocoond.sock` (`0660`, in a `0750` -directory — filesystem permissions are the authorization boundary). -`--api-socket` moves the socket, `--api-socket-mode` sets its permission bits; +The daemon serves HTTP/JSON over `/cocoond.sock` (`0660`, in a directory +the daemon creates `0750` when absent — filesystem permissions are the authorization boundary). +`--api-socket` moves the socket, `--api-socket-mode` sets its permission bits +(octal with a leading `0`: `660` without it is read as decimal); tighten the mode rather than widen it, since nothing else authenticates. ```bash @@ -155,8 +156,8 @@ curl --unix-socket /var/lib/cocoon/run/cocoond.sock -N http://localhost/v1/event nobody can converge must not put a working daemon into a restart loop. - `GET /v1/vms` — every supervised VM: persisted state, transition triple, current liveness, watched pid. -- `GET /v1/events` — SSE. Opens with a `sync` snapshot, then streams `ADDED`, - `MODIFIED` and `DELETED` changes. +- `GET /v1/events` — SSE. Opens with a `sync` snapshot, then streams `change` + events whose `type` is `ADDED`, `MODIFIED` or `DELETED`. The stream is a hint, not a log: it replays nothing and a slow reader loses events. After a reconnect, compare each VM's `transition_generation` against the diff --git a/docs/devices.md b/docs/devices.md index b603e0f4..8dbae0f7 100644 --- a/docs/devices.md +++ b/docs/devices.md @@ -38,7 +38,7 @@ Flags: | Flag | Default | Description | |------|---------|-------------| | `--socket` | required | Absolute path to the virtiofsd unix socket | -| `--tag` | required | Guest mount tag (also detach key) | +| `--tag` | required | Guest mount tag (also detach key; `^[a-zA-Z0-9][a-zA-Z0-9_-]{0,35}$`) | | `--num-queues` | `1` | Request queues | | `--queue-size` | `1024` | Queue depth | @@ -113,13 +113,13 @@ cocoon vm device detach my-vm --id mygpu | Flag | Applies to | Description | | ---- | ---------- | ----------- | | `--pci` | `attach` (required) | Short BDF (`01:00.0`), full BDF (`0000:01:00.0`) or a sysfs path under `/sys/bus/pci/devices/` | -| `--id` | `attach` (optional), `detach` (required) | Device id; Cloud Hypervisor generates one when `attach` omits it, and `detach` takes the id `attach` returned. Must not start with `cocoon-` | +| `--id` | `attach` (optional), `detach` (required) | Device id; Cloud Hypervisor generates one when `attach` omits it, and `detach` takes the id `attach` returned. `^[A-Za-z0-9][A-Za-z0-9_.-]{0,63}$`, must not start with `cocoon-` | `cocoon vm inspect VM` includes an `attached_devices` field for running VMs that surfaces every attached vhost-user-fs share, VFIO device, and hot-attached disk, read live from CH `vm.info`. The field is omitted for stopped VMs and whenever the live attach set is empty. ## Firecracker (`--pci`) -Firecracker VMs created with `--pci` accept `vm disk attach`/`detach` (vhost-user-fs and VFIO stay Cloud Hypervisor only); MMIO VMs are rejected. Cocoon gives each `--pci` VM 8 PCIe root ports. A Firecracker build that places hot-plugged devices behind them notifies the guest, which then finds and releases the devices itself. An older build plugs devices onto the root bus and does not notify the guest, so cocoon prints the guest-side step after each call, and `--output json` carries it as `hints`: +Firecracker VMs created with `--pci` accept `vm disk attach`/`detach` (vhost-user-fs and VFIO stay Cloud Hypervisor only); MMIO VMs are rejected. Cocoon gives each `--pci` VM 8 PCIe root ports. A Firecracker build that places hot-plugged devices behind them notifies the guest, which then finds and releases the devices itself. An older build plugs devices onto the root bus and does not notify the guest. cocoon cannot tell the builds apart, so it prints the guest-side step after each call (a root-port build needs none), and `--output json` carries it as `hints`: ```bash cocoon vm disk attach my-vm --path /vols/scratch.raw --name scratch diff --git a/docs/gc.md b/docs/gc.md index e3a361d0..d8b9a212 100644 --- a/docs/gc.md +++ b/docs/gc.md @@ -38,7 +38,7 @@ Reasons: - **snapshot**: `orphan` (dataDir without DB record), `stale-pending` (a dead save's pending record — its build lease is free), `missing-dir` (a record whose dataDir is gone), `orphan-lease` (a lease file whose snapshot has no record), `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), `stale-clone-lock` (a clone lock past the grace whose holder died) - **images (oci, cloudimg)**: `unreferenced` -- **cni**: `orphan` (netns in this installation's `net_scope` family without active VM) +- **cni**: `orphan` (netns in this installation's `net_scope` family, or CNI records, without an active VM) - **bridge**: `orphan-tap` (TAP in this installation's `net_scope` family without active VM) - **cgroup**: `orphan-scope` (empty `vm-.scope` under `cgroup_parent` that no VM record owns; scopes with live members are left alone) - **vmlock**: `orphan-lease` (lease file for a VM no backend knows; the ops lock unlinks on release, so `locks/vm/` is normally empty even with VMs running) diff --git a/docs/index.md b/docs/index.md index 58918597..ff247b80 100644 --- a/docs/index.md +++ b/docs/index.md @@ -60,7 +60,7 @@ cocoon CLI ──► images: OCI (EROFS layers, direct boot) | cloudimg (qcow2, - **Mergeable memory** — opt-in `--mergeable` (Cloud Hypervisor only) madvises guest memory `MADV_MERGEABLE` so host KSM can dedup identical pages across VMs; excludes `--hugepages`/`--shared-memory` - **Memory balloon** — 25% of memory returned via virtio-balloon (deflate-on-OOM, free-page reporting); `--no-balloon` opts out at create and the setting follows the snapshot - **Graceful shutdown** — ACPI power-button for every Cloud Hypervisor VM with configurable timeout, fallback to SIGTERM → SIGKILL -- **Interactive console** — `cocoon vm console` with bidirectional PTY relay, an SSH-style escape sequence (`^]` then `.` disconnects; `--escape-char` changes the first key), SIGWINCH propagation +- **Interactive console** — `cocoon vm console` with bidirectional PTY relay, an SSH-style escape sequence (`^]` then `.` disconnects; `--escape-char` changes the first key), SIGWINCH propagation on the Cloud Hypervisor direct-boot PTY console - **Snapshot & clone** — `cocoon snapshot save` captures a running VM's full state (memory, disks, config); `cocoon vm clone` restores it as a new VM with fresh network and identity; guest CPU, memory and storage inherit verbatim from the snapshot and so does the NIC count unless `--nics` overrides it, while host-side cgroup CPU policy comes from clone flags (see [CPU Isolation](vm.md#cpu-isolation-cgroup-v2)) - **Snapshot export & import** — `cocoon snapshot export` packages a snapshot into a portable `.tar` archive (`.tar.gz` with `--gzip`, sparse-aware pax headers); `cocoon snapshot import` restores it on another host or cluster; supports piping via stdout/stdin for direct host-to-host transfer; `--to-dir` writes a directory form (with `snapshot.json` envelope) for NFS / rsync-friendly handoff - **Clone / restore from a directory** — `cocoon vm clone --from-dir DIR` and `cocoon vm restore --from-dir DIR` consume any directory containing a `snapshot.json` envelope without first registering the snapshot in the local DB; the dir is treated as read-only so multi-VM golden-image use cases work without copying diff --git a/docs/install.md b/docs/install.md index 26937b22..17208c08 100644 --- a/docs/install.md +++ b/docs/install.md @@ -15,6 +15,7 @@ Requirements, install paths, the doctor script, and a first VM. - UEFI firmware (`CLOUDHV.fd`, for cloud images and, on aarch64, OCI images; not needed with `--fc`); on x86_64 `cocoon-check --upgrade` installs the [firmware fork](https://github.com/cocoonstack/rust-hypervisor-firmware/tree/dev) `dev` build (EFI ResetSystem for ACPI power-button shutdown and the IA32_FEATURE_CONTROL/VMXON lock, both needed by Windows guests — see [known issues](known-issues.md)); on aarch64 it installs the [EDK2](https://github.com/cloud-hypervisor/edk2/releases) `CLOUDHV_EFI.fd` build `ch-97eeb7b09`, which loads the OCI kernel over fw_cfg (see [Images](images.md#arm64-boot)) - CNI plugins (`bridge`, `host-local`, `loopback`) - `mkfs.ext4` (from e2fsprogs, for the per-VM COW disk) +- `root_dir` on a local filesystem: the default sqlite metadata engine refuses NFS, CIFS/SMB and FUSE, and `cocoon-check` reports them as FAIL - Go 1.27+ (build only) ## Installation @@ -25,10 +26,10 @@ Download pre-built binaries from [GitHub Releases](https://github.com/cocoonstac ```bash # Linux amd64 -curl -fsSL -o cocoon.tar.gz https://github.com/cocoonstack/cocoon/releases/download/v0.7.6/cocoon_0.7.6_Linux_x86_64.tar.gz +curl -fsSL -o cocoon.tar.gz https://github.com/cocoonstack/cocoon/releases/download/v0.7.7/cocoon_0.7.7_Linux_x86_64.tar.gz # Linux arm64 -curl -fsSL -o cocoon.tar.gz https://github.com/cocoonstack/cocoon/releases/download/v0.7.6/cocoon_0.7.6_Linux_arm64.tar.gz +curl -fsSL -o cocoon.tar.gz https://github.com/cocoonstack/cocoon/releases/download/v0.7.7/cocoon_0.7.7_Linux_arm64.tar.gz tar -xzf cocoon.tar.gz install -m 0755 cocoon /usr/local/bin/ @@ -55,7 +56,7 @@ Cocoon ships a diagnostic script that checks your environment and can auto-insta ```bash # Get script -curl -fsSL -o cocoon-check https://raw.githubusercontent.com/cocoonstack/cocoon/refs/tags/v0.7.6/doctor/check.sh +curl -fsSL -o cocoon-check https://raw.githubusercontent.com/cocoonstack/cocoon/refs/tags/v0.7.7/doctor/check.sh install -m 0755 cocoon-check /usr/local/bin/ # Check only — reports PASS/FAIL for each requirement diff --git a/docs/known-issues.md b/docs/known-issues.md index 57dad6f2..1e47d787 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -9,7 +9,7 @@ Limitations, workarounds, and upstream tracking. Fixed entries are kept for oper - **Cloud Hypervisor** hot-swaps the snapshot's NICs while the VM is paused, so the clone comes up on a new interface with a fresh MAC (`ens3` → `ens8` on cloudimg, `eth0` → `eth1` on OCI). The snapshot's MAC-matched configuration then matches nothing and the guest has **no IPv4 address at all**: it cannot reach the network, and it cannot collide with the source VM either. - **Firecracker** keeps the source VM's guest MAC in the restored vmstate (see [Firecracker clone guest MAC address](#firecracker-clone-guest-mac-address)), so the per-MAC configuration still matches and the guest wakes up holding **the source VM's IP**. -**Consequence on Firecracker**: if the source VM is still running, both VMs advertise the same IP with different MACs, the upstream gateway flaps between them, and both see **intermittent connectivity loss** until the clone is reconfigured. Nothing in the data path prevents this — it is L2 end to end (TC ingress redirect + bridge), no component checks the guest's source IP against the allocated one, and bridge CNI does not enforce IP ↔ veth binding (`host-local` IPAM only tracks allocations in its control-plane state files, not in data-plane rules). +**Consequence on Firecracker**: if the source VM is still running, both VMs advertise the same IP from the same guest MAC on two bridge ports, the bridge forwarding table flaps between them, and both see **intermittent connectivity loss** until the clone is reconfigured. Nothing in the data path prevents this — it is L2 end to end (TC ingress redirect + bridge), no component checks the guest's source IP against the allocated one, and bridge CNI does not enforce IP ↔ veth binding (`host-local` IPAM only tracks allocations in its control-plane state files, not in data-plane rules). **Mitigation**: run the post-clone guest setup commands printed by `cocoon vm clone` (see [Post-Clone Guest Setup](snapshots.md#post-clone-guest-setup)). For cloudimg VMs they re-run `cloud-init`; for OCI VMs they replace the generated `/etc/systemd/network/10-*.network` files with per-MAC units for the new IP and restart `systemd-networkd`; on Firecracker they set the new MAC first. The clone answers on its allocated IP a few seconds after they run. @@ -67,7 +67,7 @@ On x86_64, cocoon uses [rust-hypervisor-firmware](https://github.com/cloud-hyper - Ubuntu 22.04 (Jammy) -This is an upstream issue tracked in [rust-hypervisor-firmware#333](https://github.com/cloud-hypervisor/rust-hypervisor-firmware/issues/333) and [cloud-hypervisor#7356](https://github.com/cloud-hypervisor/cloud-hypervisor/issues/7356). As a workaround, use **OCI VM images** for Ubuntu 24.04 — OCI images use direct kernel boot and are not affected. +This is an upstream issue tracked in [rust-hypervisor-firmware#333](https://github.com/cloud-hypervisor/rust-hypervisor-firmware/issues/333); [cloud-hypervisor#7356](https://github.com/cloud-hypervisor/cloud-hypervisor/issues/7356) was closed without a fix. As a workaround, use **OCI VM images** for Ubuntu 24.04 — OCI images use direct kernel boot and are not affected. ## Data disk fsck sends an existing VM to the emergency shell @@ -203,7 +203,7 @@ A cloudimg VM snapshotted after a stop/start carries an unreferenced ~1 MB spars ## Firecracker clone guest MAC address -Firecracker does not support overriding the guest MAC address during snapshot/load. Cloned FC VMs retain the source VM's guest MAC (baked into the vmstate binary). In Cocoon's TC redirect architecture, each VM runs in an isolated network namespace, so MAC identity is not visible to other VMs or the host bridge — **no MAC conflict occurs in practice**. +Firecracker does not support overriding the guest MAC address during snapshot/load. Cloned FC VMs retain the source VM's guest MAC (baked into the vmstate binary). The TC redirect passes guest frames to the bridge unchanged, so while the source runs both VMs send from that MAC (see [clone networking](#clone-networking-needs-in-guest-reconfiguration)); the post-clone guest setup sets the clone's own MAC. On CNI plugins with strict per-veth MAC enforcement (Cilium eBPF, Calico eBPF), the guest MAC vs veth MAC mismatch could theoretically cause packet drops. This has not been observed in testing with the standard bridge CNI. @@ -217,15 +217,15 @@ On CNI plugins with strict per-veth MAC enforcement (Cilium eBPF, Calico eBPF), ## A killed capture leaves the guest paused -`snapshot save` and `vm hibernate` pause the guest for the capture. A canceled or timed-out capture attempts to resume the guest with a separate 10 minute timeout. If the cocoon process dies or that resume fails, the guest can remain paused: the VM record reads `running`, the guest is frozen, and `vm exec` hangs. SIGKILL does this. SIGPIPE does too: when the process that reads cocoon's stderr exits, cocoon dies on its next log line, and for a hibernate that line comes after the snapshot persists and before the VMM stops. +`snapshot save`, `vm hibernate` and `vm migrate capture` pause the guest for the capture. A canceled or timed-out capture attempts to resume the guest with a separate 10 minute timeout. If the cocoon process dies or that resume fails, the guest can remain paused: the VM record reads `running`, the guest is frozen, and `vm exec` hangs. SIGKILL does this. SIGPIPE does too: when the process that reads cocoon's stderr exits, cocoon dies on its next log line, and for a hibernate that line comes after the snapshot persists and before the VMM stops. `cocoon vm start VM` resumes it. The hypervisor API does not answer during a dump, so until the dump ends the command fails after 30 s; retry it. `vm inspect` also waits on that API, up to 30 s per device list, then prints without the attached devices; `vm list` does not call it. -A killed capture also leaves its files. A `snapshot-*` directory under the VM's run dir stays until `vm rm` or a `gc` 24 hours later. A hibernate killed while it persists leaves a hidden pending snapshot that keeps its `--name` taken until the next `gc`. A hibernate killed after it persists leaves a listed snapshot that keeps its `--name` until `snapshot rm`. +A killed capture also leaves its files. A `snapshot-*` directory under the VM's run dir stays until `vm rm` or a `gc` 24 hours later. A hibernate or `snapshot save` killed while it persists leaves a hidden pending snapshot that keeps its `--name` taken until the next `gc` or `snapshot rm `. A hibernate killed after it persists leaves a listed snapshot that keeps its `--name` until `snapshot rm`. ## Windows: a capture right after a clone can strand the next clone's NIC -A clone's eject ack does not mean its replacement NIC has started: netkvm finishes starting 0.2-1.8 seconds later. A snapshot taken in between yields a clone whose own eject never acks. Its next capture then waits the full 30 seconds and warns, and a clone of that snapshot has no new NIC while `vm clone` reports success. On Windows, a capture that finds an eject pending therefore also waits, inside the same 30 seconds, until every NIC has sent a frame, and it re-sends `vm.remove-device` every 5 seconds for an eject the guest dropped. A capture started in the short window after the ack and before the NIC starts is not covered: wait a few seconds after cloning a Windows VM before capturing it. +A clone's eject ack does not mean its replacement NIC has started: netkvm finishes starting 0.2-1.8 seconds later. A snapshot taken in between yields a clone whose own eject never acks. Its next capture then waits the full 30 seconds and warns, and a clone of that snapshot has no new NIC while `vm clone` reports success. A capture that finds an eject pending re-sends `vm.remove-device` every 5 seconds for an eject the guest dropped; on Windows it also waits, inside the same 30 seconds, until every NIC has sent a frame. A capture started in the short window after the ack and before the NIC starts is not covered: wait a few seconds after cloning a Windows VM before capturing it. A NIC in Error code 10 (netkvm event 5005) at capture strands the eject the same way, with any delay. It appears when Windows re-enables several NICs at once: it moves a NIC's BAR into a range Cloud Hypervisor still assigns to another disabled NIC, Cloud Hypervisor refuses and logs `Failed moving device BAR ... keeping old BAR`, and the driver starts on resources that do not match. Disable and re-enable NICs one at a time; the clone hint and the image's `CocoonNicAutoHeal` task do. diff --git a/docs/networking.md b/docs/networking.md index cef57ef5..c83c81a2 100644 --- a/docs/networking.md +++ b/docs/networking.md @@ -69,10 +69,10 @@ cocoon vm net my-vm --nics 2 cocoon vm net my-vm --nics 1 ``` -On NIC removal, cocoon waits for the guest to ACK B0EJ (CH polls `device_tree` until the device disappears) before tearing down the host TAP / veth / CNI lease. If the guest never ACKs within the 30s eject timeout, the command fails and leaves the cocoon record + host plumbing intact so the operator can quiesce the guest (driver unbind, NetworkManager removal, Windows NDIS halt) and retry. +On NIC removal, cocoon waits for the guest to ACK B0EJ (CH polls `device_tree` until the device disappears) before tearing down the host TAP / veth / CNI lease. If the guest never ACKs within the 30s eject timeout, the command fails and leaves the cocoon record + host plumbing intact so the operator can quiesce the guest (driver unbind, NetworkManager removal, Windows NDIS halt) and retry; on Cloud Hypervisor the retry asks the guest to eject again, then reclaims the host side. -A removal or `vm rm` interrupted mid-teardown (crash, SIGKILL) leaves a recovery marker on the VM. The next `vm rm` or `vm net` on that VM first finishes the interrupted teardown exactly as recorded; rerunning the same command resumes it and completes, while a different intent is refused with a conflict error instead of guessed at — rerun the command in that case. `vm start` and `vm restore` finish it too, then rebuild every NIC the VM record still lists, so a removal interrupted before its record update comes back with the NIC; rerun `vm net` to remove it. A hot removal killed after the device left the VMM and before the record update has no marker yet: `vm net` back to the old count reports success one NIC short, and on Cloud Hypervisor `snapshot save` refuses until the record matches. Rerun the removal, or stop and start the VM. +A removal or `vm rm` interrupted mid-teardown (crash, SIGKILL) leaves a recovery marker on the VM. The next `vm rm` or `vm net` on that VM first finishes the interrupted teardown exactly as recorded; rerunning the same command resumes it and completes, while a different intent is refused with a conflict error instead of guessed at — rerun the command in that case. `vm start` and `vm restore` finish an interrupted removal too, then rebuild every NIC the VM record still lists, so a removal interrupted before its record update comes back with the NIC; rerun `vm net` to remove it. On an interrupted `vm rm` they finish the delete instead. A hot removal killed after the device left the VMM and before the record update has no marker yet: `vm net` back to the old count reports success one NIC short, and `snapshot save` refuses until the record matches. Rerun the removal, or stop and start the VM. -A Firecracker build that plugs devices onto the root bus adds and drops them without telling the guest, and a build with PCIe root ports needs no step. On the root bus, after an add the guest runs `echo 1 > /sys/bus/pci/rescan`, after a remove it drops the stale node with `echo 1 > /sys/class/net/ethN/device/../remove`. `cocoon vm net` prints both (and returns them as `hints` with `--output json`); down the NIC inside the guest before reducing the count. MMIO Firecracker VMs are rejected. +A Firecracker build that plugs devices onto the root bus adds and drops them without telling the guest, and a build with PCIe root ports needs no step. On the root bus, after an add the guest runs `echo 1 > /sys/bus/pci/rescan`, after a remove it drops the stale node with `echo 1 > /sys/class/net/ethN/device/../remove`. cocoon cannot tell the builds apart, so `cocoon vm net` prints both after every Firecracker resize (and returns them as `hints` with `--output json`); down the NIC inside the guest before reducing the count. MMIO Firecracker VMs are rejected. Resize from zero is supported: under CNI, `--nics 0` still provisions a per-VM netns at boot, provided a conflist loads (CH lives in it from the start), so a later `cocoon vm net --nics N` hot-plugs into the same namespace. Bridge mode keeps CH in the host netns regardless of NIC count, so 0→N adds TAPs onto the configured bridge. The named path is created when the VM is created and rebuilt by `vm start`/`vm restore` only if it has gone missing (host reboot, manual `ip netns del`, or an entry left in its place that is not a mounted netns); if it is removed while the VM runs, see [known issues](known-issues.md#named-netns-path-removed-while-a-vm-runs). diff --git a/docs/snapshots.md b/docs/snapshots.md index dba3d9d2..69b899d7 100644 --- a/docs/snapshots.md +++ b/docs/snapshots.md @@ -148,7 +148,7 @@ cocoon vm hibernate my-vm --name nap cocoon vm restore my-vm nap ``` -Pause, capture, persist, and VMM termination share one pause window: the snapshot point and the stop coincide (nothing the guest does can be lost in between), and the VMM dies only after the snapshot is durably saved — if saving fails (disk full, snapshot DB error), the VM simply resumes running and the command fails. Restore of the hibernated VM preserves machine identity (entropy-only reseed): running processes, sessions, and tmpfs contents continue where they stopped. +Pause, capture, persist, and VMM termination share one pause window: the snapshot point and the stop coincide (nothing the guest does can be lost in between), and the VMM dies only after the snapshot is durably saved — if saving fails (disk full, snapshot DB error), the VM simply resumes running and the command fails. Restore of the hibernated VM preserves machine identity (entropy-only reseed): running processes, sessions, and tmpfs contents continue where they stopped. The hibernate snapshot outlives the restore and keeps its `--name` and disk space until `snapshot rm`; `vm start` on a hibernated VM cold-boots from disk and leaves the snapshot unused. A canceled or timed-out `vm hibernate` or `snapshot save` waits for the hypervisor to end its memory dump, up to 10 minutes, then resumes the guest, so SIGTERM can take that long to end the command. SIGKILL skips the resume; see [known issues](known-issues.md#a-killed-capture-leaves-the-guest-paused). @@ -184,7 +184,7 @@ Rules for the caller: - Copy every file a round lists, in the listed round order. `state.json` and `config.json` are replaced each round. A `.ref` file never leaves the source. A short or missing disk delta fails the receive. - A VM has one series. A new baseline ends the previous one and deletes its directory; the directory of a series nobody continues stays until `gc` removes it after 24 hours. A delta with an old series ID fails and leaves the guest alone. -- A failed round ends the series, resumes the guest and deletes its directory; start again with a new baseline. A round after a killed capture fails the same way. `snapshot save`, removing a NIC, a device detach, a reboot or a stop ends the series too, and the next delta fails. +- A failed round ends the series, resumes the guest and deletes its directory; start again with a new baseline. A capture killed mid-round makes the next round fail the same way; a gap in `round` means a capture died after it finished, so copy every non-`.ref` file of the directory or start again. `snapshot save`, removing a NIC, a device detach, a reboot or a stop ends the series too, and the next delta fails. - A VM cloned or received seconds ago may still eject its old NICs. A capture waits up to 30 seconds for that, then goes on with a warning; if an eject lands during a series, the next delta fails, so start again. - Pull the image on the target before `--final`. A receive that has to pull does it during the pause. - The receive uses the source's CNI network name. When the target has no conflist of that name, pass `--network`. diff --git a/docs/vm.md b/docs/vm.md index c12acbe2..d743596c 100644 --- a/docs/vm.md +++ b/docs/vm.md @@ -10,7 +10,7 @@ States, shutdown behavior, cloud-init first boot, data disks, performance tuning | `created` | Registered, hypervisor process not yet started | | `running` | Hypervisor process alive, guest is up | | `stopped` | Hypervisor process exited cleanly | -| `error` | Start, stop, or restore failed — recover with `vm restore`; a failed restore also leaves an interrupted-restore marker (and quarantines the record on a partial merge), so `vm start` is refused until a later restore succeeds or `vm rm` deletes it. Not sticky once the VM has been up: the daemon and the next `vm start` converge a dead VMM to `stopped` / `unexpected-exit`, and a VMM that is still alive is repaired to `running`; only a failed first start leaves `error` in place | +| `error` | Start, stop, or restore failed — recover with `vm restore`; a restore that fails after stopping the VM also leaves an interrupted-restore marker (and quarantines the record when the run dir is half-rewritten), so `vm start` is refused until a later restore succeeds or `vm rm` deletes it. A failed stop or hibernate is not sticky: the daemon and the next `vm start` converge a dead VMM to `stopped` / `unexpected-exit`, and a VMM that is still alive is repaired to `running`; a failed start leaves `error` in place until the next successful `vm start` | | `stopped (stale)` | Rendered, never persisted: the record reads `running` but the VMM is gone; `vm list -o json`, `vm status --event --format json` and `vm inspect` report `"state": "stopped"` with `"stale": true` | ### Shutdown Behavior @@ -104,7 +104,7 @@ With `cgroup_cpus=0-14`, the reserved core 15 has no VM competition and acts as Firecracker attaches virtio devices over MMIO by default (and adds `pci=off` to the guest command line itself). `--pci` on `vm run`/`vm create` starts Firecracker with `--enable-pci`, so the guest enumerates virtio-pci devices instead; this is the prerequisite for Firecracker device hot-plug (Developer Preview upstream). The transport is fixed for the VM's lifetime and travels with its snapshots: clones and restores relaunch on the transport the snapshot was taken with, and a snapshot cannot move between transports. The guest kernel needs `CONFIG_PCI`, `CONFIG_VIRTIO_PCI` and `CONFIG_ACPI`; distro kernels have both transports, while the sandbox guest kernel is built without `CONFIG_VIRTIO_MMIO`, so sandbox images can never use the MMIO transport (booting them on Firecracker at all is not validated yet). Cloud Hypervisor is always virtio-pci, so `--pci` is rejected there. -With `--pci`, `vm net`, `vm disk attach`/`detach`, clone `--nics` and clone `--data-disk` work as on Cloud Hypervisor; on a Firecracker build that plugs devices onto the root bus cocoon prints the guest-side rescan/remove step after each change (see [Firecracker `--pci`](devices.md#firecracker---pci)). Unmount or down a device inside the guest before detaching it. Hot-attached disks show up as `/dev/vdX` (Firecracker has no virtio-blk serial) and, as on Cloud Hypervisor, block snapshot and hibernate until detached. +With `--pci`, `vm net`, `vm disk attach`/`detach`, clone `--nics` and clone `--data-disk` work as on Cloud Hypervisor; cocoon cannot tell a Firecracker build that plugs devices onto the root bus, so it prints the guest-side rescan/remove step after each change (see [Firecracker `--pci`](devices.md#firecracker---pci)). Unmount or down a device inside the guest before detaching it. Hot-attached disks show up as `/dev/vdX` (Firecracker has no virtio-blk serial) and, as on Cloud Hypervisor, block snapshot and hibernate until detached. ## Cloud-init & First Boot diff --git a/doctor/vm-inspect.sh b/doctor/vm-inspect.sh index c0ebe4c3..4f8dc733 100755 --- a/doctor/vm-inspect.sh +++ b/doctor/vm-inspect.sh @@ -327,8 +327,8 @@ else # Check tap queue count via /sys TAP_QUEUES="" if ip netns exec "$NETNS_NAME" test -d "/sys/class/net/${N_TAP}/queues" 2>/dev/null; then - TX_Q=$(ip netns exec "$NETNS_NAME" ls "/sys/class/net/${N_TAP}/queues/" 2>/dev/null | grep -c '^tx-' || echo 0) - RX_Q=$(ip netns exec "$NETNS_NAME" ls "/sys/class/net/${N_TAP}/queues/" 2>/dev/null | grep -c '^rx-' || echo 0) + TX_Q=$(ip netns exec "$NETNS_NAME" ls "/sys/class/net/${N_TAP}/queues/" 2>/dev/null | grep -c '^tx-' || true) + RX_Q=$(ip netns exec "$NETNS_NAME" ls "/sys/class/net/${N_TAP}/queues/" 2>/dev/null | grep -c '^rx-' || true) TAP_QUEUES="tx=$TX_Q rx=$RX_Q" kv " Queues" "$TAP_QUEUES" diff --git a/extend/disk/disk.go b/extend/disk/disk.go index 8c812e4f..1adbf87b 100644 --- a/extend/disk/disk.go +++ b/extend/disk/disk.go @@ -28,7 +28,7 @@ type Spec struct { DirectIO *bool } -// Normalize enforces required fields; Name doubles as the guest serial (/dev/disk/by-id/virtio-) and the detach key. +// Normalize enforces required fields. func (s *Spec) Normalize() error { if s.Path == "" { return fmt.Errorf("path is required") @@ -84,7 +84,7 @@ func ValidateName(name string) error { 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 fmt.Errorf("name %q invalid: must match ^[a-z][a-z0-9_-]{0,19}$ without the cocoon- prefix", name) } return nil } diff --git a/hypervisor/backend.go b/hypervisor/backend.go index 8a8fae61..e5fc4ddc 100644 --- a/hypervisor/backend.go +++ b/hypervisor/backend.go @@ -96,6 +96,10 @@ type AfterExtractHook func(ctx context.Context, vmID string, vmCfg *types.VMConf type RunDirHook func(runDir string) error +type PIDHook func(pid int) error + +type RecordHook func(rec *VMRecord) error + type DirHook func(rec *VMRecord, dir string) error type ProcessHook func(ctx context.Context, rec *VMRecord, sockPath string, pid int) error diff --git a/hypervisor/cloudhypervisor/cloudhypervisor.go b/hypervisor/cloudhypervisor/cloudhypervisor.go index eb95610b..1ac9b879 100644 --- a/hypervisor/cloudhypervisor/cloudhypervisor.go +++ b/hypervisor/cloudhypervisor/cloudhypervisor.go @@ -3,7 +3,6 @@ package cloudhypervisor import ( "context" - "fmt" "github.com/cocoonstack/cocoon/config" "github.com/cocoonstack/cocoon/extend/disk" @@ -34,9 +33,6 @@ type CloudHypervisor struct { // New creates a CloudHypervisor backend; a nil rec falls back to NopRecorder. func New(conf *config.Config, rec metering.Recorder, store meta.Store) (*CloudHypervisor, error) { - if conf == nil { - return nil, fmt.Errorf("config is nil") - } cfg := NewConfig(conf) backend, err := hypervisor.NewBackend(typ, cfg, rec, store) if err != nil { diff --git a/hypervisor/cloudhypervisor/create.go b/hypervisor/cloudhypervisor/create.go index f35bccde..5703122a 100644 --- a/hypervisor/cloudhypervisor/create.go +++ b/hypervisor/cloudhypervisor/create.go @@ -2,6 +2,7 @@ package cloudhypervisor import ( "context" + "errors" "fmt" "os" @@ -100,7 +101,7 @@ func (ch *CloudHypervisor) generateCidata(vmID string, vmCfg *types.VMConfig, ne _ = f.Close() return fmt.Errorf("generate cidata: %w", err) } - return f.Close() + return errors.Join(f.Sync(), f.Close()) } func buildMountSpecs(configs []*types.StorageConfig) []metadata.MountSpec { diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index 20f3147a..0f4f2c35 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/hypervisor/cloudhypervisor/extend.go @@ -153,12 +153,13 @@ func (ch *CloudHypervisor) FsAttach(ctx context.Context, vmRef string, spec fs.S } func (ch *CloudHypervisor) FsDetach(ctx context.Context, vmRef, tag string) error { + id := fs.DeriveID(tag) return ch.detachWith(ctx, vmRef, func(info *chVMInfoResponse) (string, error) { - i := slices.IndexFunc(info.Config.Fs, func(f chFs) bool { return f.Tag == tag }) - if i < 0 { - return "", fmt.Errorf("fs tag %q not attached", tag) + _, present := info.DeviceTree[id] + if present || slices.ContainsFunc(info.Config.Fs, func(f chFs) bool { return f.ID == id }) { + return id, nil } - return info.Config.Fs[i].ID, nil + return "", fmt.Errorf("fs tag %q not attached", tag) }, nil) } @@ -247,7 +248,7 @@ func (ch *CloudHypervisor) attachWith(ctx context.Context, vmRef, endpoint strin return addDevice(ctx, hc, endpoint, makeBody(&rec)) } -func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID findIDFn, afterEject func(*hypervisor.VMRecord) error) error { +func (ch *CloudHypervisor) detachWith(ctx context.Context, vmRef string, findID findIDFn, afterEject hypervisor.RecordHook) 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 5f965fef..a0e520b3 100644 --- a/hypervisor/cloudhypervisor/netresize.go +++ b/hypervisor/cloudhypervisor/netresize.go @@ -5,6 +5,8 @@ import ( "fmt" "net/http" + "github.com/projecteru2/core/log" + "github.com/cocoonstack/cocoon/extend/netresize" "github.com/cocoonstack/cocoon/hypervisor" "github.com/cocoonstack/cocoon/network" @@ -36,7 +38,7 @@ func (o chNICOps) AddNIC(ctx context.Context, _ int, nc *types.NetworkConfig) (s } func (o chNICOps) RemoveNIC(ctx context.Context, id string) error { - if err := removeDeviceVM(ctx, o.hc, id); err != nil { + if err := removeDevicePending(ctx, o.hc, id); err != nil { return err } if err := waitDeviceEjected(ctx, o.hc, id); err != nil { @@ -51,10 +53,15 @@ func (ch *CloudHypervisor) NetResize(ctx context.Context, vmRef string, spec net if err := spec.Normalize(); err != nil { return netresize.Result{}, err } - hc, rec, _, unlock, err := ch.lockedDeviceOp(ctx, vmRef) + hc, rec, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) if err != nil { return netresize.Result{}, err } defer unlock() + for _, id := range pendingNICEjects(info) { + if rmErr := removeDevicePending(ctx, hc, id); rmErr != nil { + log.WithFunc("cloudhypervisor.NetResize").Warnf(ctx, "re-request eject of %s: %v", id, rmErr) + } + } return ch.NetResizeWith(ctx, rec.ID, &rec, chNICOps{hc: hc}, plumbing, spec.Target) } diff --git a/hypervisor/cloudhypervisor/utils.go b/hypervisor/cloudhypervisor/utils.go index 453663bc..60bb4e02 100644 --- a/hypervisor/cloudhypervisor/utils.go +++ b/hypervisor/cloudhypervisor/utils.go @@ -170,7 +170,6 @@ func addDiskVM(ctx context.Context, hc *http.Client, disk chDisk) error { return vmPutJSON(ctx, hc, "vm.add-disk", "add-disk request", disk, http.StatusOK, http.StatusNoContent) } -// removeDeviceVM is non-idempotent — a retry after a lost ACK would surface as "id not found". func removeDeviceVM(ctx context.Context, hc *http.Client, deviceID string) error { return vmPutJSON(ctx, hc, "vm.remove-device", "remove-device request", map[string]string{"id": deviceID}) } diff --git a/hypervisor/create.go b/hypervisor/create.go index 23c24e1e..466e9c5d 100644 --- a/hypervisor/create.go +++ b/hypervisor/create.go @@ -82,7 +82,7 @@ func (b *Backend) FinalizeCreate(ctx context.Context, id string, info *types.VM, RunDir: existing.RunDir, LogDir: existing.LogDir, } - // The subpackage building info cannot reach markTransition; without this both create states share generation zero. + // Without this the placeholder and the created record share generation zero. markTransition(rec, info.State, types.TransitionCreate, timeNow()) return t.Put(id, rec) }); err != nil { diff --git a/hypervisor/firecracker/clone.go b/hypervisor/firecracker/clone.go index 626a4139..c2d54e9f 100644 --- a/hypervisor/firecracker/clone.go +++ b/hypervisor/firecracker/clone.go @@ -28,21 +28,6 @@ const sourcePlaceholderProbeInterval = 2 * time.Millisecond var errBindSetup = errors.New("bind-mount clone redirect unavailable") -type bindRedirectPlan struct { - binds [][2]string - leases []*flock.SharedLease -} - -func (p *bindRedirectPlan) files() []*os.File { - files := make([]*os.File, 0, len(p.leases)) - for _, lease := range p.leases { - files = append(files, lease.File()) - } - return files -} - -func (p *bindRedirectPlan) close() { closeLeases(p.leases) } - // recordExistsFn reports whether the source VM's record survives; absent means the drive can be replaced with a placeholder. type recordExistsFn func(string) (bool, error) @@ -58,6 +43,21 @@ type cloneLaunch struct { dataDisks []*types.StorageConfig } +type bindRedirectPlan struct { + binds [][2]string + leases []*flock.SharedLease +} + +func (p *bindRedirectPlan) files() []*os.File { + files := make([]*os.File, 0, len(p.leases)) + for _, lease := range p.leases { + files = append(files, lease.File()) + } + return files +} + +func (p *bindRedirectPlan) close() { closeLeases(p.leases) } + func (fc *Firecracker) Clone(ctx context.Context, vmID string, vmCfg *types.VMConfig, net types.NetSetup, snapshotConfig *types.SnapshotConfig, snapshot io.Reader) (*types.VM, error) { if err := validateCloneNetworkMTUs(snapshotConfig.NICMTUs, net.NetworkConfigs); err != nil { return nil, err diff --git a/hypervisor/firecracker/extend.go b/hypervisor/firecracker/extend.go index dca30a09..d1390e96 100644 --- a/hypervisor/firecracker/extend.go +++ b/hypervisor/firecracker/extend.go @@ -49,7 +49,6 @@ func (o fcNICOps) RemoveNIC(ctx context.Context, id string) error { return nil } -// TAPQueues is one queue pair: Firecracker opens every TAP single-queue. func (fcNICOps) TAPQueues(int) int { return network.NetNumQueues(1) } func (fc *Firecracker) NetResize(ctx context.Context, vmRef string, spec netresize.Spec, plumbing netresize.Plumbing) (netresize.Result, error) { diff --git a/hypervisor/firecracker/extend_test.go b/hypervisor/firecracker/extend_test.go index 3217b692..c72e55f5 100644 --- a/hypervisor/firecracker/extend_test.go +++ b/hypervisor/firecracker/extend_test.go @@ -111,12 +111,17 @@ func TestDiskDetachRejectsAnInvalidNameBeforeLocking(t *testing.T) { } } -func TestRefuseHotAttachedDisks(t *testing.T) { +func TestRefuseUnrecordedDevices(t *testing.T) { + rec := &hypervisor.VMRecord{NetworkConfigs: []*types.NetworkConfig{{MAC: "aa:bb:cc:dd:ee:00"}}} srv := newFCStub(t, fcVMConfig{Drives: []fcDrive{{DriveID: "drive_0"}, {DriveID: hotDiskIDPrefix + "scratch", PathOnHost: "/vols/s.raw"}}}) - err := refuseHotAttachedDisks(t.Context(), srv.client) + err := refuseUnrecordedDevices(t.Context(), srv.client, rec) if err == nil || !strings.Contains(err.Error(), `"scratch"`) { t.Fatalf("err = %v, want the hot-attached disk named", err) } + srv = newFCStub(t, fcVMConfig{NetworkInterfaces: []fcNetworkInterface{{IfaceID: "eth0"}, {IfaceID: "eth1"}}}) + if err := refuseUnrecordedDevices(t.Context(), srv.client, rec); err == nil || !strings.Contains(err.Error(), "rerun vm net") { + t.Fatalf("err = %v, want a NIC added after the record refused", err) + } if got := hotAttachedDisks(&fcVMConfig{Drives: []fcDrive{{DriveID: "drive_1"}}}); got != nil { t.Fatalf("create-path drives listed as hot-attached: %+v", got) } diff --git a/hypervisor/firecracker/firecracker.go b/hypervisor/firecracker/firecracker.go index ca012057..166f2385 100644 --- a/hypervisor/firecracker/firecracker.go +++ b/hypervisor/firecracker/firecracker.go @@ -3,7 +3,6 @@ package firecracker import ( "context" - "fmt" "github.com/cocoonstack/cocoon/config" "github.com/cocoonstack/cocoon/extend/disk" @@ -27,9 +26,6 @@ type Firecracker struct { // New creates a Firecracker backend; a nil rec falls back to NopRecorder. func New(conf *config.Config, rec metering.Recorder, store meta.Store) (*Firecracker, error) { - if conf == nil { - return nil, fmt.Errorf("config is nil") - } cfg := NewConfig(conf) backend, err := hypervisor.NewBackend(typ, cfg, rec, store) if err != nil { diff --git a/hypervisor/firecracker/snapshot.go b/hypervisor/firecracker/snapshot.go index 6b3b2c2e..1cd06dbf 100644 --- a/hypervisor/firecracker/snapshot.go +++ b/hypervisor/firecracker/snapshot.go @@ -34,7 +34,7 @@ func (fc *Firecracker) snapshotSpec(ctx context.Context) hypervisor.SnapshotSpec return hypervisor.SnapshotSpec{ Pause: func(rec *hypervisor.VMRecord, hc *http.Client) error { if rec.Config.PCI { - if err := refuseHotAttachedDisks(ctx, hc); err != nil { + if err := refuseUnrecordedDevices(ctx, hc, rec); err != nil { return err } } @@ -73,8 +73,8 @@ func buildSnapshotMeta(rec *hypervisor.VMRecord, _ string) (*snapshot.SnapshotMe return meta, nil } -// A runtime-only disk has no sidecar entry, so the snapshot could never restore. -func refuseHotAttachedDisks(ctx context.Context, hc *http.Client) error { +// A runtime-only disk or NIC has no record entry, so the snapshot could never restore. +func refuseUnrecordedDevices(ctx context.Context, hc *http.Client, rec *hypervisor.VMRecord) error { cfg, err := getVMConfig(ctx, hc) if err != nil { return err @@ -82,5 +82,8 @@ func refuseHotAttachedDisks(ctx context.Context, hc *http.Client) error { if hot := hotAttachedDisks(cfg); len(hot) > 0 { return fmt.Errorf("hot-attached disk %q: %w", hot[0].Name, hypervisor.ErrHotAttached) } + if len(cfg.NetworkInterfaces) != len(rec.NetworkConfigs) { + return fmt.Errorf("vm %s has %d NICs but its record has %d; rerun vm net first", rec.ID, len(cfg.NetworkInterfaces), len(rec.NetworkConfigs)) + } return nil } diff --git a/hypervisor/firecracker/start.go b/hypervisor/firecracker/start.go index 6824b34e..7d13cc62 100644 --- a/hypervisor/firecracker/start.go +++ b/hypervisor/firecracker/start.go @@ -143,8 +143,9 @@ func (fc *Firecracker) launchProcessWithLeases(ctx context.Context, rec *hypervi fcLog := fc.LogFilePath(rec.LogDir) // FC opens its log O_WRONLY|O_APPEND without O_CREATE, so cocoon creates it; appending keeps the previous launch's tail. - if f, createErr := os.OpenFile(fcLog, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o600); createErr == nil { //nolint:gosec - _ = f.Close() + logFile, createErr := os.OpenFile(fcLog, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o600) //nolint:gosec + if createErr == nil { + defer logFile.Close() //nolint:errcheck } master, slave, err := pty.Open() @@ -158,6 +159,9 @@ func (fc *Firecracker) launchProcessWithLeases(ctx context.Context, rec *hypervi fcCmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} fcCmd.Stdin = slave fcCmd.Stdout = slave + if logFile != nil { + fcCmd.Stderr = logFile + } pid, exited, err := fc.LaunchVMProcess(ctx, hypervisor.LaunchSpec{ Cmd: fcCmd, NetnsPath: netnsPath, diff --git a/hypervisor/gc.go b/hypervisor/gc.go index 1bbfed82..774b9a5f 100644 --- a/hypervisor/gc.go +++ b/hypervisor/gc.go @@ -123,7 +123,7 @@ func (b *Backend) gcCollect(ctx context.Context, ids []string, snap VMGCSnapshot errs := b.sweepStaleCaptureDirs(ctx, snap.sweepDirs(b.Conf.RunDir())) errs = append(errs, b.sweepStaleCloneLocks(ctx)...) for _, id := range ids { - // Hold the ops lock: a create mkdirs before its record lands. + // Hold the ops lock: records are scanned before dirs, so an in-flight create's dirs read as orphans. ok := b.withOpsTryLock(ctx, id, func() { rec, err := b.PeekRecord(ctx, id) if err != nil { diff --git a/hypervisor/inspect.go b/hypervisor/inspect.go index c00c0297..6d642737 100644 --- a/hypervisor/inspect.go +++ b/hypervisor/inspect.go @@ -48,37 +48,39 @@ func (b *Backend) ToVM(rec *VMRecord) *types.VM { info.PID = 0 } } - info.SnapshotIDs = maps.Clone(info.SnapshotIDs) return &info } func (b *Backend) ResolveRef(ctx context.Context, ref string) (string, error) { var id string - return id, b.view(ctx, func(t *vmTx) error { + err := b.view(ctx, func(t *vmTx) error { var err error id, err = t.resolve(ref) return err }) + return id, err } // ResolveRefs batch-resolves under a single transaction. func (b *Backend) ResolveRefs(ctx context.Context, refs []string) ([]string, error) { var ids []string - return ids, b.view(ctx, func(t *vmTx) error { + err := b.view(ctx, func(t *vmTx) error { var err error ids, err = t.ResolveMany(refs, ErrNotFound) return err }) + return ids, err } // LoadRecord returns a detached copy of the record. func (b *Backend) LoadRecord(ctx context.Context, id string) (VMRecord, error) { var rec VMRecord - return rec, b.view(ctx, func(t *vmTx) error { + err := b.view(ctx, func(t *vmTx) error { var err error rec, err = t.loadDetached(id) return err }) + return rec, err } // ResolveAndLoad combines ResolveRef + LoadRecord in a single transaction. @@ -87,7 +89,7 @@ func (b *Backend) ResolveAndLoad(ctx context.Context, ref string) (string, VMRec id string rec VMRecord ) - return id, rec, b.view(ctx, func(t *vmTx) error { + err := b.view(ctx, func(t *vmTx) error { var err error if id, err = t.resolve(ref); err != nil { return err @@ -95,10 +97,11 @@ func (b *Backend) ResolveAndLoad(ctx context.Context, ref string) (string, VMRec rec, err = t.loadDetached(id) return err }) + return id, rec, err } // UpdateRecord runs mutate on the VM's record inside one transaction and persists the result. -func (b *Backend) UpdateRecord(ctx context.Context, vmID string, mutate func(*VMRecord) error) error { +func (b *Backend) UpdateRecord(ctx context.Context, vmID string, mutate RecordHook) error { return b.update(ctx, func(t *vmTx) error { r, err := t.getRecord(vmID) if err != nil { diff --git a/hypervisor/netresize.go b/hypervisor/netresize.go index c92b524c..921bd144 100644 --- a/hypervisor/netresize.go +++ b/hypervisor/netresize.go @@ -18,7 +18,7 @@ import ( // nicRollbackTimeout bounds a rollback that outlives a canceled resize (guest eject on CH can take 14 s). const nicRollbackTimeout = 60 * time.Second -// ErrEjectPending marks a RemoveNIC whose request the VMM accepted but the guest has not released; the host slot is reclaimed anyway. +// ErrEjectPending marks a RemoveNIC whose request the VMM accepted but the guest has not released. var ErrEjectPending = errors.New("guest has not released the device") // LiveNIC is one NIC device the VMM exposes right now; Index is its host slot (record position), -1 when the VMM reports a device cocoon cannot place. diff --git a/hypervisor/restore.go b/hypervisor/restore.go index df8585a7..9c083c6c 100644 --- a/hypervisor/restore.go +++ b/hypervisor/restore.go @@ -22,11 +22,11 @@ type restoreRun struct { vmCfg *types.VMConfig sourceSnapshotID string kill KillHook - apply func(*VMRecord) error + apply RecordHook afterExtract AfterExtractHook } -func (b *Backend) KillForRestore(ctx context.Context, vmID string, rec *VMRecord, terminate func(pid int) error, runtimeFiles []string) error { +func (b *Backend) KillForRestore(ctx context.Context, vmID string, rec *VMRecord, terminate PIDHook, runtimeFiles []string) error { killErr := b.WithRunningVM(ctx, rec, terminate) if killErr != nil && !errors.Is(killErr, ErrNotRunning) { b.failRestore(ctx, vmID, rec.State) @@ -50,7 +50,7 @@ func (b *Backend) FinalizeRestore(ctx context.Context, vmID string, vmCfg *types return nil, fmt.Errorf("update record: %w", err) } if rmErr := os.Remove(filepath.Join(rec.RunDir, restoreDirtyName)); rmErr != nil && !errors.Is(rmErr, fs.ErrNotExist) { - log.WithFunc(b.Typ+".FinalizeRestore").Errorf(ctx, rmErr, "clear restore-dirty marker for %s; start will refuse it until the file goes", vmID) + log.WithFunc(b.Typ+".FinalizeRestore").Warnf(ctx, "clear restore-dirty marker for %s: %v; the next start clears it", vmID, rmErr) } info := rec.VM @@ -82,12 +82,10 @@ func (b *Backend) RestoreSequence(ctx context.Context, vmRef string, spec Restor } apply := func(rec *VMRecord) error { if err := spec.Clean(rec.RunDir); err != nil { - // The sweep may already have deleted snapshot files; a stopped origin would otherwise stay startable on mixed state. b.QuarantineVM(ctx, vmID, "partial restore cleanup") return err } if mergeErr := MergeDirInto(stagingDir, rec.RunDir); mergeErr != nil { - // A partial merge mixes vintages; quarantine any origin so vm start cannot boot it. b.QuarantineVM(ctx, vmID, "partial snapshot merge") return fmt.Errorf("apply staged snapshot: %w", mergeErr) } diff --git a/hypervisor/state.go b/hypervisor/state.go index fd4b72dd..4a283fb7 100644 --- a/hypervisor/state.go +++ b/hypervisor/state.go @@ -27,7 +27,7 @@ const ( ) // WithRunningVM calls fn if rec still points to a live VM process. -func (b *Backend) WithRunningVM(ctx context.Context, rec *VMRecord, fn func(pid int) error) error { +func (b *Backend) WithRunningVM(ctx context.Context, rec *VMRecord, fn PIDHook) error { return b.withRunningVM(ctx, rec, nil, fn) } @@ -156,7 +156,7 @@ func (b *Backend) ReconcileToRunning(ctx context.Context, id string) (uint64, er } // withRunningVM resolves liveness against the caller's /proc walk; a nil scan walks now, so batch callers turn N walks into one. -func (b *Backend) withRunningVM(ctx context.Context, rec *VMRecord, scan *utils.ProcScan, fn func(pid int) error) error { +func (b *Backend) withRunningVM(ctx context.Context, rec *VMRecord, scan *utils.ProcScan, fn PIDHook) error { logger := log.WithFunc(b.Typ + ".withRunningVM") pid, pidErr := utils.ReadPIDFile(b.PIDFilePath(rec.RunDir)) if pidErr != nil && !errors.Is(pidErr, fs.ErrNotExist) { diff --git a/hypervisor/state_test.go b/hypervisor/state_test.go index 9a15b365..4a011dc2 100644 --- a/hypervisor/state_test.go +++ b/hypervisor/state_test.go @@ -163,8 +163,8 @@ func TestEmitDeleteCloseClosesIntervalAfterMarkError(t *testing.T) { b.emitDeleteClose(ctx, "vm1", shapeFromConfig(loaded.Config), metering.ReasonStopCrash, hasOpenComputeInterval(loaded)) entries := rec.Entries() - if len(entries) != 2 { - t.Fatalf("got %d entries, want 2 (compute.stop + storage.stop)", len(entries)) + if len(entries) != 2 || entries[0].Kind != metering.KindVMComputeStop || entries[1].Kind != metering.KindVMStorageStop { + t.Fatalf("entries = %+v, want compute.stop then storage.stop", entries) } } @@ -550,15 +550,15 @@ func TestPrepareStartRefusesQuarantined(t *testing.T) { seedVMRecord(t, b, "vm1", 1, 1<<30, 10<<30, true) b.QuarantineVM(ctx, "vm1", "partial snapshot merge") - if _, err := b.PrepareStart(ctx, "vm1", nil, StartSpec{}); err == nil { - t.Fatal("PrepareStart must refuse a quarantined VM") + if _, err := b.PrepareStart(ctx, "vm1", nil, StartSpec{}); err == nil || !strings.Contains(err.Error(), "quarantined") { + t.Fatalf("PrepareStart = %v, want the quarantine refusal", err) } if err := b.UpdateState(ctx, "vm1", types.VMStateStopped); err != nil { t.Fatalf("UpdateState: %v", err) } - if _, err := b.PrepareStart(ctx, "vm1", nil, StartSpec{}); err == nil { - t.Fatal("PrepareStart must refuse a quarantined VM after stop rewrote the state") + if _, err := b.PrepareStart(ctx, "vm1", nil, StartSpec{}); err == nil || !strings.Contains(err.Error(), "quarantined") { + t.Fatalf("PrepareStart = %v, want the quarantine refusal after stop rewrote the state", err) } } @@ -579,8 +579,8 @@ func TestPrepareStartRefusesInterruptedRestore(t *testing.T) { if err := os.WriteFile(filepath.Join(runDir, restoreDirtyName), nil, 0o600); err != nil { t.Fatalf("mk tombstone: %v", err) } - if _, err := b.PrepareStart(ctx, id, nil, StartSpec{}); err == nil { - t.Fatal("PrepareStart must refuse a run dir with a restore-dirty tombstone") + if _, err := b.PrepareStart(ctx, id, nil, StartSpec{}); err == nil || !strings.Contains(err.Error(), "interrupted restore") { + t.Fatalf("PrepareStart = %v, want the interrupted-restore refusal", err) } } @@ -621,8 +621,8 @@ func TestPrepareStartRefusesCreating(t *testing.T) { if err := b.ReserveVM(ctx, "vm1", &types.VMConfig{Name: "n1"}, nil, t.TempDir(), t.TempDir()); err != nil { t.Fatalf("ReserveVM: %v", err) } - if _, err := b.PrepareStart(ctx, "vm1", nil, StartSpec{}); err == nil { - t.Fatal("PrepareStart must refuse a creating placeholder") + if _, err := b.PrepareStart(ctx, "vm1", nil, StartSpec{}); err == nil || !strings.Contains(err.Error(), "still being created") { + t.Fatalf("PrepareStart = %v, want the creating-placeholder refusal", err) } } diff --git a/hypervisor/teardown.go b/hypervisor/teardown.go index fa3800b4..14a8c516 100644 --- a/hypervisor/teardown.go +++ b/hypervisor/teardown.go @@ -86,7 +86,7 @@ func (b *Backend) deleteVMProtocol(ctx context.Context, id string, rec *VMRecord var resumed *tombstone.Record var err error leaseID, resumed, err = ts.Acquire(ctx, t.w, id, func() (tombstone.Payload, error) { - return tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}, nil + return tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}, nil }) if err != nil || resumed == nil { return err diff --git a/hypervisor/teardown_test.go b/hypervisor/teardown_test.go index 38e9eeea..2341f80f 100644 --- a/hypervisor/teardown_test.go +++ b/hypervisor/teardown_test.go @@ -130,7 +130,7 @@ func TestRecoverLeasedRollsBack(t *testing.T) { } ts := b.tombstones() if err := b.update(ctx, func(tx *vmTx) error { - _, err := ts.Lease(ctx, tx.w, "vmproto3", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}) + _, err := ts.Lease(ctx, tx.w, "vmproto3", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) @@ -160,7 +160,7 @@ func TestTombstoneFencingABA(t *testing.T) { } var leaseA string if err := b.update(ctx, func(tx *vmTx) error { - leaseA, err = ts.Lease(ctx, tx.w, "vmproto4", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}) + leaseA, err = ts.Lease(ctx, tx.w, "vmproto4", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) @@ -195,7 +195,7 @@ func TestEntryGuardDisciplines(t *testing.T) { } if err := b.update(ctx, func(tx *vmTx) error { - _, err := ts.Lease(ctx, tx.w, "vmproto5", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}) + _, err := ts.Lease(ctx, tx.w, "vmproto5", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) @@ -206,7 +206,7 @@ func TestEntryGuardDisciplines(t *testing.T) { var lease string if err := b.update(ctx, func(tx *vmTx) error { - lease, err = ts.Lease(ctx, tx.w, "vmproto5", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}) + lease, err = ts.Lease(ctx, tx.w, "vmproto5", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) @@ -237,7 +237,7 @@ func TestLiveHolderKeepsLease(t *testing.T) { } var leaseA string if err := b.update(ctx, func(tx *vmTx) error { - leaseA, err = ts.Lease(ctx, tx.w, "vmlive1", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}) + leaseA, err = ts.Lease(ctx, tx.w, "vmlive1", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) @@ -290,7 +290,7 @@ func TestPrepareStartRefusesMidDeleting(t *testing.T) { } var leaseA string if err := b.update(ctx, func(tx *vmTx) error { - leaseA, err = ts.Lease(ctx, tx.w, "vmentry1", tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}) + leaseA, err = ts.Lease(ctx, tx.w, "vmentry1", tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) diff --git a/hypervisor/utils.go b/hypervisor/utils.go index 76ace5c4..4d2b9ad0 100644 --- a/hypervisor/utils.go +++ b/hypervisor/utils.go @@ -37,7 +37,7 @@ const ( CloneLocksDirName = "clone-locks" restoreStagingName = ".restore-staging" - // restoreDirtyName is cleared only by FinalizeRestore, never by GC. + // GC never clears restoreDirtyName. restoreDirtyName = ".restore-dirty" captureDirPrefix = "snapshot-" @@ -49,7 +49,7 @@ const ( var hostCPUCount = sync.OnceValue(func() int { return hostCPUCountAt(onlineCPUsPath) }) -// SnapshotFileKind tells clone how to bring a file over: Memory links, COW copies, Meta plain-copies, Skip omits. +// SnapshotFileKind tells CloneSnapshotFiles how to bring a file over. type SnapshotFileKind int // LockVMOps serializes mutating verbs on one VM across processes; the flock dies with the holder so a crash never wedges the VM. @@ -187,7 +187,7 @@ func BlobHexFromPath(path string) string { return strings.TrimSuffix(base, filepath.Ext(base)) } -// BuildBaseCmdline composes the cocoon-shared cmdline: prefix (backend-specific console+quirks) + boot/layers/cow + per-NIC ip= params. +// BuildBaseCmdline appends cocoon's overlay and per-NIC ip= params to the backend prefix. func BuildBaseCmdline(prefix, layers, cow string, networkConfigs []*types.NetworkConfig, vmName string, dnsServers []string) string { var b strings.Builder fmt.Fprintf(&b, "%s boot=cocoon-overlay cocoon.layers=%s cocoon.cow=%s clocksource=kvm-clock rw", prefix, layers, cow) diff --git a/images/cloudimg/commit.go b/images/cloudimg/commit.go index a98b8de4..0f75f8e9 100644 --- a/images/cloudimg/commit.go +++ b/images/cloudimg/commit.go @@ -39,7 +39,7 @@ func commit(ctx context.Context, conf *Config, store *images.Store[imageEntry], tracker.OnEvent(cloudimgProgress.Event{Phase: cloudimgProgress.PhaseCommit}) - // Lock spans rename→index-commit so GC cannot delete a blob that is on disk but not yet indexed (design §5). + // Lock spans rename→index-commit so GC cannot delete a blob that is on disk but not yet indexed. var blobLocks images.BlobLocks defer blobLocks.Release() if err := blobLocks.Lock(conf.BlobLockPath(digestHex)); err != nil { diff --git a/images/gc.go b/images/gc.go index 609cfdf7..07c22d26 100644 --- a/images/gc.go +++ b/images/gc.go @@ -133,6 +133,9 @@ func collectBlob[E any](ctx context.Context, cfg GCModuleConfig[E], logger *log. errs = append(errs, fmt.Errorf("remove blob %s: %w", hex, err)) } } + if len(errs) > 0 { + return errors.Join(errs...) + } logger.Infof(ctx, "collected blob=%s reason=unreferenced", hex) - return errors.Join(errs...) + return nil } diff --git a/images/images.go b/images/images.go index 2741ea7b..cb242a2a 100644 --- a/images/images.go +++ b/images/images.go @@ -26,6 +26,6 @@ type Images interface { Config(context.Context, *types.VMConfig) ([]*types.StorageConfig, *types.BootConfig, error) - // PinBlobs holds the digest locks while the caller commits a pin on the resolved blobs (design §5: every re-pinning flow takes the digest lock). + // PinBlobs holds the digest locks while the caller commits a pin on the resolved blobs. PinBlobs(ctx context.Context, blobIDs map[string]struct{}) (release func(), err error) } diff --git a/images/index.go b/images/index.go index 8697fe81..b20d3f55 100644 --- a/images/index.go +++ b/images/index.go @@ -31,11 +31,7 @@ type Index[E any] struct { func ReferencedDigests[E Entry](images map[string]*E) map[string]struct{} { refs := make(map[string]struct{}) for _, ep := range images { - if ep == nil { - continue - } - e := *ep - for _, hex := range e.DigestHexes() { + for _, hex := range (*ep).DigestHexes() { refs[hex] = struct{}{} } } @@ -53,18 +49,14 @@ func LookupOne[E Entry](images map[string]*E, id string) (string, *E, bool) { // LookupRefs returns all ref keys matching id by exact key or digest prefix. func LookupRefs[E Entry](images map[string]*E, id string) []string { - if entry, ok := images[id]; ok && entry != nil { + if _, ok := images[id]; ok { return []string{id} } // minHexLen guards against over-broad prefixes like "sha256:a". idHex := strings.TrimPrefix(id, "sha256:") var refs []string for ref, ep := range images { - if ep == nil { - continue - } - e := *ep - dStr := e.EntryID() + dStr := (*ep).EntryID() dHex := strings.TrimPrefix(dStr, "sha256:") if dStr == id || dHex == id { refs = append(refs, ref) @@ -79,13 +71,9 @@ func LookupRefs[E Entry](images map[string]*E, id string) []string { // refsShareDigest reports whether every ref names the same digest (tag aliases of one image). func refsShareDigest[E Entry](images map[string]*E, refs []string) bool { - first := images[refs[0]] - if first == nil { - return false - } + first := (*images[refs[0]]).EntryID() for _, ref := range refs[1:] { - e := images[ref] - if e == nil || (*e).EntryID() != (*first).EntryID() { + if (*images[ref]).EntryID() != first { return false } } @@ -115,9 +103,6 @@ func deleteByID[E Entry](ctx context.Context, logPrefix string, images map[strin } func entryToImage[E Entry](entry *E, typ string) *types.Image { - if entry == nil { - return nil - } e := *entry return &types.Image{ ID: e.EntryID(), @@ -134,9 +119,6 @@ func listImages[E Entry](images map[string]*E, typ string) []*types.Image { } out := make([]*types.Image, 0, len(images)) for _, ep := range images { - if ep == nil { - continue - } out = append(out, entryToImage(ep, typ)) } return out diff --git a/images/oci/commit.go b/images/oci/commit.go index b5c9a8ad..5e5abfcf 100644 --- a/images/oci/commit.go +++ b/images/oci/commit.go @@ -27,7 +27,6 @@ func moveBootFile(src, dst, bootDir string, layerIdx int, name string) error { return nil } -// finishImport commits blobs, then records the entry in one pure transaction. func finishImport(ctx context.Context, conf *Config, store *images.Store[imageEntry], name string, manifestDigest images.Digest, results []pullLayerResult, tracker progress.Tracker, verb string) error { logger := log.WithFunc("oci.finishImport") diff --git a/images/oci/uuid.go b/images/oci/uuid.go index 3caeb52d..64792061 100644 --- a/images/oci/uuid.go +++ b/images/oci/uuid.go @@ -2,7 +2,6 @@ package oci import "github.com/google/uuid" -// uuidV5 returns the RFC 4122 version 5 UUID of name in the URL namespace. func uuidV5(name string) string { return uuid.NewSHA1(uuid.NameSpaceURL, []byte(name)).String() } diff --git a/images/store.go b/images/store.go index 3d904026..17351995 100644 --- a/images/store.go +++ b/images/store.go @@ -63,9 +63,6 @@ func (s *Store[E]) applyDiff(ctx context.Context, w meta.Writer, fn IndexFunc[E] } } for id, entry := range idx.Images { - if entry == nil { - continue - } raw, err := json.Marshal(entry, json.Deterministic(true)) if err != nil { return fmt.Errorf("encode image entry %q: %w", id, err) diff --git a/meta/broadcast_test.go b/meta/broadcast_test.go new file mode 100644 index 00000000..5cce3e52 --- /dev/null +++ b/meta/broadcast_test.go @@ -0,0 +1,29 @@ +package meta + +import ( + "errors" + "testing" + "time" + + "github.com/fsnotify/fsnotify" +) + +func TestBroadcasterWatchErrorRunsCheck(t *testing.T) { + errs := make(chan error) + b := NewBroadcaster(&fsnotify.Watcher{Events: make(chan fsnotify.Event), Errors: errs}) + checked := make(chan struct{}, 1) + go b.Run(func() { + select { + case checked <- struct{}{}: + default: + } + }) + defer close(b.done) + + errs <- errors.New("forced overflow") + select { + case <-checked: + case <-time.After(3 * time.Second): + t.Fatal("watch error did not run check") + } +} diff --git a/meta/json/events.go b/meta/json/events.go index 753853bd..ad3973e0 100644 --- a/meta/json/events.go +++ b/meta/json/events.go @@ -36,10 +36,9 @@ type fileToken struct { // Tokens are touched only by newNotifier and the Run goroutine, never concurrently. type notifier struct { - b *meta.Broadcaster - watcher *fsnotify.Watcher // kept for the severed-watch test seam - files []string - tokens map[string]fileToken + b *meta.Broadcaster + files []string + tokens map[string]fileToken } func newNotifier(nss map[string]*nsState) (*notifier, error) { @@ -47,7 +46,7 @@ func newNotifier(nss map[string]*nsState) (*notifier, error) { if err != nil { return nil, err } - n := ¬ifier{b: meta.NewBroadcaster(watcher), watcher: watcher, tokens: map[string]fileToken{}} + n := ¬ifier{b: meta.NewBroadcaster(watcher), tokens: map[string]fileToken{}} dirs := map[string]struct{}{} for _, st := range nss { n.files = append(n.files, st.def.FilePath) diff --git a/meta/json/model.go b/meta/json/model.go index 64192e7d..216f51b1 100644 --- a/meta/json/model.go +++ b/meta/json/model.go @@ -1,4 +1,4 @@ -// Package json is the meta engine over today's per-namespace JSON files: same formats, same .prev crash story, same flocks (design §8). +// Package json is the meta engine over today's per-namespace JSON files: same formats, same .prev crash story, same flocks. package json import ( diff --git a/meta/json/store_test.go b/meta/json/store_test.go index b6a47425..5164da9f 100644 --- a/meta/json/store_test.go +++ b/meta/json/store_test.go @@ -6,7 +6,6 @@ import ( "path/filepath" "slices" "testing" - "time" "github.com/cocoonstack/cocoon/meta" "github.com/cocoonstack/cocoon/meta/contracttest" @@ -176,42 +175,6 @@ func TestOpenValidation(t *testing.T) { } } -func TestEventsForcedOverflow(t *testing.T) { - ctx := t.Context() - dir := t.TempDir() - - s := newStore(t, dir, "alpha") - - ch, release, err := s.Events(ctx) - if err != nil { - t.Fatal(err) - } - defer release() - - if err := s.events.watcher.Remove(dir); err != nil { - t.Fatalf("remove watch: %v", err) - } - c := meta.NewCollection[map[string]int]("alpha", "records") - v := map[string]int{"v": 1} - if err := s.Update(ctx, meta.Scope{Write: "alpha"}, meta.CommitDurable, func(w meta.Writer) error { - return c.Insert(ctx, w, "a", &v) - }); err != nil { - t.Fatal(err) - } - - select { - case <-ch: - case <-time.After(200 * time.Millisecond): - } - - s.events.watcher.Errors <- errors.New("forced overflow") - select { - case <-ch: - case <-time.After(3 * time.Second): - t.Fatal("no signal after forced overflow (safety poll is 5s, so this was the overflow path)") - } -} - func TestDurableSyncOrder(t *testing.T) { var steps []string testCrashStep = func(step string) error { steps = append(steps, step); return nil } diff --git a/meta/json/tables.go b/meta/json/tables.go index 2d8d544a..a4d8f370 100644 --- a/meta/json/tables.go +++ b/meta/json/tables.go @@ -59,7 +59,7 @@ func (c TableCodec) Decode(data []byte) (*Model, error) { if _, err := dec.ReadToken(); err != nil { return nil, err } - // Legacy json.Unmarshal rejected trailing bytes; a truncated-then-appended main must fall back to .prev, not decode (§9 format fidelity). + // Legacy json.Unmarshal rejected trailing bytes; a truncated-then-appended main must fall back to .prev, not decode. if _, err := dec.ReadToken(); !errors.Is(err, io.EOF) { return nil, errors.New("namespace file: trailing data after document") } diff --git a/meta/log.go b/meta/log.go index 7a27471b..8b121f00 100644 --- a/meta/log.go +++ b/meta/log.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json/v2" "fmt" + "strconv" ) const seqCursorID = "cursor" @@ -42,11 +43,7 @@ func (l *Log[R]) Append(ctx context.Context, w Writer, rec *R, opts ...WriteOpt) if perr := w.PutRaw(ctx, l.ns, l.table, seqID(next), data, relaxedOK(opts)); perr != nil { return 0, perr } - curRaw, err := json.Marshal(next) - if err != nil { - return 0, err - } - if perr := w.PutRaw(ctx, l.ns, l.table, seqCursorID, curRaw, relaxedOK(opts)); perr != nil { + if perr := w.PutRaw(ctx, l.ns, l.table, seqCursorID, strconv.AppendUint(nil, uint64(next), 10), relaxedOK(opts)); perr != nil { return 0, perr } return next, nil diff --git a/meta/sqlite/events.go b/meta/sqlite/events.go index be469202..ab9b6d30 100644 --- a/meta/sqlite/events.go +++ b/meta/sqlite/events.go @@ -10,7 +10,7 @@ import ( "github.com/cocoonstack/cocoon/meta" ) -// Events pins one connection; data_version moves only for other connections' commits (§7). +// Events pins one connection; data_version moves only for other connections' commits. func (s *Store) Events(ctx context.Context) (<-chan struct{}, func(), error) { s.mu.Lock() defer s.mu.Unlock() @@ -26,46 +26,39 @@ func (s *Store) Events(ctx context.Context) (<-chan struct{}, func(), error) { return ch, func() { stop(); release() }, nil } -// Version is touched only by the init call and the Run goroutine; ctx lives until stop. +// Version is touched only by the init call and the Run goroutine. type notifier struct { b *meta.Broadcaster watcher *fsnotify.Watcher // kept for the severed-watch test seam pinned *sql.Conn pinDB *sql.DB - ctx context.Context - cancel context.CancelFunc version int64 } func newNotifier(dbPath string) (*notifier, error) { - ctx, cancel := context.WithCancel(context.Background()) db, err := open(dbPath, "FULL", false) if err != nil { - cancel() return nil, err } db.SetMaxOpenConns(1) - conn, err := db.Conn(ctx) + conn, err := db.Conn(context.Background()) if err != nil { - cancel() _ = db.Close() return nil, mapErr(err) } watcher, err := fsnotify.NewWatcher() if err != nil { - cancel() _ = conn.Close() _ = db.Close() return nil, err } if err := watcher.Add(filepath.Dir(dbPath)); err != nil { - cancel() _ = watcher.Close() _ = conn.Close() _ = db.Close() return nil, err } - n := ¬ifier{b: meta.NewBroadcaster(watcher), watcher: watcher, pinned: conn, pinDB: db, ctx: ctx, cancel: cancel} + n := ¬ifier{b: meta.NewBroadcaster(watcher), watcher: watcher, pinned: conn, pinDB: db} n.version, _ = n.dataVersion() go n.b.Run(n.check) return n, nil @@ -73,7 +66,6 @@ func newNotifier(dbPath string) (*notifier, error) { func (n *notifier) stop() { n.b.Stop() - n.cancel() _ = n.pinned.Close() _ = n.pinDB.Close() } @@ -89,6 +81,6 @@ func (n *notifier) check() { func (n *notifier) dataVersion() (int64, error) { var v int64 - err := n.pinned.QueryRowContext(n.ctx, "PRAGMA data_version").Scan(&v) + err := n.pinned.QueryRowContext(context.Background(), "PRAGMA data_version").Scan(&v) return v, err } diff --git a/meta/sqlite/fscheck.go b/meta/sqlite/fscheck.go index 76091352..8b4216b4 100644 --- a/meta/sqlite/fscheck.go +++ b/meta/sqlite/fscheck.go @@ -7,7 +7,7 @@ import ( "github.com/cocoonstack/cocoon/meta" ) -// CheckFS refuses unsupported filesystems before any WAL work (§4); the env seam lets every entry path (open, init, convert) be asserted in tests. +// CheckFS refuses unsupported filesystems before any WAL work; the env seam lets every entry path (open, init, convert) be asserted in tests. func CheckFS(dbPath string) error { if name := os.Getenv("COCOON_TEST_UNSUPPORTED_FS"); name != "" { return fsRefusal(dbPath, name) diff --git a/meta/sqlite/fscheck_linux.go b/meta/sqlite/fscheck_linux.go index 2c683e10..b7cd2bf8 100644 --- a/meta/sqlite/fscheck_linux.go +++ b/meta/sqlite/fscheck_linux.go @@ -6,7 +6,7 @@ import ( "golang.org/x/sys/unix" ) -// WAL needs coherent shared memory; these magics mark filesystems that cannot provide it (§4), FUSE refused as unknowable. +// WAL needs coherent shared memory; these magics mark filesystems that cannot provide it, FUSE refused as unknowable. var unsupportedFS = map[uint32]string{ unix.NFS_SUPER_MAGIC: "nfs", unix.CIFS_SUPER_MAGIC: "cifs", diff --git a/meta/sqlite/fscheck_other.go b/meta/sqlite/fscheck_other.go index e3d264b8..7a066e81 100644 --- a/meta/sqlite/fscheck_other.go +++ b/meta/sqlite/fscheck_other.go @@ -2,5 +2,5 @@ package sqlite -// statfsCheck is enforced on Linux (§4); other platforms are dev-only. +// statfsCheck is enforced on Linux; other platforms are dev-only. func statfsCheck(string) error { return nil } diff --git a/meta/sqlite/init.go b/meta/sqlite/init.go index c4b98287..f5f0d2af 100644 --- a/meta/sqlite/init.go +++ b/meta/sqlite/init.go @@ -15,7 +15,7 @@ import ( const initLockName = "init.lock" -// Init creates a fresh store — schema, identity pragmas and one meta_state row per namespace — in one transaction, so a crash mid-init leaves nothing or a complete store (§6). +// Init creates a fresh store — schema, identity pragmas and one meta_state row per namespace — in one transaction, so a crash mid-init leaves nothing or a complete store. func Init(ctx context.Context, dbPath string, namespaces ...Namespace) error { if err := RefuseManifest(dbPath); err != nil { return err @@ -40,7 +40,7 @@ func InitIfMissing(ctx context.Context, dbPath string, namespaces ...Namespace) }) } -// InitForRecoveryIfNeeded creates or repairs the conversion target without the manifest guard — the conversion tool creates its target while the manifest is necessarily present (§6) — and passes a completed one through. +// InitForRecoveryIfNeeded creates or repairs the conversion target without the manifest guard — the conversion tool creates its target while the manifest is necessarily present — and passes a completed one through. func InitForRecoveryIfNeeded(ctx context.Context, dbPath string, namespaces ...Namespace) error { need, err := initNeeded(ctx, dbPath) if err != nil || !need { diff --git a/meta/sqlite/maintenance.go b/meta/sqlite/maintenance.go index b75e5ea4..143a8f68 100644 --- a/meta/sqlite/maintenance.go +++ b/meta/sqlite/maintenance.go @@ -12,10 +12,10 @@ import ( "github.com/cocoonstack/cocoon/utils" ) -// testBackupStep injects crashes between backup steps (§9 gate); nil in prod. +// testBackupStep injects crashes between backup steps; nil in prod. var testBackupStep func(step string) error -// MarkConverted records a namespace's conversion provenance in meta_state (§6). +// MarkConverted records a namespace's conversion provenance in meta_state. func MarkConverted(ctx context.Context, dbPath, ns, source, sha256 string, records int) error { return withDB(dbPath, func(db *sql.DB) error { _, err := db.ExecContext(ctx, "UPDATE meta_state SET state = 'converted', source = ?, sha256 = ?, records = ?, applied_at = datetime('now') WHERE namespace = ?", source, sha256, records, ns) @@ -23,7 +23,7 @@ func MarkConverted(ctx context.Context, dbPath, ns, source, sha256 string, recor }) } -// Checkpoint folds the WAL back into the main file (TRUNCATE) so the database is a single self-contained file (§6 aside rule). +// Checkpoint folds the WAL back into the main file (TRUNCATE) so the database is a single self-contained file. func Checkpoint(ctx context.Context, dbPath string) error { return withDB(dbPath, func(db *sql.DB) error { _, err := db.ExecContext(ctx, "PRAGMA wal_checkpoint(TRUNCATE)") @@ -31,7 +31,7 @@ func Checkpoint(ctx context.Context, dbPath string) error { }) } -// Backup replaces destPath atomically (VACUUM INTO a temp file, integrity check, fsync, rename), so a previously published backup stays intact until the rename commits (§9). +// Backup replaces destPath atomically (VACUUM INTO a temp file, integrity check, fsync, rename), so a previously published backup stays intact until the rename commits. func Backup(ctx context.Context, dbPath, destPath string) error { if !utils.FileExists(dbPath) { return fmt.Errorf("no sqlite store at %s to back up", dbPath) diff --git a/meta/sqlite/store.go b/meta/sqlite/store.go index 5ca4344f..05251e6f 100644 --- a/meta/sqlite/store.go +++ b/meta/sqlite/store.go @@ -1,4 +1,4 @@ -// Package sqlite is the meta scale engine: one WAL database, namespace = table group, generic (id, data) rows per root-declared table (§2-§4). +// Package sqlite is the meta scale engine: one WAL database, namespace = table group, generic (id, data) rows per root-declared table. package sqlite import ( @@ -29,7 +29,7 @@ const ( // UserVersion is the schema generation, verified on every open and written only at init; scripts/meta-upgrade.py moves a store between generations. UserVersion = 2 - // DBFileName is the single database under the meta root; ManifestName beside it marks an in-flight conversion, which ordinary opens refuse (§6). + // DBFileName is the single database under the meta root; ManifestName beside it marks an in-flight conversion, which ordinary opens refuse. DBFileName = "meta.db" ManifestName = "meta-convert.manifest" @@ -52,7 +52,7 @@ type tableStmts struct { var _ meta.Store = (*Store)(nil) -// Store is the sqlite engine; statements are prepared per handle at Open, since the table set is static (§4). +// Store is the sqlite engine; statements are prepared per handle at Open, since the table set is static. type Store struct { path string nss map[string]Namespace @@ -76,7 +76,7 @@ func Open(dbPath string, namespaces ...Namespace) (*Store, error) { return openStore(dbPath, namespaces) } -// OpenForRecovery is Open without the manifest guard, for the conversion tool itself (§6) — never for ordinary callers. +// OpenForRecovery is Open without the manifest guard, for the conversion tool itself — never for ordinary callers. func OpenForRecovery(dbPath string, namespaces ...Namespace) (*Store, error) { return openStore(dbPath, namespaces) } @@ -228,7 +228,7 @@ func (s *Store) checkScope(nss []string) error { return nil } -// verifyIdentity never writes (§6): a wrong id or a mismatched version fails closed, and a namespace with no meta_state row is uninitialized, never empty. +// verifyIdentity never writes: a wrong id or a mismatched version fails closed, and a namespace with no meta_state row is uninitialized, never empty. func (s *Store) verifyIdentity() error { var appID, version int64 if err := s.readers.QueryRow("PRAGMA application_id").Scan(&appID); err != nil { @@ -383,7 +383,7 @@ func scopeSet(nss []string) map[string]struct{} { return set } -// open applies the per-connection runtime contract (§4); cache_size is KB (negative form) and mmap_size covers the whole file at meta scale. +// open applies the per-connection runtime contract; cache_size is KB (negative form) and mmap_size covers the whole file at meta scale. func open(dbPath, synchronous string, writer bool) (*sql.DB, error) { dsn := "file:" + dbPath + "?_pragma=journal_mode(WAL)&_pragma=busy_timeout(50)&_pragma=foreign_keys(1)&_pragma=trusted_schema(0)" + "&_pragma=cache_size(-16384)&_pragma=mmap_size(268435456)&_pragma=synchronous(" + synchronous + ")" diff --git a/meta/tombstone/tombstone.go b/meta/tombstone/tombstone.go index 8316ecb1..8e02d111 100644 --- a/meta/tombstone/tombstone.go +++ b/meta/tombstone/tombstone.go @@ -1,4 +1,4 @@ -// Package tombstone is the §5 phase protocol: leased rolls back, deleting rolls forward, every mutation fenced by the holder's lease id. +// Package tombstone is the delete phase protocol: leased rolls back, deleting rolls forward, every mutation fenced by the holder's lease id. package tombstone import ( @@ -14,34 +14,27 @@ import ( ) const ( - // TableName is the tombstones satellite table every §5 namespace carries. + // TableName is the tombstones satellite table every tombstoned namespace carries. TableName = "tombstones" PhaseLeased Phase = "leased" PhaseDeleting Phase = "deleting" - KindRecord Kind = "record" - KindOrphan Kind = "orphan" - ModeAggregate Mode = "aggregate" ModeSubset Mode = "subset" ) -// ErrLost reports a fenced write that matched zero rows: another worker recovered and finalized this lease after its owner died. +// ErrLost reports a fenced write whose lease no longer matches: another worker recovered and finalized it after its owner died. var ErrLost = errors.New("tombstone lease lost") // Phase is the tombstone's protocol position. type Phase string -// Kind distinguishes a record-backed candidate from a recordless orphan. -type Kind string - // Mode distinguishes a full teardown from an explicit subset; recovering a subset as an aggregate would destroy healthy resources. type Mode string // Payload is written whole at lease time and immutable after: recovery reads it and nothing else. type Payload struct { - Kind Kind `json:"kind"` Mode Mode `json:"mode"` Cleanup jsontext.Value `json:"cleanup"` } diff --git a/metadata/metadata.go b/metadata/metadata.go index 7a584fb8..0ace80c5 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -128,7 +128,7 @@ type NetworkInfo struct { IP string Prefix int Gateway string - MAC string // matched by macaddress in network-config + MAC string } // MountSpec is one cloud-init `mounts:` row; fields render verbatim. diff --git a/network/cni/cni.go b/network/cni/cni.go index 98eb5ad1..398cb1fb 100644 --- a/network/cni/cni.go +++ b/network/cni/cni.go @@ -50,9 +50,6 @@ type CNI struct { // New creates a CNI provider; conflist loading is best-effort, so Delete still tears down a 0-NIC VM when none is available and Add fails. func New(conf *config.Config, store meta.Store) (*CNI, error) { - if conf == nil { - return nil, fmt.Errorf("config is nil") - } cfg := NewConfig(conf) if err := utils.EnsureDirs(cfg.dbDir()); err != nil { return nil, fmt.Errorf("ensure cni dirs: %w", err) diff --git a/network/cni/cni_test.go b/network/cni/cni_test.go index 8fd3ee56..15426b64 100644 --- a/network/cni/cni_test.go +++ b/network/cni/cni_test.go @@ -504,6 +504,21 @@ func TestAddRecoveryPassesIdentityToPlugin(t *testing.T) { } } +func TestAddRecoveryRollbackKeepsRecordedNICsAllocated(t *testing.T) { + c, exec := newTestCNIWithStore(t) + stubLifecycleSeams(t) + seedRecords(t, c, "vm1", "eth0", "eth1") + exec.failIf = "eth1" + existing := []*types.NetworkConfig{{MAC: "02:00:00:00:00:01", Network: &types.Network{IP: "10.0.0.5"}}, {MAC: "02:00:00:00:00:02"}} + if _, err := c.Add(t.Context(), "vm1", testVMCfg(), network.AddRecover(existing)...); err == nil { + t.Fatal("recover Add succeeded, want the eth1 failure") + } + if !slices.Equal(exec.attempted, []string{"eth0", "eth0", "eth1"}) { + t.Fatalf("plugin calls = %v, want no rollback DEL that frees a recorded NIC's IP", exec.attempted) + } + assertRecordIDs(t, c, []string{"n-eth0", "n-eth1"}) +} + func TestAddRecoveryRecordsAnUnrecordedNIC(t *testing.T) { c, _ := newTestCNIWithStore(t) stubLifecycleSeams(t) diff --git a/network/cni/gc.go b/network/cni/gc.go index 16d6e5ea..1f2c8841 100644 --- a/network/cni/gc.go +++ b/network/cni/gc.go @@ -60,7 +60,7 @@ func (c *CNI) GCModule(vmInUse network.VMInUse) gc.Module[cniSnapshot] { logger := log.WithFunc("gc.cni") var errs []error for _, vmID := range ids { - // The owning VM's lock covers network teardown (design §5); a held lock means an in-flight lifecycle operation — skip. + // The owning VM's lock covers network teardown; a held lock means an in-flight lifecycle operation — skip. lk, lockErr := vmlock.New(c.conf.RootDir, vmID) if lockErr != nil { errs = append(errs, lockErr) @@ -94,7 +94,7 @@ func (c *CNI) GCModule(vmInUse network.VMInUse) gc.Module[cniSnapshot] { } } -// gcRecover resumes existing network tombstones by phase before discovery, each under its owning VM's lock (design §5 recovery-precedes-discovery). +// gcRecover resumes existing network tombstones by phase before discovery, each under its owning VM's lock. func (c *CNI) gcRecover(ctx context.Context) []error { var ids []string if err := c.view(ctx, func(t *netTx) error { diff --git a/network/cni/lifecycle.go b/network/cni/lifecycle.go index 0fd7d170..145f66e7 100644 --- a/network/cni/lifecycle.go +++ b/network/cni/lifecycle.go @@ -81,16 +81,19 @@ func (c *CNI) Add(ctx context.Context, vmID string, vmCfg *types.VMConfig, specs } for _, spec := range specs[:attempted] { ifn := ifName(spec.Index) - if delErr := c.cniDel(rctx, confList, vmID, nsPath, ifn); delErr != nil { - logger.Warnf(rctx, "rollback CNI DEL %s/%s: %v (record kept)", vmID, ifn, delErr) - continue + recID, staged := recIDs[spec.Index] + if staged { + if delErr := c.cniDel(rctx, confList, vmID, nsPath, ifn); delErr != nil { + logger.Warnf(rctx, "rollback CNI DEL %s/%s: %v (record kept)", vmID, ifn, delErr) + continue + } } // setupTCRedirect creates the TAP; it would leak in the persisting netns. if delErr := deleteTAPFn(nsPath, tapNameForVM(vmID, spec.Index)); delErr != nil { logger.Warnf(rctx, "rollback tap delete %s: %v (record kept)", tapNameForVM(vmID, spec.Index), delErr) continue } - if recID := recIDs[spec.Index]; recID != "" { + if staged { releasedIDs = append(releasedIDs, recID) } } diff --git a/network/cni/teardown.go b/network/cni/teardown.go index 36feecb5..f3db1221 100644 --- a/network/cni/teardown.go +++ b/network/cni/teardown.go @@ -57,15 +57,11 @@ func (c *CNI) teardownProtocol(ctx context.Context, vmID string, subset []string } var resumed *tombstone.Record leaseID, resumed, err = ts.Acquire(ctx, t.Writer(), vmID, func() (tombstone.Payload, error) { - kind := tombstone.KindRecord - if len(cl.Records) == 0 { - kind = tombstone.KindOrphan // a 0-NIC VM still owns its netns - } cleanup, mErr := tombstone.MarshalCleanup(cl) if mErr != nil { return tombstone.Payload{}, mErr } - return tombstone.Payload{Kind: kind, Mode: mode, Cleanup: cleanup}, nil + return tombstone.Payload{Mode: mode, Cleanup: cleanup}, nil }) if err != nil || resumed == nil { return err @@ -137,7 +133,7 @@ func (c *CNI) finishTeardown(ctx context.Context, vmID, leaseID string, cl netCl return nil } -// recoverTombstone drives vmID's tombstone under the held VM lock; rolledForward reports a completed deleting recovery so entrypoints refuse the current operation (design §5 binding rule). +// recoverTombstone drives vmID's tombstone under the held VM lock; rolledForward reports a completed deleting recovery so entrypoints refuse the current operation. func (c *CNI) recoverTombstone(ctx context.Context, vmID string) (rolledForward bool, err error) { ts := c.tombstones() var ( diff --git a/network/cni/teardown_test.go b/network/cni/teardown_test.go index 971de5ba..119eb789 100644 --- a/network/cni/teardown_test.go +++ b/network/cni/teardown_test.go @@ -267,7 +267,7 @@ func markDeleting(t *testing.T, c *CNI, vmID string, mode tombstone.Mode, cl net if err != nil { return err } - leaseID, err = ts.Lease(ctx, tx.Writer(), vmID, tombstone.Payload{Kind: tombstone.KindRecord, Mode: mode, Cleanup: cleanup}) + leaseID, err = ts.Lease(ctx, tx.Writer(), vmID, tombstone.Payload{Mode: mode, Cleanup: cleanup}) return err }); err != nil { t.Fatal(err) diff --git a/network/tap_linux.go b/network/tap_linux.go index bd02f9d1..b7dc0646 100644 --- a/network/tap_linux.go +++ b/network/tap_linux.go @@ -17,7 +17,7 @@ const ( GROMaxSize = 65536 ) -// CreateTAP adds a multi-queue TAP and returns its index; the VMM reopens it by name. +// CreateTAP adds a persistent TAP with numQueues/2 queue pairs and returns its index; the VMM reopens it by name. func CreateTAP(name string, numQueues int) (int, error) { // Queue pairs = num_queues / 2 (TX+RX pair); multi-queue needs >1 and must match the VMM's IFF_MULTI_QUEUE expectation. queuePairs := max(1, numQueues/2) diff --git a/progress/cloudimg/cloudimg.go b/progress/cloudimg/cloudimg.go index 0100b1d1..27d3ba6b 100644 --- a/progress/cloudimg/cloudimg.go +++ b/progress/cloudimg/cloudimg.go @@ -14,6 +14,6 @@ type Phase int // Event describes a single cloud image pull progress update. type Event struct { Phase Phase - BytesTotal int64 // from Content-Length; -1 if unknown + BytesTotal int64 // 0 or less if unknown BytesDone int64 // bytes downloaded so far (download phase only) } diff --git a/scripts/meta-upgrade.py b/scripts/meta-upgrade.py index 9cb74dde..b8786da8 100755 --- a/scripts/meta-upgrade.py +++ b/scripts/meta-upgrade.py @@ -34,10 +34,6 @@ def placement(record: dict[str, Any]) -> str | None: def upgrade_sqlite(db_path: str) -> None: - if os.path.exists(os.path.join(os.path.dirname(db_path), "meta-convert.manifest")): - sys.exit( - f"{db_path}: a meta convert is in flight; finish it with the binary that started it" - ) db = sqlite3.connect(db_path, timeout=5, isolation_level=None) app_id = db.execute("PRAGMA application_id").fetchone()[0] if app_id != APPLICATION_ID: @@ -74,7 +70,7 @@ def upgrade_sqlite(db_path: str) -> None: def write_file(path: str, data: str) -> None: tmp = path + ".upgrade" - with open(tmp, "w") as f: + with open(tmp, "w", encoding="utf-8") as f: f.write(data) f.flush() os.fsync(f.fileno()) @@ -91,7 +87,7 @@ def upgrade_json(root: str) -> None: found += 1 with open(os.path.join(db_dir, "vms.lock"), "a") as lock: fcntl.flock(lock, fcntl.LOCK_EX) - with open(path) as f: + with open(path, encoding="utf-8") as f: doc = json.load(f) rows = { vm_id: cpus @@ -126,6 +122,10 @@ def main() -> int: ) root = parser.parse_args().root_dir db_path = os.path.join(root, "meta", "meta.db") + if os.path.exists(os.path.join(root, "meta", "meta-convert.manifest")): + sys.exit( + f"{root}: a meta convert is in flight; finish it with the binary that started it" + ) if os.path.exists(db_path): upgrade_sqlite(db_path) else: diff --git a/snapshot/localfile/gc.go b/snapshot/localfile/gc.go index 9fbef65a..1b783148 100644 --- a/snapshot/localfile/gc.go +++ b/snapshot/localfile/gc.go @@ -45,7 +45,7 @@ type snapshotGCSnapshot struct { dataDirs []string leaseIDs []string stalePending []string - missingDir []string // finalized records whose data dir vanished + missingDir []string records map[string]snapshotMeta reasons map[string]string policy EvictionPolicy diff --git a/snapshot/localfile/localfile.go b/snapshot/localfile/localfile.go index 1a972edc..9c369b4c 100644 --- a/snapshot/localfile/localfile.go +++ b/snapshot/localfile/localfile.go @@ -66,9 +66,6 @@ type LocalFile struct { // New builds a LocalFile snapshot backend; rec may be nil and falls back to NopRecorder on emit. func New(conf *config.Config, rec metering.Recorder, store meta.Store, opts ...Option) (*LocalFile, error) { - if conf == nil { - return nil, fmt.Errorf("config is nil") - } cfg := NewConfig(conf) if err := cfg.EnsureDirs(); err != nil { return nil, fmt.Errorf("ensure dirs: %w", err) @@ -162,7 +159,7 @@ func (lf *LocalFile) CreateFromDir(ctx context.Context, cfg *types.SnapshotConfi } return "", false, fmt.Errorf("move capture into store: %w", err) } - if err = os.Chmod(dataDir, 0o750); err != nil { //nolint:gosec // match the store's 0o750 data dirs (MkdirAll in Create) + if err = os.Chmod(dataDir, 0o750); err != nil { //nolint:gosec // match the store's 0o750 data dirs (utils.EnsureDirs) return "", false, fmt.Errorf("chmod data dir: %w", err) } // Durable before kill: the VMM dies once we return and a bare rename fsyncs nothing, so sync files and dir entries before finalize. @@ -178,7 +175,7 @@ func (lf *LocalFile) CreateFromDir(ctx context.Context, cfg *types.SnapshotConfi // List returns all snapshots (excluding pending ones). func (lf *LocalFile) List(ctx context.Context) ([]*types.Snapshot, error) { var result []*types.Snapshot - return result, lf.view(ctx, func(t *snapTx) error { + err := lf.view(ctx, func(t *snapTx) error { return t.Scan(func(_ string, rec *snapshotRecord) error { if rec.Pending { return nil @@ -187,6 +184,7 @@ func (lf *LocalFile) List(ctx context.Context) ([]*types.Snapshot, error) { return nil }) }) + return result, err } func (lf *LocalFile) Inspect(ctx context.Context, ref string) (*types.Snapshot, error) { @@ -197,7 +195,7 @@ func (lf *LocalFile) Inspect(ctx context.Context, ref string) (*types.Snapshot, return new(rec.Snapshot), nil } -// Delete removes each ref (rm dir → DB update); a mid-loop rm-OK-then-DB-fail leaves a stale DB record for GC. +// Delete removes each ref under its tombstone; a failure leaves that tombstone for a retry or gc to finish. func (lf *LocalFile) Delete(ctx context.Context, refs []string) ([]string, error) { var ids []string if err := lf.view(ctx, func(t *snapTx) error { @@ -436,7 +434,9 @@ func (lf *LocalFile) lookupRecord(ctx context.Context, ref string, touch bool) ( return nil } if touch { - return rec, lf.touch(ctx, apply) + err := lf.touch(ctx, apply) + return rec, err } - return rec, lf.view(ctx, apply) + err := lf.view(ctx, apply) + return rec, err } diff --git a/snapshot/localfile/localfile_test.go b/snapshot/localfile/localfile_test.go index 76363f6a..ef1fd9d3 100644 --- a/snapshot/localfile/localfile_test.go +++ b/snapshot/localfile/localfile_test.go @@ -45,13 +45,6 @@ func TestNew(t *testing.T) { } } -func TestNew_NilConfig(t *testing.T) { - _, err := New(nil, metering.NopRecorder{}, nil) - if err == nil { - t.Fatal("expected error for nil config") - } -} - func TestCreateAndDeleteEmitMetering(t *testing.T) { rec := meteringcapture.New() lf := newTestLFWithRecorder(t, rec) diff --git a/snapshot/localfile/teardown.go b/snapshot/localfile/teardown.go index 6fd1ac29..c746178d 100644 --- a/snapshot/localfile/teardown.go +++ b/snapshot/localfile/teardown.go @@ -57,7 +57,7 @@ func (lf *LocalFile) deleteSnapshotProtocol(ctx context.Context, id string, reva if mErr != nil { return tombstone.Payload{}, mErr } - return tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cleanup}, nil + return tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cleanup}, nil }) if err != nil || resumed == nil { return err diff --git a/snapshot/localfile/teardown_test.go b/snapshot/localfile/teardown_test.go index bd57baca..b44f08b4 100644 --- a/snapshot/localfile/teardown_test.go +++ b/snapshot/localfile/teardown_test.go @@ -128,7 +128,7 @@ func injectSnapTombstone(t *testing.T, lf *LocalFile, id string, phase tombstone if err != nil { return err } - leaseID, err := ts.Lease(ctx, tx.Writer(), id, tombstone.Payload{Kind: tombstone.KindRecord, Mode: tombstone.ModeAggregate, Cleanup: cl}) + leaseID, err := ts.Lease(ctx, tx.Writer(), id, tombstone.Payload{Mode: tombstone.ModeAggregate, Cleanup: cl}) if err != nil { return err } diff --git a/snapshot/snapshot.go b/snapshot/snapshot.go index 4b542e63..d7546fad 100644 --- a/snapshot/snapshot.go +++ b/snapshot/snapshot.go @@ -45,7 +45,7 @@ type Snapshot interface { Create(ctx context.Context, cfg *types.SnapshotConfig, stream io.Reader) (string, error) List(ctx context.Context) ([]*types.Snapshot, error) Inspect(ctx context.Context, ref string) (*types.Snapshot, error) - // Delete removes snapshots by ID or name. Returns the list of actually deleted IDs. + // Delete removes snapshots by ID or name and returns the IDs it deleted. Delete(ctx context.Context, refs []string) ([]string, error) Restore(ctx context.Context, ref string) (types.SnapshotConfig, io.ReadCloser, error) diff --git a/types/vm.go b/types/vm.go index f38083c7..55a6a456 100644 --- a/types/vm.go +++ b/types/vm.go @@ -31,7 +31,7 @@ var ( validName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,62}$`) validSnapshotName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._:/-]{0,62}$`) validUsername = regexp.MustCompile(`^[a-z_][a-z0-9_-]{0,31}$`) - // shellUnsafe rejects chars that break the chpasswd YAML scalar in cidata. + // shellUnsafe rejects chars that break the cidata chpasswd scalar and the sh -c runcmd lines. shellUnsafe = regexp.MustCompile("[`$;|&(){}\\\\<>!'\"\\x00-\\x1f\\x7f]") ) @@ -102,13 +102,13 @@ type VM struct { State VMState `json:"state"` Config VMConfig `json:"config"` - // Runtime — populated only while State == VMStateRunning. + // Runtime — ToVM fills these only while State == VMStateRunning. PID int `json:"pid"` SocketPath string `json:"socket_path,omitempty"` // VMM API Unix socket VsockSocket string `json:"vsock_socket,omitempty"` // hybrid vsock UDS for cocoon-agent ConsolePath string `json:"console_path,omitempty"` // guest console: console.sock UDS or CH-allocated PTY (direct boot) - // Placement is held from launch until the VM leaves VMStateRunning; --cpuset-cpus auto records its cache domain as CPUSet. + // Placement is held from launch until the VM is Stopped; --cpuset-cpus auto records its cache domain as CPUSet. CPUSet string `json:"cpuset,omitempty"` QueueCPUs string `json:"queue_cpus,omitempty"` // host cpus the writable virtio-blk queue threads pin to @@ -133,9 +133,6 @@ type VM struct { } func (v *VM) ResolvedNetnsPath() string { - if v == nil { - return "" - } if nic := v.firstNIC(); nic != nil { return cmp.Or(v.NetnsPath, nic.NetnsPath) } @@ -143,9 +140,6 @@ func (v *VM) ResolvedNetnsPath() string { } func (v *VM) ResolvedNetBackend() string { - if v == nil { - return "" - } if nic := v.firstNIC(); nic != nil { return cmp.Or(v.NetBackend, nic.Backend, BackendCNI) } @@ -153,9 +147,6 @@ func (v *VM) ResolvedNetBackend() string { } func (v *VM) ResolvedNetBridgeDev() string { - if v == nil { - return "" - } if nic := v.firstNIC(); nic != nil { return cmp.Or(v.NetBridgeDev, nic.BridgeDev) } @@ -163,7 +154,7 @@ func (v *VM) ResolvedNetBridgeDev() string { } func (v *VM) firstNIC() *NetworkConfig { - if v == nil || len(v.NetworkConfigs) == 0 { + if len(v.NetworkConfigs) == 0 { return nil } return v.NetworkConfigs[0] diff --git a/types/vm_test.go b/types/vm_test.go index 0f054b3f..3789f279 100644 --- a/types/vm_test.go +++ b/types/vm_test.go @@ -267,19 +267,6 @@ func TestVMResolvedNetFields(t *testing.T) { } } -func TestVMResolvedNetNilReceiver(t *testing.T) { - var v *VM - if got := v.ResolvedNetnsPath(); got != "" { - t.Errorf("nil ResolvedNetnsPath = %q, want \"\"", got) - } - if got := v.ResolvedNetBackend(); got != "" { - t.Errorf("nil ResolvedNetBackend = %q, want \"\"", got) - } - if got := v.ResolvedNetBridgeDev(); got != "" { - t.Errorf("nil ResolvedNetBridgeDev = %q, want \"\"", got) - } -} - func validConfig() VMConfig { return VMConfig{ Name: "test-vm", diff --git a/utils/batch.go b/utils/batch.go index c660b6cf..de25bbac 100644 --- a/utils/batch.go +++ b/utils/batch.go @@ -26,7 +26,7 @@ func ForEach[T any](ctx context.Context, items []T, fn func(context.Context, T) for i, item := range items { g.Go(func() error { errs[i] = fn(gctx, item) - return nil // always nil so errgroup processes all items (best-effort) + return nil }) } _ = g.Wait()