OTA-1546: Reconcile cv.spec.desiredUpdate.acceptRisks #1284
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds support for accepting update risks: feature-gate wiring, operator gating, propagation of accepted-risk state into available-updates and status, per-risk condition tracking, signature and constant updates, test coverage, and E2E/utilities for accept-risks behavior. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Comment |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
evaluateConditionalUpdateis called once per ConditionalUpdate with a sharedriskConditionsmap (line 444), conditions for the same risk name across different updates are overwritten or ignored.Additionally, the
TestEvaluateConditionalUpdatetest callsevaluateConditionalUpdatewith 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
⛔ Files ignored due to path filters (35)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_feature.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_scheduling.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
go.modpkg/cvo/availableupdates.gopkg/cvo/cvo.gopkg/cvo/status.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.gopkg/internal/constants.gopkg/cvo/cvo.gopkg/featuregates/featuregates.gogo.modpkg/cvo/status.gopkg/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.ConditionalUpdateConditionTypeRecommendedinstead 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 asshouldReconcileCVOConfiguration()and correctly gates the functionality behind theAcceptRisksfeature gate while excluding HyperShift environments.pkg/cvo/availableupdates.go (1)
177-207: LGTM on struct additions.The new fields
ShouldReconcileAcceptRisks,AcceptRisks, andRiskConditionsare well-placed in the struct and align with the risk acceptance feature requirements.pkg/featuregates/featuregates.go (2)
38-41: LGTM!The
AcceptRisksfeature gate is properly added following the established pattern for other CVO feature gates (StatusReleaseArchitecture,CVOConfiguration).Also applies to: 55-56, 70-72
103-104: VerifyFeatureGateClusterUpdateAcceptRisksis exported by the features package.The code references
features.FeatureGateClusterUpdateAcceptRisksimported fromgithub.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 officialopenshift/apipackage.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
shouldReconcileAcceptRiskscallback to gate the risk reconciliation behavior.
452-465: LGTM onconditionalUpdateRiskshelper.The function correctly deduplicates risks by name and returns a flat list of unique
ConditionalUpdateRiskentries.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
replacedirective points to a personal fork (github.com/hongkailiu/api) instead of the officialgithub.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.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/cvo/status.go (1)
425-454: Add nil check forgetAvailableUpdates()at line 428.Line 428 directly accesses
getAvailableUpdates().RiskConditions, but the function signature indicatesgetAvailableUpdatescan returnnil. This would cause a panic. ThemergeReleaseMetadatafunction at line 227 safely checks for nil (cvo.go:909), butconditionalUpdateWithRiskNamesAndRiskConditionsdoes 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:
- Build a set of risk names from
conditionalUpdate.Risks- Remove stale conditions (not in fetched
riskConditions)- Merge in new conditions from
riskConditionsHowever, 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.Conditionsif they're not found inriskConditions[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
⛔ Files ignored due to path filters (23)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/custom.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gathererconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagatherspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeclaimreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/generated_expansion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/interface.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/expansion_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (11)
go.modpkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/metrics.gopkg/cvo/metrics_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.modpkg/cvo/metrics_test.gopkg/cvo/status_test.gopkg/payload/precondition/clusterversion/recommendedupdate.gopkg/cvo/status.gopkg/cvo/metrics.gopkg/featuregates/featuregates.gopkg/cvo/availableupdates.gopkg/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/apiat version20251222154221, but line 97 replaces it with a fork at version20251223025917. Line 99 replacesopenshift/client-gowith 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.ConditionalUpdateConditionTypeRecommendedinstead 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.ConditionalUpdateConditionTypeRecommendedis 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.ConditionalUpdateConditionTypeRecommendedmaintains 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:
fakeRiFlagsnow implements the completeCvoGateCheckerinterface with the newAcceptRisks()method- The
updateClusterVersionStatuscall includes the newshouldReconcileAcceptRiskscallback with a conservative default (false) appropriate for this test's focus on error filteringAlso applies to: 221-223, 749-751
pkg/cvo/status.go (6)
20-20: LGTM! Import addition is appropriate.The
setsimport is correctly added to support the new risk acceptance functionality.
44-44: LGTM! Constant usage centralized.The switch to
internal.ConditionalUpdateConditionTypeRecommendedaligns with the PR's objective of using centralized internal constants.
195-203: LGTM! Function signature properly extended.The addition of
shouldReconcileAcceptRisks func() boolparameter is appropriate for feature-gating the risk acceptance logic, and the call site at line 184 correctly passesoptr.shouldReconcileAcceptRisks.
232-236: LGTM! Risk processing logic is properly gated.The risk-aware processing integration correctly:
- Gates execution with
shouldReconcileAcceptRisks()- Computes
riskNamesForDesiredImagefor the desired image- Updates
ConditionalUpdatesandConditionalUpdateRisksfields
241-247: LGTM! Risk message construction logic is correct.The conditional message appending properly handles both scenarios:
- Empty
risksMsg: constructs a complete sentence- Non-empty
risksMsg: appends with appropriate separatorThe logic is correctly gated by both
shouldReconcileAcceptRisks()and the presence ofriskNamesForDesiredImage.
456-469: LGTM! Risk deduplication logic is correct.The
conditionalUpdateRisksfunction 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
setsimport is appropriately added to support the newAcceptRisksfield handling in tests.
29-29: LGTM! Feature gates import is appropriate.The
featuregatesimport enables test setup to configureenabledFeatureGates, which is essential for testing the new gated behavior.
211-212: LGTM! Comparison option correctly ignores function field.Adding
ShouldReconcileAcceptRisksto 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
enabledFeatureGateswithDefaultCvoGatesbefore invokingsyncAvailableUpdates, ensuring the code under test has access to gate-checking functionality.
515-517: LGTM! Test call signature correctly updated.The
evaluateConditionalUpdatecall is properly updated with the new parameters:
sets.New[string]()for empty accepted risksfunc() bool { return false }to disable risk acceptancemap[string][]metav1.Condition{}for empty risk conditionsThese 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
enabledFeatureGatesbefore calling sync methods, ensuring uniform test behavior.pkg/cvo/availableupdates.go (11)
19-19: LGTM! Import addition is appropriate.The
setsimport correctly supports the newAcceptRisksfield of typesets.Set[string].
26-26: LGTM! Internal constants import is appropriate.The
internalpackage import provides access to centralized constants likeConditionalUpdateConditionTypeRecommended, aligning with the PR's objective of using internal constants.
55-60: LGTM! Nil-safety properly implemented.The
acceptRisksinitialization correctly guards against a nilDesiredUpdateby checkingconfig.Spec.DesiredUpdate != nilbefore accessing itsAcceptRisksfield. This addresses the previously flagged nil pointer concern.
140-141: LGTM! AvailableUpdates fields properly initialized.The
ShouldReconcileAcceptRisksandAcceptRisksfields 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) andAcceptRisks(risk name set) are correctly typed and positioned to support the risk acceptance feature.
208-208: LGTM! RiskConditions field appropriately added.The
RiskConditionsmap 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:
ShouldReconcileAcceptRisksuses the current operator method (ensuring latest gate state)- Line 312:
AcceptRisksis copied from stored stateObservation:
RiskConditionsis not copied here and remains nil in the returned struct. This appears intentional sinceRiskConditionsis populated duringevaluateConditionalUpdates. Verify this is the expected behavior.
446-446: LGTM! Function call correctly updated.The
evaluateConditionalUpdatecall properly passes the new parameters (u.AcceptRisks,u.ShouldReconcileAcceptRisks,u.RiskConditions) to support risk acceptance evaluation.
505-505: LGTM! Constant appropriately added.The
riskConditionReasonEvaluationFailedconstant provides a consistent reason value for risk condition evaluation failures.
523-530: LGTM! Function signature appropriately extended.The
evaluateConditionalUpdatesignature is correctly extended with:
acceptRisks sets.Set[string]: accepted risk namesshouldReconcileAcceptRisks func() bool: feature gate checkriskConditions map[string][]metav1.Condition: per-risk condition trackingAll call sites have been updated accordingly.
532-532: LGTM! Constant usage centralized.The switch to
internal.ConditionalUpdateConditionTypeRecommendedaligns with the PR's objective of using internal constants consistently.
hongkailiu
force-pushed
the
accept-cvo
branch
2 times, most recently
from
a46418c to
5eee754
Compare
There was a problem hiding this comment.
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:
Map nil panic (duplicate concern from past review): If
riskConditionsis nil when line 573 executes, it will panic. This can occur whenneedFreshFetchis false and the cachedRiskConditionsis nil (e.g., from an older state before line 142 was added, or if the map reference from line 314 is nil).Stale conditions on re-evaluation: The check
if _, ok := riskConditions[risk.Name]; !okmeans existing conditions are never updated. WhenevaluateConditionalUpdatesis called multiple times (e.g., whenneedFreshFetchis 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
evaluateConditionalUpdatesbefore 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 whengetAvailableUpdates()is nilWhen
shouldReconcileAcceptRisks()is true butgetAvailableUpdates()returns nil,conditionalUpdateWithRiskNamesAndRiskConditionstreatsriskConditionsas empty and will remove all existingrisk.ConditionsfromcvStatus.ConditionalUpdatesbefore re‑adding none. If CVO restarts with preexisting conditional updates in status but has not yet refreshed in‑memoryavailableUpdates, this could transiently clear risk condition details in status. You may want to early‑return the originalconditionalUpdatesunchanged whenupdates == nilto 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
📒 Files selected for processing (11)
pkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/cvo/metrics.gopkg/cvo/metrics_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.gopkg/cvo/status.gopkg/featuregates/featuregates.gopkg/cvo/availableupdates.gopkg/cvo/cvo_test.gopkg/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 correctlyThe new
shouldReconcileAcceptRiskshelper cleanly reuses the feature‑gate plus!hypershiftpattern fromshouldReconcileCVOConfiguration, 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 gatesThe
AcceptRisksflag is correctly plumbed through theCvoGateCheckerinterface,CvoGatesstruct, defaults, andCvoGatesFromFeatureGateenable/disable logic, mirroring the existing gate patterns. Only caveat is that adding a method toCvoGateCheckeris 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 behaviorUsing
cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks")and initializingoptr.enabledFeatureGateswithfeaturegates.DefaultCvoGates("version")keeps existing tests focused on functional behavior while accommodating the new gate. The extendedcheckStatusgeneric to include*configv1.ClusterVersionfits 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 correctUsing
internal.ConditionalUpdateConditionTypeRecommendedinfindRecommendedConditionremoves the hard‑coded string, and threadingshouldReconcileAcceptRisksintoupdateClusterVersionStatusgives a single gate for all accept‑risks behavior. The new helpers cleanly recomputeConditionalUpdates, populateConditionalUpdateRisks, and, via the localupdates := getAvailableUpdates()guard, avoid nil dereferences whenavailableUpdatesis 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 propagationThe new table-driven cases for
evaluateConditionalUpdateand the expectations onRiskConditionsin the sync tests exercise zero/one/many risks, matches vs. non-matches, evaluation failures, and the “accepted risk” path viaacceptRisks/shouldReconcileAcceptRisks. InitializingenabledFeatureGateswithfeaturegates.DefaultCvoGates("version")and ignoringShouldReconcileAcceptRisksin 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.DesiredUpdatecorrectly 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
ShouldReconcileAcceptRisksallows for dynamic evaluation, and the map-basedRiskConditionsstructure 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
recommendedReasonExposedOnlyToAcceptedRisksstate, 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 inacceptRisks- 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
AcceptRisksandRiskConditions. The concern about shared map mutations is unfounded—these fields are mutated only during the construction phase inevaluateConditionalUpdates()(called at line 158), not aftergetAvailableUpdates()returns a read-only snapshot.The nil panic scenario cannot occur:
RiskConditionsis always initialized to an empty map at line 142 before any use. The intentional shallow-copy pattern (evidenced byConditionRegistryat line 318) reflects the design choice to return snapshots for read access to cached state.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
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
📒 Files selected for processing (11)
pkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/cvo/metrics.gopkg/cvo/metrics_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.gopkg/cvo/status.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo_test.gopkg/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 conservativeInterface, struct field, default initialization, and FeatureGate mapping for
FeatureGateClusterUpdateAcceptRisksare all consistent and symmetric; defaulting tofalsein 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 soundUsing
cmp.Diffwithcmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks")and explicitly assertingRiskConditionsin 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 behaviorThe 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 cleanlyPlumbing
getAvailableUpdatesandshouldReconcileAcceptRisksintoupdateClusterVersionStatus, and only enrichingConditionalUpdates,ConditionalUpdateRisks, andAcceptedRisks/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-endBuilding
acceptRisksfromspec.desiredUpdate.acceptRisks, threading it plusShouldReconcileAcceptRisksandRiskConditionsthroughavailableUpdatesintoevaluateConditionalUpdate, 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
|
@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. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
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
📒 Files selected for processing (11)
pkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/cvo/metrics.gopkg/cvo/metrics_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.gopkg/featuregates/featuregates.gopkg/payload/precondition/clusterversion/recommendedupdate.gopkg/cvo/status_test.gopkg/internal/constants.gopkg/cvo/status.gopkg/cvo/cvo_test.gopkg/cvo/availableupdates.gopkg/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 constantinternal.ConditionalUpdateConditionTypeRecommendedimproves 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
ConditionalUpdateConditionTypeRecommendedandConditionalUpdateRiskConditionTypeAppliescentralize 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
AcceptRisksfeature 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
falseis 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 asshouldReconcileCVOConfiguration():
- 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
fakeRiFlagstest struct now implements the completeCvoGateCheckerinterface by adding theacceptRisksfield andAcceptRisks()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
updateClusterVersionStatuspasses a function returningfalseforshouldReconcileAcceptRisks, 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 ofDeepCopy()(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 semanticsThe additions here (ignoring
ShouldReconcileAcceptRisksviaavailableUpdatesCmpOpts, populatingRiskConditionsexpectations, and extendingTestEvaluateConditionalUpdate/TestSyncAvailableUpdatesDesiredUpdateto cover accept‑risks and per‑risk conditions) line up with the implementation inavailableupdates.go. The defaulting ofshouldReconcileAcceptRisksandriskConditionsin 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
findRecommendedConditionto useinternal.ConditionalUpdateConditionTypeRecommendedensures consistent type usage across the codebase.updateClusterVersionStatusnow:
- Safely derives
riskNamesForDesiredImageviaconditionalUpdateWithRiskNamesAndRiskConditions, and only whenshouldReconcileAcceptRisks()is true.- Populates
ConditionalUpdateRisksviaconditionalUpdateRisks, giving consumers a flatter view of risks.- Extends
risksMsgwith a clear explanation when desired image equals the loaded payload and there are accepted risks associated with that image.conditionalUpdateWithRiskNamesAndRiskConditionsis defensive againstgetAvailableUpdates()being nil and realigns risk conditions fromRiskConditionsback intocvStatus.ConditionalUpdateswithout risking panics.This all looks reasonable and correctly scoped to the accept‑risks feature.
Also applies to: 180-199, 228-247, 425-489
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cvo/availableupdates.go (1)
576-578:riskConditionsentries are not refreshed on re-evaluation.When
needFreshFetch=falsebutneedsConditionalUpdateEval=true, theRiskConditionsmap is reused from the previous fetch (viagetAvailableUpdates()at line 63). Theif !okguard 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
RiskConditionsreflects 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 == wantWhile 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
📒 Files selected for processing (11)
pkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/cvo/metrics.gopkg/cvo/metrics_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.gopkg/cvo/availableupdates.gopkg/cvo/status.gopkg/featuregates/featuregates.gopkg/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
AcceptRisksfeature 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 inCvoGatesFromFeatureGate.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.DesiredUpdatebefore accessingAcceptRisksis correctly in place, preventing potential panics.
140-142: LGTM! New fields properly wired.The new
ShouldReconcileAcceptRisks,AcceptRisks, andRiskConditionsfields are correctly initialized on fresh fetches and properly copied ingetAvailableUpdates(). 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
shouldReconcileAcceptRisksmethod correctly mirrors the established pattern fromshouldReconcileCVOConfiguration, 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
shouldReconcileAcceptRisksreturnsfalsewhen not explicitly set, preventing accidental test pollution.
472-498: No action required. TheValidQueuesize mismatch is not a problem becauseValid()is never called during the execution of this test case. The evaluation logic only callsMatch(), notPruneInvalid(), so theValidQueuesetup 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:
- Nil check for
getAvailableUpdates()at lines 429-432 (prevents nil pointer dereference)riskNamesForDesiredImageis captured at line 469 afterconditionalUpdate.RiskNamesis computed at line 464 (correct ordering)- Fallback condition at lines 442-448 handles missing risk conditions gracefully
191-199: LGTM! Clean risk-aware status processing.The extended
updateClusterVersionStatussignature properly threadsshouldReconcileAcceptRisksthrough 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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cvo/availableupdates.go (1)
576-578:riskConditionsentries are not refreshed on re-evaluation.The
if !okguard only inserts a condition when the key is missing. On re-evaluations without a fresh fetch (whenneedFreshFetch=falsebutneedsConditionalUpdateEval=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 acceptanceThe expanded table tests (including accepted risks, multiple risks, and evaluation failures) plus explicit
riskConditionsassertions give good coverage of the new API shape and behavior. You might consider one additional case whereacceptRisksis non-empty butshouldReconcileAcceptRisksreturns 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
📒 Files selected for processing (11)
pkg/cvo/availableupdates.gopkg/cvo/availableupdates_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.gopkg/cvo/metrics.gopkg/cvo/metrics_test.gopkg/cvo/status.gopkg/cvo/status_test.gopkg/featuregates/featuregates.gopkg/internal/constants.gopkg/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.gopkg/cvo/cvo.gopkg/featuregates/featuregates.gopkg/cvo/availableupdates_test.gopkg/cvo/status.gopkg/cvo/cvo_test.gopkg/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 patternGating on
enabledFeatureGates.AcceptRisks()and!optr.hypershiftis consistent withshouldReconcileCVOConfigurationand the PR’s stated behavior; no issues here.pkg/cvo/status_test.go (1)
202-224: Risk-name/condition tests and gate-plumbing look correctExtending
fakeRiFlagswithAcceptRisks(), threading the newshouldReconcileAcceptRiskscallback intoupdateClusterVersionStatus, and the newconditionalUpdate*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 fieldSwitching to
cmp.Diffand ignoringShouldReconcileAcceptRisksisolates tests from this internal gate while still asserting all otheravailableUpdatesstate, which is appropriate here.Also applies to: 2792-2793
pkg/featuregates/featuregates.go (1)
39-41: AcceptRisks gate wiring is consistent and conservativeAdding
AcceptRiskstoCvoGateChecker/CvoGates, defaulting it to false, and toggling it withFeatureGateClusterUpdateAcceptRisksmirrors 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 testsIgnoring
ShouldReconcileAcceptRisksin comparisons, assertingRiskConditions, and initializingenabledFeatureGatesviafeaturegates.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.ConditionalUpdateConditionTypeRecommendedconstant 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 (capturingriskNamesForDesiredImageafter computingRiskNames) 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.DesiredUpdateproperly prevents the potential panic identified previously.
140-142: LGTM!Proper initialization of new fields including the empty map for
RiskConditionswhen performing a fresh fetch.
311-322: Verify:RiskConditionsis a shallow copy (map reference).Unlike
UpdatesandConditionalUpdateswhich are deep-copied (lines 325-337),RiskConditionsis assigned directly, meaning mutations to the returned struct'sRiskConditionswill affect the cachedoptr.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 != wantDue to operator precedence (
&&binds tighter than||), this parses as:
now == recommendedReasonRisksNotExposed || (now == recommendedReasonExposedOnlyToAcceptedRisks && now != want)If the intent was to return
wantwhennowis either "safe" reason AND differs fromwant, consider adding parentheses for clarity or confirm this logic is intentional.
hongkailiu
force-pushed
the
accept-cvo
branch
from
32fa03f to
83555a0
Compare
openshift-merge-robot
added
the
needs-rebase
hongkailiu
force-pushed
the
accept-cvo
branch
from
83555a0 to
d62f5d1
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cvo/availableupdates.go`:
- Around line 55-60: The code only updates acceptRisks (and the gate) inside the
needFreshFetch branch so changes to config.Spec.DesiredUpdate.AcceptRisks are
ignored until the next fetch; move or duplicate the logic that builds
acceptRisks (and sets the gate function) so it runs on every sync path (i.e.
outside the needFreshFetch conditional) using config.Spec.DesiredUpdate to
refresh the sets and gate each sync, and apply the same change to the other
occurrence referenced (the second acceptRisks/gate assignment around lines
140-142) so both code paths always reflect spec changes.
♻️ Duplicate comments (1)
pkg/cvo/availableupdates.go (1)
529-578: Always refresh RiskConditions entries on re-evaluation.The current guard only writes when a key is missing, so risk conditions can become stale across evaluations. Overwrite on every pass so status reflects the latest exposure.
🛠️ Suggested fix
- if _, ok := riskConditions[risk.Name]; !ok { - riskConditions[risk.Name] = []metav1.Condition{riskCondition} - } + riskConditions[risk.Name] = []metav1.Condition{riskCondition}
hongkailiu
force-pushed
the
accept-cvo
branch
from
d62f5d1 to
6d14cdf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cvo/availableupdates.go`:
- Around line 55-60: The acceptRisks set is only applied to optrAvailableUpdates
inside the needFreshFetch path, leaving optrAvailableUpdates.AcceptRisks stale
when needFreshFetch is false; update the code to always refresh
optrAvailableUpdates.AcceptRisks and
optrAvailableUpdates.ShouldReconcileAcceptRisks after constructing acceptRisks
(from config.Spec.DesiredUpdate.AcceptRisks) regardless of needFreshFetch
outcome—i.e., build acceptRisks as you already do, then after the needFreshFetch
conditional assign optrAvailableUpdates.AcceptRisks = acceptRisks and
recompute/set optrAvailableUpdates.ShouldReconcileAcceptRisks before returning
or using optrAvailableUpdates so spec changes take effect immediately
(references: acceptRisks, optrAvailableUpdates, needFreshFetch,
getAvailableUpdates, ShouldReconcileAcceptRisks,
config.Spec.DesiredUpdate.AcceptRisks).
♻️ Duplicate comments (1)
pkg/cvo/availableupdates.go (1)
576-578: Remove the "only if missing" guard and always update risk conditions.The guard at lines 576-578 should be removed, though not for the stated reason in the original review. The original claim about "re-evaluation without fresh fetch" is incorrect—
RiskConditionsis always reset to an empty map (line 142) beforeevaluateConditionalUpdatesis called. The guard's only real effect would be to skip updates if the same risk name appears in multipleConditionalUpdateswithin a single evaluation cycle, which is not a realistic scenario based on current usage patterns.However, removing the guard and always updating the condition is still the correct approach for defensive coding. It ensures that if risk evaluations change, the latest state is reflected, and it aligns with the expectation in
status.go(line 440-441) that all evaluated risks will have entries inRiskConditions.- if _, ok := riskConditions[risk.Name]; !ok { - riskConditions[risk.Name] = []metav1.Condition{riskCondition} - } + riskConditions[risk.Name] = []metav1.Condition{riskCondition}
🧹 Nitpick comments (2)
pkg/cvo/status_test.go (1)
945-1138: Consider using the internal Applies condition constant to avoid string drift.
This keeps tests aligned with production constants and avoids typo risk.♻️ Proposed refactor
+const appliesConditionType = internal.ConditionalUpdateRiskConditionTypeApplies + var ( cu1 = configv1.ConditionalUpdate{ Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, RiskNames: []string{"wrongName"}, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType, @@ - Type: "Applies", + Type: appliesConditionType,pkg/cvo/availableupdates.go (1)
518-527: Verify reason priority logic.The
newRecommendedReasonfunction has complex conditions for handling the newExposedOnlyToAcceptedRisksreason:
- Line 520: Returns
wantif current isNotExposedor (ExposedOnlyToAcceptedRisksAND different from want)- Line 522: Returns current if current is
ExposedOnlyToAcceptedRisksand want isNotExposed, or if they're equalThis implements priority where exposed risks take precedence over accepted risks, which take precedence over not-exposed. The logic appears correct but is subtle.
Consider adding a brief comment explaining the priority
func newRecommendedReason(now, want string) string { + // Priority: Exposed/EvaluationFailed > ExposedOnlyToAcceptedRisks > NotExposedToRisks switch { case now == recommendedReasonRisksNotExposed || now == recommendedReasonExposedOnlyToAcceptedRisks && now != want:
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.
hongkailiu
force-pushed
the
accept-cvo
branch
from
6d14cdf to
dfb63fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cvo/status.go`:
- Around line 463-466: The code only assigns conditionalUpdate.RiskNames when
riskNames.Len() > 0, leaving stale values when risks are cleared; change this to
always set conditionalUpdate.RiskNames from riskNames (i.e., remove the Len()
check and unconditionally assign conditionalUpdate.RiskNames =
sets.List[string](riskNames)) so that RiskNames accurately reflects the current
conditionalUpdate.Risks even when empty.
In `@test/cvo/accept_risks.go`:
- Around line 61-89: Backup/restoration logic and assertions around
cv.Spec.DesiredUpdate.AcceptRisks are unsafe and inverted: preserve the original
bk (already DeepCopied), and when restoring set cv.Spec.DesiredUpdate = bk;
after the update, avoid dereferencing when cv.Spec.DesiredUpdate is nil by
checking for nil first and then assert absence of the previously-added risks
(use BeFalse for slices.Contains for riskA/riskB) or, if bk was non-nil, assert
that the restored AcceptRisks equals the original bk.AcceptRisks (or contains
the same names); reference bk, cv.Spec.DesiredUpdate and AcceptRisks when making
the nil-safe checks and flipping the assertions in the "Checking that new risks
can be removed" section.
♻️ Duplicate comments (3)
pkg/cvo/cvo_test.go (1)
2791-2792: AvoidAcceptRisksnil/empty diffs inavailableUpdatescomparison.
AcceptRiskscan be initialized to an empty set even when fixtures leave it nil, which can create deterministic diffs. Consider ignoring it (or initializing in expectations).🛠️ Suggested fix
- if diff := cmp.Diff(tt.wantUpdates, optr.availableUpdates, cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks")); diff != "" { + if diff := cmp.Diff( + tt.wantUpdates, + optr.availableUpdates, + cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks", "AcceptRisks"), + ); diff != "" { t.Fatalf("unexpected: %s", diff) }pkg/cvo/availableupdates.go (2)
529-578: RiskConditions are not refreshed on re‑evaluation.
TheriskConditionsmap is only written when the key is missing, so re‑evaluation keeps stale conditions and status can drift from current cluster state.🔧 Proposed fix
- if _, ok := riskConditions[risk.Name]; !ok { - riskConditions[risk.Name] = []metav1.Condition{riskCondition} - } + // Always refresh to reflect the latest evaluation. + riskConditions[risk.Name] = []metav1.Condition{riskCondition}
55-60: Refresh accept‑risk inputs even when reusing cached updates.
AcceptRisksandShouldReconcileAcceptRisksare only assigned inside theneedFreshFetchpath, so changes tospec.desiredUpdate.acceptRiskswon’t take effect until a fresh fetch. Please refresh these after the fetch decision.🔧 Proposed fix
if needFreshFetch { // ... - optrAvailableUpdates.ShouldReconcileAcceptRisks = optr.shouldReconcileAcceptRisks - optrAvailableUpdates.AcceptRisks = acceptRisks optrAvailableUpdates.RiskConditions = map[string][]metav1.Condition{} // ... } + + // Always refresh accept-risk inputs even when skipping a fetch. + optrAvailableUpdates.ShouldReconcileAcceptRisks = optr.shouldReconcileAcceptRisks + optrAvailableUpdates.AcceptRisks = acceptRisksAlso applies to: 140-143
| if riskNames.Len() > 0 { | ||
| conditionalUpdate.RiskNames = sets.List[string](riskNames) | ||
| } | ||
| conditionalUpdate.Risks = risks |
There was a problem hiding this comment.
Clear stale RiskNames when no risks remain.
Right now RiskNames is only updated when the set is non-empty, so a previous non-empty list can persist after risks are removed, leading to stale status/history messaging. Assign it unconditionally to reflect the current Risks.
🛠️ Suggested fix
- if riskNames.Len() > 0 {
- conditionalUpdate.RiskNames = sets.List[string](riskNames)
- }
+ conditionalUpdate.RiskNames = sets.List[string](riskNames)📝 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.
| if riskNames.Len() > 0 { | |
| conditionalUpdate.RiskNames = sets.List[string](riskNames) | |
| } | |
| conditionalUpdate.Risks = risks | |
| conditionalUpdate.RiskNames = sets.List[string](riskNames) | |
| conditionalUpdate.Risks = risks |
🤖 Prompt for AI Agents
In `@pkg/cvo/status.go` around lines 463 - 466, The code only assigns
conditionalUpdate.RiskNames when riskNames.Len() > 0, leaving stale values when
risks are cleared; change this to always set conditionalUpdate.RiskNames from
riskNames (i.e., remove the Len() check and unconditionally assign
conditionalUpdate.RiskNames = sets.List[string](riskNames)) so that RiskNames
accurately reflects the current conditionalUpdate.Risks even when empty.
test/cvo/accept_risks.go
Outdated
| bk := cv.Spec.DesiredUpdate | ||
| if bk != nil { | ||
| bk = bk.DeepCopy() | ||
| } | ||
|
|
||
| g.By("Checking that new risks can be saved") | ||
| if cv.Spec.DesiredUpdate == nil { | ||
| cv.Spec.DesiredUpdate = &configv1.Update{} | ||
| } | ||
| cv.Spec.DesiredUpdate.AcceptRisks = append(cv.Spec.DesiredUpdate.AcceptRisks, []configv1.AcceptRisk{ | ||
| {Name: "riskA"}, | ||
| {Name: "riskB"}, | ||
| }...) | ||
|
|
||
| cv, err = configv1Client.ClusterVersions().Update(context.Background(), cv, metav1.UpdateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| var names []string | ||
| for _, r := range cv.Spec.DesiredUpdate.AcceptRisks { | ||
| names = append(names, r.Name) | ||
| } | ||
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeTrue()) | ||
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeTrue()) | ||
|
|
||
| g.By("Checking that new risks can be removed") | ||
| cv.Spec.DesiredUpdate = bk | ||
| cv, err = configv1Client.ClusterVersions().Update(context.Background(), cv, metav1.UpdateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeTrue()) | ||
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeTrue()) |
There was a problem hiding this comment.
Fix nil deref and inverted “remove risks” assertions.
If the original desiredUpdate is nil, Line 85 restores it to nil and Line 88 dereferences it, which can panic. Also, the “removed” step still asserts that risks are present. Consider restoring safely and asserting absence (or matching the backup).
🛠️ Suggested fix
g.By("Checking that new risks can be removed")
- cv.Spec.DesiredUpdate = bk
+ if bk == nil {
+ cv.Spec.DesiredUpdate = nil
+ } else {
+ cv.Spec.DesiredUpdate = bk
+ }
cv, err = configv1Client.ClusterVersions().Update(context.Background(), cv, metav1.UpdateOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
- o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeTrue())
- o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeTrue())
+ if cv.Spec.DesiredUpdate != nil {
+ o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeFalse())
+ o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeFalse())
+ }📝 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.
| bk := cv.Spec.DesiredUpdate | |
| if bk != nil { | |
| bk = bk.DeepCopy() | |
| } | |
| g.By("Checking that new risks can be saved") | |
| if cv.Spec.DesiredUpdate == nil { | |
| cv.Spec.DesiredUpdate = &configv1.Update{} | |
| } | |
| cv.Spec.DesiredUpdate.AcceptRisks = append(cv.Spec.DesiredUpdate.AcceptRisks, []configv1.AcceptRisk{ | |
| {Name: "riskA"}, | |
| {Name: "riskB"}, | |
| }...) | |
| cv, err = configv1Client.ClusterVersions().Update(context.Background(), cv, metav1.UpdateOptions{}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| var names []string | |
| for _, r := range cv.Spec.DesiredUpdate.AcceptRisks { | |
| names = append(names, r.Name) | |
| } | |
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeTrue()) | |
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeTrue()) | |
| g.By("Checking that new risks can be removed") | |
| cv.Spec.DesiredUpdate = bk | |
| cv, err = configv1Client.ClusterVersions().Update(context.Background(), cv, metav1.UpdateOptions{}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeTrue()) | |
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeTrue()) | |
| g.By("Checking that new risks can be removed") | |
| if bk == nil { | |
| cv.Spec.DesiredUpdate = nil | |
| } else { | |
| cv.Spec.DesiredUpdate = bk | |
| } | |
| cv, err = configv1Client.ClusterVersions().Update(context.Background(), cv, metav1.UpdateOptions{}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| if cv.Spec.DesiredUpdate != nil { | |
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskA"})).To(o.BeFalse()) | |
| o.Expect(slices.Contains(cv.Spec.DesiredUpdate.AcceptRisks, configv1.AcceptRisk{Name: "riskB"})).To(o.BeFalse()) | |
| } |
🤖 Prompt for AI Agents
In `@test/cvo/accept_risks.go` around lines 61 - 89, Backup/restoration logic and
assertions around cv.Spec.DesiredUpdate.AcceptRisks are unsafe and inverted:
preserve the original bk (already DeepCopied), and when restoring set
cv.Spec.DesiredUpdate = bk; after the update, avoid dereferencing when
cv.Spec.DesiredUpdate is nil by checking for nil first and then assert absence
of the previously-added risks (use BeFalse for slices.Contains for riskA/riskB)
or, if bk was non-nil, assert that the restored AcceptRisks equals the original
bk.AcceptRisks (or contains the same names); reference bk, cv.Spec.DesiredUpdate
and AcceptRisks when making the nil-safe checks and flipping the assertions in
the "Checking that new risks can be removed" section.
dfb63fd to
95ab40e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/util/util.go`:
- Around line 31-39: The function IsTechPreviewNoUpgrade can dereference a nil
featureGate if c.FeatureGates().Get returns a non-NotFound error because
o.Expect(err).NotTo(...) may not stop execution; after the
o.Expect(err).NotTo(o.HaveOccurred(), ...) call in the error branch, add an
explicit return (e.g., return false) to avoid continuing and dereferencing
featureGate, ensuring the function exits safely when an unexpected error occurs.
| func IsTechPreviewNoUpgrade(c *configclientv1.ConfigV1Client) bool { | ||
| featureGate, err := c.FeatureGates().Get(context.Background(), "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| if kapierrs.IsNotFound(err) { | ||
| return false | ||
| } | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "could not retrieve feature-gate: %v", err) | ||
| } | ||
| return featureGate.Spec.FeatureSet == configv1.TechPreviewNoUpgrade |
There was a problem hiding this comment.
Avoid potential nil dereference after a non-NotFound error
o.Expect(err) may not abort execution in all test setups, so featureGate can still be nil and the dereference on Line 39 can panic. Add a return after the expectation to keep this safe.
🔧 Suggested fix
func IsTechPreviewNoUpgrade(c *configclientv1.ConfigV1Client) bool {
featureGate, err := c.FeatureGates().Get(context.Background(), "cluster", metav1.GetOptions{})
if err != nil {
if kapierrs.IsNotFound(err) {
return false
}
o.Expect(err).NotTo(o.HaveOccurred(), "could not retrieve feature-gate: %v", err)
+ return false
}
return featureGate.Spec.FeatureSet == configv1.TechPreviewNoUpgrade
}📝 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.
| func IsTechPreviewNoUpgrade(c *configclientv1.ConfigV1Client) bool { | |
| featureGate, err := c.FeatureGates().Get(context.Background(), "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| if kapierrs.IsNotFound(err) { | |
| return false | |
| } | |
| o.Expect(err).NotTo(o.HaveOccurred(), "could not retrieve feature-gate: %v", err) | |
| } | |
| return featureGate.Spec.FeatureSet == configv1.TechPreviewNoUpgrade | |
| func IsTechPreviewNoUpgrade(c *configclientv1.ConfigV1Client) bool { | |
| featureGate, err := c.FeatureGates().Get(context.Background(), "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| if kapierrs.IsNotFound(err) { | |
| return false | |
| } | |
| o.Expect(err).NotTo(o.HaveOccurred(), "could not retrieve feature-gate: %v", err) | |
| return false | |
| } | |
| return featureGate.Spec.FeatureSet == configv1.TechPreviewNoUpgrade | |
| } |
🤖 Prompt for AI Agents
In `@test/util/util.go` around lines 31 - 39, The function IsTechPreviewNoUpgrade
can dereference a nil featureGate if c.FeatureGates().Get returns a non-NotFound
error because o.Expect(err).NotTo(...) may not stop execution; after the
o.Expect(err).NotTo(o.HaveOccurred(), ...) call in the error branch, add an
explicit return (e.g., return false) to avoid continuing and dereferencing
featureGate, ensuring the function exits safely when an unexpected error occurs.
95ab40e to
85e0753
Compare
|
/retest requied |
|
/retest-required |
|
@hongkailiu: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@hongkailiu: 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. |
3 participants
From this pull, CVO starts to reconcile
cv.spec.desiredUpdate.acceptRiskson 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:
CVO adds risk names into status.history.acceptedRisks if a conditional
update is accepted.