diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 2e72bd013..6f512e595 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -16,6 +16,12 @@ import ( const testArch = "amd64" +const ( + testSeedWorkspaceID = "ws1" + testSeedVolumeName = "ws1-workspace" + testSeedSourceDir = "/local/src" +) + func TestLocalDockerDelivery_Phase(t *testing.T) { d := &LocalDockerDelivery{} assert.Equal(t, PhasePreStart, d.Phase()) @@ -415,3 +421,56 @@ func TestLocalDockerDelivery_Cleanup_RemovesManagedVolumes(t *testing.T) { assert.Contains(t, removed, "devsy-agent-ws1") assert.Contains(t, removed, "ws1-workspace") } + +func TestLocalDockerDelivery_SeedExcludesBuildInternal(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "run.log") + + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 1; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"create\" ]; then exit 0; fi\n" + + "if [ \"$1\" = \"run\" ]; then echo \"$@\" >> \"" + logPath + "\"; exit 0; fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ + WorkspaceID: testSeedWorkspaceID, + VolumeName: testSeedVolumeName, + SourceDir: testSeedSourceDir, + }) + require.NoError(t, err) + + logged, err := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control + require.NoError(t, err) + assert.Contains(t, string(logged), "--exclude") + assert.Contains(t, string(logged), ".devsy-internal") +} + +func TestLocalDockerDelivery_Seed_CopyAndCleanupFailureJoined(t *testing.T) { + tmpDir := t.TempDir() + + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"inspect\" ]; then exit 1; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"create\" ]; then exit 0; fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"rm\" ]; then echo 'rm boom' 1>&2; exit 1; fi\n" + + "if [ \"$1\" = \"run\" ]; then echo 'copy boom' 1>&2; exit 1; fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + err := d.SeedWorkspaceVolume(context.Background(), WorkspaceSeedOptions{ + WorkspaceID: testSeedWorkspaceID, + VolumeName: testSeedVolumeName, + SourceDir: testSeedSourceDir, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "seed workspace volume") + assert.Contains(t, err.Error(), "remove partial volume") +} diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go index 9d50fcf4c..953a08ebf 100644 --- a/pkg/agent/delivery/workspace_seed.go +++ b/pkg/agent/delivery/workspace_seed.go @@ -2,10 +2,12 @@ package delivery import ( "context" + "errors" "fmt" "strings" pkgconfig "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" ) @@ -52,18 +54,21 @@ func (d *LocalDockerDelivery) SeedWorkspaceVolume( } labels := pkgconfig.DockerVolumeLabels(opts.WorkspaceID, pkgconfig.VolumeRoleWorkspace) + labels[pkgconfig.DockerSeededLabel] = pkgconfig.LabelValueTrue if err := d.createVolume(ctx, opts.VolumeName, labels); err != nil { return fmt.Errorf("create workspace volume: %w", err) } if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { - // Leave the volume in place but unseeded so a retry re-copies. + if rmErr := d.removeVolume(ctx, opts.VolumeName); rmErr != nil { + return errors.Join( + fmt.Errorf("seed workspace volume: %w", err), + fmt.Errorf("remove partial volume %s: %w", opts.VolumeName, rmErr), + ) + } return fmt.Errorf("seed workspace volume: %w", err) } - if err := d.markVolumeSeeded(ctx, opts.VolumeName); err != nil { - log.Debugf("failed to mark volume %s seeded: %v", opts.VolumeName, err) - } log.Infof("seeded workspace volume %s from %s", opts.VolumeName, opts.SourceDir) return nil } @@ -107,10 +112,6 @@ func (d *LocalDockerDelivery) prepareSeedTarget( return true, nil } -// volumeSeedState reports whether the volume is devsy-managed (from its label) -// and whether it has already been seeded (from a sentinel file written inside -// the volume after a successful copy). Docker cannot add labels to an existing -// volume, so seeded state is tracked with the sentinel rather than a label. func (d *LocalDockerDelivery) volumeSeedState( ctx context.Context, name string, @@ -121,26 +122,15 @@ func (d *LocalDockerDelivery) volumeSeedState( out, err := d.cmd(ctx, "volume", "inspect", - "--format", "{{index .Labels \""+pkgconfig.DockerManagedLabel+"\"}}", + "--format", "{{index .Labels \""+pkgconfig.DockerManagedLabel+"\"}},"+ + "{{index .Labels \""+pkgconfig.DockerSeededLabel+"\"}}", name, ).CombinedOutput() if err != nil { return false, false, fmt.Errorf("inspect volume %s: %s: %w", name, string(out), err) } - managed = strings.TrimSpace(string(out)) == "true" - - return managed, d.volumeSeeded(ctx, name), nil -} - -// volumeSeeded reports whether the seeded sentinel file exists in the volume. -func (d *LocalDockerDelivery) volumeSeeded(ctx context.Context, name string) bool { - args := []string{ - cmdRun, flagRM, - "-v", name + ":/target:ro", - d.helperImageName(), - "sh", "-c", "[ -e /target/" + seededSentinel + " ]", - } - return d.cmd(ctx, args...).Run() == nil + managedStr, seededStr, _ := strings.Cut(strings.TrimSpace(string(out)), ",") + return managedStr == pkgconfig.LabelValueTrue, seededStr == pkgconfig.LabelValueTrue, nil } func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) bool { @@ -148,35 +138,22 @@ func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) boo return err == nil } -// copyDirIntoVolume copies the contents of sourceDir into the volume root using -// a throwaway helper container. The source is bind-mounted read-only. func (d *LocalDockerDelivery) copyDirIntoVolume( ctx context.Context, sourceDir, volumeName string, ) error { - args := []string{ - cmdRun, flagRM, - "-v", sourceDir + ":/source:ro", - "-v", volumeName + ":/target", - d.helperImageName(), - "sh", "-c", "cp -a /source/. /target/", - } - out, err := d.cmd(ctx, args...).CombinedOutput() - if err != nil { - return fmt.Errorf("%s: %w", string(out), err) + var script strings.Builder + script.WriteString("cd /source && tar -c") + for _, artifact := range config.BuildArtifactExcludes() { + script.WriteString(" --exclude='" + artifact + "'") } - return nil -} - -// markVolumeSeeded records that the volume has been populated by writing a -// sentinel file inside it. Docker cannot add labels to an existing volume, so -// the sentinel is the source of truth for seeded state. -func (d *LocalDockerDelivery) markVolumeSeeded(ctx context.Context, volumeName string) error { + script.WriteString(" . | tar -x -C /target") args := []string{ cmdRun, flagRM, + "-v", sourceDir + ":/source:ro", "-v", volumeName + ":/target", d.helperImageName(), - "sh", "-c", "touch /target/" + seededSentinel, + "sh", "-c", script.String(), } out, err := d.cmd(ctx, args...).CombinedOutput() if err != nil { @@ -184,5 +161,3 @@ func (d *LocalDockerDelivery) markVolumeSeeded(ctx context.Context, volumeName s } return nil } - -const seededSentinel = ".devsy-seeded" diff --git a/pkg/agent/tunnelserver/tunnelserver.go b/pkg/agent/tunnelserver/tunnelserver.go index 637fcf4b3..cddac4942 100644 --- a/pkg/agent/tunnelserver/tunnelserver.go +++ b/pkg/agent/tunnelserver/tunnelserver.go @@ -453,6 +453,8 @@ func (t *tunnelServer) StreamWorkspace( } } + excludes = append(excludes, config.BuildArtifactExcludes()...) + buf := bufio.NewWriterSize(NewStreamWriter(stream), 10*1024) err = extract.WriteTarExclude(buf, t.workspace.Source.LocalFolder, false, excludes) if err != nil { @@ -497,6 +499,8 @@ func (t *tunnelServer) StreamMount( } } + excludes = append(excludes, config.BuildArtifactExcludes()...) + buf := bufio.NewWriterSize(NewStreamWriter(stream), 10*1024) err := extract.WriteTarExclude(buf, mount.Source, false, excludes) if err != nil { diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index cb3b03e50..2c137226a 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -672,8 +672,7 @@ func getContainerContextAndDockerfile( } func cleanupBuildInformation(c *config.DevContainerConfig) { - contextPath := config.GetContextPath(c) - _ = os.RemoveAll(filepath.Join(contextPath, config.DevsyContextFeatureFolder)) + config.RemoveBuildArtifacts(config.GetContextPath(c)) } func featureSecretOpts(options provider.BuildOptions) *feature.SecretOptions { diff --git a/pkg/devcontainer/config/artifacts.go b/pkg/devcontainer/config/artifacts.go new file mode 100644 index 000000000..cae41b5b7 --- /dev/null +++ b/pkg/devcontainer/config/artifacts.go @@ -0,0 +1,25 @@ +package config + +import ( + "os" + "path/filepath" + + "github.com/devsy-org/devsy/pkg/log" +) + +var buildArtifactNames = []string{ + DevsyContextFeatureFolder, +} + +func BuildArtifactExcludes() []string { + return append([]string(nil), buildArtifactNames...) +} + +func RemoveBuildArtifacts(contextPath string) { + for _, name := range buildArtifactNames { + path := filepath.Join(contextPath, name) + if err := os.RemoveAll(path); err != nil { + log.Debugf("failed to remove build artifact %s: %v", path, err) + } + } +} diff --git a/pkg/devcontainer/config/artifacts_test.go b/pkg/devcontainer/config/artifacts_test.go new file mode 100644 index 000000000..a9573f012 --- /dev/null +++ b/pkg/devcontainer/config/artifacts_test.go @@ -0,0 +1,53 @@ +package config + +import ( + "os" + "path/filepath" + "slices" + "testing" +) + +func TestBuildArtifactExcludesIncludesFeatureFolder(t *testing.T) { + got := BuildArtifactExcludes() + if !slices.Contains(got, DevsyContextFeatureFolder) { + t.Errorf("expected %q in excludes, got %v", DevsyContextFeatureFolder, got) + } +} + +func TestBuildArtifactExcludesReturnsCopy(t *testing.T) { + got := BuildArtifactExcludes() + if len(got) == 0 { + t.Fatal("expected at least one artifact") + } + got[0] = "mutated" + if BuildArtifactExcludes()[0] == "mutated" { + t.Error("BuildArtifactExcludes must return a defensive copy") + } +} + +func TestRemoveBuildArtifacts(t *testing.T) { + contextPath := t.TempDir() + artifactDir := filepath.Join(contextPath, DevsyContextFeatureFolder) + // #nosec G301 -- test fixture + if err := os.MkdirAll(filepath.Join(artifactDir, "0"), 0o755); err != nil { + t.Fatal(err) + } + keep := filepath.Join(contextPath, "keep.txt") + // #nosec G306 -- test fixture + if err := os.WriteFile(keep, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + + RemoveBuildArtifacts(contextPath) + + if _, err := os.Stat(artifactDir); !os.IsNotExist(err) { + t.Errorf("expected artifact dir removed, stat err = %v", err) + } + if _, err := os.Stat(keep); err != nil { + t.Errorf("workspace file should survive, got %v", err) + } +} + +func TestRemoveBuildArtifactsAbsentIsNoError(t *testing.T) { + RemoveBuildArtifacts(t.TempDir()) +} diff --git a/pkg/devcontainer/config/prebuild.go b/pkg/devcontainer/config/prebuild.go index 479d6b72e..0b787e24a 100644 --- a/pkg/devcontainer/config/prebuild.go +++ b/pkg/devcontainer/config/prebuild.go @@ -42,7 +42,9 @@ func CalculatePrebuildHash(params PrebuildHashParams) (string, error) { log.Debugf("failed to read .dockerignore: %v", err) return "", fmt.Errorf("failed to read dockerignore: %w", err) } - excludes = append(excludes, DevsyContextFeatureFolder+"/") + for _, artifact := range BuildArtifactExcludes() { + excludes = append(excludes, artifact+"/") + } var includes []string if params.BuildInfo != nil && params.BuildInfo.Dockerfile != nil { diff --git a/pkg/devcontainer/prebuild.go b/pkg/devcontainer/prebuild.go index ebfc876b4..410263d54 100644 --- a/pkg/devcontainer/prebuild.go +++ b/pkg/devcontainer/prebuild.go @@ -3,8 +3,6 @@ package devcontainer import ( "context" "fmt" - "os" - "path/filepath" "strings" "github.com/devsy-org/devsy/pkg/devcontainer/build" @@ -27,11 +25,7 @@ func (r *runner) Build(ctx context.Context, options provider.BuildOptions) (stri prebuildRepo := getPrebuildRepository(substitutedConfig) - // remove build information - defer func() { - contextPath := config.GetContextPath(substitutedConfig.Config) - _ = os.RemoveAll(filepath.Join(contextPath, config.DevsyContextFeatureFolder)) - }() + defer config.RemoveBuildArtifacts(config.GetContextPath(substitutedConfig.Config)) // check if we need to build container buildInfo, err := r.build(ctx, substitutedConfig, substitutionContext, options)