WIP: pkg/readiness: Add readiness checks and wire into proposal controller#1395
Open
harche wants to merge 1 commit into
Open
WIP: pkg/readiness: Add readiness checks and wire into proposal controller#1395harche wants to merge 1 commit into
harche wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds dynamic client wiring to CVO and proposal controller; implements a readiness framework (9 checks) with client helpers, tests, schema, proposal integration embedding per-target readiness JSON, e2e readiness tests, and CI payload entries. ChangesCluster Readiness Checks
Sequence DiagramssequenceDiagram
participant CVO as cvo.Operator
participant RunAll as readiness.RunAll
participant Checks as Check (multiple)
participant Dyn as dynamic.Interface
CVO->>RunAll: RunAll(ctx, Dyn, current, target)
RunAll->>Checks: Run(ctx, Dyn, current, target) (concurrent)
Checks->>Dyn: GetResource/ListResources calls
Checks-->>RunAll: per-check map[string]any
RunAll-->>CVO: *Output{Checks, Meta}
sequenceDiagram
participant Sync as proposal.Sync
participant getProposals as getProposals
participant RunAll as readiness.RunAll
participant LLM as LLM service
participant Dyn as dynamic.Interface
Sync->>getProposals: invoke getProposals(ctx, ...)
loop per-target
getProposals->>RunAll: RunAll(ctx, Dyn, current, target)
RunAll-->>getProposals: readiness Output
getProposals->>getProposals: marshal readiness Output to JSON
getProposals->>LLM: POST proposal including readiness JSON
LLM-->>getProposals: proposals
end
getProposals-->>Sync: proposals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harche The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
test/cvo/readiness.go (1)
20-20: ⚡ Quick winAdd Ginkgo labels to the new e2e suite.
This new suite has no
Label(...), which makes targeted execution/selection harder in CI and local runs.As per coding guidelines, "Use Ginkgo Labels to mark test categories (e.g., TechPreview, serial)".
🤖 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 `@test/cvo/readiness.go` at line 20, The new Ginkgo suite declaration using g.Describe(`[Jira:"Cluster Version Operator"] cluster-version-operator readiness checks`, func() {...}) is missing Labels; update that Describe invocation to include appropriate Ginkgo Label options (e.g., g.Label("e2e"), g.Label("serial"), g.Label("TechPreview") or other team-standard tags) by adding g.Label(...) into the Describe call so the suite can be targeted in CI and local runs while keeping the existing description string and test body unchanged.pkg/proposal/controller.go (1)
292-312: 🏗️ Heavy liftAvoid re-running cluster-wide readiness checks for every target version.
Line 292 notes this already: 7 of 9 checks are target-independent, but Line 300 and Line 311 execute all checks per target. This can make sync latency scale with update count and slow reconciliation on clusters with many targets.
🤖 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.go` around lines 292 - 312, The loop currently calls runReadinessJSON for every target, re-running cluster-wide checks repeatedly; refactor so you compute target-independent readiness once and reuse it for all targets while still running only the target-dependent checks per target. Concretely: call a new or adjusted function (e.g., runClusterWideReadiness or runReadinessJSON(ctx,dynamicClient,currentVersion,"") returning baseReadinessJSON) before iterating availableUpdates/conditionalUpdates, then inside each loop call a lightweight runTargetedReadiness (or runReadinessJSON with a flag) that merges target-specific checks (api_deprecations, olm_lifecycle) into the base JSON, and pass the merged readinessJSON to getProposal; keep collecting errs and proposals as before.pkg/readiness/checks_test.go (1)
501-612: ⚡ Quick winUse table-driven subtests for paired scenarios of the same check.
TestAPIDeprecationsCheck/TestAPIDeprecationsCheck_NoBlockersandTestCRDCompatCheck/TestCRDCompatCheck_NoIssuesare the same test shape with different fixtures/expected values; folding them into one table-driven test per check will reduce duplication and future drift.As per coding guidelines, "Prefer table-driven tests over multiple similar test functions. If two test functions differ only in setup values, collapse them into one function with test-case tuples".
Also applies to: 613-708
🤖 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/readiness/checks_test.go` around lines 501 - 612, Collapse the two tests TestAPIDeprecationsCheck and TestAPIDeprecationsCheck_NoBlockers into a single table-driven test: define a slice of test cases each containing the name, input objects (runtime.Object slice), from/to versions, and expected counts/entries, then iterate cases with t.Run; inside each subtest create the fake client via newFakeDynamicClient, instantiate APIDeprecationsCheck, call check.Run and assert blocker_apis, warning_apis and summary values for that case. Do the same refactor for TestCRDCompatCheck/TestCRDCompatCheck_NoIssues (and the other paired tests mentioned) so each check has one test function that loops over subtests and performs per-case setup and assertions, preserving the existing assertions but scoped to the subtest.
🤖 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 1436-1441: The code currently computes jsonStart by adding
len("```json\n") before checking for a missing fence which can produce incorrect
slices; change the logic in the test to first find start :=
strings.Index(request, "```json\n") and if start < 0 call t.Fatal, then set
jsonStart := start + len("```json\n"), compute jsonEnd :=
strings.Index(request[jsonStart:], "\n```") and if jsonEnd < 0 call t.Fatal, and
finally extract readinessJSON using readinessJSON := request[jsonStart :
jsonStart+jsonEnd]; update references to jsonStart/jsonEnd accordingly.
In `@pkg/proposal/controller.go`:
- Around line 451-454: In runReadinessJSON, when dynamicClient == nil the
function currently returns "{}" silently; add a log entry (info or warning)
before returning that includes the function name and the currentVersion and
targetVersion to make the fallback visible; obtain a logger from the ctx if your
project uses context-based logging (or use the package logger used elsewhere in
this file) and emit something like "dynamicClient nil, returning empty readiness
JSON" with versions attached, then return "{}".
In `@pkg/readiness/check_test.go`:
- Around line 150-224: TestRunAllMixedResults reimplements orchestration instead
of calling RunAll; change it to exercise the real RunAll path by injecting the
fake checks via the AllChecks helper. Replace the manual loop with code that
temporarily overrides the AllChecks function (or uses any existing injection
point) to return []Check{okCheck, failCheck, partialCheck}, call
RunAll(context.Background(), nil, "4.21.5", "4.21.8"), then restore AllChecks
and assert counts and individual CheckResult entries (referencing
TestRunAllMixedResults, RunAll, AllChecks, and fakeCheck to locate the code).
Ensure the test still verifies passing, failing, and partial results and
restores the original AllChecks to avoid test pollution.
In `@pkg/readiness/check.go`:
- Around line 154-158: The SectionError function dereferences err with
err.Error() which will panic if err == nil; update SectionError to guard against
nil by checking if err == nil before calling Error(), and append a safe
representation (e.g. nil or an empty string like "") into the map under the
"error" key when err is nil so the function never calls err.Error() on a nil
error; keep the signature and the append to the errors slice intact while using
this conditional branch inside SectionError.
In `@pkg/readiness/client.go`:
- Around line 154-162: The CompareVersions function currently hides parse errors
by returning 0 on semver.ParseTolerant failures, conflating invalid input with
equal versions; change CompareVersions to return (int, error) (or (int, bool)
validity) instead of just int, and propagate parse errors from
semver.ParseTolerant for both va and vb rather than returning 0—e.g., update the
function signature and replace the two error branches so they return an
appropriate error when semver.ParseTolerant(a) or semver.ParseTolerant(b) fails,
then perform the comparison only after successful parses and return the int
comparison plus nil error (or true validity).
In `@pkg/readiness/cluster_conditions.go`:
- Around line 64-67: Update the stale comment that mentions “upstream” to
accurately describe the emitted fields by reflecting that this block populates
the result map with "channel" and "cluster_id" (i.e., keys result["channel"]
from NestedString(cv.Object, "spec", "channel") and result["cluster_id"] from
NestedString(cv.Object, "spec", "clusterID")); edit the nearby comment in
pkg/readiness/cluster_conditions.go so it no longer references upstream and
instead describes that the code extracts channel and cluster_id from the spec.
In `@pkg/readiness/etcd_health.go`:
- Around line 36-38: The current readiness logic uses phase :=
NestedString(pod.Object, "status", "phase") and treats ready := phase ==
"Running", which is wrong because a Running pod can still be NotReady; change
the check to inspect the pod's status.conditions for the condition with type
"Ready" and set ready only if that condition's status == "True". In practice,
replace the phase-based check in the function handling pod health (where
variables pod.Object, phase, and ready are used) with logic that iterates
status.conditions (or reads the Ready condition via the existing helpers) and
assigns ready = (Ready condition == "True") before updating healthy_members.
In `@pkg/readiness/network.go`:
- Around line 29-31: The current code always sets result["sdn_warning"] when
networkType == "OpenShiftSDN"; change it to only add the warning when the
requested target version crosses the deprecation boundary (e.g., targetVersion
>= 4.17). In pkg/readiness/network.go check networkType == "OpenShiftSDN" AND
compare the requested targetVersion (variable name targetVersion or equivalent)
using a semantic version compare (e.g., golang.org/x/mod/semver or a semver
package) and only set result["sdn_warning"] when the parsed/comparable
targetVersion is >= "4.17"; if targetVersion is not available in scope, add it
as a parameter to the function that builds result or obtain it from the existing
context before performing the conditional. Ensure you use explicit semver
comparison rather than string comparison to avoid incorrect matches.
In `@pkg/readiness/olm_lifecycle.go`:
- Around line 248-252: The current olmProperty type uses Value string which
makes json.Unmarshal fail if any element's "value" is non-string; change the
struct used by parseMinOCPFromProperties so Value is json.RawMessage (or
interface{}) and then in parseMinOCPFromProperties iterate each olmProperty,
attempt to decode Value as a string (falling back to converting numbers/objects
to string safely) and only consider entries where Type ==
"olm.minOpenShiftVersion" and the extracted string is valid; update references
to olmProperty and the parseMinOCPFromProperties logic to handle the new Value
type and avoid returning "" on unmarshal errors caused by non-string values.
- Around line 64-66: pkgIndex is currently built with indexByName and then
looked up by pkgName which ignores catalog/source namespace and can
mis-correlate PackageManifests; change construction to use indexByNamespacedName
and key lookups by the PackageManifest's namespaced name (use
spec.sourceNamespace) instead of plain name (replace indexByName ->
indexByNamespacedName where pkgIndex is created and any pkgIndex[pkgName]
lookups). Also harden parseMinOCPFromProperties to accept non-string
olm.properties values by unmarshaling property.Value as json.RawMessage (or
interface{}) and only treat it as a string when the property.Name ==
"olm.minOpenShiftVersion"; extract the string safely (e.g., unquote or decode
JSON string) and add a test with an object-valued property to ensure non-string
values do not cause json.Unmarshal to fail or return an empty minOCP.
In `@pkg/readiness/pdb_drain.go`:
- Around line 26-27: The code currently uses NestedString(pdb.Object, "spec",
"maxUnavailable") / "minAvailable" which loses IntOrString forms; replace these
calls with unstructured.NestedFieldNoCopy(pdb.Object, "spec", "maxUnavailable")
and unstructured.NestedFieldNoCopy(pdb.Object, "spec", "minAvailable") to fetch
the raw interface{}, then convert that raw value into a preserved string
representation (e.g., if it's already a string keep it, if it's numeric or a
JSON number use fmt.Sprintf("%v", v), or parse into
k8s.io/apimachinery/pkg/util/intstr.IntOrString when needed) so the original
IntOrString value is preserved when reporting; reference the symbols
NestedFieldNoCopy, pdb.Object, maxUnavailable, minAvailable, and
intstr.IntOrString when making the change.
In `@test/cvo/readiness.go`:
- Around line 25-54: Replace the use of context.TODO() (the ctx variable
declared at top and used in BeforeEach and later calls such as readiness.RunAll)
with a timeout-bound context created via context.WithTimeout inside BeforeEach
(e.g., ctx, cancel := context.WithTimeout(context.Background(),
<sensibleDuration>)) and ensure you defer cancel() in the same scope so all
cluster API calls (configClient.ClusterVersions().Get, dynamic/kube client
calls, and readiness.RunAll) use this cancelable context to avoid hanging tests;
update any other test helpers that reference the package-level ctx to accept or
use this new timeout context.
---
Nitpick comments:
In `@pkg/proposal/controller.go`:
- Around line 292-312: The loop currently calls runReadinessJSON for every
target, re-running cluster-wide checks repeatedly; refactor so you compute
target-independent readiness once and reuse it for all targets while still
running only the target-dependent checks per target. Concretely: call a new or
adjusted function (e.g., runClusterWideReadiness or
runReadinessJSON(ctx,dynamicClient,currentVersion,"") returning
baseReadinessJSON) before iterating availableUpdates/conditionalUpdates, then
inside each loop call a lightweight runTargetedReadiness (or runReadinessJSON
with a flag) that merges target-specific checks (api_deprecations,
olm_lifecycle) into the base JSON, and pass the merged readinessJSON to
getProposal; keep collecting errs and proposals as before.
In `@pkg/readiness/checks_test.go`:
- Around line 501-612: Collapse the two tests TestAPIDeprecationsCheck and
TestAPIDeprecationsCheck_NoBlockers into a single table-driven test: define a
slice of test cases each containing the name, input objects (runtime.Object
slice), from/to versions, and expected counts/entries, then iterate cases with
t.Run; inside each subtest create the fake client via newFakeDynamicClient,
instantiate APIDeprecationsCheck, call check.Run and assert blocker_apis,
warning_apis and summary values for that case. Do the same refactor for
TestCRDCompatCheck/TestCRDCompatCheck_NoIssues (and the other paired tests
mentioned) so each check has one test function that loops over subtests and
performs per-case setup and assertions, preserving the existing assertions but
scoped to the subtest.
In `@test/cvo/readiness.go`:
- Line 20: The new Ginkgo suite declaration using g.Describe(`[Jira:"Cluster
Version Operator"] cluster-version-operator readiness checks`, func() {...}) is
missing Labels; update that Describe invocation to include appropriate Ginkgo
Label options (e.g., g.Label("e2e"), g.Label("serial"), g.Label("TechPreview")
or other team-standard tags) by adding g.Label(...) into the Describe call so
the suite can be targeted in CI and local runs while keeping the existing
description string and test body unchanged.
🪄 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: 0f81fe59-c00a-4053-8218-57d36ea17f93
📒 Files selected for processing (22)
pkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/proposal/controller.gopkg/proposal/controller_test.gopkg/readiness/api_deprecations.gopkg/readiness/check.gopkg/readiness/check_test.gopkg/readiness/checks_test.gopkg/readiness/client.gopkg/readiness/client_test.gopkg/readiness/cluster_conditions.gopkg/readiness/crd_compat.gopkg/readiness/etcd_health.gopkg/readiness/network.gopkg/readiness/node_capacity.gopkg/readiness/olm_lifecycle.gopkg/readiness/olm_lifecycle_test.gopkg/readiness/operator_health.gopkg/readiness/pdb_drain.gopkg/start/start.gotest/cvo/readiness.go
Contributor
Author
|
/hold until tested against the CVO skill in agentic-skills repo. |
openshift-ci
Bot
added
the
do-not-merge/hold
harche
force-pushed
the
readiness_pkg
branch
2 times, most recently
from
8be9908 to
a3f0869
Compare
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/readiness/check.go`:
- Around line 92-125: The goroutine launched in RunAll that calls Check.Run can
panic and crash the process; wrap the goroutine body with a defer recover to
catch panics, convert the panic value (and optionally debug.Stack()) into an
error string, set result.Status = StatusError, set result.Error to that string
(and ensure result.Data is a non-nil map), and then continue to store the result
into results[ch.Name()] under the existing mu lock so the panic is recorded
instead of crashing; implement this recovery logic inside the anonymous func
passed to go (the closure that invokes ch.Run) and ensure the defer runs before
wg.Done().
🪄 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: 97741758-f119-4be3-b58c-be64deb6dbfe
📒 Files selected for processing (23)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsonpkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/proposal/controller.gopkg/proposal/controller_test.gopkg/readiness/api_deprecations.gopkg/readiness/check.gopkg/readiness/check_test.gopkg/readiness/checks_test.gopkg/readiness/client.gopkg/readiness/client_test.gopkg/readiness/cluster_conditions.gopkg/readiness/crd_compat.gopkg/readiness/etcd_health.gopkg/readiness/network.gopkg/readiness/node_capacity.gopkg/readiness/olm_lifecycle.gopkg/readiness/olm_lifecycle_test.gopkg/readiness/operator_health.gopkg/readiness/pdb_drain.gopkg/start/start.gotest/cvo/readiness.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/readiness/node_capacity.go
- pkg/cvo/availableupdates_test.go
- pkg/readiness/cluster_conditions.go
- pkg/readiness/etcd_health.go
- pkg/readiness/client_test.go
- pkg/start/start.go
- pkg/readiness/crd_compat.go
- pkg/readiness/client.go
- pkg/readiness/network.go
- pkg/cvo/cvo.go
- pkg/readiness/check_test.go
- pkg/proposal/controller.go
- pkg/readiness/pdb_drain.go
- pkg/readiness/olm_lifecycle_test.go
- pkg/readiness/olm_lifecycle.go
- test/cvo/readiness.go
- pkg/readiness/operator_health.go
- pkg/readiness/checks_test.go
- pkg/proposal/controller_test.go
Contributor
Author
Manual Test Report — Readiness Checks on Live ClusterEnvironment
ResultsProposals created: 11 (one per available update: 4.21.6 → 4.21.16) ✅ ProposalApprovals created: 11 (by lightspeed operator, all stages Manual — no action taken without human approval) ✅ Readiness checks (sample from
Total readiness time: 2.25s per target | All 9 checks OK, 0 errored ✅ What was verified
Not tested
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/cvo/readiness.go`:
- Around line 197-199: The direct assertion
result.Data["blocking_pdbs"].([]map[string]any) can panic; change to a safe
check: retrieve value from result.Data, use a comma-ok type assertion for
[]map[string]any (or []any then convert), assert ok with o.Expect (e.g.,
o.Expect(ok).To(BeTrue(), "...")) and only then compute len(blockingPDBs) and
compare to expectedBlocking using the existing o.Expect call; reference symbols:
blockingPDBs, result.Data["blocking_pdbs"], o.Expect, expectedBlocking.
- Line 130: The direct type assertion result.Data["summary"].(map[string]any)
can panic; change the code in test/cvo/readiness.go to use a safe two-value
assertion (e.g., v, ok := result.Data["summary"].(map[string]any) and fail the
test with a clear message if !ok) or replace the assertion with a Gomega matcher
(e.g., Expect(result.Data["summary"]).To(BeAssignableToTypeOf(map[string]any{}))
/ Expect(result.Data["summary"]).To(BeAKindOf(map[string]any))) before using the
variable `summary` so the test reports a failure instead of panicking.
🪄 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: 7ff63e7c-fd65-4b18-a2ac-95a18bff8ad1
📒 Files selected for processing (23)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsonpkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/proposal/controller.gopkg/proposal/controller_test.gopkg/readiness/api_deprecations.gopkg/readiness/check.gopkg/readiness/check_test.gopkg/readiness/checks_test.gopkg/readiness/client.gopkg/readiness/client_test.gopkg/readiness/cluster_conditions.gopkg/readiness/crd_compat.gopkg/readiness/etcd_health.gopkg/readiness/network.gopkg/readiness/node_capacity.gopkg/readiness/olm_lifecycle.gopkg/readiness/olm_lifecycle_test.gopkg/readiness/operator_health.gopkg/readiness/pdb_drain.gopkg/start/start.gotest/cvo/readiness.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cvo/cvo_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- pkg/cvo/availableupdates_test.go
- pkg/readiness/cluster_conditions.go
- pkg/readiness/pdb_drain.go
- pkg/readiness/api_deprecations.go
- pkg/readiness/network.go
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
- pkg/readiness/operator_health.go
- pkg/readiness/client.go
- pkg/proposal/controller.go
- pkg/readiness/client_test.go
- pkg/readiness/node_capacity.go
- pkg/readiness/check_test.go
- pkg/readiness/olm_lifecycle_test.go
- pkg/readiness/check.go
- pkg/readiness/olm_lifecycle.go
- pkg/readiness/crd_compat.go
- pkg/cvo/cvo.go
- pkg/start/start.go
- pkg/readiness/checks_test.go
- pkg/proposal/controller_test.go
- pkg/readiness/etcd_health.go
|
Actionable comments posted: 0 |
4 tasks
Contributor
Author
|
/hold Blocked on openshift/agentic-skills#13 — the product-lifecycle skill needs to merge first so the skills image includes it when CVO creates proposals. |
Add pkg/readiness package with 9 cluster readiness checks that gather pre-upgrade health data: cluster conditions, operator health, API deprecations, node capacity, PDB drain blockers, etcd health, network config, CRD compatibility, and OLM operator lifecycle. Wire readiness.RunAll() into the proposal controller, replacing the hardcoded readinessJSON placeholder with real per-target readiness data that gets embedded in each proposal's request body. Plumb dynamic.Interface from pkg/start through cvo.New() to the proposal controller to support the readiness checks' cluster queries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/proposal/controller.go (1)
152-156: 💤 Low valueConsider breaking early after finding Progressing condition.
The loop continues iterating through all conditions even after finding
Progressing=True. While not incorrect (the early return happens regardless), breaking immediately would be more efficient.♻️ Proposed optimization
for _, cond := range cv.Status.Conditions { if cond.Type == "Progressing" && cond.Status == configv1.ConditionTrue { klog.V(i.Normal).Infof("Skipping proposal sync: cluster is progressing (%s)", cond.Message) return nil } }Replace with:
for _, cond := range cv.Status.Conditions { if cond.Type == "Progressing" && cond.Status == configv1.ConditionTrue { klog.V(i.Normal).Infof("Skipping proposal sync: cluster is progressing (%s)", cond.Message) return nil } + if cond.Type == "Progressing" { + break + } }Or use a helper function for clarity.
🤖 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.go` around lines 152 - 156, The loop over cv.Status.Conditions should stop scanning once a Progressing condition with status configv1.ConditionTrue is found; replace the current early return inside the loop with a break so you only call klog.V(i.Normal).Infof("Skipping proposal sync: cluster is progressing (%s)", cond.Message) and then break out of the for-loop, and ensure the function returns nil immediately after the loop (preserving the original behavior) so change the return nil to occur after the loop rather than inside it; locate the check using cv.Status.Conditions, cond.Type == "Progressing", configv1.ConditionTrue, and the klog.V(i.Normal).Infof call to apply this fix.
🤖 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 `@install/0000_00_cluster-version-operator_30_deployment.yaml`:
- Around line 80-81: The deployment uses a mutable CI image tag in the
environment variable LIGHTSPEED_SKILLS_IMAGE; replace the value
"quay.io/openshift/ci:ocp_5.0_agentic-skills" with an immutable reference
(either a semantic version tag like "quay.io/openshift/ci:v1.2.3" or an image
digest like "quay.io/openshift/ci@sha256:<digest>") in the
LIGHTSPEED_SKILLS_IMAGE entry in the manifest so the deployment uses a fixed,
reproducible image.
In `@pkg/proposal/controller.go`:
- Around line 385-386: The controller currently registers skills with the paths
"/skills/documentation/openshift-docs" and
"/skills/documentation/kubernetes-docs" which mismatch test expectations; update
those two entries in the skills list in proposal controller to
"/skills/documentation/openshift" and "/skills/documentation/kubernetes" so they
match the expectations referenced in controller_test.go (replace the two string
literals shown in the diff), or alternatively update all test expectations to
include the "-docs" suffix if you intend to keep the current strings.
---
Nitpick comments:
In `@pkg/proposal/controller.go`:
- Around line 152-156: The loop over cv.Status.Conditions should stop scanning
once a Progressing condition with status configv1.ConditionTrue is found;
replace the current early return inside the loop with a break so you only call
klog.V(i.Normal).Infof("Skipping proposal sync: cluster is progressing (%s)",
cond.Message) and then break out of the for-loop, and ensure the function
returns nil immediately after the loop (preserving the original behavior) so
change the return nil to occur after the loop rather than inside it; locate the
check using cv.Status.Conditions, cond.Type == "Progressing",
configv1.ConditionTrue, and the klog.V(i.Normal).Infof call to apply this fix.
🪄 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: ea50e5cd-f75a-4c06-94af-388fa5fa806a
📒 Files selected for processing (25)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsoninstall/0000_00_cluster-version-operator_30_deployment.yamlpkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/proposal/analysis_schema.jsonpkg/proposal/controller.gopkg/proposal/controller_test.gopkg/readiness/api_deprecations.gopkg/readiness/check.gopkg/readiness/check_test.gopkg/readiness/checks_test.gopkg/readiness/client.gopkg/readiness/client_test.gopkg/readiness/cluster_conditions.gopkg/readiness/crd_compat.gopkg/readiness/etcd_health.gopkg/readiness/network.gopkg/readiness/node_capacity.gopkg/readiness/olm_lifecycle.gopkg/readiness/olm_lifecycle_test.gopkg/readiness/operator_health.gopkg/readiness/pdb_drain.gopkg/start/start.gotest/cvo/readiness.go
🚧 Files skipped from review as they are similar to previous changes (20)
- pkg/cvo/availableupdates_test.go
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
- pkg/readiness/crd_compat.go
- pkg/cvo/cvo_test.go
- pkg/readiness/node_capacity.go
- pkg/readiness/operator_health.go
- pkg/readiness/cluster_conditions.go
- pkg/readiness/etcd_health.go
- pkg/readiness/api_deprecations.go
- pkg/readiness/client_test.go
- pkg/readiness/pdb_drain.go
- pkg/cvo/cvo.go
- pkg/readiness/network.go
- pkg/start/start.go
- pkg/readiness/olm_lifecycle_test.go
- pkg/readiness/olm_lifecycle.go
- pkg/readiness/client.go
- pkg/readiness/check_test.go
- pkg/readiness/check.go
- pkg/readiness/checks_test.go
Comment on lines
+80
to
+81
| - name: LIGHTSPEED_SKILLS_IMAGE | ||
| value: "quay.io/openshift/ci:ocp_5.0_agentic-skills" |
There was a problem hiding this comment.
Replace CI image tag with a versioned release tag or digest.
The LIGHTSPEED_SKILLS_IMAGE value references quay.io/openshift/ci:ocp_5.0_agentic-skills, which appears to be a mutable CI tag rather than a versioned release. CI tags can change unexpectedly and are unsuitable for production deployments.
Before merging, replace with either a semantic version tag (e.g., v1.2.3) or an immutable digest reference.
🤖 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 `@install/0000_00_cluster-version-operator_30_deployment.yaml` around lines 80
- 81, The deployment uses a mutable CI image tag in the environment variable
LIGHTSPEED_SKILLS_IMAGE; replace the value
"quay.io/openshift/ci:ocp_5.0_agentic-skills" with an immutable reference
(either a semantic version tag like "quay.io/openshift/ci:v1.2.3" or an image
digest like "quay.io/openshift/ci@sha256:<digest>") in the
LIGHTSPEED_SKILLS_IMAGE entry in the manifest so the deployment uses a fixed,
reproducible image.
Comment on lines
385
to
+386
| "/skills/cluster-update/product-lifecycle", | ||
| "/skills/documentation/openshift", | ||
| "/skills/documentation/kubernetes", | ||
| "/skills/documentation/openshift-docs", |
There was a problem hiding this comment.
Critical: Skills paths mismatch between controller and tests.
The controller specifies:
"/skills/documentation/openshift-docs",
"/skills/documentation/kubernetes-docs",
But all test expectations in controller_test.go (lines 112-113, 806-807, 870-871, 943-944, 1000-1001, 1062-1063, 1124-1125) expect:
"/skills/documentation/openshift",
"/skills/documentation/kubernetes",
This inconsistency will cause test failures. Remove the -docs suffix here to match test expectations, or update all test expectations to include the suffix.
🐛 Proposed fix to match test expectations
"/skills/cluster-update/update-advisor",
"/skills/cluster-update/product-lifecycle",
- "/skills/documentation/openshift-docs",
- "/skills/documentation/kubernetes-docs",
+ "/skills/documentation/openshift",
+ "/skills/documentation/kubernetes",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "/skills/cluster-update/product-lifecycle", | |
| "/skills/documentation/openshift", | |
| "/skills/documentation/kubernetes", | |
| "/skills/documentation/openshift-docs", | |
| "/skills/cluster-update/update-advisor", | |
| "/skills/cluster-update/product-lifecycle", | |
| "/skills/documentation/openshift", | |
| "/skills/documentation/kubernetes", |
🤖 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.go` around lines 385 - 386, The controller currently
registers skills with the paths "/skills/documentation/openshift-docs" and
"/skills/documentation/kubernetes-docs" which mismatch test expectations; update
those two entries in the skills list in proposal controller to
"/skills/documentation/openshift" and "/skills/documentation/kubernetes" so they
match the expectations referenced in controller_test.go (replace the two string
literals shown in the diff), or alternatively update all test expectations to
include the "-docs" suffix if you intend to keep the current strings.
Contributor
|
@harche: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg/readinesspackage with 9 cluster readiness checks that gather pre-upgrade health data:readiness.RunAll()into the proposal controller, replacing the hardcodedreadinessJSON := "{}"placeholder (from PR OTA-1966: Install namespace/openshift-lightspeed only when TechPreview is enabled #1394) with real per-target readiness datadynamic.Interfacefrompkg/start→cvo.New()→proposal.NewController()to support cluster queriesTest plan
go build ./...passesgo test ./pkg/readiness/— unit tests for each check +RunAllintegration test with fake cluster (all 9 checks exercised with non-trivial data)go test ./pkg/proposal/— controller tests including end-to-end test verifying real readiness JSON flows into proposal request bodygo test ./pkg/cvo/— existing tests updated for new signatures🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation