Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit0f3a1e9

Browse files
authored
fix(agent/agentcontainers): split Init into Init and Start for early API responses (#18640)
Previously in#18635 we delayed the containers API `Init` to avoid producingerrors due to Docker and `@devcontainers/cli` not yet being installed by startupscripts. This had an adverse effect on the UX via UI responsiveness as thedetection of devcontainers was greatly delayed.This change splits `Init` into `Init` and `Start` so that we can immediatelyafter `Init` start serving known devcontainers (defined in Terraform), improvingthe UX.Related#18635Related#18640
1 parente46d892 commit0f3a1e9

File tree

3 files changed

+66
-36
lines changed

3 files changed

+66
-36
lines changed

‎agent/agent.go

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,11 +1158,26 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
11581158
}
11591159
}
11601160

1161-
scripts:=manifest.Scripts
1161+
var (
1162+
scripts=manifest.Scripts
1163+
devcontainerScriptsmap[uuid.UUID]codersdk.WorkspaceAgentScript
1164+
)
11621165
ifa.containerAPI!=nil {
1166+
// Init the container API with the manifest and client so that
1167+
// we can start accepting requests. The final start of the API
1168+
// happens after the startup scripts have been executed to
1169+
// ensure the presence of required tools. This means we can
1170+
// return existing devcontainers but actual container detection
1171+
// and creation will be deferred.
1172+
a.containerAPI.Init(
1173+
agentcontainers.WithManifestInfo(manifest.OwnerName,manifest.WorkspaceName,manifest.AgentName),
1174+
agentcontainers.WithDevcontainers(manifest.Devcontainers,manifest.Scripts),
1175+
agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger,aAPI)),
1176+
)
1177+
11631178
// Since devcontainer are enabled, remove devcontainer scripts
11641179
// from the main scripts list to avoid showing an error.
1165-
scripts,_=agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers,manifest.Scripts)
1180+
scripts,devcontainerScripts=agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers,scripts)
11661181
}
11671182
err=a.scriptRunner.Init(scripts,aAPI.ScriptCompleted)
11681183
iferr!=nil {
@@ -1183,13 +1198,10 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
11831198
err:=a.scriptRunner.Execute(a.gracefulCtx,agentscripts.ExecuteStartScripts)
11841199

11851200
ifa.containerAPI!=nil {
1186-
a.containerAPI.Init(
1187-
agentcontainers.WithManifestInfo(manifest.OwnerName,manifest.WorkspaceName,manifest.AgentName),
1188-
agentcontainers.WithDevcontainers(manifest.Devcontainers,manifest.Scripts),
1189-
agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger,aAPI)),
1190-
)
1191-
1192-
_,devcontainerScripts:=agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers,manifest.Scripts)
1201+
// Start the container API after the startup scripts have
1202+
// been executed to ensure that the required tools can be
1203+
// installed.
1204+
a.containerAPI.Start()
11931205
for_,dc:=rangemanifest.Devcontainers {
11941206
cErr:=a.createDevcontainer(ctx,aAPI,dc,devcontainerScripts[dc.ID])
11951207
err=errors.Join(err,cErr)

‎agent/agentcontainers/api.go

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ type API struct {
5353
cancel context.CancelFunc
5454
watcherDonechanstruct{}
5555
updaterDonechanstruct{}
56-
initialUpdateDonechanstruct{}// Closed after first update in updaterLoop.
5756
updateTriggerchanchanerror// Channel to trigger manual refresh.
5857
updateInterval time.Duration// Interval for periodic container updates.
5958
logger slog.Logger
@@ -73,7 +72,8 @@ type API struct {
7372
workspaceNamestring
7473
parentAgentstring
7574

76-
mu sync.RWMutex
75+
mu sync.RWMutex// Protects the following fields.
76+
initDonechanstruct{}// Closed by Init.
7777
closedbool
7878
containers codersdk.WorkspaceAgentListContainersResponse// Output from the last list operation.
7979
containersErrerror// Error from the last list operation.
@@ -270,7 +270,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
270270
api:=&API{
271271
ctx:ctx,
272272
cancel:cancel,
273-
initialUpdateDone:make(chanstruct{}),
273+
initDone:make(chanstruct{}),
274274
updateTrigger:make(chanchanerror),
275275
updateInterval:defaultUpdateInterval,
276276
logger:logger,
@@ -322,18 +322,37 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
322322
}
323323

324324
// Init applies a final set of options to the API and then
325-
// begins the watcherLoop and updaterLoop. This function
326-
// must only be called once.
325+
// closes initDone. This method can only be called once.
327326
func (api*API)Init(opts...Option) {
328327
api.mu.Lock()
329328
deferapi.mu.Unlock()
330329
ifapi.closed {
331330
return
332331
}
332+
select {
333+
case<-api.initDone:
334+
return
335+
default:
336+
}
337+
deferclose(api.initDone)
333338

334339
for_,opt:=rangeopts {
335340
opt(api)
336341
}
342+
}
343+
344+
// Start starts the API by initializing the watcher and updater loops.
345+
// This method calls Init, if it is desired to apply options after
346+
// the API has been created, it should be done by calling Init before
347+
// Start. This method must only be called once.
348+
func (api*API)Start() {
349+
api.Init()
350+
351+
api.mu.Lock()
352+
deferapi.mu.Unlock()
353+
ifapi.closed {
354+
return
355+
}
337356

338357
api.watcherDone=make(chanstruct{})
339358
api.updaterDone=make(chanstruct{})
@@ -412,9 +431,6 @@ func (api *API) updaterLoop() {
412431
}else {
413432
api.logger.Debug(api.ctx,"initial containers update complete")
414433
}
415-
// Signal that the initial update attempt (successful or not) is done.
416-
// Other services can wait on this if they need the first data to be available.
417-
close(api.initialUpdateDone)
418434

419435
// We utilize a TickerFunc here instead of a regular Ticker so that
420436
// we can guarantee execution of the updateContainers method after
@@ -474,7 +490,7 @@ func (api *API) UpdateSubAgentClient(client SubAgentClient) {
474490
func (api*API)Routes() http.Handler {
475491
r:=chi.NewRouter()
476492

477-
ensureInitialUpdateDoneMW:=func(next http.Handler) http.Handler {
493+
ensureInitDoneMW:=func(next http.Handler) http.Handler {
478494
returnhttp.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
479495
select {
480496
case<-api.ctx.Done():
@@ -485,9 +501,8 @@ func (api *API) Routes() http.Handler {
485501
return
486502
case<-r.Context().Done():
487503
return
488-
case<-api.initialUpdateDone:
489-
// Initial update is done, we can start processing
490-
// requests.
504+
case<-api.initDone:
505+
// API init is done, we can start processing requests.
491506
}
492507
next.ServeHTTP(rw,r)
493508
})
@@ -496,7 +511,7 @@ func (api *API) Routes() http.Handler {
496511
// For now, all endpoints require the initial update to be done.
497512
// If we want to allow some endpoints to be available before
498513
// the initial update, we can enable this per-route.
499-
r.Use(ensureInitialUpdateDoneMW)
514+
r.Use(ensureInitDoneMW)
500515

501516
r.Get("/",api.handleList)
502517
// TODO(mafredri): Simplify this route as the previous /devcontainers

‎agent/agentcontainers/api_test.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,7 @@ func TestAPI(t *testing.T) {
437437
agentcontainers.WithContainerCLI(mLister),
438438
agentcontainers.WithContainerLabelIncludeFilter("this.label.does.not.exist.ignore.devcontainers","true"),
439439
)
440-
api.Init()
440+
api.Start()
441441
deferapi.Close()
442442
r.Mount("/",api.Routes())
443443

@@ -627,7 +627,7 @@ func TestAPI(t *testing.T) {
627627
agentcontainers.WithDevcontainers(tt.setupDevcontainers,nil),
628628
)
629629

630-
api.Init()
630+
api.Start()
631631
deferapi.Close()
632632
r.Mount("/",api.Routes())
633633

@@ -1068,7 +1068,7 @@ func TestAPI(t *testing.T) {
10681068
}
10691069

10701070
api:=agentcontainers.NewAPI(logger,apiOptions...)
1071-
api.Init()
1071+
api.Start()
10721072
deferapi.Close()
10731073

10741074
r.Mount("/",api.Routes())
@@ -1158,7 +1158,7 @@ func TestAPI(t *testing.T) {
11581158
[]codersdk.WorkspaceAgentScript{{LogSourceID:uuid.New(),ID:dc.ID}},
11591159
),
11601160
)
1161-
api.Init()
1161+
api.Start()
11621162
deferapi.Close()
11631163

11641164
// Make sure the ticker function has been registered
@@ -1254,7 +1254,7 @@ func TestAPI(t *testing.T) {
12541254
agentcontainers.WithWatcher(fWatcher),
12551255
agentcontainers.WithClock(mClock),
12561256
)
1257-
api.Init()
1257+
api.Start()
12581258
deferapi.Close()
12591259

12601260
r:=chi.NewRouter()
@@ -1408,7 +1408,7 @@ func TestAPI(t *testing.T) {
14081408
agentcontainers.WithDevcontainerCLI(fakeDCCLI),
14091409
agentcontainers.WithManifestInfo("test-user","test-workspace","test-parent-agent"),
14101410
)
1411-
api.Init()
1411+
api.Start()
14121412
apiClose:=func() {
14131413
closeOnce.Do(func() {
14141414
// Close before api.Close() defer to avoid deadlock after test.
@@ -1635,7 +1635,7 @@ func TestAPI(t *testing.T) {
16351635
agentcontainers.WithSubAgentClient(fakeSAC),
16361636
agentcontainers.WithDevcontainerCLI(&fakeDevcontainerCLI{}),
16371637
)
1638-
api.Init()
1638+
api.Start()
16391639
deferapi.Close()
16401640

16411641
tickerTrap.MustWait(ctx).MustRelease(ctx)
@@ -1958,7 +1958,7 @@ func TestAPI(t *testing.T) {
19581958
agentcontainers.WithSubAgentURL("test-subagent-url"),
19591959
agentcontainers.WithWatcher(watcher.NewNoop()),
19601960
)
1961-
api.Init()
1961+
api.Start()
19621962
deferapi.Close()
19631963

19641964
// Close before api.Close() defer to avoid deadlock after test.
@@ -2052,7 +2052,7 @@ func TestAPI(t *testing.T) {
20522052
agentcontainers.WithSubAgentURL("test-subagent-url"),
20532053
agentcontainers.WithWatcher(watcher.NewNoop()),
20542054
)
2055-
api.Init()
2055+
api.Start()
20562056
deferapi.Close()
20572057

20582058
// Close before api.Close() defer to avoid deadlock after test.
@@ -2158,7 +2158,7 @@ func TestAPI(t *testing.T) {
21582158
agentcontainers.WithWatcher(watcher.NewNoop()),
21592159
agentcontainers.WithManifestInfo("test-user","test-workspace","test-parent-agent"),
21602160
)
2161-
api.Init()
2161+
api.Start()
21622162
deferapi.Close()
21632163

21642164
// Close before api.Close() defer to avoid deadlock after test.
@@ -2228,7 +2228,7 @@ func TestAPI(t *testing.T) {
22282228
agentcontainers.WithExecer(fakeExec),
22292229
agentcontainers.WithCommandEnv(commandEnv),
22302230
)
2231-
api.Init()
2231+
api.Start()
22322232
deferapi.Close()
22332233

22342234
// Call RefreshContainers directly to trigger CommandEnv usage.
@@ -2318,13 +2318,16 @@ func TestAPI(t *testing.T) {
23182318
agentcontainers.WithWatcher(fWatcher),
23192319
agentcontainers.WithClock(mClock),
23202320
)
2321-
api.Init()
2321+
api.Start()
23222322
deferfunc() {
23232323
close(fakeSAC.createErrC)
23242324
close(fakeSAC.deleteErrC)
23252325
api.Close()
23262326
}()
23272327

2328+
err:=api.RefreshContainers(ctx)
2329+
require.NoError(t,err,"RefreshContainers should not error")
2330+
23282331
r:=chi.NewRouter()
23292332
r.Mount("/",api.Routes())
23302333

@@ -2335,7 +2338,7 @@ func TestAPI(t *testing.T) {
23352338
require.Equal(t,http.StatusOK,rec.Code)
23362339

23372340
varresponse codersdk.WorkspaceAgentListContainersResponse
2338-
err:=json.NewDecoder(rec.Body).Decode(&response)
2341+
err=json.NewDecoder(rec.Body).Decode(&response)
23392342
require.NoError(t,err)
23402343

23412344
assert.Empty(t,response.Devcontainers,"ignored devcontainer should not be in response when ignore=true")
@@ -2519,7 +2522,7 @@ func TestSubAgentCreationWithNameRetry(t *testing.T) {
25192522
agentcontainers.WithSubAgentClient(fSAC),
25202523
agentcontainers.WithWatcher(watcher.NewNoop()),
25212524
)
2522-
api.Init()
2525+
api.Start()
25232526
deferapi.Close()
25242527

25252528
tickerTrap.MustWait(ctx).MustRelease(ctx)

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp