diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 81e4d8bc04..a4d26d30ea 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -12,16 +12,21 @@ 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" 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 +55,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 +140,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 +181,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 +208,9 @@ type availableUpdates struct { ConditionRegistry clusterconditions.ConditionRegistry Condition configv1.ClusterOperatorStatusCondition + + // RiskConditions stores the condition for every risk (name, url, message, matchingRules). + RiskConditions map[string][]metav1.Condition } func (u *availableUpdates) RecentlyAttempted(interval time.Duration) bool { @@ -291,14 +310,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 { @@ -426,8 +448,11 @@ 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) + condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions) if condition.Status == metav1.ConditionTrue { u.addUpdate(conditionalUpdate.Release) @@ -441,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 { @@ -478,13 +522,18 @@ func newRecommendedStatus(now, want metav1.ConditionStatus) metav1.ConditionStat } const ( - recommendedReasonRisksNotExposed = "NotExposedToRisks" - 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. recommendedReasonExposed = "ExposedToRisks" + + riskConditionReasonEvaluationFailed = "EvaluationFailed" + riskConditionReasonMatch = "Match" + riskConditionReasonNotMatch = "NotMatch" ) // Reasons follow same pattern as k8s Condition Reasons @@ -492,19 +541,28 @@ 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 == recommendedReasonAllExposedRisksAccepted || + now == want: return want - case want: + case want == recommendedReasonRisksNotExposed: 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 +571,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, 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 { + 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..38a620cadf 100644 --- a/pkg/cvo/availableupdates_test.go +++ b/pkg/cvo/availableupdates_test.go @@ -19,12 +19,15 @@ 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" 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 +210,7 @@ var cvFixture = &configv1.ClusterVersion{ } var availableUpdatesCmpOpts = []cmp.Option{ + cmpopts.IgnoreFields(availableUpdates{}, "ShouldReconcileAcceptRisks"), cmpopts.IgnoreTypes(time.Time{}), cmpopts.IgnoreInterfaces(struct { clusterconditions.ConditionRegistry @@ -230,7 +234,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 +325,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 +352,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 +369,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 +391,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 +413,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: "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"}}}, }, { name: "matching risk with name that cannot be used as a condition reason", @@ -421,6 +459,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 +470,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 +485,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 +493,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 +526,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 +551,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 +596,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 +620,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 +635,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 +651,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 +666,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 +683,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 +698,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 +724,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 +742,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 +757,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 +772,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 +786,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 +809,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 +820,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) } @@ -758,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) + } + + }) + } +} 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..ea97e3cce4 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] 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] which were all explicitly accepted by the cluster administrator.", risksMsg, strings.Join(riskNamesForDesiredImage, ",")) + } + } } mergeOperatorHistory(cvStatus, desired, status.Verified, now, status.Completed > 0, risksMsg, status.loadPayloadStatus.Local) @@ -402,6 +422,75 @@ 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) + } + } + sort.Slice(result, func(i, j int) bool { + return result[i].Name < result[j].Name + }) + 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..bae07757db 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: "Risk2", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionFalse, + }, + }}, {Name: "Risk3", + Conditions: []metav1.Condition{{ + Type: "Applies", + Status: metav1.ConditionTrue, + }, + }}}, + }, + } + 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