refactor(operatorgroups): bail out after successful namespace update

- OperatorGroup sync bails out after successful namespace update
detected
- Check CSV status before re-transitioning to failed states due to
OperatorGroup issues
- Check CSV annotations before setting OperatorGroup annotations
- Make OperatorGroup requeuing more aggressive on CSV deletion
- Requeue OperatorGroup in OperatorGroup sync if CSV update fails and
CSV still exists

Author: njhale

pkg/controller/operators/olm/operator.go

@@ -346,6 +346,22 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 	defer func(csv v1alpha1.ClusterServiceVersion) {
 		logger.Debug("removing csv from queue set")
 		a.csvQueueSet.Remove(csv.GetName(), csv.GetNamespace())
+		
+		// Requeue all OperatorGroups in the namespace
+		logger.Debug("requeueing operatorgroups in namespace")
+		operatorGroups, err := a.lister.OperatorsV1alpha2().OperatorGroupLister().OperatorGroups(csv.GetNamespace()).List(labels.Everything())
+		if err != nil {
+			logger.WithError(err).Warnf("an error occurred while listing operatorgroups to requeue after csv deletion")
+			return
+		}
+
+		for _, operatorGroup := range operatorGroups {
+			logger := logger.WithField("operatorgroup", operatorGroup.GetName())
+			logger.Debug("requeueing")
+			if err := a.ogQueueSet.Requeue(operatorGroup.GetName(), operatorGroup.GetNamespace()); err != nil {
+				logger.WithError(err).Debug("error requeueing operatorgroup")
+			}
+		}
 	}(*clusterServiceVersion)
 
 	targetNamespaces, ok := clusterServiceVersion.Annotations[v1alpha2.OperatorGroupTargetsAnnotationKey]
@@ -360,28 +376,16 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 		return
 	}
 
