fix(og): Fix missing MultiOperatorGroups condition in some cases (#2305)

In some special cases, the MultiOperatorGroups condition is missing
even though there are multiple OGs in the same namespace. The process
of adding this condition happens during syncNamespace which sometimes
doesn't happen if syncOperatorGroups fails prematurely due to other
errors. Moving the MultiOperatorGroups condition to syncOperatorGroups
to ensure it will be run everytime.

Upstream-repository: operator-lifecycle-manager
Upstream-commit: d140ef062304440b37712692a170a93b3dc76fba

Signed-off-by: Vu Dinh <[email protected]>

Author: dinhxuanvu

staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go

@@ -15,7 +15,6 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	meta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -880,41 +879,6 @@ func (a *Operator) syncNamespace(obj interface{}) error {
 		return err
 	}
 
-	// Query OG in this namespace
-	groups, err := a.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(namespace.GetName()).List(labels.Everything())
-	if err != nil {
-		logger.WithError(err).Warn("failed to list OperatorGroups in the namespace")
-		return err
-	}
-
-	// Check if there is a stale multiple OG condition and clear it if existed.
-	if len(groups) == 1 {
-		og := groups[0]
-		if c := meta.FindStatusCondition(og.Status.Conditions, v1.MutlipleOperatorGroupCondition); c != nil {
-			meta.RemoveStatusCondition(&og.Status.Conditions, v1.MutlipleOperatorGroupCondition)
-			_, err = a.client.OperatorsV1().OperatorGroups(namespace.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
-			if err != nil {
-				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", og.GetName(), c, err.Error())
-			}
-		}
-	} else if len(groups) > 1 {
-		// Add to all OG's status conditions to indicate they're multiple OGs in the
-		// same namespace which is not allowed.
-		cond := metav1.Condition{
-			Type:    v1.MutlipleOperatorGroupCondition,
-			Status:  metav1.ConditionTrue,
-			Reason:  v1.MultipleOperatorGroupsReason,
-			Message: "Multiple OperatorGroup found in the same namespace",
-		}
-		for _, og := range groups {
-			meta.SetStatusCondition(&og.Status.Conditions, cond)
-			_, err = a.client.OperatorsV1().OperatorGroups(namespace.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
-			if err != nil {
-				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", og.GetName(), cond, err.Error())
-			}
-		}
-	}
-
 	for _, group := range operatorGroupList {
 		namespaceSet := resolver.NewNamespaceSet(group.Status.Namespaces)
 

staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go

@@ -4543,11 +4543,6 @@ func TestOperatorGroupConditions(t *testing.T) {
 	clockFake := utilclock.NewFakeClock(time.Date(2006, time.January, 2, 15, 4, 5, 0, time.FixedZone("MST", -7*3600)))
 
 	operatorNamespace := "operator-ns"
-	opNamespace := &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: operatorNamespace,
-		},
-	}
 	serviceAccount := serviceAccount("sa", operatorNamespace)
 
 	type initial struct {
@@ -4665,7 +4660,7 @@ func TestOperatorGroupConditions(t *testing.T) {
 							UID:       "cdc9643e-7c52-4f7c-ae75-28ccb6aec97d",
 						},
 						Spec: v1.OperatorGroupSpec{
-							TargetNamespaces: []string{operatorNamespace},
+							TargetNamespaces: []string{operatorNamespace, "some-namespace"},
 						},
 					},
 				},
@@ -4720,20 +4715,6 @@ func TestOperatorGroupConditions(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			// wait on operator group updated status to be in the cache
-			err = wait.PollImmediate(1*time.Millisecond, 5*time.Second, func() (bool, error) {
-				og, err := op.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(tt.initial.operatorGroup.GetName())
-				if err != nil || og == nil {
-					return false, err
-				}
-				return true, nil
-			})
-			require.NoError(t, err)
-
-			// sync namespace
-			err = op.syncNamespace(opNamespace)
-			require.NoError(t, err)
-
 			operatorGroup, err := op.client.OperatorsV1().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(context.TODO(), tt.initial.operatorGroup.GetName(), metav1.GetOptions{})
 			require.NoError(t, err)
 			assert.Equal(t, len(tt.expectedConditions), len(operatorGroup.Status.Conditions))

staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go

@@ -13,6 +13,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	meta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/errors"
@@ -64,8 +65,51 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 		"namespace":     op.GetNamespace(),
 	})
 
+	// Query OG in this namespace
+	groups, err := a.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(op.GetNamespace()).List(labels.Everything())
+	if err != nil {
+		logger.WithError(err).Warnf("failed to list OperatorGroups in the namespace")
+	}
+
+	// Check if there is a stale multiple OG condition and clear it if existed.
+	if len(groups) == 1 {
+		og := groups[0]
+		if c := meta.FindStatusCondition(og.Status.Conditions, v1.MutlipleOperatorGroupCondition); c != nil {
+			meta.RemoveStatusCondition(&og.Status.Conditions, v1.MutlipleOperatorGroupCondition)
+			if og.GetName() == op.GetName() {
+				meta.RemoveStatusCondition(&op.Status.Conditions, v1.MutlipleOperatorGroupCondition)
+			}
+			_, err = a.client.OperatorsV1().OperatorGroups(op.GetNamespace()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
+			if err != nil {
+				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", og.GetName(), c, err.Error())
+			}
+		}
+	} else if len(groups) > 1 {
+		// Add to all OG's status conditions to indicate they're multiple OGs in the
+		// same namespace which is not allowed.
+		cond := metav1.Condition{
+			Type:    v1.MutlipleOperatorGroupCondition,
+			Status:  metav1.ConditionTrue,
+			Reason:  v1.MultipleOperatorGroupsReason,
+			Message: "Multiple OperatorGroup found in the same namespace",
+		}
+		for _, og := range groups {
+			if c := meta.FindStatusCondition(og.Status.Conditions, v1.MutlipleOperatorGroupCondition); c != nil {
+				continue
+			}
+			meta.SetStatusCondition(&og.Status.Conditions, cond)
+			if og.GetName() == op.GetName() {
+				meta.SetStatusCondition(&op.Status.Conditions, cond)
+			}
+			_, err = a.client.OperatorsV1().OperatorGroups(op.GetNamespace()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
+			if err != nil {
+				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", og.GetName(), cond, err.Error())
+			}
+		}
+	}
+
 	previousRef := op.Status.ServiceAccountRef.DeepCopy()
-	op, err := a.serviceAccountSyncer.SyncOperatorGroup(op)
+	op, err = a.serviceAccountSyncer.SyncOperatorGroup(op)
 	if err != nil {
 		logger.Errorf("error updating service account - %v", err)
 		return err
@@ -109,6 +153,7 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
 		op.Status = v1.OperatorGroupStatus{
 			Namespaces:  targetNamespaces,
 			LastUpdated: a.now(),
+			Conditions:  op.Status.Conditions,
 		}
 
 		if _, err = a.client.OperatorsV1().OperatorGroups(op.GetNamespace()).UpdateStatus(context.TODO(), op, metav1.UpdateOptions{}); err != nil && !k8serrors.IsNotFound(err) {