Skip to content

Commit f245429

Browse files
committed
refactor: shared CH/FC backend templates and helper consolidation
Pull pause/resume, restore, and process-launch sequences into Backend methods on the hypervisor package so both backends call thin wrappers: - WithPausedVM (around CH/FC snapshot capture): centralizes pause → defer-resume → eager-resume-on-success bookkeeping. - RestoreSequence + DirectRestoreSequence: validate-CPU → resolve → optional override-check → preflight → kill → optional disk lock → merge-or-populate → after-extract. Backends supply only the differing hooks. FC's tar-stream and direct-dir paths now share both templates. - LaunchVMProcess: netns enter → exec.Cmd start → WritePIDFile → WaitForSocket, with deferred kill-and-cleanup on any error path. CH/FC start.go drop ~30 LOC of identical fork/wait scaffolding each. Also pull cross-cutting helpers into hypervisor/utils: CloneStorageConfigs, IsUnderDir, ValidateMetaPaths, LoadAndValidateMeta, ReverseLayers. CH now applies the same sidecar-path-trust check FC has had — defense in depth against tampered cocoon.json paths in imported snapshots. Smaller cleanups bundled in: - utils.MapValues + utils.PeekReader generic helpers (used by image index list, network list, vm list, gzip and qcow2 sniffers). - cmp.Or for trivial fallback assignments in cmd/core, cmd/vm, oci/boot. - EnsureVmlinux uses utils.AtomicWriteFile. - saveCmdline uses utils.AtomicWriteFile + cmdlineFileName constant. - LaunchSpec drops BinaryName/Timeout fields — derived from BackendConfig. - Sweep A renames: runtimeFiletoCLIArg → runtimeFileToCLIArg, fcPid → fcPID, GCStaleTemp/GCCollectBlobs moved above private helpers. - Bug fix: unwrapGzip no longer wraps a nil error when the stream is shorter than the gzip magic. Tests: PeekReader, MapValues, IsUnderDir, ValidateMetaPaths, CloneStorageConfigs, ReverseLayers. ValidateMetaPaths is security-sensitive (path traversal rejection) and gets dedicated cases for /etc/shadow, kernel-out-of-root, and initrd-out-of-root. make lint clean on linux + darwin; make test passes (utils coverage 82.5 → was 79.8; hypervisor 7.2 → 3.6).
1 parent 70e58d1 commit f245429

30 files changed

Lines changed: 804 additions & 487 deletions

