Skip to content

Commit f2d4ca5

Browse files
hongkailiuhongkailiu
committed
OTA-1546: Reconcile cv.spec.desiredUpdate.acceptRisks
From this pull, CVO starts to reconcile `cv.spec.desiredUpdate.acceptRisks` on a cluser if FeatureGateClusterUpdateAcceptRisks is enabled no a non-hypershift cluster. - CVO does not block a conditional update if all risks that apply to the cluster are accpeted. - CVO fills up the new fields in status: - status.conditionalUpdateRisks, - status.conditionalUpdate.riskNames, and - status.conditionalUpdate.risks.conditions - CVO adds risk names into status.history.acceptedRisks if a conditional update is accepted.
1 parent e444b8e commit f2d4ca5

File tree

11 files changed

+524
-58
lines changed

11 files changed

+524
-58
lines changed

‎pkg/cvo/availableupdates.go‎

Lines changed: 81 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,14 @@ import (
1616
"k8s.io/apimachinery/pkg/api/equality"
1717
"k8s.io/apimachinery/pkg/api/meta"
1818
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
19+
"k8s.io/apimachinery/pkg/util/sets"
1920
"k8s.io/klog/v2"
2021

2122
configv1 "github.com/openshift/api/config/v1"
2223
"github.com/openshift/cluster-version-operator/lib/resourcemerge"
2324
"github.com/openshift/cluster-version-operator/pkg/cincinnati"
2425
"github.com/openshift/cluster-version-operator/pkg/clusterconditions"
26+
"github.com/openshift/cluster-version-operator/pkg/internal"
2527
)
2628

2729
const noArchitecture string = "NoArchitecture"
@@ -50,6 +52,12 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
5052
channel := config.Spec.Channel
5153
desiredArch := optr.getDesiredArchitecture(config.Spec.DesiredUpdate)
5254
currentArch := optr.getCurrentArchitecture()
55+
acceptRisks := sets.New[string]()
56+
if config.Spec.DesiredUpdate != nil {
57+
for _, risk := range config.Spec.DesiredUpdate.AcceptRisks {
58+
acceptRisks.Insert(risk.Name)
59+
}
60+
}
5361

5462
// updates are only checked at most once per minimumUpdateCheckInterval or if the generation changes
5563
optrAvailableUpdates := optr.getAvailableUpdates()
@@ -129,6 +137,9 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
129137
optrAvailableUpdates.UpdateService = updateService
130138
optrAvailableUpdates.Channel = channel
131139
optrAvailableUpdates.Architecture = desiredArch
140+
optrAvailableUpdates.ShouldReconcileAcceptRisks = optr.shouldReconcileAcceptRisks
141+
optrAvailableUpdates.AcceptRisks = acceptRisks
142+
optrAvailableUpdates.RiskConditions = map[string][]metav1.Condition{}
132143
optrAvailableUpdates.ConditionRegistry = optr.conditionRegistry
133144
optrAvailableUpdates.Condition = condition
134145

@@ -167,9 +178,11 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
167178
}
168179

169180
type availableUpdates struct {
170-
UpdateService string
171-
Channel string
172-
Architecture string
181+
UpdateService string
182+
Channel string
183+
Architecture string
184+
ShouldReconcileAcceptRisks func() bool
185+
AcceptRisks sets.Set[string]
173186

174187
// LastAttempt records the time of the most recent attempt at update
175188
// retrieval, regardless of whether it was successful.
@@ -192,6 +205,11 @@ type availableUpdates struct {
192205
ConditionRegistry clusterconditions.ConditionRegistry
193206

194207
Condition configv1.ClusterOperatorStatusCondition
208+
209+
// RiskConditions stores the condition for every risk (name, url, message, matchingRules).
210+
// The key of the risk is represented by its name which is ensured by validating our graph-data.
211+
// https://github.com/openshift/cincinnati-graph-data/blob/af701850c24b4a53426c2a5400c63895fdf9de60/hack/validate-blocked-edges.py#L25C77-L25C90
212+
RiskConditions map[string][]metav1.Condition
195213
}
196214

197215
func (u *availableUpdates) RecentlyAttempted(interval time.Duration) bool {
@@ -291,14 +309,17 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates {
291309
}
292310

293311
u := &availableUpdates{
294-
UpdateService: optr.availableUpdates.UpdateService,
295-
Channel: optr.availableUpdates.Channel,
296-
Architecture: optr.availableUpdates.Architecture,
297-
LastAttempt: optr.availableUpdates.LastAttempt,
298-
LastSyncOrConfigChange: optr.availableUpdates.LastSyncOrConfigChange,
299-
Current: *optr.availableUpdates.Current.DeepCopy(),
300-
ConditionRegistry: optr.availableUpdates.ConditionRegistry, // intentionally not a copy, to preserve cache state
301-
Condition: optr.availableUpdates.Condition,
312+
UpdateService: optr.availableUpdates.UpdateService,
313+
Channel: optr.availableUpdates.Channel,
314+
Architecture: optr.availableUpdates.Architecture,
315+
ShouldReconcileAcceptRisks: optr.shouldReconcileAcceptRisks,
316+
AcceptRisks: optr.availableUpdates.AcceptRisks,
317+
RiskConditions: optr.availableUpdates.RiskConditions,
318+
LastAttempt: optr.availableUpdates.LastAttempt,
319+
LastSyncOrConfigChange: optr.availableUpdates.LastSyncOrConfigChange,
320+
Current: *optr.availableUpdates.Current.DeepCopy(),
321+
ConditionRegistry: optr.availableUpdates.ConditionRegistry, // intentionally not a copy, to preserve cache state
322+
Condition: optr.availableUpdates.Condition,
302323
}
303324

304325
if optr.availableUpdates.Updates != nil {
@@ -427,7 +448,7 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
427448
return vi.GTE(vj)
428449
})
429450
for i, conditionalUpdate := range u.ConditionalUpdates {
430-
condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry)
451+
condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions)
431452

