Repository: operator-framework/operator-lifecycle-manager (go)

Issue:
invalid operator group - no operator group found that is managing this namespace
Install plan fails with the following error:

`   Message:               invalid operator group - no operator group found that is managing this namespace      
`

I got an operator group for the namespace `istio-system`:

```
Name:         operator-group
Namespace:    istio-system
Labels:       gitops.jenkins-x.io/pipeline=namespaces
Annotations:  meta.helm.sh/release-name: istio-system
API Version:  operators.coreos.com/v1
Kind:         OperatorGroup
Metadata:
  Creation Timestamp:  2021-06-15T07:58:41Z
  Generation:          1
  Managed Fields:
    API Version:  operators.coreos.com/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
          f:meta.helm.sh/release-name:
        f:labels:
          .:
          f:gitops.jenkins-x.io/pipeline:
      f:spec:
        .:
        f:targetNamespaces:
          .:
          v:"istio-system":
    Manager:      kubectl
    Operation:    Update
    Time:         2021-06-15T07:58:41Z
    API Version:  operators.coreos.com/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:lastUpdated:
        f:namespaces:
          .:
          v:"istio-system":
    Manager:         olm
    Operation:       Update
    Time:            2021-06-15T08:12:37Z
  Resource Version:  115903077
  UID:               04cba808-9a2e-487b-ba0d-959b1b83cde4
Spec:
  Target Namespaces:
    istio-system
Status:
  Last Updated:  2021-06-15T08:12:37Z
  Namespaces:
    istio-system
Events:  <none>
```

and here the installation plan:

```
Name:         install-lq52r
Namespace:    istio-system
Labels:       operators.coreos.com/kiali.istio-system=
Annotations:  <none>
API Version:  operators.coreos.com/v1alpha1
Kind:         InstallPlan
Metadata:
  Creation Timestamp:  2021-06-15T07:40:37Z
  Generate Name:       install-
  Generation:          1
  Managed Fields:
    API Version:  operators.coreos.com/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:generateName:
        f:ownerReferences:
          .:
          k:{"uid":"91870dd8-17c6-4ba9-a05a-c714eeae3ab8"}:
            .:
            f:apiVersion:
            f:blockOwnerDeletion:
            f:controller:
            f:kind:
            f:name:
            f:uid:
      f:spec:
        .:
        f:approval:
        f:approved:
        f:clusterServiceVersionNames:
        f:generation:
      f:status:
        .:
        f:bundleLookups:
        f:catalogSources:
        f:conditions:
        f:phase:
    Manager:      catalog
    Operation:    Update
    Time:         2021-06-15T07:40:37Z
    API Version:  operators.coreos.com/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          .:
          f:operators.coreos.com/kiali.istio-system:
    Manager:    olm
    Operation:  Update
    Time:       2021-06-15T08:12:44Z
  Owner References:
    API Version:           operators.coreos.com/v1alpha1
    Block Owner Deletion:  false
    Controller:            false
    Kind:                  Subscription
    Name:                  istio-kiali
    UID:                   91870dd8-17c6-4ba9-a05a-c714eeae3ab8
  Resource Version:        115903226
  UID:                     c486e04f-838d-4ed9-9f9c-c94659467283
Spec:
  Approval:  Automatic
  Approved:  true
  Cluster Service Version Names:
    kiali-operator.v1.36.0
  Generation:  1
Status:
  Bundle Lookups:
    Catalog Source Ref:
      Name:       operatorhubio-catalog
      Namespace:  operator-lifecycle-manager
    Conditions:
      Message:   bundle contents have not yet been persisted to installplan status
      Reason:    BundleNotUnpacked
      Status:    True
      Type:      BundleLookupNotPersisted
      Message:   unpack job not yet started
      Reason:    JobNotStarted
      Status:    True
      Type:      BundleLookupPending
    Identifier:  kiali-operator.v1.36.0
    Path:        quay.io/operatorhubio/kiali:v1.36.0
    Properties:  {"properties":[{"type":"olm.gvk","value":{"group":"kiali.io","kind":"Kiali","version":"v1alpha1"}},{"type":"olm.gvk","value":{"group":"monitoring.kiali.io","kind":"MonitoringDashboard","version":"v1alpha1"}},{"type":"olm.package","value":{"packageName":"kiali","version":"1.36.0"}}]}
    Replaces:    kiali-operator.v1.35.0
  Catalog Sources:
  Conditions:
    Last Transition Time:  2021-06-15T07:40:37Z
    Last Update Time:      2021-06-15T07:40:37Z
    Message:               invalid operator group - no operator group found that is managing this namespace
    Reason:                InstallCheckFailed
    Status:                False
    Type:                  Installed
  Phase:                   Failed
Events:                    <none>
```
I am with kubernetes 1.18.2

Pull request description:
(fix)InstallPlan: Do not tranisition IP to failed on OG/SA failure
**Description of the change:**

In #2077, a new phase `Failed` was introduced for InstallPlans, and failure in
detecting a valid OperatorGroup(OG) or a Service Account(SA) for the namespace
the InstallPlan was being created in would transition the InstallPlan to the
`Failed` state, i.e failure to detected these resources when the InstallPlan was
reconciled the first time was considered a permanant failure. This is a regression
from the previous behavior of InstallPlans where failure to detect OG/SA would
requeue the InstallPlan for reconciliation, so creating the required resources before
the retry limit of the informer queue was reached would transition the InstallPlan
from the `Installing` phase to the `Complete` phase(unless the bundle unpacking step
failed, in which case #2093 introduced transitioning the InstallPlan to the `Failed`
phase).

This regression introduced oddities for users who has infra built that applies a
set of manifests simultaneously to install an operator that includes a Subscription to
an operator (that creates InstallPlans) along with the required OG/SAs. In those cases,
whenever there was a delay in the reconciliation of the OG/SA, the InstallPlan would
be transitioned to a state of permanant faliure.

This PR:
* Removes the logic that transitioned the InstallPlan to `Failed`. Instead, the
InstallPlan will again be requeued for any reconciliation error.

* Introduces logic to bubble up reconciliation error through the InstallPlan's
status.Conditions, eg:

When no OperatorGroup is detected:

```
conditions:
    - lastTransitionTime: "2021-06-23T18:16:00Z"
      lastUpdateTime: "2021-06-23T18:16:16Z"
      message: attenuated service account query failed - no operator group found that
        is managing this namespace
      reason: InstallCheckFailed
      status: "False"
      type: Installed
```

Then when a valid OperatorGroup is created:

```
conditions:
    - lastTransitionTime: "2021-06-23T18:33:37Z"
      lastUpdateTime: "2021-06-23T18:33:37Z"
      status: "True"
      type: Installed
```

**Reviewer Checklist**
- [ ] Implementation matches the proposed design, or proposal is updated to match implementation
- [ ] Sufficient unit test coverage
- [ ] Sufficient end-to-end test coverage
- [ ] Docs updated or added to `/doc`
- [ ] Commit messages sensible and descriptive


Closes #2207 


Patch:
diff --git a/deploy/chart/templates/0000_90_olm_01-prometheus-rule.yaml b/deploy/chart/templates/0000_90_olm_01-prometheus-rule.yaml
index 18b7afba4..0d5001a16 100644
--- a/deploy/chart/templates/0000_90_olm_01-prometheus-rule.yaml
+++ b/deploy/chart/templates/0000_90_olm_01-prometheus-rule.yaml
@@ -30,7 +30,7 @@ spec:
   - name: olm.installplan.rules
     rules:
     - alert: InstallPlanStepAppliedWithWarnings
-      expr: sum(increase(installplan_warnings_total[5m])) > 0
+      expr: sum(sum_over_time(installplan_warnings_total[5m])) > 0
       labels:
         severity: warning
       annotations:
