OTA-1966: pkg/readiness/crd_compat: Drop unnecessary CustomResourceDefinition check#1415
Conversation
…heck We grew this check recently in b0e4c90 (pkg/readiness: Add readiness checks and wire into proposal controller, 2026-05-27, openshift#1395). But I don't think we need it. I agree with the guard, that having a CRD with storage versions that were not served would be weird, and probably not what the CRD maintainers intended. But what would a cluster-admin be expected to do about it? There's nothing in the code I'm removing here, or in https://github.com/openshift/agentic-skills as far as I can see, that turns "stored version no longer served" into an actionable plan. In the CRD-evolution space, OpenShift packages the Kube storage-version migrator. But we don't run the trigger logic [1]. And, as far as I can tell, the migration operator isn't checking to see if any StorageVersionMigrations are completed [2]. I expect that, if OpenShift ever decides to remove support for a previously-stored version, we'll set up something to run (and require the completion of) a StorageVersionMigration to ensure storage is off the outgoing version before we leave the last release where it was served. So that would protect OpenShift users from OpenShift-managed CRD evolution, without needing the guard I'm deleting In addition to OpenShift-managed CRDs, there might be many CRDs on an OpenShift cluster that are not part of the OpenShift release payload. They may have been installed by OLM-installed operators. They may have been installed by individual cluster users running 'oc apply ...'. Lots of other folks who could be maintaining CRDs on the cluster. We don't want to confuse admins who are considering an OpenShift version update by any issues that might be going on with those non-OpenShift CRDs. [1]: openshift/cluster-kube-storage-version-migrator-operator@807c909#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R7 [2]: https://github.com/kubernetes-sigs/kube-storage-version-migrator/blob/master/USER_GUIDE.md#check-if-migration-has-completed
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: This pull request references OTA-1966 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe PR removes the CRD compatibility readiness check from the readiness registry and updates tests and fixtures to expect one fewer readiness check and no ChangesReadiness compatibility check removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/proposal/controller_test.go`:
- Around line 1359-1364: The aggregate readiness expectations in
controller_test.go are still asserting 9 checks even though the updated check
list in the readiness loop no longer includes crd_compat and RunAll() now
reports 8 total checks. Update the assertions around meta["total_checks"] and
meta["checks_ok"] to match the new 8-check aggregate, and verify the related
readiness metadata expectations stay consistent with the check names used in the
loop and RunAll().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6d648dc8-afba-438c-8ecb-27c1a63781a5
📒 Files selected for processing (7)
pkg/proposal/controller_test.gopkg/readiness/check.gopkg/readiness/check_test.gopkg/readiness/checks_test.gopkg/readiness/client.gopkg/readiness/crd_compat.gotest/cvo/readiness.go
💤 Files with no reviewable changes (3)
- pkg/readiness/client.go
- pkg/readiness/crd_compat.go
- pkg/readiness/check.go
| for _, name := range []string{ | ||
| "cluster_conditions", "operator_health", "api_deprecations", | ||
| "node_capacity", "pdb_drain", "etcd_health", "network", | ||
| "crd_compat", "olm_operator_lifecycle", | ||
| "olm_operator_lifecycle", | ||
| } { | ||
| check, ok := checks[name].(map[string]any) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the aggregate readiness assertions to 8 as well.
This loop drops crd_compat, but the same test still expects meta["total_checks"] and meta["checks_ok"] to be 9 on Lines 1346-1350. RunAll() now reports 8 checks, so this test will fail even with the new check list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/proposal/controller_test.go` around lines 1359 - 1364, The aggregate
readiness expectations in controller_test.go are still asserting 9 checks even
though the updated check list in the readiness loop no longer includes
crd_compat and RunAll() now reports 8 total checks. Update the assertions around
meta["total_checks"] and meta["checks_ok"] to match the new 8-check aggregate,
and verify the related readiness metadata expectations stay consistent with the
check names used in the loop and RunAll().
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
We grew this check recently in b0e4c90 (#1395). But I don't think we need it. I agree with the guard, having a CRD with storage versions that were not served would be weird, and probably not what the CRD maintainers intended. But what would a cluster-admin be expected to do about it? There's nothing in the code I'm removing here, or in
https://github.com/openshift/agentic-skills as far as I can see, that turns
stored version no longer servedinto an actionable plan.In the CRD-evolution space, OpenShift packages the Kube storage-version migrator. But we don't run the trigger logic. And, as far as I can tell, the migration operator isn't checking to see if any StorageVersionMigrations are completed. I expect that, if OpenShift ever decides to remove support for a previously-stored version, we'll set up something to run (and require the completion of) a StorageVersionMigration to ensure storage is off the outgoing version before we leave the last release where it was served. So that would protect OpenShift users from OpenShift-managed CRD evolution, without needing the guard I'm deleting.
In addition to OpenShift-managed CRDs, there might be many CRDs on an OpenShift cluster that are not part of the OpenShift release payload. They may have been installed by OLM-installed operators. They may have been installed by individual cluster users running
oc apply .... Lots of other folks who could be maintaining CRDs on the cluster. We don't want to confuse admins who are considering an OpenShift version update by any issues that might be going on with those non-OpenShift CRDs.Summary by CodeRabbit
Bug Fixes
Tests