Repository navigation
🤖 feat: delete unwanted CoderWorkspacePool members new before old, release used members, and delete unclaimed members when consent goes - #380
Conversation
…p the failure run across a rescale Change-Id: I06dd5f74f7c244bf1ce6b444438fee1f4fcea25e Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex security review |
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. |
🛡️ 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. |
![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: 775524e164
ℹ️ 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".
…ilure run on a version rename, and skip failures at an old timeout Change-Id: I669be22671a8a465f71f12ddfe4a5030676365ba Signed-off-by: Thomas Kosiewski <tk@coder.com>
775524e to
e903e46
Compare
|
@codex review |
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e903e46a9f
ℹ️ 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".
…, and check pool consent uncached whatever the demand Change-Id: I24972f715031adeae241dbf3b92f6e975046cc59 Signed-off-by: Thomas Kosiewski <tk@coder.com>
e903e46 to
dff1d29
Compare
|
@codex review |
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dff1d29421
ℹ️ 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".
…and drop the template from the failure-limit advice Change-Id: I74d2b9f228ea3f2a6f08307f1a565fa006609cbe Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ase used members, and delete unclaimed members when consent goes Change-Id: If2ba2072d7cfa7379495a67e3b8f0ea7e4524246 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…r cap already deleted Change-Id: I92f9b54bbc1453c1ace314193787b700f426cf26 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…l needs no create Change-Id: I5e6dc789d00252e41e256193698a62cc092e3696 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I12ec1a3bb4be216f5884862537716ae328764254 Signed-off-by: Thomas Kosiewski <tk@coder.com>
dff1d29 to
69a00fc
Compare
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
🛡️ 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. |
…elete-order Change-Id: Ia608ff1049f20cecb6c6d3dd135a6ebe8b76794c Signed-off-by: Thomas Kosiewski <tk@coder.com>
Summary
This PR adds the delete side to the dormant
CoderWorkspacePoolreconciler (plan 6.3 steps 4, 8, and 9). Nothing registers the reconciler yet, so no user sees a change.Used=True(someone connected without a claim) loses the pool's controller reference (decision D6). The release also sets the pool label again if it drifted, because the label is the only link left (Codex 4274801560). It then never counts as unclaimed, the pool never deletes it, and a pool deletion does not garbage-collect it. It keeps the pool label, sostatus.usedReplicasstill counts it. The update carries the listedresourceVersion.resourceVersion. So a member that its reconciler marked used after the pool's list answers 409 and stays, and the next reconcile releases it. Bound, used, failed, and deleting members are never deleted here.replicasminus the current ready members, and deletes the oldest ready first. So old members stay claimable until new ones are ready.spec.replicas: first members without a create request, then members that are not ready (newest first), then ready members (oldest first).spec.workspacePools, or the owner fails the owner rules (for example, it gains an admin role), the pool deletes every unclaimed member and waits withOwnerNotAllowed,TemplateNotAllowed, orOwnerNotEligible. Each unclaimed member holds the owner's token from its start build. Bound and used members stay."Ready" here means what the status counts use: phase
ReadyandReady=True. So a member withReady=False(an agent is not ready) counts as not ready for the delete order too.Refs #153
Background
This is PR 10b of the
CoderWorkspacePoolstack (#153). It stacks on PR 10a (#379). The plan row for PR 10 is ~450 lines. 10a and 10b together are over the 500-line limit per PR, so I split them.Validation
New tests (
coderworkspacepool_delete_test.go):TestPoolDeleteOrder: 5 current members (the member without a create request is the oldest), then the pool scales down one at a time from 5 to 1. The pool deletes, in this order: the member without a create request, the newer member that is not ready, the older member that is not ready, the ready member with the older ready time. The ready member with the newer ready time stays.TestPoolNewBeforeOld: 3 outdated members (two ready, one building) withreplicas: 2. The building one goes at once, and both ready ones stay while the pool creates two current members. One current member ready: the oldest outdated member goes. Both ready: the last outdated member goes, and the pool isReady.TestPoolReleasesUsedMember(plan mutation 5): a scale to 0 deletes the ready member, and releases the used member (no controller reference, pool label kept,usedReplicas: 1,replicas: 0). A member that becomes used between the list and the delete makes the delete answer 409, and the next reconcile releases it.TestPoolConsentLossDeletesUnclaimed: for a removed template, a removed owner, and an owner that gained theownerrole, the pool deletes the ready and the pending unclaimed members, keeps the used and the bound member, and reports the wait reason.Changed tests:
TestPoolOwnerCapUncachednow expects the unclaimed member to be deleted after the consent goes.TestPoolMemberDiscoverymakes the member with the drifted label ready first, so it stays as an outdated ready member while its replacement builds.TestPoolScaleDownAndLoweredCap:replicasfrom 3 to 1 andmaxWorkspacesto 2 in one reconcile, with finalizers on the members: the cap deletes one member, the scale-down one more, and no delete answers 409.Codex review round 1: 4274238377 (the cap and the scale-down deleted the same member, so the second delete answered 409) is fixed in
e903e46a:trimToOwnerCapmarks its deletes in the namespace list, and the scale-down reads that list. 4274238367 (a member that becomes used while its pool is deleted is garbage-collected before the pool releases it) is deferred to PR 12 and recorded in the plan. No work is lost in that race: the member's cleanup keeps the used workspace in Coder (plan 6.5 item 2).TestPoolConsentLossUncachedWhenFull: a Ready pool that needs no create, with a cache that still allows the template after the admin removed it: the pool deletes the unclaimed member and waits withTemplateNotAllowed(Codex 4274557698, fixed by the uncached read in 🤖 feat: hold CoderWorkspacePool members at a lowered owner cap, and keep the failure run across a rescale #379 and tested here; it failed on the previous head).Mutation checks: I removed one control at a time, and each removal made a named test fail (15 of 15): the used exclusion from the deletes (plan mutation 5), the release, the delete preconditions, the consent-removal deletes, the owner-refusal reason in that list, the bound exclusion, deleting outdated members that are not ready, keeping the newest outdated ready members, keeping only what current ready members do not cover, deleting members without a create request first, newest first among members that are not ready, oldest first among ready members, the namespace list for the scale-down, the cap's mark on its deletes, and the label repair on release.
Gates on the final commit: drift (codegen, manifests, installer, API reference: no diff),
make docs-check, dormant-CRD guard, docs lint and spelling,make verify-vendor,make build,make lint, actionlint,make test,make test-integration,make vuln, race. All passed.Size
370 hand-written lines (129 production, 241 test, counting removals). No generated files.
Risks
None for users. The reconciler is dormant. Recorded in the plan for PR 12: the consent-removal delete uses the cached control plane read of the resolve step. A cache that lags an admin who adds the consent back deletes warm members once more than needed. That is the fail-safe direction: no workspace leaks.
Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$154.26