Skip to content

Commit 0fdf01d

Browse files
committed
Generate riskConditions in a better way
Follow up [1]. The issues are * It goes down to the per-target level when managing the riskConditions cache which can be simplified by collecting risks while going through the targets. * The cache may not get refreshed after re-evaluation of the risk. * We self-throttle to avoid overloading Prometheus. It is a better strategy if we evaluate risks for latest versions because a cluster admin is more likely to upgrade to them. With the pull, `riskConditions` is generated by * `loadRiskVersions` stores the risks and the latest version to which it is exposed. * `risksInOrder` gets a list of risk names sorted by its latest version. * `loadRiskConditions` evaluates the risks according the order above. Since all risk have been evaluated, the Recommended condition can be determined easily. [1]. https://github.com/openshift/cluster-version-operator/pull/1284/changes#r2734918038
1 parent 54dc4ef commit 0fdf01d

6 files changed

Lines changed: 267 additions & 141 deletions

File tree

pkg/cvo/availableupdates.go

Lines changed: 91 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,6 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
144144
optrAvailableUpdates.Architecture = desiredArch
145145
optrAvailableUpdates.ShouldReconcileAcceptRisks = optr.shouldReconcileAcceptRisks
146146
optrAvailableUpdates.AcceptRisks = acceptRisks
147-
optrAvailableUpdates.RiskConditions = map[string][]metav1.Condition{}
148147
optrAvailableUpdates.ConditionRegistry = optr.conditionRegistry
149148
optrAvailableUpdates.Condition = condition
150149

@@ -342,6 +341,80 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates {
342341
return u
343342
}
344343

