Skip to content

Commit eb6412a

Browse files
mafredrimafredri
authored
refactor(agent/agentcontainers): update routes and locking in container api (#17768)
This refactor updates the devcontainer routes and in-api locking for better clarity. Updates #16424
1 parent b6d72c8 commit eb6412a

File tree

2 files changed

+83
-57
lines changed

2 files changed

+83
-57
lines changed

‎agent/agentcontainers/api.go

+81-55
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,10 @@ func (api *API) Routes() http.Handler {
214214
r := chi.NewRouter()
215215

216216
r.Get("/", api.handleList)
217-
r.Get("/devcontainers", api.handleListDevcontainers)
218-
r.Post("/{id}/recreate", api.handleRecreate)
217+
r.Route("/devcontainers", func(r chi.Router) {
218+
r.Get("/", api.handleDevcontainersList)
219+
r.Post("/container/{container}/recreate", api.handleDevcontainerRecreate)
220+
})
219221

220222
return r
221223
}
@@ -376,12 +378,13 @@ func (api *API) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListC
376378
return copyListContainersResponse(api.containers), nil
377379
}
378380

379-
// handleRecreate handles the HTTP request to recreate a container.
380-
func (api *API) handleRecreate(w http.ResponseWriter, r *http.Request) {
381+
// handleDevcontainerRecreate handles the HTTP request to recreate a
382+
// devcontainer by referencing the container.
383+
func (api *API) handleDevcontainerRecreate(w http.ResponseWriter, r *http.Request) {
381384
ctx := r.Context()
382-
id := chi.URLParam(r, "id")
385+
containerID := chi.URLParam(r, "container")
383386

384-
if id == "" {
387+
if containerID == "" {
385388
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
386389
Message: "Missing container ID or name",
387390
Detail: "Container ID or name is required to recreate a devcontainer.",
@@ -399,7 +402,7 @@ func (api *API) handleRecreate(w http.ResponseWriter, r *http.Request) {
399402
}
400403

401404
containerIdx := slices.IndexFunc(containers.Containers, func(c codersdk.WorkspaceAgentContainer) bool {
402-
return c.Match(id)
405+
return c.Match(containerID)
403406
})
404407
if containerIdx == -1 {
405408
httpapi.Write(ctx, w, http.StatusNotFound, codersdk.Response{
@@ -418,7 +421,7 @@ func (api *API) handleRecreate(w http.ResponseWriter, r *http.Request) {
418421
if workspaceFolder == "" {
419422
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
420423
Message: "Missing workspace folder label",
421-
Detail: "The workspace folder label is required to recreate a devcontainer.",
424+
Detail: "The container is not a devcontainer, the container must have the workspace folder label to support recreation.",
422425
})
423426
return
424427
}
@@ -434,32 +437,28 @@ func (api *API) handleRecreate(w http.ResponseWriter, r *http.Request) {
434437

435438
// TODO(mafredri): Temporarily handle clearing the dirty state after
436439
// recreation, later on this should be handled by a "container watcher".
437-
select {
438-
case <-api.ctx.Done():
439-
return
440-
case <-ctx.Done():
441-
return
442-
case api.lockCh <- struct{}{}:
443-
defer func() { <-api.lockCh }()
444-
}
445-
for i := range api.knownDevcontainers {
446-
if api.knownDevcontainers[i].WorkspaceFolder == workspaceFolder {
447-
if api.knownDevcontainers[i].Dirty {
448-
api.logger.Info(ctx, "clearing dirty flag after recreation",
449-
slog.F("workspace_folder", workspaceFolder),
450-
slog.F("name", api.knownDevcontainers[i].Name),
451-
)
452-
api.knownDevcontainers[i].Dirty = false
440+
if !api.doLockedHandler(w, r, func() {
441+
for i := range api.knownDevcontainers {
442+
if api.knownDevcontainers[i].WorkspaceFolder == workspaceFolder {
443+
if api.knownDevcontainers[i].Dirty {
444+
api.logger.Info(ctx, "clearing dirty flag after recreation",
445+
slog.F("workspace_folder", workspaceFolder),
446+
slog.F("name", api.knownDevcontainers[i].Name),
447+
)
448+
api.knownDevcontainers[i].Dirty = false
449+
}
450+
return
453451
}
454-
break
455452
}
453+
}) {
454+
return
456455
}
457456

458457
w.WriteHeader(http.StatusNoContent)
459458
}
460459

461-
// handleListDevcontainers handles the HTTP request to list known devcontainers.
462-
func (api *API) handleListDevcontainers(w http.ResponseWriter, r *http.Request) {
460+
// handleDevcontainersList handles the HTTP request to list known devcontainers.
461+
func (api *API) handleDevcontainersList(w http.ResponseWriter, r *http.Request) {
463462
ctx := r.Context()
464463

465464
// Run getContainers to detect the latest devcontainers and their state.
@@ -472,15 +471,12 @@ func (api *API) handleListDevcontainers(w http.ResponseWriter, r *http.Request)
472471
return
473472
}
474473

475-
select {
476-
case <-api.ctx.Done():
474+
var devcontainers []codersdk.WorkspaceAgentDevcontainer
475+
if !api.doLockedHandler(w, r, func() {
476+
devcontainers = slices.Clone(api.knownDevcontainers)
477+
}) {
477478
return
478-
case <-ctx.Done():
479-
return
480-
case api.lockCh <- struct{}{}:
481479
}
482-
devcontainers := slices.Clone(api.knownDevcontainers)
483-
<-api.lockCh
484480

485481
slices.SortFunc(devcontainers, func(a, b codersdk.WorkspaceAgentDevcontainer) int {
486482
if cmp := strings.Compare(a.WorkspaceFolder, b.WorkspaceFolder); cmp != 0 {
@@ -499,34 +495,64 @@ func (api *API) handleListDevcontainers(w http.ResponseWriter, r *http.Request)
499495
// markDevcontainerDirty finds the devcontainer with the given config file path
500496
// and marks it as dirty. It acquires the lock before modifying the state.
501497
func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) {
498+
ok := api.doLocked(func() {
499+
// Record the timestamp of when this configuration file was modified.
500+
api.configFileModifiedTimes[configPath] = modifiedAt
501+
502+
for i := range api.knownDevcontainers {
503+
if api.knownDevcontainers[i].ConfigPath != configPath {
504+
continue
505+
}
506+
507+
// TODO(mafredri): Simplistic mark for now, we should check if the
508+
// container is running and if the config file was modified after
509+
// the container was created.
510+
if !api.knownDevcontainers[i].Dirty {
511+
api.logger.Info(api.ctx, "marking devcontainer as dirty",
512+
slog.F("file", configPath),
513+
slog.F("name", api.knownDevcontainers[i].Name),
514+
slog.F("workspace_folder", api.knownDevcontainers[i].WorkspaceFolder),
515+
slog.F("modified_at", modifiedAt),
516+
)
517+
api.knownDevcontainers[i].Dirty = true
518+
}
519+
}
520+
})
521+
if !ok {
522+
api.logger.Debug(api.ctx, "mark devcontainer dirty failed", slog.F("file", configPath))
523+
}
524+
}
525+
526+
func (api *API) doLockedHandler(w http.ResponseWriter, r *http.Request, f func()) bool {
502527
select {
528+
case <-r.Context().Done():
529+
httpapi.Write(r.Context(), w, http.StatusRequestTimeout, codersdk.Response{
530+
Message: "Request canceled",
531+
Detail: "Request was canceled before we could process it.",
532+
})
533+
return false
503534
case <-api.ctx.Done():
504-
return
535+
httpapi.Write(r.Context(), w, http.StatusServiceUnavailable, codersdk.Response{
536+
Message: "API closed",
537+
Detail: "The API is closed and cannot process requests.",
538+
})
539+
return false
505540
case api.lockCh <- struct{}{}:
506541
defer func() { <-api.lockCh }()
507542
}
543+
f()
544+
return true
545+
}
508546

509-
// Record the timestamp of when this configuration file was modified.
510-
api.configFileModifiedTimes[configPath] = modifiedAt
511-
512-
for i := range api.knownDevcontainers {
513-
if api.knownDevcontainers[i].ConfigPath != configPath {
514-
continue
515-
}
516-
517-
// TODO(mafredri): Simplistic mark for now, we should check if the
518-
// container is running and if the config file was modified after
519-
// the container was created.
520-
if !api.knownDevcontainers[i].Dirty {
521-
api.logger.Info(api.ctx, "marking devcontainer as dirty",
522-
slog.F("file", configPath),
523-
slog.F("name", api.knownDevcontainers[i].Name),
524-
slog.F("workspace_folder", api.knownDevcontainers[i].WorkspaceFolder),
525-
slog.F("modified_at", modifiedAt),
526-
)
527-
api.knownDevcontainers[i].Dirty = true
528-
}
547+
func (api *API) doLocked(f func()) bool {
548+
select {
549+
case <-api.ctx.Done():
550+
return false
551+
case api.lockCh <- struct{}{}:
552+
defer func() { <-api.lockCh }()
529553
}
554+
f()
555+
return true
530556
}
531557

532558
func (api *API) Close() error {

‎agent/agentcontainers/api_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ func TestAPI(t *testing.T) {
173173
wantBody string
174174
}{
175175
{
176-
name: "Missing ID",
176+
name: "Missing container ID",
177177
containerID: "",
178178
lister: &fakeLister{},
179179
devcontainerCLI: &fakeDevcontainerCLI{},
@@ -260,7 +260,7 @@ func TestAPI(t *testing.T) {
260260
r.Mount("/", api.Routes())
261261

262262
// Simulate HTTP request to the recreate endpoint.
263-
req := httptest.NewRequest(http.MethodPost, "/"+tt.containerID+"/recreate", nil)
263+
req := httptest.NewRequest(http.MethodPost, "/devcontainers/container/"+tt.containerID+"/recreate", nil)
264264
rec := httptest.NewRecorder()
265265
r.ServeHTTP(rec, req)
266266

0 commit comments

Comments
 (0)