diff --git a/doc/design/architecture.md b/doc/design/architecture.md
index aa1511b76..b5cd283e0 100644
--- a/doc/design/architecture.md
+++ b/doc/design/architecture.md
@@ -102,11 +102,12 @@ None --> Planning +------>------->------> Installing +---> Complete
 | Phase            | Description                                                                                    |
 |------------------|------------------------------------------------------------------------------------------------|
 | None             | initial phase, once seen by the Operator, it is immediately transitioned to `Planning`         |
-| Planning         | dependencies between resources are being resolved, to be stored in the InstallPlan `Status` |
+| Planning         | dependencies between resources are being resolved, to be stored in the InstallPlan `Status`    |
 | RequiresApproval | occurs when using manual approval, will not transition phase until `approved` field is true    |
-| Installing       | resolved resources in the InstallPlan `Status` block are being created                      |
+| Installing       | waiting for reconciliation of required resource(OG/SA etc), or resolved resources in the 
+                     InstallPlan `Status` block are being created                                                   |
 | Complete         | all resolved resources in the `Status` block exist                                             |
-| Failed           | occurs when resources fail to install or there is an invalid OperatorGroup                     |
+| Failed           | occurs when resources fail to install or when bundle unpacking fails                           |
 
 ### Subscription Control Loop
 
diff --git a/doc/design/building-your-csv.md b/doc/design/building-your-csv.md
index 277bdf466..7da59ee4f 100644
--- a/doc/design/building-your-csv.md
+++ b/doc/design/building-your-csv.md
@@ -314,7 +314,7 @@ Ensure that the `serviceAccountName` used in the `deployment` spec matches one o
 
 Multiple Roles should be described to reduce the scope of any actions needed containers that the Operator may run on the cluster. For example, if you have a component that generates a TLS Secret upon start up, a Role that allows `create` but not `list` on Secrets is more secure than using a single all-powerful Service Account.
 
-It is also possible to set custom labels for Deployment in addition to the system labels set by the OLM. This labels should be present in the `label` fields of the `deployments` section.  
+It is also possible to set custom labels for Deployment in addition to the system labels set by the OLM. This labels should be present in the `labels` fields of the `deployments` section.  
 
 Here’s a full example:
 
@@ -323,7 +323,7 @@ Here’s a full example:
     spec:
       deployments:
         - name: example-operator
-          label:
+          labels:
             application: example-operator
             technology: general
           spec:
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index 2a48ed3cf..efd1685b9 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/pkg/controller/operators/catalog/operator.go
@@ -1387,28 +1387,36 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 
 	querier := o.serviceAccountQuerier.NamespaceQuerier(plan.GetNamespace())
 	ref, err := querier()
