another go

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

Author: Per Goncalves da Silva

pkg/api/client/clientset/versioned/fake/decorator.go

@@ -53,7 +53,6 @@ func NewReactionForwardingClientsetDecorator(objects []runtime.Object, options .
 	for _, option := range options {
 		option(decorator)
 	}
-
 	return decorator
 }
 

pkg/controller/operators/catalog/operator.go

@@ -1477,25 +1477,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
 	}
 
@@ -1667,15 +1672,19 @@ func (o *Operator) ensureInstallPlan(logger *logrus.Entry, namespace string, gen
 	}
 	logger.Warn("no installplan found with matching generation, creating new one")
 
-	return o.createInstallPlan(namespace, gen, subs, installPlanApproval, steps, bundleLookups)
+	ip, err := o.createInstallPlan(namespace, gen, subs, installPlanApproval, steps, bundleLookups)
+	if err != nil {
+		return nil, err
+	}
+	return reference.GetReference(ip)
 }
 
 func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1.Subscription, installPlanApproval v1alpha1.Approval, steps []*v1alpha1.Step, bundleLookups []v1alpha1.BundleLookup) (*corev1.ObjectReference, error) {
 	if len(steps) == 0 && len(bundleLookups) == 0 {
 		return nil, nil
 	}
 
-	csvNames := []string{}
+	var csvNames []string
 	catalogSourceMap := map[string]struct{}{}
 	for _, s := range steps {
 		if s.Resource.Kind == "ClusterServiceVersion" {
@@ -1684,7 +1693,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 +1729,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 +1764,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 {

pkg/controller/operators/catalog/operator_test.go

@@ -2083,7 +2083,7 @@ func NewFakeOperator(ctx context.Context, namespace string, namespaces []string,
 	clientFake := fake.NewReactionForwardingClientsetDecorator(config.clientObjs, config.clientOptions...)
 	// TODO: Using the ReactionForwardingClientsetDecorator for k8s objects causes issues with adding Resources for discovery.
 	// For now, directly use a SimpleClientset instead.
-	k8sClientFake := k8sfake.NewSimpleClientset(config.k8sObjs...)
+	k8sClientFake := k8sfake.NewClientset(config.k8sObjs...)
 	k8sClientFake.Resources = apiResourcesForObjects(append(config.extObjs, config.regObjs...))
 	opClientFake := operatorclient.NewClient(k8sClientFake, apiextensionsfake.NewSimpleClientset(config.extObjs...), apiregistrationfake.NewSimpleClientset(config.regObjs...))
 	dynamicClientFake := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme())