feat(agent): support deleting dev containers #21247
Conversation
49794cc to
4504755
Compare
4504755 to
0fcab3f
Compare
mafredri
left a comment
There was a problem hiding this comment.
I did a quick review, didn't really dig into the tests yet.
| return | ||
| } | ||
|
|
||
| // Check if the devcontainer is currently starting - if so, we can't delete it. |
There was a problem hiding this comment.
This should be rewritten and explain why we've decided to have this limitation. Technically there's nothing preventing us from implementing cancellation of create, etc.
PS. I see you -.
There was a problem hiding this comment.
Believe it or not, I think I wrote that 😂
| } | ||
|
|
||
| // Similarly, if already stopping, don't allow another delete. | ||
| if dc.Status == codersdk.WorkspaceAgentDevcontainerStatusStopping { |
There was a problem hiding this comment.
Could we make this state deleting instead? I would like to keep stopping as a separate state for the future, one that does not mean delete (i.e. start/stop/delete for the full suite).
Logically a delete could be a stop followed by a delete. I think both are valid use-cases (a user may want to just quickly restart the container to reset the processes, deletion will take potentially much longer).
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Error paths seem to be lacking state handling for the devcontainer. Should probably update dc.Error, perhaps others?
| delete(api.recreateSuccessTimes, workspaceFolder) | ||
| delete(api.recreateErrorTimes, workspaceFolder) | ||
| delete(api.usingWorkspaceFolderName, workspaceFolder) | ||
| delete(api.injectedSubAgentProcs, workspaceFolder) |
There was a problem hiding this comment.
Check: Will this leave the devcontainer available in the dashboard to start, like we do for e.g. detected containers?
Just because the user deleted it, doesn't necessarily mean they won't want to start it again.
| } | ||
|
|
||
| // Delete the subagent if it exists. | ||
| if subAgentID != uuid.Nil { |
There was a problem hiding this comment.
Should we run this cleanup even if stop/remove fails?
| httpapi.Write(ctx, w, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "An internal error occurred deleting the agent", | ||
| Detail: err.Error(), | ||
| }) |
There was a problem hiding this comment.
These failures leave the API in a weird state, we've entered a terminal state (stopping), and can't re-invoke delete even if the errors are ephemeral. The container will become immortal.
| } | ||
| } | ||
| if dc.ID == uuid.Nil { | ||
| dc, err := api.devcontainerByIDLocked(devcontainerID) |
| return nil, f.execErr | ||
| } | ||
|
|
||
| func (f *fakeContainerCLI) Stop(ctx context.Context, name string) error { |
There was a problem hiding this comment.
Since we're introducing mutation, we'll need to add a mutex to these.
Relates to #19062
Add logic to the agent to allow deleting Dev Containers
PR Stack: