Skip to content

Commit 978c871

Browse files
DanielleMaywoodDanielleMaywood
committed
fix(agent): start devcontainers through agentcontainers package
Fixes coder/internal#706 Context for the implementation here coder/internal#706 (comment) Synchronously starts dev containers defined in terraform with our `DevcontainerCLI` abstraction, instead of piggybacking off of our `agentscripts` package. This gives us more control over logs, instead of being reliant on packages which may or may not exist in the user-provided image.
1 parent b49e62f commit 978c871

File tree

7 files changed

+64
-424
lines changed

7 files changed

+64
-424
lines changed

‎agent/agent.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,13 +1145,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
11451145
scripts = manifest.Scripts
11461146
scriptRunnerOpts []agentscripts.InitOption
11471147
)
1148-
if a.experimentalDevcontainersEnabled {
1149-
var dcScripts []codersdk.WorkspaceAgentScript
1150-
scripts, dcScripts = agentcontainers.ExtractAndInitializeDevcontainerScripts(manifest.Devcontainers, scripts)
1151-
// See ExtractAndInitializeDevcontainerScripts for motivation
1152-
// behind running dcScripts as post start scripts.
1153-
scriptRunnerOpts = append(scriptRunnerOpts, agentscripts.WithPostStartScripts(dcScripts...))
1154-
}
11551148
err = a.scriptRunner.Init(scripts, aAPI.ScriptCompleted, scriptRunnerOpts...)
11561149
if err != nil {
11571150
return xerrors.Errorf("init script runner: %w", err)
@@ -1169,7 +1162,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
11691162
// finished (both start and post start). For instance, an
11701163
// autostarted devcontainer will be included in this time.
11711164
err := a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecuteStartScripts)
1172-
err = errors.Join(err, a.scriptRunner.Execute(a.gracefulCtx, agentscripts.ExecutePostStartScripts))
1165+
1166+
if cAPI := a.containerAPI.Load(); cAPI != nil {
1167+
for _, dc := range manifest.Devcontainers {
1168+
err = errors.Join(err, cAPI.CreateDevcontainer(dc))
1169+
}
1170+
}
1171+
11731172
dur := time.Since(start).Seconds()
11741173
if err != nil {
11751174
a.logger.Warn(ctx, "startup script(s) failed", slog.Error(err))

‎agent/agentcontainers/api.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -794,14 +794,25 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques
794794
})
795795
}
796796

797+
func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer) error {
798+
api.mu.Lock()
799+
dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting
800+
dc.Container = nil
801+
api.knownDevcontainers[dc.WorkspaceFolder] = dc
802+
api.asyncWg.Add(1)
803+
api.mu.Unlock()
804+
805+
return api.recreateDevcontainer(dc, dc.ConfigPath)
806+
}
807+
797808
// recreateDevcontainer should run in its own goroutine and is responsible for
798809
// recreating a devcontainer based on the provided devcontainer configuration.
799810
// It updates the devcontainer status and logs the process. The configPath is
800811
// passed as a parameter for the odd chance that the container being recreated
801812
// has a different config file than the one stored in the devcontainer state.
802813
// The devcontainer state must be set to starting and the asyncWg must be
803814
// incremented before calling this function.
804-
func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) {
815+
func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) error {
805816
defer api.asyncWg.Done()
806817

807818
var (
@@ -857,7 +868,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con
857868
api.knownDevcontainers[dc.WorkspaceFolder] = dc
858869
api.recreateErrorTimes[dc.WorkspaceFolder] = api.clock.Now("agentcontainers", "recreate", "errorTimes")
859870
api.mu.Unlock()
860-
return
871+
return err
861872
}
862873

863874
logger.Info(ctx, "devcontainer recreated successfully")
@@ -884,7 +895,10 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con
884895
// devcontainer state after recreation.
885896
if err := api.RefreshContainers(ctx); err != nil {
886897
logger.Error(ctx, "failed to trigger immediate refresh after devcontainer recreation", slog.Error(err))
898+
return err
887899
}
900+
901+
return nil
888902
}
889903

890904
// markDevcontainerDirty finds the devcontainer with the given config file path

‎agent/agentcontainers/devcontainer.go

Lines changed: 0 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@ package agentcontainers
22

33
import (
44
"context"
5-
"fmt"
65
"os"
76
"path/filepath"
8-
"strings"
97

108
"cdr.dev/slog"
119
"github.com/coder/coder/v2/codersdk"
@@ -22,61 +20,6 @@ const (
2220
DevcontainerDefaultContainerWorkspaceFolder = "/workspaces"
2321
)
2422

25-
const devcontainerUpScriptTemplate = `
26-
if ! which devcontainer > /dev/null 2>&1; then
27-
echo "ERROR: Unable to start devcontainer, @devcontainers/cli is not installed or not found in \$PATH." 1>&2
28-
echo "Please install @devcontainers/cli by running \"npm install -g @devcontainers/cli\" or by using the \"devcontainers-cli\" Coder module." 1>&2
29-
exit 1
30-
fi
31-
devcontainer up %s
32-
`
33-
34-
// ExtractAndInitializeDevcontainerScripts extracts devcontainer scripts from
35-
// the given scripts and devcontainers. The devcontainer scripts are removed
36-
// from the returned scripts so that they can be run separately.
37-
//
38-
// Dev Containers have an inherent dependency on start scripts, since they
39-
// initialize the workspace (e.g. git clone, npm install, etc). This is
40-
// important if e.g. a Coder module to install @devcontainer/cli is used.
41-
func ExtractAndInitializeDevcontainerScripts(
42-
devcontainers []codersdk.WorkspaceAgentDevcontainer,
43-
scripts []codersdk.WorkspaceAgentScript,
44-
) (filteredScripts []codersdk.WorkspaceAgentScript, devcontainerScripts []codersdk.WorkspaceAgentScript) {
45-
ScriptLoop:
46-
for _, script := range scripts {
47-
for _, dc := range devcontainers {
48-
// The devcontainer scripts match the devcontainer ID for
49-
// identification.
50-
if script.ID == dc.ID {
51-
devcontainerScripts = append(devcontainerScripts, devcontainerStartupScript(dc, script))
52-
continue ScriptLoop
53-
}
54-
}
55-
56-
filteredScripts = append(filteredScripts, script)
57-
}
58-
59-
return filteredScripts, devcontainerScripts
60-
}
61-
62-
func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript {
63-
args := []string{
64-
"--log-format json",
65-
fmt.Sprintf("--workspace-folder %q", dc.WorkspaceFolder),
66-
}
67-
if dc.ConfigPath != "" {
68-
args = append(args, fmt.Sprintf("--config %q", dc.ConfigPath))
69-
}
70-
cmd := fmt.Sprintf(devcontainerUpScriptTemplate, strings.Join(args, " "))
71-
// Force the script to run in /bin/sh, since some shells (e.g. fish)
72-
// don't support the script.
73-
script.Script = fmt.Sprintf("/bin/sh -c '%s'", cmd)
74-
// Disable RunOnStart, scripts have this set so that when devcontainers
75-
// have not been enabled, a warning will be surfaced in the agent logs.
76-
script.RunOnStart = false
77-
return script
78-
}
79-
8023
// ExpandAllDevcontainerPaths expands all devcontainer paths in the given
8124
// devcontainers. This is required by the devcontainer CLI, which requires
8225
// absolute paths for the workspace folder and config path.

0 commit comments

Comments
 (0)