-	operatorGroupName, ok := clusterServiceVersion.Annotations[v1alpha2.OperatorGroupAnnotationKey]
-	if !ok {
+	if _, ok = clusterServiceVersion.Annotations[v1alpha2.OperatorGroupAnnotationKey]; !ok {
 		logger.Debug("missing operatorgroup name annotation on csv")
 		return
 	}
 
-	if clusterServiceVersion.Status.Reason == v1alpha1.CSVReasonCopied {
+	if clusterServiceVersion.IsCopied() {
 		logger.Debug("deleted csv is copied. skipping additional cleanup steps")
 		return
 	}
 
-	logger = logger.WithField("operatorgroup", operatorGroupName)
-	logger.Info("active csv deleted, removing providedAPIs from operatorgroup annotations")
-	if operatorGroup, _ := a.lister.OperatorsV1alpha2().OperatorGroupLister().OperatorGroups(operatorNamespace).Get(operatorGroupName); operatorGroup != nil {
-		logger.Debug("requeueing")
-		if err := a.ogQueueSet.Requeue(operatorGroup.GetName(), operatorGroup.GetNamespace()); err != nil {
-			logger.WithError(err).Debug("error requeueing")
-		}
-	} else {
-		logger.Debug("operatorgroup not found during csv deletion")
-	}
-
 	logger.Info("gcing children")
 	namespaces := []string{}
 	if targetNamespaces == "" {
@@ -524,7 +528,7 @@ func (a *Operator) operatorGroupForActiveCSV(logger *logrus.Entry, csv *v1alpha1
 
 	// No OperatorGroup annotation
 	if !ok {
-		logger.Info("no operatorgroup annotation")
+		logger.Info("no olm.operatorGroup annotation")
 		return nil
 	}
 
@@ -537,9 +541,17 @@ func (a *Operator) operatorGroupForActiveCSV(logger *logrus.Entry, csv *v1alpha1
 		return nil
 	}
 
+	targets, ok := annotations[v1alpha2.OperatorGroupTargetsAnnotationKey]
+
+	// No target annotation
+	if !ok {
+		logger.Info("no olm.targetNamespaces annotation")
+		return nil
+	}
+	
 	// Target namespaces don't match
-	if annotations[v1alpha2.OperatorGroupTargetsAnnotationKey] != strings.Join(operatorGroup.Status.Namespaces, ",") {
-		logger.Info("target namespace annotation doesn't match operatorgroup namespace list")
+	if  targets != strings.Join(operatorGroup.Status.Namespaces, ",") {
+		logger.Info("olm.targetNamespaces annotation doesn't match operatorgroup status")
 		return nil
 	}
 
@@ -590,46 +602,54 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			operatorGroup = operatorGroups[0]
 			logger = logger.WithField("opgroup", operatorGroup.GetName())
 
-			a.addOperatorGroupAnnotations(&out.ObjectMeta, operatorGroup, true)
-			_, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(out)
-			if err != nil {
-				logger.Error("error adding operatorgroup annotation")
-				syncError = err
+			if a.operatorGroupAnnotationsDiffer(&out.ObjectMeta, operatorGroup) {
+				a.setOperatorGroupAnnotations(&out.ObjectMeta, operatorGroup, true)
+				if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(out); err != nil {
+					logger.WithError(err).Warn("error adding operatorgroup annotations")
+					syncError = err
+				}
 			}
+
 			return
 		}
 		logger.Info("csv in operatorgroup")
 	default:
 		syncError = fmt.Errorf("csv created in namespace with multiple operatorgroups, can't pick one automatically")
-		logger.Warn(syncError.Error())
-		out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonTooManyOperatorGroups, syncError.Error(), now, a.recorder)
+		logger.WithError(syncError).Warn("csv failed to become an operatorgroup member")
+		if out.Status.Reason != v1alpha1.CSVReasonTooManyOperatorGroups {
+			out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonTooManyOperatorGroups, syncError.Error(), now, a.recorder)
+		}
 		return
 	}
 
 	modeSet, err := v1alpha1.NewInstallModeSet(out.Spec.InstallModes)
 	if err != nil {
 		syncError = err
-		logger.Warn(err.Error())
-		out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonInvalidInstallModes, syncError.Error(), now, a.recorder)
+		logger.WithError(err).Warn("csv has invalid installmodes")
+		if out.Status.Reason != v1alpha1.CSVReasonInvalidInstallModes {
+			out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonInvalidInstallModes, syncError.Error(), now, a.recorder)
+		}
 		return
 	}
 
 	// Check if the CSV supports its operatorgroup's selected namespaces
 	targets, ok := out.GetAnnotations()[v1alpha2.OperatorGroupTargetsAnnotationKey]
 	if ok {
 		namespaces := strings.Split(targets, ",")
-		err = modeSet.Supports(out.GetNamespace(), namespaces)
-		if err != nil {
-			logger.WithField("reason", err.Error()).Infof("InstallModeSet does not support OperatorGroup namespace selection")
+		
+		if err := modeSet.Supports(out.GetNamespace(), namespaces); err != nil {
+			logger.WithField("reason", err.Error()).Info("installmodeset does not support operatorgroups namespace selection")
 			if out.Status.Reason != v1alpha1.CSVReasonUnsupportedOperatorGroup {
 				out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonUnsupportedOperatorGroup, err.Error(), now, a.recorder)
 			}
 			return
 		}
 	} else {
-		// This should never be the case
-		logger.Info("no targets annotation defined for CSV")
-		out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonNoTargetNamespaces, err.Error(), now, a.recorder)
+		logger.Info("csv missing olm.targetNamespaces annotation")
+		if out.Status.Reason != v1alpha1.CSVReasonNoTargetNamespaces {
+			out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonNoTargetNamespaces, "csv missing olm.targetNamespaces annotation", now, a.recorder)
+		}
+		return
 	}
 
 	// Check for intersecting provided APIs in intersecting OperatorGroups

pkg/controller/operators/olm/operator_test.go

