From e9f3c198da0fdb944bd26e574759a4fdc342748d Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Fri, 2 Jan 2026 18:48:39 +0800 Subject: [PATCH 1/6] OTA-1546: Reconcile cv.spec.desiredUpdate.acceptRisks From this pull, CVO starts to reconcile `cv.spec.desiredUpdate.acceptRisks` on a cluster 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 accepted. - 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. --- pkg/cvo/availableupdates.go | 108 ++++++--- pkg/cvo/availableupdates_test.go | 94 +++++++- pkg/cvo/cvo.go | 9 + pkg/cvo/cvo_test.go | 13 +- pkg/cvo/metrics.go | 2 +- pkg/cvo/metrics_test.go | 14 +- pkg/cvo/status.go | 104 ++++++++- pkg/cvo/status_test.go | 214 +++++++++++++++++- pkg/featuregates/featuregates.go | 13 ++ pkg/internal/constants.go | 8 + .../clusterversion/recommendedupdate.go | 3 +- 11 files changed, 524 insertions(+), 58 deletions(-) diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 81e4d8bc04..c52ae6f44a 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -16,12 +16,14 @@ import ( "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-version-operator/lib/resourcemerge" "github.com/openshift/cluster-version-operator/pkg/cincinnati" "github.com/openshift/cluster-version-operator/pkg/clusterconditions" + "github.com/openshift/cluster-version-operator/pkg/internal" ) const noArchitecture string = "NoArchitecture" @@ -50,6 +52,12 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1 channel := config.Spec.Channel desiredArch := optr.getDesiredArchitecture(config.Spec.DesiredUpdate) currentArch := optr.getCurrentArchitecture() + acceptRisks := sets.New[string]() + if config.Spec.DesiredUpdate != nil { + for _, risk := range config.Spec.DesiredUpdate.AcceptRisks { + acceptRisks.Insert(risk.Name) + } + } // updates are only checked at most once per minimumUpdateCheckInterval or if the generation changes optrAvailableUpdates := optr.getAvailableUpdates() @@ -129,6 +137,9 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1 optrAvailableUpdates.UpdateService = updateService optrAvailableUpdates.Channel = channel optrAvailableUpdates.Architecture = desiredArch + optrAvailableUpdates.ShouldReconcileAcceptRisks = optr.shouldReconcileAcceptRisks + optrAvailableUpdates.AcceptRisks = acceptRisks + optrAvailableUpdates.RiskConditions = map[string][]metav1.Condition{} optrAvailableUpdates.ConditionRegistry = optr.conditionRegistry optrAvailableUpdates.Condition = condition @@ -167,9 +178,11 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1 } type availableUpdates struct { - UpdateService string - Channel string - Architecture string + UpdateService string + Channel string + Architecture string + ShouldReconcileAcceptRisks func() bool + AcceptRisks sets.Set[string] // LastAttempt records the time of the most recent attempt at update // retrieval, regardless of whether it was successful. @@ -192,6 +205,11 @@ type availableUpdates struct { ConditionRegistry clusterconditions.ConditionRegistry Condition configv1.ClusterOperatorStatusCondition + + // RiskConditions stores the condition for every risk (name, url, message, matchingRules). + // The key of the risk is represented by its name which is ensured by validating our graph-data. + // https://github.com/openshift/cincinnati-graph-data/blob/af701850c24b4a53426c2a5400c63895fdf9de60/hack/validate-blocked-edges.py#L25C77-L25C90 + RiskConditions map[string][]metav1.Condition } func (u *availableUpdates) RecentlyAttempted(interval time.Duration) bool { @@ -291,14 +309,17 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates { } u := &availableUpdates{ - UpdateService: optr.availableUpdates.UpdateService, - Channel: optr.availableUpdates.Channel, - Architecture: optr.availableUpdates.Architecture, - LastAttempt: optr.availableUpdates.LastAttempt, - LastSyncOrConfigChange: optr.availableUpdates.LastSyncOrConfigChange, - Current: *optr.availableUpdates.Current.DeepCopy(), - ConditionRegistry: optr.availableUpdates.ConditionRegistry, // intentionally not a copy, to preserve cache state - Condition: optr.availableUpdates.Condition, + UpdateService: optr.availableUpdates.UpdateService, + Channel: optr.availableUpdates.Channel, + Architecture: optr.availableUpdates.Architecture, + ShouldReconcileAcceptRisks: optr.shouldReconcileAcceptRisks, + AcceptRisks: optr.availableUpdates.AcceptRisks, + RiskConditions: optr.availableUpdates.RiskConditions, + LastAttempt: optr.availableUpdates.LastAttempt, + LastSyncOrConfigChange: optr.availableUpdates.LastSyncOrConfigChange, + Current: *optr.availableUpdates.Current.DeepCopy(), + ConditionRegistry: optr.availableUpdates.ConditionRegistry, // intentionally not a copy, to preserve cache state + Condition: optr.availableUpdates.Condition, } if optr.availableUpdates.Updates != nil { @@ -427,7 +448,7 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) { return vi.GTE(vj) }) for i, conditionalUpdate := range u.ConditionalUpdates { - condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry) + condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions) if condition.Status == metav1.ConditionTrue { u.addUpdate(conditionalUpdate.Release) @@ -478,13 +499,18 @@ func newRecommendedStatus(now, want metav1.ConditionStatus) metav1.ConditionStat } const ( - recommendedReasonRisksNotExposed = "NotExposedToRisks" - recommendedReasonEvaluationFailed = "EvaluationFailed" - recommendedReasonMultiple = "MultipleReasons" + recommendedReasonRisksNotExposed = "NotExposedToRisks" + recommendedReasonExposedOnlyToAcceptedRisks = "ExposedOnlyToAcceptedRisks" + recommendedReasonEvaluationFailed = "EvaluationFailed" + recommendedReasonMultiple = "MultipleReasons" // recommendedReasonExposed is used instead of the original name if it does // not match the pattern for a valid k8s condition reason. recommendedReasonExposed = "ExposedToRisks" + + riskConditionReasonEvaluationFailed = "EvaluationFailed" + riskConditionReasonMatch = "Match" + riskConditionReasonNotMatch = "NotMatch" ) // Reasons follow same pattern as k8s Condition Reasons @@ -492,19 +518,26 @@ const ( var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`) func newRecommendedReason(now, want string) string { - switch now { - case recommendedReasonRisksNotExposed: + switch { + case now == recommendedReasonRisksNotExposed || now == recommendedReasonExposedOnlyToAcceptedRisks && now != want: return want - case want: + case now == recommendedReasonExposedOnlyToAcceptedRisks && want == recommendedReasonRisksNotExposed || now == want: return now default: return recommendedReasonMultiple } } -func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) metav1.Condition { +func evaluateConditionalUpdate( + ctx context.Context, + risks []configv1.ConditionalUpdateRisk, + conditionRegistry clusterconditions.ConditionRegistry, + acceptRisks sets.Set[string], + shouldReconcileAcceptRisks func() bool, + riskConditions map[string][]metav1.Condition, +) metav1.Condition { recommended := metav1.Condition{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionTrue, // FIXME: ObservedGeneration? That would capture upstream/channel, but not necessarily the currently-reconciling version. Reason: recommendedReasonRisksNotExposed, @@ -513,18 +546,39 @@ func evaluateConditionalUpdate(ctx context.Context, risks []configv1.Conditional var errorMessages []string for _, risk := range risks { + riskCondition := metav1.Condition{ + Type: internal.ConditionalUpdateRiskConditionTypeApplies, + Status: metav1.ConditionFalse, + Reason: riskConditionReasonNotMatch, + } if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil { + msg := unknownExposureMessage(risk, err) recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown) recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed) - errorMessages = append(errorMessages, unknownExposureMessage(risk, err)) + errorMessages = append(errorMessages, msg) + riskCondition.Status = metav1.ConditionUnknown + riskCondition.Reason = riskConditionReasonEvaluationFailed + riskCondition.Message = msg } else if match { - recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse) - wantReason := recommendedReasonExposed - if reasonPattern.MatchString(risk.Name) { - wantReason = risk.Name + riskCondition.Status = metav1.ConditionTrue + riskCondition.Reason = riskConditionReasonMatch + if shouldReconcileAcceptRisks() && acceptRisks.Has(risk.Name) { + recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionTrue) + recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonExposedOnlyToAcceptedRisks) + recommended.Message = "The update is recommended, because either risk does not apply to this cluster or it is accepted by cluster admins." + klog.V(2).Infof("Risk with name %q is accepted by the cluster admin and thus not in the evaluation of conditional update", risk.Name) + } else { + recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse) + wantReason := recommendedReasonExposed + if reasonPattern.MatchString(risk.Name) { + wantReason = risk.Name + } + recommended.Reason = newRecommendedReason(recommended.Reason, wantReason) + errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL)) } - recommended.Reason = newRecommendedReason(recommended.Reason, wantReason) - errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL)) + } + if _, ok := riskConditions[risk.Name]; !ok { + riskConditions[risk.Name] = []metav1.Condition{riskCondition} } } if len(errorMessages) > 0 { diff --git a/pkg/cvo/availableupdates_test.go b/pkg/cvo/availableupdates_test.go index 919ef21dda..0e41367d96 100644 --- a/pkg/cvo/availableupdates_test.go +++ b/pkg/cvo/availableupdates_test.go @@ -19,12 +19,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/workqueue" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-version-operator/pkg/clusterconditions" "github.com/openshift/cluster-version-operator/pkg/clusterconditions/always" "github.com/openshift/cluster-version-operator/pkg/clusterconditions/mock" + "github.com/openshift/cluster-version-operator/pkg/featuregates" ) // notFoundProxyLister is a stub for ProxyLister @@ -207,6 +209,7 @@ var cvFixture = &configv1.ClusterVersion{ } var availableUpdatesCmpOpts = []cmp.Option{ + cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks"), cmpopts.IgnoreTypes(time.Time{}), cmpopts.IgnoreInterfaces(struct { clusterconditions.ConditionRegistry @@ -230,7 +233,9 @@ func TestSyncAvailableUpdates(t *testing.T) { Type: configv1.RetrievedUpdates, Status: configv1.ConditionTrue, } + expectedAvailableUpdates.RiskConditions = map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}} + optr.enabledFeatureGates = featuregates.DefaultCvoGates("version") err := optr.syncAvailableUpdates(context.Background(), cvFixture) if err != nil { @@ -319,6 +324,7 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing. tc.modifyOriginalState(optr) tc.modifyCV(cv, fixture.expectedConditionalUpdates[0]) + optr.enabledFeatureGates = featuregates.DefaultCvoGates("version") err := optr.syncAvailableUpdates(context.Background(), cv) if err != nil { @@ -345,10 +351,14 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing. func TestEvaluateConditionalUpdate(t *testing.T) { testcases := []struct { - name string - risks []configv1.ConditionalUpdateRisk - mockPromql clusterconditions.Condition - expected metav1.Condition + name string + risks []configv1.ConditionalUpdateRisk + mockPromql clusterconditions.Condition + acceptRisks sets.Set[string] + shouldReconcileAcceptRisks func() bool + riskConditions map[string][]metav1.Condition + expected metav1.Condition + expectedRiskConditions map[string][]metav1.Condition }{ { name: "no risks", @@ -358,6 +368,7 @@ func TestEvaluateConditionalUpdate(t *testing.T) { Reason: recommendedReasonRisksNotExposed, Message: "The update is recommended, because none of the conditional update risks apply to this cluster.", }, + expectedRiskConditions: map[string][]metav1.Condition{}, }, { name: "one risk that does not match", @@ -379,6 +390,7 @@ func TestEvaluateConditionalUpdate(t *testing.T) { Reason: recommendedReasonRisksNotExposed, Message: "The update is recommended, because none of the conditional update risks apply to this cluster.", }, + expectedRiskConditions: map[string][]metav1.Condition{"ShouldNotApply": {{Type: "Applies", Status: metav1.ConditionFalse, Reason: "NotMatch"}}}, }, { name: "one risk that matches", @@ -400,6 +412,31 @@ func TestEvaluateConditionalUpdate(t *testing.T) { Reason: "RiskThatApplies", Message: "This is a risk! https://match.es", }, + expectedRiskConditions: map[string][]metav1.Condition{"RiskThatApplies": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}}, + }, + { + name: "one risk that matches and is accepted", + risks: []configv1.ConditionalUpdateRisk{ + { + URL: "https://match.es", + Name: "RiskThatApplies", + Message: "This is a risk!", + MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}}, + }, + }, + mockPromql: &mock.Mock{ + ValidQueue: []error{nil}, + MatchQueue: []mock.MatchResult{{Match: true, Error: nil}}, + }, + acceptRisks: sets.New[string]("RiskThatApplies", "not-important"), + shouldReconcileAcceptRisks: func() bool { return true }, + expected: metav1.Condition{ + Type: "Recommended", + Status: metav1.ConditionTrue, + Reason: "ExposedOnlyToAcceptedRisks", + Message: "The update is recommended, because either risk does not apply to this cluster or it is accepted by cluster admins.", + }, + expectedRiskConditions: map[string][]metav1.Condition{"RiskThatApplies": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}}, }, { name: "matching risk with name that cannot be used as a condition reason", @@ -421,6 +458,7 @@ func TestEvaluateConditionalUpdate(t *testing.T) { Reason: recommendedReasonExposed, Message: "This is a risk! https://match.es", }, + expectedRiskConditions: map[string][]metav1.Condition{"RISK-THAT-APPLIES": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}}, }, { name: "two risks that match", @@ -431,6 +469,12 @@ func TestEvaluateConditionalUpdate(t *testing.T) { Message: "This is a risk!", MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}}, }, + { + URL: "https://doesnotmat.ch", + Name: "ShouldNotApply", + Message: "ShouldNotApply", + MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}}, + }, { URL: "https://match.es/too", Name: "RiskThatAppliesToo", @@ -440,7 +484,7 @@ func TestEvaluateConditionalUpdate(t *testing.T) { }, mockPromql: &mock.Mock{ ValidQueue: []error{nil, nil}, - MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: true, Error: nil}}, + MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: false, Error: nil}, {Match: true, Error: nil}}, }, expected: metav1.Condition{ Type: "Recommended", @@ -448,6 +492,9 @@ func TestEvaluateConditionalUpdate(t *testing.T) { Reason: recommendedReasonMultiple, Message: "This is a risk! https://match.es\n\nThis is a risk too! https://match.es/too", }, + expectedRiskConditions: map[string][]metav1.Condition{"RiskThatApplies": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}, + "ShouldNotApply": {{Type: "Applies", Status: metav1.ConditionFalse, Reason: "NotMatch"}}, + "RiskThatAppliesToo": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}}, }, { name: "first risk matches, second fails to evaluate", @@ -478,6 +525,8 @@ func TestEvaluateConditionalUpdate(t *testing.T) { " RiskThatFailsToEvaluate description: This is a risk too!\n" + " RiskThatFailsToEvaluate URL: https://whokno.ws", }, + expectedRiskConditions: map[string][]metav1.Condition{"RiskThatApplies": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}, + "RiskThatFailsToEvaluate": {{Type: "Applies", Status: metav1.ConditionUnknown, Reason: "EvaluationFailed", Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n RiskThatFailsToEvaluate description: This is a risk too!\n RiskThatFailsToEvaluate URL: https://whokno.ws"}}}, }, { name: "one risk that fails to evaluate", @@ -501,16 +550,29 @@ func TestEvaluateConditionalUpdate(t *testing.T) { " RiskThatFailsToEvaluate description: This is a risk!\n" + " RiskThatFailsToEvaluate URL: https://whokno.ws", }, + expectedRiskConditions: map[string][]metav1.Condition{"RiskThatFailsToEvaluate": {{Type: "Applies", Status: metav1.ConditionUnknown, Reason: "EvaluationFailed", Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n RiskThatFailsToEvaluate description: This is a risk!\n RiskThatFailsToEvaluate URL: https://whokno.ws"}}}, }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { registry := clusterconditions.NewConditionRegistry() registry.Register("PromQL", tc.mockPromql) - actual := evaluateConditionalUpdate(context.Background(), tc.risks, registry) + if tc.shouldReconcileAcceptRisks == nil { + tc.shouldReconcileAcceptRisks = func() bool { + return false + } + } + if tc.riskConditions == nil { + tc.riskConditions = map[string][]metav1.Condition{} + } + actual := evaluateConditionalUpdate(context.Background(), tc.risks, registry, tc.acceptRisks, tc.shouldReconcileAcceptRisks, tc.riskConditions) if diff := cmp.Diff(tc.expected, actual); diff != "" { t.Errorf("actual condition differs from expected:\n%s", diff) } + + if diff := cmp.Diff(tc.expectedRiskConditions, tc.riskConditions); diff != "" { + t.Errorf("actual risk conditions differs from expected:\n%s", diff) + } }) } } @@ -533,9 +595,10 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates []configv1.ConditionalUpdate } tests := []struct { - name string - args args - expected expected + name string + args args + expected expected + expectedRiskConditions map[string][]metav1.Condition }{ // -------------------------------- Valid set desiredUpdate field combinations -------------------------------- // Some combinations, such as all fields being set, are omitted due to them causing API validation errors; @@ -556,6 +619,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, { name: "operator is multi, image is specified, version is not specified, architecture is not specified", @@ -570,6 +634,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, { name: "operator is multi, image is not specified, version is specified, architecture is specified", @@ -585,6 +650,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, { name: "operator is multi, image is not specified, version is specified, architecture is not specified", @@ -599,6 +665,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, // ---------------- Cases where the operator is single arch { @@ -615,6 +682,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: runtime.GOARCH, }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, { name: "operator is not multi, image is specified, version is not specified, architecture is not specified", @@ -629,6 +697,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: runtime.GOARCH, }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, { name: "operator is not multi, image is not specified, version is specified, architecture is specified - migration to multi arch issued", @@ -654,6 +723,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { updates: []configv1.Release{{Version: data.from.version, Image: data.from.image}}, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{}, }, { name: "operator is not multi, image is not specified, version is specified, architecture is specified - migration && update", @@ -671,6 +741,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { updates: []configv1.Release{{Version: data.from.version, Image: data.from.image}}, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{}, }, { name: "operator is not multi, image is not specified, version is specified, architecture is not specified", @@ -685,6 +756,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: runtime.GOARCH, }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, // -------------------------------- Desired Update Is NOT set -------------------------------- // ---------------- The operator is multi arch @@ -699,6 +771,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: "multi", }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, // ---------------- The operator is single arch { @@ -712,6 +785,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { conditionalUpdates: expectedConditionalUpdates, queryParamArch: runtime.GOARCH, }, + expectedRiskConditions: map[string][]metav1.Condition{"FourFiveSix": {{Type: "Applies", Status: "True", Reason: "Match"}}}, }, } for _, tt := range tests { @@ -734,6 +808,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { Type: configv1.RetrievedUpdates, Status: configv1.ConditionTrue, } + expectedAvailableUpdates.RiskConditions = tt.expectedRiskConditions expectedQueryParams := url.Values{ "arch": {tt.expected.queryParamArch}, @@ -744,6 +819,7 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { cv := cvFixture.DeepCopy() cv.Spec.DesiredUpdate = tt.args.desiredUpdate + optr.enabledFeatureGates = featuregates.DefaultCvoGates("version") if err := optr.syncAvailableUpdates(context.Background(), cv); err != nil { t.Fatalf("syncAvailableUpdates() unexpected error: %v", err) } diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index cc6874c735..2def402752 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -1091,3 +1091,12 @@ func (optr *Operator) shouldReconcileCVOConfiguration() bool { // The relevant CRD and CR are not applied in HyperShift, which configures the CVO via a configuration file return optr.enabledFeatureGates.CVOConfiguration() && !optr.hypershift } + +// shouldReconcileAcceptRisks returns whether the CVO should reconcile spec.desiredUpdate.acceptRisks and populate the +// relevant fields in status. +// +// enabledFeatureGates must be initialized before the function is called. +func (optr *Operator) shouldReconcileAcceptRisks() bool { + // HyperShift will be supported later if needed + return optr.enabledFeatureGates.AcceptRisks() && !optr.hypershift +} diff --git a/pkg/cvo/cvo_test.go b/pkg/cvo/cvo_test.go index 9ef4c75bd9..33b568a8bc 100644 --- a/pkg/cvo/cvo_test.go +++ b/pkg/cvo/cvo_test.go @@ -16,6 +16,7 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -2302,7 +2303,6 @@ func TestOperator_sync(t *testing.T) { func TestOperator_availableUpdatesSync(t *testing.T) { id := uuid.Must(uuid.NewRandom()).String() - tests := []struct { name string key string @@ -2363,6 +2363,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Reason: "VersionNotFound", Message: `Unable to retrieve available updates: currently reconciling cluster version 4.0.1 not found in the "fast" channel`, }, + RiskConditions: map[string][]metav1.Condition{}, }, }, { @@ -2406,6 +2407,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Reason: noChannel, Message: "The update channel has not been configured.", }, + RiskConditions: map[string][]metav1.Condition{}, }, }, { @@ -2448,6 +2450,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Reason: "NoCurrentVersion", Message: "The cluster version does not have a semantic version assigned and cannot calculate valid upgrades.", }, + RiskConditions: map[string][]metav1.Condition{}, }, }, { @@ -2497,6 +2500,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Reason: "ResponseFailed", Message: "Unable to retrieve available updates: unexpected HTTP status: 500 Internal Server Error", }, + RiskConditions: map[string][]metav1.Condition{}, }, }, { @@ -2565,6 +2569,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Type: configv1.RetrievedUpdates, Status: configv1.ConditionTrue, }, + RiskConditions: map[string][]metav1.Condition{}, }, }, { @@ -2631,6 +2636,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Type: configv1.RetrievedUpdates, Status: configv1.ConditionTrue, }, + RiskConditions: map[string][]metav1.Condition{}, }, }, { @@ -2729,6 +2735,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) { Reason: "ResponseFailed", Message: "Unable to retrieve available updates: unexpected HTTP status: 500 Internal Server Error", }, + RiskConditions: map[string][]metav1.Condition{}, }, }, } @@ -2781,8 +2788,8 @@ func TestOperator_availableUpdatesSync(t *testing.T) { } } - if !reflect.DeepEqual(optr.availableUpdates, tt.wantUpdates) { - t.Fatalf("unexpected: %s", cmp.Diff(tt.wantUpdates, optr.availableUpdates)) + if diff := cmp.Diff(tt.wantUpdates, optr.availableUpdates, cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks")); diff != "" { + t.Fatalf("unexpected: %s", diff) } if (optr.queue.Len() > 0) != (optr.availableUpdates != nil) { t.Fatalf("unexpected queue") diff --git a/pkg/cvo/metrics.go b/pkg/cvo/metrics.go index 2220aac4de..a6fc10dc5d 100644 --- a/pkg/cvo/metrics.go +++ b/pkg/cvo/metrics.go @@ -445,7 +445,7 @@ func (m *operatorMetrics) Describe(ch chan<- *prometheus.Desc) { func (m *operatorMetrics) collectConditionalUpdates(ch chan<- prometheus.Metric, updates []configv1.ConditionalUpdate) { for _, update := range updates { for _, condition := range update.Conditions { - if condition.Type != ConditionalUpdateConditionTypeRecommended { + if condition.Type != internal.ConditionalUpdateConditionTypeRecommended { continue } diff --git a/pkg/cvo/metrics_test.go b/pkg/cvo/metrics_test.go index 6392ed5a46..18950dc904 100644 --- a/pkg/cvo/metrics_test.go +++ b/pkg/cvo/metrics_test.go @@ -47,7 +47,7 @@ func Test_operatorMetrics_Collect(t *testing.T) { Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, Conditions: []metav1.Condition{ { - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionTrue, LastTransitionTime: metav1.NewTime(time.Unix(3, 0)), Reason: "RiskDoesNotApply", @@ -838,7 +838,7 @@ func TestCollectUnknownConditionalUpdates(t *testing.T) { Release: configv1.Release{Version: "4.13.1", Image: "pullspec"}, Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk"}}, Conditions: []metav1.Condition{{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionFalse, LastTransitionTime: metav1.NewTime(anchorTime), Reason: "RiskApplies", @@ -858,7 +858,7 @@ func TestCollectUnknownConditionalUpdates(t *testing.T) { Release: configv1.Release{Version: "4.13.1", Image: "pullspec"}, Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk"}}, Conditions: []metav1.Condition{{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionTrue, LastTransitionTime: metav1.NewTime(anchorTime), Reason: "RiskDoesNotApply", @@ -878,7 +878,7 @@ func TestCollectUnknownConditionalUpdates(t *testing.T) { Release: configv1.Release{Version: "4.13.1", Image: "pullspec"}, Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk"}}, Conditions: []metav1.Condition{{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionUnknown, LastTransitionTime: metav1.NewTime(anchorTime), Reason: "EvaluationFailed", @@ -898,7 +898,7 @@ func TestCollectUnknownConditionalUpdates(t *testing.T) { Release: configv1.Release{Version: "4.13.1", Image: "pullspec"}, Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk"}}, Conditions: []metav1.Condition{{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionFalse, LastTransitionTime: metav1.NewTime(anchorTime), Reason: "RiskApplies", @@ -909,7 +909,7 @@ func TestCollectUnknownConditionalUpdates(t *testing.T) { Release: configv1.Release{Version: "4.13.2", Image: "pullspec"}, Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk"}}, Conditions: []metav1.Condition{{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionTrue, LastTransitionTime: metav1.NewTime(anchorTime.Add(time.Minute)), Reason: "RiskDoesNotApply", @@ -920,7 +920,7 @@ func TestCollectUnknownConditionalUpdates(t *testing.T) { Release: configv1.Release{Version: "4.13.3", Image: "pullspec"}, Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk"}}, Conditions: []metav1.Condition{{ - Type: ConditionalUpdateConditionTypeRecommended, + Type: internal.ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionUnknown, LastTransitionTime: metav1.NewTime(anchorTime.Add(time.Minute * 2)), Reason: "EvaluationFailed", diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 8194d683a2..5af661e738 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -17,6 +17,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" configv1 "github.com/openshift/api/config/v1" @@ -29,10 +30,6 @@ import ( ) const ( - // ConditionalUpdateConditionTypeRecommended is a type of the condition present on a conditional update - // that indicates whether the conditional update is recommended or not - ConditionalUpdateConditionTypeRecommended = "Recommended" - // MaxHistory is the maximum size of ClusterVersion history. Once exceeded // ClusterVersion history will be pruned. It is declared here and passed // into the pruner function to allow easier testing. @@ -40,7 +37,7 @@ const ( ) func findRecommendedCondition(conditions []metav1.Condition) *metav1.Condition { - return meta.FindStatusCondition(conditions, ConditionalUpdateConditionTypeRecommended) + return meta.FindStatusCondition(conditions, internal.ConditionalUpdateConditionTypeRecommended) } func mergeEqualVersions(current *configv1.UpdateHistory, desired configv1.Release) bool { @@ -180,7 +177,7 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1 original = config.DeepCopy() } - updateClusterVersionStatus(&config.Status, status, optr.release, optr.getAvailableUpdates, optr.enabledFeatureGates, validationErrs) + updateClusterVersionStatus(&config.Status, status, optr.release, optr.getAvailableUpdates, optr.enabledFeatureGates, validationErrs, optr.shouldReconcileAcceptRisks) if klog.V(6).Enabled() { klog.Infof("Apply config: %s", cmp.Diff(original, config)) @@ -191,9 +188,15 @@ func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1 } // updateClusterVersionStatus updates the passed cvStatus with the latest status information -func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status *SyncWorkerStatus, - release configv1.Release, getAvailableUpdates func() *availableUpdates, enabledGates featuregates.CvoGateChecker, - validationErrs field.ErrorList) { +func updateClusterVersionStatus( + cvStatus *configv1.ClusterVersionStatus, + status *SyncWorkerStatus, + release configv1.Release, + getAvailableUpdates func() *availableUpdates, + enabledGates featuregates.CvoGateChecker, + validationErrs field.ErrorList, + shouldReconcileAcceptRisks func() bool, +) { cvStatus.ObservedGeneration = status.Generation if len(status.VersionHash) > 0 { @@ -222,9 +225,26 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status desired.Architecture = configv1.ClusterVersionArchitecture("") } + var riskNamesForDesiredImage []string + if shouldReconcileAcceptRisks() { + cvStatus.ConditionalUpdates, riskNamesForDesiredImage = conditionalUpdateWithRiskNamesAndRiskConditions(cvStatus.ConditionalUpdates, getAvailableUpdates, desired.Image) + cvStatus.ConditionalUpdateRisks = conditionalUpdateRisks(cvStatus.ConditionalUpdates) + } + risksMsg := "" if desired.Image == status.loadPayloadStatus.Update.Image { risksMsg = status.loadPayloadStatus.AcceptedRisks + // Include the accepted risks from clusterversion.spec.desiredUpdate.acceptRisks that apply to the cluster + // Ideally this should be formed earlier like others from a condition's message. + // However, it is not easy to follow the tradition because the existing pattern is for the error cases. + // Here Recommended=True because admins accept risks already leads to no error. + if shouldReconcileAcceptRisks() && len(riskNamesForDesiredImage) > 0 { + if risksMsg == "" { + risksMsg = fmt.Sprintf("The target release %s is exposed to the risks [%s] and accepted by CVO because either the risk is considered acceptable by the cluster admin or it does not apply to the cluster.", desired.Image, strings.Join(riskNamesForDesiredImage, ",")) + } else { + risksMsg = fmt.Sprintf("%s; It is exposed to the risks [%s] and accepted by CVO because either the risk is considered acceptable by the cluster admin or it does not apply to the cluster.", risksMsg, strings.Join(riskNamesForDesiredImage, ",")) + } + } } mergeOperatorHistory(cvStatus, desired, status.Verified, now, status.Completed > 0, risksMsg, status.loadPayloadStatus.Local) @@ -402,6 +422,72 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status } } +func conditionalUpdateWithRiskNamesAndRiskConditions(conditionalUpdates []configv1.ConditionalUpdate, getAvailableUpdates func() *availableUpdates, desiredImage string) ([]configv1.ConditionalUpdate, []string) { + var result []configv1.ConditionalUpdate + var riskNamesForDesiredImage []string + var riskConditions map[string][]metav1.Condition + updates := getAvailableUpdates() + if updates != nil { + riskConditions = updates.RiskConditions + } + for _, conditionalUpdate := range conditionalUpdates { + riskNames := sets.New[string]() + var risks []configv1.ConditionalUpdateRisk + for _, risk := range conditionalUpdate.Risks { + riskNames.Insert(risk.Name) + riskTypesToRemove := sets.New[string]() + conditions, ok := riskConditions[risk.Name] + if !ok { + // This should never happen + conditions = []metav1.Condition{ + { + Type: internal.ConditionalUpdateRiskConditionTypeApplies, + Status: metav1.ConditionUnknown, + Reason: "InternalErrorNoConditionCollected", + }, + } + } + 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) + } + for _, condition := range conditions { + meta.SetStatusCondition(&risk.Conditions, condition) + } + risks = append(risks, risk) + } + if riskNames.Len() > 0 { + conditionalUpdate.RiskNames = sets.List[string](riskNames) + } + conditionalUpdate.Risks = risks + + if desiredImage == conditionalUpdate.Release.Image { + riskNamesForDesiredImage = conditionalUpdate.RiskNames + } + result = append(result, conditionalUpdate) + } + return result, riskNamesForDesiredImage +} + +func conditionalUpdateRisks(conditionalUpdates []configv1.ConditionalUpdate) []configv1.ConditionalUpdateRisk { + var result []configv1.ConditionalUpdateRisk + riskNames := sets.New[string]() + for _, conditionalUpdate := range conditionalUpdates { + for _, risk := range conditionalUpdate.Risks { + if riskNames.Has(risk.Name) { + continue + } + riskNames.Insert(risk.Name) + result = append(result, risk) + } + } + return result +} + // getReasonMessageFromError returns the reason and the message from an error. // If the reason or message is not available, an empty string is returned. func getReasonMessageFromError(err error) (reason, message string) { diff --git a/pkg/cvo/status_test.go b/pkg/cvo/status_test.go index 1e1711c3c6..91d13add11 100644 --- a/pkg/cvo/status_test.go +++ b/pkg/cvo/status_test.go @@ -203,6 +203,7 @@ type fakeRiFlags struct { unknownVersion bool statusReleaseArchitecture bool cvoConfiguration bool + acceptRisks bool } func (f fakeRiFlags) UnknownVersion() bool { @@ -217,6 +218,10 @@ func (f fakeRiFlags) CVOConfiguration() bool { return f.cvoConfiguration } +func (f fakeRiFlags) AcceptRisks() bool { + return f.acceptRisks +} + func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t *testing.T) { ignoreLastTransitionTime := cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime") type args struct { @@ -741,7 +746,9 @@ func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t if tc.shouldModifyWhenNotReconcilingAndHistoryNotEmpty && !c.isReconciling && !c.isHistoryEmpty { expectedCondition = tc.expectedConditionModified } - updateClusterVersionStatus(cvStatus, tc.args.syncWorkerStatus, release, getAvailableUpdates, gates, noErrors) + updateClusterVersionStatus(cvStatus, tc.args.syncWorkerStatus, release, getAvailableUpdates, gates, noErrors, func() bool { + return false + }) condition := resourcemerge.FindOperatorStatusCondition(cvStatus.Conditions, internal.ClusterStatusFailing) if diff := cmp.Diff(expectedCondition, condition, ignoreLastTransitionTime); diff != "" { t.Errorf("unexpected condition when Reconciling == %t && isHistoryEmpty == %t\n:%s", c.isReconciling, c.isHistoryEmpty, diff) @@ -935,3 +942,208 @@ func Test_filterOutUpdateErrors(t *testing.T) { }) } } + +var ( + cu1 = configv1.ConditionalUpdate{ + Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, + RiskNames: []string{"wrongName"}, + Risks: []configv1.ConditionalUpdateRisk{ + { + Name: "Risk1", + Conditions: []metav1.Condition{ + {Type: "wrongType", Status: metav1.ConditionUnknown}, + }, + }, + }, + } + + cu2 = configv1.ConditionalUpdate{ + Release: configv1.Release{Version: "4.5.7", Image: "pullspec/4.5.7"}, + Risks: []configv1.ConditionalUpdateRisk{ + { + Name: "Risk1", + }, + { + Name: "Risk2", + }, + }, + } +) + +func Test_conditionalUpdateWithRiskNamesAndRiskConditions(t *testing.T) { + tests := []struct { + name string + conditionalUpdates []configv1.ConditionalUpdate + desiredImage string + availableUpdates *availableUpdates + expected []configv1.ConditionalUpdate + expectedNames []string + }{ + { + name: "nil available updates", + conditionalUpdates: []configv1.ConditionalUpdate{*cu1.DeepCopy()}, + expected: []configv1.ConditionalUpdate{{ + RiskNames: []string{"Risk1"}, + Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, + Risks: []configv1.ConditionalUpdateRisk{ + { + Name: "Risk1", Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionUnknown, + Reason: "InternalErrorNoConditionCollected", + }, + }}, + }, + }}, + }, + { + name: "no risk conditions", + availableUpdates: &availableUpdates{}, + conditionalUpdates: []configv1.ConditionalUpdate{*cu1.DeepCopy()}, + expected: []configv1.ConditionalUpdate{{ + Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, + RiskNames: []string{"Risk1"}, + Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionUnknown, + Reason: "InternalErrorNoConditionCollected", + }, + }}}, + }}, + }, + { + name: "no risk conditions but desired image matches", + desiredImage: "pullspec/4.5.6", + availableUpdates: &availableUpdates{}, + conditionalUpdates: []configv1.ConditionalUpdate{*cu1.DeepCopy()}, + expected: []configv1.ConditionalUpdate{{ + Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, + RiskNames: []string{"Risk1"}, + Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionUnknown, + Reason: "InternalErrorNoConditionCollected", + }, + }}}, + }}, + expectedNames: []string{"Risk1"}, + }, + { + name: "basic case", + desiredImage: "pullspec/4.5.7", + availableUpdates: &availableUpdates{ + RiskConditions: map[string][]metav1.Condition{ + "Risk1": {{Type: "Applies", Status: metav1.ConditionTrue}}, + "Risk2": {{Type: "Applies", Status: metav1.ConditionFalse}}, + }, + }, + conditionalUpdates: []configv1.ConditionalUpdate{*cu1.DeepCopy(), *cu2.DeepCopy()}, + expected: []configv1.ConditionalUpdate{{ + Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, + RiskNames: []string{"Risk1"}, + Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}}, + }, { + Release: configv1.Release{Version: "4.5.7", Image: "pullspec/4.5.7"}, + RiskNames: []string{"Risk1", "Risk2"}, + Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}, {Name: "Risk2", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionFalse, + }, + }}}, + }}, + expectedNames: []string{"Risk1", "Risk2"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + getAvailableUpdates := func() *availableUpdates { + return tt.availableUpdates + } + actual, actualNames := conditionalUpdateWithRiskNamesAndRiskConditions(tt.conditionalUpdates, getAvailableUpdates, tt.desiredImage) + if difference := cmp.Diff(tt.expected, actual, cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime")); difference != "" { + t.Errorf("conditional updates differ from expected:\n%s", difference) + } + if difference := cmp.Diff(tt.expectedNames, actualNames); difference != "" { + t.Errorf("risk names differ from expected:\n%s", difference) + } + }) + } +} + +func Test_conditionalUpdateRisks(t *testing.T) { + tests := []struct { + name string + conditionalUpdates []configv1.ConditionalUpdate + expected []configv1.ConditionalUpdateRisk + }{ + { + name: "basic case", + conditionalUpdates: []configv1.ConditionalUpdate{{ + Release: configv1.Release{Version: "4.5.6", Image: "pullspec/4.5.6"}, + RiskNames: []string{"Risk1", "Risk3"}, + Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}, {Name: "Risk3", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}}, + }, { + Release: configv1.Release{Version: "4.5.7", Image: "pullspec/4.5.7"}, + RiskNames: []string{"Risk1", "Risk2"}, + Risks: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}, {Name: "Risk2", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionFalse, + }, + }}}, + }}, + expected: []configv1.ConditionalUpdateRisk{{Name: "Risk1", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}, {Name: "Risk3", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}, {Name: "Risk2", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionFalse, + }, + }}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual := conditionalUpdateRisks(tt.conditionalUpdates) + if difference := cmp.Diff(tt.expected, actual, cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime")); difference != "" { + t.Errorf("actual differ from expected:\n%s", difference) + } + }) + } +} diff --git a/pkg/featuregates/featuregates.go b/pkg/featuregates/featuregates.go index 27fd627c3f..a66e19ca2b 100644 --- a/pkg/featuregates/featuregates.go +++ b/pkg/featuregates/featuregates.go @@ -35,6 +35,9 @@ type CvoGateChecker interface { // CVOConfiguration controls whether the CVO reconciles the ClusterVersionOperator resource that corresponds // to its configuration. CVOConfiguration() bool + + // AcceptRisks controls whether the CVO reconciles spec.desiredUpdate.acceptRisks. + AcceptRisks() bool } // CvoGates contains flags that control CVO functionality gated by product feature gates. The @@ -49,6 +52,7 @@ type CvoGates struct { unknownVersion bool statusReleaseArchitecture bool cvoConfiguration bool + acceptRisks bool } func (c CvoGates) StatusReleaseArchitecture() bool { @@ -63,6 +67,10 @@ func (c CvoGates) CVOConfiguration() bool { return c.cvoConfiguration } +func (c CvoGates) AcceptRisks() bool { + return c.acceptRisks +} + // DefaultCvoGates apply when actual features for given version are unknown func DefaultCvoGates(version string) CvoGates { return CvoGates{ @@ -70,6 +78,7 @@ func DefaultCvoGates(version string) CvoGates { unknownVersion: true, statusReleaseArchitecture: false, cvoConfiguration: false, + acceptRisks: false, } } @@ -91,6 +100,8 @@ func CvoGatesFromFeatureGate(gate *configv1.FeatureGate, version string) CvoGate enabledGates.statusReleaseArchitecture = true case features.FeatureGateCVOConfiguration: enabledGates.cvoConfiguration = true + case features.FeatureGateClusterUpdateAcceptRisks: + enabledGates.acceptRisks = true } } for _, disabled := range g.Disabled { @@ -99,6 +110,8 @@ func CvoGatesFromFeatureGate(gate *configv1.FeatureGate, version string) CvoGate enabledGates.statusReleaseArchitecture = false case features.FeatureGateCVOConfiguration: enabledGates.cvoConfiguration = false + case features.FeatureGateClusterUpdateAcceptRisks: + enabledGates.acceptRisks = false } } } diff --git a/pkg/internal/constants.go b/pkg/internal/constants.go index ec53376c0e..bce432ebc9 100644 --- a/pkg/internal/constants.go +++ b/pkg/internal/constants.go @@ -70,4 +70,12 @@ const ( // UpgradeableUpgradeInProgress is True if an update is in progress. UpgradeableUpgradeInProgress configv1.ClusterStatusConditionType = "UpgradeableUpgradeInProgress" + + // ConditionalUpdateConditionTypeRecommended is a type of the condition present on a conditional update + // that indicates whether the conditional update is recommended or not + ConditionalUpdateConditionTypeRecommended = "Recommended" + + // ConditionalUpdateRiskConditionTypeApplies is a type of the condition present on a conditional update risk + // that indicates whether the conditional update risk applies to the cluster + ConditionalUpdateRiskConditionTypeApplies = "Applies" ) diff --git a/pkg/payload/precondition/clusterversion/recommendedupdate.go b/pkg/payload/precondition/clusterversion/recommendedupdate.go index c4d7729c6a..acfd671133 100644 --- a/pkg/payload/precondition/clusterversion/recommendedupdate.go +++ b/pkg/payload/precondition/clusterversion/recommendedupdate.go @@ -11,6 +11,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/openshift/cluster-version-operator/lib/resourcemerge" + "github.com/openshift/cluster-version-operator/pkg/internal" precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition" ) @@ -51,7 +52,7 @@ func (ru *RecommendedUpdate) Run(ctx context.Context, releaseContext preconditio for _, conditionalUpdate := range clusterVersion.Status.ConditionalUpdates { if conditionalUpdate.Release.Version == releaseContext.DesiredVersion { for _, condition := range conditionalUpdate.Conditions { - if condition.Type == "Recommended" { + if condition.Type == internal.ConditionalUpdateConditionTypeRecommended { switch condition.Status { case metav1.ConditionTrue: return nil From aa851c2477fb7bc6fc7a6dcd42ea045c9a35e935 Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Thu, 29 Jan 2026 20:56:50 +0800 Subject: [PATCH 2/6] Add sanity check on ConditionalUpdates We expect no failures out of the risks coming to CVO. If any happens, there are logs to help us (CVO developers) and cluster admins to figure out what goes wrong. --- pkg/cvo/availableupdates.go | 27 ++++++++++++++-- pkg/cvo/availableupdates_test.go | 53 ++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index c52ae6f44a..540d7f41d7 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -12,10 +12,13 @@ import ( "time" "github.com/blang/semver/v4" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" @@ -207,8 +210,6 @@ type availableUpdates struct { Condition configv1.ClusterOperatorStatusCondition // RiskConditions stores the condition for every risk (name, url, message, matchingRules). - // The key of the risk is represented by its name which is ensured by validating our graph-data. - // https://github.com/openshift/cincinnati-graph-data/blob/af701850c24b4a53426c2a5400c63895fdf9de60/hack/validate-blocked-edges.py#L25C77-L25C90 RiskConditions map[string][]metav1.Condition } @@ -447,6 +448,9 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) { vj := semver.MustParse(u.ConditionalUpdates[j].Release.Version) return vi.GTE(vj) }) + if err := sanityCheck(u.ConditionalUpdates); err != nil { + klog.Errorf("Sanity check failed which might impact risk evaluation: %v", err) + } for i, conditionalUpdate := range u.ConditionalUpdates { condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions) @@ -462,6 +466,25 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) { } } +func sanityCheck(updates []configv1.ConditionalUpdate) error { + risks := map[string]configv1.ConditionalUpdateRisk{} + var errs []error + for _, update := range updates { + for _, risk := range update.Risks { + if v, ok := risks[risk.Name]; ok { + if diff := cmp.Diff(v, risk, cmpopts.IgnoreFields(configv1.ConditionalUpdateRisk{}, "Conditions")); diff != "" { + errs = append(errs, fmt.Errorf("found collision on risk %s: %v and %v", risk.Name, v, risk)) + } + } else if trimmed := strings.TrimSpace(risk.Name); trimmed == "" { + errs = append(errs, fmt.Errorf("found invalid name on risk %v", risk)) + } else { + risks[risk.Name] = risk + } + } + } + return utilerrors.NewAggregate(errs) +} + func (u *availableUpdates) addUpdate(release configv1.Release) { for _, update := range u.Updates { if update.Image == release.Image { diff --git a/pkg/cvo/availableupdates_test.go b/pkg/cvo/availableupdates_test.go index 0e41367d96..68b844ac8e 100644 --- a/pkg/cvo/availableupdates_test.go +++ b/pkg/cvo/availableupdates_test.go @@ -19,6 +19,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/workqueue" @@ -834,3 +835,55 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) { }) } } + +func Test_sanityCheck(t *testing.T) { + tests := []struct { + name string + updates []configv1.ConditionalUpdate + expected error + }{ + { + name: "good", + updates: []configv1.ConditionalUpdate{ + {Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}}}, + {Risks: []configv1.ConditionalUpdateRisk{{Name: "riskB"}}}, + }, + }, + { + name: "invalid risk name", + updates: []configv1.ConditionalUpdate{ + {Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}, {Name: "", URL: "some"}}}, + }, + expected: utilerrors.NewAggregate([]error{fmt.Errorf("found invalid name on risk {[] some []}")}), + }, + { + name: "bad in one update", + updates: []configv1.ConditionalUpdate{ + {Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}, {Name: "riskA", URL: "some"}}}, + }, + expected: utilerrors.NewAggregate([]error{fmt.Errorf("found collision on risk riskA: {[] riskA []} and {[] some riskA []}")}), + }, + { + name: "bad in two updates", + updates: []configv1.ConditionalUpdate{ + {Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}}}, + {Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA", URL: "some"}}}, + }, + expected: utilerrors.NewAggregate([]error{fmt.Errorf("found collision on risk riskA: {[] riskA []} and {[] some riskA []}")}), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual := sanityCheck(tt.updates) + if diff := cmp.Diff(tt.expected, actual, cmp.Comparer(func(x, y error) bool { + if x == nil || y == nil { + return x == nil && y == nil + } + return x.Error() == y.Error() + })); diff != "" { + t.Errorf("sanityCheck() mismatch (-want +got):\n%s", diff) + } + + }) + } +} From 1061efdaba9df5c7cefbd0f0c470c3aad6ac4bba Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Thu, 29 Jan 2026 21:15:02 +0800 Subject: [PATCH 3/6] Rename recommendedReasonExposedOnlyToAcceptedRisks to recommendedReasonAllExposedRisksAccepted --- pkg/cvo/availableupdates.go | 14 +++++++------- pkg/cvo/availableupdates_test.go | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 540d7f41d7..01aaf71a73 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -522,10 +522,10 @@ func newRecommendedStatus(now, want metav1.ConditionStatus) metav1.ConditionStat } const ( - recommendedReasonRisksNotExposed = "NotExposedToRisks" - recommendedReasonExposedOnlyToAcceptedRisks = "ExposedOnlyToAcceptedRisks" - recommendedReasonEvaluationFailed = "EvaluationFailed" - recommendedReasonMultiple = "MultipleReasons" + recommendedReasonRisksNotExposed = "NotExposedToRisks" + recommendedReasonAllExposedRisksAccepted = "AllExposedRisksAccepted" + recommendedReasonEvaluationFailed = "EvaluationFailed" + recommendedReasonMultiple = "MultipleReasons" // recommendedReasonExposed is used instead of the original name if it does // not match the pattern for a valid k8s condition reason. @@ -542,9 +542,9 @@ var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ func newRecommendedReason(now, want string) string { switch { - case now == recommendedReasonRisksNotExposed || now == recommendedReasonExposedOnlyToAcceptedRisks && now != want: + case now == recommendedReasonRisksNotExposed || now == recommendedReasonAllExposedRisksAccepted && now != want: return want - case now == recommendedReasonExposedOnlyToAcceptedRisks && want == recommendedReasonRisksNotExposed || now == want: + case now == recommendedReasonAllExposedRisksAccepted && want == recommendedReasonRisksNotExposed || now == want: return now default: return recommendedReasonMultiple @@ -587,7 +587,7 @@ func evaluateConditionalUpdate( riskCondition.Reason = riskConditionReasonMatch if shouldReconcileAcceptRisks() && acceptRisks.Has(risk.Name) { recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionTrue) - recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonExposedOnlyToAcceptedRisks) + recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonAllExposedRisksAccepted) recommended.Message = "The update is recommended, because either risk does not apply to this cluster or it is accepted by cluster admins." klog.V(2).Infof("Risk with name %q is accepted by the cluster admin and thus not in the evaluation of conditional update", risk.Name) } else { diff --git a/pkg/cvo/availableupdates_test.go b/pkg/cvo/availableupdates_test.go index 68b844ac8e..38a620cadf 100644 --- a/pkg/cvo/availableupdates_test.go +++ b/pkg/cvo/availableupdates_test.go @@ -434,7 +434,7 @@ func TestEvaluateConditionalUpdate(t *testing.T) { expected: metav1.Condition{ Type: "Recommended", Status: metav1.ConditionTrue, - Reason: "ExposedOnlyToAcceptedRisks", + Reason: "AllExposedRisksAccepted", Message: "The update is recommended, because either risk does not apply to this cluster or it is accepted by cluster admins.", }, expectedRiskConditions: map[string][]metav1.Condition{"RiskThatApplies": {{Type: "Applies", Status: metav1.ConditionTrue, Reason: "Match"}}}, From 9cec473bb0db3d7800a3b48a0a6815c686fca054 Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Fri, 30 Jan 2026 12:32:42 +0800 Subject: [PATCH 4/6] Simplify newRecommendedReason --- pkg/cvo/availableupdates.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 01aaf71a73..a4d26d30ea 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -542,9 +542,11 @@ var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ func newRecommendedReason(now, want string) string { switch { - case now == recommendedReasonRisksNotExposed || now == recommendedReasonAllExposedRisksAccepted && now != want: + case now == recommendedReasonRisksNotExposed || + now == recommendedReasonAllExposedRisksAccepted || + now == want: return want - case now == recommendedReasonAllExposedRisksAccepted && want == recommendedReasonRisksNotExposed || now == want: + case want == recommendedReasonRisksNotExposed: return now default: return recommendedReasonMultiple From 7a10659fd00eac3a7638b520694ceed6dcdf032e Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Fri, 30 Jan 2026 13:14:35 +0800 Subject: [PATCH 5/6] Improve the risk message --- pkg/cvo/status.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 5af661e738..6991cf8376 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -240,9 +240,9 @@ func updateClusterVersionStatus( // Here Recommended=True because admins accept risks already leads to no error. if shouldReconcileAcceptRisks() && len(riskNamesForDesiredImage) > 0 { if risksMsg == "" { - risksMsg = fmt.Sprintf("The target release %s is exposed to the risks [%s] and accepted by CVO because either the risk is considered acceptable by the cluster admin or it does not apply to the cluster.", desired.Image, strings.Join(riskNamesForDesiredImage, ",")) + risksMsg = fmt.Sprintf("The target release %s is exposed to the risks [%s] which were all explicitly accepted by the cluster administrator.", desired.Image, strings.Join(riskNamesForDesiredImage, ",")) } else { - risksMsg = fmt.Sprintf("%s; It is exposed to the risks [%s] and accepted by CVO because either the risk is considered acceptable by the cluster admin or it does not apply to the cluster.", risksMsg, strings.Join(riskNamesForDesiredImage, ",")) + risksMsg = fmt.Sprintf("%s; It is exposed to the risks [%s] which were all explicitly accepted by the cluster administrator.", risksMsg, strings.Join(riskNamesForDesiredImage, ",")) } } } From 14dffd77184940a1da14112198ce00106b3e4ba7 Mon Sep 17 00:00:00 2001 From: Hongkai Liu Date: Fri, 30 Jan 2026 14:15:36 +0800 Subject: [PATCH 6/6] Sort ConditionalUpdateRisks by name --- pkg/cvo/status.go | 3 +++ pkg/cvo/status_test.go | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 6991cf8376..ea97e3cce4 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -485,6 +485,9 @@ func conditionalUpdateRisks(conditionalUpdates []configv1.ConditionalUpdate) []c result = append(result, risk) } } + sort.Slice(result, func(i, j int) bool { + return result[i].Name < result[j].Name + }) return result } diff --git a/pkg/cvo/status_test.go b/pkg/cvo/status_test.go index 91d13add11..bae07757db 100644 --- a/pkg/cvo/status_test.go +++ b/pkg/cvo/status_test.go @@ -1125,15 +1125,15 @@ func Test_conditionalUpdateRisks(t *testing.T) { Type: "Applies", Status: metav1.ConditionTrue, }, - }}, {Name: "Risk3", + }}, {Name: "Risk2", Conditions: []metav1.Condition{{ Type: "Applies", - Status: metav1.ConditionTrue, + Status: metav1.ConditionFalse, }, - }}, {Name: "Risk2", + }}, {Name: "Risk3", Conditions: []metav1.Condition{{ Type: "Applies", - Status: metav1.ConditionFalse, + Status: metav1.ConditionTrue, }, }}}, },