Skip to content

[SS] Fix local snapshot check #11302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maggie-lou merged 1 commit into from
Feb 20, 2026
Merged

[SS] Fix local snapshot check #11302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions enterprise/server/remote_execution/containers/firecracker/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ go_test(
"//enterprise/server/remote_execution/container",
"//enterprise/server/remote_execution/copy_on_write",
"//enterprise/server/remote_execution/filecache",
"//enterprise/server/remote_execution/snaploader",
"//enterprise/server/remote_execution/snaputil",
"//enterprise/server/remote_execution/workspace",
"//enterprise/server/testutil/testcontainer",
Expand Down
18 changes: 11 additions & 7 deletions enterprise/server/remote_execution/containers/firecracker/firecracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,8 +825,9 @@ func NewContainer(ctx context.Context, env environment.Env, task *repb.Execution
return nil, err
}
snap, err := loader.GetSnapshot(ctx, c.snapshotKeySet, &snaploader.GetSnapshotOptions{
RemoteReadEnabled: c.supportsRemoteSnapshots,
ReadPolicy: readPolicy,
SupportsRemoteChunks: c.supportsRemoteSnapshots,
SupportsRemoteManifest: c.supportsRemoteSnapshots,
ReadPolicy: readPolicy,
})
c.createFromSnapshot = err == nil
label := ""
Expand Down Expand Up @@ -1282,7 +1283,8 @@ func (c *FirecrackerContainer) LoadSnapshot(ctx context.Context) error {
maxFallbackAge = d
}
snap, err := c.loader.GetSnapshot(ctx, c.snapshotKeySet, &snaploader.GetSnapshotOptions{
RemoteReadEnabled: c.supportsRemoteSnapshots,
SupportsRemoteChunks: c.supportsRemoteSnapshots,
SupportsRemoteManifest: c.supportsRemoteSnapshots,
ReadPolicy: readPolicy,
MaxStaleFallbackSnapshotAge: maxFallbackAge,
})
Expand Down Expand Up @@ -3347,8 +3349,9 @@ func (c *FirecrackerContainer) hasRemoteSnapshotForKey(ctx context.Context, load
return false
}
_, err = loader.GetSnapshot(ctx, &fcpb.SnapshotKeySet{BranchKey: key}, &snaploader.GetSnapshotOptions{
RemoteReadEnabled: c.supportsRemoteSnapshots,
ReadPolicy: readPolicy,
SupportsRemoteChunks: c.supportsRemoteSnapshots,
SupportsRemoteManifest: c.supportsRemoteSnapshots,
ReadPolicy: readPolicy,
})
return err == nil
}
Expand All @@ -3358,8 +3361,9 @@ func (c *FirecrackerContainer) hasLocalSnapshotForKey(ctx context.Context, loade
return false
}
_, err = loader.GetSnapshot(ctx, &fcpb.SnapshotKeySet{BranchKey: key}, &snaploader.GetSnapshotOptions{
RemoteReadEnabled: false,
ReadPolicy: readPolicy,
SupportsRemoteChunks: c.supportsRemoteSnapshots,
SupportsRemoteManifest: false,
ReadPolicy: readPolicy,
})
return err == nil
}
Expand Down
116 changes: 116 additions & 0 deletions enterprise/server/remote_execution/containers/firecracker/firecracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/containers/firecracker"
"github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/copy_on_write"
"github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/filecache"
"github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/snaploader"
"github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/snaputil"
"github.com/buildbuddy-io/buildbuddy/enterprise/server/remote_execution/workspace"
"github.com/buildbuddy-io/buildbuddy/enterprise/server/testutil/testcontainer"
Expand Down Expand Up @@ -803,6 +804,121 @@ func TestFirecracker_LocalSnapshotSharing_DontResave(t *testing.T) {
}
}

