Skip to content

Commit 7fa1ad8

Browse files
mafredrimafredri
authored
fix(agent/agentcontainers): reduce need to recreate sub agents (coder#18402)
1 parent 7e9a9e0 commit 7fa1ad8

File tree

5 files changed

+192
-99
lines changed

5 files changed

+192
-99
lines changed

‎agent/agent_test.go

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2080,6 +2080,10 @@ func TestAgent_DevcontainerAutostart(t *testing.T) {
20802080
subAgentConnected := make(chan subAgentRequestPayload, 1)
20812081
subAgentReady := make(chan struct{}, 1)
20822082
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2083+
if r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/api/v2/workspaceagents/me/") {
2084+
return
2085+
}
2086+
20832087
t.Logf("Sub-agent request received: %s %s", r.Method, r.URL.Path)
20842088

20852089
if r.Method != http.MethodPost {
@@ -2226,11 +2230,22 @@ func TestAgent_DevcontainerAutostart(t *testing.T) {
22262230
// Ensure the container update routine runs.
22272231
tickerFuncTrap.MustWait(ctx).MustRelease(ctx)
22282232
tickerFuncTrap.Close()
2229-
_, next := mClock.AdvanceNext()
2230-
next.MustWait(ctx)
22312233

2232-
// Verify that a subagent was created.
2233-
subAgents := agentClient.GetSubAgents()
2234+
// Since the agent does RefreshContainers, and the ticker function
2235+
// is set to skip instead of queue, we must advance the clock
2236+
// multiple times to ensure that the sub-agent is created.
2237+
var subAgents []*proto.SubAgent
2238+
for {
2239+
_, next := mClock.AdvanceNext()
2240+
next.MustWait(ctx)
2241+
2242+
// Verify that a subagent was created.
2243+
subAgents = agentClient.GetSubAgents()
2244+
if len(subAgents) > 0 {
2245+
t.Logf("Found sub-agents: %d", len(subAgents))
2246+
break
2247+
}
2248+
}
22342249
require.Len(t, subAgents, 1, "expected one sub agent")
22352250

22362251
subAgent := subAgents[0]

‎agent/agentcontainers/api.go

Lines changed: 58 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -671,9 +671,9 @@ func (api *API) getContainers() (codersdk.WorkspaceAgentListContainersResponse,
671671
if len(api.knownDevcontainers) > 0 {
672672
devcontainers = make([]codersdk.WorkspaceAgentDevcontainer, 0, len(api.knownDevcontainers))
673673
for _, dc := range api.knownDevcontainers {
674-
// Include the agent if it's been created (we're iterating over
674+
// Include the agent if it's running (we're iterating over
675675
// copies, so mutating is fine).
676-
if proc := api.injectedSubAgentProcs[dc.WorkspaceFolder]; proc.agent.ID != uuid.Nil && dc.Container != nil && proc.containerID == dc.Container.ID {
676+
if proc := api.injectedSubAgentProcs[dc.WorkspaceFolder]; proc.agent.ID != uuid.Nil {
677677
dc.Agent = &codersdk.WorkspaceAgentDevcontainerAgent{
678678
ID: proc.agent.ID,
679679
Name: proc.agent.Name,
@@ -977,7 +977,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
977977
)
978978

979979
// Check if subagent already exists for this devcontainer.
980-
recreateSubAgent := false
980+
maybeRecreateSubAgent := false
981981
proc, injected := api.injectedSubAgentProcs[dc.WorkspaceFolder]
982982
if injected {
983983
if proc.containerID == container.ID && proc.ctx.Err() == nil {
@@ -992,12 +992,15 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
992992
logger.Debug(ctx, "container ID changed, injecting subagent into new container",
993993
slog.F("old_container_id", proc.containerID),
994994
)
995-
recreateSubAgent = true
995+
maybeRecreateSubAgent = proc.agent.ID != uuid.Nil
996996
}
997997

998998
// Container ID changed or the subagent process is not running,
999999
// stop the existing subagent context to replace it.
10001000
proc.stop()
1001+
} else {
1002+
// Set SubAgent defaults.
1003+
proc.agent.OperatingSystem = "linux" // Assuming Linux for devcontainers.
10011004
}
10021005

10031006
// Prepare the subAgentProcess to be used when running the subagent.
@@ -1090,36 +1093,29 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
10901093
// logger.Warn(ctx, "set CAP_NET_ADMIN on agent binary failed", slog.Error(err))
10911094
// }
10921095

1093-
// Detect workspace folder by executing `pwd` in the container.
1094-
// NOTE(mafredri): This is a quick and dirty way to detect the
1095-
// workspace folder inside the container. In the future we will
1096-
// rely more on `devcontainer read-configuration`.
1097-
var pwdBuf bytes.Buffer
1098-
err = api.dccli.Exec(ctx, dc.WorkspaceFolder, dc.ConfigPath, "pwd", []string{},
1099-
WithExecOutput(&pwdBuf, io.Discard),
1100-
WithExecContainerID(container.ID),
1101-
)
1102-
if err != nil {
1103-
return xerrors.Errorf("check workspace folder in container: %w", err)
1104-
}
1105-
directory := strings.TrimSpace(pwdBuf.String())
1106-
if directory == "" {
1107-
logger.Warn(ctx, "detected workspace folder is empty, using default workspace folder",
1108-
slog.F("default_workspace_folder", DevcontainerDefaultContainerWorkspaceFolder),
1096+
subAgentConfig := proc.agent.CloneConfig(dc)
1097+
if proc.agent.ID == uuid.Nil || maybeRecreateSubAgent {
1098+
// Detect workspace folder by executing `pwd` in the container.
1099+
// NOTE(mafredri): This is a quick and dirty way to detect the
1100+
// workspace folder inside the container. In the future we will
1101+
// rely more on `devcontainer read-configuration`.
1102+
var pwdBuf bytes.Buffer
1103+
err = api.dccli.Exec(ctx, dc.WorkspaceFolder, dc.ConfigPath, "pwd", []string{},
1104+
WithExecOutput(&pwdBuf, io.Discard),
1105+
WithExecContainerID(container.ID),
11091106
)
1110-
directory = DevcontainerDefaultContainerWorkspaceFolder
1111-
}
1112-
1113-
if proc.agent.ID != uuid.Nil && recreateSubAgent {
1114-
logger.Debug(ctx, "deleting existing subagent for recreation", slog.F("agent_id", proc.agent.ID))
1115-
client := *api.subAgentClient.Load()
1116-
err = client.Delete(ctx, proc.agent.ID)
11171107
if err != nil {
1118-
return xerrors.Errorf("delete existing subagent failed: %w", err)
1108+
return xerrors.Errorf("check workspace folder in container: %w", err)
11191109
}
1120-
proc.agent = SubAgent{}
1121-
}
1122-
if proc.agent.ID == uuid.Nil {
1110+
directory := strings.TrimSpace(pwdBuf.String())
1111+
if directory == "" {
1112+
logger.Warn(ctx, "detected workspace folder is empty, using default workspace folder",
1113+
slog.F("default_workspace_folder", DevcontainerDefaultContainerWorkspaceFolder),
1114+
)
1115+
directory = DevcontainerDefaultContainerWorkspaceFolder
1116+
}
1117+
subAgentConfig.Directory = directory
1118+
11231119
displayAppsMap := map[codersdk.DisplayApp]bool{
11241120
// NOTE(DanielleMaywood):
11251121
// We use the same defaults here as set in terraform-provider-coder.
@@ -1138,6 +1134,13 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
11381134

11391135
for _, customization := range coderCustomization {
11401136
for app, enabled := range customization.DisplayApps {
1137+
if _, ok := displayAppsMap[app]; !ok {
1138+
logger.Warn(ctx, "unknown display app in devcontainer customization, ignoring",
1139+
slog.F("app", app),
1140+
slog.F("enabled", enabled),
1141+
)
1142+
continue
1143+
}
11411144
displayAppsMap[app] = enabled
11421145
}
11431146
}
@@ -1149,26 +1152,41 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
11491152
displayApps = append(displayApps, app)
11501153
}
11511154
}
1155+
slices.Sort(displayApps)
11521156

1157+
subAgentConfig.DisplayApps = displayApps
1158+
}
1159+
1160+
deleteSubAgent := proc.agent.ID != uuid.Nil && maybeRecreateSubAgent && !proc.agent.EqualConfig(subAgentConfig)
1161+
if deleteSubAgent {
1162+
logger.Debug(ctx, "deleting existing subagent for recreation", slog.F("agent_id", proc.agent.ID))
1163+
client := *api.subAgentClient.Load()
1164+
err = client.Delete(ctx, proc.agent.ID)
1165+
if err != nil {
1166+
return xerrors.Errorf("delete existing subagent failed: %w", err)
1167+
}
1168+
proc.agent = SubAgent{} // Clear agent to signal that we need to create a new one.
1169+
}
1170+
1171+
if proc.agent.ID == uuid.Nil {
11531172
logger.Debug(ctx, "creating new subagent",
1154-
slog.F("directory", directory),
1155-
slog.F("display_apps", displayApps),
1173+
slog.F("directory", subAgentConfig.Directory),
1174+
slog.F("display_apps", subAgentConfig.DisplayApps),
11561175
)
11571176

11581177
// Create new subagent record in the database to receive the auth token.
11591178
client := *api.subAgentClient.Load()
1160-
proc.agent, err = client.Create(ctx, SubAgent{
1161-
Name: dc.Name,
1162-
Directory: directory,
1163-
OperatingSystem: "linux", // Assuming Linux for devcontainers.
1164-
Architecture: arch,
1165-
DisplayApps: displayApps,
1166-
})
1179+
newSubAgent, err := client.Create(ctx, subAgentConfig)
11671180
if err != nil {
11681181
return xerrors.Errorf("create subagent failed: %w", err)
11691182
}
1183+
proc.agent = newSubAgent
11701184

11711185
logger.Info(ctx, "created new subagent", slog.F("agent_id", proc.agent.ID))
1186+
} else {
1187+
logger.Debug(ctx, "subagent already exists, skipping recreation",
1188+
slog.F("agent_id", proc.agent.ID),
1189+
)
11721190
}
11731191

11741192
api.mu.Lock() // Re-lock to update the agent.

0 commit comments

Comments
 (0)