-	if err != nil {
-
-		// Retry sync if non-fatal error
-		if !scoped.IsOperatorGroupError(err) {
-			syncError = fmt.Errorf("attenuated service account query failed - %v", err)
-			return
-		}
-
-		// Mark the InstallPlan as failed for a fatal Operator Group related error
-		logger.Infof("attenuated service account query failed - %v", err)
-		ipFailError := fmt.Errorf("invalid operator group - %v", err)
 
-		if err := o.transitionInstallPlanToFailed(plan, logger, v1alpha1.InstallPlanReasonInstallCheckFailed, ipFailError.Error()); err != nil {
-			// retry for failure to update status
+	out := plan.DeepCopy()
+	if err != nil {
+		// Set status condition/message and retry sync if any error
+		ipFailError := fmt.Errorf("attenuated service account query failed - %v", err)
+		logger.Infof(ipFailError.Error())
+		_, err := o.setInstallPlanInstalledCond(out, v1alpha1.InstallPlanReasonInstallCheckFailed, err.Error(), logger)
+		if err != nil {
 			syncError = err
 			return
 		}
-
-		// Requeue subscription to propagate SubscriptionInstallPlanFailed condtion to subscription
-		o.requeueSubscriptionForInstallPlan(plan, logger)
-
+		syncError = ipFailError
 		return
+	} else {
+		// reset condition/message if it had been set in previous sync. This condition is being reset since any delay in the next steps
+		// (bundle unpacking/plan step errors being retried for a duration) could lead to this condition sticking around, even after
+		// the serviceAccountQuerier returns no error since the error has been resolved (by creating the required resources), which would
+		// be confusing to the user
+
+		// NOTE: this makes the assumption that the InstallPlanInstalledCheckFailed reason is only set in the previous if clause, which is
+		// true in the current iteration of the catalog operator. Any future implementation change that aims at setting the reason as
+		// InstallPlanInstalledCheckFailed must make sure that either this assumption is not breached, or the condition being set elsewhere
+		// is not being unset here unintentionally.
+		if cond := out.Status.GetCondition(v1alpha1.InstallPlanInstalled); cond.Reason == v1alpha1.InstallPlanReasonInstallCheckFailed {
+			plan, err = o.setInstallPlanInstalledCond(out, v1alpha1.InstallPlanConditionReason(corev1.ConditionUnknown), "", logger)
+			if err != nil {
+				syncError = err
+				return
+			}
+		}
 	}
 
 	if ref != nil {
@@ -1553,6 +1561,18 @@ func (o *Operator) requeueSubscriptionForInstallPlan(plan *v1alpha1.InstallPlan,
 	}
 }
 
+func (o *Operator) setInstallPlanInstalledCond(ip *v1alpha1.InstallPlan, reason v1alpha1.InstallPlanConditionReason, message string, logger *logrus.Entry) (*v1alpha1.InstallPlan, error) {
+	now := o.now()
+	ip.Status.SetCondition(v1alpha1.ConditionFailed(v1alpha1.InstallPlanInstalled, reason, message, &now))
+	outIP, err := o.client.OperatorsV1alpha1().InstallPlans(ip.GetNamespace()).UpdateStatus(context.TODO(), ip, metav1.UpdateOptions{})
+	if err != nil {
+		logger = logger.WithField("updateError", err.Error())
+		logger.Errorf("error updating InstallPlan status")
+		return nil, nil
+	}
+	return outIP, nil
+}
+
 type installPlanTransitioner interface {
 	ExecutePlan(*v1alpha1.InstallPlan) error
 }
diff --git a/pkg/controller/registry/reconciler/configmap.go b/pkg/controller/registry/reconciler/configmap.go
index 08c9b4d4b..ae27bbeb4 100644
--- a/pkg/controller/registry/reconciler/configmap.go
+++ b/pkg/controller/registry/reconciler/configmap.go
@@ -101,7 +101,7 @@ func (s *configMapCatalogSourceDecorator) Service() *v1.Service {
 }
 
 func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod {
-	pod := Pod(s.CatalogSource, "configmap-registry-server", image, "", s.Labels(), s.Annotations(), 5, 5)
+	pod := Pod(s.CatalogSource, "configmap-registry-server", image, "", s.Labels(), s.Annotations(), 5, 2)
 	pod.Spec.ServiceAccountName = s.GetName() + ConfigMapServerPostfix
 	pod.Spec.Containers[0].Command = []string{"configmap-server", "-c", s.Spec.ConfigMap, "-n", s.GetNamespace()}
 	ownerutil.AddOwner(pod, s.CatalogSource, false, false)
diff --git a/pkg/controller/registry/reconciler/reconciler.go b/pkg/controller/registry/reconciler/reconciler.go
index ddfd7fb92..4b7b53b88 100644
--- a/pkg/controller/registry/reconciler/reconciler.go
+++ b/pkg/controller/registry/reconciler/reconciler.go
@@ -139,7 +139,6 @@ func Pod(source *v1alpha1.CatalogSource, name string, image string, saName strin
 							},
 						},
 						InitialDelaySeconds: livenessDelay,
-						TimeoutSeconds:      5,
 					},
 					Resources: v1.ResourceRequirements{
 						Requests: v1.ResourceList{


Test patch:
diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
index c5df7e35a..334e2ec19 100644
--- a/pkg/controller/operators/catalog/operator_test.go
+++ b/pkg/controller/operators/catalog/operator_test.go
@@ -166,12 +166,12 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 	)
 
 	tests := []struct {
-		testName      string
-		err           error
-		in            *v1alpha1.InstallPlan
-		expectedPhase v1alpha1.InstallPlanPhase
-
-		clientObjs []runtime.Object
+		testName          string
+		err               error
+		in                *v1alpha1.InstallPlan
+		expectedPhase     v1alpha1.InstallPlanPhase
+		expectedCondition *v1alpha1.InstallPlanCondition
+		clientObjs        []runtime.Object
 	}{
 		{
 			testName:      "NoStatus",
@@ -180,18 +180,22 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 			in:            installPlan("p", namespace, v1alpha1.InstallPlanPhaseNone),
 		},
 		{
-			// This checks that an installplan is marked as failed when no operatorgroup is present
+			// This checks that an installplan's status.Condition contains a condition with error message when no operatorgroup is present
 			testName:      "HasSteps/NoOperatorGroup",
-			err:           nil,
-			expectedPhase: v1alpha1.InstallPlanPhaseFailed,
-			in:            ipWithSteps,
+			err:           fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
+			expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
+			expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "no operator group found that is managing this namespace"},
+			in: ipWithSteps,
 		},
 		{
-			// This checks that an installplan is marked as failed when multiple operator groups are present for the same namespace
+			// This checks that an installplan's status.Condition contains a condition with error message when multiple operator groups are present for the same namespace
 			testName:      "HasSteps/TooManyOperatorGroups",
-			err:           nil,
-			expectedPhase: v1alpha1.InstallPlanPhaseFailed,
+			err:           fmt.Errorf("attenuated service account query failed - more than one operator group(s) are managing this namespace count=2"),
+			expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
 			in:            ipWithSteps,
+			expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "more than one operator group(s) are managing this namespace count=2"},
 			clientObjs: []runtime.Object{
 				operatorGroup("og1", "sa", namespace,
 					&corev1.ObjectReference{
@@ -208,11 +212,13 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 			},
 		},
 		{
-			// This checks that an installplan is marked as failed when no service account is synced for the operator group, i.e the service account ref doesn't exist
+			// This checks that an installplan's status.Condition contains a condition with error message when no service account is synced for the operator group, i.e the service account ref doesn't exist
 			testName:      "HasSteps/NonExistentServiceAccount",
-			err:           nil,
-			expectedPhase: v1alpha1.InstallPlanPhaseFailed,
-			in:            ipWithSteps,
+			err:           fmt.Errorf("attenuated service account query failed - please make sure the service account exists. sa=sa1 operatorgroup=ns/og"),
+			expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
+			expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "please make sure the service account exists. sa=sa1 operatorgroup=ns/og"},
+			in: ipWithSteps,
 			clientObjs: []runtime.Object{
 				operatorGroup("og", "sa1", namespace, nil),
 			},
@@ -236,6 +242,11 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 			require.NoError(t, err)
 
 			require.Equal(t, tt.expectedPhase, ip.Status.Phase)
+
+			if tt.expectedCondition != nil {
+				require.True(t, hasExpectedCondition(ip, *tt.expectedCondition))
+			}
+
 		})
 	}
 }
