Repository navigation
🤖 feat: pool Kind phases P6 to P8, offline stubs for every phase, and deleting a released used member deletes its workspace (#153 PR 13) - #445
Conversation
…ling) Change-Id: Icbcdea82803da826274cfa54cdf5bc75bb3bee1f Signed-off-by: Thomas Kosiewski <tk@coder.com>
…n deletes Change-Id: Id610170c60d395917e4d9aa2d948aaa901a9773f Signed-off-by: Thomas Kosiewski <tk@coder.com>
…eletes, dry run) Change-Id: I4f77797d3e5332837b0f0b46aa75a2d208816b54 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…and negative checks Change-Id: Ifdd690b257cab683e9305d2d7e719fd900c50719 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Iff29b554de2fcfbea163ac133d1c34e4137a5416 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Icbc53b9f2f0f9048984ea5fdac4c74e6c3e30bf5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I857c89ce10a28568122f86ff89e460fe4eb7d430 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I60abf0f28bb0fbb7c332d2d9f87faf6394ef7bf3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Dogfood on a P6 to P8 by hand. The usage request made the member dogfood-p6-p8.mp4After P6: 6 workspaces. The pool
After the scale to 1: the kept unclaimed, the bound, the used and the R1 workspace.
After the claim and pool deletes: only the used workspace and the R1 workspace stay.
After the used member delete: only the R1 workspace stays.
R1 (#444). Opening an unclaimed workspace's page as an admin moved its
The 3 members that the R1 reproductions released stayed in Coder until I deleted the members by hand. Each delete then deleted the workspace (decision B): dogfood-r1-leftovers.mp4 |
|
@codex security review |
|
CI evidence on
The pool step took 2 min 20 s, and its offline stub test (14 scenarios) took 22 s. The E2E job took 13 min 45 s. The last merge-queue E2E job on R1, idle half: passed. A ready member of a separate pool with a connected, ready agent kept its |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…y when a used member keeps its workspace Change-Id: Ia203d6cdeae60c621614d5509839dde10a3a95bb Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review New head |
|
@codex security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f6227d206
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…pace Change-Id: I107ca9ba227caa96bf5b5182eb9bfeab5361d693 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review New head |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Record for head
|
…ber (failing) Change-Id: I4a42997354c01978a509a53b96a09dc56b2973a8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ing deleted Change-Id: I3749d3adb9be216160caa0bd845f90180510fa21 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review New head |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Record for head
|
Summary
Plan PR 13 of #153. The Kind E2E job now runs
CoderWorkspacePoolphases P6 to P8: use detection, scale-down, deletes, and a server dry run.hack/e2e-workspace-pool_test.shnow covers every phase P1 to P8 offline. A cluster admin who deletes a used member that the pool released now deletes its Coder workspace too.Refs #153
Background
PR 12 (#441) activated the pool and ran Kind phases P1 to P5. Plan row 13 adds P6 to P8, offline stub tests for every phase, and the check of risk R1. #441 left one open discrepancy for this PR: a used member that a person deletes kept its workspace, but plan P8 expects the delete to remove it.
Decision: deleting a released used member deletes its workspace (option B)
The plan contradicts itself here:
This PR follows P8. The last-use check exists for deletes that cannot see the use: a scale-down that races a usage flush, and garbage collection after a pool delete. A used member that the pool released has no controller owner, so garbage collection never deletes it. Users have read-only RBAC on members (T4). Only a cluster admin can delete it, which is a deliberate act, like deleting a claim. A namespace deletion is not such a delete. The namespace controller deletes members and the
CoderControlPlanein no fixed order (an independent verifier showed that the member can come first). So before the delete build, a released used member reads its namespace without the cache, and if the namespace is being deleted it releases without a Coder call, as the control-plane-gone path does. A failed namespace read keeps the finalizer and returns the error. A used member that its pool still controls keeps today's rule, and so does a use that the member first sees at delete time.Implementation
coderworkspacepoolmember_cleanup.go: the cleanup runs no last-use check for a member that isUsed=Trueand has no controller owner reference. Its delete then works like a claim's delete, unless the member's namespace is being deleted. The operator reads the namespace without the cache and already hasgeton namespaces, so RBAC does not change.TestPoolMemberCleanupDeletesReleasedUsedMemberfailed before the fix (commit 916c67c) and passes after it. It also checks that two members keep their workspace: a used member that its pool still controls, and a member without a controller (for example after a pool delete with--cascade=orphan) whose use the delete finds first. Without theUsed=Trueterm in the new rule, the second case fails.TestPoolMemberCleanupKeepsReleasedWorkspaceInDeletedNamespacedeletes the namespace with the member first and with the control plane first, and checks that a failed namespace read keeps the finalizer. It failed before the namespace check (commit d6c700a: member first and read failure red, control plane first green).usedReplicasfield doc and the how-to now say this plainly. Deleting a released used member deletes its workspace in Coder. To keep the work, copy it out first. A used member that still lists the pool as its controller is not released yet, and deleting it keeps the workspace. Deleting the namespace keeps a used member's workspace. I found no other way to keep it that the code supports: a removed finalizer comes back while the member is not being deleted.hack/e2e-workspace-pool.sh(plan 10.4):POST /api/v2/workspaces/{id}/usagewith an empty body for one unclaimed member. Within 3 minutes the member must beUsed=Trueand released, the pool must build back to 4 ready unclaimed members, andusedReplicasmust be 1.include_deletedread must show a succeeded delete build for each of the 3 extra unclaimed workspaces. The kept, the bound and the used workspace must stay live.kubectl apply --dry-run=serverof the pool must create no pool, no member and no workspace within 10 s.spec.workspacePools, because P8 already deleted every object.hack/e2e-workspace-pool_test.sh: kubectl and curl stubs over one fake cluster and Coder, with a fake pool controller that runs one pass per stub call. It has 16 scenarios:--replicas=101, a bind that builds, a duplicate create after the restart, an undetected use, a false use, a scale-down that keeps workspaces, a claim delete and a pool delete that keep their workspaces, a pool delete that takes the used member, a used-member delete that keeps the workspace, and a dry run that creates the pool.::add-mask::line, in argv, or in a file left in the work dir. It also fails when the port-forward keeps running.Validation
Local gates passed:
make verify-vendor,make test,make build,make lint,make test-integration,make docs-check(mkdocs strict throughnix develop), markdownlint, cspell, actionlint, andmake test-scripts(642 s, every lifecycle scenario and all pool scenarios).Stub harness controls (scratch copies of the driver):
main(P1 to P5 only) fails the pass scenario ("phases P1 to P8 did not all run") and all 6 controls for P6 to P8: 5 report "driver passed", and 1 fails with another message.The verifier's 13-case delete matrix (
TestVerifierDeleteControls, run through a Go overlay at the new head) passes: only the hand delete of a released used member sends a delete build. Namespace deletion in both orders, control plane deletion, scale-down, pool deletion and orphan use send none.Kind (cluster from
hack/kind-dev.sh, kindest/node v1.32.0, Coder v2.37.2, a CI bootstrap replica with steps 7 to 25, operator built from this branch): the driver passed P1 to P8 on the first try.Used=True62 s after the usage request)P6 to P8 add about 2 minutes against PR 12 (R6), far below the 10-minute limit of decision D8. The CI job time is in a comment below.
Dogfood: a terminal recording and Coder UI screenshots follow in a comment.
Risks
R1 checked: it fails for the workspace page, tracked in CoderWorkspacePool: a dashboard page view marks an unclaimed member Used #444. Measured on Kind with Coder v2.37.2:
last_used_at. The member turnsUsed=Trueat the next usage flush, and the pool releases it and builds a replacement. I reproduced it twice with agent-browser.GET /api/v2/workspaceagents/{id}/containerswithout a browser also moved it. The page sends that request.last_used_atfor 2.5 minutes, and the workspaces list page did not move it.The result is no data loss. A released workspace costs one running workspace until an admin deletes its member, and with this PR that delete removes it in Coder. The how-to now warns about the dashboard page. The 30-minute idle half of the R1 check is in a comment below.
A person who deletes a used member before the pool released it still keeps its workspace. The window is short, because the pool releases it at its next reconcile.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh