- Notifications
You must be signed in to change notification settings - Fork921
fix(coderd): mark sub agent deletion via boolean instead of delete#18411
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
db5700a to7d73585Comparecoderd/database/dbgen/dbgen.go Outdated
| // Add a test antagonist. For every agent we add a deleted sub agent | ||
| // to discover cases where deletion should be handled. | ||
| subAgt, err := db.InsertWorkspaceAgent(genCtx, database.InsertWorkspaceAgentParams{ |
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: This may be a bit unusual, but it already helped me catch an issue with theworkspace_prebuilds VIEW.
Ideally this potential cause would somehow be propagated to the user in case of test failure, but not sure how that could be done. I'll add a log entry to help with discovery.
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 like this a lot! I do agree that it could get confusing to troubleshoot, but the log message in combination withdbtestutil.DumpOnFailure should help. You could also potentially set some relevant fields of the sub-agent so that its purpose is clear.
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.
How about this? 😆bc370a8 (#18411)
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.
SATISFACTORY
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 spot-checked the migrations and the updated views/triggers look OK to me.
Would like a second pair of eyes on this for approval in case there's something I missed.
coderd/database/dbgen/dbgen.go Outdated
| // Add a test antagonist. For every agent we add a deleted sub agent | ||
| // to discover cases where deletion should be handled. | ||
| subAgt, err := db.InsertWorkspaceAgent(genCtx, database.InsertWorkspaceAgentParams{ |
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 like this a lot! I do agree that it could get confusing to troubleshoot, but the log message in combination withdbtestutil.DumpOnFailure should help. You could also potentially set some relevant fields of the sub-agent so that its purpose is clear.
| UPDATE | ||
| workspace_agents | ||
| SET | ||
| deleted = TRUE | ||
| WHERE | ||
| id = $1 | ||
| AND parent_id IS NOT NULL | ||
| AND deleted = FALSE; |
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.
👍 literally impossible to delete a 'top-level' 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.
obligatory reminder to check migration number!
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.
LGTM but awaiting approval from someone on prebuilds
c26d6ab to732eb5fCompare732eb5f to6121231Compare511fd09 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Since deletion of data is uncommon in our database, the introduction of sub agents and their deletion introduced issues with foreign key assumptions incoder/internal#685. We could have only addressed the specific case by allowing cascade deletion of stats as well as handling in the stats collector, but it's unclear how many more such edge-cases we would've run into.
For this reason, we decided to mark the rows as deleted via boolean instead, and filter them out in all queries that seem relevant so that coderd doesn't see the deleted agents.
Testing is covered by updating the relevant
agentapitests as well as adding an antagonist deleted agent todbgenwhich automatically gets applied to all tests.Fixescoder/internal#685
Note that there are some queries that read
workspace_agentsthat were not updated with deletion handling, but each one was analyzed and deemed unnecessary because of how they are used, doing so may introduce more harm than good. On that note, many of these queries can return results for non-latest builds which essentially means, "deleted" agents.