@@ -1830,3 +1841,12 @@ func operatorGroup(ogName, saName, namespace string, saRef *corev1.ObjectReferen
 		},
 	}
 }
+
+func hasExpectedCondition(ip *v1alpha1.InstallPlan, expectedCondition v1alpha1.InstallPlanCondition) bool {
+	for _, cond := range ip.Status.Conditions {
+		if cond.Type == expectedCondition.Type && cond.Message == expectedCondition.Message && cond.Status == expectedCondition.Status {
+			return true
+		}
+	}
+	return false
+}
diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go
index ad022026b..d52495678 100644
--- a/test/e2e/installplan_e2e_test.go
+++ b/test/e2e/installplan_e2e_test.go
@@ -3145,160 +3145,99 @@ var _ = Describe("Install Plan", func() {
 		require.Equal(GinkgoT(), 1, len(ips.Items), "If this test fails it should be taken seriously and not treated as a flake. \n%v", ips.Items)
 	})
 
-	It("should fail an InstallPlan when no OperatorGroup is present", func() {
+	When("an InstallPlan is created with no valid OperatorGroup present", func() {
 
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
+		var (
+			c               operatorclient.ClientInterface
+			crc             versioned.Interface
+			installPlanName string
+			ns              *corev1.Namespace
+		)
+		BeforeEach(func() {
+			c = newKubeClient()
+			crc = newCRClient()
 
-		c := newKubeClient()
-		crc := newCRClient()
+			ns = &corev1.Namespace{}
+			ns.SetName(genName("ns-"))
 
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
+			// Create a namespace
+			Eventually(func() error {
+				return ctx.Ctx().Client().Create(context.Background(), ns)
+			}, timeout, interval).Should(Succeed(), "could not create Namespace")
 
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err := c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
+			// Create InstallPlan
+			installPlanName = "ip"
+			ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseInstalling)
+			outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
 			Expect(err).NotTo(HaveOccurred())
-		}()
-
-		// Create InstallPlan
-		installPlanName := "ip"
-		ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
-
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).NotTo(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with phase %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
-	})
+			Expect(outIP).NotTo(BeNil())
 
-	It("should fail an InstallPlan when multiple OperatorGroups are present", func() {
-
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
-
-		c := newKubeClient()
-		crc := newCRClient()
-
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err = c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
-			Expect(err).ToNot(HaveOccurred())
-		}()
-
-		// Create 2 operatorgroups in the same namespace
-		og1 := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og1",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces: []string{ns.GetName()},
-			},
-		}
-		og2 := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og2",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces: []string{ns.GetName()},
-			},
-		}
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og1, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og2, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-
-		installPlanName := "ip"
-		ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
+			// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
+			// InstallPlans without any steps or bundle lookups
+			outIP.Status = ip.Status
+			_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
+			Expect(err).NotTo(HaveOccurred())
+		})
 
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
+		It("should clear clear up the condition in the InstallPlan status that contains an error message when a valid OperatorGroup is created", func() {
 
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).ToNot(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
-	})
+			// first check that a condition with a message exists
+			fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseInstalling))
+			Expect(err).NotTo(HaveOccurred())
+			Expect(fetchedInstallPlan).NotTo(BeNil())
+			cond := v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
+				Message: "no operator group found that is managing this namespace"}
+			Expect(fetchedInstallPlan.Status.Phase).To(Equal(v1alpha1.InstallPlanPhaseInstalling))
+			Expect(hasCondition(fetchedInstallPlan, cond)).To(BeTrue())
 
