Repository navigation
🤖 feat: write a cleanup record before each CoderAIProvider create (#353 part 2a) - #374
Conversation
Change-Id: Ice66d8e736e7372a3afd95d8c96a79955aca6b04 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 |
![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: 553cad7604
ℹ️ 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 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. |
…lete only a valid record, and use the recorded provider ID on delete Change-Id: I43013d332be2b96542173c7871a73bdf8efe4957 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: 07a012629f
ℹ️ 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".
…ntrol plane reaches another deployment, and guard the setup Change-Id: Iee4add631b870c6070066c275f89f84ed86762fa 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: 6e1d1bf90f
ℹ️ 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>
|
@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: 6e1d1bf90f
ℹ️ 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 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: 887b0f10db
ℹ️ 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".
… an empty status Change-Id: Ib7fa8097dbaf483d8f50c72676c746ceceabc1c1 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…nup-records Change-Id: I391ceb4bb8f88ccfa3c5f8a6d3601d9f3b654557 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: 76eb17fc39
ℹ️ 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 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. |
1 participant
Part 2a of #353: the operator now writes a cleanup record before every
CoderAIProvidercreate in Coder. When a namespace deletion (or a gone control plane) releases the provider without a delete, the record stays and names the Coder deployment and the provider ID. The sweep that uses these records is the stacked PR (part 2b). TheCoderAIGatewayKeyhalf follows after both.Refs #353
Design note (option B′, maintainer decision in #353 (comment))
Why records. A namespace deletion removes Coder's pods at once, so no finalizer or hold can delete the provider in Coder (#362 covers only a direct control-plane delete). A sweeper without records would have to guess from absence and could delete another cluster's providers. A record proves which deployment holds which provider.
Record format. One ConfigMap per object, named
ck8s-cleanup-<object UID>, labelcoder.com/cleanup-record=CoderAIProvider, in the operator's namespace:POD_NAMESPACE, else the service account namespace, elsecoder-system(the namespace of the shipped Role). Data:kind,uid,namespace,name,deploymentID,nonce(the create display namecoder-k8s:<uid>),type(the provider type),coderID(once known) andcreateAttemptTime. The ConfigMap creation time is the created-at.How this PR meets the invariants of the decision (2b covers 4–8):
createAIProvidercallswriteAIProviderRecordbefore the create marker. A failed write or a failed deployment-ID read returns before any POST.coderDeploymentIDreads/api/v2/buildinfodeployment_idright before the create, and again when the provider ID is first recorded. Only a canonical UUID is accepted.ensureAIProviderRecordruns before the status write and before every sync). A forgedstatus.providerIDfailscheckAIProviderIDfirst, so it never reaches a record.deleteStepremoves the record only whendeleteInCoderconfirms (success or Coder's own 404, or a read that proves nothing of this object exists), and when the status proves no create was ever confirmed.releaseWithoutDeletekeeps it.manager-roleincoder-system(from a kubebuilder marker) with ConfigMap create/update/delete, bound only to the operator service account by the new RoleBindingcoder-k8s-cleanup-records. The ClusterRole keeps ConfigMap read only.CoderAIProvider.Upgrade. A provider that exists from before this release has no record. The first reconcile that reaches Coder writes one, with the deployment ID that Coder answers then. That includes the disable path for a failing key Secret: the record write there never delays the disable. A malformed
spec.baseURLstalls before any Coder access, so such an object gets its record only after the URL is fixed.Deployment check on delete (review round 2). Before a delete, the controller compares Coder's current deployment ID with the record's. When the control plane now reaches another deployment (for example after its database Secret changed), it sends no delete there and releases the finalizer without a delete. The record stays for a sweep. The record backfill also runs first in every reconcile now, before the base URL and key Secret checks.
Deployment change under a live object (review round 3). When the create path finds a record of another deployment, the object stalls with
CoderDeploymentChanged. The record keeps the old locator, and the controller creates nothing in the new deployment.Reads and deletes (review round 1). Every record read goes through a live API reader (
APIReader, wired tomgr.GetAPIReader()), because the cache can miss a record written moments ago. A record delete reads the ConfigMap first, deletes only a valid record of that UID, and uses a UID precondition. Whenstatus.providerIDis empty, the delete path uses the record'scoderID, so a lost status write after a create cannot make the controller treat its own provider as someone else's.A ConfigMap of the record's name that is not a valid record of that UID stops the reconcile with an error. The operator never writes over it.
Other changes
internal/controller/cleanup_record*.TestControlPlaneHoldRevokesWorkspaceRBACnow registers the Gateway API scheme itself. It failed when run alone (it passed in the full suite only because another test registered the scheme first).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 record before the create, no provider ID before the sync, record kept after a confirmed delete, record removed on a release without delete, no backfill, no canonical-UUID check of the deployment ID, no name check of the record.
The dogfood of the whole provider half (records plus sweep) is on the stacked PR, because a record alone changes nothing a user can see except the ConfigMap.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high