From 63a2ec149b72145a10d466909e221e34a1f09e92 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 24 Mar 2025 16:31:41 +0000 Subject: [PATCH 01/15] wip: feat(agent): add devcontainer autostart support --- agent/agent.go | 8 ++++- agent/agentcontainers/devcontainer.go | 25 ++++++++++++++ agent/agentscripts/agentscripts.go | 48 +++++++++++++++++++++++---- 3 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 agent/agentcontainers/devcontainer.go diff --git a/agent/agent.go b/agent/agent.go index 6d7c1c8038daa..bbf38f3a37aa9 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1115,7 +1115,12 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } } - err = a.scriptRunner.Init(manifest.Scripts, aAPI.ScriptCompleted) + var postStartScripts []codersdk.WorkspaceAgentScript + for _, dc := range manifest.Devcontainers { + postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc)) + } + + err = a.scriptRunner.Init(manifest.Scripts, aAPI.ScriptCompleted, agentscripts.WithPostStartScripts(postStartScripts...)) if err != nil { return xerrors.Errorf("init script runner: %w", err) } @@ -1124,6 +1129,7 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // here we use the graceful context because the script runner is not directly tied // to the agent API. err := a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecuteStartScripts) + err = errors.Join(err, a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecutePostStartScripts)) // Measure the time immediately after the script has finished dur := time.Since(start).Seconds() if err != nil { diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go new file mode 100644 index 0000000000000..4b84de9ab55e7 --- /dev/null +++ b/agent/agentcontainers/devcontainer.go @@ -0,0 +1,25 @@ +package agentcontainers + +import ( + "fmt" + + "github.com/google/uuid" + + "github.com/coder/coder/v2/codersdk" +) + +func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentScript { + script := fmt.Sprintf("devcontainer up --workspace-folder %q", dc.WorkspaceFolder) + if dc.ConfigPath != "" { + script = fmt.Sprintf("%s --config %q", script, dc.ConfigPath) + } + return codersdk.WorkspaceAgentScript{ + ID: uuid.New(), + LogSourceID: uuid.Nil, // TODO(mafredri): Add a devcontainer log source? + LogPath: "", + Script: script, + Cron: "", + Timeout: 0, + DisplayName: fmt.Sprintf("Dev Container (%s)", dc.WorkspaceFolder), + } +} diff --git a/agent/agentscripts/agentscripts.go b/agent/agentscripts/agentscripts.go index bd83d71875c73..4e4921b87ee5b 100644 --- a/agent/agentscripts/agentscripts.go +++ b/agent/agentscripts/agentscripts.go @@ -80,6 +80,21 @@ func New(opts Options) *Runner { type ScriptCompletedFunc func(context.Context, *proto.WorkspaceAgentScriptCompletedRequest) (*proto.WorkspaceAgentScriptCompletedResponse, error) +type runnerScript struct { + runOnPostStart bool + codersdk.WorkspaceAgentScript +} + +func toRunnerScript(scripts ...codersdk.WorkspaceAgentScript) []runnerScript { + var rs []runnerScript + for _, s := range scripts { + rs = append(rs, runnerScript{ + WorkspaceAgentScript: s, + }) + } + return rs +} + type Runner struct { Options @@ -90,7 +105,7 @@ type Runner struct { closeMutex sync.Mutex cron *cron.Cron initialized atomic.Bool - scripts []codersdk.WorkspaceAgentScript + scripts []runnerScript dataDir string scriptCompleted ScriptCompletedFunc @@ -119,16 +134,35 @@ func (r *Runner) RegisterMetrics(reg prometheus.Registerer) { reg.MustRegister(r.scriptsExecuted) } +// InitOption describes an option for the runner initialization. +type InitOption func(*Runner) + +// WithPostStartScripts adds scripts that should be run after the workspace +// start scripts but before the workspace is marked as started. +func WithPostStartScripts(scripts ...codersdk.WorkspaceAgentScript) InitOption { + return func(r *Runner) { + for _, s := range scripts { + r.scripts = append(r.scripts, runnerScript{ + runOnPostStart: true, + WorkspaceAgentScript: s, + }) + } + } +} + // Init initializes the runner with the provided scripts. // It also schedules any scripts that have a schedule. // This function must be called before Execute. -func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted ScriptCompletedFunc) error { +func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted ScriptCompletedFunc, opts ...InitOption) error { if r.initialized.Load() { return xerrors.New("init: already initialized") } r.initialized.Store(true) - r.scripts = scripts + r.scripts = toRunnerScript(scripts...) r.scriptCompleted = scriptCompleted + for _, opt := range opts { + opt(r) + } r.Logger.Info(r.cronCtx, "initializing agent scripts", slog.F("script_count", len(scripts)), slog.F("log_dir", r.LogDir)) err := r.Filesystem.MkdirAll(r.ScriptBinDir(), 0o700) @@ -136,13 +170,13 @@ func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted S return xerrors.Errorf("create script bin dir: %w", err) } - for _, script := range scripts { + for _, script := range r.scripts { if script.Cron == "" { continue } script := script _, err := r.cron.AddFunc(script.Cron, func() { - err := r.trackRun(r.cronCtx, script, ExecuteCronScripts) + err := r.trackRun(r.cronCtx, script.WorkspaceAgentScript, ExecuteCronScripts) if err != nil { r.Logger.Warn(context.Background(), "run agent script on schedule", slog.Error(err)) } @@ -186,6 +220,7 @@ type ExecuteOption int const ( ExecuteAllScripts ExecuteOption = iota ExecuteStartScripts + ExecutePostStartScripts ExecuteStopScripts ExecuteCronScripts ) @@ -196,6 +231,7 @@ func (r *Runner) Execute(ctx context.Context, option ExecuteOption) error { for _, script := range r.scripts { runScript := (option == ExecuteStartScripts && script.RunOnStart) || (option == ExecuteStopScripts && script.RunOnStop) || + (option == ExecutePostStartScripts && script.runOnPostStart) || (option == ExecuteCronScripts && script.Cron != "") || option == ExecuteAllScripts @@ -205,7 +241,7 @@ func (r *Runner) Execute(ctx context.Context, option ExecuteOption) error { script := script eg.Go(func() error { - err := r.trackRun(ctx, script, option) + err := r.trackRun(ctx, script.WorkspaceAgentScript, option) if err != nil { return xerrors.Errorf("run agent script %q: %w", script.LogSourceID, err) } From d7480cb69c5019982b547a25a3461c1dcddb5767 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 24 Mar 2025 16:39:12 +0000 Subject: [PATCH 02/15] docs --- agent/agent.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index bbf38f3a37aa9..b1374c7dfcf93 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1115,8 +1115,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } } + // Any defined Dev Containers may be autostarted after the start + // scripts have completed. var postStartScripts []codersdk.WorkspaceAgentScript for _, dc := range manifest.Devcontainers { + // TODO(mafredri): Verify `@devcontainers/cli` presence. + // TODO(mafredri): Verify workspace folder exists. + // TODO(mafredri): If set, verify config path exists. postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc)) } @@ -1126,11 +1131,18 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } err = a.trackGoroutine(func() { start := time.Now() - // here we use the graceful context because the script runner is not directly tied - // to the agent API. + // Here we use the graceful context because the script runner is + // not directly tied to the agent API. + // + // First we run the start scripts to ensure the workspace has + // been initialized and then the post start scripts which may + // depend on the workspace start scripts. + // + // Measure the time immediately after the start scripts have + // finished (both start and post start). For instance, an + // autostarted devcontainer will be included in this time. err := a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecuteStartScripts) err = errors.Join(err, a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecutePostStartScripts)) - // Measure the time immediately after the script has finished dur := time.Since(start).Seconds() if err != nil { a.logger.Warn(ctx, "startup script(s) failed", slog.Error(err)) From 9bfdfaf81dc46020fb152b13032d9fcb12b88000 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 25 Mar 2025 10:36:15 +0000 Subject: [PATCH 03/15] expand --- agent/agent.go | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index b1374c7dfcf93..77abbd870126d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1075,7 +1075,7 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // // An example is VS Code Remote, which must know the directory // before initializing a connection. - manifest.Directory, err = expandDirectory(manifest.Directory) + manifest.Directory, err = expandPathToAbs(manifest.Directory) if err != nil { return xerrors.Errorf("expand directory: %w", err) } @@ -1119,13 +1119,18 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // scripts have completed. var postStartScripts []codersdk.WorkspaceAgentScript for _, dc := range manifest.Devcontainers { + dc = expandDevcontainerPaths(a.logger, dc) // TODO(mafredri): Verify `@devcontainers/cli` presence. // TODO(mafredri): Verify workspace folder exists. // TODO(mafredri): If set, verify config path exists. postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc)) } - err = a.scriptRunner.Init(manifest.Scripts, aAPI.ScriptCompleted, agentscripts.WithPostStartScripts(postStartScripts...)) + err = a.scriptRunner.Init( + manifest.Scripts, + aAPI.ScriptCompleted, + agentscripts.WithPostStartScripts(postStartScripts...), + ) if err != nil { return xerrors.Errorf("init script runner: %w", err) } @@ -1864,30 +1869,42 @@ func userHomeDir() (string, error) { return u.HomeDir, nil } -// expandDirectory converts a directory path to an absolute path. -// It primarily resolves the home directory and any environment -// variables that may be set -func expandDirectory(dir string) (string, error) { - if dir == "" { +// expandPathToAbs converts a path to an absolute path. It primarily resolves +// the home directory and any environment variables that may be set. +func expandPathToAbs(path string) (string, error) { + if path == "" { return "", nil } - if dir[0] == '~' { + if path[0] == '~' { home, err := userHomeDir() if err != nil { return "", err } - dir = filepath.Join(home, dir[1:]) + path = filepath.Join(home, path[1:]) } - dir = os.ExpandEnv(dir) + path = os.ExpandEnv(path) - if !filepath.IsAbs(dir) { + if !filepath.IsAbs(path) { home, err := userHomeDir() if err != nil { return "", err } - dir = filepath.Join(home, dir) + path = filepath.Join(home, path) + } + return path, nil +} + +func expandDevcontainerPaths(logger slog.Logger, dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentDevcontainer { + var err error + if dc.WorkspaceFolder, err = expandPathToAbs(dc.WorkspaceFolder); err != nil { + logger.Warn(context.Background(), "expand devcontainer workspace folder failed", slog.Error(err)) + } + if dc.ConfigPath != "" { + if dc.ConfigPath, err = expandPathToAbs(dc.ConfigPath); err != nil { + logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) + } } - return dir, nil + return dc } // EnvAgentSubsystem is the environment variable used to denote the From 304b1a12a01a66514f8010396cfc6edf6d14d799 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 25 Mar 2025 11:23:39 +0000 Subject: [PATCH 04/15] devcontainer scripts and timings --- agent/agent.go | 27 +++++-- agent/agentcontainers/devcontainer.go | 19 ++--- .../provisionerdserver/provisionerdserver.go | 71 ++++++++++++------- 3 files changed, 71 insertions(+), 46 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 77abbd870126d..730fe4c385c8c 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1115,15 +1115,32 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } } - // Any defined Dev Containers may be autostarted after the start - // scripts have completed. - var postStartScripts []codersdk.WorkspaceAgentScript + var ( + // Clone the scripts so we can remove the devcontainer scripts. + scripts = slices.Clone(manifest.Scripts) + // The post-start scripts are used to autostart Dev Containers + // after the start scripts have completed. This is necessary + // because the Dev Container may depend on the workspace being + // initialized (git clone, etc). + postStartScripts []codersdk.WorkspaceAgentScript + ) for _, dc := range manifest.Devcontainers { - dc = expandDevcontainerPaths(a.logger, dc) // TODO(mafredri): Verify `@devcontainers/cli` presence. // TODO(mafredri): Verify workspace folder exists. // TODO(mafredri): If set, verify config path exists. - postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc)) + dc = expandDevcontainerPaths(a.logger, dc) + + for i, s := range scripts { + // The devcontainer scripts match the devcontainer ID for + // identification. + if s.ID == dc.ID { + scripts = slices.Delete(scripts, i, i+1) + if a.experimentalDevcontainersEnabled { + postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc, s)) + } + break + } + } } err = a.scriptRunner.Init( diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 4b84de9ab55e7..328037cf637ab 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -3,23 +3,14 @@ package agentcontainers import ( "fmt" - "github.com/google/uuid" - "github.com/coder/coder/v2/codersdk" ) -func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentScript { - script := fmt.Sprintf("devcontainer up --workspace-folder %q", dc.WorkspaceFolder) +func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript { + cmd := fmt.Sprintf("devcontainer up --workspace-folder %q", dc.WorkspaceFolder) if dc.ConfigPath != "" { - script = fmt.Sprintf("%s --config %q", script, dc.ConfigPath) - } - return codersdk.WorkspaceAgentScript{ - ID: uuid.New(), - LogSourceID: uuid.Nil, // TODO(mafredri): Add a devcontainer log source? - LogPath: "", - Script: script, - Cron: "", - Timeout: 0, - DisplayName: fmt.Sprintf("Dev Container (%s)", dc.WorkspaceFolder), + cmd = fmt.Sprintf("%s --config %q", cmd, dc.ConfigPath) } + script.Script = cmd + return script } diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 05cadb5875e5a..0838ce4c2b6d8 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -2051,6 +2051,34 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. } } + var ( + devcontainers = prAgent.GetDevcontainers() + devcontainerIDs = make([]uuid.UUID, 0, len(devcontainers)) + devcontainerNames = make([]string, 0, len(devcontainers)) + devcontainerWorkspaceFolders = make([]string, 0, len(devcontainers)) + devcontainerConfigPaths = make([]string, 0, len(devcontainers)) + ) + if len(devcontainers) > 0 { + for _, dc := range devcontainers { + devcontainerIDs = append(devcontainerIDs, uuid.New()) + devcontainerNames = append(devcontainerNames, dc.Name) + devcontainerWorkspaceFolders = append(devcontainerWorkspaceFolders, dc.WorkspaceFolder) + devcontainerConfigPaths = append(devcontainerConfigPaths, dc.ConfigPath) + } + + _, err = db.InsertWorkspaceAgentDevcontainers(ctx, database.InsertWorkspaceAgentDevcontainersParams{ + WorkspaceAgentID: agentID, + CreatedAt: dbtime.Now(), + ID: devcontainerIDs, + Name: devcontainerNames, + WorkspaceFolder: devcontainerWorkspaceFolders, + ConfigPath: devcontainerConfigPaths, + }) + if err != nil { + return xerrors.Errorf("insert agent devcontainer: %w", err) + } + } + logSourceIDs := make([]uuid.UUID, 0, len(prAgent.Scripts)) logSourceDisplayNames := make([]string, 0, len(prAgent.Scripts)) logSourceIcons := make([]string, 0, len(prAgent.Scripts)) @@ -2078,6 +2106,22 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. scriptRunOnStart = append(scriptRunOnStart, script.RunOnStart) scriptRunOnStop = append(scriptRunOnStop, script.RunOnStop) } + // Add a log source and script for each devcontainer so we can + // track logs and timings for each. + for _, id := range devcontainerIDs { + logSourceIDs = append(logSourceIDs, uuid.New()) + logSourceDisplayNames = append(logSourceDisplayNames, "Dev Container") + logSourceIcons = append(logSourceIcons, "/emojis/1f4e6.png") // Emoji package. Or perhaps /icon/container.svg? + scriptIDs = append(scriptIDs, id) // Re-use the devcontainer ID as the script ID for identification. + scriptDisplayName = append(scriptDisplayName, "Dev Container") // TODO(mafredri): Make it unique? Grab from id used in TF? + scriptLogPaths = append(scriptLogPaths, "") + scriptSources = append(scriptSources, "") + scriptCron = append(scriptCron, "") + scriptTimeout = append(scriptTimeout, 0) + scriptStartBlocksLogin = append(scriptStartBlocksLogin, false) + scriptRunOnStart = append(scriptRunOnStart, false) + scriptRunOnStop = append(scriptRunOnStop, false) + } _, err = db.InsertWorkspaceAgentLogSources(ctx, database.InsertWorkspaceAgentLogSourcesParams{ WorkspaceAgentID: agentID, @@ -2108,33 +2152,6 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. return xerrors.Errorf("insert agent scripts: %w", err) } - if devcontainers := prAgent.GetDevcontainers(); len(devcontainers) > 0 { - var ( - devcontainerIDs = make([]uuid.UUID, 0, len(devcontainers)) - devcontainerNames = make([]string, 0, len(devcontainers)) - devcontainerWorkspaceFolders = make([]string, 0, len(devcontainers)) - devcontainerConfigPaths = make([]string, 0, len(devcontainers)) - ) - for _, dc := range devcontainers { - devcontainerIDs = append(devcontainerIDs, uuid.New()) - devcontainerNames = append(devcontainerNames, dc.Name) - devcontainerWorkspaceFolders = append(devcontainerWorkspaceFolders, dc.WorkspaceFolder) - devcontainerConfigPaths = append(devcontainerConfigPaths, dc.ConfigPath) - } - - _, err = db.InsertWorkspaceAgentDevcontainers(ctx, database.InsertWorkspaceAgentDevcontainersParams{ - WorkspaceAgentID: agentID, - CreatedAt: dbtime.Now(), - ID: devcontainerIDs, - Name: devcontainerNames, - WorkspaceFolder: devcontainerWorkspaceFolders, - ConfigPath: devcontainerConfigPaths, - }) - if err != nil { - return xerrors.Errorf("insert agent devcontainer: %w", err) - } - } - for _, app := range prAgent.Apps { // Similar logic is duplicated in terraform/resources.go. slug := app.Slug From e1048b1875cf0beeafad1fefdf2ef9efeb514d84 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 25 Mar 2025 12:23:41 +0000 Subject: [PATCH 05/15] use name, print warning --- agent/agent.go | 26 +++--- agent/agentcontainers/devcontainer.go | 1 + .../provisionerdserver/provisionerdserver.go | 91 ++++++++++--------- 3 files changed, 62 insertions(+), 56 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 730fe4c385c8c..f366fea999bc6 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1124,21 +1124,21 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // initialized (git clone, etc). postStartScripts []codersdk.WorkspaceAgentScript ) - for _, dc := range manifest.Devcontainers { - // TODO(mafredri): Verify `@devcontainers/cli` presence. - // TODO(mafredri): Verify workspace folder exists. - // TODO(mafredri): If set, verify config path exists. - dc = expandDevcontainerPaths(a.logger, dc) - - for i, s := range scripts { - // The devcontainer scripts match the devcontainer ID for - // identification. - if s.ID == dc.ID { - scripts = slices.Delete(scripts, i, i+1) - if a.experimentalDevcontainersEnabled { + if a.experimentalDevcontainersEnabled { + for _, dc := range manifest.Devcontainers { + // TODO(mafredri): Verify `@devcontainers/cli` presence. + // TODO(mafredri): Verify workspace folder exists. + // TODO(mafredri): If set, verify config path exists. + dc = expandDevcontainerPaths(a.logger, dc) + + for i, s := range scripts { + // The devcontainer scripts match the devcontainer ID for + // identification. + if s.ID == dc.ID { + scripts = slices.Delete(scripts, i, i+1) postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc, s)) + break } - break } } } diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 328037cf637ab..050531e84d6dd 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -11,6 +11,7 @@ func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script co if dc.ConfigPath != "" { cmd = fmt.Sprintf("%s --config %q", cmd, dc.ConfigPath) } + script.RunOnStart = false script.Script = cmd return script } diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 0838ce4c2b6d8..d5a999dd806b6 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -2051,34 +2051,6 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. } } - var ( - devcontainers = prAgent.GetDevcontainers() - devcontainerIDs = make([]uuid.UUID, 0, len(devcontainers)) - devcontainerNames = make([]string, 0, len(devcontainers)) - devcontainerWorkspaceFolders = make([]string, 0, len(devcontainers)) - devcontainerConfigPaths = make([]string, 0, len(devcontainers)) - ) - if len(devcontainers) > 0 { - for _, dc := range devcontainers { - devcontainerIDs = append(devcontainerIDs, uuid.New()) - devcontainerNames = append(devcontainerNames, dc.Name) - devcontainerWorkspaceFolders = append(devcontainerWorkspaceFolders, dc.WorkspaceFolder) - devcontainerConfigPaths = append(devcontainerConfigPaths, dc.ConfigPath) - } - - _, err = db.InsertWorkspaceAgentDevcontainers(ctx, database.InsertWorkspaceAgentDevcontainersParams{ - WorkspaceAgentID: agentID, - CreatedAt: dbtime.Now(), - ID: devcontainerIDs, - Name: devcontainerNames, - WorkspaceFolder: devcontainerWorkspaceFolders, - ConfigPath: devcontainerConfigPaths, - }) - if err != nil { - return xerrors.Errorf("insert agent devcontainer: %w", err) - } - } - logSourceIDs := make([]uuid.UUID, 0, len(prAgent.Scripts)) logSourceDisplayNames := make([]string, 0, len(prAgent.Scripts)) logSourceIcons := make([]string, 0, len(prAgent.Scripts)) @@ -2106,21 +2078,54 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. scriptRunOnStart = append(scriptRunOnStart, script.RunOnStart) scriptRunOnStop = append(scriptRunOnStop, script.RunOnStop) } - // Add a log source and script for each devcontainer so we can - // track logs and timings for each. - for _, id := range devcontainerIDs { - logSourceIDs = append(logSourceIDs, uuid.New()) - logSourceDisplayNames = append(logSourceDisplayNames, "Dev Container") - logSourceIcons = append(logSourceIcons, "/emojis/1f4e6.png") // Emoji package. Or perhaps /icon/container.svg? - scriptIDs = append(scriptIDs, id) // Re-use the devcontainer ID as the script ID for identification. - scriptDisplayName = append(scriptDisplayName, "Dev Container") // TODO(mafredri): Make it unique? Grab from id used in TF? - scriptLogPaths = append(scriptLogPaths, "") - scriptSources = append(scriptSources, "") - scriptCron = append(scriptCron, "") - scriptTimeout = append(scriptTimeout, 0) - scriptStartBlocksLogin = append(scriptStartBlocksLogin, false) - scriptRunOnStart = append(scriptRunOnStart, false) - scriptRunOnStop = append(scriptRunOnStop, false) + + // Dev Containers require a script and log/source, so we do this before + // the logs insert below. + if devcontainers := prAgent.GetDevcontainers(); len(devcontainers) > 0 { + var ( + devcontainerIDs = make([]uuid.UUID, 0, len(devcontainers)) + devcontainerNames = make([]string, 0, len(devcontainers)) + devcontainerWorkspaceFolders = make([]string, 0, len(devcontainers)) + devcontainerConfigPaths = make([]string, 0, len(devcontainers)) + ) + for _, dc := range devcontainers { + id := uuid.New() + devcontainerIDs = append(devcontainerIDs, id) + devcontainerNames = append(devcontainerNames, dc.Name) + devcontainerWorkspaceFolders = append(devcontainerWorkspaceFolders, dc.WorkspaceFolder) + devcontainerConfigPaths = append(devcontainerConfigPaths, dc.ConfigPath) + + // Add a log source and script for each devcontainer so we can + // track logs and timings for each devcontainer. + displayName := fmt.Sprintf("Dev Container (%s)", dc.Name) + logSourceIDs = append(logSourceIDs, uuid.New()) + logSourceDisplayNames = append(logSourceDisplayNames, displayName) + logSourceIcons = append(logSourceIcons, "/emojis/1f4e6.png") // Emoji package. Or perhaps /icon/container.svg? + scriptIDs = append(scriptIDs, id) // Re-use the devcontainer ID as the script ID for identification. + scriptDisplayName = append(scriptDisplayName, displayName) + scriptLogPaths = append(scriptLogPaths, "") + scriptSources = append(scriptSources, `echo "WARNING: Dev Containers are early access. If you're seeing this message then Dev Containers haven't been enabled for your workspace yet. To enable, the agent needs to run with the environment variable CODER_AGENT_DEVCONTAINERS_ENABLE=true set."`) + scriptCron = append(scriptCron, "") + scriptTimeout = append(scriptTimeout, 0) + scriptStartBlocksLogin = append(scriptStartBlocksLogin, false) + // Run on start to surface the warning message in case the + // terraform resource is used, but the experiment hasn't + // been enabled. + scriptRunOnStart = append(scriptRunOnStart, true) + scriptRunOnStop = append(scriptRunOnStop, false) + } + + _, err = db.InsertWorkspaceAgentDevcontainers(ctx, database.InsertWorkspaceAgentDevcontainersParams{ + WorkspaceAgentID: agentID, + CreatedAt: dbtime.Now(), + ID: devcontainerIDs, + Name: devcontainerNames, + WorkspaceFolder: devcontainerWorkspaceFolders, + ConfigPath: devcontainerConfigPaths, + }) + if err != nil { + return xerrors.Errorf("insert agent devcontainer: %w", err) + } } _, err = db.InsertWorkspaceAgentLogSources(ctx, database.InsertWorkspaceAgentLogSourcesParams{ From d85e9a2e47211b6fcd93bff9be587f16892ecbaa Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 25 Mar 2025 13:30:42 +0000 Subject: [PATCH 06/15] refactor --- agent/agent.go | 48 +++++---------------------- agent/agentcontainers/devcontainer.go | 38 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 40 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index f366fea999bc6..e1f9a6b50052e 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1116,38 +1116,19 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } var ( - // Clone the scripts so we can remove the devcontainer scripts. - scripts = slices.Clone(manifest.Scripts) + scripts = manifest.Scripts + scriptRunnerOpts []agentscripts.InitOption + ) + if a.experimentalDevcontainersEnabled { + var dcScripts []codersdk.WorkspaceAgentScript + scripts, dcScripts = agentcontainers.ExtractDevcontainerScripts(a.logger, expandPathToAbs, manifest.Devcontainers, scripts) // The post-start scripts are used to autostart Dev Containers // after the start scripts have completed. This is necessary // because the Dev Container may depend on the workspace being // initialized (git clone, etc). - postStartScripts []codersdk.WorkspaceAgentScript - ) - if a.experimentalDevcontainersEnabled { - for _, dc := range manifest.Devcontainers { - // TODO(mafredri): Verify `@devcontainers/cli` presence. - // TODO(mafredri): Verify workspace folder exists. - // TODO(mafredri): If set, verify config path exists. - dc = expandDevcontainerPaths(a.logger, dc) - - for i, s := range scripts { - // The devcontainer scripts match the devcontainer ID for - // identification. - if s.ID == dc.ID { - scripts = slices.Delete(scripts, i, i+1) - postStartScripts = append(postStartScripts, agentcontainers.DevcontainerStartupScript(dc, s)) - break - } - } - } + scriptRunnerOpts = append(scriptRunnerOpts, agentscripts.WithPostStartScripts(dcScripts...)) } - - err = a.scriptRunner.Init( - manifest.Scripts, - aAPI.ScriptCompleted, - agentscripts.WithPostStartScripts(postStartScripts...), - ) + err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...) if err != nil { return xerrors.Errorf("init script runner: %w", err) } @@ -1911,19 +1892,6 @@ func expandPathToAbs(path string) (string, error) { return path, nil } -func expandDevcontainerPaths(logger slog.Logger, dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentDevcontainer { - var err error - if dc.WorkspaceFolder, err = expandPathToAbs(dc.WorkspaceFolder); err != nil { - logger.Warn(context.Background(), "expand devcontainer workspace folder failed", slog.Error(err)) - } - if dc.ConfigPath != "" { - if dc.ConfigPath, err = expandPathToAbs(dc.ConfigPath); err != nil { - logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) - } - } - return dc -} - // EnvAgentSubsystem is the environment variable used to denote the // specialized environment in which the agent is running // (e.g. envbox, envbuilder). diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 050531e84d6dd..ee143ef84cb3a 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -1,8 +1,11 @@ package agentcontainers import ( + "context" "fmt" + "cdr.dev/slog" + "github.com/coder/coder/v2/codersdk" ) @@ -15,3 +18,38 @@ func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script co script.Script = cmd return script } + +func ExtractDevcontainerScripts( + logger slog.Logger, + expandPath func(string) (string, error), + devcontainers []codersdk.WorkspaceAgentDevcontainer, + scripts []codersdk.WorkspaceAgentScript, +) (other []codersdk.WorkspaceAgentScript, devcontainerScripts []codersdk.WorkspaceAgentScript) { + for _, dc := range devcontainers { + dc = expandDevcontainerPaths(logger, expandPath, dc) + for _, script := range scripts { + // The devcontainer scripts match the devcontainer ID for + // identification. + if script.ID == dc.ID { + devcontainerScripts = append(devcontainerScripts, DevcontainerStartupScript(dc, script)) + } else { + other = append(other, script) + } + } + } + + return other, devcontainerScripts +} + +func expandDevcontainerPaths(logger slog.Logger, expandPath func(string) (string, error), dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentDevcontainer { + var err error + if dc.WorkspaceFolder, err = expandPath(dc.WorkspaceFolder); err != nil { + logger.Warn(context.Background(), "expand devcontainer workspace folder failed", slog.Error(err)) + } + if dc.ConfigPath != "" { + if dc.ConfigPath, err = expandPath(dc.ConfigPath); err != nil { + logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) + } + } + return dc +} From 7954f8d0bef74704935a5045a3c42dd2ad8e208d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 25 Mar 2025 13:42:12 +0000 Subject: [PATCH 07/15] tmpl --- agent/agentcontainers/devcontainer.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index ee143ef84cb3a..6b0439676fdf7 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -3,17 +3,29 @@ package agentcontainers import ( "context" "fmt" + "strings" "cdr.dev/slog" "github.com/coder/coder/v2/codersdk" ) +const devcontainerUpScriptTemplate = ` +if ! which devcontainer > /dev/null 2>&1; then + echo "ERROR: Unable to start devcontainer, @devcontainers/cli is not installed." + exit 1 +fi +devcontainer up %s +` + +// DevcontainerStartupScript returns a script that starts a devcontainer. func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript { - cmd := fmt.Sprintf("devcontainer up --workspace-folder %q", dc.WorkspaceFolder) + var args []string + args = append(args, fmt.Sprintf("--workspace-folder %q", dc.WorkspaceFolder)) if dc.ConfigPath != "" { - cmd = fmt.Sprintf("%s --config %q", cmd, dc.ConfigPath) + args = append(args, fmt.Sprintf("--config %q", dc.ConfigPath)) } + cmd := fmt.Sprintf(devcontainerUpScriptTemplate, strings.Join(args, " ")) script.RunOnStart = false script.Script = cmd return script From 234fc252dca4e04399956ab8f27f4e6d07c602aa Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 26 Mar 2025 11:10:20 +0000 Subject: [PATCH 08/15] test --- agent/agent.go | 8 +- agent/agentcontainers/devcontainer.go | 54 +++-- agent/agentcontainers/devcontainer_test.go | 226 +++++++++++++++++++++ 3 files changed, 261 insertions(+), 27 deletions(-) create mode 100644 agent/agentcontainers/devcontainer_test.go diff --git a/agent/agent.go b/agent/agent.go index e1f9a6b50052e..678b4101c5f88 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1121,11 +1121,9 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, ) if a.experimentalDevcontainersEnabled { var dcScripts []codersdk.WorkspaceAgentScript - scripts, dcScripts = agentcontainers.ExtractDevcontainerScripts(a.logger, expandPathToAbs, manifest.Devcontainers, scripts) - // The post-start scripts are used to autostart Dev Containers - // after the start scripts have completed. This is necessary - // because the Dev Container may depend on the workspace being - // initialized (git clone, etc). + scripts, dcScripts = agentcontainers.ExtractAndInitializeDevcontainerScripts(a.logger, expandPathToAbs, manifest.Devcontainers, scripts) + // See ExtractAndInitializeDevcontainerScripts for motivation + // behind running dcScripts as post start scripts. scriptRunnerOpts = append(scriptRunnerOpts, agentscripts.WithPostStartScripts(dcScripts...)) } err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...) diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 6b0439676fdf7..a6362e2a091c6 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -18,39 +18,49 @@ fi devcontainer up %s ` -// DevcontainerStartupScript returns a script that starts a devcontainer. -func DevcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript { - var args []string - args = append(args, fmt.Sprintf("--workspace-folder %q", dc.WorkspaceFolder)) - if dc.ConfigPath != "" { - args = append(args, fmt.Sprintf("--config %q", dc.ConfigPath)) - } - cmd := fmt.Sprintf(devcontainerUpScriptTemplate, strings.Join(args, " ")) - script.RunOnStart = false - script.Script = cmd - return script -} - -func ExtractDevcontainerScripts( +// ExtractAndInitializeDevcontainerScripts extracts devcontainer scripts from +// the given scripts and devcontainers. The devcontainer scripts are removed +// from the returned scripts so that they can be run separately. +// +// Dev Containers have an inherent dependency on start scripts, since they +// initialize the workspace (e.g. git clone, npm install, etc). This is +// important if e.g. a Coder module to install @devcontainer/cli is used. +func ExtractAndInitializeDevcontainerScripts( logger slog.Logger, expandPath func(string) (string, error), devcontainers []codersdk.WorkspaceAgentDevcontainer, scripts []codersdk.WorkspaceAgentScript, -) (other []codersdk.WorkspaceAgentScript, devcontainerScripts []codersdk.WorkspaceAgentScript) { - for _, dc := range devcontainers { - dc = expandDevcontainerPaths(logger, expandPath, dc) - for _, script := range scripts { +) (filteredScripts []codersdk.WorkspaceAgentScript, devcontainerScripts []codersdk.WorkspaceAgentScript) { +ScriptLoop: + for _, script := range scripts { + for _, dc := range devcontainers { // The devcontainer scripts match the devcontainer ID for // identification. if script.ID == dc.ID { - devcontainerScripts = append(devcontainerScripts, DevcontainerStartupScript(dc, script)) - } else { - other = append(other, script) + dc = expandDevcontainerPaths(logger, expandPath, dc) + devcontainerScripts = append(devcontainerScripts, devcontainerStartupScript(dc, script)) + continue ScriptLoop } } + + filteredScripts = append(filteredScripts, script) } - return other, devcontainerScripts + return filteredScripts, devcontainerScripts +} + +func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript { + var args []string + args = append(args, fmt.Sprintf("--workspace-folder %q", dc.WorkspaceFolder)) + if dc.ConfigPath != "" { + args = append(args, fmt.Sprintf("--config %q", dc.ConfigPath)) + } + cmd := fmt.Sprintf(devcontainerUpScriptTemplate, strings.Join(args, " ")) + script.Script = cmd + // Disable RunOnStart, scripts have this set so that when devcontainers + // have not been enabled, a warning will be surfaced in the agent logs. + script.RunOnStart = false + return script } func expandDevcontainerPaths(logger slog.Logger, expandPath func(string) (string, error), dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentDevcontainer { diff --git a/agent/agentcontainers/devcontainer_test.go b/agent/agentcontainers/devcontainer_test.go new file mode 100644 index 0000000000000..875b8a8a14fec --- /dev/null +++ b/agent/agentcontainers/devcontainer_test.go @@ -0,0 +1,226 @@ +package agentcontainers_test + +import ( + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/agent/agentcontainers" + "github.com/coder/coder/v2/codersdk" +) + +func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { + t.Parallel() + + scriptIDs := []uuid.UUID{uuid.New(), uuid.New()} + devcontainerIDs := []uuid.UUID{uuid.New(), uuid.New()} + + type args struct { + expandPath func(string) (string, error) + devcontainers []codersdk.WorkspaceAgentDevcontainer + scripts []codersdk.WorkspaceAgentScript + } + tests := []struct { + name string + args args + wantFilteredScripts []codersdk.WorkspaceAgentScript + wantDevcontainerScripts []codersdk.WorkspaceAgentScript + }{ + { + name: "no scripts", + args: args{ + expandPath: nil, + devcontainers: nil, + scripts: nil, + }, + wantFilteredScripts: nil, + wantDevcontainerScripts: nil, + }, + { + name: "no devcontainers", + args: args{ + expandPath: nil, + devcontainers: nil, + scripts: []codersdk.WorkspaceAgentScript{ + {ID: scriptIDs[0]}, + {ID: scriptIDs[1]}, + }, + }, + wantFilteredScripts: []codersdk.WorkspaceAgentScript{ + {ID: scriptIDs[0]}, + {ID: scriptIDs[1]}, + }, + wantDevcontainerScripts: nil, + }, + { + name: "no scripts match devcontainers", + args: args{ + expandPath: nil, + devcontainers: []codersdk.WorkspaceAgentDevcontainer{ + {ID: devcontainerIDs[0]}, + {ID: devcontainerIDs[1]}, + }, + scripts: []codersdk.WorkspaceAgentScript{ + {ID: scriptIDs[0]}, + {ID: scriptIDs[1]}, + }, + }, + wantFilteredScripts: []codersdk.WorkspaceAgentScript{ + {ID: scriptIDs[0]}, + {ID: scriptIDs[1]}, + }, + wantDevcontainerScripts: nil, + }, + { + name: "scripts match devcontainers and sets RunOnStart=false", + args: args{ + expandPath: nil, + devcontainers: []codersdk.WorkspaceAgentDevcontainer{ + {ID: devcontainerIDs[0], WorkspaceFolder: "workspace1"}, + {ID: devcontainerIDs[1], WorkspaceFolder: "workspace2"}, + }, + scripts: []codersdk.WorkspaceAgentScript{ + {ID: scriptIDs[0], RunOnStart: true}, + {ID: scriptIDs[1], RunOnStart: true}, + {ID: devcontainerIDs[0], RunOnStart: true}, + {ID: devcontainerIDs[1], RunOnStart: true}, + }, + }, + wantFilteredScripts: []codersdk.WorkspaceAgentScript{ + {ID: scriptIDs[0], RunOnStart: true}, + {ID: scriptIDs[1], RunOnStart: true}, + }, + wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ + { + ID: devcontainerIDs[0], + Script: "devcontainer up --workspace-folder \"workspace1\"", + RunOnStart: false, + }, + { + ID: devcontainerIDs[1], + Script: "devcontainer up --workspace-folder \"workspace2\"", + RunOnStart: false, + }, + }, + }, + { + name: "scripts match devcontainers with config path", + args: args{ + expandPath: nil, + devcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerIDs[0], + WorkspaceFolder: "workspace1", + ConfigPath: "config1", + }, + { + ID: devcontainerIDs[1], + WorkspaceFolder: "workspace2", + ConfigPath: "config2", + }, + }, + scripts: []codersdk.WorkspaceAgentScript{ + {ID: devcontainerIDs[0]}, + {ID: devcontainerIDs[1]}, + }, + }, + wantFilteredScripts: []codersdk.WorkspaceAgentScript{}, + wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ + { + ID: devcontainerIDs[0], + Script: "devcontainer up --workspace-folder \"workspace1\" --config \"config1\"", + RunOnStart: false, + }, + { + ID: devcontainerIDs[1], + Script: "devcontainer up --workspace-folder \"workspace2\" --config \"config2\"", + RunOnStart: false, + }, + }, + }, + { + name: "scripts match devcontainers with expand path", + args: args{ + expandPath: func(s string) (string, error) { + return "expanded/" + s, nil + }, + devcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerIDs[0], + WorkspaceFolder: "workspace1", + ConfigPath: "config1", + }, + { + ID: devcontainerIDs[1], + WorkspaceFolder: "workspace2", + ConfigPath: "config2", + }, + }, + scripts: []codersdk.WorkspaceAgentScript{ + {ID: devcontainerIDs[0], RunOnStart: true}, + {ID: devcontainerIDs[1], RunOnStart: true}, + }, + }, + wantFilteredScripts: []codersdk.WorkspaceAgentScript{}, + wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ + { + ID: devcontainerIDs[0], + Script: "devcontainer up --workspace-folder \"expanded/workspace1\" --config \"expanded/config1\"", + RunOnStart: false, + }, + { + ID: devcontainerIDs[1], + Script: "devcontainer up --workspace-folder \"expanded/workspace2\" --config \"expanded/config2\"", + RunOnStart: false, + }, + }, + }, + } + // nolint:foo + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + logger := slogtest.Make(t, nil) + if tt.args.expandPath == nil { + tt.args.expandPath = func(s string) (string, error) { + return s, nil + } + } + gotFilteredScripts, gotDevcontainerScripts := agentcontainers.ExtractAndInitializeDevcontainerScripts( + logger, + tt.args.expandPath, + tt.args.devcontainers, + tt.args.scripts, + ) + + if diff := cmp.Diff(tt.wantFilteredScripts, gotFilteredScripts, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ExtractAndInitializeDevcontainerScripts() gotFilteredScripts mismatch (-want +got):\n%s", diff) + } + + // Preprocess the devcontainer scripts to remove scripting part. + for i := range gotDevcontainerScripts { + gotDevcontainerScripts[i].Script = textGrep("devcontainer up", gotDevcontainerScripts[i].Script) + require.NotEmpty(t, gotDevcontainerScripts[i].Script, "devcontainer up script not found") + } + if diff := cmp.Diff(tt.wantDevcontainerScripts, gotDevcontainerScripts); diff != "" { + t.Errorf("ExtractAndInitializeDevcontainerScripts() gotDevcontainerScripts mismatch (-want +got):\n%s", diff) + } + }) + } +} + +// textGrep returns matching lines from multiline string. +func textGrep(want, got string) (filtered string) { + var lines []string + for _, line := range strings.Split(got, "\n") { + if strings.Contains(line, want) { + lines = append(lines, line) + } + } + return strings.Join(lines, "\n") +} From 2d93ee32c8d6ed97b7ca85bec345d99b6083f8ec Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 26 Mar 2025 12:49:11 +0000 Subject: [PATCH 09/15] add integration test --- agent/agent_test.go | 128 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/agent/agent_test.go b/agent/agent_test.go index 73b31dd6efe72..8ccf9b4cd7ebb 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1937,6 +1937,134 @@ func TestAgent_ReconnectingPTYContainer(t *testing.T) { require.ErrorIs(t, tr.ReadUntil(ctx, nil), io.EOF) } +// This tests end-to-end functionality of auto-starting a devcontainer. +// It runs "devcontainer up" which creates a real Docker container. As +// such, it does not run by default in CI. +// +// You can run it manually as follows: +// +// CODER_TEST_USE_DOCKER=1 go test -count=1 ./agent -run TestAgent_DevcontainerAutostart +func TestAgent_DevcontainerAutostart(t *testing.T) { + t.Parallel() + if os.Getenv("CODER_TEST_USE_DOCKER") != "1" { + t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test") + } + + ctx := testutil.Context(t, testutil.WaitLong) + + // Connect to Docker + pool, err := dockertest.NewPool("") + require.NoError(t, err, "Could not connect to docker") + + // Prepare temporary devcontainer for test (mywork). + devcontainerID := uuid.New() + tempWorkspaceFolder := t.TempDir() + tempWorkspaceFolder = filepath.Join(tempWorkspaceFolder, "mywork") + t.Logf("Workspace folder: %s", tempWorkspaceFolder) + devcontainerPath := filepath.Join(tempWorkspaceFolder, ".devcontainer") + err = os.MkdirAll(devcontainerPath, 0o755) + require.NoError(t, err, "create devcontainer directory") + devcontainerFile := filepath.Join(devcontainerPath, "devcontainer.json") + err = os.WriteFile(devcontainerFile, []byte(`{ + "name": "mywork", + "image": "busybox:latest", + "cmd": ["sleep", "infinity"] + }`), 0o600) + require.NoError(t, err, "write devcontainer.json") + + manifest := agentsdk.Manifest{ + // Set up pre-conditions for auto-starting a devcontainer, the script + // is expected to be prepared by the provisioner normally. + Devcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerID, + Name: "test", + WorkspaceFolder: tempWorkspaceFolder, + }, + }, + Scripts: []codersdk.WorkspaceAgentScript{ + { + ID: devcontainerID, + LogSourceID: agentsdk.ExternalLogSourceID, + RunOnStart: true, + Script: "echo this-will-be-replaced", + DisplayName: "Dev Container (test)", + }, + }, + } + // nolint: dogsled + conn, _, _, _, _ := setupAgent(t, manifest, 0, func(_ *agenttest.Client, o *agent.Options) { + o.ExperimentalDevcontainersEnabled = true + }) + + t.Logf("Waiting for container with label: devcontainer.local_folder=%s", tempWorkspaceFolder) + + var container docker.APIContainers + require.Eventually(t, func() bool { + containers, err := pool.Client.ListContainers(docker.ListContainersOptions{All: true}) + if err != nil { + t.Logf("Error listing containers: %v", err) + return false + } + + for _, c := range containers { + t.Logf("Found container: %s with labels: %v", c.ID[:12], c.Labels) + if labelValue, ok := c.Labels["devcontainer.local_folder"]; ok { + if labelValue == tempWorkspaceFolder { + t.Logf("Found matching container: %s", c.ID[:12]) + container = c + return true + } + } + } + + return false + }, testutil.WaitSuperLong, testutil.IntervalMedium, "no container with workspace folder label found") + + t.Cleanup(func() { + // We can't rely on pool here because the container is not + // managed by it (it is managed by @devcontainer/cli). + err := pool.Client.RemoveContainer(docker.RemoveContainerOptions{ + ID: container.ID, + RemoveVolumes: true, + Force: true, + }) + assert.NoError(t, err, "remove container") + }) + + containerInfo, err := pool.Client.InspectContainer(container.ID) + require.NoError(t, err, "inspect container") + t.Logf("Container state: status: %v", containerInfo.State.Status) + require.True(t, containerInfo.State.Running, "container should be running") + + ac, err := conn.ReconnectingPTY(ctx, uuid.New(), 80, 80, "", func(opts *workspacesdk.AgentReconnectingPTYInit) { + opts.Container = container.ID + }) + require.NoError(t, err, "failed to create ReconnectingPTY") + defer ac.Close() + + // Use terminal reader so we can see output in case somethin goes wrong. + tr := testutil.NewTerminalReader(t, ac) + + require.NoError(t, tr.ReadUntil(ctx, func(line string) bool { + return strings.Contains(line, "#") || strings.Contains(line, "$") + }), "find prompt") + + wantFileName := "file-from-devcontainer" + wantFile := filepath.Join(tempWorkspaceFolder, wantFileName) + + require.NoError(t, json.NewEncoder(ac).Encode(workspacesdk.ReconnectingPTYRequest{ + // NOTE(mafredri): We must use absolute path here for some reason. + Data: fmt.Sprintf("touch /workspaces/mywork/%s; exit\r", wantFileName), + }), "create file inside devcontainer") + + // Wait for the connection to close to ensure the touch was executed. + require.ErrorIs(t, tr.ReadUntil(ctx, nil), io.EOF) + + _, err = os.Stat(wantFile) + require.NoError(t, err, "file should exist outside devcontainer") +} + func TestAgent_Dial(t *testing.T) { t.Parallel() From 9aca1c83d05c28ccfe156f6ff89b539494281146 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 26 Mar 2025 12:56:52 +0000 Subject: [PATCH 10/15] unfoo --- agent/agentcontainers/devcontainer_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/agentcontainers/devcontainer_test.go b/agent/agentcontainers/devcontainer_test.go index 875b8a8a14fec..21c494a48b8b4 100644 --- a/agent/agentcontainers/devcontainer_test.go +++ b/agent/agentcontainers/devcontainer_test.go @@ -181,7 +181,6 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { }, }, } - // nolint:foo for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() From 9c1bf01874cdd911008275c79ab87e1ec30af626 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 26 Mar 2025 13:37:07 +0000 Subject: [PATCH 11/15] add agentscripts test for execute option --- agent/agentscripts/agentscripts_test.go | 153 +++++++++++++++++++++++- 1 file changed, 152 insertions(+), 1 deletion(-) diff --git a/agent/agentscripts/agentscripts_test.go b/agent/agentscripts/agentscripts_test.go index 0d6e41772cdb7..cf914daa3d09e 100644 --- a/agent/agentscripts/agentscripts_test.go +++ b/agent/agentscripts/agentscripts_test.go @@ -4,6 +4,8 @@ import ( "context" "path/filepath" "runtime" + "slices" + "sync" "testing" "time" @@ -151,11 +153,160 @@ func TestCronClose(t *testing.T) { require.NoError(t, runner.Close(), "close runner") } +func TestExecuteOptions(t *testing.T) { + t.Parallel() + + startScript := codersdk.WorkspaceAgentScript{ + ID: uuid.New(), + LogSourceID: uuid.New(), + Script: "echo start", + RunOnStart: true, + } + stopScript := codersdk.WorkspaceAgentScript{ + ID: uuid.New(), + LogSourceID: uuid.New(), + Script: "echo stop", + RunOnStop: true, + } + postStartScript := codersdk.WorkspaceAgentScript{ + ID: uuid.New(), + LogSourceID: uuid.New(), + Script: "echo poststart", + } + regularScript := codersdk.WorkspaceAgentScript{ + ID: uuid.New(), + LogSourceID: uuid.New(), + Script: "echo regular", + } + + scripts := []codersdk.WorkspaceAgentScript{ + startScript, + stopScript, + regularScript, + } + allScripts := append(slices.Clone(scripts), postStartScript) + + scriptByID := func(t *testing.T, id uuid.UUID) codersdk.WorkspaceAgentScript { + for _, script := range allScripts { + if script.ID == id { + return script + } + } + t.Fatal("script not found") + return codersdk.WorkspaceAgentScript{} + } + + wantOutput := map[uuid.UUID]string{ + startScript.ID: "start", + stopScript.ID: "stop", + postStartScript.ID: "poststart", + regularScript.ID: "regular", + } + + testCases := []struct { + name string + option agentscripts.ExecuteOption + wantRun []uuid.UUID + }{ + { + name: "ExecuteAllScripts", + option: agentscripts.ExecuteAllScripts, + wantRun: []uuid.UUID{startScript.ID, stopScript.ID, regularScript.ID, postStartScript.ID}, + }, + { + name: "ExecuteStartScripts", + option: agentscripts.ExecuteStartScripts, + wantRun: []uuid.UUID{startScript.ID}, + }, + { + name: "ExecutePostStartScripts", + option: agentscripts.ExecutePostStartScripts, + wantRun: []uuid.UUID{postStartScript.ID}, + }, + { + name: "ExecuteStopScripts", + option: agentscripts.ExecuteStopScripts, + wantRun: []uuid.UUID{stopScript.ID}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + executedScripts := make(map[uuid.UUID]bool) + fLogger := &executeOptionTestLogger{ + tb: t, + executedScripts: executedScripts, + wantOutput: wantOutput, + } + + runner := setup(t, func(uuid.UUID) agentscripts.ScriptLogger { + return fLogger + }) + defer runner.Close() + + aAPI := agenttest.NewFakeAgentAPI(t, testutil.Logger(t), nil, nil) + err := runner.Init( + scripts, + aAPI.ScriptCompleted, + agentscripts.WithPostStartScripts(postStartScript), + ) + require.NoError(t, err) + + err = runner.Execute(ctx, tc.option) + require.NoError(t, err) + + gotRun := map[uuid.UUID]bool{} + for _, id := range tc.wantRun { + gotRun[id] = true + require.True(t, executedScripts[id], + "script %s should have run when using filter %s", scriptByID(t, id).Script, tc.name) + } + + for _, script := range allScripts { + if _, ok := gotRun[script.ID]; ok { + continue + } + require.False(t, executedScripts[script.ID], + "script %s should not have run when using filter %s", script.Script, tc.name) + } + }) + } +} + +type executeOptionTestLogger struct { + tb testing.TB + executedScripts map[uuid.UUID]bool + wantOutput map[uuid.UUID]string + mu sync.Mutex +} + +func (l *executeOptionTestLogger) Send(_ context.Context, logs ...agentsdk.Log) error { + l.mu.Lock() + defer l.mu.Unlock() + for _, log := range logs { + l.tb.Log(log.Output) + for id, output := range l.wantOutput { + if log.Output == output { + l.executedScripts[id] = true + break + } + } + } + return nil +} + +func (*executeOptionTestLogger) Flush(context.Context) error { + return nil +} + func setup(t *testing.T, getScriptLogger func(logSourceID uuid.UUID) agentscripts.ScriptLogger) *agentscripts.Runner { t.Helper() if getScriptLogger == nil { // noop - getScriptLogger = func(uuid uuid.UUID) agentscripts.ScriptLogger { + getScriptLogger = func(uuid.UUID) agentscripts.ScriptLogger { return noopScriptLogger{} } } From ef089be64243a1504bd4536533144946a4f3ce7b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 26 Mar 2025 14:09:45 +0000 Subject: [PATCH 12/15] safe assign --- agent/agentcontainers/devcontainer.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index a6362e2a091c6..5c5b723821601 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -64,13 +64,16 @@ func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script co } func expandDevcontainerPaths(logger slog.Logger, expandPath func(string) (string, error), dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentDevcontainer { - var err error - if dc.WorkspaceFolder, err = expandPath(dc.WorkspaceFolder); err != nil { + if wf, err := expandPath(dc.WorkspaceFolder); err != nil { logger.Warn(context.Background(), "expand devcontainer workspace folder failed", slog.Error(err)) + } else { + dc.WorkspaceFolder = wf } if dc.ConfigPath != "" { - if dc.ConfigPath, err = expandPath(dc.ConfigPath); err != nil { + if cp, err := expandPath(dc.ConfigPath); err != nil { logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) + } else { + dc.ConfigPath = cp } } return dc From 55c5c46b2f91853cba41535076fec194de015716 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 26 Mar 2025 16:36:16 +0000 Subject: [PATCH 13/15] improve logger, fix config path realtive to workspace folder --- agent/agentcontainers/devcontainer.go | 24 ++++++++-- agent/agentcontainers/devcontainer_test.go | 52 +++++++++++++++++++--- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 5c5b723821601..202047fc35d6e 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -3,6 +3,8 @@ package agentcontainers import ( "context" "fmt" + "os" + "path/filepath" "strings" "cdr.dev/slog" @@ -64,17 +66,33 @@ func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script co } func expandDevcontainerPaths(logger slog.Logger, expandPath func(string) (string, error), dc codersdk.WorkspaceAgentDevcontainer) codersdk.WorkspaceAgentDevcontainer { + logger = logger.With(slog.F("devcontainer", dc.Name), slog.F("workspace_folder", dc.WorkspaceFolder), slog.F("config_path", dc.ConfigPath)) + if wf, err := expandPath(dc.WorkspaceFolder); err != nil { logger.Warn(context.Background(), "expand devcontainer workspace folder failed", slog.Error(err)) } else { dc.WorkspaceFolder = wf } if dc.ConfigPath != "" { - if cp, err := expandPath(dc.ConfigPath); err != nil { - logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) + // Let expandPath handle home directory, otherwise assume relative to + // workspace folder or absoulte. + if dc.ConfigPath[0] == '~' { + if cp, err := expandPath(dc.ConfigPath); err != nil { + logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) + } else { + dc.ConfigPath = cp + } } else { - dc.ConfigPath = cp + dc.ConfigPath = relativePathToAbs(dc.WorkspaceFolder, dc.ConfigPath) } } return dc } + +func relativePathToAbs(workdir, path string) string { + path = os.ExpandEnv(path) + if !filepath.IsAbs(path) { + path = filepath.Join(workdir, path) + } + return path +} diff --git a/agent/agentcontainers/devcontainer_test.go b/agent/agentcontainers/devcontainer_test.go index 21c494a48b8b4..82f2f39dc4142 100644 --- a/agent/agentcontainers/devcontainer_test.go +++ b/agent/agentcontainers/devcontainer_test.go @@ -1,6 +1,7 @@ package agentcontainers_test import ( + "path/filepath" "strings" "testing" @@ -133,12 +134,12 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ { ID: devcontainerIDs[0], - Script: "devcontainer up --workspace-folder \"workspace1\" --config \"config1\"", + Script: "devcontainer up --workspace-folder \"workspace1\" --config \"workspace1/config1\"", RunOnStart: false, }, { ID: devcontainerIDs[1], - Script: "devcontainer up --workspace-folder \"workspace2\" --config \"config2\"", + Script: "devcontainer up --workspace-folder \"workspace2\" --config \"workspace2/config2\"", RunOnStart: false, }, }, @@ -147,7 +148,7 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { name: "scripts match devcontainers with expand path", args: args{ expandPath: func(s string) (string, error) { - return "expanded/" + s, nil + return "/home/" + s, nil }, devcontainers: []codersdk.WorkspaceAgentDevcontainer{ { @@ -170,12 +171,53 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ { ID: devcontainerIDs[0], - Script: "devcontainer up --workspace-folder \"expanded/workspace1\" --config \"expanded/config1\"", + Script: "devcontainer up --workspace-folder \"/home/workspace1\" --config \"/home/workspace1/config1\"", RunOnStart: false, }, { ID: devcontainerIDs[1], - Script: "devcontainer up --workspace-folder \"expanded/workspace2\" --config \"expanded/config2\"", + Script: "devcontainer up --workspace-folder \"/home/workspace2\" --config \"/home/workspace2/config2\"", + RunOnStart: false, + }, + }, + }, + { + name: "expand config path when ~", + args: args{ + expandPath: func(s string) (string, error) { + s = strings.Replace(s, "~/", "", 1) + if filepath.IsAbs(s) { + return s, nil + } + return "/home/" + s, nil + }, + devcontainers: []codersdk.WorkspaceAgentDevcontainer{ + { + ID: devcontainerIDs[0], + WorkspaceFolder: "workspace1", + ConfigPath: "~/config1", + }, + { + ID: devcontainerIDs[1], + WorkspaceFolder: "workspace2", + ConfigPath: "/config2", + }, + }, + scripts: []codersdk.WorkspaceAgentScript{ + {ID: devcontainerIDs[0], RunOnStart: true}, + {ID: devcontainerIDs[1], RunOnStart: true}, + }, + }, + wantFilteredScripts: []codersdk.WorkspaceAgentScript{}, + wantDevcontainerScripts: []codersdk.WorkspaceAgentScript{ + { + ID: devcontainerIDs[0], + Script: "devcontainer up --workspace-folder \"/home/workspace1\" --config \"/home/config1\"", + RunOnStart: false, + }, + { + ID: devcontainerIDs[1], + Script: "devcontainer up --workspace-folder \"/home/workspace2\" --config \"/config2\"", RunOnStart: false, }, }, From 4676898fffc115e8b454376579a3394a7f5c49f5 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 27 Mar 2025 11:42:52 +0200 Subject: [PATCH 14/15] Update agent/agentcontainers/devcontainer.go Co-authored-by: Danielle Maywood --- agent/agentcontainers/devcontainer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/devcontainer.go b/agent/agentcontainers/devcontainer.go index 202047fc35d6e..59fa9a5e35e82 100644 --- a/agent/agentcontainers/devcontainer.go +++ b/agent/agentcontainers/devcontainer.go @@ -75,7 +75,7 @@ func expandDevcontainerPaths(logger slog.Logger, expandPath func(string) (string } if dc.ConfigPath != "" { // Let expandPath handle home directory, otherwise assume relative to - // workspace folder or absoulte. + // workspace folder or absolute. if dc.ConfigPath[0] == '~' { if cp, err := expandPath(dc.ConfigPath); err != nil { logger.Warn(context.Background(), "expand devcontainer config path failed", slog.Error(err)) From f797b411e6b7bc7a8ca077a6bfea287071a7cb8f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 27 Mar 2025 10:11:22 +0000 Subject: [PATCH 15/15] skip path separator tests on windows --- agent/agentcontainers/devcontainer_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/agent/agentcontainers/devcontainer_test.go b/agent/agentcontainers/devcontainer_test.go index 82f2f39dc4142..eb836af928a50 100644 --- a/agent/agentcontainers/devcontainer_test.go +++ b/agent/agentcontainers/devcontainer_test.go @@ -31,6 +31,8 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { args args wantFilteredScripts []codersdk.WorkspaceAgentScript wantDevcontainerScripts []codersdk.WorkspaceAgentScript + + skipOnWindowsDueToPathSeparator bool }{ { name: "no scripts", @@ -143,6 +145,7 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { RunOnStart: false, }, }, + skipOnWindowsDueToPathSeparator: true, }, { name: "scripts match devcontainers with expand path", @@ -180,6 +183,7 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { RunOnStart: false, }, }, + skipOnWindowsDueToPathSeparator: true, }, { name: "expand config path when ~", @@ -221,11 +225,16 @@ func TestExtractAndInitializeDevcontainerScripts(t *testing.T) { RunOnStart: false, }, }, + skipOnWindowsDueToPathSeparator: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() + if tt.skipOnWindowsDueToPathSeparator && filepath.Separator == '\\' { + t.Skip("Skipping test on Windows due to path separator difference.") + } + logger := slogtest.Make(t, nil) if tt.args.expandPath == nil { tt.args.expandPath = func(s string) (string, error) {