func TestFirecracker_LocalSnapshotSharing_DontResave_RemoteChunkFallback(t *testing.T) {
ctx := context.Background()
env := getTestEnv(ctx, t, envOpts{})
env.SetAuthenticator(testauth.NewTestAuthenticator(t, testauth.TestUsers("US1", "GR1")))

filecacheRoot := testfs.MakeTempDir(t)
fc, err := filecache.NewFileCache(filecacheRoot, fileCacheSize, false)
require.NoError(t, err)
fc.WaitForDirectoryScanToComplete()
env.SetFileCache(fc)

rootDir := testfs.MakeTempDir(t)
cfg := getExecutorConfig(t)

var containersToCleanup []*firecracker.FirecrackerContainer
t.Cleanup(func() {
for _, vm := range containersToCleanup {
err := vm.Remove(ctx)
assert.NoError(t, err)
}
})

task := &repb.ExecutionTask{
Command: &repb.Command{
Platform: &repb.Platform{Properties: []*repb.Platform_Property{
{Name: "recycle-runner", Value: "true"},
// Only a snapshot it none exists
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment has a typo: "Only a snapshot it none exists" should be "Only save a snapshot if none exists"

Suggested change
// Only a snapshot it none exists
// Only save a snapshot if none exists

Copilot uses AI. Check for mistakes.
{Name: platform.SnapshotSavePolicyPropertyName, Value: platform.OnlySaveNonDefaultSnapshotIfNoneAvailable},
}},
// Use ci_runner so it supports remote snapshots
Arguments: []string{"./buildbuddy_ci_runner"},
EnvironmentVariables: []*repb.Command_EnvironmentVariable{
{Name: "GIT_REPO_DEFAULT_BRANCH", Value: "main"},
{Name: "GIT_BRANCH", Value: "pr"},
},
},
}

workDir := testfs.MakeDirAll(t, rootDir, "work")
opts := firecracker.ContainerOpts{
ContainerImage: busyboxImage,
ActionWorkingDirectory: workDir,
VMConfiguration: &fcpb.VMConfiguration{
NumCpus: 1,
MemSizeMb: minMemSizeMB,
NetworkMode: fcpb.NetworkMode_NETWORK_MODE_OFF,
ScratchDiskSizeMb: 100,
},
ExecutorConfig: cfg,
}
baseVM, err := firecracker.NewContainer(ctx, env, task, opts)
require.NoError(t, err)
containersToCleanup = append(containersToCleanup, baseVM)
err = container.PullImageIfNecessary(ctx, env, baseVM, oci.Credentials{}, opts.ContainerImage)
require.NoError(t, err)
err = baseVM.Create(ctx, opts.ActionWorkingDirectory)
require.NoError(t, err)

// Create an initial snapshot. Data written to this snapshot should persist
// when other VMs reuse the snapshot
cmd := appendToLog("Base")
res := baseVM.Exec(ctx, cmd, nil /*=stdio*/)
require.NoError(t, res.Error)
require.Equal(t, "Base\n", string(res.Stdout))
assert.True(t, res.VMMetadata.GetSavedLocalSnapshot())
err = baseVM.Pause(ctx)
require.NoError(t, err)

// Delete some chunks from the local filecache. The chunks should still
// exist in the remote cache, so the local manifest should still be valid.
loader, err := snaploader.New(env)
require.NoError(t, err)
snapshotKeys := baseVM.SnapshotKeySet()
snap, err := loader.GetSnapshot(ctx, snapshotKeys, &snaploader.GetSnapshotOptions{
SupportsRemoteChunks: true,
SupportsRemoteManifest: true,
ReadPolicy: platform.AlwaysReadNewestSnapshot,
})
require.NoError(t, err)

deletedCount := 0
for _, cf := range snap.GetChunkedFiles() {
for i, c := range cf.GetChunks() {
// Delete every other chunk
if i%2 == 0 {
deleted := fc.DeleteFile(ctx, &repb.FileNode{Digest: c.GetDigest()})
if deleted {
deletedCount++
}
}
}
}
require.Greater(t, deletedCount, 0, "should have deleted at least one chunk")

// Create a new VM that loads from the snapshot. Even though some chunks
// are missing locally, they should be fetched from the remote cache.
workDir2 := testfs.MakeDirAll(t, rootDir, "fork")
opts.ActionWorkingDirectory = workDir2
forkVM, err := firecracker.NewContainer(ctx, env, task, opts)
require.NoError(t, err)
containersToCleanup = append(containersToCleanup, forkVM)

err = forkVM.Unpause(ctx)
require.NoError(t, err)

cmd = appendToLog("Fork")
res = forkVM.Exec(ctx, cmd, nil /*=stdio*/)
require.NoError(t, res.Error)
require.Equal(t, "Base\nFork\n", string(res.Stdout))

// The fork should not save a new local snapshot because the local manifest
// exists and all chunks are available (locally or remotely).
assert.False(t, res.VMMetadata.GetSavedLocalSnapshot())
}

