Skip to content

Commit 4f872bb

Browse files
hongkailiuhongkailiu
committed
ToSquash: Address comments from review
1 parent 2203076 commit 4f872bb

3 files changed

Lines changed: 44 additions & 26 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: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -529,10 +529,18 @@ type AlertGetter interface {
529529
}
530530

531531
func (u *availableUpdates) evaluateAlertRisks(ctx context.Context) error {
532+
if u == nil || u.AlertGetter == nil {
533+
u.AlertRisks = nil
534+
return nil
535+
}
532536
alerts, err := u.AlertGetter.Get(ctx)
533537
if err != nil {
534538
return fmt.Errorf("failed to get alerts: %w", err)
535539
}
540+
if alerts == nil {
541+
u.AlertRisks = nil
542+
return nil
543+
}
536544
u.AlertRisks = alertsToRisks(alerts.Data.Alerts)
537545
return nil
538546
}
@@ -583,7 +591,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
583591
if alertName == "PodDisruptionBudgetLimit" {
584592
details = fmt.Sprintf("Namespace=%s, PodDisruptionBudget=%s. %s", alert.Labels.Namespace, alert.Labels.PodDisruptionBudget, details)
585593
}
586-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
594+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
587595
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
588596
Status: metav1.ConditionTrue,
589597
Reason: fmt.Sprintf("Alert:%s", alert.State),
@@ -595,7 +603,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
595603

596604
if alertName == "PodDisruptionBudgetAtLimit" {
597605
details = fmt.Sprintf("Namespace=%s, PodDisruptionBudget=%s. %s", alert.Labels.Namespace, alert.Labels.PodDisruptionBudget, details)
598-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
606+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
599607
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
600608
Status: metav1.ConditionTrue,
601609
Reason: internal.AlertConditionReason(alert.State),
@@ -606,7 +614,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
606614
}
607615

608616
if alertName == "KubeContainerWaiting" {
609-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
617+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
610618
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
611619
Status: metav1.ConditionTrue,
612620
Reason: internal.AlertConditionReason(alert.State),
@@ -617,7 +625,7 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
617625
}
618626

619627
if alertName == "KubeNodeNotReady" || alertName == "KubeNodeReadinessFlapping" || alertName == "KubeNodeUnreachable" {
620-
addCondition(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
628+
risks[alertName] = getRisk(risks, alertName, summary, alertURL, alertPromQL, metav1.Condition{
621629
Type: internal.ConditionalUpdateRiskConditionTypeApplies,
622630
Status: metav1.ConditionTrue,
623631
Reason: internal.AlertConditionReason(alert.State),
@@ -644,10 +652,10 @@ func alertsToRisks(alerts []alert.Alert) []configv1.ConditionalUpdateRisk {
644652
return ret
645653
}
646654

647-
func addCondition(risks map[string]configv1.ConditionalUpdateRisk, riskName, message, url, promQL string, condition metav1.Condition) {
655+
func getRisk(risks map[string]configv1.ConditionalUpdateRisk, riskName, message, url, promQL string, condition metav1.Condition) configv1.ConditionalUpdateRisk {
648656
risk, ok := risks[riskName]
649657
if !ok {
650-
risks[riskName] = configv1.ConditionalUpdateRisk{
658+
return configv1.ConditionalUpdateRisk{
651659
Name: riskName,
652660
Message: message,
653661
URL: url,
@@ -661,14 +669,17 @@ func addCondition(risks map[string]configv1.ConditionalUpdateRisk, riskName, mes
661669
},
662670
Conditions: []metav1.Condition{condition},
663671
}
664-
return
665672
}
666673

667-
risk.Conditions[0].Message = fmt.Sprintf("%s; %s", risk.Conditions[0].Message, condition.Message)
668-
if risk.Conditions[0].LastTransitionTime.After(condition.LastTransitionTime.Time) {
669-
risk.Conditions[0].LastTransitionTime = condition.LastTransitionTime
674+
if c := meta.FindStatusCondition(risk.Conditions, condition.Type); c != nil {
675+
c.Message = fmt.Sprintf("%s; %s", c.Message, condition.Message)
676+
if c.LastTransitionTime.After(condition.LastTransitionTime.Time) {
677+
c.LastTransitionTime = condition.LastTransitionTime
678+
}
679+
meta.SetStatusCondition(&risk.Conditions, *c)
670680
}
671681

682+
return risk
672683
}
673684

674685
func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {

‎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)