344+
func loadRiskConditions(ctx context.Context, risks []string, riskVersions map[string]riskWithVersion, conditionRegistry clusterconditions.ConditionRegistry) map[string][]metav1.Condition {
345+
riskConditions := map[string][]metav1.Condition{}
346+
for _, riskName := range risks {
347+
risk := riskVersions[riskName].risk
348+
riskCondition := metav1.Condition{
349+
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
350+
Status: metav1.ConditionFalse,
351+
Reason: riskConditionReasonNotMatch,
352+
}
353+
if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
354+
msg := unknownExposureMessage(risk, err)
355+
riskCondition.Status = metav1.ConditionUnknown
356+
riskCondition.Reason = riskConditionReasonEvaluationFailed
357+
riskCondition.Message = msg
358+
} else if match {
359+
riskCondition.Status = metav1.ConditionTrue
360+
riskCondition.Reason = riskConditionReasonMatch
361+
}
362+
riskConditions[risk.Name] = []metav1.Condition{riskCondition}
363+
}
364+
if len(riskConditions) == 0 {
365+
return nil
366+
}
367+
return riskConditions
368+
}
369+
370+
// risksInOrder returns the list of risk names sorted by the associated version
371+
func risksInOrder(riskVersions map[string]riskWithVersion) []string {
372+
var ret []string
373+
var temp []riskWithVersion
374+
var keys []string
375+
for k := range riskVersions {
376+
keys = append(keys, k)
377+
}
378+
sort.Strings(keys)
379+
for _, k := range keys {
380+
temp = append(temp, riskVersions[k])
381+
}
382+
sort.SliceStable(temp, func(i, j int) bool {
383+
return temp[i].version.GT(temp[j].version)
384+
})
385+
for _, v := range temp {
386+
ret = append(ret, v.risk.Name)
387+
}
388+
return ret
389+
}
390+
391+
type riskWithVersion struct {
392+
version semver.Version
393+
risk configv1.ConditionalUpdateRisk
394+
}
395+
396+
// loadRiskVersions returns the map from risk names to riskWithVersion where
397+
// riskWithVersion.version is the latest version that the risk is exposed to
398+
// and riskWithVersion.risk is the risk itself
399+
func loadRiskVersions(conditionalUpdates []configv1.ConditionalUpdate) map[string]riskWithVersion {
400+
riskVersions := map[string]riskWithVersion{}
401+
for _, conditionalUpdate := range conditionalUpdates {
402+
for _, risk := range conditionalUpdate.Risks {
403+
candidate := semver.MustParse(conditionalUpdate.Release.Version)
404+
if v, ok := riskVersions[risk.Name]; !ok || candidate.GT(v.version) {
405+
riskVersions[risk.Name] = riskWithVersion{
406+
version: candidate,
407+
risk: risk,
408+
}
409+
}
410+
}
411+
}
412+
if len(riskVersions) == 0 {
413+
return nil
414+
}
415+
return riskVersions
416+
}
417+
345418
func (optr *Operator) getDesiredArchitecture(update *configv1.Update) string {
346419
if update != nil && len(update.Architecture) > 0 {
347420
return string(update.Architecture)
@@ -445,16 +518,15 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
445518
return
446519
}
447520

448-
sort.Slice(u.ConditionalUpdates, func(i, j int) bool {
449-
vi := semver.MustParse(u.ConditionalUpdates[i].Release.Version)
450-
vj := semver.MustParse(u.ConditionalUpdates[j].Release.Version)
451-
return vi.GTE(vj)
452-
})
521+
riskVersions := loadRiskVersions(u.ConditionalUpdates)
522+
risks := risksInOrder(riskVersions)
523+
u.RiskConditions = loadRiskConditions(ctx, risks, riskVersions, u.ConditionRegistry)
524+
453525
if err := sanityCheck(u.ConditionalUpdates); err != nil {
454526
klog.Errorf("Sanity check failed which might impact risk evaluation: %v", err)
455527
}
456528
for i, conditionalUpdate := range u.ConditionalUpdates {
457-
condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions)
529+
condition := evaluateConditionalUpdate(conditionalUpdate.Risks, u.AcceptRisks, u.ShouldReconcileAcceptRisks, u.RiskConditions)
458530

459531
if condition.Status == metav1.ConditionTrue {
460532
u.addUpdate(conditionalUpdate.Release)
@@ -556,9 +628,7 @@ func newRecommendedReason(now, want string) string {
556628
}
557629

558630
func evaluateConditionalUpdate(
559-
ctx context.Context,
560631
risks []configv1.ConditionalUpdateRisk,
561-
conditionRegistry clusterconditions.ConditionRegistry,
562632
acceptRisks sets.Set[string],
563633
shouldReconcileAcceptRisks func() bool,
564634
riskConditions map[string][]metav1.Condition,
@@ -573,22 +643,21 @@ func evaluateConditionalUpdate(
573643

574644
var errorMessages []string
575645
for _, risk := range risks {
576-
riskCondition := metav1.Condition{
577-
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
578-
Status: metav1.ConditionFalse,
579-
Reason: riskConditionReasonNotMatch,
646+
riskCondition := meta.FindStatusCondition(riskConditions[risk.Name], internal.ConditionalUpdateRiskConditionTypeApplies)
647+
if riskCondition == nil {
648+
// This should never happen
649+
riskCondition = &metav1.Condition{
650+
Status: metav1.ConditionUnknown,
651+
Reason: internal.ConditionalUpdateRiskConditionReasonInternalErrorFoundNoRiskCondition,
652+
Message: fmt.Sprintf("failed to find risk condition for risk %s", risk.Name),
653+
}
580654
}
581-
if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
582-
msg := unknownExposureMessage(risk, err)
655+
switch riskCondition.Status {
656+
case metav1.ConditionUnknown:
583657
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown)
584658
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed)
585-
errorMessages = append(errorMessages, msg)
586-
riskCondition.Status = metav1.ConditionUnknown
587-
riskCondition.Reason = riskConditionReasonEvaluationFailed
588-
riskCondition.Message = msg
589-
} else if match {
590-
riskCondition.Status = metav1.ConditionTrue
591-
riskCondition.Reason = riskConditionReasonMatch
659+
errorMessages = append(errorMessages, riskCondition.Message)
660+
case metav1.ConditionTrue:
592661
if shouldReconcileAcceptRisks() && acceptRisks.Has(risk.Name) {
593662
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionTrue)
594663
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonAllExposedRisksAccepted)
@@ -604,9 +673,6 @@ func evaluateConditionalUpdate(
604673
errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
605674
}
606675
}
607-
if _, ok := riskConditions[risk.Name]; !ok {
608-
riskConditions[risk.Name] = []metav1.Condition{riskCondition}
609-
}
610676
}
611677
if len(errorMessages) > 0 {
612678
recommended.Message = strings.Join(errorMessages, "\n\n")

0 commit comments

Comments
 (0)