Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update compound-bundle-constraints enhancement #108

Conversation

tylerslaton
Copy link

Signed-off-by: Tyler Slaton [email protected]

Summary

This is an update to the compound-bundle-constraints EP that is intended to address a couple of things.

  1. Make it clearer what negation does
  2. Update the negation keyword from none to not

Note The change to update none to `not will be addressed in the code directly via other PR's.

This is being done to address the issue outlined in BZ 2034319.

Copy link

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make it even more crisp? E.g. outline specifically what happens in the case where you want to install something that has a negation on something that is already installed. Basically, that this means your operator will still install as long as there is another package that can meet the dependencies. Maybe we should have similar examples explained for any and all

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved
enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved
@benluddy
Copy link

Does renaming none address the root of the confusion around this behavior? This feature as a whole provides a mechanism for specifying how to select candidates for dependency (i.e. solver.Dependency) constraints only. Dependencies -- as opposed to conflicts -- can only ever block the installation of the bundle that declares the dependency (this seems to be what Evan was referring to in #97 (comment)).

I think this is better addressed by clarifying the high-level semantics of this property versus focusing specifically on negation.

@tylerslaton tylerslaton force-pushed the update-compound-constraints-ep branch 2 times, most recently from 2e8332c to bc1f061 Compare January 25, 2022 21:02
@cdjohnson
Copy link

I'm still confused on how bundle authors can take advantage of compound bundle constraints AND the the legacy non-compound bundle constraints simultaneously. This is addressed in the Alternatives section, but not clear if this is required of the spec or not. I think we can just add the new, narrower constraint.

Example: Today, we oftentimes try to use both olm.gvk.required and olm.package.required, which works sufficiently in most cases, unless there are two packages that provide the same GVK. We've run into occasions where two separate third-parties try to OLM-Enable two operators and provide them in separate catalogs, meaning we want to try and constrain our operator to one of the packages that serves a GVK.

Today:

---
schema: olm.bundle
name: foo.v1.0.0
package: foo
properties:
- type: olm.package.required
  value:
    name: etcd
    versionRange: '>=1.0.0'
- type: olm.gvk.required
  value:
    group: etcd.database.coreos.com
    kind: EtcdBackup
    version: v1beta2

Now, we want to leverage the more narrow rule, which we would now provide both formats. Because the new compound constraint is ALSO evaluated, the narrower rule is honored:

schema: olm.bundle
name: foo.v1.0.0
package: foo
properties:
# For all versions of OLM (a package and GVK don't need to be in the same bundle):
- type: olm.package.required
  value:
    name: etcd
    versionRange: '>=1.0.0'
- type: olm.gvk.required
  value:
    group: etcd.database.coreos.com
    kind: EtcdBackup
    version: v1beta2
# Only recognized by new OLM, which limits solutions to a single bundle that provides both the package and gvk.
- type: olm.constraint
  value:
    failureMessage: All are required for Baz because...
    all:
      constraints:
      - failureMessage: Package bar is needed for...
        package:
          name: etcd
          versionRange: '>=1.0.0'
      - failureMessage: GVK EtcdBackup/v1beta2 is needed for...
        gvk:
          group: etcd.database.coreos.com
          version: v1beta2
          kind: EtcdBackup

It might be helpful to call out a best practice for creating semi-interoperable dependencies like this.

@cdjohnson
Copy link

Can you clarify if a bundle provider can specify these constraints in the dependencies.yaml or if it only works in the properties.yaml within the bundle image?

@dinhxuanvu
Copy link
Member

@cdjohnson As you mentioned, for the most use cases, the current existing olm.gvk.required and olm.package.required will be sufficient. However, as a known behavior, you list a required gvk and a required package, it may get resolved into 2 separate dependent operators depending the content of the catalogsources (shared owned API, same packages in multiple catalogsources and etc). The all type will solve this issue by ensuring all of required dependencies underneath all will only get resolved into a single dependent operator if satisfied. So to me that's the main use case for this compound constraint.
This is a new type of dependencies and it is not required or anything like that.

@dinhxuanvu
Copy link
Member

Can you clarify if a bundle provider can specify these constraints in the dependencies.yaml or if it only works in the properties.yaml within the bundle image?

Currently, it should be declared in dependencies.yaml just like other existing dependency types. We want to keep the convention the same even though you can technically add the new type in properties.yaml and OLM will understand it.

@cdjohnson
Copy link

@dinhxuanvu:

...However, as a known behavior, you list a required gvk and a required package, it may get resolved into 2 separate dependent operators depending the content of the catalogsources (shared owned API, same packages in multiple catalogsources and etc). The all type will solve this issue by ensuring all of required dependencies underneath all will only get resolved into a single dependent operator if satisfied. So to me that's the main use case for this compound constraint. This is a new type of dependencies and it is not required or anything like that.

Yes, I understand that. I'm asking if we want to leverage the new constraint AND preserve the old behavior for old OLM resolvers: Is my example is valid? I'd like to tell ALL of our products to follow this example to allow a more exact constraint when possible, and the lesser constraint when not possible.

@cdjohnson
Copy link

@dinhxuanvu

Currently, it should be declared in dependencies.yaml just like other existing dependency types. We want to keep the convention the same even though you can technically add the new type in properties.yaml and OLM will understand it.

Can we add this to the specification, so it's obvious to all readers? It seems like examples showing dependencies.yaml is more relevant to most, rather than the File Based Catalog examples.

@dinhxuanvu
Copy link
Member

@dinhxuanvu:

...However, as a known behavior, you list a required gvk and a required package, it may get resolved into 2 separate dependent operators depending the content of the catalogsources (shared owned API, same packages in multiple catalogsources and etc). The all type will solve this issue by ensuring all of required dependencies underneath all will only get resolved into a single dependent operator if satisfied. So to me that's the main use case for this compound constraint. This is a new type of dependencies and it is not required or anything like that.

Yes, I understand that. I'm asking if we want to leverage the new constraint AND preserve the old behavior for old OLM resolvers: Is my example is valid? I'd like to tell ALL of our products to follow this example to allow a more exact constraint when possible, and the lesser constraint when not possible.

Your example looks valid to me. However, on the second one, it seems you only need the olm.constraint dependency and you can remove the olm.gvk.required and olm.package.required dependencies as they are duplicates. From what I gather, you want a single dependent operator solution for the two requirements so the all type alone is sufficient.

@cdjohnson
Copy link

@dinhxuanvu:

Your example looks valid to me. However, on the second one, it seems you only need the olm.constraint dependency and you can remove the olm.gvk.required and olm.package.required dependencies as they are duplicates. From what I gather, you want a single dependent operator solution for the two requirements so the all type alone is sufficient.

My point, is that I want this same bundle to be able to be installed and the dependencies understood on OpenShift 4.6 through 4.9, which won't have OLM that understands olm.constraint. So, I need to supply BOTH the old and new format simultaneously OR i need to create different bundles with different dependency.yaml files for different versions of OpenShift (OLM), which means I need different catalogs, etc.

@benluddy
Copy link

@cdjohnson It's fine to specify both. There are global invariants on GVK providers, so only one operator can provide any given GVK. If your operator requires A, B, and (A ^ B), it's effectively equivalent to requiring only (A ^ B) for a version of catalog-operator that understands (A ^ B).

@tylerslaton tylerslaton force-pushed the update-compound-constraints-ep branch 2 times, most recently from 135e16e to e3496f6 Compare January 26, 2022 16:25
@cdjohnson
Copy link

There are global invariants on GVK providers, so only one operator can provide any given GVK

This is likely true in how the resolver produces a solution. However, in practice this is not true in the context of the universe (my understanding of "global" which we cannot control) of public catalogs, where anyone can build and publish an OLM-Enabled operator. For example, when submitting an Operator foo to the Red Hat certification program, that program appends -certified to the name of the package. So if that operator was NOT certified AND certified, there are two packages that serve the same GVK. THere are also examples of 3rd parties delivering their own version of an OLM-Enabled operator for the same GVK's.... This is the reason we're trying to provide this constraint, and why I really like this feature.

@benluddy
Copy link

benluddy commented Jan 26, 2022

Yes, exactly right. I was specifically referring to namespace resolution here, because that is what prevents the selection of a solution like { A1 B1 A2 }. If that resolution invariant didn't exist, then it would not be safe to keep the existing dependencies alongside a compound one.

Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work Tyler, some notes aimed at improving document clarity.

enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
enhancements/compound-dependency-selectors.md Outdated Show resolved Hide resolved
@tylerslaton tylerslaton force-pushed the update-compound-constraints-ep branch from e3496f6 to 94f2a52 Compare January 26, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants