Skip to content

WIP: pkg/cvo/availableupdates: Add recommend/... conditions for each matching or eval-failed risk #1132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: pkg/cvo/availableupdates: Add recommend/... conditions for each matching or eval-failed risk #1132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 57 additions & 11 deletions pkg/cvo/availableupdates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,17 +428,31 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
return vi.GTE(vj)
})
for i, conditionalUpdate := range u.ConditionalUpdates {
condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry)

if condition.Status == metav1.ConditionTrue {
u.addUpdate(conditionalUpdate.Release)
} else {
u.removeUpdate(conditionalUpdate.Release.Image)
conditions := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry)

knownTypes := make(map[string]struct{}, len(conditions))
for _, condition := range conditions {
knownTypes[condition.Type] = struct{}{}
if condition.Type == ConditionalUpdateConditionTypeRecommended {
if condition.Status == metav1.ConditionTrue {
u.addUpdate(conditionalUpdate.Release)
} else {
u.removeUpdate(conditionalUpdate.Release.Image)
}
}

meta.SetStatusCondition(&conditionalUpdate.Conditions, condition)
}

meta.SetStatusCondition(&conditionalUpdate.Conditions, condition)
u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions
for i := len(conditionalUpdate.Conditions) - 1; i >= 0; i-- {
conditionType := conditionalUpdate.Conditions[i].Type
if _, ok := knownTypes[conditionType]; !ok {
meta.RemoveStatusCondition(&conditionalUpdate.Conditions, conditionType)
}
}

u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions
}
}