cmd/core/helpers.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,7 @@ func EnsureImage(ctx context.Context, backends []imagebackend.Images, vmCfg *typ
233233
logger := log.WithFunc("core.EnsureImage")
234234

235235
// Use digest for lookup when available; fall back to tag/URL.
236-
lookupRef := vmCfg.ImageDigest
237-
if lookupRef == "" {
238-
lookupRef = vmCfg.Image
239-
}
236+
lookupRef := cmp.Or(vmCfg.ImageDigest, vmCfg.Image)
240237

241238
for _, b := range backends {
242239
if b.Type() != vmCfg.ImageType {
@@ -693,9 +690,7 @@ func normalizeDataDiskSpecs(specs []types.DataDiskSpec) error {
693690
}
694691
autoIdx := 1
695692
for i := range specs {
696-
if specs[i].FSType == "" {
697-
specs[i].FSType = types.FSTypeExt4
698-
}
693+
specs[i].FSType = cmp.Or(specs[i].FSType, types.FSTypeExt4)
699694
if specs[i].FSType != types.FSTypeExt4 && specs[i].FSType != types.FSTypeNone {
700695
return fmt.Errorf("--data-disk: invalid fstype %q", specs[i].FSType)
701696
}

cmd/vm/run.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package vm
22

33
import (
4+
"cmp"
45
"context"
56
"fmt"
67
"slices"
@@ -265,10 +266,8 @@ func (h Handler) prepareClone(ctx context.Context, cmd *cobra.Command, conf *con
265266
}
266267
}
267268

268-
nics, _ := cmd.Flags().GetInt("nics")
269-
if nics == 0 {
270-
nics = cfg.NICs
271-
}
269+
nicsFlag, _ := cmd.Flags().GetInt("nics")
270+
nics := cmp.Or(nicsFlag, cfg.NICs)
272271
if nics < cfg.NICs {
273272
return nil, "", nil, nil, fmt.Errorf("--nics %d below snapshot minimum %d", nics, cfg.NICs)
274273
}

hypervisor/backend.go

Lines changed: 227 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"io/fs"
99
"maps"
1010
"os"
11+
"os/exec"
1112
"path/filepath"
1213
"time"
1314

@@ -50,6 +51,7 @@ type BackendConfig interface {
5051
BinaryName() string
5152
PIDFileName() string
5253
TerminateGracePeriod() time.Duration
54+
SocketWaitTimeout() time.Duration
5355
EffectivePoolSize() int
5456
IndexFile() string
5557
RunDir() string
@@ -85,12 +87,7 @@ func (b *Backend) Inspect(ctx context.Context, ref string) (*types.VM, error) {
8587
func (b *Backend) List(ctx context.Context) ([]*types.VM, error) {
8688
var result []*types.VM
8789
return result, b.DB.With(ctx, func(idx *VMIndex) error {
88-
for _, rec := range idx.VMs {
89-
if rec == nil {
90-
continue
91-
}
92-
result = append(result, b.ToVM(rec))
93-
}
90+
result = utils.MapValues(idx.VMs, b.ToVM)
9491
return nil
9592
})
9693
}
@@ -147,6 +144,41 @@ func (b *Backend) WithRunningVM(ctx context.Context, rec *VMRecord, fn func(pid
147144
return fn(pid)
148145
}
149146

147+
// WithPausedVM pauses a running VM, runs fn, then resumes it. Resume is
148+
// guaranteed via defer: if fn returns an error the resume failure is logged
149+
// and the inner error is propagated; if fn succeeds the resume error is
150+
// promoted to the return value (snapshot data is captured but the VM is
151+
// stuck). Both backends use this around snapshot capture.
152+
func (b *Backend) WithPausedVM(ctx context.Context, rec *VMRecord, pause, resume, fn func() error) error {
153+
return b.WithRunningVM(ctx, rec, func(_ int) error {
154+
if err := pause(); err != nil {
155+
return fmt.Errorf("pause: %w", err)
156+
}
157+
var resumed bool
158+
var resumeErr error
159+
logger := log.WithFunc(b.Typ + ".WithPausedVM")
160+
doResume := func() {
161+
if resumed {
162+
return
163+
}
164+
resumed = true
165+
resumeErr = resume()
166+
if resumeErr != nil {
167+
logger.Warnf(ctx, "resume VM %s: %v", rec.ID, resumeErr)
168+
}
169+
}
170+
defer doResume()
171+
if err := fn(); err != nil {
172+
return err
173+
}
174+
doResume()
175+
if resumeErr != nil {
176+
return fmt.Errorf("snapshot data captured but resume failed: %w", resumeErr)
177+
}
178+
return nil
179+
})
180+
}
181+
150182
func (b *Backend) UpdateStates(ctx context.Context, ids []string, state types.VMState) error {
151183
if len(ids) == 0 {
152184
return nil
@@ -633,6 +665,66 @@ func SocketPath(runDir string) string { return filepath.Join(runDir, APISocketNa
633665

634666
func ConsoleSockPath(runDir string) string { return filepath.Join(runDir, ConsoleSockName) }
635667

668+
// LaunchSpec carries the per-call inputs to LaunchVMProcess. The shared
669+
// BinaryName / SocketWaitTimeout come from the Backend's BackendConfig and
670+
// don't repeat here.
671+
type LaunchSpec struct {
672+
Cmd *exec.Cmd
673+
PIDPath string
674+
SockPath string
675+
NetnsPath string // empty = host netns
676+
OnFail func() // optional cleanup hook on any error path
677+
}
678+
679+
// LaunchVMProcess optionally enters a netns, starts spec.Cmd, writes the PID
680+
// file, and waits for the API socket. On any post-Start error the process is
681+
// killed and the PID file is removed before returning. The caller is
682+
// responsible for reaping cmd via cmd.Wait() in a goroutine on success.
683+
func (b *Backend) LaunchVMProcess(ctx context.Context, spec LaunchSpec) (pid int, err error) {
684+
started := false
685+
pidWritten := false
686+
binaryName := b.Conf.BinaryName()
687+
defer func() {
688+
if err == nil {
689+
return
690+
}
691+
if started {
692+
_ = spec.Cmd.Process.Kill()
693+
_ = spec.Cmd.Wait()
694+
}
695+
if pidWritten {
696+
_ = os.Remove(spec.PIDPath)
697+
}
698+
if spec.OnFail != nil {
699+
spec.OnFail()
700+
}
701+
}()
702+
703+
if spec.NetnsPath != "" {
704+
restore, nsErr := EnterNetns(spec.NetnsPath)
705+
if nsErr != nil {
706+
return 0, fmt.Errorf("enter netns: %w", nsErr)
707+
}
708+
defer restore()
709+
}
710+
711+
if err = spec.Cmd.Start(); err != nil {
712+
return 0, fmt.Errorf("exec %s: %w", binaryName, err)
713+
}
714+
started = true
715+
pid = spec.Cmd.Process.Pid
716+
717+
if err = utils.WritePIDFile(spec.PIDPath, pid); err != nil {
718+
return 0, fmt.Errorf("write PID file: %w", err)
719+
}
720+
pidWritten = true
721+
722+
if err = WaitForSocket(ctx, spec.SockPath, pid, b.Conf.SocketWaitTimeout(), binaryName); err != nil {
723+
return 0, err
724+
}
725+
return pid, nil
726+
}
727+
636728
// PrepareStagingDir creates a sibling staging directory, extracts the snapshot
637729
// tar into it, and returns a cleanup function that removes the staging dir.
638730
func PrepareStagingDir(runDir string, snapshot io.Reader) (stagingDir string, cleanup func(), err error) {
@@ -650,3 +742,132 @@ func PrepareStagingDir(runDir string, snapshot io.Reader) (stagingDir string, cl
650742
}
651743
return stagingDir, cleanup, nil
652744
}
745+
746+
// RestoreSpec carries the backend-specific hooks for RestoreSequence. The
747+
// shared template handles host-CPU validation, ResolveForRestore, staging,
748+
// preflight, kill, and merge — backends supply only the differing pieces.
749+
type RestoreSpec struct {
750+
VMCfg *types.VMConfig
751+
Snapshot io.Reader
752+
OverrideCheck func(rec *VMRecord, vmCfg *types.VMConfig) error // optional pre-kill resource validation
753+
Preflight func(stagingDir string, rec *VMRecord) error
754+
Kill func(ctx context.Context, vmID string, rec *VMRecord) error
755+
Wrap func(rec *VMRecord, fn func() error) error // optional disk lock wrapping merge+afterExtract
756+
BeforeMerge func(rec *VMRecord) error // optional pre-merge hook (e.g. FC removes stale COW)
757+
AfterExtract func(ctx context.Context, vmID string, vmCfg *types.VMConfig, rec *VMRecord) (*types.VM, error)
758+
}
759+
760+
// DirectRestoreSpec mirrors RestoreSpec but takes a local source directory
761+
// rather than a streaming tar. Populate replaces the staging+merge step:
762+
// backends decide how to copy the snapshot files into rec.RunDir (typically
763+
// clean old artifacts, then per-file hardlink/reflink/copy).
764+
type DirectRestoreSpec struct {
765+
VMCfg *types.VMConfig
766+
SrcDir string
767+
OverrideCheck func(rec *VMRecord, vmCfg *types.VMConfig) error
768+
Preflight func(srcDir string, rec *VMRecord) error
769+
Kill func(ctx context.Context, vmID string, rec *VMRecord) error
770+
Wrap func(rec *VMRecord, fn func() error) error
771+
Populate func(rec *VMRecord, srcDir string) error
772+
AfterExtract func(ctx context.Context, vmID string, vmCfg *types.VMConfig, rec *VMRecord) (*types.VM, error)
773+
}
774+
775+
// RestoreSequence runs the shared restore flow used by both backends:
776+
// validate host CPU, resolve VM, stage tar, preflight, kill, merge, then
777+
// hand off to AfterExtract for backend-specific resume.
778+
func (b *Backend) RestoreSequence(ctx context.Context, vmRef string, spec RestoreSpec) (*types.VM, error) {
779+
if err := ValidateHostCPU(spec.VMCfg.CPU); err != nil {
780+
return nil, err
781+
}
782+
vmID, rec, err := b.ResolveForRestore(ctx, vmRef)
783+
if err != nil {
784+
return nil, err
785+
}
786+
if spec.OverrideCheck != nil {
787+
if checkErr := spec.OverrideCheck(rec, spec.VMCfg); checkErr != nil {
788+
return nil, checkErr
789+
}
790+
}
791+
792+
stagingDir, cleanupStaging, err := PrepareStagingDir(rec.RunDir, spec.Snapshot)
793+
if err != nil {
794+
return nil, err
795+
}
796+
defer cleanupStaging()
797+
798+
if preflightErr := spec.Preflight(stagingDir, rec); preflightErr != nil {
799+
return nil, fmt.Errorf("snapshot preflight: %w", preflightErr)
800+
}
801+
if killErr := spec.Kill(ctx, vmID, rec); killErr != nil {
802+
return nil, killErr
803+
}
804+
805+
var result *types.VM
806+
inner := func() error {
807+
if spec.BeforeMerge != nil {
808+
if err := spec.BeforeMerge(rec); err != nil {
809+
return err
810+
}
811+
}
812+
if mergeErr := MergeDirInto(stagingDir, rec.RunDir); mergeErr != nil {
813+
b.MarkError(ctx, vmID)
814+
return fmt.Errorf("apply staged snapshot: %w", mergeErr)
815+
}
816+
var afterErr error
817+
result, afterErr = spec.AfterExtract(ctx, vmID, spec.VMCfg, rec)
818+
return afterErr
819+
}
820+
if spec.Wrap != nil {
821+
if err := spec.Wrap(rec, inner); err != nil {
822+
return nil, err
823+
}
824+
} else if err := inner(); err != nil {
825+
return nil, err
826+
}
827+
return result, nil
828+
}
829+
830+
// DirectRestoreSequence runs the local-snapshot-dir variant of restore: the
831+
// caller supplies a srcDir already on disk. Same outage-safe ordering as
832+
// RestoreSequence (preflight before kill, optional disk lock around populate
833+
// + afterExtract).
834+
func (b *Backend) DirectRestoreSequence(ctx context.Context, vmRef string, spec DirectRestoreSpec) (*types.VM, error) {
835+
if err := ValidateHostCPU(spec.VMCfg.CPU); err != nil {
836+
return nil, err
837+
}
838+
vmID, rec, err := b.ResolveForRestore(ctx, vmRef)
839+
if err != nil {
840+
return nil, err
841+
}
842+
if spec.OverrideCheck != nil {
843+
if checkErr := spec.OverrideCheck(rec, spec.VMCfg); checkErr != nil {
844+
return nil, checkErr
845+
}
846+
}
847+
848+
if preflightErr := spec.Preflight(spec.SrcDir, rec); preflightErr != nil {
849+
return nil, fmt.Errorf("snapshot preflight: %w", preflightErr)
850+
}
851+
if killErr := spec.Kill(ctx, vmID, rec); killErr != nil {
852+
return nil, killErr
853+
}
854+
855+
var result *types.VM
856+
inner := func() error {
857+
if populateErr := spec.Populate(rec, spec.SrcDir); populateErr != nil {
858+
b.MarkError(ctx, vmID)
859+
return populateErr
860+
}
861+
var afterErr error
862+
result, afterErr = spec.AfterExtract(ctx, vmID, spec.VMCfg, rec)
863+
return afterErr
864+
}
865+
if spec.Wrap != nil {
866+
if wrapErr := spec.Wrap(rec, inner); wrapErr != nil {
867+
return nil, wrapErr
868+
}
869+
} else if innerErr := inner(); innerErr != nil {
870+
return nil, innerErr
871+
}
872+
return result, nil
873+
}

hypervisor/cloudhypervisor/args.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,10 +151,10 @@ func buildCLIArgs(cfg *chVMConfig, socketPath string) []string {
151151
}
152152

153153
if cfg.Serial != nil {
154-
args = append(args, "--serial", runtimeFiletoCLIArg(cfg.Serial))
154+
args = append(args, "--serial", runtimeFileToCLIArg(cfg.Serial))
155155
}
156156
if cfg.Console != nil {
157-
args = append(args, "--console", runtimeFiletoCLIArg(cfg.Console))
157+
args = append(args, "--console", runtimeFileToCLIArg(cfg.Console))
158158
}
159159

160160
return args
@@ -248,7 +248,7 @@ func balloonToCLIArg(b *chBalloon) string {
248248
return args.String()
249249
}
250250

251-
func runtimeFiletoCLIArg(c *chRuntimeFile) string {
251+
func runtimeFileToCLIArg(c *chRuntimeFile) string {
252252
switch strings.ToLower(c.Mode) {
253253
case "file":
254254
return "file=" + c.File

hypervisor/cloudhypervisor/clone.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func (ch *CloudHypervisor) cloneAfterExtract(ctx context.Context, vmID string, v
4747
return nil, fmt.Errorf("parse CH config: %w", err)
4848
}
4949

50-
meta, err := hypervisor.LoadSnapshotMeta(runDir)
50+
meta, err := hypervisor.LoadAndValidateMeta(runDir, ch.conf.RootDir, ch.conf.Config.RunDir)
5151
if err != nil {
5252
return nil, fmt.Errorf("load snapshot meta: %w", err)
5353
}

0 commit comments

Comments
 (0)