Skip to content

Commit a558c77

Browse files
hongkailiuhongkailiu
committed
ToSquash: Address comments from review
1 parent 3487f55 commit a558c77

5 files changed

Lines changed: 69 additions & 33 deletions

File tree

‎pkg/alert/inspectalerts.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"io"
99
"net/http"
1010
"net/url"
11+
"time"
1112

1213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1314
"k8s.io/apimachinery/pkg/util/errors"
@@ -46,12 +47,12 @@ func getWithRoundTripper(ctx context.Context, roundTripper http.RoundTripper, ge
4647
return nil, err
4748
}
4849

49-
client := &http.Client{Transport: roundTripper}
50+
client := &http.Client{Transport: roundTripper, Timeout: 15 * time.Second}
5051
errs := make([]error, 0, len(route.Status.Ingress))
5152
for _, ingress := range route.Status.Ingress {
5253
uri := *baseURI
5354
uri.Host = ingress.Host
54-
content, err := checkedGet(uri, client)
55+
content, err := checkedGet(ctx, uri, client)
5556
if err == nil {
5657
return content, nil
5758
} else {
@@ -67,8 +68,8 @@ func getWithRoundTripper(ctx context.Context, roundTripper http.RoundTripper, ge
6768

6869
}
6970

70-
func checkedGet(uri url.URL, client *http.Client) ([]byte, error) {
71-
req, err := http.NewRequest("GET", uri.String(), nil)
71+
func checkedGet(ctx context.Context, uri url.URL, client *http.Client) ([]byte, error) {
72+
req, err := http.NewRequestWithContext(ctx, "GET", uri.String(), nil)
7273
if err != nil {
7374
return nil, err
7475
}
@@ -77,7 +78,9 @@ func checkedGet(uri url.URL, client *http.Client) ([]byte, error) {
7778
if err != nil {
7879
return nil, err
7980
}
80-
defer resp.Body.Close()
81+
defer func() {
82+
_ = resp.Body.Close()
83+
}()
8184

8285
body, err := io.ReadAll(resp.Body)
8386
if err != nil {

‎pkg/cvo/availableupdates.go‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -531,10 +531,18 @@ type AlertGetter interface {
531531
}
532532

533533
func (u *availableUpdates) evaluateAlertRisks(ctx context.Context) error {
534+
if u == nil || u.AlertGetter == nil {
535+
u.AlertRisks = nil
536+
return nil
537+
}
534538
alerts, err := u.AlertGetter.Get(ctx)
535539
if err != nil {
536540
return fmt.Errorf("failed to get alerts: %w", err)
537541
}
542+
if alerts == nil {
543+
u.AlertRisks = nil
544+
return nil
545+
}
538546
u.AlertRisks = alertsToRisks(alerts.Data.Alerts)
539547
return nil
540548
}
@@ -585,7 +593,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
585593
if alertName == "PodDisruptionBudgetLimit" {
586594
details = fmt.Sprintf("Namespace=%s, PodDisruptionBudget=%s. %s", alert.Labels.Namespace, alert.Labels.PodDisruptionBudget, details)
587595
}
588-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
596+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
589597
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
590598
Status: metav1.ConditionTrue,
591599
Reason: fmt.Sprintf("Alert:%s", alert.State),
@@ -597,7 +605,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
597605

598606
if alertName == "PodDisruptionBudgetAtLimit" {
599607
details = fmt.Sprintf("Namespace=%s, PodDisruptionBudget=%s. %s", alert.Labels.Namespace, alert.Labels.PodDisruptionBudget, details)
600-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
608+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
601609
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
602610
Status: metav1.ConditionTrue,
603611
Reason: internal.AlertConditionReason(alert.State),
@@ -608,7 +616,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
608616
}
609617

610618
if alertName == "KubeContainerWaiting" {
611-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
619+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
612620
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
613621
Status: metav1.ConditionTrue,
614622
Reason: internal.AlertConditionReason(alert.State),
@@ -619,7 +627,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
619627
}
620628

621629
if alertName == "KubeNodeNotReady" || alertName == "KubeNodeReadinessFlapping" || alertName == "KubeNodeUnreachable" {
622-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
630+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
623631
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
624632
Status: metav1.ConditionTrue,
625633
Reason: internal.AlertConditionReason(alert.State),
@@ -646,10 +654,10 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
646654
return ret
647655
}
648656

649-
func addCondition(risks map[string]configv1.ConditionalUpdateRisk, riskName, message, url, promQL string, condition metav1.Condition) {
657+
func getRisk(risks map[string]configv1.ConditionalUpdateRisk, riskName, message, url, promQL string, condition metav1.Condition) configv1.ConditionalUpdateRisk {
650658
risk, ok := risks[riskName]
651659
if !ok {
652-
risks[riskName] = configv1.ConditionalUpdateRisk{
660+
return configv1.ConditionalUpdateRisk{
653661
Name: riskName,
654662
Message: message,
655663
URL: url,
@@ -663,14 +671,17 @@ func addCondition(risks map[string]configv1.ConditionalUpdateRisk, riskName, mes
663671
},
664672
Conditions: []metav1.Condition{condition},
665673
}
666-
return
667674
}
668675

669-
risk.Conditions[0].Message = fmt.Sprintf("%s; %s", risk.Conditions[0].Message, condition.Message)
670-
if risk.Conditions[0].LastTransitionTime.After(condition.LastTransitionTime.Time) {
671-
risk.Conditions[0].LastTransitionTime = condition.LastTransitionTime
676+
if c := meta.FindStatusCondition(risk.Conditions, condition.Type); c != nil {
677+
c.Message = fmt.Sprintf("%s; %s", c.Message, condition.Message)
678+
if c.LastTransitionTime.After(condition.LastTransitionTime.Time) {
679+
c.LastTransitionTime = condition.LastTransitionTime
680+
}
681+
meta.SetStatusCondition(&risk.Conditions, *c)
672682
}
673683

684+
return risk
674685
}
675686

676687
func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
@@ -682,7 +693,7 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
682693
risks := risksInOrder(riskVersions)
683694
u.RiskConditions = loadRiskConditions(ctx, risks, riskVersions, u.ConditionRegistry)
684695

685-
if err := sanityCheck(u.ConditionalUpdates); err != nil {
696+
if err := sanityCheck(u.ConditionalUpdates, u.AlertRisks); err != nil {
686697
klog.Errorf("Sanity check failed which might impact risk evaluation: %v", err)
687698
}
688699
for i, conditionalUpdate := range u.ConditionalUpdates {
@@ -700,7 +711,7 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
700711
}
701712
}
702713

703-
func sanityCheck(updates []configv1.ConditionalUpdate) error {
714+
func sanityCheck(updates []configv1.ConditionalUpdate, alertRisks []configv1.ConditionalUpdateRisk) error {
704715
risks := map[string]configv1.ConditionalUpdateRisk{}
705716
var errs []error
706717
for _, update := range updates {
@@ -716,6 +727,11 @@ func sanityCheck(updates []configv1.ConditionalUpdate) error {
716727
}
717728
}
718729
}
730+
for _, alertRisk := range alertRisks {
731+
if _, ok := risks[alertRisk.Name]; ok {
732+
errs = append(errs, fmt.Errorf("found alert risk and conditional update risk share the name: %s", alertRisk.Name))
733+
}
734+
}
719735
return utilerrors.NewAggregate(errs)
720736
}
721737

‎pkg/cvo/availableupdates_test.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/blang/semver/v4"
1515
"github.com/google/go-cmp/cmp"
1616
"github.com/google/go-cmp/cmp/cmpopts"
17+
1718
corev1 "k8s.io/api/core/v1"
1819
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1920
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -781,16 +782,18 @@ func TestSyncAvailableUpdatesDesiredUpdate(t *testing.T) {
781782

782783
func Test_sanityCheck(t *testing.T) {
783784
tests := []struct {
784-
name string
785-
updates []configv1.ConditionalUpdate
786-
expected error
785+
name string
786+
updates []configv1.ConditionalUpdate
787+
alertRisks []configv1.ConditionalUpdateRisk
788+
expected error
787789
}{
788790
{
789791
name: "good",
790792
updates: []configv1.ConditionalUpdate{
791793
{Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}}},
792794
{Risks: []configv1.ConditionalUpdateRisk{{Name: "riskB"}}},
793795
},
796+
alertRisks: []configv1.ConditionalUpdateRisk{{Name: "SomeAlert"}},
794797
},
795798
{
796799
name: "invalid risk name",
@@ -814,10 +817,19 @@ func Test_sanityCheck(t *testing.T) {
814817
},
815818
expected: utilerrors.NewAggregate([]error{fmt.Errorf("found collision on risk riskA: {[] riskA []} and {[] some riskA []}")}),
816819
},
820+
{
821+
name: "alert risk and conditional update risk conflict",
822+
updates: []configv1.ConditionalUpdate{
823+
{Risks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}}},
824+
{Risks: []configv1.ConditionalUpdateRisk{{Name: "riskB"}}},
825+
},
826+
alertRisks: []configv1.ConditionalUpdateRisk{{Name: "riskA"}},
827+
expected: utilerrors.NewAggregate([]error{fmt.Errorf("found alert risk and conditional update risk share the name: riskA")}),
828+
},
817829
}
818830
for _, tt := range tests {
819831
t.Run(tt.name, func(t *testing.T) {
820-
actual := sanityCheck(tt.updates)
832+
actual := sanityCheck(tt.updates, tt.alertRisks)
821833
if diff := cmp.Diff(tt.expected, actual, cmp.Comparer(func(x, y error) bool {
822834
if x == nil || y == nil {
823835
return x == nil && y == nil

‎pkg/internal/constants.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ package internal
22

33
import (
44
"fmt"
5-
"k8s.io/klog/v2"
65
"strings"
76

7+
"k8s.io/klog/v2"
8+
89
configv1 "github.com/openshift/api/config/v1"
910
)
1011

‎pkg/payload/precondition/clusterversion/recommendedupdate.go‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ func (ru *RecommendedUpdate) Run(ctx context.Context, releaseContext preconditio
4545
NonBlockingWarning: true,
4646
}
4747
}
48+
// This function should be guarded by shouldReconcileAcceptRisks()
49+
// https://github.com/openshift/cluster-version-operator/blob/f4b9dfa6f6968d117b919089c6b32918f20843c9/pkg/cvo/cvo.go#L1187
50+
// However, here we do not have a handler for the operator
51+
// Now it is guarded by clusterVersion.Status.ConditionalUpdateRisks which is guarded by the above function
4852
unAcceptRisks := sets.New[string]()
4953
acceptRisks := sets.New[string]()
5054
if clusterVersion.Spec.DesiredUpdate != nil {
@@ -75,10 +79,7 @@ func (ru *RecommendedUpdate) Run(ctx context.Context, releaseContext preconditio
7579
}
7680
for _, recommended := range clusterVersion.Status.AvailableUpdates {
7781
if recommended.Version == releaseContext.DesiredVersion {
78-
if alertError != nil {
79-
return alertError
80-
}
81-
return nil
82+
return aggregate(alertError, nil)
8283
}
8384
}
8485

@@ -88,7 +89,7 @@ func (ru *RecommendedUpdate) Run(ctx context.Context, releaseContext preconditio
8889
if condition.Type == internal.ConditionalUpdateConditionTypeRecommended {
8990
switch condition.Status {
9091
case metav1.ConditionTrue:
91-
return nil
92+
return aggregate(alertError, nil)
9293
case metav1.ConditionFalse:
9394
return aggregate(alertError, &precondition.Error{
9495
Reason: condition.Reason,
@@ -120,13 +121,13 @@ func (ru *RecommendedUpdate) Run(ctx context.Context, releaseContext preconditio
120121
}
121122

122123
if clusterVersion.Spec.Channel == "" {
123-
return &precondition.Error{
124+
return aggregate(alertError, &precondition.Error{
124125
Reason: "NoChannel",
125126
Message: fmt.Sprintf("Configured channel is unset, so the recommended status of updating from %s to %s is unknown.",
126127
clusterVersion.Status.Desired.Version, releaseContext.DesiredVersion),
127128
Name: ru.Name(),
128129
NonBlockingWarning: true,
129-
}
130+
})
130131
}
131132

132133
reason := "UnknownUpdate"
@@ -143,17 +144,20 @@ func (ru *RecommendedUpdate) Run(ctx context.Context, releaseContext preconditio
143144
}
144145

145146
if msg != "" {
146-
return &precondition.Error{
147+
return aggregate(alertError, &precondition.Error{
147148
Reason: reason,
148149
Message: msg,
149150
Name: ru.Name(),
150151
NonBlockingWarning: true,
151-
}
152+
})
152153
}
153-
return nil
154+
return aggregate(alertError, nil)
154155
}
155156

156-
func aggregate(e1, e2 *precondition.Error) *precondition.Error {
157+
func aggregate(e1, e2 *precondition.Error) error {
158+
if e1 == nil && e2 == nil {
159+
return nil
160+
}
157161
if e1 == nil {
158162
return e2
159163
}

0 commit comments

Comments
 (0)