432453
if condition.Status == metav1.ConditionTrue {
433454
u.addUpdate(conditionalUpdate.Release)
@@ -478,33 +499,45 @@ func newRecommendedStatus(now, want metav1.ConditionStatus) metav1.ConditionStat
478499
}
479500

480501
const (
481-
recommendedReasonRisksNotExposed = "NotExposedToRisks"
482-
recommendedReasonEvaluationFailed = "EvaluationFailed"
483-
recommendedReasonMultiple = "MultipleReasons"
502+
recommendedReasonRisksNotExposed = "NotExposedToRisks"
503+
recommendedReasonExposedOnlyToAcceptedRisks = "ExposedOnlyToAcceptedRisks"
504+
recommendedReasonEvaluationFailed = "EvaluationFailed"
505+
recommendedReasonMultiple = "MultipleReasons"
484506

485507
// recommendedReasonExposed is used instead of the original name if it does
486508
// not match the pattern for a valid k8s condition reason.
487509
recommendedReasonExposed = "ExposedToRisks"
510+
511+
riskConditionReasonEvaluationFailed = "EvaluationFailed"
512+
riskConditionReasonMatch = "Match"
513+
riskConditionReasonNotMatch = "NotMatch"
488514
)
489515

490516
// Reasons follow same pattern as k8s Condition Reasons
491517
// https://github.com/openshift/api/blob/59fa376de7cb668ddb95a7ee4e9879d7f6ca2767/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1535-L1536
492518
var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`)
493519

494520
func newRecommendedReason(now, want string) string {
495-
switch now {
496-
case recommendedReasonRisksNotExposed:
521+
switch {
522+
case now == recommendedReasonRisksNotExposed || now == recommendedReasonExposedOnlyToAcceptedRisks && now != want:
497523
return want
498-
case want:
524+
case now == recommendedReasonExposedOnlyToAcceptedRisks && want == recommendedReasonRisksNotExposed || now == want:
499525
return now
500526
default:
501527
return recommendedReasonMultiple
502528
}
503529
}
504530

505-
func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) metav1.Condition {
531+
func evaluateConditionalUpdate(
532+
ctx context.Context,
533+
risks []configv1.ConditionalUpdateRisk,
534+
conditionRegistry clusterconditions.ConditionRegistry,
535+
acceptRisks sets.Set[string],
536+
shouldReconcileAcceptRisks func() bool,
537+
riskConditions map[string][]metav1.Condition,
538+
) metav1.Condition {
506539
recommended := metav1.Condition{
507-
Type: ConditionalUpdateConditionTypeRecommended,
540+
Type: internal.ConditionalUpdateConditionTypeRecommended,
508541
Status: metav1.ConditionTrue,
509542
// FIXME: ObservedGeneration? That would capture upstream/channel, but not necessarily the currently-reconciling version.
510543
Reason: recommendedReasonRisksNotExposed,
@@ -513,18 +546,39 @@ func evaluateConditionalUpdate(ctx context.Context, risks []configv1.Conditional
513546

514547
var errorMessages []string
515548
for _, risk := range risks {
549+
riskCondition := metav1.Condition{
550+
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
551+
Status: metav1.ConditionFalse,
552+
Reason: riskConditionReasonNotMatch,
553+
}
516554
if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
555+
msg := unknownExposureMessage(risk, err)
517556
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown)
518557
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed)
519-
errorMessages = append(errorMessages, unknownExposureMessage(risk, err))
558+
errorMessages = append(errorMessages, msg)
559+
riskCondition.Status = metav1.ConditionUnknown
560+
riskCondition.Reason = riskConditionReasonEvaluationFailed
561+
riskCondition.Message = msg
520562
} else if match {
521-
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse)
522-
wantReason := recommendedReasonExposed
523-
if reasonPattern.MatchString(risk.Name) {
524-
wantReason = risk.Name
563+
riskCondition.Status = metav1.ConditionTrue
564+
riskCondition.Reason = riskConditionReasonMatch
565+
if shouldReconcileAcceptRisks() && acceptRisks.Has(risk.Name) {
566+
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionTrue)
567+
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonExposedOnlyToAcceptedRisks)
568+
recommended.Message = "The update is recommended, because either risk does not apply to this cluster or it is accepted by cluster admins."
569+
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)
570+
} else {
571+
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse)
572+
wantReason := recommendedReasonExposed
573+
if reasonPattern.MatchString(risk.Name) {
574+
wantReason = risk.Name
575+
}
576+
recommended.Reason = newRecommendedReason(recommended.Reason, wantReason)
577+
errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
525578
}
526-
recommended.Reason = newRecommendedReason(recommended.Reason, wantReason)
527-
errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
579+
}
580+
if _, ok := riskConditions[risk.Name]; !ok {
581+
riskConditions[risk.Name] = []metav1.Condition{riskCondition}
528582
}
529583
}
530584
if len(errorMessages) > 0 {

0 commit comments

Comments
 (0)