Skip to content

Commit d94dfe3

Browse files
harcheclaude
harche
and
claude
committed
pkg/readiness: Address CodeRabbit review findings
- Fix fence index validation before offset math in test JSON extraction - Log when dynamicClient is nil in runReadinessJSON fallback path - Add nil guard in SectionError to prevent panic on nil error - Fix stale comment "upstream" -> "cluster identity" in cluster_conditions - Use Pod Ready condition instead of phase for etcd member health - Make SDN deprecation warning target-version aware (blocker vs advisory) - Scope PackageManifest lookup by sourceNamespace for multi-catalog clusters - Use json.RawMessage for olmProperty.Value to handle non-string values - Use NestedFieldNoCopy for PDB maxUnavailable/minAvailable (IntOrString) - Use timeout-bound context in e2e tests instead of context.TODO() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e98b557 commit d94dfe3

11 files changed

Lines changed: 66 additions & 30 deletions

File tree

‎pkg/proposal/controller.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,7 @@ func classifyUpdate(current, target string) string {
450450

451451
func runReadinessJSON(ctx context.Context, dynamicClient dynamic.Interface, currentVersion, targetVersion string) string {
452452
if dynamicClient == nil {
453+
klog.V(i.Normal).Infof("Dynamic client is nil; skipping readiness checks for %s -> %s", currentVersion, targetVersion)
453454
return "{}"
454455
}
455456
output := readiness.RunAll(ctx, dynamicClient, currentVersion, targetVersion)

‎pkg/proposal/controller_test.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1327,17 +1327,17 @@ func TestGetProposals_WithReadinessData(t *testing.T) {
13271327
&unstructured.Unstructured{Object: map[string]interface{}{
13281328
"apiVersion": "v1", "kind": "Pod",
13291329
"metadata": map[string]interface{}{"name": "etcd-master-0", "namespace": "openshift-etcd", "labels": map[string]interface{}{"app": "etcd"}},
1330-
"spec": map[string]interface{}{"nodeName": "master-0"}, "status": map[string]interface{}{"phase": "Running"},
1330+
"spec": map[string]interface{}{"nodeName": "master-0"}, "status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{map[string]interface{}{"type": "Ready", "status": "True"}}},
13311331
}},
13321332
&unstructured.Unstructured{Object: map[string]interface{}{
13331333
"apiVersion": "v1", "kind": "Pod",
13341334
"metadata": map[string]interface{}{"name": "etcd-master-1", "namespace": "openshift-etcd", "labels": map[string]interface{}{"app": "etcd"}},
1335-
"spec": map[string]interface{}{"nodeName": "master-1"}, "status": map[string]interface{}{"phase": "Running"},
1335+
"spec": map[string]interface{}{"nodeName": "master-1"}, "status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{map[string]interface{}{"type": "Ready", "status": "True"}}},
13361336
}},
13371337
&unstructured.Unstructured{Object: map[string]interface{}{
13381338
"apiVersion": "v1", "kind": "Pod",
13391339
"metadata": map[string]interface{}{"name": "etcd-master-2", "namespace": "openshift-etcd", "labels": map[string]interface{}{"app": "etcd"}},
1340-
"spec": map[string]interface{}{"nodeName": "master-2"}, "status": map[string]interface{}{"phase": "Running"},
1340+
"spec": map[string]interface{}{"nodeName": "master-2"}, "status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{map[string]interface{}{"type": "Ready", "status": "True"}}},
13411341
}},
13421342
// Nodes
13431343
&unstructured.Unstructured{Object: map[string]interface{}{
@@ -1433,10 +1433,14 @@ func TestGetProposals_WithReadinessData(t *testing.T) {
14331433
}
14341434

14351435
// Extract JSON from the request
1436-
jsonStart := strings.Index(request, "```json\n") + len("```json\n")
1436+
start := strings.Index(request, "```json\n")
1437+
if start < 0 {
1438+
t.Fatal("could not find readiness JSON fence in request")
1439+
}
1440+
jsonStart := start + len("```json\n")
14371441
jsonEnd := strings.Index(request[jsonStart:], "\n```")
1438-
if jsonStart < 0 || jsonEnd < 0 {
1439-
t.Fatal("could not extract readiness JSON from request")
1442+
if jsonEnd < 0 {
1443+
t.Fatal("could not find closing fence for readiness JSON")
14401444
}
14411445
readinessJSON := request[jsonStart : jsonStart+jsonEnd]
14421446

‎pkg/readiness/check.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,9 @@ func RunAll(ctx context.Context, c dynamic.Interface, current, target string) *O
152152

153153
// SectionError appends a section error entry to the errors slice.
154154
func SectionError(errors *[]map[string]any, section string, err error) {
155+
if err == nil {
156+
return
157+
}
155158
*errors = append(*errors, map[string]any{
156159
"section": section,
157160
"error": err.Error(),

‎pkg/readiness/checks_test.go‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -166,22 +166,28 @@ func TestEtcdHealthCheck(t *testing.T) {
166166
"apiVersion": "v1", "kind": "Pod",
167167
"metadata": map[string]interface{}{"name": "etcd-master-0", "namespace": "openshift-etcd",
168168
"labels": map[string]interface{}{"app": "etcd"}},
169-
"spec": map[string]interface{}{"nodeName": "master-0"},
170-
"status": map[string]interface{}{"phase": "Running"},
169+
"spec": map[string]interface{}{"nodeName": "master-0"},
170+
"status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{
171+
map[string]interface{}{"type": "Ready", "status": "True"},
172+
}},
171173
}},
172174
&unstructured.Unstructured{Object: map[string]interface{}{
173175
"apiVersion": "v1", "kind": "Pod",
174176
"metadata": map[string]interface{}{"name": "etcd-master-1", "namespace": "openshift-etcd",
175177
"labels": map[string]interface{}{"app": "etcd"}},
176-
"spec": map[string]interface{}{"nodeName": "master-1"},
177-
"status": map[string]interface{}{"phase": "Running"},
178+
"spec": map[string]interface{}{"nodeName": "master-1"},
179+
"status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{
180+
map[string]interface{}{"type": "Ready", "status": "True"},
181+
}},
178182
}},
179183
&unstructured.Unstructured{Object: map[string]interface{}{
180184
"apiVersion": "v1", "kind": "Pod",
181185
"metadata": map[string]interface{}{"name": "etcd-master-2", "namespace": "openshift-etcd",
182186
"labels": map[string]interface{}{"app": "etcd"}},
183-
"spec": map[string]interface{}{"nodeName": "master-2"},
184-
"status": map[string]interface{}{"phase": "Failed"},
187+
"spec": map[string]interface{}{"nodeName": "master-2"},
188+
"status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{
189+
map[string]interface{}{"type": "Ready", "status": "False"},
190+
}},
185191
}},
186192
}
187193

@@ -836,17 +842,17 @@ func fakeClusterObjects() []runtime.Object {
836842
&unstructured.Unstructured{Object: map[string]interface{}{
837843
"apiVersion": "v1", "kind": "Pod",
838844
"metadata": map[string]interface{}{"name": "etcd-master-0", "namespace": "openshift-etcd", "labels": map[string]interface{}{"app": "etcd"}},
839-
"spec": map[string]interface{}{"nodeName": "master-0"}, "status": map[string]interface{}{"phase": "Running"},
845+
"spec": map[string]interface{}{"nodeName": "master-0"}, "status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{map[string]interface{}{"type": "Ready", "status": "True"}}},
840846
}},
841847
&unstructured.Unstructured{Object: map[string]interface{}{
842848
"apiVersion": "v1", "kind": "Pod",
843849
"metadata": map[string]interface{}{"name": "etcd-master-1", "namespace": "openshift-etcd", "labels": map[string]interface{}{"app": "etcd"}},
844-
"spec": map[string]interface{}{"nodeName": "master-1"}, "status": map[string]interface{}{"phase": "Running"},
850+
"spec": map[string]interface{}{"nodeName": "master-1"}, "status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{map[string]interface{}{"type": "Ready", "status": "True"}}},
845851
}},
846852
&unstructured.Unstructured{Object: map[string]interface{}{
847853
"apiVersion": "v1", "kind": "Pod",
848854
"metadata": map[string]interface{}{"name": "etcd-master-2", "namespace": "openshift-etcd", "labels": map[string]interface{}{"app": "etcd"}},
849-
"spec": map[string]interface{}{"nodeName": "master-2"}, "status": map[string]interface{}{"phase": "Running"},
855+
"spec": map[string]interface{}{"nodeName": "master-2"}, "status": map[string]interface{}{"phase": "Running", "conditions": []interface{}{map[string]interface{}{"type": "Ready", "status": "True"}}},
850856
}},
851857

852858
// --- Nodes (node_capacity) ---

‎pkg/readiness/cluster_conditions.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func (c *ClusterConditionsCheck) Run(ctx context.Context, dc dynamic.Interface,
6161
}
6262
result["recent_history"] = historyEntries
6363

64-
// Channel and upstream
64+
// Channel and cluster identity
6565
result["channel"] = NestedString(cv.Object, "spec", "channel")
6666
result["cluster_id"] = NestedString(cv.Object, "spec", "clusterID")
6767

‎pkg/readiness/etcd_health.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,11 @@ func (c *EtcdHealthCheck) Run(ctx context.Context, dc dynamic.Interface, current
3434
healthyMembers := 0
3535
for _, pod := range etcdPods {
3636
phase := NestedString(pod.Object, "status", "phase")
37-
ready := phase == "Running"
37+
podConds := GetConditions(&pod)
38+
ready := false
39+
if cond, ok := podConds["Ready"]; ok {
40+
ready = cond.Status == ConditionTrue
41+
}
3842
if ready {
3943
healthyMembers++
4044
}

‎pkg/readiness/network.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ func (c *NetworkCheck) Run(ctx context.Context, dc dynamic.Interface, current, t
2727

2828
// SDN deprecation warning
2929
if networkType == "OpenShiftSDN" {
30-
result["sdn_warning"] = "OpenShiftSDN is deprecated. Migration to OVN-Kubernetes is required before 4.17+."
30+
if target != "" && CompareVersions(target, "4.17.0") >= 0 {
31+
result["sdn_warning"] = "OpenShiftSDN blocks upgrades to 4.17+; migrate to OVN-Kubernetes first."
32+
} else {
33+
result["sdn_warning"] = "OpenShiftSDN detected. Migration to OVN-Kubernetes is required for future upgrades to 4.17+."
34+
}
3135
}
3236

3337
// Check proxy

‎pkg/readiness/olm_lifecycle.go‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func (c *OLMOperatorLifecycleCheck) Run(ctx context.Context, dc dynamic.Interfac
6262
}
6363

6464
csvIndex := indexByNamespacedName(csvs)
65-
pkgIndex := indexByName(pkgManifests)
65+
pkgIndex := indexByNamespacedName(pkgManifests)
6666

6767
// Parse current/target once to avoid repeated semver parsing per operator.
6868
parsedTarget, errTarget := semver.ParseTolerant(target)
@@ -141,7 +141,11 @@ func (c *OLMOperatorLifecycleCheck) Run(ctx context.Context, dc dynamic.Interfac
141141

142142
pkgName := NestedString(sub.Object, "spec", "name")
143143
subChannel := NestedString(sub.Object, "spec", "channel")
144-
if pm, ok := pkgIndex[pkgName]; ok {
144+
pkgNS := NestedString(sub.Object, "spec", "sourceNamespace")
145+
if pkgNS == "" {
146+
pkgNS = sub.GetNamespace()
147+
}
148+
if pm, ok := pkgIndex[pkgNS+"/"+pkgName]; ok {
145149
compat := extractOCPCompat(pm, subChannel)
146150
if compat != nil {
147151
entry["ocp_compat"] = compat
@@ -247,20 +251,22 @@ func extractOCPCompat(pm map[string]interface{}, channelName string) map[string]
247251

248252
// olmProperty represents a single entry in the olm.properties JSON annotation.
249253
type olmProperty struct {
250-
Type string `json:"type"`
251-
Value string `json:"value"`
254+
Type string `json:"type"`
255+
Value json.RawMessage `json:"value"`
252256
}
253257

254-
// parseMinOCPFromProperties extracts the minimum OCP version from the olm.properties
255-
// JSON annotation, which is a JSON array of {type, value} objects.
256258
func parseMinOCPFromProperties(props string) string {
257259
var properties []olmProperty
258260
if err := json.Unmarshal([]byte(props), &properties); err != nil {
259261
return ""
260262
}
261263
for _, p := range properties {
262264
if p.Type == "olm.minOpenShiftVersion" {
263-
return p.Value
265+
var v string
266+
if err := json.Unmarshal(p.Value, &v); err == nil {
267+
return v
268+
}
269+
return ""
264270
}
265271
}
266272
return ""

‎pkg/readiness/olm_lifecycle_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func TestOLMOperatorLifecycleCheck_Basic(t *testing.T) {
4242
// PackageManifest for elasticsearch-operator
4343
&unstructured.Unstructured{Object: map[string]interface{}{
4444
"apiVersion": "packages.operators.coreos.com/v1", "kind": "PackageManifest",
45-
"metadata": map[string]interface{}{"name": "elasticsearch-operator"},
45+
"metadata": map[string]interface{}{"name": "elasticsearch-operator", "namespace": "openshift-marketplace"},
4646
"status": map[string]interface{}{
4747
"channels": []interface{}{
4848
map[string]interface{}{
@@ -235,7 +235,7 @@ func TestOLMOperatorLifecycleCheck_IncompatibleWithTarget(t *testing.T) {
235235
}},
236236
&unstructured.Unstructured{Object: map[string]interface{}{
237237
"apiVersion": "packages.operators.coreos.com/v1", "kind": "PackageManifest",
238-
"metadata": map[string]interface{}{"name": "jaeger-product"},
238+
"metadata": map[string]interface{}{"name": "jaeger-product", "namespace": "openshift-operators"},
239239
"status": map[string]interface{}{
240240
"channels": []interface{}{
241241
map[string]interface{}{

‎pkg/readiness/pdb_drain.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66

7+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
78
"k8s.io/client-go/dynamic"
89
)
910

@@ -23,8 +24,10 @@ func (c *PDBDrainCheck) Run(ctx context.Context, dc dynamic.Interface, current,
2324
issues := make([]map[string]any, 0)
2425
for _, pdb := range pdbs {
2526
// Check for zero-disruption PDBs
26-
maxUnavailable := NestedString(pdb.Object, "spec", "maxUnavailable")
27-
minAvailable := NestedString(pdb.Object, "spec", "minAvailable")
27+
maxUnavailableRaw, _, _ := unstructured.NestedFieldNoCopy(pdb.Object, "spec", "maxUnavailable")
28+
maxUnavailable := fmt.Sprintf("%v", maxUnavailableRaw)
29+
minAvailableRaw, _, _ := unstructured.NestedFieldNoCopy(pdb.Object, "spec", "minAvailable")
30+
minAvailable := fmt.Sprintf("%v", minAvailableRaw)
2831

2932
currentHealthy := NestedInt64(pdb.Object, "status", "currentHealthy")
3033
desiredHealthy := NestedInt64(pdb.Object, "status", "desiredHealthy")

0 commit comments

Comments
 (0)