enhancements: compound-bundle-constraints #97

estroz · 2021-10-20T00:29:18Z

Signed-off-by: Eric Stroczynski [email protected]

estroz · 2021-10-20T00:29:33Z

/cc @joelanford @kevinrizza @dinhxuanvu

enhancements/compound-bundle-constraints.md

estroz · 2021-10-20T19:49:14Z

/cc @dmesser

enhancements/compound-bundle-constraints.md

joelanford

Overall, looks pretty solid. A couple of comments/questions.

joelanford · 2021-10-21T00:24:39Z

enhancements/compound-bundle-constraints.md

+  - There is no quick way to determine whether an expression is unsatisfiable
+  other than by using a SAT solver, which the resolver uses internally.
+  Compound constraints must be vetted by hand.


Not in scope for this EP, but this is another good use case for making the resolver usable in other contexts (e.g. verifying that a bundle's properties and constraints are not internally unsatisfiable)

joelanford · 2021-10-21T00:44:57Z

enhancements/compound-bundle-constraints.md

+
+### Graduation Criteria
+
+The alpha feature gate will be removed and this feature will be on permanently by OLM v1.0.0.


We probably need to go into a little bit more detail here, and v1.0.0 seems somewhat arbitrary. Why not a v0.y or v1.1+?

Specific areas to address are:

Why are we introducing as tech preview?

What do we hope to learn while this feature is in tech preview and how do we plan to learn it?

What will trigger us to graduate this feature from tech preview to GA?

This decision was arbitrary. Perhaps v0.(y+2) from release is better, since we want feedback on how well this feature tackles the outlined use cases vs. improvements to the spec that result in breaking changes.

enhancements/compound-bundle-constraints.md

ecordell · 2021-11-04T19:06:46Z

enhancements/compound-bundle-constraints.md

+schema: olm.bundle
+name: baz.v1.0.0
+properties:
+- type: olm.none.required


this is interesting, but I think it more matches "conflicts" constraints in the resolver than it does negation.

negation would let you say things like A && B || A && !C etc

I am unclear on what you mean. Negation here intentionally means "use any GVK/package that satisfies other constraints except these GVKs/packages". Are you saying this is not useful, or just that the implementation of negation in the resolver is "conflicts"?

enhancements/compound-bundle-constraints.md

estroz · 2021-11-09T19:13:03Z

Any showstoppers for this getting merged? The plan is to start this feature off in alpha, so if changes to the spec are needed based on the higher level constraint vs. property discussion, they can be made easily and I'll update this EP then.

joelanford · 2021-11-09T19:54:50Z

Any showstoppers for this getting merged? The plan is to start this feature off in alpha, so if changes to the spec are needed based on the higher level constraint vs. property discussion, they can be made easily and I'll update this EP then.

Can you call this out specifically in the EP as a risk or open question, and then describe what the remediation will be?

It seems like this is well-defined enough to begin implementation, but if we're pretty confident that the syntax will change (even slightly), I think we'd do ourselves a favor if we kept the code related to parsing the constraint in a separate branch so it doesn't get merged downstream until we're confident it aligns with the discussion in #91.

Do you think its worthwhile running through another prototype demo based on what exists here to get something concrete in front of folks. I'd be interested in getting this on the agenda of one of our upstream meetings to get community feedback.

@cdjohnson Any comments/questions?

estroz · 2021-11-10T16:47:20Z

Do you think its worthwhile running through another prototype demo based on what exists here to get something concrete in front of folks. I'd be interested in getting this on the agenda of one of our upstream meetings to get community feedback.

Most definitely. Let me update the POC and add that to the agenda for the next OLM upstream meeting.

enhancements/compound-bundle-constraints.md

perdasilva · 2021-11-12T02:14:38Z

One thing that's kind of bother me a little is the interchange b/w property and constraint. E.g. if I just want a simple package level constraint, then that's a property. The same structure however would be re-used under constraint in the nested example. Maybe we should just use property everywhere?

cdjohnson · 2021-11-12T17:59:30Z

Question on Interoperability:

Older versions of OLM (compound constraints are not known) will ignore the new constraint properties and continue to honor the old required properties at the root level.
Newer versions of OLM (compound constraint aware) will honor BOTH the new and old properties.

I think we need a way to signal OLM to ignore the old properties if the new properties are present, or use the fact that there is an olm.all|any|none.required property at the top level to IGNORE the non-constraint properties at the top. It seems like we would want to deprecate the ability specify constraints at the top level and always express them in a compound dependency.

For example, if I wanted to build a Bundle that works on both, I could do something like this:

- type: olm.package.required
  value:
    description:  This is for legacy versions of OLM.
    packageName: bar
    versionRange: '>=1.0.0'
- type: olm.gvk.required
  value:
    description:  This is for legacy versions of OLM.
    group: etcd.database.coreos.com
    kind: EtcdBackup
    version: v1beta2
- type: olm.all.required
  value:
    constraints:
      - type: olm.package.required
        value:
          packageName: bar
          versionRange: '>=1.0.0'
      - type: olm.none.required
        value:
          - type: olm.package.required
            value:
              description: This package provides the same GVK, but I don't work with this one.
              packageName: brokenbar
              versionRange: '>=1.0.0'
      - type: olm.gvk.required
        value:
          description:  This is for legacy versions of OLM.
          group: etcd.database.coreos.com
          kind: EtcdBackup
          version: v1beta2

If the new version of OLM honors BOTH the old and new formats, then the new compound constraints will be ignored in this case.

cdjohnson · 2021-11-12T18:02:13Z

Question on resolver scope:
Is each constraint applied to a single bundle? Or on the entire visible domain of operators (catalog sources and installed bundles)?

Example: I want GVK and Package to be served by the same bundle. Not by two different bundles.

estroz · 2021-11-12T19:27:38Z

One thing that's kind of bother me a little is the interchange b/w property and constraint. E.g. if I just want a simple package level constraint, then that's a property. The same structure however would be re-used under constraint in the nested example. Maybe we should just use property everywhere?

@perdasilva this is being resolved in #91 (comment). All constraints are properties, but not all properties are constraints, hence the distinction.

I think we need a way to signal OLM to ignore the old properties if the new properties are present, or use the fact that there is an olm.all|any|none.required property at the top level to IGNORE the non-constraint properties at the top.

@cdjohnson I think this is a good idea for backwards-compatibility of bundles with OLM. The resolver can infer what to use by the presence of a compound constraint. I will say though, mixing both "legacy" and compound constraints in this way is confusing. I'll call out that this behavior needs to be documented and have warnings logged when this scenario is encountered.

Is each constraint applied to a single bundle? Or on the entire visible domain of operators (catalog sources and installed bundles)?

If I understand what you're asking, the latter. This is the same scope as for existing constraints.

cdjohnson · 2021-11-12T19:53:52Z

@estroz

Is each constraint applied to a single bundle? Or on the entire visible domain of operators (catalog sources and installed bundles)?

If I understand what you're asking, the latter. This is the same scope as for existing constraints.

After re-reading and talking to @dmesser, it appears that each Constraint Root (top level property) must evaluate to true for a single bundle. This wasn't clear to me until I re-read the old resolver EP. Essentially each Tree of constraints is evaluated independently against each bundle. So, by having a Compound Constraint they must evaluate to TRUE for a single bundle. I was incorrectly (I hope) thinking that the compound constraints were applied more broadly. I think it would be helpful to re-describe how the resolver uses this information (one tree at a time).

cdjohnson · 2021-11-12T20:00:25Z

I think we need a way to signal OLM to ignore the old properties if the new properties are present, or use the fact that there is an olm.all|any|none.required property at the top level to IGNORE the non-constraint properties at the top.

@cdjohnson I think this is a good idea for backwards-compatibility of bundles with OLM. The resolver can infer what to use by the presence of a compound constraint. I will say though, mixing both "legacy" and compound constraints in this way is confusing. I'll call out that this behavior needs to be documented and have warnings logged when this scenario is encountered.

@estroz I can think of a few alternatives:

Force catalogs and bundles to introduce OLM-Version-Specific versions of itself. Not a fan of this if we can help it.
Add an additional property to the legacy top-level value to explicitly ignore it by the "new compound-enabled resolver", to make it a bit more explicit.

Example:

- type: olm.package.required
  value:
    description:  This is for legacy versions of OLM.
    packageName: bar
    versionRange: '>=1.0.0'
    ignore: true

Old OLM versions ignore the ignore: true

estroz · 2021-11-12T21:02:40Z

Essentially each Tree of constraints is evaluated independently against each bundle. So, by having a Compound Constraint they must evaluate to TRUE for a single bundle. I was incorrectly (I hope) thinking that the compound constraints were applied more broadly.

Not exactly. A single bundle may never satisfy all constraints because one or more may reference GVKs/a package outside of that bundle's, so the resolver chooses a set of bundles from the universe (cluster) that satisfy all of a bundle's constraints.

I can think of a few alternatives

Thanks, it's worth calling out alternatives. I am not a fan of either approach though. The first makes ignoring top-level constraints in new OLM versions superfluous, since you're breaking backwards-compat anyway. The second adds a field with no context: why is this field ignored, and when should it be ignored?

There's a change currently being worked on by @dinhxuanvu in #91 that moves all constraints under the olm.constraints property. The "legacy" top-level constraints will be respected unless that property is defined, as we're discussing here. This change combined with docs and logs should raise this behavior's visibility appropriately.

enhancements/compound-bundle-constraints.md

joelanford · 2021-11-18T18:45:53Z

enhancements/compound-bundle-constraints.md

+I also propose that `olm.gvk.required` and `olm.package.required` are redefined as
+`gvk` and `package` fields to align with other properties under `olm.constraint`.
+The only other difference between existing and new GVK/package constraint schemas
+is the new schemas cannot be specified as top-level, non-compound fields; they must
+always be housed under one of `all`, `any`, or `none`. Given this difference,
+OLM will only interpret one set of schemas or the other as inputs to the resolver:


Shouldn't it also be possible to have something like this:

type: olm.constraint value: gvk: group: example.com version: v1 kind: MyApp

Basically you can only use gvk and package within an olm.constraint value. But I don't think it should be required that these two types are nested within a compound constraint.

My thought process was that top-level constraints are always a conjunction, so why not make it explicit as all. I am ok relaxing this given most users will probably not need compound constraints.

joelanford · 2021-11-18T18:50:27Z

enhancements/compound-bundle-constraints.md

+1. If at least one `olm.constraint` is present, interpret only `olm.constraint` properties
+and ignore all other constraint properties.


So anyone wanting use to a cel expression from #91 would need to also translate their olm.gvk.required and olm.package.required properties to be olm.constraint properties with gvk and package sub-fields?

I'm trying to understand the motivation for new versions of OLM ignoring the existing root level olm.package.required and olm.gvk.required.

joelanford · 2021-11-18T18:53:33Z

What effect does this EP have on the current GRPC API, which has requiredAPIs and dependencies fields in the Bundle object?

estroz · 2021-11-18T19:11:35Z

What effect does this EP have on the current GRPC API, which has requiredAPIs and dependencies fields in the Bundle object?

Is this change not being handled by #91? I would expect that olm.constraint's value is just sent wholesale over the gRPC API as a dependency, and requiredAPIs is left as-is since there isn't a good way to say what is required anymore. How would requiredAPIs be populated by a cel expression, for example?

joelanford · 2021-11-18T19:19:30Z

Is this change not being handled by #91? I would expect that olm.constraint's value is just sent wholesale over the gRPC API as a dependency, and requiredAPIs is left as-is since there isn't a good way to say what is required anymore. How would requiredAPIs be populated by a cel expression, for example?

I'm not sure #91 specifically covered this topic. Maybe we should revisit it there?

In general, this all makes sense. I'm just worried about something on the consuming end that doesn't know that requiredGVKs and dependencies are now a little more complicated and may not fully describe the constraints like they used to. I wonder if these fields need to be deprecated from the GRPC API.

estroz · 2021-11-18T19:43:24Z

Since #91 introduced olm.constraint and cel we should revisit it there.

I wonder if these fields need to be deprecated from the GRPC API.

Agreed, and perhaps a constraints field should be added to the API with contains all requiredAPIs plus the serialized values of new constraints. This further enforces the idea that constraints are properties and arbitrary, but can now be explicitly differentiated from properties as olm.constraints.

Signed-off-by: Eric Stroczynski <[email protected]>

estroz · 2021-11-19T00:33:36Z

Updates:

Allow gvk and package to be directly specified as olm.constraint values
OLM will interpret old and new constraints simultaneously
List OLM being able to choose between constraint schemas as an alternative

perdasilva · 2021-11-29T22:39:35Z

enhancements/compound-bundle-constraints.md

+The above bundle `foo.v1.0.0` requires that both a bundle in package `bar` that is of at least
+version 1.0.0 _and_ a bundle that provides the `EtcdBackup` API in group `etcd.database.coreos.com`
+and at version `v1beta2`. The resolver will translate those constraints into a boolean expression
+that is evaluated over all installed bundles's properties in the cluster; if both `bar` and `etcd`


Suggested change

that is evaluated over all installed bundles's properties in the cluster; if both `bar` and `etcd`

that is evaluated over all installed bundles' properties in the cluster; if both `bar` and `etcd`

perdasilva · 2021-11-29T22:53:30Z

enhancements/compound-bundle-constraints.md

+              kind: Foo
+```
+
+The maximum raw size of an `olm.constraint` is 64KB to limit resource exhaustion attacks.


perdasilva

lgtm - this is great work! Thank you for the amazing effort!

enhancements/compound-bundle-constraints.md

Signed-off-by: Eric Stroczynski <[email protected]>

joelanford · 2021-11-30T19:14:35Z

/approve

joelanford · 2021-11-30T19:19:10Z

/approve cancel

One last question is around Tech Preview vs. GA. I'm personally satisfied with the state of this EP being GA'd immediately. If others feel we need a release or two of Tech Preview time, I think we may need an update to this EP clarifying a few things about the Tech Preview state of this feature:

Is OLM upgradable if the feature is enabled?
How can a customer enable the feature gate? (OLMConfig object? CVO Unmanaged + manual change to OLM deployment?)
What would happen if a bundle specifies compound constraints but the feature is not enabled?

Signed-off-by: Eric Stroczynski <[email protected]>

joelanford · 2021-11-30T21:08:21Z

/approve

Great work @estroz

openshift-ci bot requested review from dinhxuanvu, joelanford and kevinrizza October 20, 2021 00:29

jmccormick2001 reviewed Oct 20, 2021

View reviewed changes

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

openshift-ci bot requested a review from dmesser October 20, 2021 19:49

perdasilva reviewed Oct 20, 2021

View reviewed changes

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

joelanford reviewed Oct 21, 2021

View reviewed changes

joelanford mentioned this pull request Nov 3, 2021

Add enhancement proposal for generic constraints feature #91

Merged

estroz requested review from joelanford and perdasilva November 3, 2021 20:43

ecordell reviewed Nov 4, 2021

View reviewed changes

perdasilva reviewed Nov 12, 2021

View reviewed changes

enhancements/compound-bundle-constraints.md Show resolved Hide resolved

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

estroz commented Nov 17, 2021

View reviewed changes

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

estroz requested review from ecordell, perdasilva and jmccormick2001 November 17, 2021 00:33

joelanford reviewed Nov 18, 2021

View reviewed changes

enhancements: compound-bundle-constraints

fb3cbc4

Signed-off-by: Eric Stroczynski <[email protected]>

estroz requested a review from joelanford November 19, 2021 00:33

perdasilva reviewed Nov 29, 2021

View reviewed changes

perdasilva approved these changes Nov 29, 2021

View reviewed changes

joelanford reviewed Nov 29, 2021

View reviewed changes

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

enhancements/compound-bundle-constraints.md Outdated Show resolved Hide resolved

fixed 2 sentences

25697f4

Signed-off-by: Eric Stroczynski <[email protected]>

remove alpha feature gate

40c22a5

Signed-off-by: Eric Stroczynski <[email protected]>

joelanford merged commit a162765 into operator-framework:master Nov 30, 2021

estroz deleted the compound-bundle-constraints branch December 1, 2021 17:18

estroz mentioned this pull request Dec 1, 2021

feat(resolver): olm.constraint property and compound constraints operator-framework/operator-lifecycle-manager#2418

Merged

5 tasks

benluddy mentioned this pull request Jan 25, 2022

Update compound-bundle-constraints enhancement #108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancements: compound-bundle-constraints #97

enhancements: compound-bundle-constraints #97

estroz commented Oct 20, 2021

estroz commented Oct 20, 2021

estroz commented Oct 20, 2021

joelanford left a comment

joelanford Oct 21, 2021

joelanford Oct 21, 2021

estroz Nov 3, 2021

ecordell Nov 4, 2021

estroz Nov 9, 2021

estroz commented Nov 9, 2021

joelanford commented Nov 9, 2021

estroz commented Nov 10, 2021

perdasilva commented Nov 12, 2021 •

edited

Loading

cdjohnson commented Nov 12, 2021

cdjohnson commented Nov 12, 2021

estroz commented Nov 12, 2021 •

edited

Loading

cdjohnson commented Nov 12, 2021

cdjohnson commented Nov 12, 2021

estroz commented Nov 12, 2021

joelanford Nov 18, 2021

estroz Nov 18, 2021

joelanford Nov 18, 2021

joelanford commented Nov 18, 2021 •

edited

Loading

estroz commented Nov 18, 2021

joelanford commented Nov 18, 2021

estroz commented Nov 18, 2021

estroz commented Nov 19, 2021

perdasilva Nov 29, 2021

perdasilva Nov 29, 2021

perdasilva left a comment

joelanford commented Nov 30, 2021

joelanford commented Nov 30, 2021

joelanford commented Nov 30, 2021


		### Graduation Criteria

		The alpha feature gate will be removed and this feature will be on permanently by OLM v1.0.0.

		1. If at least one `olm.constraint` is present, interpret only `olm.constraint` properties
		and ignore all other constraint properties.

	that is evaluated over all installed bundles's properties in the cluster; if both `bar` and `etcd`
	that is evaluated over all installed bundles' properties in the cluster; if both `bar` and `etcd`

enhancements: compound-bundle-constraints #97

enhancements: compound-bundle-constraints #97

Conversation

estroz commented Oct 20, 2021

estroz commented Oct 20, 2021

estroz commented Oct 20, 2021

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Nov 9, 2021

joelanford commented Nov 9, 2021

estroz commented Nov 10, 2021

perdasilva commented Nov 12, 2021 • edited Loading

cdjohnson commented Nov 12, 2021

cdjohnson commented Nov 12, 2021

estroz commented Nov 12, 2021 • edited Loading

cdjohnson commented Nov 12, 2021

cdjohnson commented Nov 12, 2021

estroz commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford commented Nov 18, 2021 • edited Loading

estroz commented Nov 18, 2021

joelanford commented Nov 18, 2021

estroz commented Nov 18, 2021

estroz commented Nov 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perdasilva left a comment

Choose a reason for hiding this comment

joelanford commented Nov 30, 2021

joelanford commented Nov 30, 2021

joelanford commented Nov 30, 2021

perdasilva commented Nov 12, 2021 •

edited

Loading

estroz commented Nov 12, 2021 •

edited

Loading

joelanford commented Nov 18, 2021 •

edited

Loading