Expand Down Expand Up @@ -491,6 +505,7 @@ const (
// Reasons follow same pattern as k8s Condition Reasons
// https://github.com/openshift/api/blob/59fa376de7cb668ddb95a7ee4e9879d7f6ca2767/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1535-L1536
var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`)
var reasonAlwaysSafeCharacters = regexp.MustCompile(`[A-Za-z]`)

func newRecommendedReason(now, want string) string {
switch {
Expand All @@ -503,36 +518,67 @@ 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) []metav1.Condition {
recommended := metav1.Condition{
Type: 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.",
}
conditions := make([]metav1.Condition, 0, len(risks)+1)

var errorMessages []string
for _, risk := range risks {
cleanName := risk.Name
if !reasonPattern.MatchString(cleanName) {
safeCharacters := reasonAlwaysSafeCharacters.FindAllString(cleanName, -1)
if len(safeCharacters) > 0 {
cleanName = strings.Join(safeCharacters, "")
} else {
cleanName = "UnsalvageableRiskName"
}
}
conditionType := fmt.Sprintf("%s/%s", strings.ToLower(ConditionalUpdateConditionTypeRecommended), cleanName)
if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown)
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed)
errorMessages = append(errorMessages, unknownExposureMessage(risk, err))
msg := unknownExposureMessage(risk, err)
errorMessages = append(errorMessages, msg)
conditions = append(conditions, metav1.Condition{
Type: conditionType,
Status: metav1.ConditionUnknown,
Reason: recommendedReasonEvaluationFailed,
Message: msg,
})
} else if match {
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))
msg := fmt.Sprintf("%s %s", risk.Message, risk.URL)
errorMessages = append(errorMessages, msg)
conditions = append(conditions, metav1.Condition{
Type: conditionType,
Status: metav1.ConditionFalse,
Reason: wantReason,
Message: msg,
})
}
}
if len(errorMessages) > 0 {
recommended.Message = strings.Join(errorMessages, "\n\n")
}

return recommended
for foundTypeOverlap := true; foundTypeOverlap; {
foundTypeOverlap = false
// FIXME: add suffixes to any matching types in conditions
}

conditions = append(conditions, recommended)
return conditions
}

func injectClusterIdIntoConditionalUpdates(clusterId string, updates []configv1.ConditionalUpdate) []configv1.ConditionalUpdate {
Expand Down
82 changes: 65 additions & 17 deletions pkg/cvo/availableupdates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ func osusWithSingleConditionalEdge() (*httptest.Server, clusterconditions.Condit
},
},
Conditions: []metav1.Condition{
{
Type: "recommended/FourFiveSix",
Status: metav1.ConditionFalse,
Reason: "FourFiveSix",
Message: "Four Five Five is just fine https://example.com/" + to,
LastTransitionTime: metav1.Now(),
},
{
Type: "Recommended",
Status: metav1.ConditionFalse,
Expand Down Expand Up @@ -233,8 +240,10 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing.
availableUpdates, optr := newOperator(fakeOsus.URL, version, mockPromql)
optr.availableUpdates = availableUpdates
optr.availableUpdates.ConditionalUpdates = conditionalUpdates
expectedConditions := []metav1.Condition{{}}
conditionalUpdates[0].Conditions[0].DeepCopyInto(&expectedConditions[0])
expectedConditions := make([]metav1.Condition, 0, len(conditionalUpdates[0].Conditions))
for _, condition := range conditionalUpdates[0].Conditions {
expectedConditions = append(expectedConditions, *condition.DeepCopy())
}
cv := cvFixture.DeepCopy()
tc.modifyOriginalState(optr)
tc.modifyCV(cv, conditionalUpdates[0])
Expand Down Expand Up @@ -268,16 +277,16 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
name string
risks []configv1.ConditionalUpdateRisk
mockPromql clusterconditions.Condition
expected metav1.Condition
expected []metav1.Condition
}{
{
name: "no risks",
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "Recommended",
Status: metav1.ConditionTrue,
Reason: recommendedReasonRisksNotExposed,
Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
},
}},
},
{
name: "one risk that does not match",
Expand All @@ -293,12 +302,12 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
ValidQueue: []error{nil},
MatchQueue: []mock.MatchResult{{Match: false, Error: nil}},
},
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "Recommended",
Status: metav1.ConditionTrue,
Reason: recommendedReasonRisksNotExposed,
Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
},
}},
},
{
name: "one risk that matches",
Expand All @@ -314,12 +323,17 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
ValidQueue: []error{nil},
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}},
},
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "recommended/RiskThatApplies",
Status: metav1.ConditionFalse,
Reason: "RiskThatApplies",
Message: "This is a risk! https://match.es",
}, {
Type: "Recommended",
Status: metav1.ConditionFalse,
Reason: "RiskThatApplies",
Message: "This is a risk! https://match.es",
},
}},
},
{
name: "matching risk with name that cannot be used as a condition reason",
Expand All @@ -335,12 +349,17 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
ValidQueue: []error{nil},
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}},
},
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "recommended/RISKTHATAPPLIES",
Status: metav1.ConditionFalse,
Reason: recommendedReasonExposed,
Message: "This is a risk! https://match.es",
}, {
Type: "Recommended",
Status: metav1.ConditionFalse,
Reason: recommendedReasonExposed,
Message: "This is a risk! https://match.es",
},
}},
},
{
name: "two risks that match",
Expand All @@ -362,12 +381,22 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
ValidQueue: []error{nil, nil},
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: true, Error: nil}},
},
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "recommended/RiskThatApplies",
Status: metav1.ConditionFalse,
Reason: "RiskThatApplies",
Message: "This is a risk! https://match.es",
}, {
Type: "recommended/RiskThatAppliesToo",
Status: metav1.ConditionFalse,
Reason: "RiskThatAppliesToo",
Message: "This is a risk too! https://match.es/too",
}, {
Type: "Recommended",
Status: metav1.ConditionFalse,
Reason: recommendedReasonMultiple,
Message: "This is a risk! https://match.es\n\nThis is a risk too! https://match.es/too",
},
}},
},
{
name: "first risk matches, second fails to evaluate",
Expand All @@ -389,15 +418,27 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
ValidQueue: []error{nil, nil},
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: false, Error: errors.New("ERROR")}},
},
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "recommended/RiskThatApplies",
Status: metav1.ConditionFalse,
Reason: "RiskThatApplies",
Message: "This is a risk! https://match.es",
}, {
Type: "recommended/RiskThatFailsToEvaluate",
Status: metav1.ConditionUnknown,
Reason: recommendedReasonEvaluationFailed,
Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
" RiskThatFailsToEvaluate description: This is a risk too!\n" +
" RiskThatFailsToEvaluate URL: https://whokno.ws",
}, {
Type: "Recommended",
Status: metav1.ConditionFalse,
Reason: recommendedReasonMultiple,
Message: "This is a risk! https://match.es\n\n" +
"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",
Expand All @@ -413,14 +454,21 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
ValidQueue: []error{nil},
MatchQueue: []mock.MatchResult{{Match: false, Error: errors.New("ERROR")}},
},
expected: metav1.Condition{
expected: []metav1.Condition{{
Type: "recommended/RiskThatFailsToEvaluate",
Status: metav1.ConditionUnknown,
Reason: recommendedReasonEvaluationFailed,
Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
" RiskThatFailsToEvaluate description: This is a risk!\n" +
" RiskThatFailsToEvaluate URL: https://whokno.ws",
}, {
Type: "Recommended",
Status: metav1.ConditionUnknown,
Reason: recommendedReasonEvaluationFailed,
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 {
Expand Down