Repository navigation
🤖 fix: finish the control plane hold and cleanup record follow-ups of #353 - #410
Conversation
… hold end (#353) Change-Id: Ib45cb0e8ada72ddebca134b0c75dcb7262aa6e7c Signed-off-by: Thomas Kosiewski <tk@coder.com>
…old end (#353) Change-Id: Idd99f51f9b69f1d51228090a15b9d965082bbce8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I12ec09f9374ff5c91cedf843b7d0be637891dda8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…nup_released_without_delete_total (#353) Change-Id: Ia7fc446d421c2f95496d26d23065894dd96f6216 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ch kind (#353) Change-Id: I0efae04d44d60e32c821b656636f24365e2703bf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…fter a status edit (#353) Change-Id: Ia86a2e47096359ec5248b654b3da7e6afaf90eea Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tus.lastCreateAttemptTime (#353) Change-Id: If34b70a7c627e0a500f6168129502ac060e3cf98 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…witch (#353) Change-Id: Ibde71ab3c6d21f32d016f7c963aae62cf2e7cde5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… pending create is confirmed (#353) Change-Id: I07403a11500125126e518ada28479b362443abcf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ployment does not hold (#353) Change-Id: Iada71ed3a9231a388eb8737ff927936f9d01bfc5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…nd its Coder Deployment (#353) Change-Id: I96e990e46152796b2fee072da36c588a45ae53cf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ses without a hold (#353) Change-Id: I2a732a6a1531c2c558e4f5f4ac89c43adc09b553 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…being deleted (#353) Change-Id: I122bdcbb2d1529376d035e1bd35283ec99d275d1 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. |
…lowups Change-Id: Ibac7c737a909417b7d410b2ac1e3aa796a633d5b Signed-off-by: Thomas Kosiewski <tk@coder.com>
…t reconciles first (#353) Change-Id: Ie1b0cd061d176ec43a0b93e34f99b925629d6460 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…#353) An orphan delete or a cache miss shows no Deployment while Coder's pods still run. Ending the hold then removed the control plane before its dependents deleted their resources in Coder. Change-Id: I967bbab03cbfafddc46a302fd11a4ed7ce830720 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…oes not get it back (#353) Change-Id: Ibbbb21ce939ed0c2483a9161d3ee165759a40dd7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…lowups Change-Id: I37be0530a097ff2bf1339b2747eba03277d02848 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Breezy! 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: I56c4f4ea8c22c10463fa435820f4c30da91d74b5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
A provider cleanup that read a valid operator token before a Postgres failure cleared operator access still calls Coder. Releasing the control plane at once lets its Service and Deployment go and loses that cleanup, so the hold applies as on main. TestControlPlaneHoldsForCleanupInFlight covers the case. Change-Id: I315344184b059e1067b3ffb0e7ddd0d261fc6d11 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I24abcf46ed6aab2f3b9fe4b3bcc2b3851cbe7abf 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. |
![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: 902e888f50
ℹ️ 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: I803385a71a62827cf1a28e6f0a95678c34b854e4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex 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". |
Summary
This PR fixes 6 of the 14 follow-ups listed in #353. It leaves 7 open with a reason each, and 1 was already fixed. A provider delete under the control plane hold ends at the hold end, as key and MCP server deletes already do. A
CoderMCPServerthat releases its finalizer without a delete in Coder now counts incoder_k8s_cleanup_released_without_delete_total. Two cleanup record gaps are closed, and the how-tos name MCP servers in the hold.Refs #353
Refs #156
Background
#353 tracks what a namespace or control plane deletion can leave in Coder. Its main parts shipped in v0.1.5 (#362, #374, #375, #384, #385, #386). Two comments of #353 list follow-ups from those reviews: items 1 to 9 from #362 (#353 (comment)) and items 1 to 5 from #384 (#353 (comment)). I checked each item against
mainat 3d902c9.mainbefore this PROperatorAccessNotReady, then release.time.Now()).coderaiprovider_create.go, which another PR edits, and the MCP server path reads the wall clock too.Implementation
controlPlaneHoldsis unchanged frommain. Earlier revisions of this PR ended the hold early in two cases, and an independent check showed that each lost cleanup thatmainperforms. No replacement signal is added.mainperforms: after an orphan delete of the Deployment (Coder's pods still run), or one cache read that missed it, the control plane went first and its dependents released without any DELETE.TestControlPlaneHoldsWithoutCoderDeploymentcovers the case.TestControlPlaneHoldsForCleanupInFlightcovers the case.recordAIGatewayKeyIDalso rewrites the record when its create time is older thanstatus.lastCreateAttemptTime.ensureAIProviderRecordskips the write only for a record with a provider ID. A record without an ID belongs to a pending create. So the write reads the deployment ID and stalls withCoderDeploymentChangedon a mismatch, before a name conflict in the new deployment can clear the create marker.TestControlPlaneDependentsPerKindpins the per-kind count with each kind as the largest.Validation
mainand passes with the fix:TestAIProviderDeleteUnderHoldEndsAtHoldEnd. The pass took 30 s (the request timeout) instead of ending at the hold end 5 s later. This is a fake-client test, like the key and MCP server tests of the same deadline, because it measures real time against a Coder that hangs. The envtestTestCoderAIProviderDeleteUnderControlPlaneHoldstill covers the provider delete under the hold.TestCoderMCPServerReleasedWithoutDeleteMetric. The count stayed at 0. With the fix but without the call that drops an earlier record, itsrecoveredsubtest fails.TestCoderAIGatewayKeyRecordAfterClearedCreateTime. The record kept the time from 1 hour before.TestCoderAIProviderPendingCreateAfterDeploymentSwitch. The object stalled withNameConflictinstead ofCoderDeploymentChanged.TestControlPlaneDependentsPerKindpasses onmain. It fails when the MCP server count is dropped from the per-kind maximum.TestControlPlaneHoldsWithoutCoderDeploymentfailed on the earlier revision (b92c573) in all 4 subtests: the control plane went at once, and the key, provider and MCP server objects released without a DELETE. It passes on this head and onmain. Its subtests delete the Deployment with orphan propagation, then run the control plane reconcile before each dependent (keys, provider, MCP server), and run a provider whose first DELETE gets a 502 before the control plane.TestControlPlaneHoldsForCleanupInFlightpauses a provider delete right after its operator token Secret read returns a valid token. During the pause, a control plane reconcile with a failing provisioner persistsoperatorAccessReady: false, the test deletes the control plane, and a second reconcile runs. The control plane holds (requeue 10 s), and the resumed cleanup deletes the provider in Coder. It passes on this head and onmain. It fails when the operator access terms are put back intocontrolPlaneHolds. This is the verifier's in-flight probe as an envtest.make verify-vendor,make build,make lint,make test,make test-integration,make docs-check.mainat ef86c32, with 🤖 fix: resend the AI provider key after a read confirms a create #397, 🤖 fix: revoke the operator token when its Secret loses its owner, and check the owner in every reader #401, 🤖 fix: evict unused Coder transports, normalize default ports, and export connection wait time #402, 🤖 fix: pool member counts every last_used_at past its baseline as use #404, 🤖 docs: add a runnable GitOps promotion example for CoderTemplateTest #405, 🤖 feat: add tailLines to the coderworkspaces/log subresource #411 and 🤖 fix: share one connection pool across Coder host case and make apiserverapp envtests repeat-safe #412. The merges had no conflicts.Risks
main. A status without operator access does not prove that no dependent cleanup is in flight: a cleanup that already read the token can still call Coder. Generation checks stop stale status writes, but not a status snapshot or token that another reconciler already holds.foregroundDeletionmarker is gone still holds for 10 minutes, as onmain. The dependents' calls fail during that time, and they release after the hold. A missing Deployment alone cannot end the hold, because an orphan delete or a cache miss shows the same thing while Coder's pods still run. A safe signal needs more, for example an uncached read that finds neither the Deployment nor a Coder pod.ConfirmingCreate, retried every 5 s), each reconcile sends one more buildinfo request and one record update with unchanged data.ensureAIProviderRecordrewrites a record whose ID is set whilestatus.providerIDis empty, and the write drops that ID.maindoes the same. Codex flagged it on this PR. Keeping the recorded ID needs its own record design change.CoderMCPServerneed their own design. Option B′ allows a delete of exact recorded IDs, so a recorded config ID fits. A delete of a config found only by its temporary display name would extend invariant 5, which names a key prefix and a provider nonce. MCP server configs belong to an organization, so the record also needs the organization ID. Providers and keys each took a record PR and a sweep PR with a Kind run, so MCP server records are a separate follow-up, together with the MCP server part of 🤖 fix: hold a deleted CoderControlPlane while AI Gateway keys and providers still use it #362 item 3.Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:high