Repository navigation
🤖 feat: hold CoderWorkspacePool members at a lowered owner cap, and keep the failure run across a rescale - #379
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: 92242ff0a2
ℹ️ 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>
|
@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: ee015d1bc9
ℹ️ 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>
|
@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: 007545e9b8
ℹ️ 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>
|
@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 |
…eletes Change-Id: I8f645bb2b9743fef214dceb47c5d91756fa08910 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Codex Review: Didn't find any major issues. Chef's kiss. 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 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. Hooray! 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". |
Summary
This PR closes Codex security finding 4271920249 from #359, and it keeps the failure run of a
CoderWorkspacePoolacross a rescale. Both changes are in the dormant pool and member reconcilers. Nothing registers them yet, so no user sees a change.maxWorkspacesholds. When an admin lowers the owner'smaxWorkspaces, the pool can already have members that it created under the old cap and that have not sent their create request yet. Before this PR, those members still created workspaces above the new cap. Now:OwnerWorkspaceLimitand sends no create request.resourceVersion, so a member that wrote its marker after the list answers 409 and stays.metadata.generation) started a new run of failures. So a pool that an HPA rescales through/scalecould get past the 5-failure limit. Now only a change ofstatus.resolvedstarts a new run: the template, the version, the preset, the parameters, the owner, orreadyTimeoutSeconds. A change ofreplicasormaxInFlightkeeps the run.maxWorkspacesstill caps creation either way.Refs #153
Background
This is PR 10a of the
CoderWorkspacePoolstack (#153). PR 10b (delete order, new before old, used-member release, consent removal) stacks on this branch.Decision on the run reset: the maintainer asked that a replicas-only change does not reset the run, and that only input changes that can fix a failure do. I added
readyTimeoutSecondsto the list, because a longer timeout can fix aDeadlineExceededfailure. Without it, a pool that stalled on deadline failures could only leaveMemberFailureLimitthrough a template, version, or parameter change. To detect that change without a stored copy of the spec,status.resolvednow holdsreadyTimeoutSeconds, and new members pin it from there. The member input check does not compare it, so a timeout change does not make ready members outdated. The first resolve (no storedstatus.resolved) does not start a run.This also removes the case where the failure write before the deletes stores the run but not
observedGeneration: the reset no longer readsobservedGeneration. I recorded the remaining effect (that write publishes an oldobservedGenerationuntil the final write) in the plan for PR 12, as asked.API: one new optional status field,
status.resolved.readyTimeoutSeconds. The CRD is still inconfig/crd/dormant/, and the pool has no API reference page yet, somake docs-referencechanges nothing.Validation
Red, then green. I wrote the two tests first and ran them on
main(fb42486a):With this PR, both pass.
TestPoolLoweredOwnerCap(new,coderworkspacepool_cap_test.go): 3 pending members, thenmaxWorkspacesgoes from 10 to 1. Every member reconciles before the pool sees the new cap: Coder gets exactly 1 workspace, and the other two wait withOwnerWorkspaceLimitwithout a marker. The pool then deletes exactly those two, keeps the member with the workspace, and creates no new member (OwnerWorkspaceLimit).TestPoolFailureRunSurvivesRescale(new): after one failure, a/scalewrite to 2 replicas, amaxInFlightchange, and a new name of the same version keepconsecutiveFailures: 1and the backoff. AreadyTimeoutSecondschange starts a new run, a failure of a member with the old timeout does not count in it, and the new members pin the new timeout.TestPoolFailureBackoffAndLimitandTestPoolFailureRunResetsnow lift the limit with areadyTimeoutSecondschange instead of amaxInFlightchange.TestPoolResolvesInputsexpects the new field.Codex review round 1 (all fixed in
ee015d1b):TestPoolLoweredOwnerCapDeletesOnce).poolRunInputsChangedignorestemplateVersionName).readyTimeoutSecondsno longer counts in the new run. It is still deleted.Codex review round 2 (fixed in
007545e9):TestPoolMemberRechecksCapLastanswers the first control plane read with the old cap and failed on the previous code.freshOwnerCap), before the cap trim and the creates, whatever the demand. Before, a pool that needed no create skipped that read.Codex review round 3 (fixed in
4612ad65):TestPoolFailuresUncountedWhileUnresolved). Trade-off: failures during a Coder outage do not count either.MemberFailureLimitmessage names only inputs that a user can change (spec.templateis immutable).Mutation checks: I removed one control at a time, and each removal made a named test fail (10 of 10): the member's cap check, the pool's deletes above the cap, keeping members with a workspace out of those deletes, counting only members that may have a workspace in the member's check, the reset on a generation change, the resolved timeout, not counting deleting pending members as live, ignoring a version rename, not counting failures at an old timeout, and not counting failures while the inputs do not resolve. The final cap read has a red run instead:
TestPoolMemberRechecksCapLastfailed on the code before that fix.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
468 hand-written lines (230 production, 238 test, counting removals). Generated: 15 lines of the dormant CRD.
Risks
None for users. The reconcilers are dormant. One limit, recorded in the plan for PR 12: the member's cap check is exact while member reconciles run one at a time (the controller-runtime default). With parallel member reconciles, two members can pass the check together.
Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$154.26