Repository navigation
🤖 feat: bind CoderWorkspaceClaims to ready CoderWorkspacePool members and write the claim status - #387
Conversation
…nd write the claim status Change-Id: Ia47cdccaaad51b9a7cfa069a5e5de45de22be757 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. |
|
@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. |
![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: 3f5e8707ad
ℹ️ 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>
|
@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: c482469109
ℹ️ 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>
|
@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: c8c6095d95
ℹ️ 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".
Change-Id: I06dd83d03cfd1e061e4d3a03940ac6e5a2be9934 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@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: 366c494d0b
ℹ️ 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 review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
1 participant
Summary
This PR lets the dormant
CoderWorkspacePoolreconciler bindCoderWorkspaceClaimobjects to its ready members and write the claim status (plan 6.3 steps 5 and 10). Nothing registers the reconciler yet, so no user sees a change.creationTimestamp, then name). It picks a ready unclaimed member that it controls: current before outdated, then the oldestreadyTime. "Ready" is the rule that the counts use: phaseReady,Ready=True, and notUsed(plan 16, PR 9 advisor item (a)). A member whose agent is not ready, a used member, a failed member, and a deleting member are never bound.spec.claimRefand moves the controller owner reference from the pool to the claim (withblockOwnerDeletion) in oneUpdatethat carries theresourceVersionfrom the uncached member list. A member that changed after the list, for example one that its reconciler marked used, answers 409 and stays unbound. A conflict ends the reconcile. The pool label stays, so the pool still counts the member inboundReplicas. The set-once CEL rule onclaimRefrefuses a second bind of the same member.claimRef.uidis the claim's UID. A bound claim showsBound, the member, the workspace ID and name, the owner's Coder username (socoder ssh <ownerName>/<workspaceName>works), andboundTime.Ready=Truefollows the member's readiness. A claim without a member waits withPendingWaitingForReadyWorkspace. After a crash between the bind and the claim status write, the next reconcile finds the member and finishes the status, and it binds no second member.Refs #153
Background
This is PR 11a of the
CoderWorkspacePoolstack (#153). PR 11b (on-demand creation for waiting claims,PoolNotFound, and the bound-member watch that marks a claimLost) stacks on this branch.The pool lists claims through its client (the cache, once PR 12 registers the reconciler) and filters them by
spec.poolName. The member list, which decides each bind, stays uncached. PR 12 wiring adds the claim watch that maps a claim to its pool (recorded in plan section 16).resolvenow also returns the owner's Coder username from the owner read it already does.Validation
New tests (
coderworkspacepool_claim_test.go). I wrote them first and ran them on the base (8f580b0b, the reviewed #380 head): all 5 failed (member ... is not bound,the second bind of the member must fail). With this PR they pass.TestPoolBindsOldestClaimFirst: one ready member and two claims. The older claim gets the member:claimRef, the claim as the only controller withblockOwnerDeletion, no reference to the pool, the pool label kept. Its status isBoundwith the member, workspace ID and name, ownertester,boundTime, andReady=True. The newer claim waits withWaitingForReadyWorkspace. The pool counts 1 bound member and creates a replacement.TestPoolBindsCurrentBeforeOutdated: a current member (newer) goes to the first claim, an older outdated member to the second claim, and a third claim waits. A used member and a member withReady=Falseare never bound.TestPoolBindRace(plan mutation check 4): two reconciles race for one member, the second with a cache that has not seen the older claim. Exactly one bind lands, and the other reconcile fails. Second case: someone uses the member right after the pool's member list. The bind answers 409, and the member stays unbound.TestPoolFinishesClaimAfterCrash: a member already holds the claim, but the claim has no status. The reconcile writesBoundfrom that member, with the owner name, and binds no second member.TestPoolNeverRebindsClaim(round 1): a member of another pool that names the claim is not its holder. After the bound member is deleted, a reconcile whose claim list has no status binds no second member, and the claim becomesLostwithMemberDeletedandStalled=True.TestPoolClaimDeleteDeletesWorkspace: after the bind, the claimant uses the workspace, then the claim is deleted. envtest runs no garbage collector, so the test checks the owner reference and deletes the member as the collector would. The member's cleanup starts the delete build and then releases its finalizer.Codex review round 1 (all fixed in
c4824691):boundTimeset, or phaseBoundorLost) never binds again. Right before each bind, the pool reads the claim without the cache, so a status that the cache has not shown yet counts too. A once-bound claim whose member is gone becomesLostwithMemberDeleted(TestPoolNeverRebindsClaim).spec.poolName, so a member of another pool never counts.Codex review round 2 (fixed in
c8c6095d):refreshWaitingClaims, before the bind. A claim that is gone, or whose name a new claim took, leaves the list (TestPoolSkipsReplacedClaim).Lostfor a once-bound claim without a holder, the pool lists the members again without the cache (TestPoolClaimNotLostOnOverlap).Both new tests failed on the previous head.
Codex review round 3 (fixed in
366c494d): 4277508974: a free member must also name the pool in its immutablespec.poolName, so a misnamed member is never bound (TestPoolBindsCurrentBeforeOutdated).Codex review round 4 (blockers only, both deferred to before PR 12 and recorded in plan section 16): 4277607232 (two overlapping pool reconciles, which need two operator replicas at once, can bind two different members to one claim; planned fix: reserve the member in the claim status first) and 4277607237 (a new pool that reuses a deleted pool's name with another owner overwrites the claim's
ownerName). Round 5: clean.Mutation checks: I removed one control at a time, and each removal made a named test fail (20 of 20): bind with an empty
resourceVersion(plan mutation 4: the API server refuses it), bind with aresourceVersionread again just before the update (the used-after-list case catches it), newest claim first, outdated before current, binding a used member, binding a member withReady=False, ignoring a member that already holds the claim, keeping the pool as controller, not moving the counts, dropping the owner username, scoping the holder to the pool, the uncached claim read before a bind, the once-bound rule,LostwithMemberDeleted, the label repair, skipping a replaced claim, the holder check beforeLost, the claim refresh, and thespec.poolNamecheck on free members.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
672 hand-written lines (265 production, 407 test, counting removals). This is over the 500-line target: the first version had 477, and the three review rounds added about 195 lines of race handling and tests. No generated files.
Risks
None for users. The reconciler is dormant. Recorded for PR 12: the claim watch and its mapping to the pool key.
Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$173.65