feat(agent/agentcontainers): implement sub agent injection #18245
Conversation
d49f84e to
011a8aa
Compare
91ff08e to
3960774
Compare
011a8aa to
63f93bc
Compare
3960774 to
1cf1905
Compare
63f93bc to
0deaab8
Compare
1cf1905 to
f190036
Compare
0deaab8 to
8796ba3
Compare
dc146ab to
d1447f3
Compare
8796ba3 to
adbfd45
Compare
d1447f3 to
3547372
Compare
This change adds support for sub agent creation and injection into dev containers. Closes coder/internal#621
3547372 to
7358ee0
Compare
a8e4495 to
eb29bba
Compare
|
I'm still working on an integration test and the existing mocks are being a PITA (think those are about sorted now though). Promoting this to "ready for review" to get some feedback on the approach @DanielleMaywood @johnstcn. (Also going to break out the "follow-up PR" tasks into new issues before merging this.) |
| // Destination path inside the container, we store it in a fixed location | ||
| // under /.coder-agent/coder to avoid conflicts and avoid being shadowed | ||
| // by tmpfs or other mounts. This assumes the container root filesystem is | ||
| // read-write, which seems sensible for dev containers. | ||
| coderPathInsideContainer = "/.coder-agent/coder" |
| if err := api.cleanupSubAgents(api.ctx); err != nil { | ||
| api.logger.Error(api.ctx, "cleanup subagents failed", slog.Error(err)) | ||
| } else { | ||
| api.logger.Debug(api.ctx, "subagent cleanup complete") |
There was a problem hiding this comment.
nit: consistency
| api.logger.Debug(api.ctx, "subagent cleanup complete") | |
| api.logger.Debug(api.ctx, "cleanup subagents complete") |
| if _, injected := api.injectedSubAgentProcs[dc.Container.ID]; !injected && dc.Status == codersdk.WorkspaceAgentDevcontainerStatusRunning { | ||
| err := api.injectSubAgentIntoContainerLocked(ctx, dc) | ||
| if err != nil { | ||
| api.logger.Error(ctx, "inject subagent into container failed", slog.Error(err)) |
There was a problem hiding this comment.
suggest also logging container ID
| err := api.dccli.Exec(agentCtx, dc.WorkspaceFolder, dc.ConfigPath, agentPath, []string{"agent"}, | ||
| WithContainerID(container.ID), | ||
| WithRemoteEnv( | ||
| "CODER_AGENT_URL="+api.subAgentURL, | ||
| "CODER_AGENT_TOKEN="+agent.AuthToken.String(), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Would it make more sense to background this? If the parent agent ends up crashing and being restarted, we'll lose the sub-agents and have to re-inject them. We can keep track of the expected PID in e.g. /.coder-agent/pid
| if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "setcap", "cap_net_admin+ep", coderPathInsideContainer); err != nil { | ||
| logger.Warn(ctx, "set CAP_NET_ADMIN on agent binary failed", slog.Error(err)) | ||
| } |
There was a problem hiding this comment.
This will probably fail unless the container is running as privileged or has the specific CAP_NET_ADMIN privilege set on the container?
| // Make sure the agent binary is executable so we can run it. | ||
| if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "chmod", "+x", coderPathInsideContainer); err != nil { | ||
| return xerrors.Errorf("set agent binary executable: %w", err) | ||
| } |
There was a problem hiding this comment.
Do we also need to chown the binary so that it's readable by the default container user?
|
|
||
| logger.Info(ctx, "starting subagent in dev container") | ||
|
|
||
| err := api.dccli.Exec(agentCtx, dc.WorkspaceFolder, dc.ConfigPath, agentPath, []string{"agent"}, |
There was a problem hiding this comment.
Do we try to execute this as a non-root user?
| injected := make(map[uuid.UUID]bool, len(api.injectedSubAgentProcs)) | ||
| for _, proc := range api.injectedSubAgentProcs { | ||
| injected[proc.agent.ID] = true | ||
| } |
There was a problem hiding this comment.
This could probably be a map[uuid.UUID]struct{} instead, and then below on line 888 just check for _, found := injected[agent.ID]
| for _, agent := range agents { | ||
| if injected[agent.ID] { | ||
| continue | ||
| } | ||
| err := api.subAgentClient.Delete(ctx, agent.ID) | ||
| if err != nil { | ||
| api.logger.Error(ctx, "failed to delete agent", | ||
| slog.Error(err), | ||
| slog.F("agent_id", agent.ID), | ||
| slog.F("agent_name", agent.Name), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we set an upper bound on deletion attempts and raise if more than say 3 attempts fail?
This change adds support for sub agent creation and injection into dev
containers.
TODO:
devcontainer.jsonparsing, follow-up PR).customizations.coder.devcontainer.namefrom docker container label (materializeddevcontainer.jsonon creation, follow-up PR)Updates coder/internal#621