builds: add feature-gated ephemeral ext4 BuildKit root volume - #332
builds: add feature-gated ephemeral ext4 BuildKit root volume#332rgarcia wants to merge 5 commits into
Conversation
When build.disk_root.enabled is set, the build manager creates a fixed-size ext4 volume per build, attaches it to the builder VM at /var/lib/buildkit, and deletes it with the builder VM. The builder agent already detects the mount and uses it directly; when disabled, the tmpfs fallback path is unchanged.
A host crash mid-build leaked the deterministic build-disk-<id> volume; on recovery, setupDiskRootVolume failed on ErrAlreadyExists and the recovered build could never run. Delete the leftover and create a fresh volume instead.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: BuildKit mount blocked by validation
- I added a narrow validation exception for
/var/lib/buildkitso disk-root builders can attach the BuildKit volume while other/varmounts remain blocked.
- I added a narrow validation exception for
- ✅ Fixed: Leftover disk delete when attached
- On leftover disk
ErrInUse, the build manager now deletes the stale recorded builder instance, clears its metadata, retries volume deletion, and then recreates the disk volume.
- On leftover disk
Or push these changes by commenting:
@cursor push 411978317b
Preview (411978317b)
diff --git a/lib/builds/disk_root_test.go b/lib/builds/disk_root_test.go
--- a/lib/builds/disk_root_test.go
+++ b/lib/builds/disk_root_test.go
@@ -120,6 +120,60 @@
assert.Empty(t, volID)
}
+func TestSetupDiskRootVolume_LeftoverInUseCleansStaleBuilder(t *testing.T) {
+ mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t)
+ defer os.RemoveAll(tempDir)
+ mgr.config.DiskRootEnabled = true
+
+ buildID := "build-1"
+ staleBuilderID := "inst-builder-build-1"
+ meta := &buildMetadata{
+ ID: buildID,
+ Status: StatusBuilding,
+ Request: &CreateBuildRequest{Dockerfile: "FROM alpine"},
+ CreatedAt: time.Now(),
+ BuilderInstance: &staleBuilderID,
+ }
+ require.NoError(t, writeMetadata(mgr.paths, meta))
+ instanceMgr.instances[staleBuilderID] = &instances.Instance{
+ StoredMetadata: instances.StoredMetadata{
+ Id: staleBuilderID,
+ Name: "builder-build-1",
+ },
+ State: instances.StateRunning,
+ }
+
+ created := false
+ volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) {
+ if !created {
+ created = true
+ return nil, volumes.ErrAlreadyExists
+ }
+ return &volumes.Volume{Id: *req.Id, Name: req.Name, SizeGb: req.SizeGb}, nil
+ }
+
+ deleteAttempts := 0
+ volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+ deleteAttempts++
+ if deleteAttempts == 1 {
+ return volumes.ErrInUse
+ }
+ return nil
+ }
+
+ volID, err := mgr.setupDiskRootVolume(context.Background(), buildID)
+
+ require.NoError(t, err)
+ assert.Equal(t, "build-disk-build-1", volID)
+ assert.Equal(t, 1, instanceMgr.deleteCallCount)
+ assert.Equal(t, 2, deleteAttempts)
+ assert.Equal(t, 2, volumeMgr.createCallCount)
+
+ metaAfter, err := readMetadata(mgr.paths, buildID)
+ require.NoError(t, err)
+ assert.Nil(t, metaAfter.BuilderInstance)
+}
+
func TestBuilderVolumeAttachments(t *testing.T) {
attachments := builderVolumeAttachments("src-vol", "cfg-vol", "")
require.Len(t, attachments, 2)
diff --git a/lib/builds/manager.go b/lib/builds/manager.go
--- a/lib/builds/manager.go
+++ b/lib/builds/manager.go
@@ -803,7 +803,16 @@
if errors.Is(err, volumes.ErrAlreadyExists) {
// A previous attempt at this build crashed before cleanup. Delete
// the leftover volume and start fresh.
- if delErr := m.volumeManager.DeleteVolume(ctx, volID); delErr != nil {
+ delErr := m.volumeManager.DeleteVolume(ctx, volID)
+ if errors.Is(delErr, volumes.ErrInUse) {
+ // The leftover volume may still be attached to a stale builder
+ // instance from a crashed process. Remove it and retry.
+ if cleanupErr := m.cleanupStaleBuilderInstance(ctx, buildID); cleanupErr != nil {
+ return "", fmt.Errorf("cleanup stale builder instance: %w", cleanupErr)
+ }
+ delErr = m.volumeManager.DeleteVolume(ctx, volID)
+ }
+ if delErr != nil {
return "", fmt.Errorf("delete leftover buildkit root volume: %w", delErr)
}
_, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{
@@ -818,6 +827,28 @@
return volID, nil
}
+func (m *manager) cleanupStaleBuilderInstance(ctx context.Context, buildID string) error {
+ meta, err := readMetadata(m.paths, buildID)
+ if err != nil {
+ return fmt.Errorf("read build metadata: %w", err)
+ }
+ if meta.BuilderInstance == nil || *meta.BuilderInstance == "" {
+ return nil
+ }
+
+ builderInstanceID := *meta.BuilderInstance
+ if err := m.instanceManager.DeleteInstance(ctx, builderInstanceID); err != nil && !errors.Is(err, instances.ErrNotFound) {
+ return fmt.Errorf("delete stale builder instance %s: %w", builderInstanceID, err)
+ }
+
+ meta.BuilderInstance = nil
+ if err := writeMetadata(m.paths, meta); err != nil {
+ return fmt.Errorf("clear stale builder instance metadata: %w", err)
+ }
+
+ return nil
+}
+
// builderVolumeAttachments returns the volume attachments for a builder VM.
// diskRootVolID is empty when the disk root feature is disabled.
func builderVolumeAttachments(sourceVolID, configVolID, diskRootVolID string) []instances.VolumeAttachment {
diff --git a/lib/instances/create.go b/lib/instances/create.go
--- a/lib/instances/create.go
+++ b/lib/instances/create.go
@@ -51,6 +51,12 @@
"/var",
}
+// allowedSystemMountPaths are explicit exceptions under system directories that
+// are required for internal platform workloads.
+var allowedSystemMountPaths = map[string]struct{}{
+ "/var/lib/buildkit": {},
+}
+
// generateVsockCID converts first 8 chars of instance ID to a unique CID
// CIDs 0-2 are reserved (hypervisor, loopback, host)
// Returns value in range 3 to 4294967295
@@ -665,7 +671,7 @@
cleanPath := filepath.Clean(vol.MountPath)
// Check for system directories
- if isSystemDirectory(cleanPath) {
+ if isSystemDirectory(cleanPath) && !isAllowedSystemMountPath(cleanPath) {
return fmt.Errorf("volume %s: cannot mount to system directory %q", vol.VolumeID, cleanPath)
}
@@ -704,6 +710,11 @@
return false
}
+func isAllowedSystemMountPath(path string) bool {
+ _, ok := allowedSystemMountPaths[filepath.Clean(path)]
+ return ok
+}
+
// startAndBootVM starts the VMM and boots the VM
func (m *manager) startAndBootVM(
ctx context.Context,
diff --git a/lib/instances/resource_limits_test.go b/lib/instances/resource_limits_test.go
--- a/lib/instances/resource_limits_test.go
+++ b/lib/instances/resource_limits_test.go
@@ -44,6 +44,29 @@
assert.Contains(t, err.Error(), "system directory")
}
+func TestValidateVolumeAttachments_BuildkitRootSystemPathAllowed(t *testing.T) {
+ t.Parallel()
+ volumes := []VolumeAttachment{{
+ VolumeID: "vol-1",
+ MountPath: "/var/lib/buildkit",
+ }}
+
+ err := validateVolumeAttachments(volumes)
+ assert.NoError(t, err)
+}
+
+func TestValidateVolumeAttachments_VarSubdirectoryStillBlocked(t *testing.T) {
+ t.Parallel()
+ volumes := []VolumeAttachment{{
+ VolumeID: "vol-1",
+ MountPath: "/var/lib/other",
+ }}
+
+ err := validateVolumeAttachments(volumes)
+ assert.Error(t, err)
+ assert.Contains(t, err.Error(), "system directory")
+}
+
func TestValidateVolumeAttachments_DuplicatePaths(t *testing.T) {
t.Parallel()
volumes := []VolumeAttachment{You can send follow-ups to the cloud agent here.
- Allow the exact /var/lib/buildkit mount path through volume attachment validation so builder creation succeeds with the disk root feature. - When a leftover buildkit root volume is still attached to a surviving builder VM, delete the stale builder (detaching the volume) and retry the delete instead of failing recovery on ErrInUse. - Write the build config ext4 disk to a unique per-call temp directory instead of a fixed TMPDIR path that concurrent builds and parallel test runs can collide on.
The exact-path exemption applied to every instance creation, so any API caller could mount a volume at /var/lib/buildkit in their own VM. Gate it behind AllowSystemVolumeMounts on the domain create request, which is never populated from API requests; only the builds manager sets it when creating builder VMs.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Wrong error wrapped on lookup
- Updated the stale-builder lookup failure path to wrap
getErrinstead of the prior volumeErrInUse, preserving the correct error chain.
- Updated the stale-builder lookup failure path to wrap
Or push these changes by commenting:
@cursor push c5e6c475b9
Preview (c5e6c475b9)
diff --git a/lib/builds/manager.go b/lib/builds/manager.go
--- a/lib/builds/manager.go
+++ b/lib/builds/manager.go
@@ -835,7 +835,7 @@
builderName := fmt.Sprintf("builder-%s", buildID)
inst, getErr := m.instanceManager.GetInstance(ctx, builderName)
if getErr != nil {
- return fmt.Errorf("leftover buildkit root volume still attached and stale builder %q not found: %w", builderName, err)
+ return fmt.Errorf("leftover buildkit root volume still attached and stale builder %q not found: %w", builderName, getErr)
}
if delErr := m.instanceManager.DeleteInstance(ctx, inst.Id); delErr != nil {
return fmt.Errorf("delete stale builder %s holding leftover buildkit root volume: %w", inst.Id, delErr)You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 4654ffa. Configure here.
bba318a to
0aa1048
Compare
7114b82 to
b66a729
Compare
9844255 to
bba318a
Compare
|
Closing in favor of a fresh first-class Builder resource stack. Useful implementation and test work from this PR will be selectively reapplied in smaller reviewable layers. |


Summary
When
build.disk_root.enabledis set, the build manager creates a fixed-size ext4 volume per build (build-disk-<id>, size frombuild.disk_root.size_gb, default 20 GB), attaches it to the builder VM at/var/lib/buildkit, and deletes it with the builder VM. The builder agent already detects that mountpoint and uses it directly (see the PR below in this stack), so the guest skips its tmpfs fallback.When disabled (the default), no volume is created and the existing tmpfs path is unchanged.
Changes
builds.Config: newDiskRootEnabled/DiskRootSizeGBfields;buildkitRootMountPathandDefaultDiskRootSizeGBconstants.executeBuild:setupDiskRootVolumecreates the volume when enabled (error fails the build before instance creation);builderVolumeAttachmentsbuilds the attachment list, appending the rw/var/lib/buildkitmount only when a disk root volume exists. Volume deletion is deferred alongside the existing source/config volume cleanup.setupDiskRootVolumetolerates a leftover volume from a crashed build attempt: the deterministicbuild-disk-<id>is deleted and recreated instead of permanently failing the recovered build onErrAlreadyExists.build.disk_root.enabled/size_gbincmd/api/config(with validation),lib/providers, andconfig.example.yaml.Tests
lib/builds/disk_root_test.go(no privileged mounts; mock instance/volume managers):executeBuildlifecycle: volume created, attached at/var/lib/buildkit, deleted when the build finishesgo test ./lib/builds/... ./cmd/api/config/... ./lib/providers/...andgo build ./...pass.Note
Medium Risk
Changes build VM volume lifecycle and host disk use per build when enabled; default-off and well-tested, but recovery deletes stale builder instances and widens mount rules for internal creates only.
Overview
Adds optional
build.disk_rootconfig so each source build can get a dedicated ext4 volume (build-disk-<id>) mounted at/var/lib/buildkiton the builder VM instead of tmpfs, with size fromsize_gb(default 20 when unset). When disabled, behavior is unchanged.executeBuildcreates the volume before the builder instance (failure aborts before VM create), attaches it viabuilderVolumeAttachments, and defers deletion with the other build volumes.setupDiskRootVolumehandlesErrAlreadyExistsfrom crashed runs by deleting leftovers; if delete hitsErrInUse, it removes a stalebuilder-<id>instance and retries.Instance validation gains internal-only
AllowSystemVolumeMountsso builder VMs may mount exactly/var/lib/buildkit(not exposed from the API). Config disk creation now uses per-call unique temp directories to avoid path collisions under sharedTMPDIR.Tests cover disabled/enabled paths, crash recovery, and full
executeBuildlifecycle with mocks.Reviewed by Cursor Bugbot for commit bba318a. Bugbot is set up for automated code reviews on this repo. Configure here.