From dddf446d258f081a6f7c38dae028b0284e0d817f Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sun, 2 Aug 2026 02:53:52 +0000 Subject: [PATCH 1/5] builds: add feature-gated ephemeral ext4 BuildKit root volume 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. --- cmd/api/config/config.go | 22 +++- config.example.yaml | 4 + lib/builds/disk_root_test.go | 233 +++++++++++++++++++++++++++++++++++ lib/builds/manager.go | 87 +++++++++++-- lib/providers/providers.go | 2 + 5 files changed, 331 insertions(+), 17 deletions(-) create mode 100644 lib/builds/disk_root_test.go diff --git a/cmd/api/config/config.go b/cmd/api/config/config.go index 366fb496..6324ee66 100644 --- a/cmd/api/config/config.go +++ b/cmd/api/config/config.go @@ -146,11 +146,20 @@ type ImagesConfig struct { // BuildConfig holds source-to-image build system settings. type BuildConfig struct { - MaxConcurrentSourceBuilds int `koanf:"max_concurrent_source_builds"` - BuilderImage string `koanf:"builder_image"` - Timeout int `koanf:"timeout"` - SecretsDir string `koanf:"secrets_dir"` - DockerSocket string `koanf:"docker_socket"` + MaxConcurrentSourceBuilds int `koanf:"max_concurrent_source_builds"` + BuilderImage string `koanf:"builder_image"` + Timeout int `koanf:"timeout"` + SecretsDir string `koanf:"secrets_dir"` + DockerSocket string `koanf:"docker_socket"` + DiskRoot BuildDiskRootConfig `koanf:"disk_root"` +} + +// BuildDiskRootConfig holds settings for the ephemeral per-build BuildKit root +// disk. When enabled, each builder VM gets a dedicated ext4 volume mounted at +// /var/lib/buildkit instead of a tmpfs, and the volume is deleted with the VM. +type BuildDiskRootConfig struct { + Enabled bool `koanf:"enabled"` + SizeGB int `koanf:"size_gb"` } // InstancesConfig holds instance-manager internal settings. @@ -629,6 +638,9 @@ func (c *Config) Validate() error { if c.Build.Timeout <= 0 { return fmt.Errorf("build.timeout must be positive, got %d", c.Build.Timeout) } + if c.Build.DiskRoot.SizeGB < 0 { + return fmt.Errorf("build.disk_root.size_gb must be >= 0, got %d", c.Build.DiskRoot.SizeGB) + } if c.Hypervisor.FirecrackerMaxConcurrentRestores < 0 { return fmt.Errorf("hypervisor.firecracker_max_concurrent_restores must be >= 0, got %d", c.Hypervisor.FirecrackerMaxConcurrentRestores) } diff --git a/config.example.yaml b/config.example.yaml index 410fd8b2..a25dc005 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -152,6 +152,10 @@ data_dir: /var/lib/hypeman # docker_socket: /var/run/docker.sock # max_concurrent_source_builds: 2 # timeout: 600 +# disk_root: +# enabled: false # attach a dedicated ext4 volume at /var/lib/buildkit +# # in each builder VM instead of a tmpfs +# size_gb: 20 # size of the per-build BuildKit root volume # ============================================================================= # Resource Limits diff --git a/lib/builds/disk_root_test.go b/lib/builds/disk_root_test.go new file mode 100644 index 00000000..7b53d993 --- /dev/null +++ b/lib/builds/disk_root_test.go @@ -0,0 +1,233 @@ +package builds + +import ( + "context" + "errors" + "os" + "testing" + "time" + + "github.com/kernel/hypeman/lib/instances" + "github.com/kernel/hypeman/lib/volumes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSetupDiskRootVolume_Disabled(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.NoError(t, err) + assert.Empty(t, volID) + assert.Equal(t, 0, volumeMgr.createCallCount) +} + +func TestSetupDiskRootVolume_DefaultSize(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + var gotReq volumes.CreateVolumeRequest + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + gotReq = req + return &volumes.Volume{Id: *req.Id, Name: req.Name, SizeGb: req.SizeGb}, nil + } + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.NoError(t, err) + assert.Equal(t, "build-disk-build-1", volID) + require.NotNil(t, gotReq.Id) + assert.Equal(t, "build-disk-build-1", *gotReq.Id) + assert.Equal(t, DefaultDiskRootSizeGB, gotReq.SizeGb) +} + +func TestSetupDiskRootVolume_ConfiguredSize(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + mgr.config.DiskRootSizeGB = 42 + + var gotReq volumes.CreateVolumeRequest + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + gotReq = req + return &volumes.Volume{Id: *req.Id, Name: req.Name, SizeGb: req.SizeGb}, nil + } + + _, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.NoError(t, err) + assert.Equal(t, 42, gotReq.SizeGb) +} + +func TestSetupDiskRootVolume_CreateError(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + return nil, errors.New("disk full") + } + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.Error(t, err) + assert.Empty(t, volID) +} + +func TestBuilderVolumeAttachments(t *testing.T) { + attachments := builderVolumeAttachments("src-vol", "cfg-vol", "") + require.Len(t, attachments, 2) + assert.Equal(t, "/src", attachments[0].MountPath) + assert.False(t, attachments[0].Readonly) + assert.Equal(t, "/config", attachments[1].MountPath) + assert.True(t, attachments[1].Readonly) + + attachments = builderVolumeAttachments("src-vol", "cfg-vol", "disk-vol") + require.Len(t, attachments, 3) + assert.Equal(t, "disk-vol", attachments[2].VolumeID) + assert.Equal(t, "/var/lib/buildkit", attachments[2].MountPath) + assert.False(t, attachments[2].Readonly) +} + +// prepareBuildOnDisk writes the metadata, source, and config files executeBuild +// expects, without going through CreateBuild (which would start a background +// build goroutine via the queue). +func prepareBuildOnDisk(t *testing.T, mgr *manager, id string, req CreateBuildRequest) { + t.Helper() + + meta := &buildMetadata{ + ID: id, + Status: StatusQueued, + Request: &req, + CreatedAt: time.Now(), + } + require.NoError(t, writeMetadata(mgr.paths, meta)) + require.NoError(t, mgr.storeSource(id, []byte("fake-tarball-data"))) + + config := &BuildConfig{ + JobID: id, + RegistryURL: mgr.config.RegistryURL, + SourcePath: "/src", + Dockerfile: req.Dockerfile, + TimeoutSeconds: 600, + NetworkMode: "isolated", + } + require.NoError(t, writeBuildConfig(mgr.paths, id, config)) +} + +// stoppedBuilderInstance returns a CreateInstance hook that records the +// request and reports the instance as already stopped, so waitForResult +// returns quickly. +func stoppedBuilderInstance(instanceMgr *mockInstanceManager, gotReq *instances.CreateInstanceRequest) { + instanceMgr.createFunc = func(ctx context.Context, req instances.CreateInstanceRequest) (*instances.Instance, error) { + if gotReq != nil { + *gotReq = req + } + inst := &instances.Instance{ + StoredMetadata: instances.StoredMetadata{ + Id: "inst-" + req.Name, + Name: req.Name, + }, + State: instances.StateStopped, + } + instanceMgr.instances[inst.Id] = inst + return inst, nil + } +} + +// TestExecuteBuild_DiskRootLifecycle runs executeBuild with the disk root +// feature enabled and verifies the volume is created, attached at +// /var/lib/buildkit, and deleted when the build finishes. It uses mock +// managers only — no privileged mounts. +func TestExecuteBuild_DiskRootLifecycle(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + ctx := context.Background() + req := CreateBuildRequest{ + Dockerfile: "FROM alpine\nRUN echo hello", + } + prepareBuildOnDisk(t, mgr, "build-1", req) + + var createReq instances.CreateInstanceRequest + stoppedBuilderInstance(instanceMgr, &createReq) + + policy := DefaultBuildPolicy() + result, err := mgr.executeBuild(ctx, "build-1", req, &policy) + require.NoError(t, err) + require.NotNil(t, result) + assert.False(t, result.Success) + + // Attached at /var/lib/buildkit in the builder instance request. + require.Len(t, createReq.Volumes, 3) + assert.Equal(t, "build-disk-build-1", createReq.Volumes[2].VolumeID) + assert.Equal(t, "/var/lib/buildkit", createReq.Volumes[2].MountPath) + assert.False(t, createReq.Volumes[2].Readonly) + + // Deleted with the builder VM. + _, err = volumeMgr.GetVolume(ctx, "build-disk-build-1") + assert.ErrorIs(t, err, volumes.ErrNotFound) + assert.GreaterOrEqual(t, volumeMgr.deleteCallCount, 1) +} + +// TestExecuteBuild_DiskRootDisabledLeavesDefaultPath verifies that with the +// feature disabled no disk root volume is created and the builder instance +// only gets the source and config attachments. +func TestExecuteBuild_DiskRootDisabledLeavesDefaultPath(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + ctx := context.Background() + req := CreateBuildRequest{ + Dockerfile: "FROM alpine\nRUN echo hello", + } + prepareBuildOnDisk(t, mgr, "build-1", req) + + var createReq instances.CreateInstanceRequest + stoppedBuilderInstance(instanceMgr, &createReq) + + policy := DefaultBuildPolicy() + _, err := mgr.executeBuild(ctx, "build-1", req, &policy) + require.NoError(t, err) + + require.Len(t, createReq.Volumes, 2) + assert.Equal(t, "/src", createReq.Volumes[0].MountPath) + assert.Equal(t, "/config", createReq.Volumes[1].MountPath) + for _, v := range volumeMgr.volumes { + assert.NotContains(t, v.Name, "build-disk-") + } +} + +// TestExecuteBuild_DiskRootCreateErrorFailsBuild verifies a volume creation +// failure fails the build before the builder instance is created. +func TestExecuteBuild_DiskRootCreateErrorFailsBuild(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + ctx := context.Background() + req := CreateBuildRequest{ + Dockerfile: "FROM alpine\nRUN echo hello", + } + prepareBuildOnDisk(t, mgr, "build-1", req) + + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + if req.Name == "build-disk-build-1" { + return nil, errors.New("disk full") + } + vol := &volumes.Volume{Id: "vol-" + req.Name, Name: req.Name} + volumeMgr.volumes[vol.Id] = vol + return vol, nil + } + + policy := DefaultBuildPolicy() + _, err := mgr.executeBuild(ctx, "build-1", req, &policy) + + require.Error(t, err) + assert.Contains(t, err.Error(), "buildkit root volume") + assert.Equal(t, 0, instanceMgr.createCallCount) +} diff --git a/lib/builds/manager.go b/lib/builds/manager.go index 87f21dfe..bf73a333 100644 --- a/lib/builds/manager.go +++ b/lib/builds/manager.go @@ -89,8 +89,23 @@ type Config struct { // DockerSocket is the path to the Docker socket for building the builder image DockerSocket string + + // DiskRootEnabled attaches a dedicated ext4 volume at /var/lib/buildkit in + // each builder VM instead of relying on the tmpfs fallback in the guest. + DiskRootEnabled bool + + // DiskRootSizeGB is the size of the per-build BuildKit root volume. + // Values <= 0 use DefaultDiskRootSizeGB. + DiskRootSizeGB int } +// buildkitRootMountPath is where the BuildKit root volume is mounted in the +// builder guest. The builder agent detects the mount and uses it directly. +const buildkitRootMountPath = "/var/lib/buildkit" + +// DefaultDiskRootSizeGB is the default size of the per-build BuildKit root volume. +const DefaultDiskRootSizeGB = 20 + // DefaultConfig returns the default build manager configuration func DefaultConfig() Config { return Config{ @@ -719,6 +734,16 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe } defer m.volumeManager.DeleteVolume(context.Background(), configVolID) + // Optionally create a dedicated BuildKit root volume for this build. + // It is attached at /var/lib/buildkit and deleted with the builder VM. + diskRootVolID, err := m.setupDiskRootVolume(ctx, id) + if err != nil { + return nil, fmt.Errorf("create buildkit root volume: %w", err) + } + if diskRootVolID != "" { + defer m.volumeManager.DeleteVolume(context.Background(), diskRootVolID) + } + // Create builder instance builderName := fmt.Sprintf("builder-%s", id) networkEnabled := policy.NetworkMode == "egress" @@ -729,18 +754,7 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe Size: int64(policy.MemoryMB) * 1024 * 1024, Vcpus: policy.CPUs, NetworkEnabled: networkEnabled, - Volumes: []instances.VolumeAttachment{ - { - VolumeID: sourceVolID, - MountPath: "/src", - Readonly: false, // Builder needs to write generated Dockerfile - }, - { - VolumeID: configVolID, - MountPath: "/config", - Readonly: true, - }, - }, + Volumes: builderVolumeAttachments(sourceVolID, configVolID, diskRootVolID), }) if err != nil { return nil, fmt.Errorf("create builder instance: %w", err) @@ -767,6 +781,55 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe return result, nil } +// setupDiskRootVolume creates the ephemeral BuildKit root volume for a build +// when the disk root feature is enabled. Returns "" when disabled. +func (m *manager) setupDiskRootVolume(ctx context.Context, buildID string) (string, error) { + if !m.config.DiskRootEnabled { + return "", nil + } + + sizeGB := m.config.DiskRootSizeGB + if sizeGB <= 0 { + sizeGB = DefaultDiskRootSizeGB + } + + volID := fmt.Sprintf("build-disk-%s", buildID) + _, err := m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{ + Id: &volID, + Name: volID, + SizeGb: sizeGB, + }) + if err != nil { + return "", err + } + return volID, 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 { + attachments := []instances.VolumeAttachment{ + { + VolumeID: sourceVolID, + MountPath: "/src", + Readonly: false, // Builder needs to write generated Dockerfile + }, + { + VolumeID: configVolID, + MountPath: "/config", + Readonly: true, + }, + } + if diskRootVolID != "" { + attachments = append(attachments, instances.VolumeAttachment{ + VolumeID: diskRootVolID, + MountPath: buildkitRootMountPath, + Readonly: false, + }) + } + return attachments +} + // waitForResult waits for the build result from the builder agent via vsock func (m *manager) waitForResult(ctx context.Context, buildID string, inst *instances.Instance) (*BuildResult, error) { // Wait a bit for the VM to start and the builder agent to listen on vsock diff --git a/lib/providers/providers.go b/lib/providers/providers.go index 090d2ccf..99361fbb 100644 --- a/lib/providers/providers.go +++ b/lib/providers/providers.go @@ -421,6 +421,8 @@ func ProvideBuildManager(p *paths.Paths, cfg *config.Config, instanceManager ins DefaultTimeout: cfg.Build.Timeout, RegistrySecret: cfg.JwtSecret, // Use same secret for registry tokens DockerSocket: cfg.Build.DockerSocket, + DiskRootEnabled: cfg.Build.DiskRoot.Enabled, + DiskRootSizeGB: cfg.Build.DiskRoot.SizeGB, } // Configure secret provider (use NoOpSecretProvider as fallback to avoid nil panics) From 74bfc3bcbfb0e5b3ff73dfebd66444ef1f5caf53 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sun, 2 Aug 2026 03:44:56 +0000 Subject: [PATCH 2/5] builds: recreate leftover buildkit root volume on recovery A host crash mid-build leaked the deterministic build-disk- volume; on recovery, setupDiskRootVolume failed on ErrAlreadyExists and the recovered build could never run. Delete the leftover and create a fresh volume instead. --- lib/builds/disk_root_test.go | 43 ++++++++++++++++++++++++++++++++++++ lib/builds/manager.go | 13 +++++++++++ 2 files changed, 56 insertions(+) diff --git a/lib/builds/disk_root_test.go b/lib/builds/disk_root_test.go index 7b53d993..42d811ff 100644 --- a/lib/builds/disk_root_test.go +++ b/lib/builds/disk_root_test.go @@ -77,6 +77,49 @@ func TestSetupDiskRootVolume_CreateError(t *testing.T) { assert.Empty(t, volID) } +func TestSetupDiskRootVolume_LeftoverFromCrash(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + // Simulate a volume left behind by a crashed build: the first create + // fails with ErrAlreadyExists, the leftover is deleted, and the retry + // succeeds. + 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 + } + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.NoError(t, err) + assert.Equal(t, "build-disk-build-1", volID) + assert.Equal(t, 1, volumeMgr.deleteCallCount) + assert.Equal(t, 2, volumeMgr.createCallCount) +} + +func TestSetupDiskRootVolume_LeftoverDeleteError(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + return nil, volumes.ErrAlreadyExists + } + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + return errors.New("volume attached") + } + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.Error(t, err) + assert.Empty(t, volID) +} + 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 index bf73a333..86894493 100644 --- a/lib/builds/manager.go +++ b/lib/builds/manager.go @@ -5,6 +5,7 @@ import ( "context" _ "embed" "encoding/json" + "errors" "fmt" "log/slog" "net" @@ -799,6 +800,18 @@ func (m *manager) setupDiskRootVolume(ctx context.Context, buildID string) (stri Name: volID, SizeGb: sizeGB, }) + 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 { + return "", fmt.Errorf("delete leftover buildkit root volume: %w", delErr) + } + _, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{ + Id: &volID, + Name: volID, + SizeGb: sizeGB, + }) + } if err != nil { return "", err } From c7864cd2ce48127986ec1a1b15a685c755532327 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sun, 2 Aug 2026 04:43:20 +0000 Subject: [PATCH 3/5] builds: fix buildkit root mount validation and recovery - 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. --- lib/builds/disk_root_test.go | 94 +++++++++++++++++++++++++++ lib/builds/manager.go | 48 ++++++++++++-- lib/instances/create.go | 20 +++++- lib/instances/resource_limits_test.go | 20 ++++++ 4 files changed, 174 insertions(+), 8 deletions(-) diff --git a/lib/builds/disk_root_test.go b/lib/builds/disk_root_test.go index 42d811ff..705cda28 100644 --- a/lib/builds/disk_root_test.go +++ b/lib/builds/disk_root_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "path/filepath" "testing" "time" @@ -120,6 +121,99 @@ func TestSetupDiskRootVolume_LeftoverDeleteError(t *testing.T) { assert.Empty(t, volID) } +// TestSetupDiskRootVolume_LeftoverAttachedToStaleBuilder verifies recovery +// when a crash left the volume attached to a surviving builder VM: the first +// delete fails with ErrInUse, the stale builder is deleted (detaching the +// volume), and the retry succeeds. +func TestSetupDiskRootVolume_LeftoverAttachedToStaleBuilder(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + 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 + } + builderDeleted := false + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + if !builderDeleted { + return volumes.ErrInUse + } + return nil + } + instanceMgr.getFunc = func(ctx context.Context, id string) (*instances.Instance, error) { + if id == "builder-build-1" { + return &instances.Instance{ + StoredMetadata: instances.StoredMetadata{Id: "inst-builder-build-1", Name: "builder-build-1"}, + }, nil + } + return nil, instances.ErrNotFound + } + instanceMgr.deleteFunc = func(ctx context.Context, id string) error { + assert.Equal(t, "inst-builder-build-1", id) + builderDeleted = true + return nil + } + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.NoError(t, err) + assert.Equal(t, "build-disk-build-1", volID) + assert.True(t, builderDeleted, "stale builder holding the volume must be deleted") + assert.Equal(t, 2, volumeMgr.deleteCallCount) + assert.Equal(t, 2, volumeMgr.createCallCount) +} + +// TestSetupDiskRootVolume_LeftoverInUseWithoutBuilder verifies recovery fails +// loudly when the leftover volume is attached but no stale builder exists to +// clean up. +func TestSetupDiskRootVolume_LeftoverInUseWithoutBuilder(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + mgr.config.DiskRootEnabled = true + + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + return nil, volumes.ErrAlreadyExists + } + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + return volumes.ErrInUse + } + + volID, err := mgr.setupDiskRootVolume(context.Background(), "build-1") + + require.Error(t, err) + assert.Contains(t, err.Error(), "stale builder") + assert.Empty(t, volID) +} + +// TestCreateBuildConfigVolume_UniqueTempPath verifies the config disk is +// written to a unique per-call path, so concurrent builds reusing a build ID +// (as parallel test runs sharing TMPDIR do) never collide on a fixed file. +func TestCreateBuildConfigVolume_UniqueTempPath(t *testing.T) { + mgr, _, _, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + req := CreateBuildRequest{Dockerfile: "FROM alpine"} + prepareBuildOnDisk(t, mgr, "build-1", req) + + path1, err := mgr.createBuildConfigVolume("build-1", "build-config-build-1") + require.NoError(t, err) + defer os.RemoveAll(filepath.Dir(path1)) + path2, err := mgr.createBuildConfigVolume("build-1", "build-config-build-1") + require.NoError(t, err) + defer os.RemoveAll(filepath.Dir(path2)) + + assert.NotEqual(t, path1, path2) + for _, p := range []string{path1, path2} { + _, err := os.Stat(p) + require.NoError(t, err) + } +} + 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 index 86894493..23a87ef5 100644 --- a/lib/builds/manager.go +++ b/lib/builds/manager.go @@ -710,7 +710,7 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe if err != nil { return nil, fmt.Errorf("create config volume: %w", err) } - defer os.Remove(configVolPath) // Clean up the config disk file + defer os.RemoveAll(filepath.Dir(configVolPath)) // Clean up the config disk file // Register the config volume with the volume manager _, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{ @@ -803,8 +803,8 @@ func (m *manager) setupDiskRootVolume(ctx context.Context, buildID string) (stri 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 { - return "", fmt.Errorf("delete leftover buildkit root volume: %w", delErr) + if delErr := m.deleteLeftoverDiskRootVolume(ctx, buildID, volID); delErr != nil { + return "", delErr } _, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{ Id: &volID, @@ -818,6 +818,33 @@ func (m *manager) setupDiskRootVolume(ctx context.Context, buildID string) (stri return volID, nil } +// deleteLeftoverDiskRootVolume deletes the buildkit root volume left behind +// by a crashed build attempt. When the crash left the volume attached to a +// surviving builder VM the delete fails with ErrInUse; delete the stale +// builder (which detaches its volumes) and retry. +func (m *manager) deleteLeftoverDiskRootVolume(ctx context.Context, buildID, volID string) error { + err := m.volumeManager.DeleteVolume(ctx, volID) + if !errors.Is(err, volumes.ErrInUse) { + if err != nil { + return fmt.Errorf("delete leftover buildkit root volume: %w", err) + } + return nil + } + + 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) + } + 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) + } + if err := m.volumeManager.DeleteVolume(ctx, volID); err != nil { + return fmt.Errorf("delete leftover buildkit root volume after stale builder removal: %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 { @@ -1497,8 +1524,8 @@ func readFile(path string) ([]byte, error) { return os.ReadFile(path) } -// createBuildConfigVolume creates an ext4 disk containing the build.json config file -// Returns the path to the disk file +// createBuildConfigVolume creates an ext4 disk containing the build.json config file. +// Returns the path to the disk file; the caller removes its parent directory. func (m *manager) createBuildConfigVolume(buildID, volID string) (string, error) { // Read the build config configPath := m.paths.BuildConfig(buildID) @@ -1529,10 +1556,17 @@ func (m *manager) createBuildConfigVolume(buildID, volID string) (string, error) metadataPath := filepath.Join(tmpDir, "metadata.json") os.WriteFile(metadataPath, metadataData, 0644) - // Create ext4 disk from the directory - diskPath := filepath.Join(os.TempDir(), fmt.Sprintf("build-config-%s.ext4", buildID)) + // Create the ext4 disk in a dedicated temp directory so concurrent + // builds and parallel test runs sharing TMPDIR never collide on a + // fixed path. The caller removes the directory. + diskDir, err := os.MkdirTemp("", "hypeman-build-config-disk-*") + if err != nil { + return "", fmt.Errorf("create config disk dir: %w", err) + } + diskPath := filepath.Join(diskDir, "build-config.ext4") _, err = images.ExportRootfs(tmpDir, diskPath, images.FormatExt4) if err != nil { + os.RemoveAll(diskDir) return "", fmt.Errorf("create config disk: %w", err) } diff --git a/lib/instances/create.go b/lib/instances/create.go index 1bc6c867..0b97d09c 100644 --- a/lib/instances/create.go +++ b/lib/instances/create.go @@ -32,6 +32,13 @@ const ( MaxVolumesPerInstance = 23 ) +// allowedSystemMountPaths are exact paths exempt from the system directory +// check: internal services that own the path and require a volume mounted at +// a fixed location under it. +var allowedSystemMountPaths = []string{ + "/var/lib/buildkit", // BuildKit root volume in builder VMs +} + // systemDirectories are paths that cannot be used as volume mount points var systemDirectories = []string{ "/", @@ -665,7 +672,7 @@ func validateVolumeAttachments(volumes []VolumeAttachment) error { 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) } @@ -689,6 +696,17 @@ func validateVolumeAttachments(volumes []VolumeAttachment) error { return nil } +// isAllowedSystemMountPath reports whether path is an exact match for a +// system path that an internal service is allowed to mount a volume at. +func isAllowedSystemMountPath(path string) bool { + for _, allowed := range allowedSystemMountPaths { + if path == allowed { + return true + } + } + return false +} + // isSystemDirectory checks if a path is or is under a system directory func isSystemDirectory(path string) bool { cleanPath := filepath.Clean(path) diff --git a/lib/instances/resource_limits_test.go b/lib/instances/resource_limits_test.go index 32cad66c..86c8de7d 100644 --- a/lib/instances/resource_limits_test.go +++ b/lib/instances/resource_limits_test.go @@ -88,6 +88,26 @@ func TestValidateVolumeAttachments_Empty(t *testing.T) { assert.NoError(t, err) } +func TestValidateVolumeAttachments_AllowedSystemMountPath(t *testing.T) { + t.Parallel() + // The BuildKit root volume in builder VMs mounts at a fixed path under + // /var and is explicitly allowed. + err := validateVolumeAttachments([]VolumeAttachment{{ + VolumeID: "vol-1", + MountPath: "/var/lib/buildkit", + }}) + assert.NoError(t, err) + + // Parent paths and other /var subdirectories remain rejected. + for _, path := range []string{"/var", "/var/lib", "/var/lib/buildkit/nested", "/var/log"} { + err := validateVolumeAttachments([]VolumeAttachment{{ + VolumeID: "vol-1", + MountPath: path, + }}) + assert.Error(t, err, "expected %q to be rejected", path) + } +} + func TestValidateVolumeAttachments_OverlayRequiresReadonly(t *testing.T) { t.Parallel() // Overlay=true with Readonly=false should fail From 4654ffa75b7c7c6a4d31a48186d57b014f392ed4 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sun, 2 Aug 2026 05:24:21 +0000 Subject: [PATCH 4/5] instances: scope /var/lib/buildkit mount exemption to internal builders 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. --- lib/builds/manager.go | 13 +++++----- lib/instances/create.go | 17 +++++++----- lib/instances/resource_limits_test.go | 37 +++++++++++++++------------ lib/instances/types.go | 6 +++++ 4 files changed, 44 insertions(+), 29 deletions(-) diff --git a/lib/builds/manager.go b/lib/builds/manager.go index 23a87ef5..6062fbdc 100644 --- a/lib/builds/manager.go +++ b/lib/builds/manager.go @@ -750,12 +750,13 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe networkEnabled := policy.NetworkMode == "egress" inst, err := m.instanceManager.CreateInstance(ctx, instances.CreateInstanceRequest{ - Name: builderName, - Image: m.config.BuilderImage, - Size: int64(policy.MemoryMB) * 1024 * 1024, - Vcpus: policy.CPUs, - NetworkEnabled: networkEnabled, - Volumes: builderVolumeAttachments(sourceVolID, configVolID, diskRootVolID), + Name: builderName, + Image: m.config.BuilderImage, + Size: int64(policy.MemoryMB) * 1024 * 1024, + Vcpus: policy.CPUs, + NetworkEnabled: networkEnabled, + Volumes: builderVolumeAttachments(sourceVolID, configVolID, diskRootVolID), + AllowSystemVolumeMounts: true, // builder owns the BuildKit root mount }) if err != nil { return nil, fmt.Errorf("create builder instance: %w", err) diff --git a/lib/instances/create.go b/lib/instances/create.go index 0b97d09c..25f34d1c 100644 --- a/lib/instances/create.go +++ b/lib/instances/create.go @@ -33,8 +33,9 @@ const ( ) // allowedSystemMountPaths are exact paths exempt from the system directory -// check: internal services that own the path and require a volume mounted at -// a fixed location under it. +// check for instances created with AllowSystemVolumeMounts: internal +// services that own the path and require a volume mounted at a fixed +// location under it. var allowedSystemMountPaths = []string{ "/var/lib/buildkit", // BuildKit root volume in builder VMs } @@ -640,15 +641,17 @@ func validateCreateRequest(req *CreateInstanceRequest) error { req.RestartPolicy = normalizedRestartPolicy // Validate volume attachments - if err := validateVolumeAttachments(req.Volumes); err != nil { + if err := validateVolumeAttachments(req.Volumes, req.AllowSystemVolumeMounts); err != nil { return err } return nil } -// validateVolumeAttachments validates volume attachment requests -func validateVolumeAttachments(volumes []VolumeAttachment) error { +// validateVolumeAttachments validates volume attachment requests. +// allowSystemPaths permits mounts at the reserved paths in +// allowedSystemMountPaths and is only set for internal instances. +func validateVolumeAttachments(volumes []VolumeAttachment, allowSystemPaths bool) error { // Count total devices needed (each overlay volume needs 2 devices: base + overlay) totalDevices := 0 for _, vol := range volumes { @@ -672,7 +675,7 @@ func validateVolumeAttachments(volumes []VolumeAttachment) error { cleanPath := filepath.Clean(vol.MountPath) // Check for system directories - if isSystemDirectory(cleanPath) && !isAllowedSystemMountPath(cleanPath) { + if isSystemDirectory(cleanPath) && !(allowSystemPaths && isAllowedSystemMountPath(cleanPath)) { return fmt.Errorf("volume %s: cannot mount to system directory %q", vol.VolumeID, cleanPath) } @@ -697,7 +700,7 @@ func validateVolumeAttachments(volumes []VolumeAttachment) error { } // isAllowedSystemMountPath reports whether path is an exact match for a -// system path that an internal service is allowed to mount a volume at. +// system path that an internal instance may mount a volume at. func isAllowedSystemMountPath(path string) bool { for _, allowed := range allowedSystemMountPaths { if path == allowed { diff --git a/lib/instances/resource_limits_test.go b/lib/instances/resource_limits_test.go index 86c8de7d..f1d50a98 100644 --- a/lib/instances/resource_limits_test.go +++ b/lib/instances/resource_limits_test.go @@ -27,7 +27,7 @@ func TestValidateVolumeAttachments_MaxVolumes(t *testing.T) { } } - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "cannot attach more than 23") } @@ -39,7 +39,7 @@ func TestValidateVolumeAttachments_SystemDirectory(t *testing.T) { MountPath: "/etc/secrets", // system directory }} - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "system directory") } @@ -51,7 +51,7 @@ func TestValidateVolumeAttachments_DuplicatePaths(t *testing.T) { {VolumeID: "vol-2", MountPath: "/mnt/data"}, // duplicate } - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "duplicate mount path") } @@ -63,7 +63,7 @@ func TestValidateVolumeAttachments_RelativePath(t *testing.T) { MountPath: "relative/path", // not absolute }} - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "must be absolute") } @@ -75,35 +75,40 @@ func TestValidateVolumeAttachments_Valid(t *testing.T) { {VolumeID: "vol-2", MountPath: "/mnt/logs"}, } - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.NoError(t, err) } func TestValidateVolumeAttachments_Empty(t *testing.T) { t.Parallel() - err := validateVolumeAttachments(nil) + err := validateVolumeAttachments(nil, false) assert.NoError(t, err) - err = validateVolumeAttachments([]VolumeAttachment{}) + err = validateVolumeAttachments([]VolumeAttachment{}, false) assert.NoError(t, err) } func TestValidateVolumeAttachments_AllowedSystemMountPath(t *testing.T) { t.Parallel() // The BuildKit root volume in builder VMs mounts at a fixed path under - // /var and is explicitly allowed. - err := validateVolumeAttachments([]VolumeAttachment{{ + // /var and is allowed only for internal instances that opt in. + buildkitRoot := []VolumeAttachment{{ VolumeID: "vol-1", MountPath: "/var/lib/buildkit", - }}) + }} + err := validateVolumeAttachments(buildkitRoot, true) assert.NoError(t, err) - // Parent paths and other /var subdirectories remain rejected. + err = validateVolumeAttachments(buildkitRoot, false) + assert.Error(t, err, "system mount path rejected without AllowSystemVolumeMounts") + + // Parent paths and other /var subdirectories remain rejected even for + // internal instances. for _, path := range []string{"/var", "/var/lib", "/var/lib/buildkit/nested", "/var/log"} { err := validateVolumeAttachments([]VolumeAttachment{{ VolumeID: "vol-1", MountPath: path, - }}) + }}, true) assert.Error(t, err, "expected %q to be rejected", path) } } @@ -119,7 +124,7 @@ func TestValidateVolumeAttachments_OverlayRequiresReadonly(t *testing.T) { OverlaySize: 100 * 1024 * 1024, }} - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "overlay mode requires readonly=true") } @@ -135,7 +140,7 @@ func TestValidateVolumeAttachments_OverlayRequiresSize(t *testing.T) { OverlaySize: 0, // Invalid: overlay requires size }} - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "overlay_size is required") } @@ -151,7 +156,7 @@ func TestValidateVolumeAttachments_OverlayValid(t *testing.T) { OverlaySize: 100 * 1024 * 1024, // 100MB }} - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.NoError(t, err) } @@ -171,7 +176,7 @@ func TestValidateVolumeAttachments_OverlayCountsAsTwoDevices(t *testing.T) { } } - err := validateVolumeAttachments(volumes) + err := validateVolumeAttachments(volumes, false) assert.Error(t, err) assert.Contains(t, err.Error(), "cannot attach more than 23") } diff --git a/lib/instances/types.go b/lib/instances/types.go index 1ff97575..f7227185 100644 --- a/lib/instances/types.go +++ b/lib/instances/types.go @@ -274,6 +274,12 @@ type CreateInstanceRequest struct { AutoStandby *autostandby.Policy // Optional automatic standby policy HealthCheck *healthcheck.Policy // Optional workload health check policy RestartPolicy *restartpolicy.Policy // Optional whole-instance restart policy + + // AllowSystemVolumeMounts permits volume mounts at the reserved system + // paths in allowedSystemMountPaths. Only set by internal services that + // own those paths (e.g. builder VMs mounting the BuildKit root); it is + // never populated from API requests. + AllowSystemVolumeMounts bool } // StartInstanceRequest is the domain request for starting a stopped instance From bba318a5aab95ec52f1c4102284251ea7efc0511 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 2 Aug 2026 05:47:19 +0000 Subject: [PATCH 5/5] Fix wrapped error when stale builder lookup fails --- lib/builds/disk_root_test.go | 1 + lib/builds/manager.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/builds/disk_root_test.go b/lib/builds/disk_root_test.go index 705cda28..c9c80f98 100644 --- a/lib/builds/disk_root_test.go +++ b/lib/builds/disk_root_test.go @@ -187,6 +187,7 @@ func TestSetupDiskRootVolume_LeftoverInUseWithoutBuilder(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "stale builder") + assert.ErrorIs(t, err, instances.ErrNotFound, "error must wrap the builder lookup failure, not the volume delete error") assert.Empty(t, volID) } diff --git a/lib/builds/manager.go b/lib/builds/manager.go index 6062fbdc..d41a8c24 100644 --- a/lib/builds/manager.go +++ b/lib/builds/manager.go @@ -835,7 +835,7 @@ func (m *manager) deleteLeftoverDiskRootVolume(ctx context.Context, buildID, vol 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)