Prevent OLM from creating InstallPlans when bundle unpack fails

[m1kola]

Prerequisites:

• New condition types api#282
• Updates github.com/operator-framework/api #2954
• Moves bundle unpack timeout into OperatorGroup #2952

---

Description of the change:

This PR changes how InstallPlans are being created.

Motivation for the change:

Today OLM creates InstallPlans even if bundle images are not available for some reason. For example, catalog source contains references to an unreachable registry.

Consider this scenario:

• User installs an operator and enables auto upgrades
• Catalog source receives an update which references unreachable bundle images (e.g. pre-release from a private registry)
• OLM creates an InstallPlan for an upgrade which features unreachable bundle images
• At this point upgrade is stuck since the cluster can not pull bundle images
• Catalog source receives a corrective update which contains only reachable bundle images

Workaround: to resolve this issue users have to manually remove a failed InstallPlan. After that OLM is expected to create a new InstallPlan based on the latest state of catalog sources.

The goal: We would like to find a solution which prevents InstallPlan from being created with unreachable images in the first place. This eliminates the need to manually delete InstallPlan in scenarios like the one described above.

Architectural changes:

Before the change:

• On a Namespace reconciliation (syncResolvingNamespace) OLM creates an InstallPlan if required (e.g. new sub is created or a new update in a catalog is available for an existing sub)
• On InstallPlan reconciliation (syncInstallPlans) OLM creates a Job to unpack a relevant bundle.
  • On unsuccessful job unpacking OLM reports failures as InstallPlan conditions

After this change:

• On a Namespace reconciliation (syncResolvingNamespace):
  • OLM first creates a Job to unpack a relevant bundle and waits for the job to sucessfully complete
    • On unsuccessful job unpacking OLM reports failures as Subscription conditions (set on all subs in the namespace)
  • Only after successfull completion OLM creates an InstallPlan

Testing remarks:

To reproduce:

• Create a test cluster and install OLM (make run-local from this repo can be helpful)
• Create a catalog source:

  apiVersion: operators.coreos.com/v1alpha1
  kind: CatalogSource
  metadata:
    name: alex-operators
    namespace: olm
  spec:
    image: quay.io/agreene/index:unreachable-bundle-upgrade
    displayName: Alex Operators
    priority: -100
    publisher: Alex
    sourceType: grpc
    updateStrategy:
      registryPoll:
        interval: 10m0s

• And create a subscription:

  apiVersion: operators.coreos.com/v1alpha1
  kind: Subscription
  metadata:
    name: quay
    namespace: operators
  spec:
    channel: unavailable-image-upgrade
    installPlanApproval: Automatic
    name: quay
    source: alex-operators
    sourceNamespace: olm
    startingCSV: quay-operator.v3.8.3

In master - you will see two InstallPlans: one for quay-operator.v3.8.3 (successfull install) and one for quay-operator.v3.8.4 which will eventually (10min by default) report failure to unpack in bundle lookup conditions.

In this branch you will see only one InstallPlan for quay-operator.v3.8.3 (successfull install). Eventually Subscription will get a condition reporting failure to unpack (SubscriptionBundleUnpackFailed). During unpacking it should report UnpackingInProgress.

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Bug fixes are accompanied by regression test(s)
• e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
• tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
• Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
• Docs updated or added to /doc
• Commit messages sensible and descriptive
• Tests marked as [FLAKE] are truly flaky and have an issue
• Code is properly formatted

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[m1kola]

This is a big PR and I think I can't split it any further into smaller mergable PRs.

it mostly consists of moving code around with minimal changes (e.g. from syncInstallPlans into syncResolvingNamespace) and updating unit & e2e test.

I would suggest going commit by commit starting with "Changes how InstallPlans are being created". And reviewing removed code first will hopefully help to understand what was moved & modified.

Description of the PR provides some context and steps on how to reproduce, so make sure to read it as well.

[tmshort]

The actual code is not that large; it's the tests...

[m1kola]

/cc @awgreene @perdasilva @ankitathomas

[tmshort]

/lgtm

[m1kola]

I updated to address feedback.

Here is the short summary of the diff since the last push which (diff can be found here). It is arguably easier to comparae modified files to the master still.

• Updated test/e2e/fail_forward_e2e_test.go as per descusion here: reverted (with necessary changes) to having failed InstallPlan.
• Updated test/e2e/subscription_e2e_test.go to add extra coverage to show how OLM auto-recovers as per this. Initially I wanted it to be a separate PR (Extra E2E coverage for #2942 #2961) but to address fail forward I needed extra test data file which I already had in that separate PR. So I squashed them together here.
• Rearanged test data in test/e2e/testdata/fail-forward/ for above two to work.

No changes to the controller itself. Only tests.

I hate large PRs like this, but changes changes in test/e2e/fail_forward_e2e_test.go were necessary to ensure that we do not merge something which fails to catch a regression in fail forward feature. And it dragged other things in.

@tmshort @dtfranz @pgodowski @ankitathomas @perdasilva please take another look.

[perdasilva]

lgtm - excellent work!! users will be pleased ^^

[perdasilva]

Suggested change

	// which means bundle lookup has failed and subscriptions need to be updated
	// which means bundle lookup has failed and subscriptions needs to be updated

[perdasilva]

lgtm - excellent work!! users will be pleased ^^

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: m1kola, perdasilva, tmshort

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 [perdasilva]

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

[perdasilva]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: m1kola, perdasilva, tmshort

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 [perdasilva]

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