-	It("should fail an InstallPlan when an OperatorGroup specifies an unsynced ServiceAccount with no ServiceAccountRef", func() {
+			// Create an operatorgroup for the same namespace
+			og := &operatorsv1.OperatorGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "og",
+					Namespace: ns.GetName(),
+				},
+				Spec: operatorsv1.OperatorGroupSpec{
+					TargetNamespaces: []string{ns.GetName()},
+				},
+			}
+			Eventually(func() error {
+				return ctx.Ctx().Client().Create(context.Background(), og)
+			}, timeout, interval).Should(Succeed(), "could not create OperatorGroup")
 
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
+			// Wait for the OperatorGroup to be synced
+			Eventually(
+				func() ([]string, error) {
+					err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(og), og)
+					ctx.Ctx().Logf("Waiting for OperatorGroup(%v) to be synced with status.namespaces: %v", og.Name, og.Status.Namespaces)
+					return og.Status.Namespaces, err
+				},
+				1*time.Minute,
+				interval,
+			).Should(ContainElement(ns.GetName()))
 
-		c := newKubeClient()
-		crc := newCRClient()
+			// check that the condition has been cleared up
+			Eventually(func() (bool, error) {
+				fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseInstalling))
+				if err != nil {
+					return false, err
+				}
+				if fetchedInstallPlan == nil {
+					return false, err
+				}
+				if hasCondition(fetchedInstallPlan, cond) {
+					return false, nil
+				}
+				return true, nil
+			}).Should(BeTrue())
+		})
 
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err = c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
+		AfterEach(func() {
+			err := c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), metav1.DeleteOptions{})
 			Expect(err).ToNot(HaveOccurred())
-		}()
-
-		// OperatorGroup with serviceaccount specified
-		og := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces:   []string{ns.GetName()},
-				ServiceAccountName: "foobar",
-			},
-		}
-		og, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
-		Expect(og).NotTo(BeNil())
-
-		// Update OperatorGroup status with namespace but no service account reference
-		now := metav1.Now()
-		og.Status = operatorsv1.OperatorGroupStatus{
-			Namespaces:        []string{ns.GetName()},
-			ServiceAccountRef: nil,
-			LastUpdated:       &now,
-		}
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
-		Expect(err).ToNot(HaveOccurred())
 
-		installPlanName := "ip"
-		ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
-
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).NotTo(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
+			err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Delete(context.TODO(), installPlanName, metav1.DeleteOptions{})
+			Expect(err).NotTo(HaveOccurred())
+		})
 	})
 
 	When("waiting on the bundle unpacking job", func() {
@@ -3345,7 +3284,7 @@ var _ = Describe("Install Plan", func() {
 				return ctx.Ctx().Client().Create(context.Background(), og)
 			}, timeout, interval).Should(Succeed(), "could not create OperatorGroup")
 
-			// Wait for the OperatorGroup to be synced so the InstallPlan doesn't fail due to an invalid OperatorGroup
+			// Wait for the OperatorGroup to be synced so the InstallPlan doesn't have to be resynced due to an invalid OperatorGroup
 			Eventually(
 				func() ([]string, error) {
 					err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(og), og)
@@ -3641,7 +3580,7 @@ var _ = Describe("Install Plan", func() {
 
 		// Wait for the OperatorGroup to be synced and have a status.ServiceAccountRef
 		// before moving on. Otherwise the catalog operator treats it as an invalid OperatorGroup
-		// and fails the InstallPlan
+		// and the InstallPlan is resynced
 		Eventually(func() (*corev1.ObjectReference, error) {
 			outOG, err := crc.OperatorsV1().OperatorGroups(ns.GetName()).Get(context.TODO(), og.Name, metav1.GetOptions{})
 			if err != nil {
@@ -3897,7 +3836,7 @@ var _ = Describe("Install Plan", func() {
 
 		// Wait for the OperatorGroup to be synced and have a status.ServiceAccountRef
 		// before moving on. Otherwise the catalog operator treats it as an invalid OperatorGroup
-		// and fails the InstallPlan
+		// and the InstallPlan is resynced
 		Eventually(func() (*corev1.ObjectReference, error) {
 			outOG, err := crc.OperatorsV1().OperatorGroups(ns.GetName()).Get(context.TODO(), og.Name, metav1.GetOptions{})
 			if err != nil {
@@ -4072,7 +4011,7 @@ func validateCRDVersions(t GinkgoTInterface, c operatorclient.ClientInterface, n
 
 func buildInstallPlanPhaseCheckFunc(phases ...operatorsv1alpha1.InstallPlanPhase) checkInstallPlanFunc {
 	return func(fip *operatorsv1alpha1.InstallPlan) bool {
-		ctx.Ctx().Logf("installplan is %s", fip.Status.Phase)
+		ctx.Ctx().Logf("installplan %v is in phase %v", fip.GetName(), fip.Status.Phase)
 		satisfiesAny := false
 		for _, phase := range phases {
 			satisfiesAny = satisfiesAny || fip.Status.Phase == phase
@@ -4329,3 +4268,12 @@ func newInstallPlanWithDummySteps(name, namespace string, phase operatorsv1alpha
 		},
 	}
 }
+
+func hasCondition(ip *v1alpha1.InstallPlan, expectedCondition v1alpha1.InstallPlanCondition) bool {
+	for _, cond := range ip.Status.Conditions {
+		if cond.Type == expectedCondition.Type && cond.Message == expectedCondition.Message && cond.Status == expectedCondition.Status {
+			return true
+		}
+	}
+	return false
+}