From 64cf2683c97af9bea3f7d9ea935ea30690024ff4 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Mon, 2 Aug 2021 16:10:54 -0400 Subject: [PATCH] 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 --- .../pkg/controller/operators/olm/operator.go | 36 -------------- .../controller/operators/olm/operator_test.go | 21 +-------- .../controller/operators/olm/operatorgroup.go | 47 ++++++++++++++++++- .../pkg/controller/operators/olm/operator.go | 36 -------------- .../controller/operators/olm/operatorgroup.go | 47 ++++++++++++++++++- 5 files changed, 93 insertions(+), 94 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go index f561ea30dd..94d3339f50 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go +++ b/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) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go index 8c7a573ed5..5f6eae41a4 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go +++ b/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)) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go b/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go index 2c92e2e64e..eb9139d89a 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go +++ b/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) { diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go index f561ea30dd..94d3339f50 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go +++ b/vendor/github.com/operator-framework/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) diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go index 2c92e2e64e..eb9139d89a 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/operatorgroup.go +++ b/vendor/github.com/operator-framework/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) {