- Notifications
You must be signed in to change notification settings - Fork920
fix: wsbuilder: complete job and mark workspace deleted if no provisioners available#18465
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
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.
Pull Request Overview
This PR ensures that when an orphan-delete request finds no active provisioners, the builder marks the provisioner job as completed and the workspace as deleted. It also adds the UI hook for orphan deletion, updates in-memory DB behavior, and expands tests to cover the new orphan scenarios.
- Add
canOrphanflag and checkbox in the workspace delete dialog - Implement orphan-delete fallback in wsbuilder (complete job + delete workspace)
- Expand unit and integration tests to cover orphan-delete paths
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx | ExtractcanOrphan and render orphan checkbox with updated text |
| coderd/wsbuilder/wsbuilder.go | Orphan-delete logic: complete provisioner job and mark workspace deleted when no provisioners available |
| coderd/wsbuilder/wsbuilder_test.go | New tests for orphan-delete with and without available provisioners |
| coderd/workspacebuilds.go | Useqpr variable to pass provisioner job info into conversion |
| coderd/workspacebuilds_test.go | Importdbfake and add subtests for orphan build errors and permissions |
| coderd/database/dbmem/dbmem.go | Return empty slice instead ofsql.ErrNoRows for no provisioner daemons |
| cli/delete_test.go | Update CLI delete test to cover--orphan behavior and status codes |
Comments suppressed due to low confidence (2)
site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx:53
- [nitpick] Add frontend tests to verify that the "Orphan Resources" checkbox appears only when
canOrphanis true and is hidden otherwise.
const canOrphan =coderd/wsbuilder/wsbuilder.go:495
- Implement or remove the TODO for audit baggage so that delete transitions are properly logged in the audit system.
// TODO: audit baggage?site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/workspaces/WorkspaceMoreActions/WorkspaceDeleteDialog.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| client:=coderdtest.New(t,&coderdtest.Options{IncludeProvisionerDaemon:true}) | ||
| first:=coderdtest.CreateFirstUser(t,client) | ||
| ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong) | ||
| defercancel() |
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.
review: refactored to use dbfake instead
| _,err=client.Workspace(ctx,workspace.ID) | ||
| require.Error(t,err) | ||
| require.Equal(t,http.StatusGone,coderdtest.SDKError(t,err).StatusCode()) |
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.
review: moved this check to the relevant CLI test
Uh oh!
There was an error while loading.Please reload this page.
Alternate fix for#18080
Modifies wsbuilder to complete the provisioner job and mark the workspace as deleted if it is clear that no provisioner will be able to pick up the delete build.
This has a significant advantage of not deviating too much from the current semantics of
POST /api/v2/workspacebuilds.#18460 ends up returning a 204 on orphan delete due to no build being created.Downside is that we have to duplicate some responsibilities of provisionerdserver in wsbuilder.
There is a slight gotcha to this approach though: if you stop a provisioner and then immediately try to orphan-delete, the job will still be created because of the provisioner heartbeat interval. However you can cancel it and try again.