another go

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

pkg/controller/operators/catalog/operator.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"k8s.io/client-go/util/retry"
 	"reflect"
 	"sort"
 	"strings"
@@ -1477,25 +1478,30 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 	o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed)
 
 	for _, updatedSub := range updatedSubs {
-		newSub := true
+		isNewSub := true
 		updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed)
 		for i, sub := range subs {
 			if sub.Name == updatedSub.Name && sub.Namespace == updatedSub.Namespace {
 				subs[i] = updatedSub
-				newSub = false
+				isNewSub = false
 				break
 			}
 		}
-		if newSub {
+		if isNewSub {
 			subs = append(subs, updatedSub)
-			continue
 		}
 	}
 
+	// *********************************** WARNING *********************************** //
+	// if updateSubscriptionStatuses call fails and the IP reference was changed       //
+	// in this run, we'll be screwed because the IP would have been created but the    //
+	// subscription won't carry its reference. And, if the CSV gets created by the IP, //
+	// it won't be associated with the Subscription -> perma-failure on resolution     //
+	// ******************************************************************************* //
 	// Update subscriptions with all changes so far
 	_, updateErr := o.updateSubscriptionStatuses(subs)
 	if updateErr != nil {
-		logger.WithError(updateErr).Warn("failed to update subscription conditions")
+		logger.WithField("irreconcilable-resolution", namespace).WithError(updateErr).Warn("failed to patch subscription conditions")
 		return updateErr
 	}
 
@@ -1675,7 +1681,7 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
 		return nil, nil
 	}
 
-	csvNames := []string{}
+	var csvNames []string
 	catalogSourceMap := map[string]struct{}{}
 	for _, s := range steps {
 		if s.Resource.Kind == "ClusterServiceVersion" {
@@ -1684,7 +1690,7 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
 		catalogSourceMap[s.Resource.CatalogSource] = struct{}{}
 	}
 
-	catalogSources := []string{}
+	var catalogSources []string
 	for s := range catalogSourceMap {
 		catalogSources = append(catalogSources, s)
 	}
@@ -1720,12 +1726,18 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
 		CatalogSources: catalogSources,
 		BundleLookups:  bundleLookups,
 	}
-	res, err = o.client.OperatorsV1alpha1().InstallPlans(namespace).UpdateStatus(context.TODO(), res, metav1.UpdateOptions{})
+
+	// *************************** WARNING *************************** //
+	// If this call fails, the IP will never do anything since it      //
+	// requires the steps to exist in .status                          //
+	// *************************************************************** //
+	newRes, err := o.client.OperatorsV1alpha1().InstallPlans(namespace).UpdateStatus(context.TODO(), res, metav1.UpdateOptions{})
 	if err != nil {
+		o.logger.WithField("irreconcilable-ip", res.GetName()).WithError(err).Warn("error updating installplan status after creation")
 		return nil, err
 	}
 
-	return reference.GetReference(res)
+	return reference.GetReference(newRes)
 }
 
 // setSubsCond will set the condition to the subscription if it doesn't already
@@ -1749,7 +1761,7 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.Subs
 	return subList
 }
 
-// removeSubsCond removes the given condition from all of the subscriptions in the input
+// removeSubsCond removes the given condition from all the subscriptions in the input
 func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) {
 	lastUpdated := o.now()
 	for _, sub := range subs {
@@ -1775,7 +1787,19 @@ func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]
 		wg.Add(1)
 		go func(sub *v1alpha1.Subscription) {
 			defer wg.Done()
-			if _, err := o.client.OperatorsV1alpha1().Subscriptions(sub.Namespace).UpdateStatus(context.TODO(), sub, updateOpts); err != nil {
+
+			update := func() error {
+				// Update the status of the latest revision
+				latest, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Get(context.TODO(), sub.GetName(), getOpts)
+				if err != nil {
+					return err
+				}
+				latest.Status = sub.Status
+				*sub = *latest
+				_, err = o.client.OperatorsV1alpha1().Subscriptions(sub.Namespace).UpdateStatus(context.TODO(), latest, updateOpts)
+				return err
+			}
+			if err := retry.RetryOnConflict(retry.DefaultRetry, update); err != nil {
 				mu.Lock()
 				defer mu.Unlock()
 				errs = append(errs, err)

pkg/controller/registry/resolver/source_csvs.go

@@ -93,19 +93,19 @@ func (s *csvSource) Snapshot(ctx context.Context) (*cache.Snapshot, error) {
 			continue
 		}
 
-		//if cachedSubscription, ok := csvSubscriptions[csv]; !ok || cachedSubscription == nil {
-		//	// we might be in an incoherent state, so let's check with live clients to make sure
-		//	realSubscriptions, err := s.listSubscriptions(ctx)
-		//	if err != nil {
-		//		return nil, fmt.Errorf("failed to list subscriptions: %w", err)
-		//	}
-		//	for _, realSubscription := range realSubscriptions.Items {
-		//		if realSubscription.Status.InstalledCSV == csv.Name {
-		//			// oops, live cluster state is coherent
-		//			return nil, fmt.Errorf("lister caches incoherent for CSV %s/%s - found owning Subscription %s/%s", csv.Namespace, csv.Name, realSubscription.Namespace, realSubscription.Name)
-		//		}
-		//	}
-		//}
+		if cachedSubscription, ok := csvSubscriptions[csv]; !ok || cachedSubscription == nil {
+			// we might be in an incoherent state, so let's check with live clients to make sure
+			realSubscriptions, err := s.listSubscriptions(ctx)
+			if err != nil {
+				return nil, fmt.Errorf("failed to list subscriptions: %w", err)
+			}
+			for _, realSubscription := range realSubscriptions.Items {
+				if realSubscription.Status.InstalledCSV == csv.Name {
+					// oops, live cluster state is coherent
+					return nil, fmt.Errorf("lister caches incoherent for CSV %s/%s - found owning Subscription %s/%s", csv.Namespace, csv.Name, realSubscription.Namespace, realSubscription.Name)
+				}
+			}
+		}
 
 		if failForwardEnabled {
 			replacementChainEndsInFailure, err := isReplacementChainThatEndsInFailure(csv, ReplacementMapping(csvs))