Conversation
github-actions
Bot
added
component/k8s
![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: bb966c92ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // detach; re-pointing a bound sandbox to a different pool is rejected because | ||
| // the previous pool would recycle the in-use pods while the stale allocation | ||
| // record blocks the new pool from supplying replacements. | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.poolRef) || size(oldSelf.poolRef) == 0 || oldSelf.poolRef == '*' || !has(self.poolRef) || size(self.poolRef) == 0 || self.poolRef == oldSelf.poolRef",message="spec.poolRef cannot be re-pointed to a different pool; clear it first to detach" |
There was a problem hiding this comment.
Blocking: this rule raises the project Kubernetes compatibility floor to >= 1.25, conflicting with what the repo claims and tests today.
x-kubernetes-validations with oldSelf (transition rules) requires Kubernetes >= 1.25 to be accepted and enforced (GA in 1.29). On 1.23/1.24 it is behind the ValidationRules feature gate (off by default), and on < 1.23 the field is rejected outright — the CRD cannot be installed on those clusters at all.
But this repo currently:
- Runs e2e on Kind 1.21.1 / 1.22.4 / 1.24.4 in CI (
.github/workflows/kubernetes-test.yml), and the e2e suite installs the CRDs viamake install(test/e2e/e2e_test.go) — those legs will fail at CRD apply. - Declares
kubeVersion: ">=1.21.1-0"incharts/opensandbox-controller/Chart.yaml, says "Kubernetes 1.21.1+" in the chart README, and defaults local e2e toKIND_K8S_VERSION=v1.22.4(kubernetes/Makefile).
So the "Breaking Changes: None" claim in the PR description is not accurate — this effectively drops support for < 1.25 clusters. We need a maintainer decision before merge:
- Raise the documented/CI support floor to >= 1.25 (update
Chart.yamlkubeVersion, chart README, CI e2e matrix, local e2e default), or - Enforce the invariant with a version-agnostic mechanism (validating admission webhook, or a controller-side guard that rejects/events on re-pointing).
The rule logic itself is correct for the documented transitions (initial bind, "*" -> name write-back, detach, no-op); the concern is purely deployment-surface compatibility.
There was a problem hiding this comment.
Thanks — you're right. My earlier statement that the older-version CI matrix continued to pass was not supported; this PR has only run the auto-label check.
I also checked the existing validation infrastructure. OpenSandbox does not currently enable a validating webhook, so replacing the CEL rule with one would add a new deployment surface (webhook service/certificates and Helm/Kustomize wiring). A controller-side rollback cannot reject the update at admission time and may race with the old pool's orphan cleanup.
I agree that raising the Kubernetes support floor should not be done implicitly in this bug fix. Would you prefer introducing validating-webhook support for this invariant, or handling pool-to-pool re-pointing as an explicit safe controller transition? I'm happy to revise the PR once the intended direction is clear.
AI usage disclosure: I used OpenAI Codex to inspect the repository's existing validation/controller paths and help draft this response; I reviewed the conclusions before posting.
There was a problem hiding this comment.
Thanks for the pushback — I went back and verified against the Kubernetes source, and you're right. On 1.21/1.22 the XValidations field doesn't exist in the apiextensions v1 types (it was added in 1.23), so x-kubernetes-validations is silently dropped during decoding and the CRD applies cleanly — my earlier claim that the CRD cannot be installed on those clusters was wrong. On 1.23/1.24 the field is accepted but not enforced (alpha feature gate, off by default); enforcement starts at 1.25 (beta, on by default; GA in 1.29). So on 1.22 everything does run, exactly as you described: guard absent, behavior unchanged.
What remains is the softer version of my concern — on <1.25 clusters the fix silently doesn't protect, while the chart still advertises kubeVersion: ">=1.21.1-0" — which is a support-window / documentation question rather than a compatibility break. Let me think it over and follow up.
…her pool
Re-pointing a bound BatchSandbox from pool A to pool B made pool A recycle
the in-use pod as an orphan while the stale alloc-status annotation kept
pool B from supplying a replacement, permanently starving the sandbox with
no event explaining why.
Add a CEL transition rule on BatchSandboxSpec so the API server rejects
the re-point while still allowing the defined transitions: initial bind,
auto-assign resolution ("*" -> name), and detach (clear poolRef). Sync the
generated CRD and the Helm chart copy, and add envtest regression coverage
for both the rejected and the allowed transitions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bb966c9 to
e5bb6b4
Compare
|
Closing for now: this PR breaks Kubernetes 1.22 compatibility, which we need to keep supporting. ProblemThe PR adds CEL validation rules ( This is why the E2E ImpactThis is not just CI noise — once merged, any 1.22 cluster cannot apply the CRD at all, so the controller cannot be installed there. There is also no "CEL + webhook fallback" hybrid that restores 1.22 support, because the CRD manifest itself becomes unappliable. Path forwardTo keep 1.22 compatibility, |
tomsen02
changed the title
|
The Kubernetes 1.22 compatibility feedback is addressed in 35f192df, pushed to this PR's original branch. The revision removes CEL from both CRDs and uses the existing allocation's recorded source Pool across indexing, routing, sync, recovery, and cleanup. Unsupported pool-to-pool or pool-to- Validation passed: complete Could a maintainer reopen this original PR so it picks up the updated branch and runs CI? AI assistance (OpenAI Codex) was used for this revision and validation. |
github-actions
Bot
added
documentation
This branch has not been deployed
2 participants
Summary
Addresses #1433 for Pool allocations with a recorded source Pool.
Changing an allocated BatchSandbox from Pool A to Pool B previously made A recycle its in-use Pods, while stale allocation state prevented B from replacing them. The controller now keeps the existing allocation attached to the Pool recorded in
alloc-status, including after controller restart. It reports aPoolRefUpdateRejectedcondition and Warning event when the requested reference differs, including a change back to"*".The requested spec stays visible for the user to correct; the API update is accepted rather than rejected at admission. Restoring the recorded reference clears the condition. Initial binding, unallocated auto-assignment, and the existing explicit-detach/pause-resume flow remain supported.
The implementation removes the CEL transition rule from both generated and Helm CRDs, retaining Kubernetes 1.22 compatibility without introducing a webhook. Pool indexing, event routing, allocation sync/recovery, and finalizer cleanup consistently use the committed source. Annotation keys and JSON shapes are unchanged.
Legacy records without
poolRefretain existing behavior until the existing Pool-controller backfill verifies and upgrades them. This does not add live migration between Pools or an admission-time immutability guarantee.Testing
make test: complete Kubernetes unit/envtest suite passed (Kubernetes 1.33.0 assets), including existing pause/resume, detach, and legacy-backfill tests.BatchSandbox poolRef guardintegration specs passed. Both generated and Helm-rendered BatchSandbox CRDs also passed server-side dry-run apply on v1.22.4.make lint: passed; Helm lint/template and documentation build passed.Compatibility
No CEL rules or new admission component. The CRD change adds one status-condition enum value. The guard applies to committed Pool allocations; it does not claim to reject every possible pre-allocation spec transition.
AI assistance (OpenAI Codex) was used for this revision and local validation.