@@ -3576,6 +3576,10 @@ func TestSyncOperatorGroups(t *testing.T) {
 				}
 			}
 
+			// Sync again to catch provided API changes
+			err = op.syncOperatorGroups(tt.initial.operatorGroup)
+			require.NoError(t, err)
+
 			operatorGroup, err := op.GetClient().OperatorsV1alpha2().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(tt.initial.operatorGroup.GetName(), metav1.GetOptions{})
 			require.NoError(t, err)
 			assert.Equal(t, tt.expectedStatus, operatorGroup.Status)

pkg/controller/operators/olm/operatorgroup.go

@@ -51,39 +51,55 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 		"namespace":     op.GetNamespace(),
 	})
 
-	targetNamespaces, err := a.updateNamespaceList(op)
+	targetNamespaces, err := a.getOperatorGroupTargets(op)
 	if err != nil {
-		logger.WithError(err).Warn("updateNamespaceList error")
+		logger.WithError(err).Warn("issue getting operatorgroup target namespaces")
 		return err
 	}
-	logger.WithField("targetNamespaces", targetNamespaces).Debug("updated target namespaces")
+
+	if namespacesChanged(targetNamespaces, op.Status.Namespaces) {
+		// Update operatorgroup target namespace selection
+		logger.WithField("targets", targetNamespaces).Debug("namespace change detected")
+		op.Status = v1alpha2.OperatorGroupStatus{
+			Namespaces:  targetNamespaces,
+			LastUpdated: timeNow(),
+		}
+	
+		if _, err = a.client.OperatorsV1alpha2().OperatorGroups(op.GetNamespace()).UpdateStatus(op); err != nil && !k8serrors.IsNotFound(err) {
+			logger.WithError(err).Warn("operatorgroup update failed")
+			return err
+		}
+
+		// CSV requeue is handled by the succeeding sync
+		return nil
+	}
 
 	if err := a.ensureOpGroupClusterRoles(op); err != nil {
-		a.Log.Errorf("ensureOpGroupClusterRoles error: %v", err)
+		logger.WithError(err).Warn("failed to ensure operatorgroup clusterroles")
 		return err
 	}
-	a.Log.Debug("cluster roles completed")
+	logger.Debug("operatorgroup clusterroles ensured")
 
 	set := a.csvSet(op.Namespace, v1alpha1.CSVPhaseAny)
 	providedAPIs := make(resolver.APISet)
 	for _, csv := range set {
 		logger := logger.WithField("csv", csv.GetName())
-		origCSVannotations := csv.GetAnnotations()
-		a.addOperatorGroupAnnotations(&csv.ObjectMeta, op, !csv.IsCopied())
-		if !reflect.DeepEqual(origCSVannotations, csv.GetAnnotations()) {
-			// CRDs don't support strategic merge patching, but in the future if they do this should be updated to patch
-			if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).Update(csv); err != nil {
-				// TODO: return an error and requeue the OperatorGroup here? Can this cause an update to never happen if there's resource contention?
-				logger.WithError(err).Warnf("update to existing csv failed")
-				continue
-			}
-		}
 
-		// Don't union providedAPIs if the CSV is copied (member of another OperatorGroup)
+		// Don't stomp on copied CSVs
 		if csv.IsCopied() {
-			logger.Debug("csv is copied. not including in operatorgroup's provided api set")
+			logger.Debug("csv is copied. not updating annotations or including in operatorgroup's provided api set")
 			continue
 		}
+		
+		if a.operatorGroupAnnotationsDiffer(&csv.ObjectMeta, op) {
+			a.setOperatorGroupAnnotations(&csv.ObjectMeta, op, true)
+			// CRDs don't support strategic merge patching, but in the future if they do this should be updated to patch
+			if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).Update(csv); err != nil && !k8serrors.IsNotFound(err) {
+				// TODO: return an error and requeue the OperatorGroup here? Can this cause an update to never happen if there's resource contention?
+				logger.WithError(err).Warnf("error adding operatorgroup annotations")
+				return err
+			}
+		}
 
 		// TODO: Throw out CSVs that aren't members of the group due to group related failures?
 
