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

Author: hongkailiu

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,8 @@ 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.ConditionRegistry = optr.conditionRegistry
 		optrAvailableUpdates.Condition = condition
 
@@ -167,9 +177,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 +204,8 @@ type availableUpdates struct {
 	ConditionRegistry  clusterconditions.ConditionRegistry
 
 	Condition configv1.ClusterOperatorStatusCondition
+
+	RiskConditions map[string][]metav1.Condition
 }
 
 func (u *availableUpdates) RecentlyAttempted(interval time.Duration) bool {
@@ -291,14 +305,16 @@ 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,
+		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 +443,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)
@@ -485,6 +501,8 @@ const (
 	// 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"
 )
 
 // Reasons follow same pattern as k8s Condition Reasons
@@ -502,29 +520,57 @@ func newRecommendedReason(now, want string) string {
 	}
 }
 
-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,
 		Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
 	}
 
 	var errorMessages []string
-	for _, risk := range risks {
+	for i, risk := range risks {
+		riskCondition := metav1.Condition{
+			Type:               internal.ConditionalUpdateRiskConditionTypeApplies,
+			Status:             metav1.ConditionFalse,
+			LastTransitionTime: metav1.Now(),
+		}
 		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.Message = fmt.Sprintf("The matchingRules[%d] matches", i)
+			if shouldReconcileAcceptRisks() && acceptRisks.Has(risk.Name) {
+				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 riskConditions == nil {
+			riskConditions = map[string][]metav1.Condition{}
+		}
+		if _, ok := riskConditions[risk.Name]; !ok {
+			riskConditions[risk.Name] = []metav1.Condition{riskCondition}
 		}
 	}
 	if len(errorMessages) > 0 {

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
@@ -231,6 +234,7 @@ func TestSyncAvailableUpdates(t *testing.T) {
 		Status: configv1.ConditionTrue,
 	}
 
+	optr.enabledFeatureGates = featuregates.DefaultCvoGates("version")
 	err := optr.syncAvailableUpdates(context.Background(), cvFixture)
 
 	if err != nil {
@@ -319,6 +323,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 {
@@ -507,7 +512,9 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			registry := clusterconditions.NewConditionRegistry()
 			registry.Register("PromQL", tc.mockPromql)
-			actual := evaluateConditionalUpdate(context.Background(), tc.risks, registry)
+			actual := evaluateConditionalUpdate(context.Background(), tc.risks, registry, sets.New[string](), func() bool {
+				return false
+			}, map[string][]metav1.Condition{})
 			if diff := cmp.Diff(tc.expected, actual); diff != "" {
 				t.Errorf("actual condition differs from expected:\n%s", diff)
 			}
@@ -744,6 +751,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)
 			}

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
+}

pkg/cvo/metrics.go

@@ -441,7 +441,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
 			}
 

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",

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"
@@ -40,7 +41,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 +181,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 +192,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 +229,22 @@ 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
+		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,52 @@ 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
+	riskConditions := getAvailableUpdates().RiskConditions
+	for _, conditionalUpdate := range conditionalUpdates {
+		if desiredImage == conditionalUpdate.Release.Image {
+			riskNamesForDesiredImage = conditionalUpdate.RiskNames
+		}
+		riskNames := sets.New[string]()
+		for _, risk := range conditionalUpdate.Risks {
+			riskNames.Insert(risk.Name)
+			riskTypesToRemove := sets.New[string]()
+			conditions := riskConditions[risk.Name]
+			for _, condition := range risk.Conditions {
+				if found := meta.FindStatusCondition(conditions, condition.Type); found == nil {
+					riskTypesToRemove.Insert(condition.Type)
+				}
+			}
+			for riskTypeToRemove := range riskTypesToRemove {
+				meta.RemoveStatusCondition(&risk.Conditions, riskTypeToRemove)
+			}
+			for _, condition := range conditions {
+				meta.SetStatusCondition(&risk.Conditions, condition)
+			}
+		}
+		conditionalUpdate.RiskNames = sets.List[string](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) {