Skip to content

Commit 196518f

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

5 files changed

Lines changed: 47 additions & 27 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
@@ -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) {

‎pkg/cvo/availableupdates_test.go‎

Lines changed: 1 addition & 0 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"

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