Merge pull request #1098 from wking/preserve-cache-on-update-service-error

OCPBUGS-44018: pkg/cvo/availableupdates: Preserve update advice on update-service failures

Author: openshift-merge-bot[bot]

pkg/cvo/availableupdates.go

@@ -57,6 +57,8 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
 	// updates are only checked at most once per minimumUpdateCheckInterval or if the generation changes
 	optrAvailableUpdates := optr.getAvailableUpdates()
 	needFreshFetch := true
+	preserveCacheOnFailure := false
+	maximumCacheInterval := 24 * time.Hour
 	if optrAvailableUpdates == nil {
 		klog.V(2).Info("First attempt to retrieve available updates")
 		optrAvailableUpdates = &availableUpdates{}
@@ -66,14 +68,18 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
 		for i := range config.Status.ConditionalUpdates {
 			optrAvailableUpdates.ConditionalUpdates = append(optrAvailableUpdates.ConditionalUpdates, *config.Status.ConditionalUpdates[i].DeepCopy())
 		}
-	} else if !optrAvailableUpdates.RecentlyChanged(optr.minimumUpdateCheckInterval) {
-		klog.V(2).Infof("Retrieving available updates again, because more than %s has elapsed since %s", optr.minimumUpdateCheckInterval, optrAvailableUpdates.LastAttempt.Format(time.RFC3339))
 	} else if channel != optrAvailableUpdates.Channel {
 		klog.V(2).Infof("Retrieving available updates again, because the channel has changed from %q to %q", optrAvailableUpdates.Channel, channel)
 	} else if desiredArch != optrAvailableUpdates.Architecture {
 		klog.V(2).Infof("Retrieving available updates again, because the architecture has changed from %q to %q", optrAvailableUpdates.Architecture, desiredArch)
+	} else if !optrAvailableUpdates.RecentlyChanged(maximumCacheInterval) {
+		klog.V(2).Infof("Retrieving available updates again, because more than %s has elapsed since last change at %s.  Will clear the cache if this fails.", maximumCacheInterval, optrAvailableUpdates.LastAttempt.Format(time.RFC3339))
+	} else if !optrAvailableUpdates.RecentlyAttempted(optr.minimumUpdateCheckInterval) {
+		klog.V(2).Infof("Retrieving available updates again, because more than %s has elapsed since last attempt at %s", optr.minimumUpdateCheckInterval, optrAvailableUpdates.LastAttempt.Format(time.RFC3339))
+		preserveCacheOnFailure = true
 	} else if updateService == optrAvailableUpdates.UpdateService || (updateService == defaultUpdateService && optrAvailableUpdates.UpdateService == "") {
 		needsConditionalUpdateEval := false
+		preserveCacheOnFailure = true
 		for _, conditionalUpdate := range optrAvailableUpdates.ConditionalUpdates {
 			if recommended := findRecommendedCondition(conditionalUpdate.Conditions); recommended == nil {
 				needsConditionalUpdateEval = true
@@ -126,11 +132,19 @@ func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1
 		optrAvailableUpdates.UpdateService = updateService
 		optrAvailableUpdates.Channel = channel
 		optrAvailableUpdates.Architecture = desiredArch
-		optrAvailableUpdates.Current = current
-		optrAvailableUpdates.Updates = updates
-		optrAvailableUpdates.ConditionalUpdates = conditionalUpdates
 		optrAvailableUpdates.ConditionRegistry = optr.conditionRegistry
 		optrAvailableUpdates.Condition = condition
+
+		responseFailed := (condition.Type == configv1.RetrievedUpdates &&
+			condition.Status == configv1.ConditionFalse &&
+			(condition.Reason == "RemoteFailed" ||
+				condition.Reason == "ResponseFailed" ||
+				condition.Reason == "ResponseInvalid"))
+		if !responseFailed || (responseFailed && !preserveCacheOnFailure) {
+			optrAvailableUpdates.Current = current
+			optrAvailableUpdates.Updates = updates
+			optrAvailableUpdates.ConditionalUpdates = conditionalUpdates
+		}
 	}
 
 	optrAvailableUpdates.evaluateConditionalUpdates(ctx)
@@ -183,10 +197,14 @@ type availableUpdates struct {
 	Condition configv1.ClusterOperatorStatusCondition
 }
 
-func (u *availableUpdates) RecentlyChanged(interval time.Duration) bool {
+func (u *availableUpdates) RecentlyAttempted(interval time.Duration) bool {
 	return u.LastAttempt.After(time.Now().Add(-interval))
 }
 
+func (u *availableUpdates) RecentlyChanged(interval time.Duration) bool {
+	return u.LastSyncOrConfigChange.After(time.Now().Add(-interval))
+}
+
 func (u *availableUpdates) NeedsUpdate(original *configv1.ClusterVersion) *configv1.ClusterVersion {
 	if u == nil {
 		return nil

pkg/cvo/cvo_test.go

@@ -2675,17 +2675,18 @@ func TestOperator_availableUpdatesSync(t *testing.T) {
 			},
 		},
 		{
-			name: "if last check time was too recent, do nothing",
+			name: "if last successful check time was too recent, do nothing",
 			handler: func(w http.ResponseWriter, req *http.Request) {
 				http.Error(w, "bad things", http.StatusInternalServerError)
 			},
 			optr: &Operator{
 				updateService:              "http://localhost:8080/graph",
 				minimumUpdateCheckInterval: 1 * time.Minute,
 				availableUpdates: &availableUpdates{
-					UpdateService: "http://localhost:8080/graph",
-					Channel:       "fast",
-					LastAttempt:   time.Now(),
+					UpdateService:          "http://localhost:8080/graph",
+					Channel:                "fast",
+					LastAttempt:            time.Now(),
+					LastSyncOrConfigChange: time.Now(),
 				},
 				release: configv1.Release{
 					Version: "4.0.1",