- Notifications
You must be signed in to change notification settings - Fork1.1k
feat(agent): support deleting dev containers#21247
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?
Conversation
49794cc to4504755Compare4504755 to0fcab3fCompare
mafredri left a comment
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Believe it or not, I think I wrote that 😂
| } | ||
| // Similarly, if already stopping, don't allow another delete. | ||
| ifdc.Status==codersdk.WorkspaceAgentDevcontainerStatusStopping { |
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.
Could we make this statedeleting instead? I would like to keepstopping 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Error paths seem to be lacking state handling for the devcontainer. Should probably updatedc.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Check: Will this leave the devcontainer available in the dashboard tostart, 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. | ||
| ifsubAgentID!=uuid.Nil { |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| } | ||
| } | ||
| ifdc.ID==uuid.Nil { | ||
| dc,err:=api.devcontainerByIDLocked(devcontainerID) |
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.
Nice 👍🏻
| returnnil,f.execErr | ||
| } | ||
| func (f*fakeContainerCLI)Stop(ctx context.Context,namestring)error { |
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.
Since we're introducing mutation, we'll need to add a mutex to these.
Uh oh!
There was an error while loading.Please reload this page.
Relates to#19062
Add logic to the agent to allow deleting Dev Containers
PR Stack: