From 2fd85f759a4b925db021a86dffb74d4a8ed48ac2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 11:12:30 +0000 Subject: [PATCH 01/16] refactor(agent): update agentcontainers api initialization --- agent/agent.go | 8 +-- agent/agentcontainers/api.go | 83 +++++++++++++++++----- agent/agentcontainers/api_internal_test.go | 2 +- agent/agentcontainers/api_test.go | 4 +- agent/agentcontainers/devcontainercli.go | 10 +++ agent/api.go | 6 +- cli/agent.go | 11 +-- cli/exp_rpty_test.go | 2 - cli/open_test.go | 7 +- cli/ssh_test.go | 3 +- coderd/workspaceagents_test.go | 10 +-- 11 files changed, 100 insertions(+), 46 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index b195368338242..14276a426aea2 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -89,8 +89,8 @@ type Options struct { ServiceBannerRefreshInterval time.Duration BlockFileTransfer bool Execer agentexec.Execer - ContainerLister agentcontainers.Lister + ContainerAPIOptions []agentcontainers.Option ExperimentalDevcontainersEnabled bool } @@ -154,9 +154,6 @@ func New(options Options) Agent { if options.Execer == nil { options.Execer = agentexec.DefaultExecer } - if options.ContainerLister == nil { - options.ContainerLister = agentcontainers.NoopLister{} - } hardCtx, hardCancel := context.WithCancel(context.Background()) gracefulCtx, gracefulCancel := context.WithCancel(hardCtx) @@ -192,8 +189,8 @@ func New(options Options) Agent { prometheusRegistry: prometheusRegistry, metrics: newAgentMetrics(prometheusRegistry), execer: options.Execer, - lister: options.ContainerLister, + containerAPIOptions: options.ContainerAPIOptions, experimentalDevcontainersEnabled: options.ExperimentalDevcontainersEnabled, } // Initially, we have a closed channel, reflecting the fact that we are not initially connected. @@ -276,6 +273,7 @@ type agent struct { execer agentexec.Execer lister agentcontainers.Lister + containerAPIOptions []agentcontainers.Option experimentalDevcontainersEnabled bool } diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 489bc1e55194c..0ad26f19d7484 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -39,6 +39,7 @@ type API struct { watcher watcher.Watcher cacheDuration time.Duration + execer agentexec.Execer cl Lister dccli DevcontainerCLI clock quartz.Clock @@ -51,19 +52,15 @@ type API struct { devcontainerNames map[string]struct{} // Track devcontainer names to avoid duplicates. knownDevcontainers []codersdk.WorkspaceAgentDevcontainer // Track predefined and runtime-detected devcontainers. configFileModifiedTimes map[string]time.Time // Track when config files were last modified. + + // experimentalDevcontainersEnabled indicates if the agent is + // running in experimental mode with devcontainers enabled. + experimentalDevcontainersEnabled bool } // Option is a functional option for API. type Option func(*API) -// WithLister sets the agentcontainers.Lister implementation to use. -// The default implementation uses the Docker CLI to list containers. -func WithLister(cl Lister) Option { - return func(api *API) { - api.cl = cl - } -} - // WithClock sets the quartz.Clock implementation to use. // This is primarily used for testing to control time. func WithClock(clock quartz.Clock) Option { @@ -72,6 +69,21 @@ func WithClock(clock quartz.Clock) Option { } } +// WithExecer sets the agentexec.Execer implementation to use. +func WithExecer(execer agentexec.Execer) Option { + return func(api *API) { + api.execer = execer + } +} + +// WithLister sets the agentcontainers.Lister implementation to use. +// The default implementation uses the Docker CLI to list containers. +func WithLister(cl Lister) Option { + return func(api *API) { + api.cl = cl + } +} + // WithDevcontainerCLI sets the DevcontainerCLI implementation to use. // This can be used in tests to modify @devcontainer/cli behavior. func WithDevcontainerCLI(dccli DevcontainerCLI) Option { @@ -105,7 +117,9 @@ func WithWatcher(w watcher.Watcher) Option { } // NewAPI returns a new API with the given options applied. -func NewAPI(logger slog.Logger, options ...Option) *API { +// +//nolint:revive // experimentalDevcontainersEnabled is a control flag. +func NewAPI(logger slog.Logger, experimentalDevcontainersEnabled bool, options ...Option) *API { ctx, cancel := context.WithCancel(context.Background()) api := &API{ ctx: ctx, @@ -113,22 +127,36 @@ func NewAPI(logger slog.Logger, options ...Option) *API { done: make(chan struct{}), logger: logger, clock: quartz.NewReal(), + execer: agentexec.DefaultExecer, cacheDuration: defaultGetContainersCacheDuration, lockCh: make(chan struct{}, 1), devcontainerNames: make(map[string]struct{}), knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{}, configFileModifiedTimes: make(map[string]time.Time), + + experimentalDevcontainersEnabled: experimentalDevcontainersEnabled, } for _, opt := range options { opt(api) } - if api.cl == nil { - api.cl = &DockerCLILister{} - } - if api.dccli == nil { - api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), agentexec.DefaultExecer) - } - if api.watcher == nil { + if api.experimentalDevcontainersEnabled { + if api.cl == nil { + api.cl = NewDocker(api.execer) + } + if api.dccli == nil { + api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), api.execer) + } + if api.watcher == nil { + var err error + api.watcher, err = watcher.NewFSNotify() + if err != nil { + logger.Error(ctx, "create file watcher service failed", slog.Error(err)) + api.watcher = watcher.NewNoop() + } + } + } else { + api.cl = &NoopLister{} + api.dccli = &noopDevcontainerCLI{} api.watcher = watcher.NewNoop() } @@ -187,12 +215,35 @@ func (api *API) start() { // Routes returns the HTTP handler for container-related routes. func (api *API) Routes() http.Handler { r := chi.NewRouter() + + if !api.experimentalDevcontainersEnabled { + r.Get("/", api.handleDisabledEmptyList) + r.Get("/devcontainers", api.handleDisabled) + r.Post("/{id}/recreate", api.handleDisabled) + + return r + } + r.Get("/", api.handleList) r.Get("/devcontainers", api.handleListDevcontainers) r.Post("/{id}/recreate", api.handleRecreate) + return r } +func (api *API) handleDisabled(w http.ResponseWriter, r *http.Request) { + httpapi.Write(r.Context(), w, http.StatusNotImplemented, codersdk.Response{ + Message: "Devcontainers are not enabled in this agent.", + Detail: "Devcontainers are not enabled in this agent.", + }) +} + +func (api *API) handleDisabledEmptyList(w http.ResponseWriter, r *http.Request) { + httpapi.Write(r.Context(), w, http.StatusOK, codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{}, + }) +} + // handleList handles the HTTP request to list containers. func (api *API) handleList(rw http.ResponseWriter, r *http.Request) { select { diff --git a/agent/agentcontainers/api_internal_test.go b/agent/agentcontainers/api_internal_test.go index 331c41e8df10b..3ebd4f34791b4 100644 --- a/agent/agentcontainers/api_internal_test.go +++ b/agent/agentcontainers/api_internal_test.go @@ -101,7 +101,7 @@ func TestAPI(t *testing.T) { mockLister = acmock.NewMockLister(ctrl) now = time.Now().UTC() logger = slogtest.Make(t, nil).Leveled(slog.LevelDebug) - api = NewAPI(logger, WithLister(mockLister)) + api = NewAPI(logger, true, WithLister(mockLister)) ) defer api.Close() diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index a246d929d9089..bca550249d287 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -251,6 +251,7 @@ func TestAPI(t *testing.T) { r := chi.NewRouter() api := agentcontainers.NewAPI( logger, + true, agentcontainers.WithLister(tt.lister), agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), ) @@ -564,7 +565,7 @@ func TestAPI(t *testing.T) { apiOptions = append(apiOptions, agentcontainers.WithDevcontainers(tt.knownDevcontainers)) } - api := agentcontainers.NewAPI(logger, apiOptions...) + api := agentcontainers.NewAPI(logger, true, apiOptions...) defer api.Close() r.Mount("/", api.Routes()) @@ -625,6 +626,7 @@ func TestAPI(t *testing.T) { logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) api := agentcontainers.NewAPI( logger, + true, agentcontainers.WithLister(fLister), agentcontainers.WithWatcher(fWatcher), agentcontainers.WithClock(mClock), diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index d6060f862cb40..18a6eafa222ac 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -191,3 +191,13 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { } return len(p), nil } + +// noopDevcontainerCLI is a no-op implementation of the DevcontainerCLI +// interface. It is used when devcontainers are not enabled. +type noopDevcontainerCLI struct{} + +func (n *noopDevcontainerCLI) Up(context.Context, string, string, ...DevcontainerCLIUpOptions) (string, error) { + return "", nil +} + +var _ DevcontainerCLI = &noopDevcontainerCLI{} diff --git a/agent/api.go b/agent/api.go index 97a04333f147e..d6974c44dd3d5 100644 --- a/agent/api.go +++ b/agent/api.go @@ -37,9 +37,7 @@ func (a *agent) apiHandler() (http.Handler, func() error) { cacheDuration: cacheDuration, } - containerAPIOpts := []agentcontainers.Option{ - agentcontainers.WithLister(a.lister), - } + var containerAPIOpts []agentcontainers.Option if a.experimentalDevcontainersEnabled { manifest := a.manifest.Load() if manifest != nil && len(manifest.Devcontainers) > 0 { @@ -49,7 +47,7 @@ func (a *agent) apiHandler() (http.Handler, func() error) { ) } } - containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), a.experimentalDevcontainersEnabled, containerAPIOpts...) promHandler := PrometheusMetricsHandler(a.prometheusRegistry, a.logger) diff --git a/cli/agent.go b/cli/agent.go index 18c4542a6c3a0..5d6cdbd66b4e0 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -26,7 +26,6 @@ import ( "cdr.dev/slog/sloggers/slogjson" "cdr.dev/slog/sloggers/slogstackdriver" "github.com/coder/coder/v2/agent" - "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/agent/agentssh" "github.com/coder/coder/v2/agent/reaper" @@ -319,13 +318,10 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { return xerrors.Errorf("create agent execer: %w", err) } - var containerLister agentcontainers.Lister - if !experimentalDevcontainersEnabled { - logger.Info(ctx, "agent devcontainer detection not enabled") - containerLister = &agentcontainers.NoopLister{} - } else { + if experimentalDevcontainersEnabled { logger.Info(ctx, "agent devcontainer detection enabled") - containerLister = agentcontainers.NewDocker(execer) + } else { + logger.Info(ctx, "agent devcontainer detection not enabled") } agnt := agent.New(agent.Options{ @@ -354,7 +350,6 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { PrometheusRegistry: prometheusRegistry, BlockFileTransfer: blockFileTransfer, Execer: execer, - ContainerLister: containerLister, ExperimentalDevcontainersEnabled: experimentalDevcontainersEnabled, }) diff --git a/cli/exp_rpty_test.go b/cli/exp_rpty_test.go index b7f26beb87f2f..355cc1741b5a9 100644 --- a/cli/exp_rpty_test.go +++ b/cli/exp_rpty_test.go @@ -9,7 +9,6 @@ import ( "github.com/ory/dockertest/v3/docker" "github.com/coder/coder/v2/agent" - "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/cli/clitest" "github.com/coder/coder/v2/coderd/coderdtest" @@ -112,7 +111,6 @@ func TestExpRpty(t *testing.T) { _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { o.ExperimentalDevcontainersEnabled = true - o.ContainerLister = agentcontainers.NewDocker(o.Execer) }) _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() diff --git a/cli/open_test.go b/cli/open_test.go index f0183022782d9..9ba16a32674e2 100644 --- a/cli/open_test.go +++ b/cli/open_test.go @@ -14,6 +14,7 @@ import ( "go.uber.org/mock/gomock" "github.com/coder/coder/v2/agent" + "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agentcontainers/acmock" "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/cli/clitest" @@ -335,7 +336,8 @@ func TestOpenVSCodeDevContainer(t *testing.T) { }) _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { - o.ContainerLister = mcl + o.ExperimentalDevcontainersEnabled = true + o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl)) }) _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() @@ -508,7 +510,8 @@ func TestOpenVSCodeDevContainer_NoAgentDirectory(t *testing.T) { }) _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { - o.ContainerLister = mcl + o.ExperimentalDevcontainersEnabled = true + o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl)) }) _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() diff --git a/cli/ssh_test.go b/cli/ssh_test.go index c8ad072270169..f5bcaf2540b86 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -2029,7 +2029,6 @@ func TestSSH_Container(t *testing.T) { _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { o.ExperimentalDevcontainersEnabled = true - o.ContainerLister = agentcontainers.NewDocker(o.Execer) }) _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() @@ -2058,7 +2057,7 @@ func TestSSH_Container(t *testing.T) { mLister := acmock.NewMockLister(ctrl) _ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) { o.ExperimentalDevcontainersEnabled = true - o.ContainerLister = mLister + o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mLister)) }) _ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait() diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index a6e10ea5fdabf..7e3d141ebb09d 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -35,7 +35,6 @@ import ( "github.com/coder/coder/v2/agent" "github.com/coder/coder/v2/agent/agentcontainers" "github.com/coder/coder/v2/agent/agentcontainers/acmock" - "github.com/coder/coder/v2/agent/agentexec" "github.com/coder/coder/v2/agent/agenttest" agentproto "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/coderd/coderdtest" @@ -1171,8 +1170,8 @@ func TestWorkspaceAgentContainers(t *testing.T) { }).WithAgent(func(agents []*proto.Agent) []*proto.Agent { return agents }).Do() - _ = agenttest.New(t, client.URL, r.AgentToken, func(opts *agent.Options) { - opts.ContainerLister = agentcontainers.NewDocker(agentexec.DefaultExecer) + _ = agenttest.New(t, client.URL, r.AgentToken, func(o *agent.Options) { + o.ExperimentalDevcontainersEnabled = true }) resources := coderdtest.NewWorkspaceAgentWaiter(t, client, r.Workspace.ID).Wait() require.Len(t, resources, 1, "expected one resource") @@ -1273,8 +1272,9 @@ func TestWorkspaceAgentContainers(t *testing.T) { }).WithAgent(func(agents []*proto.Agent) []*proto.Agent { return agents }).Do() - _ = agenttest.New(t, client.URL, r.AgentToken, func(opts *agent.Options) { - opts.ContainerLister = mcl + _ = agenttest.New(t, client.URL, r.AgentToken, func(o *agent.Options) { + o.ExperimentalDevcontainersEnabled = true + o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl)) }) resources := coderdtest.NewWorkspaceAgentWaiter(t, client, r.Workspace.ID).Wait() require.Len(t, resources, 1, "expected one resource") From 901a258fb2523dad52a5209ac13f7743b0e578f9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 11:21:41 +0000 Subject: [PATCH 02/16] add warn --- agent/agentcontainers/api.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 0ad26f19d7484..d4aca00a8c55b 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -155,6 +155,9 @@ func NewAPI(logger slog.Logger, experimentalDevcontainersEnabled bool, options . } } } else { + if api.cl != nil || api.dccli != nil || api.watcher != nil { + logger.Warn(ctx, "devcontainers are disabled but API is configured with devcontainer services") + } api.cl = &NoopLister{} api.dccli = &noopDevcontainerCLI{} api.watcher = watcher.NewNoop() From 44cc9a65171ae38047c8987c5ae6060a3a945ee7 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 11:22:25 +0000 Subject: [PATCH 03/16] lint --- agent/agent.go | 1 - agent/agentcontainers/api.go | 4 ++-- agent/agentcontainers/devcontainercli.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 14276a426aea2..bfbdbc59ec643 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -271,7 +271,6 @@ type agent struct { // labeled in Coder with the agent + workspace. metrics *agentMetrics execer agentexec.Execer - lister agentcontainers.Lister containerAPIOptions []agentcontainers.Option experimentalDevcontainersEnabled bool diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index d4aca00a8c55b..99c1244398051 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -234,14 +234,14 @@ func (api *API) Routes() http.Handler { return r } -func (api *API) handleDisabled(w http.ResponseWriter, r *http.Request) { +func (*API) handleDisabled(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusNotImplemented, codersdk.Response{ Message: "Devcontainers are not enabled in this agent.", Detail: "Devcontainers are not enabled in this agent.", }) } -func (api *API) handleDisabledEmptyList(w http.ResponseWriter, r *http.Request) { +func (*API) handleDisabledEmptyList(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusOK, codersdk.WorkspaceAgentListContainersResponse{ Containers: []codersdk.WorkspaceAgentContainer{}, }) diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 18a6eafa222ac..26c00055c11be 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -196,7 +196,7 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { // interface. It is used when devcontainers are not enabled. type noopDevcontainerCLI struct{} -func (n *noopDevcontainerCLI) Up(context.Context, string, string, ...DevcontainerCLIUpOptions) (string, error) { +func (*noopDevcontainerCLI) Up(context.Context, string, string, ...DevcontainerCLIUpOptions) (string, error) { return "", nil } From ebed3de758dde43f813c08d5164f2f511898d2e8 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 11:35:44 +0000 Subject: [PATCH 04/16] refactor, remove control flag --- agent/agentcontainers/api.go | 64 +++++----------------- agent/agentcontainers/api_internal_test.go | 2 +- agent/agentcontainers/api_test.go | 4 +- agent/api.go | 23 ++++++-- 4 files changed, 34 insertions(+), 59 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 99c1244398051..bfc560fcf8d94 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -52,10 +52,6 @@ type API struct { devcontainerNames map[string]struct{} // Track devcontainer names to avoid duplicates. knownDevcontainers []codersdk.WorkspaceAgentDevcontainer // Track predefined and runtime-detected devcontainers. configFileModifiedTimes map[string]time.Time // Track when config files were last modified. - - // experimentalDevcontainersEnabled indicates if the agent is - // running in experimental mode with devcontainers enabled. - experimentalDevcontainersEnabled bool } // Option is a functional option for API. @@ -117,9 +113,7 @@ func WithWatcher(w watcher.Watcher) Option { } // NewAPI returns a new API with the given options applied. -// -//nolint:revive // experimentalDevcontainersEnabled is a control flag. -func NewAPI(logger slog.Logger, experimentalDevcontainersEnabled bool, options ...Option) *API { +func NewAPI(logger slog.Logger, options ...Option) *API { ctx, cancel := context.WithCancel(context.Background()) api := &API{ ctx: ctx, @@ -133,34 +127,23 @@ func NewAPI(logger slog.Logger, experimentalDevcontainersEnabled bool, options . devcontainerNames: make(map[string]struct{}), knownDevcontainers: []codersdk.WorkspaceAgentDevcontainer{}, configFileModifiedTimes: make(map[string]time.Time), - - experimentalDevcontainersEnabled: experimentalDevcontainersEnabled, } for _, opt := range options { opt(api) } - if api.experimentalDevcontainersEnabled { - if api.cl == nil { - api.cl = NewDocker(api.execer) - } - if api.dccli == nil { - api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), api.execer) - } - if api.watcher == nil { - var err error - api.watcher, err = watcher.NewFSNotify() - if err != nil { - logger.Error(ctx, "create file watcher service failed", slog.Error(err)) - api.watcher = watcher.NewNoop() - } - } - } else { - if api.cl != nil || api.dccli != nil || api.watcher != nil { - logger.Warn(ctx, "devcontainers are disabled but API is configured with devcontainer services") + if api.cl == nil { + api.cl = NewDocker(api.execer) + } + if api.dccli == nil { + api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), api.execer) + } + if api.watcher == nil { + var err error + api.watcher, err = watcher.NewFSNotify() + if err != nil { + logger.Error(ctx, "create file watcher service failed", slog.Error(err)) + api.watcher = watcher.NewNoop() } - api.cl = &NoopLister{} - api.dccli = &noopDevcontainerCLI{} - api.watcher = watcher.NewNoop() } // Make sure we watch the devcontainer config files for changes. @@ -219,14 +202,6 @@ func (api *API) start() { func (api *API) Routes() http.Handler { r := chi.NewRouter() - if !api.experimentalDevcontainersEnabled { - r.Get("/", api.handleDisabledEmptyList) - r.Get("/devcontainers", api.handleDisabled) - r.Post("/{id}/recreate", api.handleDisabled) - - return r - } - r.Get("/", api.handleList) r.Get("/devcontainers", api.handleListDevcontainers) r.Post("/{id}/recreate", api.handleRecreate) @@ -234,19 +209,6 @@ func (api *API) Routes() http.Handler { return r } -func (*API) handleDisabled(w http.ResponseWriter, r *http.Request) { - httpapi.Write(r.Context(), w, http.StatusNotImplemented, codersdk.Response{ - Message: "Devcontainers are not enabled in this agent.", - Detail: "Devcontainers are not enabled in this agent.", - }) -} - -func (*API) handleDisabledEmptyList(w http.ResponseWriter, r *http.Request) { - httpapi.Write(r.Context(), w, http.StatusOK, codersdk.WorkspaceAgentListContainersResponse{ - Containers: []codersdk.WorkspaceAgentContainer{}, - }) -} - // handleList handles the HTTP request to list containers. func (api *API) handleList(rw http.ResponseWriter, r *http.Request) { select { diff --git a/agent/agentcontainers/api_internal_test.go b/agent/agentcontainers/api_internal_test.go index 3ebd4f34791b4..331c41e8df10b 100644 --- a/agent/agentcontainers/api_internal_test.go +++ b/agent/agentcontainers/api_internal_test.go @@ -101,7 +101,7 @@ func TestAPI(t *testing.T) { mockLister = acmock.NewMockLister(ctrl) now = time.Now().UTC() logger = slogtest.Make(t, nil).Leveled(slog.LevelDebug) - api = NewAPI(logger, true, WithLister(mockLister)) + api = NewAPI(logger, WithLister(mockLister)) ) defer api.Close() diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index bca550249d287..a246d929d9089 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -251,7 +251,6 @@ func TestAPI(t *testing.T) { r := chi.NewRouter() api := agentcontainers.NewAPI( logger, - true, agentcontainers.WithLister(tt.lister), agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), ) @@ -565,7 +564,7 @@ func TestAPI(t *testing.T) { apiOptions = append(apiOptions, agentcontainers.WithDevcontainers(tt.knownDevcontainers)) } - api := agentcontainers.NewAPI(logger, true, apiOptions...) + api := agentcontainers.NewAPI(logger, apiOptions...) defer api.Close() r.Mount("/", api.Routes()) @@ -626,7 +625,6 @@ func TestAPI(t *testing.T) { logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) api := agentcontainers.NewAPI( logger, - true, agentcontainers.WithLister(fLister), agentcontainers.WithWatcher(fWatcher), agentcontainers.WithClock(mClock), diff --git a/agent/api.go b/agent/api.go index d6974c44dd3d5..5a8fc43f47c55 100644 --- a/agent/api.go +++ b/agent/api.go @@ -37,8 +37,11 @@ func (a *agent) apiHandler() (http.Handler, func() error) { cacheDuration: cacheDuration, } - var containerAPIOpts []agentcontainers.Option + var containerAPI *agentcontainers.API if a.experimentalDevcontainersEnabled { + containerAPIOpts := []agentcontainers.Option{ + agentcontainers.WithExecer(a.execer), + } manifest := a.manifest.Load() if manifest != nil && len(manifest.Devcontainers) > 0 { containerAPIOpts = append( @@ -46,12 +49,21 @@ func (a *agent) apiHandler() (http.Handler, func() error) { agentcontainers.WithDevcontainers(manifest.Devcontainers), ) } + + containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + + r.Mount("/api/v0/containers", containerAPI.Routes()) + } else { + r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { + httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ + Message: "Container API is not enabled.", + Detail: "Set the --experimental-devcontainers flag to enable the container API.", + }) + }) } - containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), a.experimentalDevcontainersEnabled, containerAPIOpts...) promHandler := PrometheusMetricsHandler(a.prometheusRegistry, a.logger) - r.Mount("/api/v0/containers", containerAPI.Routes()) r.Get("/api/v0/listening-ports", lp.handler) r.Get("/api/v0/netcheck", a.HandleNetcheck) r.Post("/api/v0/list-directory", a.HandleLS) @@ -62,7 +74,10 @@ func (a *agent) apiHandler() (http.Handler, func() error) { r.Get("/debug/prometheus", promHandler.ServeHTTP) return r, func() error { - return containerAPI.Close() + if containerAPI != nil { + return containerAPI.Close() + } + return nil } } From 02d022831e1f142623ec3119ac8df57f23ac06ef Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 11:39:54 +0000 Subject: [PATCH 05/16] remove noop dccli --- agent/agentcontainers/devcontainercli.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 26c00055c11be..d6060f862cb40 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -191,13 +191,3 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { } return len(p), nil } - -// noopDevcontainerCLI is a no-op implementation of the DevcontainerCLI -// interface. It is used when devcontainers are not enabled. -type noopDevcontainerCLI struct{} - -func (*noopDevcontainerCLI) Up(context.Context, string, string, ...DevcontainerCLIUpOptions) (string, error) { - return "", nil -} - -var _ DevcontainerCLI = &noopDevcontainerCLI{} From 2a9a8422518bafd158cc3d5b7b516e4d39753f36 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 11:42:22 +0000 Subject: [PATCH 06/16] fix agent options --- agent/api.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/agent/api.go b/agent/api.go index 5a8fc43f47c55..5622896ab53c8 100644 --- a/agent/api.go +++ b/agent/api.go @@ -50,6 +50,9 @@ func (a *agent) apiHandler() (http.Handler, func() error) { ) } + // Append after to allow the agent options to override the default options. + containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) + containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) r.Mount("/api/v0/containers", containerAPI.Routes()) From fd6922147e8f873dbcd8230b6015a3390f26eb34 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 12:07:26 +0000 Subject: [PATCH 07/16] update agentcontainers API file watch init --- agent/agent.go | 8 +++++++- agent/agentcontainers/api.go | 27 ++++++++++++++++++--------- agent/agentcontainers/api_test.go | 5 +++++ agent/api.go | 10 ++++------ 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index bfbdbc59ec643..18699f23b214c 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -272,8 +272,9 @@ type agent struct { metrics *agentMetrics execer agentexec.Execer - containerAPIOptions []agentcontainers.Option experimentalDevcontainersEnabled bool + containerAPIOptions []agentcontainers.Option + containerAPI *agentcontainers.API } func (a *agent) TailnetConn() *tailnet.Conn { @@ -1167,6 +1168,11 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur) a.scriptRunner.StartCron() + if a.containerAPI != nil { + // Start the containerAPI service after + // devcontainers have been started. + a.containerAPI.Start() + } }) if err != nil { return xerrors.Errorf("track conn goroutine: %w", err) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index bfc560fcf8d94..4f59877c9e0c0 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -146,21 +146,30 @@ func NewAPI(logger slog.Logger, options ...Option) *API { } } + go api.loop() + + return api +} + +// Start watching for devcontainer config file changes and prime the +// cache with the current list of containers. +func (api *API) Start() { + // Prime the cache with the current list of containers. + _, _ = api.cl.List(api.ctx) + // Make sure we watch the devcontainer config files for changes. for _, devcontainer := range api.knownDevcontainers { - if devcontainer.ConfigPath != "" { - if err := api.watcher.Add(devcontainer.ConfigPath); err != nil { - api.logger.Error(ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath)) - } + if devcontainer.ConfigPath == "" { + continue } - } - - go api.start() - return api + if err := api.watcher.Add(devcontainer.ConfigPath); err != nil { + api.logger.Error(api.ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath)) + } + } } -func (api *API) start() { +func (api *API) loop() { defer close(api.done) for { diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index a246d929d9089..96ff0b7b58794 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -18,6 +18,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/agent/agentcontainers" + "github.com/coder/coder/v2/agent/agentcontainers/watcher" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" "github.com/coder/quartz" @@ -253,6 +254,7 @@ func TestAPI(t *testing.T) { logger, agentcontainers.WithLister(tt.lister), agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI), + agentcontainers.WithWatcher(watcher.NewNoop()), ) defer api.Close() r.Mount("/", api.Routes()) @@ -558,6 +560,7 @@ func TestAPI(t *testing.T) { r := chi.NewRouter() apiOptions := []agentcontainers.Option{ agentcontainers.WithLister(tt.lister), + agentcontainers.WithWatcher(watcher.NewNoop()), } if len(tt.knownDevcontainers) > 0 { @@ -631,6 +634,8 @@ func TestAPI(t *testing.T) { ) defer api.Close() + api.Start() + r := chi.NewRouter() r.Mount("/", api.Routes()) diff --git a/agent/api.go b/agent/api.go index 5622896ab53c8..18110c875a00d 100644 --- a/agent/api.go +++ b/agent/api.go @@ -37,7 +37,6 @@ func (a *agent) apiHandler() (http.Handler, func() error) { cacheDuration: cacheDuration, } - var containerAPI *agentcontainers.API if a.experimentalDevcontainersEnabled { containerAPIOpts := []agentcontainers.Option{ agentcontainers.WithExecer(a.execer), @@ -53,9 +52,8 @@ func (a *agent) apiHandler() (http.Handler, func() error) { // Append after to allow the agent options to override the default options. containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) - containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) - - r.Mount("/api/v0/containers", containerAPI.Routes()) + a.containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + r.Mount("/api/v0/containers", a.containerAPI.Routes()) } else { r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ @@ -77,8 +75,8 @@ func (a *agent) apiHandler() (http.Handler, func() error) { r.Get("/debug/prometheus", promHandler.ServeHTTP) return r, func() error { - if containerAPI != nil { - return containerAPI.Close() + if a.containerAPI != nil { + return a.containerAPI.Close() } return nil } From 17fa155d64b54cbb6cefb474b078c60f23be9272 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 12:15:48 +0000 Subject: [PATCH 08/16] rename start to ready --- agent/agent.go | 7 ++++--- agent/agentcontainers/api.go | 8 +++++--- agent/agentcontainers/api_test.go | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 18699f23b214c..e4cb47cec3162 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1169,9 +1169,10 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur) a.scriptRunner.StartCron() if a.containerAPI != nil { - // Start the containerAPI service after - // devcontainers have been started. - a.containerAPI.Start() + // Inform the container API that the agent is ready. + // This allows us to start watching for changes to + // the devcontainer configuration files. + a.containerAPI.Ready() } }) if err != nil { diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4f59877c9e0c0..671493b6a3de9 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -151,9 +151,11 @@ func NewAPI(logger slog.Logger, options ...Option) *API { return api } -// Start watching for devcontainer config file changes and prime the -// cache with the current list of containers. -func (api *API) Start() { +// Ready signals the API that we are ready to begin watching for file +// changes. This is used to prime the cache with the current list of +// containers and to start watching the devcontainer config files for +// changes. It should be called after the agent ready. +func (api *API) Ready() { // Prime the cache with the current list of containers. _, _ = api.cl.List(api.ctx) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 96ff0b7b58794..2669dd2160987 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -634,7 +634,7 @@ func TestAPI(t *testing.T) { ) defer api.Close() - api.Start() + api.Ready() r := chi.NewRouter() r.Mount("/", api.Routes()) From 3ebf2d370a2de535311940c1dc91028db230c438 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 12:30:31 +0000 Subject: [PATCH 09/16] update disabled message and use http 403 instead of 404 --- agent/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/api.go b/agent/api.go index 18110c875a00d..6fdc2405b37cf 100644 --- a/agent/api.go +++ b/agent/api.go @@ -56,9 +56,9 @@ func (a *agent) apiHandler() (http.Handler, func() error) { r.Mount("/api/v0/containers", a.containerAPI.Routes()) } else { r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { - httpapi.Write(r.Context(), w, http.StatusNotFound, codersdk.Response{ + httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{ Message: "Container API is not enabled.", - Detail: "Set the --experimental-devcontainers flag to enable the container API.", + Detail: "Set the CODER_AGENT_DEVCONTAINERS_ENABLE environment variable or use the --devcontainers-enable flag on the agent to enable the container API.", }) }) } From 9d572795cb6b4f2184bf930ae6d8d354301282c4 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 12:42:00 +0000 Subject: [PATCH 10/16] fix(site): handle containers disabled --- site/src/api/api.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 0e29fa969c903..260f5d4880ef2 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -2447,11 +2447,20 @@ class ApiMethods { labels?.map((label) => ["label", label]), ); - const res = - await this.axios.get( - `/api/v2/workspaceagents/${agentId}/containers?${params.toString()}`, - ); - return res.data; + try { + const res = + await this.axios.get( + `/api/v2/workspaceagents/${agentId}/containers?${params.toString()}`, + ); + return res.data; + } catch (err) { + // If the error is a 403, it means that experimental + // containers are not enabled on the agent. + if (isAxiosError(err) && err.response?.status === 403) { + return { containers: [] }; + } + throw err; + } }; getInboxNotifications = async (startingBeforeId?: string) => { From ea9f564bafa8c51c8ff05182334f7eab36dae48e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 12:44:14 +0000 Subject: [PATCH 11/16] fix copy --- agent/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/api.go b/agent/api.go index 6fdc2405b37cf..c09fe85764087 100644 --- a/agent/api.go +++ b/agent/api.go @@ -57,8 +57,8 @@ func (a *agent) apiHandler() (http.Handler, func() error) { } else { r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{ - Message: "Container API is not enabled.", - Detail: "Set the CODER_AGENT_DEVCONTAINERS_ENABLE environment variable or use the --devcontainers-enable flag on the agent to enable the container API.", + Message: "The agent dev containers feature is not enabled.", + Detail: "Set the CODER_AGENT_DEVCONTAINERS_ENABLE environment variable or use the --devcontainers-enable flag to enable this feature.", }) }) } From 3f9bd1dddd9a1efdd1afcdc59f519b38ec1359a6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 12:48:59 +0000 Subject: [PATCH 12/16] fix test --- agent/api.go | 4 ++-- cli/ssh.go | 2 -- cli/ssh_test.go | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/agent/api.go b/agent/api.go index c09fe85764087..48cb351c1eac5 100644 --- a/agent/api.go +++ b/agent/api.go @@ -57,8 +57,8 @@ func (a *agent) apiHandler() (http.Handler, func() error) { } else { r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{ - Message: "The agent dev containers feature is not enabled.", - Detail: "Set the CODER_AGENT_DEVCONTAINERS_ENABLE environment variable or use the --devcontainers-enable flag to enable this feature.", + Message: "The agent dev containers feature is experimental and not enabled by default.", + Detail: "To enable this feature, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.", }) }) } diff --git a/cli/ssh.go b/cli/ssh.go index e02443e7032c6..2025c1691b7d7 100644 --- a/cli/ssh.go +++ b/cli/ssh.go @@ -299,8 +299,6 @@ func (r *RootCmd) ssh() *serpent.Command { } if len(cts.Containers) == 0 { cliui.Info(inv.Stderr, "No containers found!") - cliui.Info(inv.Stderr, "Tip: Agent container integration is experimental and not enabled by default.") - cliui.Info(inv.Stderr, " To enable it, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.") return nil } var found bool diff --git a/cli/ssh_test.go b/cli/ssh_test.go index f5bcaf2540b86..b25cd1a94f724 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -2103,8 +2103,7 @@ func TestSSH_Container(t *testing.T) { assert.NoError(t, err) }) - ptty.ExpectMatch("No containers found!") - ptty.ExpectMatch("Tip: Agent container integration is experimental and not enabled by default.") + ptty.ExpectMatch("The agent dev containers feature is experimental and not enabled by default.") <-cmdDone }) } From 32b7d1828c5466d1d3e02b0ee67816b157e0072e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 13:04:39 +0000 Subject: [PATCH 13/16] echo codersdk error from agent --- coderd/workspaceagents.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 1388b61030d38..98e803581b946 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -848,6 +848,11 @@ func (api *API) workspaceAgentListContainers(rw http.ResponseWriter, r *http.Req }) return } + // If the agent returns a codersdk.Error, we can return that directly. + if cerr, ok := codersdk.AsError(err); ok { + httpapi.Write(ctx, rw, cerr.StatusCode(), cerr.Response) + return + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching containers.", Detail: err.Error(), From 6580f35be80678134d3bf6da1c11b9689a438ef8 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 13:12:36 +0000 Subject: [PATCH 14/16] fix test^2 --- cli/ssh_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/cli/ssh_test.go b/cli/ssh_test.go index b25cd1a94f724..2603c81e88cec 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -2096,15 +2096,9 @@ func TestSSH_Container(t *testing.T) { inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString()) clitest.SetupConfig(t, client, root) - ptty := ptytest.New(t).Attach(inv) - - cmdDone := tGo(t, func() { - err := inv.WithContext(ctx).Run() - assert.NoError(t, err) - }) - ptty.ExpectMatch("The agent dev containers feature is experimental and not enabled by default.") - <-cmdDone + err := inv.WithContext(ctx).Run() + require.ErrorContains(t, err, "The agent dev containers feature is experimental and not enabled by default.") }) } From 4a5842b5f899b6704ee12741b25c87837012f121 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 13:13:59 +0000 Subject: [PATCH 15/16] rename ready to signalready --- agent/agent.go | 2 +- agent/agentcontainers/api.go | 8 ++++---- agent/agentcontainers/api_test.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index e4cb47cec3162..1c04ef698f02c 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1172,7 +1172,7 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // Inform the container API that the agent is ready. // This allows us to start watching for changes to // the devcontainer configuration files. - a.containerAPI.Ready() + a.containerAPI.SignalReady() } }) if err != nil { diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 671493b6a3de9..c3779af67633a 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -151,11 +151,11 @@ func NewAPI(logger slog.Logger, options ...Option) *API { return api } -// Ready signals the API that we are ready to begin watching for file -// changes. This is used to prime the cache with the current list of -// containers and to start watching the devcontainer config files for +// SignalReady signals the API that we are ready to begin watching for +// file changes. This is used to prime the cache with the current list +// of containers and to start watching the devcontainer config files for // changes. It should be called after the agent ready. -func (api *API) Ready() { +func (api *API) SignalReady() { // Prime the cache with the current list of containers. _, _ = api.cl.List(api.ctx) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 2669dd2160987..45044b4e43e2e 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -634,7 +634,7 @@ func TestAPI(t *testing.T) { ) defer api.Close() - api.Ready() + api.SignalReady() r := chi.NewRouter() r.Mount("/", api.Routes()) From 1f413db08efe01164d6011101960b4dc0c500b3b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 29 Apr 2025 13:19:55 +0000 Subject: [PATCH 16/16] use safer atomic pointer for containerapi --- agent/agent.go | 10 +++++----- agent/api.go | 9 +++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 1c04ef698f02c..7525ecf051f69 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -90,8 +90,8 @@ type Options struct { BlockFileTransfer bool Execer agentexec.Execer - ContainerAPIOptions []agentcontainers.Option ExperimentalDevcontainersEnabled bool + ContainerAPIOptions []agentcontainers.Option // Enable ExperimentalDevcontainersEnabled for these to be effective. } type Client interface { @@ -190,8 +190,8 @@ func New(options Options) Agent { metrics: newAgentMetrics(prometheusRegistry), execer: options.Execer, - containerAPIOptions: options.ContainerAPIOptions, experimentalDevcontainersEnabled: options.ExperimentalDevcontainersEnabled, + containerAPIOptions: options.ContainerAPIOptions, } // Initially, we have a closed channel, reflecting the fact that we are not initially connected. // Each time we connect we replace the channel (while holding the closeMutex) with a new one @@ -274,7 +274,7 @@ type agent struct { experimentalDevcontainersEnabled bool containerAPIOptions []agentcontainers.Option - containerAPI *agentcontainers.API + containerAPI atomic.Pointer[agentcontainers.API] // Set by apiHandler. } func (a *agent) TailnetConn() *tailnet.Conn { @@ -1168,11 +1168,11 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, } a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur) a.scriptRunner.StartCron() - if a.containerAPI != nil { + if containerAPI := a.containerAPI.Load(); containerAPI != nil { // Inform the container API that the agent is ready. // This allows us to start watching for changes to // the devcontainer configuration files. - a.containerAPI.SignalReady() + containerAPI.SignalReady() } }) if err != nil { diff --git a/agent/api.go b/agent/api.go index 48cb351c1eac5..f09d39b172bd5 100644 --- a/agent/api.go +++ b/agent/api.go @@ -52,8 +52,9 @@ func (a *agent) apiHandler() (http.Handler, func() error) { // Append after to allow the agent options to override the default options. containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...) - a.containerAPI = agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) - r.Mount("/api/v0/containers", a.containerAPI.Routes()) + containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...) + r.Mount("/api/v0/containers", containerAPI.Routes()) + a.containerAPI.Store(containerAPI) } else { r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{ @@ -75,8 +76,8 @@ func (a *agent) apiHandler() (http.Handler, func() error) { r.Get("/debug/prometheus", promHandler.ServeHTTP) return r, func() error { - if a.containerAPI != nil { - return a.containerAPI.Close() + if containerAPI := a.containerAPI.Load(); containerAPI != nil { + return containerAPI.Close() } return nil }