Skip to content

fix(agent): start devcontainers through agentcontainers package #18471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -1145,13 +1145,6 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
scripts = manifest.Scripts
scriptRunnerOpts []agentscripts.InitOption
)
if a.experimentalDevcontainersEnabled {
var dcScripts []codersdk.WorkspaceAgentScript
scripts, dcScripts = agentcontainers.ExtractAndInitializeDevcontainerScripts(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...)
if err != nil {
return xerrors.Errorf("init script runner: %w", err)
Expand All @@ -1169,7 +1162,13 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
// 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))

if cAPI := a.containerAPI.Load(); cAPI != nil {
for _, dc := range manifest.Devcontainers {
err = errors.Join(err, cAPI.CreateDevcontainer(dc))
}
}

dur := time.Since(start).Seconds()
if err != nil {
a.logger.Warn(ctx, "startup script(s) failed", slog.Error(err))
Expand Down
22 changes: 19 additions & 3 deletions agent/agentcontainers/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,9 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques
dc.Container = nil
api.knownDevcontainers[dc.WorkspaceFolder] = dc
api.asyncWg.Add(1)
go api.recreateDevcontainer(dc, configPath)
go func() {
_ = api.recreateDevcontainer(dc, configPath)
}()

api.mu.Unlock()

Expand All @@ -794,14 +796,25 @@ func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Reques
})
}

func (api *API) CreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer) error {
api.mu.Lock()
dc.Status = codersdk.WorkspaceAgentDevcontainerStatusStarting
dc.Container = nil
api.knownDevcontainers[dc.WorkspaceFolder] = dc
api.asyncWg.Add(1)
api.mu.Unlock()

return api.recreateDevcontainer(dc, dc.ConfigPath)
}

// recreateDevcontainer should run in its own goroutine and is responsible for
// recreating a devcontainer based on the provided devcontainer configuration.
// It updates the devcontainer status and logs the process. The configPath is
// passed as a parameter for the odd chance that the container being recreated
// has a different config file than the one stored in the devcontainer state.
// The devcontainer state must be set to starting and the asyncWg must be
// incremented before calling this function.
func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) {
func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, configPath string) error {
defer api.asyncWg.Done()

var (
Expand Down Expand Up @@ -857,7 +870,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con
api.knownDevcontainers[dc.WorkspaceFolder] = dc
api.recreateErrorTimes[dc.WorkspaceFolder] = api.clock.Now("agentcontainers", "recreate", "errorTimes")
api.mu.Unlock()
return
return err
}

logger.Info(ctx, "devcontainer recreated successfully")
Expand All @@ -884,7 +897,10 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con
// devcontainer state after recreation.
if err := api.RefreshContainers(ctx); err != nil {
logger.Error(ctx, "failed to trigger immediate refresh after devcontainer recreation", slog.Error(err))
return err
}

return nil
}

// markDevcontainerDirty finds the devcontainer with the given config file path
Expand Down
57 changes: 0 additions & 57 deletions agent/agentcontainers/devcontainer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@ package agentcontainers

import (
"context"
"fmt"
"os"
"path/filepath"
"strings"

"cdr.dev/slog"
"github.com/coder/coder/v2/codersdk"
Expand All @@ -22,61 +20,6 @@ const (
DevcontainerDefaultContainerWorkspaceFolder = "/workspaces"
)

const devcontainerUpScriptTemplate = `
if ! which devcontainer > /dev/null 2>&1; then
echo "ERROR: Unable to start devcontainer, @devcontainers/cli is not installed or not found in \$PATH." 1>&2
echo "Please install @devcontainers/cli by running \"npm install -g @devcontainers/cli\" or by using the \"devcontainers-cli\" Coder module." 1>&2
exit 1
fi
devcontainer up %s
`

// 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(
devcontainers []codersdk.WorkspaceAgentDevcontainer,
scripts []codersdk.WorkspaceAgentScript,
) (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))
continue ScriptLoop
}
}

filteredScripts = append(filteredScripts, script)
}

return filteredScripts, devcontainerScripts
}

func devcontainerStartupScript(dc codersdk.WorkspaceAgentDevcontainer, script codersdk.WorkspaceAgentScript) codersdk.WorkspaceAgentScript {
args := []string{
"--log-format json",
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, " "))
// Force the script to run in /bin/sh, since some shells (e.g. fish)
// don't support the script.
script.Script = fmt.Sprintf("/bin/sh -c '%s'", 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
}

// ExpandAllDevcontainerPaths expands all devcontainer paths in the given
// devcontainers. This is required by the devcontainer CLI, which requires
// absolute paths for the workspace folder and config path.
Expand Down
Loading
Loading