@@ -122,7 +138,7 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 		logger.Debug("removing provided apis from annotation to match cluster state")
 		if _, err := a.client.OperatorsV1alpha2().OperatorGroups(op.GetNamespace()).Update(op); err != nil && !k8serrors.IsNotFound(err) {
 			logger.WithError(err).Warn("could not update provided api annotations")
-			return nil
+			return err
 		}
 	}
 
@@ -502,14 +518,30 @@ func (a *Operator) copyCsvToTargetNamespace(csv *v1alpha1.ClusterServiceVersion,
 	return nil
 }
 
-func (a *Operator) addOperatorGroupAnnotations(obj *metav1.ObjectMeta, op *v1alpha2.OperatorGroup, addTargets bool) {
+func (a *Operator) setOperatorGroupAnnotations(obj *metav1.ObjectMeta, op *v1alpha2.OperatorGroup, addTargets bool) {
 	metav1.SetMetaDataAnnotation(obj, v1alpha2.OperatorGroupNamespaceAnnotationKey, op.GetNamespace())
 	metav1.SetMetaDataAnnotation(obj, v1alpha2.OperatorGroupAnnotationKey, op.GetName())
-	if addTargets {
+
+	if addTargets && op.Status.Namespaces != nil {
 		metav1.SetMetaDataAnnotation(obj, v1alpha2.OperatorGroupTargetsAnnotationKey, strings.Join(op.Status.Namespaces, ","))
 	}
 }
 
+func (a *Operator) operatorGroupAnnotationsDiffer(obj *metav1.ObjectMeta, op *v1alpha2.OperatorGroup) bool {
+	annotations := obj.GetAnnotations()
+	if operatorGroupNamespace, ok := annotations[v1alpha2.OperatorGroupNamespaceAnnotationKey]; !ok || operatorGroupNamespace != op.GetNamespace() {
+		return true
+	}
+	if operatorGroup, ok := annotations[v1alpha2.OperatorGroupAnnotationKey]; !ok || operatorGroup != op.GetName() {
+		return true
+	}
+	if targets, ok := annotations[v1alpha2.OperatorGroupTargetsAnnotationKey]; !ok || targets != strings.Join(op.Status.Namespaces, ",") {
+		return true
+	}
+
+	return false
+}
+
 func namespacesChanged(clusterNamespaces []string, statusNamespaces []string) bool {
 	if len(clusterNamespaces) != len(statusNamespaces) {
 		return true
@@ -527,7 +559,7 @@ func namespacesChanged(clusterNamespaces []string, statusNamespaces []string) bo
 	return false
 }
 
-func (a *Operator) updateNamespaceList(op *v1alpha2.OperatorGroup) ([]string, error) {
+func (a *Operator) getOperatorGroupTargets(op *v1alpha2.OperatorGroup) ([]string, error) {
 	selector, err := metav1.LabelSelectorAsSelector(&op.Spec.Selector)
 	if err != nil {
 		return nil, err
@@ -560,22 +592,6 @@ func (a *Operator) updateNamespaceList(op *v1alpha2.OperatorGroup) ([]string, er
 	}
 	sort.StringSlice(namespaceList).Sort()
 
-	if !namespacesChanged(namespaceList, op.Status.Namespaces) {
-		// status is current with correct namespaces, so no further updates required
-		return namespaceList, nil
-	}
-
-	a.Log.Debugf("Namespace change detected, found: %v", namespaceList)
-	op.Status = v1alpha2.OperatorGroupStatus{
-		Namespaces:  namespaceList,
-		LastUpdated: timeNow(),
-	}
-
-	_, err = a.client.OperatorsV1alpha2().OperatorGroups(op.GetNamespace()).UpdateStatus(op)
-	if err != nil {
-		return namespaceList, err
-	}
-
 	return namespaceList, nil
 }