OTA-1546: Reconcile cv.spec.desiredUpdate.acceptRisks

[hongkailiu]

From this pull, CVO starts to reconcile cv.spec.desiredUpdate.acceptRisks
on a cluser if FeatureGateClusterUpdateAcceptRisks is enabled no a
non-hypershift cluster.

• CVO does not block a conditional update if all risks that apply
  to the cluster are accpeted.

• CVO fills up the new fields in status:

  • status.conditionalUpdateRisks,
  • status.conditionalUpdate.riskNames, and
  • status.conditionalUpdate.risks.conditions

• CVO adds risk names into status.history.acceptedRisks if a conditional
  update is accepted.

[openshift-ci-robot]

@hongkailiu: This pull request references OTA-1546 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In 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.

[coderabbitai]

Walkthrough

Adds feature-gated support for accepting conditional-update risks: feature-gate wiring and operator gate method, acceptance state and per-risk condition tracking on availableUpdates, propagation into evaluation, status, and metrics paths, new internal constants, go.mod replace directives, and updated tests.

Changes

Cohort / File(s)	Summary
Module / Replaces go.mod	Bumped github.com/openshift/api version and added replace directives for github.com/openshift/api and github.com/openshift/client-go.
Feature gates pkg/featuregates/featuregates.go	Added acceptRisks field and AcceptRisks() bool to gate model; wired FeatureGateClusterUpdateAcceptRisks into enable/disable logic and defaults.
Internal constants pkg/internal/constants.go	Added ConditionalUpdateConditionTypeRecommended and ConditionalUpdateRiskConditionTypeApplies.
Operator gating pkg/cvo/cvo.go	Added func (optr *Operator) shouldReconcileAcceptRisks() bool to control reconciling accept-risks behavior (feature gate + HyperShift check).
Available updates & evaluation pkg/cvo/availableupdates.go, pkg/cvo/availableupdates_test.go	Extended availableUpdates with ShouldReconcileAcceptRisks, AcceptRisks, RiskConditions; expanded evaluateConditionalUpdate signature and logic to honor accepted risks and record per-risk conditions; added riskConditionReasonEvaluationFailed; tests updated.
Status reconciliation pkg/cvo/status.go, pkg/cvo/status_test.go	updateClusterVersionStatus gains shouldReconcileAcceptRisks parameter; added helpers to include risk names and riskConditions in status when gate enabled; tests adapted.
Metrics & precondition pkg/cvo/metrics.go, pkg/cvo/metrics_test.go, pkg/payload/precondition/clusterversion/recommendedupdate.go	Replaced use of exported literal with internal.ConditionalUpdateConditionTypeRecommended across metrics, tests, and precondition check.
Tests & wiring pkg/cvo/cvo_test.go	Test expectations updated to include RiskConditions, ignore ShouldReconcileAcceptRisks in comparisons, and reflect gate wiring.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongkailiu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [hongkailiu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/cvo/availableupdates.go (1)

521-579: Fix the risk condition storage logic to accumulate all conditions instead of silently dropping duplicates.

The code at lines 570–571 only stores a risk condition if the risk name hasn't been seen before, causing silent data loss when multiple ConditionalUpdates contain risks with the same name. Since evaluateConditionalUpdate is called once per ConditionalUpdate with a shared riskConditions map (line 444), conditions for the same risk name across different updates are overwritten or ignored.

Additionally, the TestEvaluateConditionalUpdate test calls evaluateConditionalUpdate with 3 arguments, but the function signature requires 6 (acceptRisks, shouldReconcileAcceptRisks, riskConditions). The test needs to be updated to match the current function signature and verify the riskConditions map is populated correctly.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between d245c43 and eb30e14.

⛔ Files ignored due to path filters (35)

• go.sum is excluded by !**/*.sum
• vendor/github.com/openshift/api/config/v1/register.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/types_cluster_version.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/types_feature.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/types_image_policy.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/types_insights.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/types_scheduling.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-DevPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-TechPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-Default.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-DevPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-TechPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-Default.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-DevPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-TechPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-TechPreviewNoUpgrade.crd.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/api/features/features.go is excluded by !vendor/**, !**/vendor/**
• vendor/modules.txt is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (7)

• go.mod
• pkg/cvo/availableupdates.go
• pkg/cvo/cvo.go
• pkg/cvo/status.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/internal/constants.go
• pkg/cvo/cvo.go
• pkg/featuregates/featuregates.go
• go.mod
• pkg/cvo/status.go
• pkg/cvo/availableupdates.go

🧬 Code graph analysis (3)

pkg/payload/precondition/clusterversion/recommendedupdate.go (1)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/cvo/status.go (2)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/featuregates/featuregates.go (1)

• CvoGateChecker (19-41)

pkg/cvo/availableupdates.go (2)

pkg/clusterconditions/clusterconditions.go (2)

• Condition (19-29)
• ConditionRegistry (31-42)

pkg/internal/constants.go (2)

• ConditionalUpdateConditionTypeRecommended (76-76)
• ConditionalUpdateRiskConditionTypeApplies (80-80)

🔇 Additional comments (10)

pkg/payload/precondition/clusterversion/recommendedupdate.go (1)

14-15: LGTM!

Good refactor to use the centralized constant internal.ConditionalUpdateConditionTypeRecommended instead of a hardcoded string. This improves maintainability and consistency across the codebase.

Also applies to: 55-55

pkg/cvo/cvo.go (1)

1094-1102: LGTM!

The new shouldReconcileAcceptRisks() method follows the same pattern as shouldReconcileCVOConfiguration() and correctly gates the functionality behind the AcceptRisks feature gate while excluding HyperShift environments.

pkg/cvo/availableupdates.go (1)

177-207: LGTM on struct additions.

The new fields ShouldReconcileAcceptRisks, AcceptRisks, and RiskConditions are well-placed in the struct and align with the risk acceptance feature requirements.

pkg/featuregates/featuregates.go (2)

38-41: LGTM!

The AcceptRisks feature gate is properly added following the established pattern for other CVO feature gates (StatusReleaseArchitecture, CVOConfiguration).

Also applies to: 55-56, 70-72

---

103-104: Verify FeatureGateClusterUpdateAcceptRisks is exported by the features package.

The code references features.FeatureGateClusterUpdateAcceptRisks imported from github.com/openshift/api/features. While this constant is not defined locally and cannot be verified within this repository, the go.mod uses a forked version (hongkailiu/api). Ensure this constant exists in the forked API package and will remain available when migrating to the official openshift/api package.

Also applies to: 113-114

pkg/cvo/status.go (3)

40-41: LGTM!

Good refactor to use the centralized constant for condition type lookup.

---

191-199: LGTM on signature change.

The extended signature properly accepts the shouldReconcileAcceptRisks callback to gate the risk reconciliation behavior.

---

452-465: LGTM on conditionalUpdateRisks helper.

The function correctly deduplicates risks by name and returns a flat list of unique ConditionalUpdateRisk entries.

pkg/internal/constants.go (1)

73-81: LGTM!

Well-documented constants that centralize condition type identifiers. Both constants are appropriately named, follow existing patterns in this file, and are actively used throughout the codebase.

go.mod (1)

97-97: Personal fork replacement should not be merged.

This replace directive points to a personal fork (github.com/hongkailiu/api) instead of the official github.com/openshift/api. Per the commit message "to-be-replaced by api#2360", this should be updated to the official repository once the upstream PR is merged before this change can be merged to main.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

pkg/cvo/status.go (1)

425-454: Add nil check for getAvailableUpdates() at line 428.

Line 428 directly accesses getAvailableUpdates().RiskConditions, but the function signature indicates getAvailableUpdates can return nil. This would cause a panic. The mergeReleaseMetadata function at line 227 safely checks for nil (cvo.go:909), but conditionalUpdateWithRiskNamesAndRiskConditions does not.

Suggested fix

 func conditionalUpdateWithRiskNamesAndRiskConditions(conditionalUpdates []configv1.ConditionalUpdate, getAvailableUpdates func() *availableUpdates, desiredImage string) ([]configv1.ConditionalUpdate, []string) {
 	var result []configv1.ConditionalUpdate
 	var riskNamesForDesiredImage []string
-	riskConditions := getAvailableUpdates().RiskConditions
+	var riskConditions map[string][]metav1.Condition
+	if au := getAvailableUpdates(); au != nil {
+		riskConditions = au.RiskConditions
+	}
 	for _, conditionalUpdate := range conditionalUpdates {

🧹 Nitpick comments (1)

pkg/cvo/status.go (1)

429-454: Review risk condition synchronization logic.

Lines 433-450 implement risk condition synchronization with the following flow:

1. Build a set of risk names from conditionalUpdate.Risks
2. Remove stale conditions (not in fetched riskConditions)
3. Merge in new conditions from riskConditions

However, there's a subtle logic concern at lines 436-445:

for _, condition := range risk.Conditions {
    if found := meta.FindStatusCondition(conditions, condition.Type); found == nil {
        riskTypesToRemove.Insert(condition.Type)
    }
}

This removes condition types from risk.Conditions if they're not found in riskConditions[risk.Name]. Then at lines 446-448:

for _, condition := range conditions {
    meta.SetStatusCondition(&risk.Conditions, condition)
}

New conditions are added. This is correct if the intent is to replace conditions on each risk with only those from riskConditions. Please confirm this is the intended behavior.

💡 Consider adding clarifying comments

The synchronization logic would benefit from comments explaining the intent:

 		riskNames := sets.New[string]()
 		for _, risk := range conditionalUpdate.Risks {
 			riskNames.Insert(risk.Name)
+			// Remove stale condition types that are no longer present in the fetched risk conditions
 			riskTypesToRemove := sets.New[string]()
 			conditions := riskConditions[risk.Name]
 			for _, condition := range risk.Conditions {
 				if found := meta.FindStatusCondition(conditions, condition.Type); found == nil {
 					riskTypesToRemove.Insert(condition.Type)
 				}
 			}
 			for riskTypeToRemove := range riskTypesToRemove {
 				meta.RemoveStatusCondition(&risk.Conditions, riskTypeToRemove)
 			}
+			// Merge in current conditions from the risk conditions map
 			for _, condition := range conditions {
 				meta.SetStatusCondition(&risk.Conditions, condition)
 			}
 		}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between eb30e14 and cd8eab2.

⛔ Files ignored due to path filters (23)

• go.sum is excluded by !**/*.sum
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/custom.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherconfig.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gathererconfig.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherers.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagather.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagatherspec.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeclaimreference.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeconfig.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/storage.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/applyconfigurations/utils.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/config_client.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_config_client.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_insightsdatagather.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/generated_expansion.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/insightsdatagather.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/insightsdatagather.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/interface.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/informers/externalversions/generic.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/listers/config/v1/expansion_generated.go is excluded by !vendor/**, !**/vendor/**
• vendor/github.com/openshift/client-go/config/listers/config/v1/insightsdatagather.go is excluded by !vendor/**, !**/vendor/**
• vendor/modules.txt is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (11)

• go.mod
• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status.go
• pkg/cvo/status_test.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/cvo/cvo.go
• pkg/internal/constants.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• go.mod
• pkg/cvo/metrics_test.go
• pkg/cvo/status_test.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/cvo/status.go
• pkg/cvo/metrics.go
• pkg/featuregates/featuregates.go
• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go

🧬 Code graph analysis (5)

pkg/cvo/metrics_test.go (2)

pkg/cvo/status.go (1)

• ConditionalUpdateConditionTypeRecommended (35-35)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/payload/precondition/clusterversion/recommendedupdate.go (2)

pkg/cvo/status.go (1)

• ConditionalUpdateConditionTypeRecommended (35-35)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/cvo/status.go (1)

pkg/featuregates/featuregates.go (1)

• CvoGateChecker (19-41)

pkg/cvo/metrics.go (2)

pkg/cvo/status.go (1)

• ConditionalUpdateConditionTypeRecommended (35-35)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/cvo/availableupdates_test.go (2)

pkg/featuregates/featuregates.go (1)

• DefaultCvoGates (75-83)

pkg/cvo/cvo.go (1)

• New (204-296)

🔇 Additional comments (30)

go.mod (1)

13-13: Clarify the relationship and necessity of replace directives.

Line 13 requires github.com/openshift/api at version 20251222154221, but line 97 replaces it with a fork at version 20251223025917. Line 99 replaces openshift/client-go with itself at a different version.

Provide inline comments explaining:

• Why the fork in line 97 is necessary instead of using the upstream version pinned in line 13.
• Why line 99 replaces the module with itself but at a different timestamp, and whether this indicates a transitive dependency resolution issue that should be addressed differently.
• Whether these replaces are expected to be temporary or permanent, and if temporary, the plan to remove them before release.

Also applies to: 97-97, 99-99

pkg/payload/precondition/clusterversion/recommendedupdate.go (1)

14-14: LGTM! Constant centralization improves maintainability.

The refactoring to use internal.ConditionalUpdateConditionTypeRecommended instead of the string literal "Recommended" reduces duplication and the risk of typos across the codebase.

Also applies to: 55-55

pkg/featuregates/featuregates.go (1)

39-40: LGTM! Feature gate implementation follows established patterns.

The AcceptRisks feature gate is properly wired through the entire chain:

• Interface method declared
• Struct field added with conservative default (false)
• Getter method implemented
• Proper handling in both enable and disable paths

The implementation is consistent with existing feature gates (StatusReleaseArchitecture, CVOConfiguration).

Also applies to: 55-55, 70-72, 81-81, 103-104, 113-114

pkg/cvo/metrics_test.go (1)

50-50: LGTM! Test updated to use centralized constant.

The change to use internal.ConditionalUpdateConditionTypeRecommended is consistent with the constant centralization across the codebase.

pkg/cvo/metrics.go (1)

444-444: LGTM! Metrics collection updated to use centralized constant.

The change to use internal.ConditionalUpdateConditionTypeRecommended maintains the same filtering logic while aligning with the codebase-wide constant centralization.

pkg/cvo/status_test.go (1)

206-206: LGTM! Test infrastructure updated for new feature gate.

The changes properly update the test fakes and function calls:

• fakeRiFlags now implements the complete CvoGateChecker interface with the new AcceptRisks() method
• The updateClusterVersionStatus call includes the new shouldReconcileAcceptRisks callback with a conservative default (false) appropriate for this test's focus on error filtering

Also applies to: 221-223, 749-751

pkg/cvo/status.go (6)

20-20: LGTM! Import addition is appropriate.

The sets import is correctly added to support the new risk acceptance functionality.

---

44-44: LGTM! Constant usage centralized.

The switch to internal.ConditionalUpdateConditionTypeRecommended aligns with the PR's objective of using centralized internal constants.

---

195-203: LGTM! Function signature properly extended.

The addition of shouldReconcileAcceptRisks func() bool parameter is appropriate for feature-gating the risk acceptance logic, and the call site at line 184 correctly passes optr.shouldReconcileAcceptRisks.

---

232-236: LGTM! Risk processing logic is properly gated.

The risk-aware processing integration correctly:

• Gates execution with shouldReconcileAcceptRisks()
• Computes riskNamesForDesiredImage for the desired image
• Updates ConditionalUpdates and ConditionalUpdateRisks fields

---

241-247: LGTM! Risk message construction logic is correct.

The conditional message appending properly handles both scenarios:

1. Empty risksMsg: constructs a complete sentence
2. Non-empty risksMsg: appends with appropriate separator

The logic is correctly gated by both shouldReconcileAcceptRisks() and the presence of riskNamesForDesiredImage.

---

456-469: LGTM! Risk deduplication logic is correct.

The conditionalUpdateRisks function correctly deduplicates risks across all conditional updates using a set-based approach, ensuring each unique risk appears only once in the returned slice.

pkg/cvo/availableupdates_test.go (7)

22-22: LGTM! Import additions support new test functionality.

The sets import is appropriately added to support the new AcceptRisks field handling in tests.

---

29-29: LGTM! Feature gates import is appropriate.

The featuregates import enables test setup to configure enabledFeatureGates, which is essential for testing the new gated behavior.

---

211-212: LGTM! Comparison option correctly ignores function field.

Adding ShouldReconcileAcceptRisks to the ignore list is appropriate since function pointers cannot be meaningfully compared in deep equality checks.

---

237-237: LGTM! Feature gates properly initialized in tests.

The test correctly initializes enabledFeatureGates with DefaultCvoGates before invoking syncAvailableUpdates, ensuring the code under test has access to gate-checking functionality.

---

515-517: LGTM! Test call signature correctly updated.

The evaluateConditionalUpdate call is properly updated with the new parameters:

• sets.New[string]() for empty accepted risks
• func() bool { return false } to disable risk acceptance
• map[string][]metav1.Condition{} for empty risk conditions

These defaults are appropriate for testing the core evaluation logic in isolation.

---

326-326: LGTM! Consistent feature gate initialization.

The test setup consistently initializes feature gates before each sync operation.

---

754-754: LGTM! Feature gate initialization is consistent.

All test cases consistently initialize enabledFeatureGates before calling sync methods, ensuring uniform test behavior.

pkg/cvo/availableupdates.go (11)

19-19: LGTM! Import addition is appropriate.

The sets import correctly supports the new AcceptRisks field of type sets.Set[string].

---

26-26: LGTM! Internal constants import is appropriate.

The internal package import provides access to centralized constants like ConditionalUpdateConditionTypeRecommended, aligning with the PR's objective of using internal constants.

---

55-60: LGTM! Nil-safety properly implemented.

The acceptRisks initialization correctly guards against a nil DesiredUpdate by checking config.Spec.DesiredUpdate != nil before accessing its AcceptRisks field. This addresses the previously flagged nil pointer concern.

---

140-141: LGTM! AvailableUpdates fields properly initialized.

The ShouldReconcileAcceptRisks and AcceptRisks fields are correctly assigned from operator state, enabling risk acceptance logic during update evaluation.

---

183-184: LGTM! Struct fields appropriately added.

The new fields ShouldReconcileAcceptRisks (gate function) and AcceptRisks (risk name set) are correctly typed and positioned to support the risk acceptance feature.

---

208-208: LGTM! RiskConditions field appropriately added.

The RiskConditions map field enables per-risk condition tracking, mapping risk names to their associated conditions.

---

307-312: LGTM with observation: RiskConditions not copied.

The new fields are correctly populated in the copy:

• Line 311: ShouldReconcileAcceptRisks uses the current operator method (ensuring latest gate state)
• Line 312: AcceptRisks is copied from stored state

Observation: RiskConditions is not copied here and remains nil in the returned struct. This appears intentional since RiskConditions is populated during evaluateConditionalUpdates. Verify this is the expected behavior.

---

446-446: LGTM! Function call correctly updated.

The evaluateConditionalUpdate call properly passes the new parameters (u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions) to support risk acceptance evaluation.

---

505-505: LGTM! Constant appropriately added.

The riskConditionReasonEvaluationFailed constant provides a consistent reason value for risk condition evaluation failures.

---

523-530: LGTM! Function signature appropriately extended.

The evaluateConditionalUpdate signature is correctly extended with:

• acceptRisks sets.Set[string]: accepted risk names
• shouldReconcileAcceptRisks func() bool: feature gate check
• riskConditions map[string][]metav1.Condition: per-risk condition tracking

All call sites have been updated accordingly.

---

532-532: LGTM! Constant usage centralized.

The switch to internal.ConditionalUpdateConditionTypeRecommended aligns with the PR's objective of using internal constants consistently.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n go.mod | sed -n '1,110p'

Repository: openshift/cluster-version-operator

Length of output: 5226

---

🏁 Script executed:

# Search for comments related to hongkailiu or acceptRisks (mentioned in original comment)
rg -i "hongkailiu|acceptRisks|accept.risk" --type go --type md -B 2 -A 2

Repository: openshift/cluster-version-operator

Length of output: 8135

---

🏁 Script executed:

# Check what types from openshift/api are being imported
rg "from github.com/openshift/api|import.*openshift/api" --type go -A 3 | head -40

Repository: openshift/cluster-version-operator

Length of output: 60

---

🏁 Script executed:

# Search for actual imports from openshift/api package
rg "configv1|appv1|imagev1" --type go | head -20

Repository: openshift/cluster-version-operator

Length of output: 2314

---

🏁 Script executed:

# Check git log for recent changes to go.mod related to openshift/api
git log --oneline -20 go.mod | head -20

Repository: openshift/cluster-version-operator

Length of output: 140

---

🏁 Script executed:

# Check the commit details for go.mod changes
git show cd8eab20 -- go.mod

Repository: openshift/cluster-version-operator

Length of output: 4955

---

🏁 Script executed:

# Check what changed in the commit - look at actual code changes (not just go.mod)
git show cd8eab20 --stat | head -30

Repository: openshift/cluster-version-operator

Length of output: 1769

---

🏁 Script executed:

# Check the full commit message and any PR details
git show cd8eab20 --format=fuller | head -50

Repository: openshift/cluster-version-operator

Length of output: 2089

---

🏁 Script executed:

# Check if there are any comments or documentation about the fork in the repo
rg -i "fork|hongkailiu" --type md -B 2 -A 2

Repository: openshift/cluster-version-operator

Length of output: 858

---

🏁 Script executed:

# Check if there's any PR description or related documentation about why the fork is needed
ls -la | grep -E "PULL_REQUEST|PR_|ISSUE"

Repository: openshift/cluster-version-operator

Length of output: 60

---

🏁 Script executed:

# Check the most recent changes to understand if this is a new repository initialization
git log --oneline --all | tail -10

Repository: openshift/cluster-version-operator

Length of output: 532

---

🏁 Script executed:

# Check if the fork repository exists and is accessible - search for any references to what changed in hongkailiu/api
rg "github.com/hongkailiu" . --type go --type md -B 1 -A 1

Repository: openshift/cluster-version-operator

Length of output: 60

---

🏁 Script executed:

# Check what specific types/interfaces are being used from openshift/api related to acceptRisks
rg "ConditionalUpdateRisk|AcceptRisks|ConditionalUpdate" --type go -B 1 -A 1 | head -40

Repository: openshift/cluster-version-operator

Length of output: 2535

---

🏁 Script executed:

# Check the exact types from configv1 that are used in the code
rg "desiredUpdate.AcceptRisks|spec.DesiredUpdate.AcceptRisks" --type go -B 2 -A 2

Repository: openshift/cluster-version-operator

Length of output: 60

---

🏁 Script executed:

# Check what types are imported and used from openshift/api
grep -r "DesiredUpdate\|ConditionalUpdateRisk\|AcceptRisks" pkg/ --include="*.go" | head -20

Repository: openshift/cluster-version-operator

Length of output: 1955

---

🏁 Script executed:

# Check what specific fields from configv1.Update are being accessed (particularly AcceptRisks)
rg "\.AcceptRisks|configv1\.Update" --type go -B 2 -A 2 | head -50

Repository: openshift/cluster-version-operator

Length of output: 3656

---

Replace directive must be removed or documented before merge—uses personal fork instead of upstream dependency.

The replace directive at line 97 redirects github.com/openshift/api to a personal fork (hongkailiu/api). This creates supply chain and availability risks:

• Personal forks can be deleted, abandoned, or compromised, breaking builds and potentially introducing malicious changes.
• CI/CD systems with restricted network access (air-gapped builds, corporate firewalls) cannot pull from personal repositories.
• Unsustainable for production dependencies.

The code actively uses configv1 types from openshift/api (e.g., ConditionalUpdateRisk, Update, AcceptRisks fields). Before this PR merges:

1. If temporary (development only): Remove the replace directive before merge.
2. If required: Contribute missing features/types to canonical openshift/api and update this dependency to use the upstream version. Document the blocker (e.g., GitHub issue reference).
3. If transitive compatibility fix: Add a comment in go.mod explaining the reasoning.

🤖 Prompt for AI Agents

In go.mod around line 97, the replace directive redirects
github.com/openshift/api to a personal fork (hongkailiu/api) which must not be
left in the merge; either remove the replace before merging if it was only for
local/dev testing, or if the forked changes are required: open a public issue/PR
against the canonical github.com/openshift/api to get the needed types merged,
update the go.mod to require the upstream module version that contains those
changes (and remove the personal replace), or if this is an unavoidable
temporary transitive compatibility workaround, add a clear comment immediately
above the replace in go.mod documenting why the fork is used, link to the
blocker issue/PR, and include a TODO to remove it once upstream is updated so CI
and supply-chain concerns are visible.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

pkg/cvo/availableupdates.go (1)

572-574: Risk conditions may not persist across evaluations and could contain stale data.

Two issues with the risk condition tracking:

1. Map nil panic (duplicate concern from past review): If riskConditions is nil when line 573 executes, it will panic. This can occur when needFreshFetch is false and the cached RiskConditions is nil (e.g., from an older state before line 142 was added, or if the map reference from line 314 is nil).

2. Stale conditions on re-evaluation: The check if _, ok := riskConditions[risk.Name]; !ok means existing conditions are never updated. When evaluateConditionalUpdates is called multiple times (e.g., when needFreshFetch is false but conditions need re-evaluation per lines 86-104), the risk conditions remain unchanged from the previous evaluation, potentially leaving stale status/reason/message.

🔎 Recommended fixes

Fix 1: Initialize map before use

In evaluateConditionalUpdates before the loop (after line 446):

 	})
 	for i, conditionalUpdate := range u.ConditionalUpdates {
+		if u.RiskConditions == nil {
+			u.RiskConditions = map[string][]metav1.Condition{}
+		}
 		condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions)

Fix 2: Update conditions on re-evaluation

Replace the conditional assignment with unconditional:

-		if _, ok := riskConditions[risk.Name]; !ok {
-			riskConditions[risk.Name] = []metav1.Condition{riskCondition}
-		}
+		riskConditions[risk.Name] = []metav1.Condition{riskCondition}

This ensures the condition is updated on every evaluation, reflecting current state rather than preserving stale data.

🧹 Nitpick comments (1)

pkg/cvo/status.go (1)

425-476: Consider skipping risk-condition reconciliation when getAvailableUpdates() is nil

When shouldReconcileAcceptRisks() is true but getAvailableUpdates() returns nil, conditionalUpdateWithRiskNamesAndRiskConditions treats riskConditions as empty and will remove all existing risk.Conditions from cvStatus.ConditionalUpdates before re‑adding none. If CVO restarts with preexisting conditional updates in status but has not yet refreshed in‑memory availableUpdates, this could transiently clear risk condition details in status. You may want to early‑return the original conditionalUpdates unchanged when updates == nil to preserve prior conditions until fresh data is available.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between cd8eab2 and a46418c.

📒 Files selected for processing (11)

• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo.go
• pkg/cvo/cvo_test.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status.go
• pkg/cvo/status_test.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🚧 Files skipped from review as they are similar to previous changes (5)

• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/internal/constants.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status_test.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/cvo/cvo.go
• pkg/cvo/status.go
• pkg/featuregates/featuregates.go
• pkg/cvo/availableupdates.go
• pkg/cvo/cvo_test.go
• pkg/cvo/availableupdates_test.go

🧬 Code graph analysis (1)

pkg/cvo/availableupdates_test.go (3)

pkg/clusterconditions/clusterconditions.go (1)

• Condition (19-29)

pkg/featuregates/featuregates.go (1)

• DefaultCvoGates (75-83)

pkg/clusterconditions/mock/mock.go (2)

• Mock (35-44)
• MatchResult (14-20)

🔇 Additional comments (11)

pkg/cvo/cvo.go (1)

1095-1102: AcceptRisks reconciliation gate mirrors existing configuration gating correctly

The new shouldReconcileAcceptRisks helper cleanly reuses the feature‑gate plus !hypershift pattern from shouldReconcileCVOConfiguration, giving a clear single decision point for risk‑related reconciliation. Looks good.

pkg/featuregates/featuregates.go (1)

39-41: AcceptRisks feature gate wiring is consistent with existing gates

The AcceptRisks flag is correctly plumbed through the CvoGateChecker interface, CvoGates struct, defaults, and CvoGatesFromFeatureGate enable/disable logic, mirroring the existing gate patterns. Only caveat is that adding a method to CvoGateChecker is a compile‑time breaking change for any external implementations, which is fine if this interface is internal to CVO consumers.

Also applies to: 55-56, 70-72, 81-82, 99-105, 109-115

pkg/cvo/cvo_test.go (1)

19-20: Test harness updates align cleanly with new AcceptRisks behavior

Using cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks") and initializing optr.enabledFeatureGates with featuregates.DefaultCvoGates("version") keeps existing tests focused on functional behavior while accommodating the new gate. The extended checkStatus generic to include *configv1.ClusterVersion fits the existing pattern and maintains clarity of assertions.

Also applies to: 2279-2280, 2792-2794, 4057-4109

pkg/cvo/status.go (1)

43-45: Risk-acceptance status integration and nil-guarding look correct

Using internal.ConditionalUpdateConditionTypeRecommended in findRecommendedCondition removes the hard‑coded string, and threading shouldReconcileAcceptRisks into updateClusterVersionStatus gives a single gate for all accept‑risks behavior. The new helpers cleanly recompute ConditionalUpdates, populate ConditionalUpdateRisks, and, via the local updates := getAvailableUpdates() guard, avoid nil dereferences when availableUpdates is not yet initialized. Overall, the risk‑related status updates and messaging are wired coherently around the feature gate.

Also applies to: 184-186, 194-203, 232-247, 425-476

pkg/cvo/availableupdates_test.go (1)

22-23: Tests thoroughly cover acceptRisks and risk-condition propagation

The new table-driven cases for evaluateConditionalUpdate and the expectations on RiskConditions in the sync tests exercise zero/one/many risks, matches vs. non-matches, evaluation failures, and the “accepted risk” path via acceptRisks/shouldReconcileAcceptRisks. Initializing enabledFeatureGates with featuregates.DefaultCvoGates("version") and ignoring ShouldReconcileAcceptRisks in struct comparisons keeps tests stable while still validating the new behavior.

Also applies to: 211-217, 236-239, 352-372, 418-440, 489-547, 579-583, 795-805

pkg/cvo/availableupdates.go (6)

55-60: LGTM! Nil check properly guards against potential panic.

The nil check on config.Spec.DesiredUpdate correctly addresses the previous review concern about potential nil pointer dereference.

---

180-210: LGTM! Struct fields appropriately defined.

The new fields integrate well with the existing structure. The function pointer for ShouldReconcileAcceptRisks allows for dynamic evaluation, and the map-based RiskConditions structure is suitable for per-risk condition tracking.

---

499-509: LGTM! Constants are well-defined.

The new constants appropriately capture the risk acceptance semantics and evaluation failure states.

---

517-523: LGTM! Reason merging logic handles acceptance states.

The updated logic correctly handles the new recommendedReasonExposedOnlyToAcceptedRisks state, though the conditions are complex. The logic prioritizes maintaining "accepted risks only" status when appropriate.

---

558-570: LGTM! Risk acceptance logic correctly implemented.

The conditional logic properly handles accepted risks:

• Risks are only considered "accepted" when both the feature gate (shouldReconcileAcceptRisks()) is enabled AND the risk appears in acceptRisks
• Accepted risks don't block recommendations (line 559)
• Non-accepted but exposed risks do block recommendations (line 563)

This correctly implements the risk acceptance feature semantics.

---

313-314: This review comment is incorrect and should be dismissed.

Lines 313-314 safely shallow-copy AcceptRisks and RiskConditions. The concern about shared map mutations is unfounded—these fields are mutated only during the construction phase in evaluateConditionalUpdates() (called at line 158), not after getAvailableUpdates() returns a read-only snapshot.

The nil panic scenario cannot occur: RiskConditions is always initialized to an empty map at line 142 before any use. The intentional shallow-copy pattern (evidenced by ConditionRegistry at line 318) reflects the design choice to return snapshots for read access to cached state.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between a46418c and 5eee754.

📒 Files selected for processing (11)

• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo.go
• pkg/cvo/cvo_test.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status.go
• pkg/cvo/status_test.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🚧 Files skipped from review as they are similar to previous changes (6)

• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/cvo/metrics_test.go
• pkg/cvo/cvo.go
• pkg/cvo/status_test.go
• pkg/cvo/metrics.go
• pkg/internal/constants.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/featuregates/featuregates.go
• pkg/cvo/status.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo_test.go
• pkg/cvo/availableupdates.go

🧬 Code graph analysis (4)

pkg/cvo/status.go (1)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/cvo/availableupdates_test.go (2)

pkg/featuregates/featuregates.go (1)

• DefaultCvoGates (75-83)

pkg/cvo/cvo.go (1)

• New (204-296)

pkg/cvo/cvo_test.go (1)

pkg/clusterconditions/clusterconditions.go (1)

• Condition (19-29)

pkg/cvo/availableupdates.go (2)

pkg/clusterconditions/clusterconditions.go (2)

• Condition (19-29)
• ConditionRegistry (31-42)

pkg/internal/constants.go (2)

• ConditionalUpdateConditionTypeRecommended (76-76)
• ConditionalUpdateRiskConditionTypeApplies (80-80)

🔇 Additional comments (5)

pkg/featuregates/featuregates.go (1)

39-40: AcceptRisks feature-gate wiring looks correct and conservative

Interface, struct field, default initialization, and FeatureGate mapping for FeatureGateClusterUpdateAcceptRisks are all consistent and symmetric; defaulting to false in unknown-version mode is appropriately safe.

Also applies to: 55-56, 70-72, 81-82, 103-105, 113-115

pkg/cvo/cvo_test.go (1)

19-20: Updated availableUpdates expectations and comparison are sound

Using cmp.Diff with cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks") and explicitly asserting RiskConditions in the test table aligns the tests with the new risk-tracking fields while avoiding brittle comparisons on function fields and timestamps.

Also applies to: 2357-2370, 2401-2412, 2445-2455, 2495-2505, 2559-2574, 2627-2641, 2729-2741, 2792-2794

pkg/cvo/availableupdates_test.go (1)

14-16: Tests comprehensively cover new risk-acceptance and RiskConditions behavior

The additional cmp options, RiskConditions expectations, and extended TestEvaluateConditionalUpdate / desired-update tests exercise the new AcceptRisks and per-risk condition flows well, while guarding against nil-map issues and non-deterministic fields.

Also applies to: 21-23, 211-217, 219-247, 352-372, 374-417, 418-440, 462-547, 549-571, 579-583, 591-605, 610-782

pkg/cvo/status.go (1)

190-201: Risk-aware status and history updates are gated cleanly

Plumbing getAvailableUpdates and shouldReconcileAcceptRisks into updateClusterVersionStatus, and only enriching ConditionalUpdates, ConditionalUpdateRisks, and AcceptedRisks/history when the gate is on, keeps existing behavior unchanged while the feature is disabled and localizes the new logic well.

Also applies to: 223-232, 234-247

pkg/cvo/availableupdates.go (1)

19-27: AcceptRisks and per-risk condition tracking are wired coherently end-to-end

Building acceptRisks from spec.desiredUpdate.acceptRisks, threading it plus ShouldReconcileAcceptRisks and RiskConditions through availableUpdates into evaluateConditionalUpdate, and using internal constants for condition types yields a consistent, feature-gated behavior without introducing nil-map or concurrency hazards at current call sites.

Also applies to: 52-60, 136-143, 180-210, 220-259, 298-337, 437-460, 498-509, 526-581

[openshift-ci-robot]

@hongkailiu: This pull request references OTA-1546 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

From this pull, CVO starts to reconcile cv.spec.desiredUpdate.acceptRisks
on a cluser if FeatureGateClusterUpdateAcceptRisks is enabled no a
non-hypershift cluster.

• CVO does not block a conditional update if all risks that apply
  to the cluster are accpeted.

• CVO fills up the new fields in status:

• status.conditionalUpdateRisks,

• status.conditionalUpdate.riskNames, and

• status.conditionalUpdate.risks.conditions

• CVO adds risk names into status.history.acceptedRisks if a conditional
  update is accepted.

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.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 5eee754 and 220ca42.

📒 Files selected for processing (11)

• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo.go
• pkg/cvo/cvo_test.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status.go
• pkg/cvo/status_test.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/cvo/cvo.go
• pkg/featuregates/featuregates.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/cvo/status_test.go
• pkg/internal/constants.go
• pkg/cvo/status.go
• pkg/cvo/cvo_test.go
• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go

🧬 Code graph analysis (3)

pkg/payload/precondition/clusterversion/recommendedupdate.go (1)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/cvo/availableupdates.go (2)

pkg/clusterconditions/clusterconditions.go (2)

• Condition (19-29)
• ConditionRegistry (31-42)

pkg/internal/constants.go (2)

• ConditionalUpdateConditionTypeRecommended (76-76)
• ConditionalUpdateRiskConditionTypeApplies (80-80)

pkg/cvo/availableupdates_test.go (3)

pkg/featuregates/featuregates.go (1)

• DefaultCvoGates (75-83)

pkg/clusterconditions/mock/mock.go (2)

• Mock (35-44)
• MatchResult (14-20)

pkg/cvo/cvo.go (1)

• New (204-296)

🔇 Additional comments (9)

pkg/payload/precondition/clusterversion/recommendedupdate.go (1)

14-14: LGTM! Improved maintainability.

Replacing the string literal "Recommended" with the centralized constant internal.ConditionalUpdateConditionTypeRecommended improves maintainability and consistency across the codebase. This follows best practices for avoiding magic strings.

Also applies to: 55-55

pkg/internal/constants.go (1)

74-80: LGTM! Well-documented constants.

The new constants ConditionalUpdateConditionTypeRecommended and ConditionalUpdateRiskConditionTypeApplies centralize string literals for conditional update condition types. The clear documentation and public visibility support consistent usage across the codebase, improving maintainability.

pkg/featuregates/featuregates.go (1)

39-40: LGTM! Consistent feature gate implementation.

The AcceptRisks feature gate implementation follows the established pattern used by other gates in this file (StatusReleaseArchitecture, CVOConfiguration):

• Interface method, private struct field, and accessor method
• Default value of false is safe and conservative
• Enable and disable cases correctly toggle the flag

This consistency makes the code predictable and maintainable.

Also applies to: 55-55, 70-72, 81-81, 103-104, 113-114

pkg/cvo/cvo.go (1)

1095-1102: LGTM! Consistent gating pattern.

The shouldReconcileAcceptRisks() method follows the same pattern as shouldReconcileCVOConfiguration():

• Checks the feature gate
• Excludes HyperShift environments
• Has clear documentation

This consistency in gating logic makes the codebase predictable and maintainable.

pkg/cvo/status_test.go (3)

206-206: LGTM! Test scaffolding properly extended.

The fakeRiFlags test struct now implements the complete CvoGateChecker interface by adding the acceptRisks field and AcceptRisks() method. This enables testing of the new feature gate behavior.

Also applies to: 221-223

---

749-751: LGTM! Appropriate gate state for tests.

The updated call to updateClusterVersionStatus passes a function returning false for shouldReconcileAcceptRisks, which correctly tests the behavior when the feature gate is disabled.

---

946-971: LGTM! Comprehensive test coverage.

The new test data (cu1, cu2) and test functions (Test_conditionalUpdateWithRiskNamesAndRiskConditions, Test_conditionalUpdateRisks) provide good coverage for the risk-related logic. The use of DeepCopy() (lines 984, 1002, 1019, 1042) correctly prevents test data mutation issues.

Also applies to: 973-1149

pkg/cvo/availableupdates_test.go (1)

211-217: Risk-aware tests and cmp options look consistent with the new semantics

The additions here (ignoring ShouldReconcileAcceptRisks via availableUpdatesCmpOpts, populating RiskConditions expectations, and extending TestEvaluateConditionalUpdate / TestSyncAvailableUpdatesDesiredUpdate to cover accept‑risks and per‑risk conditions) line up with the implementation in availableupdates.go. The defaulting of shouldReconcileAcceptRisks and riskConditions in the tests also matches the production contract.

No issues from my side on these changes.

Also applies to: 236-247, 352-578, 598-835

pkg/cvo/status.go (1)

39-41: Risk wiring and status helpers are coherent with the new accept-risks flow

• Switching findRecommendedCondition to use internal.ConditionalUpdateConditionTypeRecommended ensures consistent type usage across the codebase.
• updateClusterVersionStatus now:
  • Safely derives riskNamesForDesiredImage via conditionalUpdateWithRiskNamesAndRiskConditions, and only when shouldReconcileAcceptRisks() is true.
  • Populates ConditionalUpdateRisks via conditionalUpdateRisks, giving consumers a flatter view of risks.
  • Extends risksMsg with a clear explanation when desired image equals the loaded payload and there are accepted risks associated with that image.
• conditionalUpdateWithRiskNamesAndRiskConditions is defensive against getAvailableUpdates() being nil and realigns risk conditions from RiskConditions back into cvStatus.ConditionalUpdates without risking panics.

This all looks reasonable and correctly scoped to the accept‑risks feature.

Also applies to: 180-199, 228-247, 425-489

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

pkg/cvo/availableupdates.go (1)

576-578: riskConditions entries are not refreshed on re-evaluation.

When needFreshFetch=false but needsConditionalUpdateEval=true, the RiskConditions map is reused from the previous fetch (via getAvailableUpdates() at line 63). The if !ok guard prevents updating existing entries, so risk conditions can become stale (e.g., an "Applies=Unknown" risk later becoming "Applies=True" won't be reflected).

Consider always overwriting the entry to ensure RiskConditions reflects the latest evaluation:

🔎 Proposed fix

-		if _, ok := riskConditions[risk.Name]; !ok {
-			riskConditions[risk.Name] = []metav1.Condition{riskCondition}
-		}
+		// Always refresh the risk condition to reflect the latest evaluation
+		riskConditions[risk.Name] = []metav1.Condition{riskCondition}

This aligns with the past review comment that was marked as addressed but the fix appears incomplete.

🧹 Nitpick comments (2)

pkg/cvo/availableupdates.go (1)

520-526: Consider adding parentheses for clarity in reason precedence logic.

The operator precedence in these conditions makes the logic hard to follow:

• Line 520: now == ... || now == ... && now != want — && binds tighter than ||
• Line 522: now == ... && want == ... || now == want

While the logic may be correct, explicit parentheses would improve readability and prevent accidental bugs during future modifications.

🔎 Suggested clarification

 func newRecommendedReason(now, want string) string {
 	switch {
-	case now == recommendedReasonRisksNotExposed || now == recommendedReasonExposedOnlyToAcceptedRisks && now != want:
+	case now == recommendedReasonRisksNotExposed || (now == recommendedReasonExposedOnlyToAcceptedRisks && now != want):
 		return want
-	case now == recommendedReasonExposedOnlyToAcceptedRisks && want == recommendedReasonRisksNotExposed || now == want:
+	case (now == recommendedReasonExposedOnlyToAcceptedRisks && want == recommendedReasonRisksNotExposed) || now == want:
 		return now
 	default:
 		return recommendedReasonMultiple
 	}
 }

pkg/cvo/status.go (1)

440-448: Consider adding a warning log for the fallback condition.

The defensive fallback for missing risk conditions (reason InternalErrorNoConditionCollected) indicates an unexpected state. A warning log would help diagnose issues in production without requiring additional debugging.

🔎 Suggested enhancement

 		conditions, ok := riskConditions[risk.Name]
 		if !ok {
 			// This should never happen
+			klog.Warningf("No conditions collected for risk %q, using fallback", risk.Name)
 			conditions = []metav1.Condition{
 				{
 					Type:   internal.ConditionalUpdateRiskConditionTypeApplies,
 					Status: metav1.ConditionUnknown,
 					Reason: "InternalErrorNoConditionCollected",
 				},
 			}
 		}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 220ca42 and b28ef90.

📒 Files selected for processing (11)

• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo.go
• pkg/cvo/cvo_test.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status.go
• pkg/cvo/status_test.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🚧 Files skipped from review as they are similar to previous changes (6)

• pkg/cvo/metrics_test.go
• pkg/cvo/metrics.go
• pkg/cvo/cvo_test.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/cvo/status_test.go
• pkg/internal/constants.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/cvo/cvo.go
• pkg/cvo/availableupdates.go
• pkg/cvo/status.go
• pkg/featuregates/featuregates.go
• pkg/cvo/availableupdates_test.go

🧬 Code graph analysis (3)

pkg/cvo/availableupdates.go (2)

pkg/clusterconditions/clusterconditions.go (2)

• Condition (19-29)
• ConditionRegistry (31-42)

pkg/internal/constants.go (2)

• ConditionalUpdateConditionTypeRecommended (76-76)
• ConditionalUpdateRiskConditionTypeApplies (80-80)

pkg/cvo/status.go (2)

pkg/internal/constants.go (1)

• ConditionalUpdateConditionTypeRecommended (76-76)

pkg/featuregates/featuregates.go (1)

• CvoGateChecker (19-41)

pkg/cvo/availableupdates_test.go (3)

pkg/clusterconditions/clusterconditions.go (1)

• Condition (19-29)

pkg/featuregates/featuregates.go (1)

• DefaultCvoGates (75-83)

pkg/clusterconditions/mock/mock.go (2)

• Mock (35-44)
• MatchResult (14-20)

🔇 Additional comments (8)

pkg/featuregates/featuregates.go (1)

38-40: LGTM! Clean feature gate wiring.

The AcceptRisks feature gate follows the established pattern used by other CVO gates (StatusReleaseArchitecture, CVOConfiguration). The default-off behavior is appropriately conservative, and both enable/disable paths are handled correctly in CvoGatesFromFeatureGate.

Also applies to: 55-55, 70-72, 81-81, 103-104, 113-114

pkg/cvo/availableupdates.go (2)

55-60: LGTM! Nil guard properly implemented.

The nil check for config.Spec.DesiredUpdate before accessing AcceptRisks is correctly in place, preventing potential panics.

---

140-142: LGTM! New fields properly wired.

The new ShouldReconcileAcceptRisks, AcceptRisks, and RiskConditions fields are correctly initialized on fresh fetches and properly copied in getAvailableUpdates(). The map initialization at line 142 ensures no nil-map panics during conditional update evaluation.

Also applies to: 180-212

pkg/cvo/cvo.go (1)

1094-1102: LGTM! Consistent gating pattern.

The shouldReconcileAcceptRisks method correctly mirrors the established pattern from shouldReconcileCVOConfiguration, with appropriate HyperShift exclusion and feature gate checking.

pkg/cvo/availableupdates_test.go (2)

352-577: LGTM! Comprehensive test coverage for risk acceptance.

The test cases thoroughly cover:

• No risks scenario
• Single risk that doesn't match
• Single risk that matches (rejected)
• Single risk that matches and is accepted (new scenario)
• Multiple risks with mixed matches
• Evaluation failures

The default initialization at lines 560-567 ensures shouldReconcileAcceptRisks returns false when not explicitly set, preventing accidental test pollution.

---

472-498: No action required. The ValidQueue size mismatch is not a problem because Valid() is never called during the execution of this test case. The evaluation logic only calls Match(), not PruneInvalid(), so the ValidQueue setup is unused.

Likely an incorrect or invalid review comment.

pkg/cvo/status.go (2)

425-473: LGTM! Past issues properly addressed.

The function correctly handles:

1. Nil check for getAvailableUpdates() at lines 429-432 (prevents nil pointer dereference)
2. riskNamesForDesiredImage is captured at line 469 after conditionalUpdate.RiskNames is computed at line 464 (correct ordering)
3. Fallback condition at lines 442-448 handles missing risk conditions gracefully

---

191-199: LGTM! Clean risk-aware status processing.

The extended updateClusterVersionStatus signature properly threads shouldReconcileAcceptRisks through the call chain. The conditional processing at lines 229-232 only activates when the feature gate is enabled, and the risk message construction at lines 241-247 provides clear, informative status messages.

Also applies to: 228-248

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

pkg/cvo/availableupdates.go (1)

576-578: riskConditions entries are not refreshed on re-evaluation.

The if !ok guard only inserts a condition when the key is missing. On re-evaluations without a fresh fetch (when needFreshFetch=false but needsConditionalUpdateEval=true), existing entries are preserved and never updated, causing stale conditions to persist even when cluster state changes.

Always overwrite to reflect the latest evaluation:

-		if _, ok := riskConditions[risk.Name]; !ok {
-			riskConditions[risk.Name] = []metav1.Condition{riskCondition}
-		}
+		riskConditions[risk.Name] = []metav1.Condition{riskCondition}

🧹 Nitpick comments (1)

pkg/cvo/availableupdates_test.go (1)

352-575: EvaluateConditionalUpdate tests thoroughly cover risk evaluation and acceptance

The expanded table tests (including accepted risks, multiple risks, and evaluation failures) plus explicit riskConditions assertions give good coverage of the new API shape and behavior. You might consider one additional case where acceptRisks is non-empty but shouldReconcileAcceptRisks returns false, to lock in that acceptance is ignored when the feature-gate is off, but this can be deferred.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between b28ef90 and 32fa03f.

📒 Files selected for processing (11)

• pkg/cvo/availableupdates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/cvo.go
• pkg/cvo/cvo_test.go
• pkg/cvo/metrics.go
• pkg/cvo/metrics_test.go
• pkg/cvo/status.go
• pkg/cvo/status_test.go
• pkg/featuregates/featuregates.go
• pkg/internal/constants.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go

🚧 Files skipped from review as they are similar to previous changes (4)

• pkg/internal/constants.go
• pkg/cvo/metrics_test.go
• pkg/payload/precondition/clusterversion/recommendedupdate.go
• pkg/cvo/metrics.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/cvo/status_test.go
• pkg/cvo/cvo.go
• pkg/featuregates/featuregates.go
• pkg/cvo/availableupdates_test.go
• pkg/cvo/status.go
• pkg/cvo/cvo_test.go
• pkg/cvo/availableupdates.go

🧬 Code graph analysis (2)

pkg/cvo/availableupdates_test.go (2)

pkg/featuregates/featuregates.go (1)

• DefaultCvoGates (75-83)

pkg/clusterconditions/mock/mock.go (2)

• Mock (35-44)
• MatchResult (14-20)

pkg/cvo/status.go (2)

pkg/internal/constants.go (2)

• ConditionalUpdateConditionTypeRecommended (76-76)
• ConditionalUpdateRiskConditionTypeApplies (80-80)

pkg/featuregates/featuregates.go (1)

• CvoGateChecker (19-41)

🔇 Additional comments (15)

pkg/cvo/cvo.go (1)

1095-1102: AcceptRisks reconciliation gate matches configuration gate pattern

Gating on enabledFeatureGates.AcceptRisks() and !optr.hypershift is consistent with shouldReconcileCVOConfiguration and the PR’s stated behavior; no issues here.

pkg/cvo/status_test.go (1)

202-224: Risk-name/condition tests and gate-plumbing look correct

Extending fakeRiFlags with AcceptRisks(), threading the new shouldReconcileAcceptRisks callback into updateClusterVersionStatus, and the new conditionalUpdate* tests collectively exercise the new risk-name and risk-condition behavior well, without mutating shared fixtures or introducing flakes.

Also applies to: 749-751, 946-1149

pkg/cvo/cvo_test.go (1)

19-20: cmp.Diff + field-ignore keeps tests stable around new gate field

Switching to cmp.Diff and ignoring ShouldReconcileAcceptRisks isolates tests from this internal gate while still asserting all other availableUpdates state, which is appropriate here.

Also applies to: 2792-2793

pkg/featuregates/featuregates.go (1)

39-41: AcceptRisks gate wiring is consistent and conservative

Adding AcceptRisks to CvoGateChecker/CvoGates, defaulting it to false, and toggling it with FeatureGateClusterUpdateAcceptRisks mirrors the existing gate patterns and provides a safe default when version gating is unknown.

Also applies to: 55-56, 70-72, 81-82, 103-105, 113-115

pkg/cvo/availableupdates_test.go (1)

22-23: Gating and RiskConditions expectations are wired correctly into available-updates tests

Ignoring ShouldReconcileAcceptRisks in comparisons, asserting RiskConditions, and initializing enabledFeatureGates via featuregates.DefaultCvoGates("version") keeps the tests aligned with the new risk-acceptance behavior without over-relaxing assertions.

Also applies to: 29-30, 211-217, 236-239, 327-328, 236-237, 811-823

pkg/cvo/status.go (5)

39-41: LGTM!

Using the centralized internal.ConditionalUpdateConditionTypeRecommended constant improves consistency across the codebase.

---

191-199: LGTM!

The expanded signature properly threads the feature gate callback through to enable conditional risk reconciliation.

---

228-247: LGTM!

The risk message construction correctly handles both scenarios (empty and non-empty initial risksMsg), and the feature gate check ensures this logic only runs when enabled.

---

425-474: LGTM!

The nil check for getAvailableUpdates() and the corrected ordering (capturing riskNamesForDesiredImage after computing RiskNames) properly address the previously identified issues.

---

476-489: LGTM!

The deduplication logic correctly aggregates unique risks across all conditional updates.

pkg/cvo/availableupdates.go (5)

55-60: LGTM!

The nil guard for config.Spec.DesiredUpdate properly prevents the potential panic identified previously.

---

140-142: LGTM!

Proper initialization of new fields including the empty map for RiskConditions when performing a fresh fetch.

---

311-322: Verify: RiskConditions is a shallow copy (map reference).

Unlike Updates and ConditionalUpdates which are deep-copied (lines 325-337), RiskConditions is assigned directly, meaning mutations to the returned struct's RiskConditions will affect the cached optr.availableUpdates.RiskConditions. This may be intentional for cache coherence, but confirm this is the desired behavior.

---

450-451: LGTM!

The call site properly threads through the new risk-related parameters.

---

518-527: Verify operator precedence in the condition.

Line 520: now == recommendedReasonRisksNotExposed || now == recommendedReasonExposedOnlyToAcceptedRisks && now != want

Due to operator precedence (&& binds tighter than ||), this parses as:
now == recommendedReasonRisksNotExposed || (now == recommendedReasonExposedOnlyToAcceptedRisks && now != want)

If the intent was to return want when now is either "safe" reason AND differs from want, consider adding parentheses for clarity or confirm this logic is intentional.

[openshift-ci]

@hongkailiu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agnostic-operator	32fa03f	link	true	/test e2e-agnostic-operator
ci/prow/e2e-hypershift-conformance	32fa03f	link	true	/test e2e-hypershift-conformance
ci/prow/e2e-hypershift	32fa03f	link	true	/test e2e-hypershift
ci/prow/e2e-agnostic-ovn-upgrade-out-of-change	32fa03f	link	true	/test e2e-agnostic-ovn-upgrade-out-of-change
ci/prow/e2e-agnostic-ovn	32fa03f	link	true	/test e2e-agnostic-ovn
ci/prow/okd-scos-images	32fa03f	link	true	/test okd-scos-images
ci/prow/e2e-aws-ovn-techpreview	32fa03f	link	true	/test e2e-aws-ovn-techpreview

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.