diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0c0c0e60e8..7d665dc01f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -312,6 +312,14 @@ sequenceDiagram orchestrator pauses the VM, snapshots it, diffs memory (dirty-page tracking) and rootfs (COW cache) against the template, caches the snapshot locally, and uploads asynchronously to object storage (with a retry budget). The sandbox leaves the Redis catalog. + - **Deferred rootfs export** (gated by the `deferred-rootfs-export` flag in + `packages/shared/pkg/featureflags`): instead of diffing the rootfs on the pause critical + path, the orchestrator ejects the writable COW cache during pause and returns, then seals it + into the rootfs diff (reflink) in the background. This moves the rootfs-diff latency off the + pause, but the local snapshot's rootfs body isn't materialized until the seal finishes, so the + async upload — and any origin-node resume/prefetch that reads the rootfs diff — waits on the + seal. A seal failure is permanent (it never re-runs), so the upload fails fast rather than + retrying. - **Resume**: same path as creation, but placement prefers the **origin node** — if the snapshot is still in its local cache, resume avoids any object-storage reads. `Checkpoint` is a pause+resume in place used to persist state while keeping the sandbox running. diff --git a/packages/orchestrator/pkg/sandbox/block/cache.go b/packages/orchestrator/pkg/sandbox/block/cache.go index e1f555423f..b0d50809e4 100644 --- a/packages/orchestrator/pkg/sandbox/block/cache.go +++ b/packages/orchestrator/pkg/sandbox/block/cache.go @@ -106,18 +106,62 @@ func (c *Cache) isClosed() bool { return c.closed.Load() } +// DiffMetadata returns the dirty/empty diff metadata from the tracker without +// copying any block data. It lets a deferred/background seal build the diff +// header (and scheduling metadata) synchronously while the actual reflink copy +// (ExportToDiff) runs off the critical path. The result matches what +// ExportToDiff computes internally, provided the cache is frozen (no writes) in +// between — which is guaranteed once the sandbox has been stopped and the cache +// ejected. +func (c *Cache) DiffMetadata() (*header.DiffMetadata, error) { + c.mu.RLock() + defer c.mu.RUnlock() + + if c.isClosed() { + return nil, NewErrCacheClosed(c.filePath) + } + + if c.mmap == nil { + return header.NewDiffMetadata(c.blockSize, nil, nil), nil + } + + dirty, empty := c.tracker.Export() + + return header.NewDiffMetadata(c.blockSize, dirty, empty), nil +} + func (c *Cache) ExportToDiff(ctx context.Context, out *os.File) (*header.DiffMetadata, error) { - ctx, childSpan := tracer.Start(ctx, "export-to-diff") - defer childSpan.End() + c.mu.Lock() + defer c.mu.Unlock() + return c.exportToDiffLocked(ctx, out, nil) +} + +// ExportToDiffWithMetadata copies the dirty ranges described by meta to out, +// using meta's bitmap instead of re-reading the tracker. Callers that captured a +// DiffMetadata earlier (e.g. the deferred rootfs seal, which reads it at setup +// and exports later in the background) use this so the copied ranges are +// guaranteed to match a header built from the same bitmap read. +func (c *Cache) ExportToDiffWithMetadata(ctx context.Context, out *os.File, meta *header.DiffMetadata) (*header.DiffMetadata, error) { c.mu.Lock() defer c.mu.Unlock() + return c.exportToDiffLocked(ctx, out, meta) +} + +func (c *Cache) exportToDiffLocked(ctx context.Context, out *os.File, meta *header.DiffMetadata) (*header.DiffMetadata, error) { + ctx, childSpan := tracer.Start(ctx, "export-to-diff") + defer childSpan.End() + if c.isClosed() { return nil, NewErrCacheClosed(c.filePath) } if c.mmap == nil { + if meta != nil { + return meta, nil + } + return header.NewDiffMetadata(c.blockSize, nil, nil), nil } @@ -137,8 +181,11 @@ func (c *Cache) ExportToDiff(ctx context.Context, out *os.File) (*header.DiffMet logger.L().Warn(ctx, "error syncing file", zap.Error(err)) } - dirty, empty := c.tracker.Export() - diffMetadata := header.NewDiffMetadata(c.blockSize, dirty, empty) + diffMetadata := meta + if diffMetadata == nil { + dirty, empty := c.tracker.Export() + diffMetadata = header.NewDiffMetadata(c.blockSize, dirty, empty) + } dst := int(out.Fd()) var writeOffset int64 diff --git a/packages/orchestrator/pkg/sandbox/block/cache_diffmetadata_test.go b/packages/orchestrator/pkg/sandbox/block/cache_diffmetadata_test.go new file mode 100644 index 0000000000..567d540277 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/block/cache_diffmetadata_test.go @@ -0,0 +1,55 @@ +//go:build linux + +package block + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/e2b-dev/infra/packages/shared/pkg/storage/header" +) + +// TestCacheDiffMetadata_MatchesExportToDiff verifies DiffMetadata (used to build +// the diff header synchronously) produces the same dirty/empty bitmaps as the +// metadata ExportToDiff computes while copying, so a background seal's header is +// exact. +func TestCacheDiffMetadata_MatchesExportToDiff(t *testing.T) { + t.Parallel() + + blockSize := int64(header.PageSize) + numBlocks := int64(4) + + c, err := NewCache(blockSize*numBlocks, blockSize, t.TempDir()+"/cache", false) + require.NoError(t, err) + t.Cleanup(func() { _ = c.Close() }) + + // Dirty block 0, zero block 2, leave 1 and 3 untouched. + dirtyBlock := make([]byte, blockSize) + for i := range dirtyBlock { + dirtyBlock[i] = 0xAB + } + _, err = c.WriteAt(dirtyBlock, 0) + require.NoError(t, err) + _, err = c.WriteZeroesAt(2*blockSize, blockSize) + require.NoError(t, err) + + // Metadata read without copying. + meta, err := c.DiffMetadata() + require.NoError(t, err) + + // Metadata computed by the actual export. + out, err := os.CreateTemp(t.TempDir(), "diff-*") + require.NoError(t, err) + t.Cleanup(func() { _ = out.Close() }) + + exportMeta, err := c.ExportToDiff(t.Context(), out) + require.NoError(t, err) + + require.True(t, meta.Dirty.Equals(exportMeta.Dirty), "dirty bitmaps must match") + require.True(t, meta.Empty.Equals(exportMeta.Empty), "empty bitmaps must match") + require.Equal(t, exportMeta.BlockSize, meta.BlockSize) + require.EqualValues(t, 1, meta.Dirty.GetCardinality()) + require.EqualValues(t, 1, meta.Empty.GetCardinality()) +} diff --git a/packages/orchestrator/pkg/sandbox/build/cache.go b/packages/orchestrator/pkg/sandbox/build/cache.go index 0f3202cb31..daab0ee01f 100644 --- a/packages/orchestrator/pkg/sandbox/build/cache.go +++ b/packages/orchestrator/pkg/sandbox/build/cache.go @@ -98,6 +98,22 @@ func NewDiffStore( // buildData will be deleted by calling buildData.Close() defer ds.resetDelete(item.Key()) + // A deferred diff whose background seal hasn't resolved would block Close() + // on the reflink; close it off the eviction goroutine so this callback + // isn't stalled. Bounded (the seal always resolves) and only reachable if + // the TTL is shortened below the seal time — the disk-pressure eviction + // path already skips unsealed diffs (see deferredDiff.sealed()). + if dd, ok := buildData.(*deferredDiff); ok && !dd.sealed() { + logCtx := context.WithoutCancel(ctx) + go func() { + if closeErr := dd.Close(); closeErr != nil { + logger.L().Warn(logCtx, "failed to close unsealed deferred diff", zap.Any("item_key", item.Key()), zap.Error(closeErr)) + } + }() + + return + } + if closeErr := buildData.Close(); closeErr != nil { logger.L().Warn(ctx, "failed to cleanup build data cache for item", zap.Any("item_key", item.Key()), zap.Error(closeErr)) } @@ -311,6 +327,14 @@ func (s *DiffStore) deleteOldestFromCache(ctx context.Context) (suc bool, e erro return true } + // Skip a deferred diff whose background rootfs seal hasn't resolved yet: + // FileSize below would block on the seal (stalling the sole eviction + // goroutine), and a fresh, still-sealing snapshot is exactly what a + // just-resumed peer needs. It becomes evictable once the seal resolves. + if dd, ok := item.Value().(*deferredDiff); ok && !dd.sealed() { + return true + } + sfSize, err := item.Value().FileSize(ctx) if err != nil { logger.L().Warn(ctx, "failed to get size of deleted item from cache", zap.Error(err)) diff --git a/packages/orchestrator/pkg/sandbox/build/cache_test.go b/packages/orchestrator/pkg/sandbox/build/cache_test.go index 092e2c84ca..fb6ea890fa 100644 --- a/packages/orchestrator/pkg/sandbox/build/cache_test.go +++ b/packages/orchestrator/pkg/sandbox/build/cache_test.go @@ -31,6 +31,7 @@ import ( blockmetrics "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/block/metrics" "github.com/e2b-dev/infra/packages/shared/pkg/featureflags" "github.com/e2b-dev/infra/packages/shared/pkg/storage/header" + "github.com/e2b-dev/infra/packages/shared/pkg/utils" ) const ( @@ -716,3 +717,43 @@ func mustParseCfg(t *testing.T) cfg.Config { return c } + +// A deferred rootfs diff whose background seal hasn't resolved must be skipped +// by disk-pressure eviction: its data methods (FileSize) block on the seal, so +// evicting it would stall the sole eviction goroutine, and a fresh, still-sealing +// snapshot is exactly what a just-resumed peer needs. Once the seal resolves the +// entry becomes evictable like any other. +func TestDiffStoreEvictionSkipsUnsealedDeferredDiff(t *testing.T) { + t.Parallel() + cachePath := t.TempDir() + + c, err := cfg.Parse() + require.NoError(t, err) + flags := flagsWithMaxBuildCachePercentage(t, 100) + store, err := NewDiffStore(c, flags, cachePath, 60*time.Second, 4*time.Second) + require.NoError(t, err) + + // Oldest entry: a deferred rootfs diff whose background seal hasn't resolved. + promise := utils.NewSetOnce[Diff]() + deferred := NewDeferredDiff(GetDiffStoreKey("deferred-unsealed", Rootfs), blockSize, promise) + store.Add(deferred) + + // A newer, ordinary diff behind it. + newer := newRootFSDiff(t, cachePath, "seal-newer") + store.Add(newer) + + // Eviction must skip the unsealed deferred diff and fall through to the + // next-oldest instead of blocking on the seal. + ok, err := store.deleteOldestFromCache(t.Context()) + require.NoError(t, err) + assert.True(t, ok) + assert.False(t, store.isBeingDeleted(deferred.CacheKey()), "unsealed deferred diff must be skipped") + assert.True(t, store.isBeingDeleted(newer.CacheKey()), "eviction must fall through to the next-oldest") + + // Once the seal resolves, the deferred diff is evictable like any other entry. + require.NoError(t, promise.SetValue(newRootFSDiff(t, cachePath, "seal-resolved"))) + ok, err = store.deleteOldestFromCache(t.Context()) + require.NoError(t, err) + assert.True(t, ok) + assert.True(t, store.isBeingDeleted(deferred.CacheKey()), "sealed deferred diff must be evictable") +} diff --git a/packages/orchestrator/pkg/sandbox/build/deferred_diff.go b/packages/orchestrator/pkg/sandbox/build/deferred_diff.go new file mode 100644 index 0000000000..fa388db16e --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/build/deferred_diff.go @@ -0,0 +1,123 @@ +//go:build linux + +package build + +import ( + "context" + "errors" + + "github.com/e2b-dev/infra/packages/shared/pkg/storage" + "github.com/e2b-dev/infra/packages/shared/pkg/utils" +) + +// ErrDeferredSealFailed marks a deferred rootfs seal that failed. The seal runs +// exactly once, so the failure is permanent: the promise stays settled with this +// error and every subsequent data-bearing method on the diff returns it. Callers +// such as the pause-upload retry loop match on it to stop retrying a diff that +// can never materialize (rather than burning the whole retry budget on it). +var ErrDeferredSealFailed = errors.New("deferred rootfs seal failed") + +// deferredDiff is a Diff whose backing data is produced asynchronously. It is +// returned synchronously from a pause that seals the rootfs in the background: +// the cache key and block size are known up front (so DiffStore.Add and the +// upload's compress-config validation work immediately), while every data- +// bearing method blocks on the inner promise until the background seal resolves +// the real Diff. +// +// The producer MUST always resolve the promise — with the sealed Diff on success +// or an error on failure — otherwise the data methods (and Close) block forever. +type deferredDiff struct { + cacheKey DiffStoreKey + blockSize int64 + inner *utils.SetOnce[Diff] +} + +var _ Diff = (*deferredDiff)(nil) + +// NewDeferredDiff wraps a promise of a Diff. cacheKey and blockSize are the +// synchronously-known identity of the diff; inner is resolved by the background +// sealer with the materialized Diff (or an error). +func NewDeferredDiff(cacheKey DiffStoreKey, blockSize int64, inner *utils.SetOnce[Diff]) Diff { + return &deferredDiff{ + cacheKey: cacheKey, + blockSize: blockSize, + inner: inner, + } +} + +func (d *deferredDiff) CacheKey() DiffStoreKey { + return d.cacheKey +} + +func (d *deferredDiff) BlockSize() int64 { + return d.blockSize +} + +// sealed reports, without blocking, whether the background seal has resolved the +// promise (with the materialized diff or an error). Cache eviction uses it to +// skip a not-yet-sealed diff rather than block on the data methods (which wait +// on the seal) or evict a fresh, still-in-flight snapshot. +func (d *deferredDiff) sealed() bool { + select { + case <-d.inner.Done: + return true + default: + return false + } +} + +func (d *deferredDiff) CachePath(ctx context.Context) (string, error) { + inner, err := d.inner.WaitWithContext(ctx) + if err != nil { + return "", err + } + + return inner.CachePath(ctx) +} + +func (d *deferredDiff) ReadAt(ctx context.Context, p []byte, off int64, ft *storage.FrameTable) (int, error) { + inner, err := d.inner.WaitWithContext(ctx) + if err != nil { + return 0, err + } + + return inner.ReadAt(ctx, p, off, ft) +} + +func (d *deferredDiff) Slice(ctx context.Context, off, length int64, ft *storage.FrameTable) ([]byte, error) { + inner, err := d.inner.WaitWithContext(ctx) + if err != nil { + return nil, err + } + + return inner.Slice(ctx, off, length, ft) +} + +func (d *deferredDiff) Size(ctx context.Context) (int64, error) { + inner, err := d.inner.WaitWithContext(ctx) + if err != nil { + return 0, err + } + + return inner.Size(ctx) +} + +func (d *deferredDiff) FileSize(ctx context.Context) (int64, error) { + inner, err := d.inner.WaitWithContext(ctx) + if err != nil { + return 0, err + } + + return inner.FileSize(ctx) +} + +// Close waits for the seal to resolve and closes the materialized diff. If the +// seal failed there is nothing to close (the producer cleans up the partial file +// on error), so only close when the diff actually materialized. +func (d *deferredDiff) Close() error { + if inner, err := d.inner.Wait(); err == nil { + return inner.Close() + } + + return nil +} diff --git a/packages/orchestrator/pkg/sandbox/build/deferred_diff_test.go b/packages/orchestrator/pkg/sandbox/build/deferred_diff_test.go new file mode 100644 index 0000000000..aefaefb425 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/build/deferred_diff_test.go @@ -0,0 +1,107 @@ +//go:build linux + +package build + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/e2b-dev/infra/packages/shared/pkg/utils" +) + +// TestDeferredDiff_SealedIsNonBlocking verifies sealed() reports readiness +// without blocking on the promise, so cache eviction can skip an unsealed diff +// instead of stalling on it. +func TestDeferredDiff_SealedIsNonBlocking(t *testing.T) { + t.Parallel() + + inner := utils.NewSetOnce[Diff]() + d := NewDeferredDiff(GetDiffStoreKey("build-id", Rootfs), 4096, inner).(*deferredDiff) + + require.False(t, d.sealed(), "unresolved promise must read as not sealed") + + require.NoError(t, inner.SetValue(&NoDiff{})) + require.True(t, d.sealed(), "resolved promise must read as sealed") +} + +// makeLocalDiff writes `data` to a fresh local diff file and materializes it. +func makeLocalDiff(t *testing.T, blockSize int64, data []byte) Diff { + t.Helper() + + f, err := NewLocalDiffFile(t.TempDir(), "build-id", Rootfs) + require.NoError(t, err) + _, err = f.WriteAt(data, 0) + require.NoError(t, err) + + d, err := f.CloseToDiff(blockSize) + require.NoError(t, err) + + return d +} + +// TestDeferredDiff_IdentityIsSynchronous verifies CacheKey and BlockSize resolve +// without blocking on the (still-unresolved) inner promise, so DiffStore.Add and +// the upload's compress-config validation work the moment the diff is created. +func TestDeferredDiff_IdentityIsSynchronous(t *testing.T) { + t.Parallel() + + inner := utils.NewSetOnce[Diff]() + key := GetDiffStoreKey("build-id", Rootfs) + d := NewDeferredDiff(key, 4096, inner) + + require.Equal(t, key, d.CacheKey()) + require.EqualValues(t, 4096, d.BlockSize()) +} + +// TestDeferredDiff_DelegatesAfterResolve verifies the data methods block until +// the promise resolves, then delegate to the materialized diff. +func TestDeferredDiff_DelegatesAfterResolve(t *testing.T) { + t.Parallel() + + blockSize := int64(4096) + data := make([]byte, blockSize) + for i := range data { + data[i] = 0x5A + } + + materialized := makeLocalDiff(t, blockSize, data) + t.Cleanup(func() { _ = materialized.Close() }) + + inner := utils.NewSetOnce[Diff]() + d := NewDeferredDiff(materialized.CacheKey(), blockSize, inner) + + // Resolve on a goroutine; the reads below block until it lands. + go func() { _ = inner.SetValue(materialized) }() + + path, err := d.CachePath(t.Context()) + require.NoError(t, err) + require.NotEmpty(t, path) + + sz, err := d.Size(t.Context()) + require.NoError(t, err) + require.Equal(t, blockSize, sz) + + buf := make([]byte, blockSize) + _, err = d.ReadAt(t.Context(), buf, 0, nil) + require.NoError(t, err) + require.Equal(t, data, buf) +} + +// TestDeferredDiff_PropagatesError verifies a failed seal surfaces through the +// data methods, and Close is a no-op (the producer cleans up the partial file). +func TestDeferredDiff_PropagatesError(t *testing.T) { + t.Parallel() + + inner := utils.NewSetOnce[Diff]() + d := NewDeferredDiff(GetDiffStoreKey("build-id", Rootfs), 4096, inner) + + sealErr := errors.New("seal failed") + go func() { _ = inner.SetError(sealErr) }() + + _, err := d.CachePath(t.Context()) + require.ErrorIs(t, err, sealErr) + + require.NoError(t, d.Close(), "Close is a no-op when the seal failed") +} diff --git a/packages/orchestrator/pkg/sandbox/build/local_diff.go b/packages/orchestrator/pkg/sandbox/build/local_diff.go index 8417b86265..ee02a04af9 100644 --- a/packages/orchestrator/pkg/sandbox/build/local_diff.go +++ b/packages/orchestrator/pkg/sandbox/build/local_diff.go @@ -53,9 +53,21 @@ func (f *LocalDiffFile) Close() error { func (f *LocalDiffFile) CloseToDiff( blockSize int64, -) (Diff, error) { +) (d Diff, e error) { defer f.File.Close() + // On any failure to produce a usable Diff (e.g. an fsync/stat error after the + // bytes were written), remove the partial cache file. Nothing registers it in + // the DiffStore, so otherwise it orphans in the cache dir until process restart + // — disk-pressure eviction can't reclaim a file it doesn't know about. + defer func() { + if e != nil { + if rmErr := os.Remove(f.cachePath); rmErr != nil && !os.IsNotExist(rmErr) { + e = fmt.Errorf("%w; remove partial diff file: %w", e, rmErr) + } + } + }() + err := f.File.Sync() if err != nil { return nil, fmt.Errorf("failed to sync file: %w", err) diff --git a/packages/orchestrator/pkg/sandbox/build/local_diff_test.go b/packages/orchestrator/pkg/sandbox/build/local_diff_test.go new file mode 100644 index 0000000000..b1631b6ab3 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/build/local_diff_test.go @@ -0,0 +1,36 @@ +//go:build linux + +package build + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestLocalDiffFileCloseToDiffRemovesPartialOnError verifies CloseToDiff does not +// leave the partial cache file behind when it fails to produce a usable Diff. +// Nothing registers such a file in the DiffStore, so a leaked orphan would sit in +// the cache dir unreclaimable by disk-pressure eviction until process restart. +func TestLocalDiffFileCloseToDiffRemovesPartialOnError(t *testing.T) { + t.Parallel() + + f, err := NewLocalDiffFile(t.TempDir(), "build-test-id", Rootfs) + require.NoError(t, err) + + // Non-empty so we're past the zero-size NoDiff branch and into materialization. + _, err = f.File.WriteAt(make([]byte, 128), 0) + require.NoError(t, err) + + cachePath := f.cachePath + require.FileExists(t, cachePath) + + // Force a materialization failure: closing the fd makes the Sync inside + // CloseToDiff fail, driving the error path. + require.NoError(t, f.File.Close()) + + diff, err := f.CloseToDiff(blockSize) + require.Error(t, err) + require.Nil(t, diff) + require.NoFileExists(t, cachePath, "partial diff file must be removed on materialization failure") +} diff --git a/packages/orchestrator/pkg/sandbox/build_upload_v3.go b/packages/orchestrator/pkg/sandbox/build_upload_v3.go index 0187a5d697..e6c2dfb0d5 100644 --- a/packages/orchestrator/pkg/sandbox/build_upload_v3.go +++ b/packages/orchestrator/pkg/sandbox/build_upload_v3.go @@ -20,11 +20,6 @@ func (u *Upload) runV3(ctx context.Context) error { return fmt.Errorf("error getting memfile diff path: %w", err) } - rootfsPath, err := u.snap.RootfsDiff.CachePath(ctx) - if err != nil { - return fmt.Errorf("error getting rootfs diff path: %w", err) - } - eg, egCtx := errgroup.WithContext(ctx) eg.Go(func() error { @@ -48,6 +43,17 @@ func (u *Upload) runV3(ctx context.Context) error { return nil } + // Gate the header publish on the rootfs seal (deferred export): the rootfs + // header resolves synchronously at pause time, so without this it could be + // finalized to storage while the background seal is still running — and if + // the seal then fails (it does not retry) storage would keep a completed + // header with no body. The body goroutine below waits on the same seal, so + // both rootfs uploads are gated on it while memfile/snapfile/metadata still + // overlap it. No-op on the synchronous path (CachePath returns immediately). + if _, err := u.snap.RootfsDiff.CachePath(egCtx); err != nil { + return fmt.Errorf("error getting rootfs diff path: %w", err) + } + return storeHeaderWithMetrics(egCtx, u.store, u.paths.RootfsHeader(), uploadFileRootfsHeader, finalizeV3(h), storage.WithMetadata(u.objectMetadata)) }) @@ -77,6 +83,14 @@ func (u *Upload) runV3(ctx context.Context) error { }) eg.Go(func() error { + // Resolve the rootfs diff path inside the group: with deferred rootfs + // export it blocks on the background seal, so doing it here lets the + // memfile/snapfile/metadata uploads overlap the reflink instead of + // waiting behind it. + rootfsPath, err := u.snap.RootfsDiff.CachePath(egCtx) + if err != nil { + return fmt.Errorf("error getting rootfs diff path: %w", err) + } if rootfsPath == "" { return nil } diff --git a/packages/orchestrator/pkg/sandbox/build_upload_v4.go b/packages/orchestrator/pkg/sandbox/build_upload_v4.go index e70dea7283..6f9caa074b 100644 --- a/packages/orchestrator/pkg/sandbox/build_upload_v4.go +++ b/packages/orchestrator/pkg/sandbox/build_upload_v4.go @@ -21,11 +21,6 @@ func (u *Upload) runV4(ctx context.Context) error { return fmt.Errorf("memfile diff path: %w", err) } - rootfsSrc, err := u.snap.RootfsDiff.CachePath(ctx) - if err != nil { - return fmt.Errorf("rootfs diff path: %w", err) - } - eg, ctx := errgroup.WithContext(ctx) eg.Go(func() error { @@ -41,6 +36,15 @@ func (u *Upload) runV4(ctx context.Context) error { }) eg.Go(func() error { + // Resolve the rootfs diff path inside the group: with deferred rootfs + // export it blocks on the background seal, so doing it here lets the + // memfile/snapfile/metadata uploads overlap the reflink instead of + // waiting behind it. + rootfsSrc, err := u.snap.RootfsDiff.CachePath(ctx) + if err != nil { + return fmt.Errorf("rootfs diff path: %w", err) + } + h, err := u.snap.RootfsDiffHeader.WaitWithContext(ctx) if err != nil { return fmt.Errorf("wait rootfs diff header: %w", err) diff --git a/packages/orchestrator/pkg/sandbox/deferred_export_test.go b/packages/orchestrator/pkg/sandbox/deferred_export_test.go new file mode 100644 index 0000000000..be65a28111 --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/deferred_export_test.go @@ -0,0 +1,178 @@ +//go:build linux + +package sandbox + +import ( + "context" + "errors" + "os" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/e2b-dev/infra/packages/orchestrator/pkg/cfg" + "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/block" + "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/build" + "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/rootfs" + "github.com/e2b-dev/infra/packages/shared/pkg/storage/header" + "github.com/e2b-dev/infra/packages/shared/pkg/utils" +) + +// failingRootfs embeds rootfs.Provider (nil) so it satisfies the interface while +// only overriding PrepareExportDiff — the single method the error branch under +// test exercises. Any other call would nil-panic, which is the intended guard. +type failingRootfs struct { + rootfs.Provider + + err error +} + +func (f failingRootfs) PrepareExportDiff(context.Context, func(context.Context) error) (*block.Cache, error) { + return nil, f.err +} + +// stubRootfs hands back a pre-built ejected cache from PrepareExportDiff, standing +// in for the NBD provider once it has frozen and ejected the writable COW cache. +type stubRootfs struct { + rootfs.Provider + + cache *block.Cache +} + +func (s stubRootfs) PrepareExportDiff(context.Context, func(context.Context) error) (*block.Cache, error) { + return s.cache, nil +} + +// TestRunDeferredRootfsExport verifies the background lifecycle of the deferred +// rootfs export: the frozen (ejected) cache is reflinked into the deferred diff, +// the diff resolves with the sealed bytes, and the cache is closed — with no +// overlay/provider interaction (the sandbox is already stopped). +func TestRunDeferredRootfsExport(t *testing.T) { + t.Parallel() + + blockSize := int64(header.PageSize) + numBlocks := int64(3) + size := blockSize * numBlocks + + // A standalone frozen cache with block 1 dirtied (stands in for the ejected + // COW cache after the sandbox is stopped). + sealCache, err := block.NewCache(size, blockSize, t.TempDir()+"/ejected", false) + require.NoError(t, err) + blockData := make([]byte, blockSize) + for i := range blockData { + blockData[i] = 0x5C + } + _, err = sealCache.WriteAt(blockData, blockSize) + require.NoError(t, err) + + s := &Sandbox{ + Resources: &Resources{}, + config: cfg.BuilderConfig{DefaultCacheDir: t.TempDir()}, + } + + buildID := uuid.New() + diffPromise := utils.NewSetOnce[build.Diff]() + + // The setup path captures the diff metadata up front and hands it to the + // background seal (so the exported bytes match the header from the same read). + meta, err := sealCache.DiffMetadata() + require.NoError(t, err) + + s.runDeferredRootfsExport(t.Context(), sealCache, buildID, blockSize, meta, diffPromise) + + diff, err := diffPromise.Result() + require.NoError(t, err) + path, err := diff.CachePath(t.Context()) + require.NoError(t, err) + got, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, blockData, got, "deferred diff must contain the dirtied block") + require.NoError(t, diff.Close()) +} + +// TestSetupDeferredRootfsExport_PrepareError verifies the earliest failure branch: +// when ejecting the writable cache fails, setup propagates the error and returns +// no diff/header/seal — so Pause falls back to the synchronous export rather than +// registering a deferred diff whose seal would never run. +func TestSetupDeferredRootfsExport_PrepareError(t *testing.T) { + t.Parallel() + + s := &Sandbox{ + Resources: &Resources{rootfs: failingRootfs{err: errors.New("eject failed")}}, + config: cfg.BuilderConfig{DefaultCacheDir: t.TempDir()}, + } + + cleanup := NewCleanup() + diff, hdr, startSeal, err := s.setupDeferredRootfsExport(t.Context(), uuid.New(), nil, cleanup) + require.Error(t, err) + require.ErrorContains(t, err, "eject failed") + require.Nil(t, diff) + require.Nil(t, hdr) + require.Nil(t, startSeal) +} + +// TestSetupDeferredRootfsExport_AbortCleanupOrder pins the cleanup ordering on the +// abort path (Pause fails before startSeal runs, so `started` stays false and the +// registered abort cleanups fire). The promise-poisoning cleanup MUST run before +// deferredDiff.Close — Close waits on the seal promise with no context, so if the +// order regressed it would block forever on a seal that will never run. The test +// therefore asserts the whole cleanup stack completes promptly (a deadlock is the +// failure it guards against) and that the deferred diff resolves to the abort +// error rather than dangling. +func TestSetupDeferredRootfsExport_AbortCleanupOrder(t *testing.T) { + t.Parallel() + + blockSize := int64(header.PageSize) + numBlocks := int64(3) + size := blockSize * numBlocks + + // A frozen ejected cache with one dirtied block, so setup takes the deferred + // (non-empty diff) branch that registers the abort cleanups. + ejected, err := block.NewCache(size, blockSize, t.TempDir()+"/ejected", false) + require.NoError(t, err) + dirty := make([]byte, blockSize) + for i := range dirty { + dirty[i] = 0xAB + } + _, err = ejected.WriteAt(dirty, blockSize) + require.NoError(t, err) + + originalHeader, err := header.NewHeader( + header.NewTemplateMetadata(uuid.New(), uint64(blockSize), uint64(size)), + nil, + ) + require.NoError(t, err) + + s := &Sandbox{ + Resources: &Resources{rootfs: stubRootfs{cache: ejected}}, + config: cfg.BuilderConfig{DefaultCacheDir: t.TempDir()}, + } + + cleanup := NewCleanup() + rootfsDiff, hdr, startSeal, err := s.setupDeferredRootfsExport(t.Context(), uuid.New(), originalHeader, cleanup) + require.NoError(t, err) + require.NotNil(t, rootfsDiff) + require.NotNil(t, hdr) + require.NotNil(t, startSeal) + + // Do NOT call startSeal: simulate Pause aborting before the seal is handed off. + // Running the cleanups must not deadlock — guard with a generous timeout so a + // wrong ordering surfaces as a clear failure instead of a hung test. + done := make(chan error, 1) + go func() { done <- cleanup.Run(t.Context()) }() + select { + case runErr := <-done: + require.NoError(t, runErr) + case <-time.After(10 * time.Second): + t.Fatal("cleanup deadlocked: deferred diff Close ran before the promise was poisoned") + } + + // The promise was poisoned, so the diff's data methods resolve (to the abort + // error) without blocking instead of waiting on a seal that never runs. The + // specific message also confirms setup took the deferred branch (a NoDiff + // fallback would register no abort cleanup and never poison anything). + _, err = rootfsDiff.FileSize(t.Context()) + require.ErrorContains(t, err, "pause aborted before deferred rootfs export ran") +} diff --git a/packages/orchestrator/pkg/sandbox/rootfs/direct.go b/packages/orchestrator/pkg/sandbox/rootfs/direct.go index a5867ba4e4..3da72c99cd 100644 --- a/packages/orchestrator/pkg/sandbox/rootfs/direct.go +++ b/packages/orchestrator/pkg/sandbox/rootfs/direct.go @@ -113,6 +113,10 @@ func (o *DirectProvider) ExportDiff( return m, nil } +func (o *DirectProvider) PrepareExportDiff(_ context.Context, _ func(context.Context) error) (*block.Cache, error) { + return nil, ErrDeferredExportNotSupported +} + func (o *DirectProvider) Close(ctx context.Context) error { o.finishedOperations <- struct{}{} diff --git a/packages/orchestrator/pkg/sandbox/rootfs/nbd.go b/packages/orchestrator/pkg/sandbox/rootfs/nbd.go index 76fc86def0..96152f74aa 100644 --- a/packages/orchestrator/pkg/sandbox/rootfs/nbd.go +++ b/packages/orchestrator/pkg/sandbox/rootfs/nbd.go @@ -69,14 +69,15 @@ func (o *NBDProvider) Start(ctx context.Context) error { return o.ready.SetValue(nbd.GetDevicePath(deviceIndex)) } -func (o *NBDProvider) ExportDiff( +// ejectAndStopSandbox detaches the writable cache from the overlay, stops the +// sandbox and waits for the overlay device to be released, returning the ejected +// (now standalone, frozen) cache. The caller owns the returned cache and must +// Close it. Shared by the synchronous ExportDiff and the deferred +// PrepareExportDiff. +func (o *NBDProvider) ejectAndStopSandbox( ctx context.Context, - out *os.File, closeSandbox func(ctx context.Context) error, -) (*header.DiffMetadata, error) { - ctx, span := tracer.Start(ctx, "cow-export") - defer span.End() - +) (*block.Cache, error) { cache, err := o.overlay.EjectCache() if err != nil { return nil, fmt.Errorf("error ejecting cache: %w", err) @@ -104,6 +105,22 @@ func (o *NBDProvider) ExportDiff( } telemetry.ReportEvent(ctx, "sandbox stopped") + return cache, nil +} + +func (o *NBDProvider) ExportDiff( + ctx context.Context, + out *os.File, + closeSandbox func(ctx context.Context) error, +) (*header.DiffMetadata, error) { + ctx, span := tracer.Start(ctx, "cow-export") + defer span.End() + + cache, err := o.ejectAndStopSandbox(ctx, closeSandbox) + if err != nil { + return nil, err + } + m, err := cache.ExportToDiff(ctx, out) if err != nil { // Close the cache to avoid leaking the mmaped memory. Log an error @@ -126,6 +143,20 @@ func (o *NBDProvider) ExportDiff( return m, nil } +// PrepareExportDiff ejects the writable cache, stops the sandbox and waits for +// the overlay device to be released, then returns the frozen ejected cache +// WITHOUT reflinking it. The caller reflinks it into a diff in the background and +// Closes it, so a pause returns without paying the reflink stall. +func (o *NBDProvider) PrepareExportDiff( + ctx context.Context, + closeSandbox func(ctx context.Context) error, +) (*block.Cache, error) { + ctx, span := tracer.Start(ctx, "cow-export-prepare") + defer span.End() + + return o.ejectAndStopSandbox(ctx, closeSandbox) +} + func (o *NBDProvider) Close(ctx context.Context) error { ctx, span := tracer.Start(ctx, "cow-close") defer span.End() diff --git a/packages/orchestrator/pkg/sandbox/rootfs/rootfs.go b/packages/orchestrator/pkg/sandbox/rootfs/rootfs.go index 8eb67de833..887aaa5878 100644 --- a/packages/orchestrator/pkg/sandbox/rootfs/rootfs.go +++ b/packages/orchestrator/pkg/sandbox/rootfs/rootfs.go @@ -4,6 +4,7 @@ package rootfs import ( "context" + "errors" "fmt" "os" "syscall" @@ -13,17 +14,27 @@ import ( "go.opentelemetry.io/otel/trace" "go.uber.org/zap" + "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/block" "github.com/e2b-dev/infra/packages/shared/pkg/logger" "github.com/e2b-dev/infra/packages/shared/pkg/storage/header" ) var tracer = otel.Tracer("github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/rootfs") +// ErrDeferredExportNotSupported is returned by PrepareExportDiff on providers +// that can't defer the rootfs export (e.g. DirectProvider). Callers use it to +// fall back to the synchronous ExportDiff instead of failing the pause. +var ErrDeferredExportNotSupported = errors.New("deferred rootfs export not supported by this provider") + type Provider interface { Start(ctx context.Context) error Close(ctx context.Context) error Path() (string, error) ExportDiff(ctx context.Context, out *os.File, closeSandbox func(context.Context) error) (*header.DiffMetadata, error) + // PrepareExportDiff ejects the writable cache and stops the sandbox, returning + // the frozen ejected cache without reflinking it, so the caller can seal it + // into a diff in the background. Only the NBD provider supports it. + PrepareExportDiff(ctx context.Context, closeSandbox func(context.Context) error) (*block.Cache, error) } // flush flushes the data to the operating system's buffer. diff --git a/packages/orchestrator/pkg/sandbox/sandbox.go b/packages/orchestrator/pkg/sandbox/sandbox.go index 6f663ba550..53acc8cba3 100644 --- a/packages/orchestrator/pkg/sandbox/sandbox.go +++ b/packages/orchestrator/pkg/sandbox/sandbox.go @@ -55,6 +55,7 @@ var ( processMemoryDurationHistogram = utils.Must(telemetry.GetHistogram(meter, telemetry.SnapshotProcessMemoryDurationName)) processRootfsDurationHistogram = utils.Must(telemetry.GetHistogram(meter, telemetry.SnapshotProcessRootfsDurationName)) + rootfsSealDurationHistogram = utils.Must(telemetry.GetHistogram(meter, telemetry.SnapshotRootfsSealDurationName)) uffdStartupPagesHistogram = utils.Must(telemetry.GetHistogram(meter, telemetry.UffdStartupPagesHistogramName)) uffdStartupSourcePagesHistogram = utils.Must(telemetry.GetHistogram(meter, telemetry.UffdStartupSourcePagesHistogramName)) @@ -1403,6 +1404,7 @@ func (s *Sandbox) Shutdown(ctx context.Context) error { type pauseOptions struct { filesystemSnapshot bool + deferRootfsExport bool } type PauseOption func(*pauseOptions) @@ -1415,6 +1417,15 @@ func WithFilesystemSnapshot() PauseOption { return func(o *pauseOptions) { o.filesystemSnapshot = true } } +// WithDeferredRootfsExport seals the rootfs diff off the critical path: the +// sandbox is ejected/stopped and the diff is reflinked in the background, so the +// pause returns without the host->NVMe writeback stall. Only safe when nothing +// reads the diff before the background seal completes — i.e. the suspend (pause) +// path, not a resume-fresh checkpoint. +func WithDeferredRootfsExport() PauseOption { + return func(o *pauseOptions) { o.deferRootfsExport = true } +} + // Pause creates a snapshot of the sandbox. // // Currently the memory snapshotting works like this: @@ -1560,21 +1571,24 @@ func (s *Sandbox) Pause( // harmless and keeps the cleanup ordering identical to the memory path. cleanup.AddNoContext(ctx, mem.Diff.Close) - rootfsDiff, rootfsHeader, err := pauseProcessRootfs( + var ( + rootfsDiff build.Diff + rootfsHeader *header.Header + // startSeal, when non-nil, reflinks the ejected cache into the diff in the + // background; the caller invokes it after the metadata is written. + startSeal func(context.Context) + ) + + rootfsDiff, rootfsHeader, startSeal, err = s.processRootfsSnapshot( ctx, buildID, originalRootfs.Header(), - &RootfsDiffCreator{ - rootfs: s.rootfs, - closeHook: s.Close, - }, - s.config.DefaultCacheDir, - pauseOpts.filesystemSnapshot, + &pauseOpts, + cleanup, ) if err != nil { return nil, fmt.Errorf("error while post processing: %w", err) } - cleanup.AddNoContext(ctx, rootfsDiff.Close) rootfsDiffHeader := NewResolvedDiffHeader(rootfsHeader) // Derive scheduling metadata synchronously so Pause never blocks on the @@ -1582,9 +1596,10 @@ func (s *Sandbox) Pause( // parent header plus the new build, whose exact bytes aren't known yet, so // we pass the pre-dedup dirty size as an upper bound. It is block-granular // (dirty blocks * diff block size) and counts pages before dedup drops the - // base-identical ones, so it over-estimates. The rootfs copy is synchronous - // today, so its new header carries the exact rootfs chain and bytes; if it - // ever becomes async, switch it to the parent plus a dirty proxy like memfile. + // base-identical ones, so it over-estimates. The rootfs header is known + // synchronously even with deferred export — the diff metadata (chain + exact + // bytes) is read up front at pause time and only the reflink seal is deferred — + // so the rootfs half of the scheduling metadata is exact. // mem.header is nil for a filesystem-only pause → rootfs-only metadata. schedulingMetadata := scheduling.FromHeaders(buildID, mem.header, rootfsHeader, mem.newBytes) @@ -1596,6 +1611,12 @@ func (s *Sandbox) Pause( return nil, err } + // The sandbox is stopped and the cache ejected; reflink it to the diff in the + // background so the pause returned without paying the writeback stall. + if startSeal != nil { + startSeal(context.WithoutCancel(ctx)) + } + return &Snapshot{ Snapfile: snapfile, Metafile: metadataFileLink, @@ -1869,29 +1890,79 @@ func buildProvisionalMemfile( return provisionalHeader, provisionalDiff, dc.MarkSwapped } -func pauseProcessRootfs( +func (s *Sandbox) processRootfsSnapshot( ctx context.Context, - buildId uuid.UUID, + buildID uuid.UUID, originalHeader *header.Header, - diffCreator DiffCreator, - cacheDir string, - filesystemOnly bool, -) (d build.Diff, h *header.Header, e error) { + pauseOpts *pauseOptions, + cleanup *Cleanup, +) (d build.Diff, h *header.Header, startSeal func(context.Context), e error) { ctx, span := tracer.Start(ctx, "process-rootfs") defer span.End() - // Duration of the rootfs export+diff, split by fs_only (runs for both pause // kinds) so the fs-only pause latency can be decomposed into quiesce + rootfs. + // This is the pause CRITICAL-PATH rootfs cost: the full export for the + // synchronous path, but only the eject/setup for the deferred path — the + // background reflink seal (runDeferredRootfsExport) is intentionally excluded. + // + // The `deferred` attribute records which of those two populations a sample + // belongs to: without it the (much smaller) deferred setup-only timings would + // be indistinguishable from full synchronous exports. It reflects the path + // actually taken — the fall-through below flips it off when a provider can't + // defer. `success` here is the critical-path outcome only; on the deferred + // path the actual export success/failure is recorded by the seal metric + // (rootfsSealDurationHistogram), so a deferred success=true is setup-only. start := time.Now() defer func() { processRootfsDurationHistogram.Record(ctx, time.Since(start).Milliseconds(), metric.WithAttributes( - attribute.Bool("fs_only", filesystemOnly), + attribute.Bool("fs_only", pauseOpts.filesystemSnapshot), + attribute.Bool("deferred", pauseOpts.deferRootfsExport), attribute.Bool("success", e == nil), )) }() - rootfsDiffFile, err := build.NewLocalDiffFile(cacheDir, buildId.String(), build.Rootfs) + if pauseOpts.deferRootfsExport { + rootfsDiff, rootfsHeader, startSeal, err := s.setupDeferredRootfsExport(ctx, buildID, originalHeader, cleanup) + switch { + case errors.Is(err, rootfs.ErrDeferredExportNotSupported): + // The provider (e.g. DirectProvider) can't defer; fall through to the + // synchronous export below. Safe because PrepareExportDiff returns this + // sentinel before ejecting/stopping anything. + pauseOpts.deferRootfsExport = false + case err != nil: + return nil, nil, nil, fmt.Errorf("deferred rootfs export setup failed: %w", err) + default: + return rootfsDiff, rootfsHeader, startSeal, nil + } + } + + rootfsDiff, rootfsHeader, err := pauseProcessRootfs( + ctx, + buildID, + originalHeader, + &RootfsDiffCreator{ + rootfs: s.rootfs, + closeHook: s.Close, + }, + s.config.DefaultCacheDir, + ) + if err != nil { + return nil, nil, nil, fmt.Errorf("synchronous rootfs export failed: %w", err) + } + cleanup.AddNoContext(ctx, rootfsDiff.Close) + + return rootfsDiff, rootfsHeader, nil, nil +} + +func pauseProcessRootfs( + ctx context.Context, + buildID uuid.UUID, + originalHeader *header.Header, + diffCreator DiffCreator, + cacheDir string, +) (d build.Diff, h *header.Header, e error) { + rootfsDiffFile, err := build.NewLocalDiffFile(cacheDir, buildID.String(), build.Rootfs) if err != nil { return nil, nil, fmt.Errorf("failed to create rootfs diff: %w", err) } @@ -1911,7 +1982,7 @@ func pauseProcessRootfs( } telemetry.ReportEvent(ctx, "converted rootfs diff file to local diff") - rootfsHeader, err := rootfsDiffMetadata.ToDiffHeader(ctx, originalHeader, buildId) + rootfsHeader, err := rootfsDiffMetadata.ToDiffHeader(ctx, originalHeader, buildID) if err != nil { err = errors.Join(err, rootfsDiff.Close()) @@ -1921,6 +1992,175 @@ func pauseProcessRootfs( return rootfsDiff, rootfsHeader, nil } +// setupDeferredRootfsExport ejects the writable cache and stops the sandbox +// (destroy path), then prepares the deferred rootfs diff + header from the frozen +// ejected cache. It returns a startSeal closure that reflinks the cache into the +// diff in the background, so the pause returns without paying the reflink stall. +// Only safe on the suspend path, where nothing reads the diff before the seal +// completes. +func (s *Sandbox) setupDeferredRootfsExport( + ctx context.Context, + buildID uuid.UUID, + originalHeader *header.Header, + cleanup *Cleanup, +) (d build.Diff, h *header.Header, startSeal func(context.Context), e error) { + sealCache, err := s.rootfs.PrepareExportDiff(ctx, s.Close) + if err != nil { + return nil, nil, nil, err + } + + diffMetadata, err := sealCache.DiffMetadata() + if err != nil { + return nil, nil, nil, errors.Join(fmt.Errorf("reading ejected cache metadata: %w", err), sealCache.Close()) + } + // Emit the same rootfs size/ratio metrics the synchronous pauseProcessRootfs + // path does, so deferring the export doesn't blind the snapshot dashboards. + recordSnapshotDiff(ctx, "rootfs", diffMetadata, originalHeader) + + rootfsHeader, err := diffMetadata.ToDiffHeader(ctx, originalHeader, buildID) + if err != nil { + return nil, nil, nil, errors.Join(fmt.Errorf("building rootfs diff header: %w", err), sealCache.Close()) + } + + // No dirty filesystem blocks: the seal would produce an empty diff, i.e. the + // same *NoDiff the synchronous path returns. Return NoDiff directly (and skip + // the background seal — nothing to reflink) so AddSnapshot omits it from the + // DiffStore and peer LookupDiff keeps returning ErrNotAvailable rather than an + // entry whose Slice/Size yield NoDiffError. Close the ejected cache now since + // no seal will own it. + if diffMetadata.Dirty.IsEmpty() { + if err := sealCache.Close(); err != nil { + return nil, nil, nil, fmt.Errorf("closing empty ejected cache: %w", err) + } + + return &build.NoDiff{}, rootfsHeader, func(context.Context) {}, nil + } + + blockSize := int64(originalHeader.Metadata.BlockSize) + diffPromise := utils.NewSetOnce[build.Diff]() + rootfsDiff := build.NewDeferredDiff(build.GetDiffStoreKey(buildID.String(), build.Rootfs), blockSize, diffPromise) + + // The ejected cache and the deferred diff's promise are both owned by the + // background seal once it starts. If Pause aborts before startSeal runs (e.g. + // m.ToFile fails), the goroutine never runs, so on that path we close the + // cache here (else its mmap + backing file leak) and poison the promise (else + // the deferred diff's Close and any waiter block forever on a seal that will + // never resolve). Both are guarded by `started`: once the seal owns them, this + // cleanup is a no-op, so we never double-close the cache nor race a spurious + // SetError against the seal's SetValue (which would drop the sealed diff). + // atomic because startSeal writes it on the pause goroutine and these cleanups + // read it on the cleanup goroutine. + var started atomic.Bool + cleanup.Add(ctx, func(context.Context) error { + if started.Load() { + return nil + } + + return sealCache.Close() + }) + + // LIFO: the abort resolver (added last) runs before rootfsDiff.Close, so on the + // error path the deferred diff's Close never blocks on a seal that won't run. + cleanup.AddNoContext(ctx, rootfsDiff.Close) + cleanup.Add(ctx, func(context.Context) error { + if started.Load() { + return nil + } + _ = diffPromise.SetError(errors.New("pause aborted before deferred rootfs export ran")) + + return nil + }) + + startSeal = func(sealCtx context.Context) { + started.Store(true) + go s.runDeferredRootfsExport(sealCtx, sealCache, buildID, blockSize, diffMetadata, diffPromise) + } + + return rootfsDiff, rootfsHeader, startSeal, nil +} + +// runDeferredRootfsExport reflinks the ejected cache into the rootfs diff and +// closes the cache. The sandbox is already stopped, so there is nothing to fold +// or serialize; the upload waits on the deferred diff, gating shutdown via the +// server's upload WaitGroup. +func (s *Sandbox) runDeferredRootfsExport( + ctx context.Context, + sealCache *block.Cache, + buildID uuid.UUID, + blockSize int64, + meta *header.DiffMetadata, + diffPromise *utils.SetOnce[build.Diff], +) { + ctx, span := tracer.Start(ctx, "deferred-rootfs-export") + defer span.End() + + // Record the background reflink seal latency separately from the + // critical-path process_rootfs.duration, so the deferred export's off-path + // cost stays visible. + start := time.Now() + err := s.sealCacheToDiff(ctx, sealCache, buildID, blockSize, meta, diffPromise) + rootfsSealDurationHistogram.Record(ctx, time.Since(start).Milliseconds(), + metric.WithAttributes(attribute.Bool("success", err == nil))) + if err != nil { + logger.L().Error(ctx, "deferred rootfs export failed", zap.Error(err)) + } else { + telemetry.ReportEvent(ctx, "rootfs diff sealed (deferred)") + } + + // The sandbox is torn down; the ejected cache is ours to close regardless of + // the export outcome. + if err := sealCache.Close(); err != nil { + logger.L().Warn(ctx, "closing ejected rootfs cache", zap.Error(err)) + } +} + +// sealCacheToDiff reflinks the frozen cache into a fresh local diff file and +// resolves diffPromise with the materialized diff. +func (s *Sandbox) sealCacheToDiff( + ctx context.Context, + sealCache *block.Cache, + buildID uuid.UUID, + blockSize int64, + meta *header.DiffMetadata, + diffPromise *utils.SetOnce[build.Diff], +) error { + diffFile, err := build.NewLocalDiffFile(s.config.DefaultCacheDir, buildID.String(), build.Rootfs) + if err != nil { + return s.failRootfsSeal(diffPromise, fmt.Errorf("create rootfs diff file: %w", err)) + } + + // Export using the metadata captured at setup so the sealed data matches the + // header built from the same bitmap read (rather than re-reading the tracker). + if _, err := sealCache.ExportToDiffWithMetadata(ctx, diffFile.File, meta); err != nil { + return s.failRootfsSeal(diffPromise, errors.Join(fmt.Errorf("export rootfs diff: %w", err), diffFile.Close())) + } + telemetry.ReportEvent(ctx, "exported rootfs") + + diff, err := diffFile.CloseToDiff(blockSize) + if err != nil { + return s.failRootfsSeal(diffPromise, fmt.Errorf("materialize rootfs diff: %w", err)) + } + + if err := diffPromise.SetValue(diff); err != nil { + // The promise was already settled (pause aborted); drop the diff so its + // cache file doesn't leak. + return errors.Join(err, diff.Close()) + } + + return nil +} + +// failRootfsSeal settles the deferred diff with err and returns it. +func (s *Sandbox) failRootfsSeal(diffPromise *utils.SetOnce[build.Diff], err error) error { + // Tag the failure with ErrDeferredSealFailed so the upload retry loop can tell + // this permanent, one-shot seal failure apart from transient upload errors and + // stop retrying (the seal never re-runs, so the diff can never materialize). + sealErr := fmt.Errorf("%w: %w", build.ErrDeferredSealFailed, err) + _ = diffPromise.SetError(sealErr) + + return sealErr +} + // createCgroup creates a cgroup for sandbox resource accounting. // The caller is responsible for registering cleanup to remove the cgroup. // diff --git a/packages/orchestrator/pkg/server/prefetch_harvest.go b/packages/orchestrator/pkg/server/prefetch_harvest.go index 55b5a8d802..c6edc18e99 100644 --- a/packages/orchestrator/pkg/server/prefetch_harvest.go +++ b/packages/orchestrator/pkg/server/prefetch_harvest.go @@ -52,10 +52,11 @@ const ( ) var ( - harvestMeter = otel.Meter("github.com/e2b-dev/infra/packages/orchestrator/pkg/server") - harvestAttemptsCounter = utils.Must(telemetry.GetCounter(harvestMeter, telemetry.PauseResumePrefetchHarvestAttempts)) - harvestDurationHistogram = utils.Must(telemetry.GetHistogram(harvestMeter, telemetry.PauseResumePrefetchHarvestDurationName)) - harvestPagesHistogram = utils.Must(telemetry.GetHistogram(harvestMeter, telemetry.PauseResumePrefetchHarvestPagesName)) + harvestMeter = otel.Meter("github.com/e2b-dev/infra/packages/orchestrator/pkg/server") + harvestAttemptsCounter = utils.Must(telemetry.GetCounter(harvestMeter, telemetry.PauseResumePrefetchHarvestAttempts)) + harvestDurationHistogram = utils.Must(telemetry.GetHistogram(harvestMeter, telemetry.PauseResumePrefetchHarvestDurationName)) + harvestPagesHistogram = utils.Must(telemetry.GetHistogram(harvestMeter, telemetry.PauseResumePrefetchHarvestPagesName)) + sealWaitDurationHistogram = utils.Must(telemetry.GetHistogram(harvestMeter, telemetry.PauseResumePrefetchSealWaitDurationName)) ) // harvestResumer resumes the throwaway instance the harvest records its trace @@ -189,8 +190,31 @@ func (s *Server) harvestResumePrefetchAsync( attribute.Bool("consume", consume), ) + // With deferred rootfs export the just-paused snapshot's rootfs diff is + // sealed (reflinked) in the background, and the throwaway warm resume below + // reads the rootfs. Wait for the seal to finish here instead of letting the + // resume block on — and burn its budget against — the reflink. Returns + // immediately for the synchronous and NoDiff paths. If the seal fails, or + // the harvest deadline fires before it completes, skip the harvest (it is + // best-effort and must never touch a half-sealed snapshot). Record the wait + // as its own metric and start the harvest timer after it, so the reflink + // wait doesn't inflate the harvest-duration (slot-hold) histogram. + sealWaitStart := time.Now() + _, sealWaitErr := res.rootfsDiff.CachePath(hCtx) + sealWaitDurationHistogram.Record(hCtx, time.Since(sealWaitStart).Milliseconds()) + start := time.Now() - pages, outcome, err := harvester.run(hCtx, sbx, res.meta, res.upload, buildID, objectMetadata, consume) + var ( + pages int + outcome harvestOutcome + err error + ) + if sealWaitErr != nil { + outcome, err = harvestSkipped, fmt.Errorf("waiting for rootfs seal: %w", sealWaitErr) + } else { + pages, outcome, err = harvester.run(hCtx, sbx, res.meta, res.upload, buildID, objectMetadata, consume) + } + durationMs := time.Since(start).Milliseconds() resultAttr := metric.WithAttributes(attribute.String("result", string(outcome))) diff --git a/packages/orchestrator/pkg/server/sandboxes.go b/packages/orchestrator/pkg/server/sandboxes.go index ecb9994f0a..a2eba0fe2e 100644 --- a/packages/orchestrator/pkg/server/sandboxes.go +++ b/packages/orchestrator/pkg/server/sandboxes.go @@ -25,6 +25,7 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox" + "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/build" "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/fc" sbxtemplate "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/template" buildenvd "github.com/e2b-dev/infra/packages/orchestrator/pkg/template/build/core/envd" @@ -748,8 +749,13 @@ func (s *Server) Pause(ctx context.Context, in *orchestrator.SandboxPauseRequest // Stop the old sandbox in background after we're done defer s.stopSandboxAsync(context.WithoutCancel(ctx), sbx) + // Defer the rootfs reflink off the pause critical path when enabled: pause is a + // suspend, so nothing reads the diff until a later resume (which waits on the + // upload anyway). NBD provider only; falls back to synchronous export otherwise. + deferRootfsExport := s.featureFlags.BoolFlag(ctx, featureflags.DeferRootfsExportFlag) + // Fire and forget - upload completes in the background - res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), map[string]string{storage.ObjectMetadataTemplateID: in.GetTemplateId()}, storage.ObjectOriginPause, in.GetFilesystemOnly()) + res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), map[string]string{storage.ObjectMetadataTemplateID: in.GetTemplateId()}, storage.ObjectOriginPause, in.GetFilesystemOnly(), deferRootfsExport) if err != nil { telemetry.ReportCriticalError(ctx, "error snapshotting sandbox", err, telemetry.WithSandboxID(in.GetSandboxId())) @@ -863,7 +869,9 @@ func (s *Server) Checkpoint(ctx context.Context, in *orchestrator.SandboxCheckpo // Checkpoint always takes a full memory snapshot; filesystem-only checkpoint // (resume-in-place would need to reboot) is not supported yet. - res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), in.GetMetadata(), storage.ObjectOriginSnapshotTemplate, false) + // Checkpoint resumes a fresh sandbox from the new build immediately, so the + // diff must be materialized synchronously — never defer the rootfs export here. + res, err := s.snapshotAndCacheSandbox(ctx, sbx, in.GetBuildId(), in.GetMetadata(), storage.ObjectOriginSnapshotTemplate, false, false) if err != nil { telemetry.ReportCriticalError(ctx, "error snapshotting sandbox for checkpoint", err, telemetry.WithSandboxID(in.GetSandboxId())) @@ -1021,6 +1029,11 @@ type snapshotResult struct { schedulingMetadata *orchestrator.SchedulingMetadata upload *sandbox.Upload completeUpload func(ctx context.Context, uploadErr error) + // rootfsDiff is the snapshot's rootfs diff. With deferred export it is a + // promise-backed diff that resolves only once the background seal finishes, + // so the prefetch harvest waits on its CachePath before its throwaway resume + // (a warm resume reads the rootfs). Nil-safe callers only: always set here. + rootfsDiff build.Diff // objectMetadata is the storage object metadata the snapshot was uploaded // with. The prefetch harvest reuses it verbatim when re-uploading the // metadata object, so the two can never drift. @@ -1040,6 +1053,7 @@ func (s *Server) snapshotAndCacheSandbox( provenance map[string]string, buildOrigin storage.ObjectOrigin, filesystemOnly bool, + deferRootfsExport bool, ) (*snapshotResult, error) { meta, err := sbx.Template.Metadata() if err != nil { @@ -1056,6 +1070,9 @@ func (s *Server) snapshotAndCacheSandbox( if filesystemOnly { pauseOpts = append(pauseOpts, sandbox.WithFilesystemSnapshot()) } + if deferRootfsExport { + pauseOpts = append(pauseOpts, sandbox.WithDeferredRootfsExport()) + } snapshot, err := sbx.Pause(ctx, meta, sandbox.SnapshotUseCasePause, pauseOpts...) if err != nil { @@ -1131,6 +1148,7 @@ func (s *Server) snapshotAndCacheSandbox( completeUpload: completeUpload, objectMetadata: objectMetadata, filesystemOnly: filesystemOnly, + rootfsDiff: snapshot.RootfsDiff, }, nil } diff --git a/packages/orchestrator/pkg/server/upload_retry.go b/packages/orchestrator/pkg/server/upload_retry.go index dfcbd34cb9..111524a4ca 100644 --- a/packages/orchestrator/pkg/server/upload_retry.go +++ b/packages/orchestrator/pkg/server/upload_retry.go @@ -36,6 +36,8 @@ func isRetryableUploadErr(err error) bool { return false // source vanished; retry cannot recover it case errors.Is(err, context.Canceled): return false // parent cancelled (shutdown) + case errors.Is(err, build.ErrDeferredSealFailed): + return false // deferred rootfs seal ran once and failed; it never re-runs default: // Includes per-attempt context.DeadlineExceeded, GCS 401/503, rate // limiting, and unknown errors — all worth retrying within the budget. diff --git a/packages/orchestrator/pkg/server/upload_retry_test.go b/packages/orchestrator/pkg/server/upload_retry_test.go index 1d0ac73c5a..50a8906e35 100644 --- a/packages/orchestrator/pkg/server/upload_retry_test.go +++ b/packages/orchestrator/pkg/server/upload_retry_test.go @@ -26,6 +26,8 @@ func TestIsRetryableUploadErr(t *testing.T) { {"object not exist", storage.ErrObjectNotExist, false}, {"object not exist wrapped", fmt.Errorf("load: %w", storage.ErrObjectNotExist), false}, {"parent cancelled", context.Canceled, false}, + {"deferred seal failed", build.ErrDeferredSealFailed, false}, + {"deferred seal failed wrapped", fmt.Errorf("rootfs diff path: %w", build.ErrDeferredSealFailed), false}, {"per-attempt deadline", context.DeadlineExceeded, true}, {"gcs 503", errors.New("server error (503)"), true}, {"unknown", errors.New("boom"), true}, diff --git a/packages/shared/pkg/featureflags/flags.go b/packages/shared/pkg/featureflags/flags.go index c438edce04..7a22e6b4c4 100644 --- a/packages/shared/pkg/featureflags/flags.go +++ b/packages/shared/pkg/featureflags/flags.go @@ -186,6 +186,15 @@ var ( // of synchronous. Only safe to enable after PeerToPeerChunkTransferFlag is ON. PeerToPeerAsyncCheckpointFlag = NewBoolFlag("peer-to-peer-async-checkpoint", false) + // DeferRootfsExportFlag moves the rootfs diff seal (the reflink, which forces a + // synchronous host->NVMe writeback) off the pause critical path: pause() ejects + // the cache and stops the sandbox, then reflinks the diff in the background so + // the call returns without the writeback stall. Applied only to the suspend + // (pause) path, where nothing reads the diff until a later resume. Off by + // default; falls back to the synchronous export when off or on a non-NBD + // provider. + DeferRootfsExportFlag = NewBoolFlag("defer-rootfs-export", false) + PersistentVolumesFlag = NewBoolFlag("can-use-persistent-volumes", env.IsDevelopment()) SandboxLabelBasedSchedulingFlag = NewBoolFlag("sandbox-label-based-scheduling", false) OptimisticResourceAccountingFlag = NewBoolFlag("sandbox-placement-optimistic-resource-accounting", false) diff --git a/packages/shared/pkg/telemetry/meters.go b/packages/shared/pkg/telemetry/meters.go index 7567bbb9ff..35353acb0d 100644 --- a/packages/shared/pkg/telemetry/meters.go +++ b/packages/shared/pkg/telemetry/meters.go @@ -125,6 +125,7 @@ const ( PauseDurationHistogramName HistogramType = "orchestrator.sandbox.pause.duration" SnapshotProcessMemoryDurationName HistogramType = "orchestrator.sandbox.snapshot.process_memory.duration" SnapshotProcessRootfsDurationName HistogramType = "orchestrator.sandbox.snapshot.process_rootfs.duration" + SnapshotRootfsSealDurationName HistogramType = "orchestrator.sandbox.snapshot.rootfs_seal.duration" // OrchestratorEnvdUpgradeDurationName is the wall-time of a resume-time envd // live-upgrade (delivery + trigger + WaitForEnvd) = overhead added to the @@ -140,8 +141,9 @@ const ( // duration is the whole throwaway resume-and-persist run (slot-hold cost); // pages is the harvested trace size (distinct 2 MiB blocks), recorded only on // success, so its bottom bucket surfaces the empty-trace (idle-at-pause) rate. - PauseResumePrefetchHarvestDurationName HistogramType = "orchestrator.sandbox.pause_resume_prefetch.harvest.duration" - PauseResumePrefetchHarvestPagesName HistogramType = "orchestrator.sandbox.pause_resume_prefetch.harvest.pages" + PauseResumePrefetchHarvestDurationName HistogramType = "orchestrator.sandbox.pause_resume_prefetch.harvest.duration" + PauseResumePrefetchHarvestPagesName HistogramType = "orchestrator.sandbox.pause_resume_prefetch.harvest.pages" + PauseResumePrefetchSealWaitDurationName HistogramType = "orchestrator.sandbox.pause_resume_prefetch.seal_wait.duration" // Sandbox startup working-set histograms: demand-fault pages/bytes a guest // needed to reach a successful envd init, recorded once per start. Sampled @@ -485,9 +487,11 @@ var histogramDesc = map[HistogramType]string{ PauseDurationHistogramName: "Time taken to pause a sandbox, labeled by fs_only (filesystem-only vs memory) and success", SnapshotProcessMemoryDurationName: "Time to export+diff the memory file during a pause snapshot (memory pauses only), labeled by success", SnapshotProcessRootfsDurationName: "Time to export+diff the rootfs during a pause snapshot, labeled by fs_only and success", + SnapshotRootfsSealDurationName: "Time for the background deferred rootfs reflink seal (off the pause critical path), labeled by success", - PauseResumePrefetchHarvestDurationName: "Time taken for a pause-resume prefetch harvest run (slot-hold cost)", - PauseResumePrefetchHarvestPagesName: "Harvested resume-prefetch trace size in 2 MiB blocks, per successful harvest", + PauseResumePrefetchHarvestDurationName: "Time taken for a pause-resume prefetch harvest run (slot-hold cost)", + PauseResumePrefetchHarvestPagesName: "Harvested resume-prefetch trace size in 2 MiB blocks, per successful harvest", + PauseResumePrefetchSealWaitDurationName: "Time the prefetch harvest waited for the deferred rootfs seal before its warm resume", UffdStartupPagesHistogramName: "Demand-fault pages a guest needed to reach a successful envd init, per start", UffdStartupSourcePagesHistogramName: "Subset of startup demand-fault pages pulled from the source (e.g. GCS), per start", @@ -539,8 +543,10 @@ var histogramUnits = map[HistogramType]string{ PauseDurationHistogramName: "ms", SnapshotProcessMemoryDurationName: "ms", SnapshotProcessRootfsDurationName: "ms", + SnapshotRootfsSealDurationName: "ms", PauseResumePrefetchHarvestDurationName: "ms", PauseResumePrefetchHarvestPagesName: "{page}", + PauseResumePrefetchSealWaitDurationName: "ms", UffdStartupPagesHistogramName: "{page}", UffdStartupSourcePagesHistogramName: "{page}", UffdStartupBytesHistogramName: "{By}",