- Notifications
You must be signed in to change notification settings - Fork905
feat(agent/agentcontainers): implement sub agent injection#18245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d49f84e to011a8aaCompare91ff08e to3960774Compare011a8aa to63f93bcCompare3960774 to1cf1905Compare63f93bc to0deaab8Compare1cf1905 tof190036Compare0deaab8 to8796ba3Comparedc146ab tod1447f3Compare8796ba3 toadbfd45Compared1447f3 to3547372CompareThis change adds support for sub agent creation and injection into devcontainers.Closescoder/internal#621
3547372 to7358ee0Comparea8e4495 toeb29bbaCompareI'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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I still have to read some more but adding my comments so far.
| // 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: consistency
| api.logger.Debug(api.ctx,"subagentcleanup 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we also need tochown 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This could probably be amap[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we set an upper bound on deletion attempts and raise if more than say 3 attempts fail?
Uh oh!
There was an error while loading.Please reload this page.
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)Updatescoder/internal#621