Repository navigation
🤖 feat: create CoderWorkspacePool members on demand for waiting claims, keep claims of a missing pool, and mark a claim Lost when its workspace is deleted - #388
Conversation
…nd write the claim status Change-Id: Ia47cdccaaad51b9a7cfa069a5e5de45de22be757 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: 18ab922fb4
ℹ️ 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".
…pool, repair the pool label, and store the owner name before the bind Change-Id: Id61727ee0e9898f2def86bd569dbc814c6b44fa4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
18ab922 to
23e6281
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: 23e6281d96
ℹ️ 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".
…eplaced claim, and check the holder again before Lost Change-Id: I536b0015fc519c2ffd84a81fb4b055c5f0a0b871 Signed-off-by: Thomas Kosiewski <tk@coder.com>
23e6281 to
e6eb14f
Compare
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Nice work! 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. |
Change-Id: I06dd83d03cfd1e061e4d3a03940ac6e5a2be9934 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… keep claims of a missing pool, and mark a claim Lost when its workspace is deleted Change-Id: I8d928c5ca1c5e31df6c36afa8e0be93a80e21196 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…claim on a stale cache Change-Id: I25dde40a2810039dbba49609db691c95cbda2a0f Signed-off-by: Thomas Kosiewski <tk@coder.com>
…iting Change-Id: I16d396849262720a8302f0f2acfb7505e7808013 Signed-off-by: Thomas Kosiewski <tk@coder.com>
e6eb14f to
380bae8
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. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
…laim-lifecycle Change-Id: I9df386f9465b999df9f6fcb0860f45ec4fa8db69 Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 participant
Summary
This PR finishes the claim side of the dormant
CoderWorkspacePoolreconcilers (plan 6.2 step 11 rule 3, 6.3 steps 7 and 10, decision D4). Nothing registers the reconcilers yet, so no user sees a change.spec.replicasplus the waiting claims, for both creates and scale-down deletes. A waiting claim is one that no member holds, that is not deleting, and that is notLost. So a pool atreplicas: 0creates one member per waiting claim, binds it once it is ready, and then wants nothing more.PoolNotFound. A reconcile for a pool that is gone or being deleted writes the status of its claims. A claim that a member holds stays as its member says, so a bound claim staysBoundafter its pool is deleted: the claim, not the pool, controls the bound member. The other claims wait withPendingPoolNotFound. The same holds for a claim on a pool that never existed.Lost). After the one extension at bind, a bound member keeps reading its workspace every 60 s, with the identity checks. Builds by the claimant, a stop included, change nothing. Once Coder answers 410 (the workspace is deleted), the member fails withWorkspaceDeleted=True, and its claim becomesLost(Stalled=True, reasonWorkspaceDeleted).Lostis final: the claim never binds again, also after its failed member is gone.Refs #153
Background
This is PR 11b of the
CoderWorkspacePoolstack (#153). It stacks on PR 11a (#387).Before this PR, a bound member stopped reconciling after the bind extension (
RequeueAfter: 0). It now polls every 60 s, as a ready unclaimed member does.TestPoolMemberExtendsOnceAtBindexpects that now. Rule 2 (the latest build must be the start build) still runs for a bound member before its extension, because it is the provenance check before that write. After the extension, the member returns before rule 2, so a claimant's builds never fail it.Validation
New tests (
coderworkspacepool_claim_lifecycle_test.go). I wrote them first and ran them on PR 11a: all 3 failed, as did the changedTestPoolMemberExtendsOnceAtBind. With this PR they pass.TestPoolCreatesOnDemand:replicas: 0and no claim give no member. One claim gives exactly one member. Once it is ready, the claim binds it, and the pool creates no more.TestPoolDeleteKeepsBoundClaims: one member bound to the older of two claims, then the pool is deleted. The bound member keeps the claim as its controller, and its claim staysBoundwith its workspace name and owner. The other claim waits withPoolNotFound, and so does a claim on a pool that never existed.TestPoolClaimLostWhenWorkspaceDeleted: a real workspace, bound and extended. The member requeues after 60 s. A stop build by the claimant leaves the memberReady. After Coder deletes the workspace, the member fails withWorkspaceDeleted=True, and the claim becomesLostwithWorkspaceDeleted. After the failed member is gone, a new ready member is not bound to theLostclaim.Changed:
TestPoolBindsOldestClaimFirstnow expects 3 members (the bound member, a replacement forreplicas: 1, and one for the waiting claim).TestPoolOrphanClaimKeepsOwnerName(round 1): the bind lands, the reconcile then fails, and the pool is deleted. The orphan reconcile showsBoundwith ownertester.TestPoolClaimLostWhenWorkspaceDeletedalso runs a reconcile whose claim list shows theLostclaim without status, after its failed member is gone: no bind.Codex review round 1: 4277241744 (a
Lostclaim on a stale cache) and 4277241752 (the owner name of an orphaned claim after a crash) are fixed in #387 (c4824691: the uncached claim read before a bind, and the owner name stored before the bind) and tested here in23e6281d. Both tests failed on the previous head.Codex review round 2: 4277392817 (a stale
Lostclaim counted as demand when no member was free) is fixed in #387 (c8c6095d: every waiting claim is read again without the cache beforepoolWant) and tested here ine6eb14ff:TestPoolClaimLostWhenWorkspaceDeletedruns a stale-cache reconcile with no free member, and no member is created. It failed on the previous head.Mutation checks: I removed one control at a time, and each removal made a named test fail (10 of 10): the claims in
want, counting only waiting claims inwant, the missing-pool reconcile, keeping bound claims of a missing pool, settingLost, never binding aLostclaim, the bound member's read after the extension, the early return before rule 2 after the extension, the owner name stored before the bind, and the uncached claim read for aLostclaim.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
286 hand-written lines (114 production, 172 test, counting removals). No generated files.
Risks
None for users. The reconcilers are dormant. Recorded for PR 12: a workspace that the claimant renames fails the identity check of a bound member (
OwnershipUnknown), so its claim showsBoundwithReady=False.Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$173.65