Skip to content

Commit 7413293

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 c38734f commit 7413293

6 files changed

Lines changed: 268 additions & 142 deletions

File tree

pkg/cvo/availableupdates.go

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

@@ -340,6 +339,80 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates {
340339
return u
341340
}
342341

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

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

457529
if condition.Status == metav1.ConditionTrue {
458530
u.addUpdate(conditionalUpdate.Release)
@@ -554,9 +626,7 @@ func newRecommendedReason(now, want string) string {
554626
}
555627

556628
func evaluateConditionalUpdate(
557-
ctx context.Context,
558629
risks []configv1.ConditionalUpdateRisk,
559-
conditionRegistry clusterconditions.ConditionRegistry,
560630
acceptRisks sets.Set[string],
561631
shouldReconcileAcceptRisks func() bool,
562632
riskConditions map[string][]metav1.Condition,
@@ -571,22 +641,21 @@ func evaluateConditionalUpdate(
571641

572642
var errorMessages []string
573643
for _, risk := range risks {
574-
riskCondition := metav1.Condition{
575-
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
576-
Status: metav1.ConditionFalse,
577-
Reason: riskConditionReasonNotMatch,
644+
riskCondition := meta.FindStatusCondition(riskConditions[risk.Name], internal.ConditionalUpdateRiskConditionTypeApplies)
645+
if riskCondition == nil {
646+
// This should never happen
647+
riskCondition = &metav1.Condition{
648+
Status: metav1.ConditionUnknown,
649+
Reason: internal.ConditionalUpdateRiskConditionReasonInternalErrorFoundNoRiskCondition,
650+
Message: fmt.Sprintf("failed to find risk condition for risk %s", risk.Name),
651+
}
578652
}
579-
if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
580-
msg := unknownExposureMessage(risk, err)
653+
switch riskCondition.Status {
654+
case metav1.ConditionUnknown:
581655
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown)
582656
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed)
583-
errorMessages = append(errorMessages, msg)
584-
riskCondition.Status = metav1.ConditionUnknown
585-
riskCondition.Reason = riskConditionReasonEvaluationFailed
586-
riskCondition.Message = msg
587-
} else if match {
588-
riskCondition.Status = metav1.ConditionTrue
589-
riskCondition.Reason = riskConditionReasonMatch
657+
errorMessages = append(errorMessages, riskCondition.Message)
658+
case metav1.ConditionTrue:
590659
if shouldReconcileAcceptRisks() && acceptRisks.Has(risk.Name) {
591660
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionTrue)
592661
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonAllExposedRisksAccepted)
@@ -602,9 +671,6 @@ func evaluateConditionalUpdate(
602671
errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
603672
}
604673
}
605-
if _, ok := riskConditions[risk.Name]; !ok {
606-
riskConditions[risk.Name] = []metav1.Condition{riskCondition}
607-
}
608674
}
609675
if len(errorMessages) > 0 {
610676
recommended.Message = strings.Join(errorMessages, "\n\n")

0 commit comments

Comments
 (0)