diff --git a/lib/builds/manager.go b/lib/builds/manager.go index 87f21dfe..92d02c17 100644 --- a/lib/builds/manager.go +++ b/lib/builds/manager.go @@ -5,7 +5,9 @@ import ( "context" _ "embed" "encoding/json" + "errors" "fmt" + "io" "log/slog" "net" "os" @@ -678,13 +680,8 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe defer sourceFile.Close() // Create volume with source (using the volume manager's archive import) - _, err = m.volumeManager.CreateVolumeFromArchive(ctx, volumes.CreateVolumeFromArchiveRequest{ - Id: &sourceVolID, - Name: sourceVolID, - SizeGb: 10, // 10GB should be enough for most source bundles - }, sourceFile) - if err != nil { - return nil, fmt.Errorf("create source volume: %w", err) + if err := m.createBuildSourceVolume(ctx, id, sourceVolID, sourceFile); err != nil { + return nil, err } defer m.volumeManager.DeleteVolume(context.Background(), sourceVolID) @@ -697,25 +694,8 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe defer os.Remove(configVolPath) // Clean up the config disk file // Register the config volume with the volume manager - _, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{ - Id: &configVolID, - Name: configVolID, - SizeGb: 1, - }) - if err != nil { - // If volume creation fails, try to use the disk file directly - // by copying it to the expected location - volPath := m.paths.VolumeData(configVolID) - if copyErr := copyFile(configVolPath, volPath); copyErr != nil { - return nil, fmt.Errorf("setup config volume: %w", copyErr) - } - } else { - // Copy our config disk over the empty volume - volPath := m.paths.VolumeData(configVolID) - if err := copyFile(configVolPath, volPath); err != nil { - m.volumeManager.DeleteVolume(context.Background(), configVolID) - return nil, fmt.Errorf("write config to volume: %w", err) - } + if err := m.registerBuildConfigVolume(ctx, id, configVolID, configVolPath); err != nil { + return nil, err } defer m.volumeManager.DeleteVolume(context.Background(), configVolID) @@ -767,6 +747,96 @@ func (m *manager) executeBuild(ctx context.Context, id string, req CreateBuildRe return result, nil } +// createBuildSourceVolume creates the source volume for a build. Build volume +// names are deterministic (build-source-), so a re-run of the same build +// after a crash (e.g. via RecoverPendingBuilds) can hit a leftover volume from +// the interrupted attempt. Tolerate that case: remove the leftover and retry +// the create once. +func (m *manager) createBuildSourceVolume(ctx context.Context, buildID, volID string, source io.Reader) error { + req := volumes.CreateVolumeFromArchiveRequest{ + Id: &volID, + Name: volID, + SizeGb: 10, // 10GB should be enough for most source bundles + } + _, err := m.volumeManager.CreateVolumeFromArchive(ctx, req, source) + if errors.Is(err, volumes.ErrAlreadyExists) { + m.logger.Info("removing leftover source volume from crashed build attempt", "build_id", buildID, "volume", volID) + if delErr := m.deleteLeftoverBuildVolume(ctx, buildID, volID); delErr != nil { + return fmt.Errorf("remove leftover source volume: %w", delErr) + } + _, err = m.volumeManager.CreateVolumeFromArchive(ctx, req, source) + } + if err != nil { + return fmt.Errorf("create source volume: %w", err) + } + return nil +} + +// registerBuildConfigVolume registers the config disk as a volume and copies +// the config data onto it. Like the source volume, the config volume has a +// deterministic name (build-config-), so a re-run of the same build after +// a crash can hit a leftover from the interrupted attempt; remove it and retry +// the create once rather than silently copying over the stale volume. +func (m *manager) registerBuildConfigVolume(ctx context.Context, buildID, volID, configDiskPath string) error { + req := volumes.CreateVolumeRequest{ + Id: &volID, + Name: volID, + SizeGb: 1, + } + _, err := m.volumeManager.CreateVolume(ctx, req) + if errors.Is(err, volumes.ErrAlreadyExists) { + m.logger.Info("removing leftover config volume from crashed build attempt", "build_id", buildID, "volume", volID) + if delErr := m.deleteLeftoverBuildVolume(ctx, buildID, volID); delErr != nil { + return fmt.Errorf("remove leftover config volume: %w", delErr) + } + _, err = m.volumeManager.CreateVolume(ctx, req) + } + if err != nil { + // If volume creation fails, try to use the disk file directly + // by copying it to the expected location + volPath := m.paths.VolumeData(volID) + if copyErr := copyFile(configDiskPath, volPath); copyErr != nil { + return fmt.Errorf("setup config volume: %w", copyErr) + } + return nil + } + // Copy our config disk over the empty volume + volPath := m.paths.VolumeData(volID) + if err := copyFile(configDiskPath, volPath); err != nil { + m.volumeManager.DeleteVolume(context.Background(), volID) + return fmt.Errorf("write config to volume: %w", err) + } + return nil +} + +// deleteLeftoverBuildVolume removes a build volume left behind by a crashed +// prior attempt of the same build. If the volume is still attached, it is +// almost certainly attached to the crashed attempt's stale builder instance +// (builder-); delete that instance (which detaches all of its volumes) and +// retry the volume delete. A volume attached to an unknown instance is never +// force-deleted; an error is returned instead. +func (m *manager) deleteLeftoverBuildVolume(ctx context.Context, buildID, volID string) error { + err := m.volumeManager.DeleteVolume(ctx, volID) + if !errors.Is(err, volumes.ErrInUse) { + return err + } + + builderName := fmt.Sprintf("builder-%s", buildID) + inst, getErr := m.instanceManager.GetInstance(ctx, builderName) + if getErr != nil { + if errors.Is(getErr, instances.ErrNotFound) { + return fmt.Errorf("volume %s is in use but stale builder instance %q was not found; refusing to force-delete a volume attached to an unknown instance", volID, builderName) + } + return fmt.Errorf("look up stale builder instance %q: %w", builderName, getErr) + } + + m.logger.Info("deleting stale builder instance from crashed build attempt", "build_id", buildID, "instance", inst.Id, "volume", volID) + if delErr := m.instanceManager.DeleteInstance(ctx, inst.Id); delErr != nil { + return fmt.Errorf("delete stale builder instance %s: %w", inst.Id, delErr) + } + return m.volumeManager.DeleteVolume(ctx, volID) +} + // 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/builds/manager_test.go b/lib/builds/manager_test.go index c26295ea..57cbd1df 100644 --- a/lib/builds/manager_test.go +++ b/lib/builds/manager_test.go @@ -1067,6 +1067,187 @@ eventLoop: } } +// setupBuildInputs writes the on-disk source tarball and build config that +// executeBuild expects for a build. +func setupBuildInputs(t *testing.T, mgr *manager, buildID string) { + t.Helper() + sourceDir := mgr.paths.BuildSourceDir(buildID) + require.NoError(t, os.MkdirAll(sourceDir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(sourceDir, "source.tar.gz"), []byte("fake-tarball-data"), 0644)) + require.NoError(t, os.MkdirAll(filepath.Dir(mgr.paths.BuildConfig(buildID)), 0755)) + require.NoError(t, os.WriteFile(mgr.paths.BuildConfig(buildID), []byte(`{"job_id":"`+buildID+`"}`), 0644)) +} + +// TestExecuteBuild_SourceVolumeAlreadyExists verifies that a leftover source +// volume from a crashed prior attempt of the same build is deleted and +// recreated, allowing the re-run to proceed past source volume creation. +func TestExecuteBuild_SourceVolumeAlreadyExists(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + buildID := "build-crash-src" + setupBuildInputs(t, mgr, buildID) + + var archiveCalls int + var deleted []string + volumeMgr.createFromArchiveFunc = func(ctx context.Context, req volumes.CreateVolumeFromArchiveRequest, archive io.Reader) (*volumes.Volume, error) { + archiveCalls++ + if archiveCalls == 1 { + // Leftover from the crashed prior attempt + return nil, volumes.ErrAlreadyExists + } + vol := &volumes.Volume{Id: *req.Id, Name: req.Name} + volumeMgr.volumes[vol.Id] = vol + return vol, nil + } + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + deleted = append(deleted, id) + delete(volumeMgr.volumes, id) + return nil + } + // Short-circuit once volume setup is done so the test doesn't enter the + // vsock wait loop. + instanceMgr.createFunc = func(ctx context.Context, req instances.CreateInstanceRequest) (*instances.Instance, error) { + return nil, fmt.Errorf("stop after volume setup") + } + + _, err := mgr.executeBuild(context.Background(), buildID, CreateBuildRequest{}, &BuildPolicy{}) + + // The build must get past source volume creation (it may fail later, e.g. + // at config disk creation or instance launch in the test environment). + require.Error(t, err) + assert.NotContains(t, err.Error(), "create source volume") + assert.Equal(t, 2, archiveCalls, "expected delete + retry of source volume creation") + require.NotEmpty(t, deleted) + assert.Equal(t, "build-source-"+buildID, deleted[0]) +} + +// TestExecuteBuild_SourceVolumeInUse_StaleBuilder verifies that when the +// leftover source volume is still attached to the crashed attempt's stale +// builder instance, the builder is deleted (detaching the volume) before the +// volume is deleted and recreated. +func TestExecuteBuild_SourceVolumeInUse_StaleBuilder(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + buildID := "build-crash-inuse" + setupBuildInputs(t, mgr, buildID) + + // Seed the stale builder instance from the crashed attempt + builderName := "builder-" + buildID + instanceMgr.instances[builderName] = &instances.Instance{ + StoredMetadata: instances.StoredMetadata{Id: builderName, Name: builderName}, + State: instances.StateRunning, + } + + var archiveCalls int + var deleteCalls int + volumeMgr.createFromArchiveFunc = func(ctx context.Context, req volumes.CreateVolumeFromArchiveRequest, archive io.Reader) (*volumes.Volume, error) { + archiveCalls++ + if archiveCalls == 1 { + return nil, volumes.ErrAlreadyExists + } + vol := &volumes.Volume{Id: *req.Id, Name: req.Name} + volumeMgr.volumes[vol.Id] = vol + return vol, nil + } + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + deleteCalls++ + if deleteCalls == 1 { + // Still attached to the stale builder + return volumes.ErrInUse + } + delete(volumeMgr.volumes, id) + return nil + } + // Short-circuit once volume setup is done so the test doesn't enter the + // vsock wait loop. + instanceMgr.createFunc = func(ctx context.Context, req instances.CreateInstanceRequest) (*instances.Instance, error) { + return nil, fmt.Errorf("stop after volume setup") + } + + _, err := mgr.executeBuild(context.Background(), buildID, CreateBuildRequest{}, &BuildPolicy{}) + + require.Error(t, err) + assert.NotContains(t, err.Error(), "create source volume") + assert.Equal(t, 2, archiveCalls, "expected delete + retry of source volume creation") + assert.Equal(t, 1, instanceMgr.deleteCallCount, "expected stale builder instance to be deleted") + _, getErr := instanceMgr.GetInstance(context.Background(), builderName) + assert.ErrorIs(t, getErr, instances.ErrNotFound, "stale builder instance should be gone") +} + +// TestExecuteBuild_SourceVolumeInUse_NoStaleBuilder verifies that when the +// leftover source volume is in use but the crashed attempt's builder instance +// is gone, the build fails with a clear error and nothing is force-deleted. +func TestExecuteBuild_SourceVolumeInUse_NoStaleBuilder(t *testing.T) { + mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + buildID := "build-crash-orphan" + setupBuildInputs(t, mgr, buildID) + + var archiveCalls int + volumeMgr.createFromArchiveFunc = func(ctx context.Context, req volumes.CreateVolumeFromArchiveRequest, archive io.Reader) (*volumes.Volume, error) { + archiveCalls++ + return nil, volumes.ErrAlreadyExists + } + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + return volumes.ErrInUse + } + + _, err := mgr.executeBuild(context.Background(), buildID, CreateBuildRequest{}, &BuildPolicy{}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to force-delete") + assert.Equal(t, 1, archiveCalls, "create must not be retried when the leftover cannot be removed") + assert.Equal(t, 0, instanceMgr.deleteCallCount, "no instance should be deleted") +} + +// TestRegisterBuildConfigVolume_AlreadyExists verifies that a leftover config +// volume from a crashed prior attempt of the same build is explicitly deleted +// and recreated rather than silently masked by the copy-over fallback. +func TestRegisterBuildConfigVolume_AlreadyExists(t *testing.T) { + mgr, _, volumeMgr, tempDir := setupTestManager(t) + defer os.RemoveAll(tempDir) + + buildID := "build-crash-config" + configVolID := "build-config-" + buildID + + // Dummy config disk to copy over the recreated volume + configData := []byte("fake-ext4-config-disk") + configDiskPath := filepath.Join(tempDir, "config.ext4") + require.NoError(t, os.WriteFile(configDiskPath, configData, 0644)) + + var createCalls int + var deleted []string + volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) { + createCalls++ + if createCalls == 1 { + // Leftover from the crashed prior attempt + return nil, volumes.ErrAlreadyExists + } + vol := &volumes.Volume{Id: *req.Id, Name: req.Name} + volumeMgr.volumes[vol.Id] = vol + return vol, nil + } + volumeMgr.deleteFunc = func(ctx context.Context, id string) error { + deleted = append(deleted, id) + delete(volumeMgr.volumes, id) + return nil + } + + err := mgr.registerBuildConfigVolume(context.Background(), buildID, configVolID, configDiskPath) + + require.NoError(t, err) + assert.Equal(t, 2, createCalls, "expected delete + retry of config volume creation") + assert.Equal(t, []string{configVolID}, deleted) + assert.Contains(t, volumeMgr.volumes, configVolID, "config volume should be recreated") + // The config data must have been copied onto the recreated volume + copied, readErr := os.ReadFile(mgr.paths.VolumeData(configVolID)) + require.NoError(t, readErr) + assert.Equal(t, configData, copied) +} + func TestExtractInternalBaseImageRepos(t *testing.T) { registryURL := "http://10.102.0.1:8085"