Skip to content

Conversation

@joelanford
Copy link
Member

@joelanford joelanford commented Aug 28, 2021

Description of the change:

  • Define olm.channel schema to specify channel membership and upgrade edges. For example:

    ---
    schema: olm.channel
    package: foo
    name: stable
    entries:
      - name: foo.v0.0.1
      - name: foo.v0.0.2
      - name: foo.v0.1.0
        replaces: foo.v0.0.1
        skipRange: <0.1.0
        skips: 
          - foo.v0.0.2
  • Remove properties on the olm.bundle blobs that are related to channel metadata. Specifically olm.channel, olm.skips, and olm.skipRange are removed.

  • Conversions from the model to the file-based config format use the olm.channel schema.

  • opm render no longer populates channel-related metadata when rendering bundle images.

Motivation for the change:

The current schema overlooked the fact that bundle properties contained channel-level metadata, which goes directly against the design goals of file-based configs.

This PR corrects that issue by implementing a new olm.channel schema and removing the ability of opm render to pull this metadata from bundles created with imperative sqlite-based workflows in mind.

This is also an improvement on the the existing olm.bundle properties because:

  1. olm.skips and olm.skipRange properties apply to all channels. This causes channel upgrade graphs to be defined by channel-specific replaces values from the olm.channel property and by the global skips and skipRange properties, which can make it extremely difficult to define usable upgrade graphs when multiple channels are involved.
  2. It centralizes all of the channel membership and upgrade edge information in a single place, making it easier for humans to visualize and for computers to quickly parse and generate upgrade graphs.
  3. It simplifies implementation of veneer APIs that focus on different upgrade graph definitions. Different channel and upgrade graph definitions can be built without touching any olm.bundle blobs.

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 /docs
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2021
@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #768 (f4868c3) into master (f5dcf40) will increase coverage by 0.17%.
The diff coverage is 91.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage   50.45%   50.62%   +0.17%     
==========================================
  Files         102      101       -1     
  Lines        8719     8691      -28     
==========================================
+ Hits         4399     4400       +1     
+ Misses       3470     3454      -16     
+ Partials      850      837      -13     
Impacted Files Coverage Δ
alpha/action/list.go 72.94% <0.00%> (+5.23%) ⬆️
alpha/declcfg/declcfg.go 100.00% <ø> (ø)
alpha/model/model.go 92.64% <ø> (ø)
alpha/property/property.go 97.43% <ø> (-0.38%) ⬇️
alpha/property/scheme.go 100.00% <ø> (ø)
pkg/registry/registry_to_model.go 59.70% <ø> (+1.03%) ⬆️
alpha/declcfg/load.go 78.48% <16.66%> (-5.09%) ⬇️
alpha/declcfg/write.go 73.01% <80.00%> (+1.31%) ⬆️
alpha/action/render.go 64.21% <100.00%> (+0.18%) ⬆️
alpha/declcfg/declcfg_to_model.go 92.04% <100.00%> (+15.85%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5dcf40...f4868c3. Read the comment docs.

@joelanford
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2021
In this PR, the `olm.channel` schema has an extensible `strategy` field
with an initial `legacy` implementation that allows each entry to define
channel-specific replaces, skips, and skipRange.

Conversions from the model to the file-based config format use the
`olm.channel` blob rather than generating `olm.channel`, `olm.skips`,
and `olm.skipRange` properties on `olm.bundle` properties.

If a package has any `olm.channel` blobs, that means that package has
opted into fully defining its channels and upgrade edges with
`olm.channel` blobs, which means:
  1. `olm.channel`, `olm.skips` and `olm.skipRange` properties are not
     allowed on any bundles in that package.
  2. It is an error if an `olm.channel` blob references a non-existent
     bundle
  3. It is an error if a bundle exists in a channel that is not
     referenced by an `olm.channel` blob.

This is an improvement on the the existing `olm.bundle` properties
because:
1. `olm.skips` and `olm.skipRange` properties apply to all channels.
   This causes channel upgrade graphs to be defined by channel-specific
   replaces values from the `olm.channel` property and by the global
   skips and skipRange properties, which can make it extremely difficult
   to define usable upgrade graphs when multiple channels are involved.
2. It centralizes all of the channel membership and upgrade edge
   information in a single place, making it easier for humans to
   visualize and for computers to quickly parse and generate upgrade
   graphs.
3. It simplifies implementation of veneer APIs that focus on
   different upgrade graph definitions. Different channel and
   upgrade graph definitions can be built without touching any
   `olm.bundle` blobs.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the dc-olm-channel-legacy branch from 479ff85 to fbcdcd7 Compare September 1, 2021 19:31
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the dc-olm-channel-legacy branch from fbcdcd7 to ca53411 Compare September 1, 2021 19:31
@joelanford joelanford changed the title WIP: (proof of concept) Introduce olm.channel schema Introduce olm.channel schema, remove channel metadata-related bundle properties Sep 1, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2021
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@dmesser
Copy link
Contributor

dmesser commented Sep 2, 2021

This might be out of scope for this particular change, but one positive aspect of having all this update-graph/channel-related metadata in the bundle is that it made adding a bundle to a catalog extremely straight forward, essentially a single step.

It seems we are moving towards a structure where, due to the separate of concerns, now multiple steps are needed in order to add a bundle to an existing set of bundles in a catalog / package definition: adding the bundle reference and then reference the version in the channel definitions and various graph controls.
I'd like us to keep in mind that the proverbial 80% of the catalog interactions are going to be adding a new version that replaces the last version (or potentially everything else) to the default catalog. That was one of the reasons we built the semver-mode for opm.

What does this process look like in the future. Will it always be two/multiple steps?

@joelanford
Copy link
Member Author

joelanford commented Sep 2, 2021

This might be out of scope for this particular change, but one positive aspect of having all this update-graph/channel-related metadata in the bundle is that it made adding a bundle to a catalog extremely straight forward, essentially a single step.

I think what this PR brings into focus is that operator authors are the owners of their index. The number of steps for an operator author hasn't changed.

Before

  1. Set replaces, skips, and skipRange in their CSV
  2. Build the bundle
  3. Add the bundle to the index

After

  1. Build the bundle
  2. Add the bundle to the index
  3. Set replaces, skips, and skipRange in the index

In the After case, step 3 can be manual configuration (basically what replaces mode is today by doing the Before step 1), or it can be automated. For example by rendering a handful of bundles into an index and then running some automation that builds channels and their upgrade graphs purely by inspecting the properties of the bundle (where I'm sure properties['olm.package].value.version will be used often)

The declcfg repo has a semver example that shows how this could work. That example could even be extended further to automation that derives the channel name from the bundle versions (e.g. automatically create stable, stable-<major>, stable-<major>.<minor>, see a prototype I wrote in March here: https://github.com/joelanford/channelizer)

@dmesser
Copy link
Contributor

dmesser commented Sep 2, 2021

For most users of any pipelines that we currently entertain the process looks like this:

  1. Merge a PR to your bundle repo branch (i.e. release a new operator version with updated replaces, skips, etc)

That's it, then a pipeline picks it up, builds the image and adds it to the index. Making it a 3-way process by default would regress on the simplicity part quite a bit. We likely want to have some automation like your channelizer. I think I don't want to create more noise in this PR review about this, let's move the discussion to https://issues.redhat.com/browse/OLM-2107

@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2021
@estroz
Copy link
Member

estroz commented Sep 2, 2021

LGTM from diff's perspective.

@joelanford
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2021
@openshift-merge-robot openshift-merge-robot merged commit 59d0ec7 into operator-framework:master Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants