Repository navigation
🤖 feat: sweep the cleanup records of CoderAIProvider objects that went without a delete (#353 part 2b) - #375
Conversation
Change-Id: Ice66d8e736e7372a3afd95d8c96a79955aca6b04 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…without a delete (#353) Change-Id: I81ad241e8a8032f3df9eac7d825c0c70fb7da105 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ice66d8e736e7372a3afd95d8c96a79955aca6b04 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…without a delete (#353) Change-Id: I81ad241e8a8032f3df9eac7d825c0c70fb7da105 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ 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. |
Dogfood: namespace deletion with an external database, then a sweepHead of the operator image: Setup: Kind v1.32.0, Coder v2.37.2 without a license, Postgres in namespace
In step 3, the second TranscriptThe script never prints the provider API key. The transcript has 0 matches of the key value. Recording: sweep-dogfood.mp4Generated with |
|
@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: 76d30aebff
ℹ️ 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".
…lete only a valid record, and use the recorded provider ID on delete Change-Id: I43013d332be2b96542173c7871a73bdf8efe4957 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I2416199a0419ac66fee5991058b8158b841ea2e7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Id47fff062cc502fe08acd8a4b7ae52512512480d Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I53d93aa17648495174eaea99b89f62cb44c830ed Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d80f29e1fe
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: d80f29e1fe
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…ntrol plane reaches another deployment, and guard the setup Change-Id: Iee4add631b870c6070066c275f89f84ed86762fa Signed-off-by: Thomas Kosiewski <tk@coder.com>
…-cleanup-sweep Change-Id: I2b8212f8353ef7511de29ca2da53bbd68bd3aa60 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ntrol plane without working access as unknown Change-Id: I14ec8db9add5723cde4a6f4f1b3308a1afb1f82c Signed-off-by: Thomas Kosiewski <tk@coder.com>
…space Change-Id: I0ce5b4e4ac65226e910f22ff8055a9100069f69e Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Dogfood on head 0884db3 (after review rounds 1 and 2)This replaces the run in the comment above, which used an earlier head. Operator image
The operator ran through the earlier runs too, so its counters add up across runs:
An earlier run on this head showed namespace ns1 being terminated in the middle of the run, and the sweep then ran through ns1 at that moment. I could not find the cause in the logs. For this run I added a watch of the namespaces (below) and a check that each namespace deletion is complete. The watch shows ns1 recreated only in step 3 and step 5, and the result matches the design. Namespace watch (ns1, ns2)TranscriptThe script never prints the provider API key: 0 matches in the transcript. Recording: sweep-dogfood-2.mp4Generated with |
🛡️ 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: 0884db3037
ℹ️ 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".
… record the provider type, and default the record namespace to coder-system Change-Id: I7b699d65437effb2e94cefb175ee758d163b2c83 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…-cleanup-sweep Change-Id: Ia86133255959b1668ffd6e0a77689f08f6341b37 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…d check the provider type on the nonce path Change-Id: I515ed4a2e1d079f0b6553532947672482b786ffe Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Dogfood on the final head 5e9e849 (after review round 3)Operator image
The namespace watch shows each namespace created and deleted only by the script steps. Namespace watch (ns1, ns2)TranscriptThe transcript has 0 matches of the provider API key. Recording: sweep-dogfood-3.mp4Generated with |
|
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". |
🛡️ 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. |
… an empty status Change-Id: Ib7fa8097dbaf483d8f50c72676c746ceceabc1c1 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…-cleanup-sweep Change-Id: Id144dbfc8aa981a1b9aff68e89e961198c550910 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…nup-records Change-Id: I391ceb4bb8f88ccfa3c5f8a6d3601d9f3b654557 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…-cleanup-sweep Change-Id: I22f854ac36863e05bcd045adf62f1c7d6702125a Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Dogfood on c4281d5 (after the round-4 blocker fix)Operator image The results match the run in 6175120718:
Namespace watchTranscript0 matches of the provider API key in the transcript. Recording: sweep-dogfood-4.mp4Generated with |
|
Codex Review: Didn't find any major issues. 👍 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. |
…nup-sweep Change-Id: Ibf55dfa736b227d368c8038ccbfa2c7c02863278 Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 participant
Part 2b of #353, stacked on #374 (base
feat/ai-provider-cleanup-records). A new controller sweeps the cleanup records ofCoderAIProviderobjects that went without a delete in Coder. After a namespace deletion with an external database, the leftover provider goes once a new control plane on the same database is ready. A different Coder deployment is never swept, also at the same URL.Refs #353
How the sweep works
CleanupRecordReconciler(controllercleanuprecord) watches the record ConfigMaps in the operator namespace. A deletedCoderAIProviderwakes its record. A control plane that gets operator access (or a new URL) wakes every record. For one record it:CoderAIProviderlive through the API reader. If an object with the record's UID exists, it does nothing and looks again in 30 minutes. A new object with the same name but another UID counts as gone./api/v2/buildinfothrough each readyCoderControlPlanein the object's own namespace, and uses the first whose deployment ID equals the record's. It never matches by URL or name. Coder reports its deployment ID itself, so a control plane in another namespace (which someone else can run) never sweeps a record (security review round 2). The operator access is read live. When a ready control plane does not answer yet (for example while its Coder pods start), it retries in 1 minute.coder_k8s_cleanup_records_unmatched{kind}counts it, and aNoMatchingDeploymentWarning Event on the ConfigMap names the object and the deployment ID. It looks again every 10 minutes. The docs tell an admin to delete the provider by hand and then the ConfigMap.The counter
coder_k8s_cleanup_records_swept_total{kind}counts swept records. A ConfigMap that is not a valid record (wrong label, name or UID) is logged and never acted on.RBAC. The sweep uses the namespaced Role from 2a to delete records. It adds
events.k8s.ioeventscreate/patchfor the Event (the newGetEventRecorderAPI). Coder access goes through the same operator token as every other controller.Docs. The provider how-to "Cleanup records" section describes the sweep, the metrics, the Event and the manual way out. It keeps the warning to delete providers before their namespace: that is the only fix when no control plane ever reconnects to that database.
Dogfood
Evidence on
c4281d51(same provider and sweep code as the head, which only adds a merge of main): #375 (comment) (earlier heads: 6175120718, 6174550134, 6173286421) (Kind, Coder v2.37.2, no license, external Postgres with databases A and B).NoMatchingDeploymentEvent.Validation
Local gates on the tree of this commit before a docs-only edit and a rebase onto main (
1c0df24c, #373: CI and E2E script files only; no Go change came with it):make build-installer,make codegen,make docs-reference, drift (git diff --exit-code),make verify-vendor,make build,make lint, govulncheck,make test,make test-integration,go test -race ./internal/controller/...,make docs-check, actionlint v1.7.10.docs/explanation/architecture.md) and the rebase:make docs-check, actionlint andgo build ./...again.Mutation checks (each one made a test fail): no namespace restriction, live access read replaced by the cache, no UID compare, no settle window, no deployment-ID match, no nonce check, record removed on a failed delete, any 404 accepted, no Event, no site-owner check, unknown deployment treated as unmatched.
Not in this PR
CoderAIGatewayKeyhalf (records with theck8s-<uid>-key prefix, sweep of keys) follows after this PR.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high