Enhance diff logic to set a new defaultChannel by adding a priority property to a channel

[joehuizenga]

Description of the change: This PoC was driven from a discussion in the oc-mirror project https://github.com/openshift/oc-mirror/issues/453

There are scenarios where the default channel for a package is not updated when diff filters out all the packages within original default channel.

This PR proposes a solution to this issue by adding a channel priority property and logic in the diff code that will use this new priority to set the defaultChannel

Motivation for the change:

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[bot]]

Hi @joehuizenga. 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.

[joelanford]

I don't think this makes sense in the context of the core design of FBC. FBC is declarative and on-cluster FBC clients don't have any concept of channel priority.

Any tool that operates on FBC should always generate a valid FBC as output. If that tool filters out the default channel, that tool is responsible for changing the default channel name specified in the package. That means that tool needs to do one of two things:

1. Allow the new default channel to be defined as input
2. Use a heuristic that chooses a new default channel.

I suppose its conceivable that one such heuristic could be "choose the channel with the highest priority as defined by a new priority property", but I'm not convinced this should be something we support in this repo.

I think there's a larger discussion to be had around the design of opm diff and oc mirror. I don't know all of the history, but my impression of the goal of opm diff is to provide oc mirror with a "slimmed down" catalog that's the minimum thing necessary for mirroring.

IMO, I think we've built some APIs incorrectly that lead us to co-mingling concepts. I think mirroring should be three explicit stages:

1. Execute $someProcess that produces the full FBC that you want to be available in the disconnected environment. Our extensible veneer concept is basically this (a veneer is anything that produces valid FBC)
2. Run a diff of the $alreadyMirroredFBCImage and the $toBeMirroredFBCImage; produce a list of images/relatedImages that are present only in the $toBeMirroredFBCImage set. The $toBeMirroredFBCImage and this set of images are fed into the actual mirroring process
3. Mirror the full set of images (or at least their layer differences) produced in step 2.

The end result of this process is that the FBC generated in step 1 is the FBC that is consumed on the disconnected side. And steps 2 and 3 ensure that it and all of the images that it references are mirrored correctly.

[joelanford]

I do think it's fair to say that oc mirror has effectively implemented a veneer with the ImageSetConfiguration processing logic, so perhaps this is just a matter of moving code around to make it clear who owns what from a support/maintenance standpoint.

If a veneer wants to define some properties on channels that it uses to drive its logic, that's completely fair game.

[joehuizenga]

In my mind I do view oc-mirror and its ISC as a veneer. From some prior discussions, I was under the impression that maybe the alpha diff code was going to move into the oc-mirror code base and then this "opinionated" use of a channel priority would be ok (ie it would live and be supported in oc-mirror). For this PoC I implemented this in operator-registry and then pulled it into oc-mirror

We have also had some initial discussions about writing 3rd party tools that would use the oc-mirror library vs trying to extend the functionality of the oc-mirror cli itself

[joelanford]

I was under the impression that maybe the alpha diff code was going to move into the oc-mirror code base and then this "opinionated" use of a channel priority would be ok (ie it would live and be supported in oc-mirror)

Yep, that all sounds about right, and is a fair take IMO, assuming the oc-mirror folks are on board of course.

[joehuizenga]

@jpower432 @dinhxuanvu @joelanford in the OLM workgroup meeting last week I briefly covered this PR, it sounded like it might be feasible if its something that the oc-mirror team is "ok" with. I know this work is transitioning to a new team and that the alpha diff code currently in this repo may move but I would like to understand if this approach is worth taking forward or if we need to look to some other way. I am new to this process so welcome any and all suggestions as to the best way to proceed.

[joehuizenga]

@dmesser @joelanford We would like to proceed with this enhancement so I have removed the WIP reference in the title for this issues. I am happy to have @pawicao-ibm onboarding to help work thru the process. This is the first time either of us has gone thru this process so any guidance or direction will be greatly appreciated!

[dinhxuanvu]

Hey folks, Just a few thoughts here. I'm in favor of the changes that @joehuizenga is proposing in the PR though admittedly I'm a bit biased given it was my idea to do this. There is a concern about if these changes should be made directly to registry given this is not necessarily relevant to registry. As we did talk about this before, this property has no meaning to OLM/resolver so indirectly it has no meaning for registry either. So keeping it out of registry will be ideal to avoid confusion. There was a plan to move all of diff code over to oc-mirror repo as the diff functionality no longer serves any purposes on registry. Frankly, it was designed for mirroring work and had nothing to do with registry in the first place. If you are open to this, I would recommend you to move the entire diff code over to oc-mirror along the changes in the PR. That would enable the changes to be merged and used directly on oc-mirror. What do you think?

[joehuizenga]

@dinhxuanvu what are your thoughts about the changes that aren't in the diff code? The new channel priority property for example? Are you suggesting that all traces of this PR need to move to oc-mirror?

[dinhxuanvu]

@dinhxuanvu what are your thoughts about the changes that aren't in the diff code? The new channel priority property for example? Are you suggesting that all traces of this PR need to move to oc-mirror?

I haven't reviewed the code line-by-line but from the initial review I think everything looks fine. Probably a few polishing here and there and it should be ready for the final review. Yes. I suggest we move the entire diff code from registry to oc-mirror including the changes in your PR.

[pawicao-ibm]

https://github.com/operator-framework/operator-registry/pull/1006 - follow-up PR with a suggestion of migration. I'd suggest to close this one and move the discussions to the new one

[pawicao-ibm]

Following the suggestion https://github.com/operator-framework/operator-registry/pull/1006#issuecomment-1208475702 this PR now includes only the changes for Channel Priorities and it shouldn't be closed.

https://github.com/operator-framework/operator-registry/pull/1006 includes the rest of the work, removing the diff logic and preparing operator-registry to serve proper variables to oc-mirror which the diff logic will be migrated into

[dinhxuanvu]

/ok-to-test

[dinhxuanvu]

@pawicao-ibm Just an update for you. We are moving forward with this PR. Please add explicit comments on all channel Properties fields to indicate that this field is for internal use and do not use for public-facing functionalities and also remind that this API is in alpha so it is subject to change. Thanks.

[codecov[bot]]

Codecov Report

Merging #969 (f23c7d3) into master (63c1932) will decrease coverage by 0.01%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #969      +/-   ##
==========================================
- Coverage   51.67%   51.65%   -0.02%     
==========================================
  Files         102      102              
  Lines        9153     9162       +9     
==========================================
+ Hits         4730     4733       +3     
- Misses       3515     3521       +6     
  Partials      908      908              

Impacted Files	Coverage Δ
alpha/model/model.go	92.64% <ø> (ø)
alpha/property/property.go	92.68% <0.00%> (-4.76%)	:arrow_down:
alpha/declcfg/declcfg_to_model.go	93.13% <100.00%> (+0.06%)	:arrow_up:
alpha/declcfg/model_to_declcfg.go	100.00% <100.00%> (ø)
alpha/property/scheme.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pawicao-ibm]

@dinhxuanvu is it okay now?

[dinhxuanvu]

@pawicao-ibm Looking good to me.

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, joehuizenga

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:

• ~~OWNERS~~ [dinhxuanvu]

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

[dinhxuanvu]

@joelanford PTAL.

[joelanford]

/lgtm

[openshift-bot]

/retest-required

Please review the full test history for this PR and help us cut down flakes.