operator-registry icon indicating copy to clipboard operation
operator-registry copied to clipboard

WIP: add reverse-migrate command to support converting from FBC to sqlite

Open joelanford opened this issue 3 years ago • 6 comments

Signed-off-by: Joe Lanford [email protected]

Description of the change: Adds opm reverse-migrate command that converts an FBC to sqlite.

The first commit (f05ca62) is from #972. The additional changes that are the focus of this PR are in the second commit.

Motivation for the change: We'd like teams to be able to opt into FBC, but then be able to ship that catalog to a cluster where only sqlite is supported.

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

joelanford avatar Apr 28 '22 18:04 joelanford

[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[bot] avatar Apr 28 '22 18:04 openshift-ci[bot]

Codecov Report

Merging #949 (013c020) into master (0899512) will decrease coverage by 0.27%. The diff coverage is 27.27%.

:exclamation: Current head 013c020 differs from pull request most recent head f1b060a. Consider uploading reports for the commit f1b060a to get more accurate results

@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
- Coverage   52.48%   52.21%   -0.28%     
==========================================
  Files         103      105       +2     
  Lines        9240     9398     +158     
==========================================
+ Hits         4850     4907      +57     
- Misses       3468     3551      +83     
- Partials      922      940      +18     
Impacted Files Coverage Δ
alpha/action/reversemigrate.go 0.00% <0.00%> (ø)
pkg/lib/registry/registry.go 22.90% <0.00%> (-0.21%) :arrow_down:
pkg/registry/channelupdateoptions.go 63.63% <0.00%> (+63.63%) :arrow_up:
pkg/registry/empty.go 0.00% <0.00%> (ø)
pkg/registry/graph.go 20.00% <0.00%> (-1.74%) :arrow_down:
pkg/sqlite/graphloader.go 69.44% <50.00%> (-1.15%) :arrow_down:
pkg/sqlite/load.go 46.86% <50.00%> (+0.58%) :arrow_up:
pkg/sqlite/query.go 64.32% <50.00%> (-0.22%) :arrow_down:
pkg/registry/populator.go 66.97% <56.25%> (+3.81%) :arrow_up:
pkg/sqlite/migrations/014_package_add_mode.go 69.69% <69.69%> (ø)
... and 7 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 0899512...f1b060a. Read the comment docs.

codecov[bot] avatar Apr 28 '22 18:04 codecov[bot]

I question the need of this command given FBC is just a format. What matters is if the binary can read FBC or not. In the end, the cluster only queries grpc endpoints that catalogsource servers. As a result, we can ship any FBC-based catalogsource to any clusters as long as it supports the existing grpc API. From the cluster perspective, it couldn't care less if the catalogsource is FBC or sqlite-based.

dinhxuanvu avatar Apr 29 '22 03:04 dinhxuanvu

@dinhxuanvu Fair point from the cluster perspective. However, we decided that we cannot change the format of existing published catalogs because it does matter to humans and other tools that are expecting SQLite. Changing something we published as SQLite to FBC would constitute a breaking change.

So this PR would enable operator teams to use FBC but for catalog publishers to make non-breaking changes to existing catalogs they've published.

To be fair, there is some concern here that an sqlite DB constructed this way is still a breaking change because there might be something here that causes the existing sqlite tooling to break in someway (e.g. does opm registry add still work, does opm registry prune still work, etc.?).

I'd posit that this PR creates a DB that is somewhat equivalent to a semver-skippatch created DB in that:

  • in FBC, bundles are identical across channels, and channels can have different upgrade edges between bundles
  • in semver-skippatch, bundles are identical across channels, and channels can have different upgrade edges between bundles.

The only restriction I've found so far is that FBC channel entries for particular bundles have to use the same skipRange across all channels where that bundle has an entry.

joelanford avatar Apr 29 '22 15:04 joelanford

does opm registry add still work, does opm registry prune still work?

One way I think we could potentially solve this problem is by adding an immutable column to the package table:

  1. When we reverse-migrate, we mark everything as immutable: true.
  2. When any other index/registry command would try to mutate a package, it would check this column and fail if its set to true.
  3. When any other index/registry command tries to completely remove a package, that would be allowed (so things like opm registry prune|rm would will work).

joelanford avatar Apr 29 '22 15:04 joelanford

@joelanford: 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.

openshift-merge-robot avatar Jul 29 '22 13:07 openshift-merge-robot

closing PR as stale. Please re-open if it's still important.

perdasilva avatar Feb 19 '24 13:02 perdasilva