func TestFirecracker_RemoteSnapshotSharing_SavePolicy(t *testing.T) {
tests := []struct {
name string
Expand Down
38 changes: 20 additions & 18 deletions enterprise/server/remote_execution/snaploader/snaploader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ func KeysetDebugString(ctx context.Context, env environment.Env, s *fcpb.Snapsho
}

func SnapshotDebugString(ctx context.Context, env environment.Env, s *Snapshot) string {
return snapshotDebugString(ctx, env, s.GetKey(), s.remoteEnabled, s.GetVMMetadata().GetSnapshotId())
return snapshotDebugString(ctx, env, s.GetKey(), s.supportsRemoteChunks, s.GetVMMetadata().GetSnapshotId())
}

func fileDigest(filePath string) (*repb.Digest, error) {
Expand All @@ -282,10 +282,10 @@ func fileDigest(filePath string) (*repb.Digest, error) {

// Snapshot holds a snapshot manifest along with the corresponding cache key.
type Snapshot struct {
key *fcpb.SnapshotKey
manifest *fcpb.SnapshotManifest
remoteEnabled bool
manifestFetchSource snaputil.ChunkSource
key *fcpb.SnapshotKey
manifest *fcpb.SnapshotManifest
supportsRemoteChunks bool
manifestFetchSource snaputil.ChunkSource
}

func (s *Snapshot) GetKey() *fcpb.SnapshotKey {
Expand Down Expand Up @@ -317,7 +317,8 @@ func (s *Snapshot) GetManifestFetchSource() snaputil.ChunkSource {
}

type GetSnapshotOptions struct {
RemoteReadEnabled bool
SupportsRemoteManifest bool
SupportsRemoteChunks bool
ReadPolicy string
MaxStaleFallbackSnapshotAge time.Duration
}
Expand Down Expand Up @@ -429,10 +430,10 @@ func (l *FileCacheLoader) GetSnapshot(ctx context.Context, keys *fcpb.SnapshotKe
continue
}
return &Snapshot{
key: key,
manifest: manifest,
remoteEnabled: opts.RemoteReadEnabled,
manifestFetchSource: manifestFetchSource,
key: key,
manifest: manifest,
supportsRemoteChunks: opts.SupportsRemoteChunks,
manifestFetchSource: manifestFetchSource,
}, nil
}
return nil, lastErr
Expand All @@ -448,7 +449,7 @@ func (l *FileCacheLoader) getSnapshot(ctx context.Context, key *fcpb.SnapshotKey
// Note that if platform.SnapshotReadPolicy=newest, the master snapshot is
// never cached locally.
if *snaputil.EnableLocalSnapshotSharing {
supportsRemoteFallback := opts.RemoteReadEnabled && *snaputil.EnableRemoteSnapshotSharing
supportsRemoteFallback := opts.SupportsRemoteChunks && *snaputil.EnableRemoteSnapshotSharing
manifest, err := l.getLocalManifest(ctx, key, supportsRemoteFallback)
if err == nil {
if validateLocalSnapshot(ctx, manifest, opts, isFallback) {
Expand All @@ -457,14 +458,14 @@ func (l *FileCacheLoader) getSnapshot(ctx context.Context, key *fcpb.SnapshotKey
}
} else {
log.CtxInfof(ctx, "Failed to get local manifest for key %s: %s", key, err)
if !opts.RemoteReadEnabled || !*snaputil.EnableRemoteSnapshotSharing {
if !opts.SupportsRemoteManifest || !*snaputil.EnableRemoteSnapshotSharing {
return nil, snaputil.ChunkSourceUnmapped, err
}
}
// If local snapshot is not valid or couldn't be found, fallback to
// fetching a remote snapshot.
} else if !opts.RemoteReadEnabled {
return nil, snaputil.ChunkSourceUnmapped, status.InternalErrorf("invalid state: EnableLocalSnapshotSharing=false and remoteEnabled=false")
} else if !opts.SupportsRemoteManifest {
return nil, snaputil.ChunkSourceUnmapped, status.InternalErrorf("invalid state: EnableLocalSnapshotSharing=false and SupportsRemoteManifest=false")
}

if opts.ReadPolicy == platform.ReadLocalSnapshotOnly {
Expand Down Expand Up @@ -684,7 +685,7 @@ func (l *FileCacheLoader) UnpackSnapshot(ctx context.Context, snapshot *Snapshot

for _, fileNode := range snapshot.manifest.Files {
outputPath := filepath.Join(outputDirectory, fileNode.GetName())
if _, err := snaputil.GetArtifact(ctx, l.env.GetFileCache(), l.env.GetByteStreamClient(), snapshot.remoteEnabled, fileNode.GetDigest(), snapshot.key.InstanceName, outputPath); err != nil {
if _, err := snaputil.GetArtifact(ctx, l.env.GetFileCache(), l.env.GetByteStreamClient(), snapshot.supportsRemoteChunks, fileNode.GetDigest(), snapshot.key.InstanceName, outputPath); err != nil {
return nil, err
}
}
Expand All @@ -694,7 +695,7 @@ func (l *FileCacheLoader) UnpackSnapshot(ctx context.Context, snapshot *Snapshot
}
// Construct COWs from chunks.
for _, cf := range snapshot.manifest.ChunkedFiles {
cow, err := l.unpackCOW(ctx, cf, snapshot.key.InstanceName, outputDirectory, snapshot.remoteEnabled)
cow, err := l.unpackCOW(ctx, cf, snapshot.key.InstanceName, outputDirectory, snapshot.supportsRemoteChunks)
if err != nil {
return nil, status.WrapError(err, "unpack COW")
}
Expand Down Expand Up @@ -1224,8 +1225,9 @@ func UnpackContainerImage(ctx context.Context, l *FileCacheLoader, instanceName,
}}

snap, err := l.GetSnapshot(ctx, key, &GetSnapshotOptions{
RemoteReadEnabled: remoteEnabled,
ReadPolicy: platform.AlwaysReadNewestSnapshot,
SupportsRemoteManifest: remoteEnabled,
SupportsRemoteChunks: remoteEnabled,
ReadPolicy: platform.AlwaysReadNewestSnapshot,
})
if err != nil && !(status.IsNotFoundError(err) || status.IsUnavailableError(err)) {
return nil, err
Expand Down
Loading
Loading