From 2377aaa6201f032a322ea17753d5477965673f8c Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 26 Jun 2026 15:58:56 -0700 Subject: [PATCH] pkg/readiness/crd_compat: Drop unnecessary CustomResourceDefinition check We grew this check recently in b0e4c90a3d (pkg/readiness: Add readiness checks and wire into proposal controller, 2026-05-27, #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]: https://github.com/openshift/cluster-kube-storage-version-migrator-operator/pull/90/changes/807c909d19d0fcde043d2d54550c34b3c5053c7e#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R7 [2]: https://github.com/kubernetes-sigs/kube-storage-version-migrator/blob/master/USER_GUIDE.md#check-if-migration-has-completed --- pkg/proposal/controller_test.go | 23 ++---- pkg/readiness/check.go | 1 - pkg/readiness/check_test.go | 6 +- pkg/readiness/checks_test.go | 124 +------------------------------- pkg/readiness/client.go | 1 - pkg/readiness/crd_compat.go | 63 ---------------- test/cvo/readiness.go | 2 +- 7 files changed, 12 insertions(+), 208 deletions(-) delete mode 100644 pkg/readiness/crd_compat.go diff --git a/pkg/proposal/controller_test.go b/pkg/proposal/controller_test.go index 1e211745a..288b58e95 100644 --- a/pkg/proposal/controller_test.go +++ b/pkg/proposal/controller_test.go @@ -1146,7 +1146,6 @@ func newFakeDynamicClient(objects ...runtime.Object) *dynamicfake.FakeDynamicCli readiness.GVRNode: "NodeList", readiness.GVRPod: "PodList", readiness.GVRPDB: "PodDisruptionBudgetList", - readiness.GVRCRD: "CustomResourceDefinitionList", readiness.GVRSubscription: "SubscriptionList", readiness.GVRCSV: "ClusterServiceVersionList", readiness.GVRInstallPlan: "InstallPlanList", @@ -1260,18 +1259,6 @@ func TestGetProposals_WithReadinessData(t *testing.T) { "conditions": []interface{}{map[string]interface{}{"type": "Deprecated", "status": "True"}}, }, }}, - // CRD with version issue - &unstructured.Unstructured{Object: map[string]interface{}{ - "apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{"name": "widgets.example.com"}, - "spec": map[string]interface{}{ - "versions": []interface{}{ - map[string]interface{}{"name": "v2", "served": true}, - map[string]interface{}{"name": "v1", "served": false}, - }, - }, - "status": map[string]interface{}{"storedVersions": []interface{}{"v1"}}, - }}, // Network, Proxy, APIServer &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "config.openshift.io/v1", "kind": "Network", @@ -1356,11 +1343,11 @@ func TestGetProposals_WithReadinessData(t *testing.T) { if !ok { t.Fatal("readiness output missing 'meta'") } - if meta["total_checks"] != float64(9) { - t.Errorf("readiness total_checks = %v, want 9", meta["total_checks"]) + if meta["total_checks"] != float64(8) { + t.Errorf("readiness total_checks = %v, want 8", meta["total_checks"]) } - if meta["checks_ok"] != float64(9) { - t.Errorf("readiness checks_ok = %v, want 9 (all checks should succeed)", meta["checks_ok"]) + if meta["checks_ok"] != float64(8) { + t.Errorf("readiness checks_ok = %v, want 8 (all checks should succeed)", meta["checks_ok"]) } checks, ok := raw["checks"].(map[string]any) @@ -1372,7 +1359,7 @@ func TestGetProposals_WithReadinessData(t *testing.T) { 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) if !ok { diff --git a/pkg/readiness/check.go b/pkg/readiness/check.go index 5e30d23ca..6ba51f0b7 100644 --- a/pkg/readiness/check.go +++ b/pkg/readiness/check.go @@ -73,7 +73,6 @@ var AllChecks = func() []Check { &PDBDrainCheck{}, // new: PDB drain blockers &EtcdHealthCheck{}, // new: deep etcd health (beyond CO condition) &NetworkCheck{}, // new: SDN migration, TLS, proxy - &CRDCompatCheck{}, // new: CRD version mismatches &OLMOperatorLifecycleCheck{}, // new: OLM operator lifecycle (OCPSTRAT-2618) // Known issues (Jira/KB) are NOT checked here — the agent uses its // redhat-support skill to query contextually based on readiness findings. diff --git a/pkg/readiness/check_test.go b/pkg/readiness/check_test.go index 73260a97b..6f53acd7e 100644 --- a/pkg/readiness/check_test.go +++ b/pkg/readiness/check_test.go @@ -242,8 +242,8 @@ func TestRunAllRecoversPanic(t *testing.T) { func TestAllChecksReturnsExpectedCount(t *testing.T) { checks := AllChecks() - if len(checks) != 9 { - t.Errorf("AllChecks() returned %d checks, want 9", len(checks)) + if len(checks) != 8 { + t.Errorf("AllChecks() returned %d checks, want 8", len(checks)) } names := make(map[string]bool) @@ -253,7 +253,7 @@ func TestAllChecksReturnsExpectedCount(t *testing.T) { expected := []string{ "cluster_conditions", "operator_health", "api_deprecations", - "node_capacity", "pdb_drain", "etcd_health", "network", "crd_compat", + "node_capacity", "pdb_drain", "etcd_health", "network", "olm_operator_lifecycle", } for _, name := range expected { diff --git a/pkg/readiness/checks_test.go b/pkg/readiness/checks_test.go index af2697637..be795e030 100644 --- a/pkg/readiness/checks_test.go +++ b/pkg/readiness/checks_test.go @@ -20,7 +20,6 @@ func newFakeDynamicClient(objects ...runtime.Object) *dynamicfake.FakeDynamicCli GVRNode: "NodeList", GVRPod: "PodList", GVRPDB: "PodDisruptionBudgetList", - GVRCRD: "CustomResourceDefinitionList", GVRSubscription: "SubscriptionList", GVRCSV: "ClusterServiceVersionList", GVRInstallPlan: "InstallPlanList", @@ -621,103 +620,6 @@ func TestAPIDeprecationsCheck_NoBlockers(t *testing.T) { } } -func TestCRDCompatCheck(t *testing.T) { - objects := []runtime.Object{ - // CRD with stored version that is still served — ok - &unstructured.Unstructured{Object: map[string]interface{}{ - "apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{"name": "widgets.example.com"}, - "spec": map[string]interface{}{ - "versions": []interface{}{ - map[string]interface{}{"name": "v1", "served": true}, - map[string]interface{}{"name": "v1beta1", "served": true}, - }, - }, - "status": map[string]interface{}{ - "storedVersions": []interface{}{"v1", "v1beta1"}, - }, - }}, - // CRD with stored version that is NO LONGER served — issue - &unstructured.Unstructured{Object: map[string]interface{}{ - "apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{"name": "gadgets.example.com"}, - "spec": map[string]interface{}{ - "versions": []interface{}{ - map[string]interface{}{"name": "v2", "served": true}, - map[string]interface{}{"name": "v1", "served": false}, - }, - }, - "status": map[string]interface{}{ - "storedVersions": []interface{}{"v1"}, - }, - }}, - } - - client := newFakeDynamicClient(objects...) - check := &CRDCompatCheck{} - - result, err := check.Run(context.Background(), client, "4.21.5", "4.21.8") - if err != nil { - t.Fatal(err) - } - - if result["total_crds"] != 2 { - t.Errorf("total_crds = %v, want 2", result["total_crds"]) - } - - issues, ok := result["version_issues"].([]map[string]any) - if !ok { - t.Fatal("version_issues not a slice") - } - if len(issues) != 1 { - t.Fatalf("version_issues len = %d, want 1", len(issues)) - } - if issues[0]["crd"] != "gadgets.example.com" { - t.Errorf("crd = %v, want gadgets.example.com", issues[0]["crd"]) - } - if issues[0]["stored_version"] != "v1" { - t.Errorf("stored_version = %v, want v1", issues[0]["stored_version"]) - } - - summary, ok := result["summary"].(map[string]any) - if !ok { - t.Fatal("summary not a map") - } - if summary["version_issues"] != 1 { - t.Errorf("summary.version_issues = %v, want 1", summary["version_issues"]) - } -} - -func TestCRDCompatCheck_NoIssues(t *testing.T) { - objects := []runtime.Object{ - &unstructured.Unstructured{Object: map[string]interface{}{ - "apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{"name": "things.example.com"}, - "spec": map[string]interface{}{ - "versions": []interface{}{ - map[string]interface{}{"name": "v1", "served": true}, - }, - }, - "status": map[string]interface{}{ - "storedVersions": []interface{}{"v1"}, - }, - }}, - } - - client := newFakeDynamicClient(objects...) - check := &CRDCompatCheck{} - - result, err := check.Run(context.Background(), client, "4.21.5", "4.21.8") - if err != nil { - t.Fatal(err) - } - - issues := result["version_issues"].([]map[string]any) - if len(issues) != 0 { - t.Errorf("expected no version issues, got %d", len(issues)) - } -} - func TestNetworkCheck(t *testing.T) { objects := []runtime.Object{ &unstructured.Unstructured{Object: map[string]interface{}{ @@ -912,19 +814,6 @@ func fakeClusterObjects() []runtime.Object { }, }}, - // --- CRDs (crd_compat) --- - &unstructured.Unstructured{Object: map[string]interface{}{ - "apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{"name": "widgets.example.com"}, - "spec": map[string]interface{}{ - "versions": []interface{}{ - map[string]interface{}{"name": "v1", "served": true}, - map[string]interface{}{"name": "v1beta1", "served": false}, - }, - }, - "status": map[string]interface{}{"storedVersions": []interface{}{"v1beta1"}}, - }}, - // --- Network, Proxy, APIServer (network) --- &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "config.openshift.io/v1", "kind": "Network", @@ -979,14 +868,14 @@ func TestRunAllWithFakeCluster(t *testing.T) { if output.TargetVersion != "4.21.8" { t.Errorf("TargetVersion = %q, want 4.21.8", output.TargetVersion) } - if output.Meta.TotalChecks != 9 { - t.Errorf("TotalChecks = %d, want 9", output.Meta.TotalChecks) + if output.Meta.TotalChecks != 8 { + t.Errorf("TotalChecks = %d, want 8", output.Meta.TotalChecks) } 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", } { r, ok := output.Checks[name] if !ok { @@ -1056,13 +945,6 @@ func TestRunAllWithFakeCluster(t *testing.T) { t.Errorf("api_deprecations blockers = %v, want 1", adSummary["blockers"]) } - // crd_compat: 1 CRD with stored version no longer served - crd := output.Checks["crd_compat"] - crdSummary := crd.Data["summary"].(map[string]any) - if crdSummary["version_issues"] != 1 { - t.Errorf("crd_compat version_issues = %v, want 1", crdSummary["version_issues"]) - } - // network: OVN, proxy configured nw := output.Checks["network"] if nw.Data["network_type"] != "OVNKubernetes" { diff --git a/pkg/readiness/client.go b/pkg/readiness/client.go index 121de91fd..01d9450f6 100644 --- a/pkg/readiness/client.go +++ b/pkg/readiness/client.go @@ -22,7 +22,6 @@ var ( GVRPDB = schema.GroupVersionResource{Group: "policy", Version: "v1", Resource: "poddisruptionbudgets"} GVRPV = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "persistentvolumes"} GVRSecret = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"} - GVRCRD = schema.GroupVersionResource{Group: "apiextensions.k8s.io", Version: "v1", Resource: "customresourcedefinitions"} GVRCSV = schema.GroupVersionResource{Group: "operators.coreos.com", Version: "v1alpha1", Resource: "clusterserviceversions"} GVRSubscription = schema.GroupVersionResource{Group: "operators.coreos.com", Version: "v1alpha1", Resource: "subscriptions"} GVRInstallPlan = schema.GroupVersionResource{Group: "operators.coreos.com", Version: "v1alpha1", Resource: "installplans"} diff --git a/pkg/readiness/crd_compat.go b/pkg/readiness/crd_compat.go deleted file mode 100644 index 723e9aa5e..000000000 --- a/pkg/readiness/crd_compat.go +++ /dev/null @@ -1,63 +0,0 @@ -package readiness - -import ( - "context" - "fmt" - - "k8s.io/client-go/dynamic" -) - -// CRDCompatCheck verifies CRD stored/served version compatibility and operator constraints. -type CRDCompatCheck struct{} - -func (c *CRDCompatCheck) Name() string { return "crd_compat" } - -func (c *CRDCompatCheck) Run(ctx context.Context, dc dynamic.Interface, current, target string) (map[string]any, error) { - result := map[string]any{} - - // Check CRDs for version mismatches - crds, err := ListResources(ctx, dc, GVRCRD, "") - if err != nil { - return nil, fmt.Errorf("failed to list CRDs: %w", err) - } - - versionIssues := make([]map[string]any, 0) - for _, crd := range crds { - storedVersions := NestedSlice(crd.Object, "status", "storedVersions") - servedVersions := NestedSlice(crd.Object, "spec", "versions") - - served := make(map[string]bool) - for _, v := range servedVersions { - vm, ok := v.(map[string]interface{}) - if !ok { - continue - } - name := NestedString(vm, "name") - isServed := NestedBool(vm, "served") - if isServed { - served[name] = true - } - } - - for _, sv := range storedVersions { - stored, _ := sv.(string) - if stored != "" && !served[stored] { - versionIssues = append(versionIssues, map[string]any{ - "crd": crd.GetName(), - "stored_version": stored, - "issue": "stored version no longer served", - }) - } - } - } - - result["total_crds"] = len(crds) - result["version_issues"] = versionIssues - - result["summary"] = map[string]any{ - "total_crds": len(crds), - "version_issues": len(versionIssues), - } - - return result, nil -} diff --git a/test/cvo/readiness.go b/test/cvo/readiness.go index 96652b54f..8d296447c 100644 --- a/test/cvo/readiness.go +++ b/test/cvo/readiness.go @@ -64,7 +64,7 @@ var _ = g.Describe(`[Jira:"Cluster Version Operator"] cluster-version-operator r g.It("should run all checks without errors", func() { output := readiness.RunAll(ctx, dynamicClient, currentVersion, targetVersion) - o.Expect(output.Meta.TotalChecks).To(o.Equal(9)) + o.Expect(output.Meta.TotalChecks).To(o.Equal(8)) o.Expect(output.Meta.ChecksErrored).To(o.Equal(0), "no check should error on a healthy cluster") })