Extend Step to surface whether a manifest is optional

[fgiloux]

Signed-off-by: Frederic Giloux [email protected]

Description of the change:

This pull request adds some logic for avoiding failure of the InstallPlan when a manifest provided by a bundle cannot be installed and has been flagged as optional through a bundle property. Here is the enhancement proposal

This PR relies on two other PRs for the api and the operator-registry repositories. https://github.com/operator-framework/api/pull/209 https://github.com/operator-framework/operator-registry/pull/893

I was not too sure how to proceed in such a case and have created the PRs at roughly the same time. It also means that for the code in this PR to work it needs to have the code of the other PRs available. For my tests I used "replace" pointing to a local repository in go.mod

Motivation for the change:

VerticalPodAutoscaler, ServiceMonitors for instances are custom resources and may not be available on all clusters. Operator authors may want to take benefit of these mechanisms for clusters having them but not having the installation fail on clusters without them as they may be seen as nice to have. This is what allows this PR.

See the enhancement proposal for details.

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

[openshift-ci[bot]]

Hi @fgiloux. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[timflannagan]

I was not too sure how to proceed in such a case and have created the PRs at roughly the same time. It also means that for the code in this PR to work it needs to have the code of the other PRs available. For my tests I used "replace" pointing to a local repository in go.mod

You can try and cut a release in your API fork and point the go.mod to your fork using the replace directive, and we can make a TODO comment that we remove that replace pin before merging this PR.

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fgiloux To complete the pull request process, please assign perdasilva after the PR has been reviewed. You can assign the PR to them by writing /assign @perdasilva in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[fgiloux]

/test unit

[openshift-ci[bot]]

@fgiloux: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test unit

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fgiloux]

Can we separate the go.mod + vendor changes into their own commit, so viewing the full diff is a bit easier? One of these days we'll remove the vendor directory entirely from OLM, so this wouldn't be needed.

I have separated the commits and done a rebase

[fgiloux]

Hi @timflannagan any further comment on this pull request?

[timflannagan]

@fgiloux I'll try and get back to this sometime this week but I'm fairly tied up today. It looks like this PR is failing some of the linting and generation checks? You may need to fix the static validation errors locally and push up the results to make the checks happy.

[fgiloux]

@timflannagan after the API rebase we now have: the API depending on k8s.io/api v0.23.0 whereas OLM was on k8s.io/api v0.22.2 and breaks when the dependency is bumped. This was introduced by: https://github.com/operator-framework/api/commit/12a70c0e4176b7ec7922e2ce3a6b3c94b4f3062d How do we move forward?

[timflannagan]

@fgiloux It looks like the CI failures are complaining about various breaking changes that happen in the v0.23.x kube dependencies. We can either tackle bumping OLM's kube dependencies, and making the requisite changes to the codebase in another PR, or update the root go.mod and utilize the replace stanza to ensure we're using the v0.22.x kube dependencies. WYDT?

[fgiloux]

It looks like the CI failures are complaining about various breaking changes that happen in the v0.23.x kube dependencies.

Yes it is what I was saying earlier.

We can either tackle bumping OLM's kube dependencies, and making the requisite changes to the codebase in another PR, or update the root go.mod and utilize the replace stanza to ensure we're using the v0.22.x kube dependencies. WYDT?

I think that bumping OLM dependencies is required by any way, not just for this PR but for anything that needs to import a newer version of the API and for compatibility with newer Kubernetes versions. I will open an issue and start looking at it.

[timflannagan]

/ok-to-test

[fgiloux]

Hi @timflannagan based on o-f api v0.13.0 all e2e tests passed except 1. I ran it locally and it was successful. One thing is that I had to downgrade grpc version to v1.40.0 in operator-registry due to the issue you identified. Can you please take a look?

[fgiloux]

@timflannagan @joelanford could you please take a look? I have rerun the failing e2e tests locally. "can satisfy an unassociated ClusterServiceVersion's non-ownership requirement" was successful. The other two were not: "should surface components in its status" and "should automatically adopt components". I cloned the master branch and run the same tests without my modifications with the same results.

This latest version applies changes requested by Joe:

• the key for identifying an optional manifest is now: group/kind/namespace/name. It works around the issue that the filename may not be available. This also means that no change to operator-registry is required anymore. An example of a property file:

$ cat bundle/metadata/properties.yaml 
properties:
- type: 'olm.manifests.optional'
  value:
    manifests:
    - 'monitoring.coreos.com/PrometheusRule//rule'

Note that for a cluster scoped resource or a resource where the namespace is not specified the part of the key should be left empty, hence //. The key is case sensitive.

[fgiloux]

pushed a rebase and a fix for field and var names in unit tests that were not reflecting the new approach

[fgiloux]

Created a PR to update the enhancement proposal with the new approach

[openshift-ci[bot]]

@fgiloux: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[perdasilva]

closing as we we'll likely want to put this effort into v1 instead