(fix)InstallPlan: Do not tranisition IP to failed on OG/SA failure

[anik120]

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

[anik120]

Note: Removed the e2e test cases that checked if the InstallPlan was being transitioned to the Failed state, instead of modifying them to checked if not state transition was happening, and if condition Installed:False condition is present because the unit test cases are doing the exact same thing, making the e2e test cases superfluous.

[benluddy]

/approve

[hasbro17]

We should probably add a unit test case to test this path and check that this holds, i.e pass an IP with the condtionInstalling=false and reason InstallPlanInstalledCheckFailed and check that the condition gets reset with an OG present.

[anik120]

Added an e2e test for this instead since an e2e test feels more natural for this.

[hasbro17]

Before we create the OG to trigger the removal of the Installed/InstallCheckFailed condition, shouldn't we check if it's present before a valid OG is present?
Since we essentially want to test for the change of that condition.

[anik120]

That's a good point, added a check to see if the InstallPlan has the expected condition/message before it is being cleared up.

[hasbro17]

Is this necessary?
I think when we get the IP above with the phase check, it's going to stall (poll 20 mins) until it finds the IP in phase Installing.

fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseInstalling))

operator-lifecycle-manager/test/e2e/installplan_e2e_test.go

Lines 4052 to 4058 in 9723d5d

	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
	fetchedInstallPlan, err = c.OperatorsV1alpha1().InstallPlans(namespace).Get(context.TODO(), name, metav1.GetOptions{})
	if err != nil || fetchedInstallPlan == nil {
	return false, err
	}
	return checkPhase(fetchedInstallPlan), nil

So the fetched IP will always be in phase Installing at this point.

[anik120]

Not a big fan of the buildInstallPlanPhaseCheckFunc, I think it's an artifact of the pre gikgo test framework we use now. fetchInstallPlan should do one thing, i.e fetch the IP, and the checking of the phase is a separate task. But looks like we're using it in a lot of places, so I'm okay with refactoring the tests later on, and leaving this check out of here for now.

[hasbro17]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, benluddy, hasbro17

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [benluddy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment