feat(opm): merge objects of same unique key in `alpha diff` #823

estroz · 2021-11-05T21:08:02Z

Description of the change: opm alpha diff now merges packages, channels, and bundles that have the same unique key with ascending preference.

Motivation for the change: Deduplicate data that may cause errors at runtime

Test this change by building opm then comparing the output of master and this branch's opm alpha diff on some test catalogs I built:

$ ./bin/opm-master alpha diff quay.io/estroz/test-catalog:latest,quay.io/estroz/test-catalog:disjoint -o yaml > nomerge.yaml
...
$ ./bin/opm-merge alpha diff quay.io/estroz/test-catalog:latest,quay.io/estroz/test-catalog:disjoint -o yaml > merge.yaml
FATA[0005] error generating diff: error converting new declarative config to model: package "bar", bundle "bar.v0.2.0" not found in any channel entries

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 · 2021-11-05T21:08:04Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2021-11-05T21:08:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: estroz
To complete the pull request process, please assign joelanford after the PR has been reviewed.
You can assign the PR to them by writing /assign @joelanford in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2021-11-05T21:13:21Z

Codecov Report

Merging #823 (c5bd41e) into master (384af6b) will increase coverage by 0.42%.
The diff coverage is 69.23%.

❗ Current head c5bd41e differs from pull request most recent head 1352797. Consider uploading reports for the commit 1352797 to get more accurate results

@@            Coverage Diff             @@
##           master     #823      +/-   ##
==========================================
+ Coverage   52.07%   52.50%   +0.42%     
==========================================
  Files         103      104       +1     
  Lines        9092     9220     +128     
==========================================
+ Hits         4735     4841     +106     
- Misses       3449     3463      +14     
- Partials      908      916       +8

Impacted Files	Coverage Δ
pkg/lib/indexer/indexer.go	`10.61% <0.00%> (-0.18%)`	⬇️
pkg/lib/registry/registry.go	`18.39% <0.00%> (-0.36%)`	⬇️
alpha/action/diff.go	`73.13% <33.33%> (-3.92%)`	⬇️
alpha/declcfg/merge.go	`83.01% <83.01%> (ø)`
alpha/declcfg/diff.go	`77.96% <100.00%> (+1.04%)`	⬆️
pkg/lib/bundle/exporter.go	`62.85% <100.00%> (+6.19%)`	⬆️
alpha/declcfg/diff_include.go	`67.92% <0.00%> (+6.60%)`	⬆️

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 384af6b...1352797. Read the comment docs.

joelanford · 2021-11-08T15:58:16Z

I'm still concerned that this puts render in the opinionated camp. And once something has opinions, its hard to say no to making that thing have variations of those opinions. My concern is that as soon as we say "opm render can merge things", we'll get all sorts of requests about how to merge those things:

Can we prefer values from references listed earlier instead of later?
Can we "intelligently" merge channel entries? (with variations on what's intelligent for various use cases)
Can we "intelligently" merge bundle properties? (with variations on what's intelligent for various use cases)
Can we override some values but merge others?
etc, etc.

If this is a specific need of the diff command, can we hardcode the correct merge logic directly into the diff command's logic?

estroz · 2021-11-08T19:34:54Z

All valid concerns. I do actually think, in the long term, opm or some library it consumes should have the ability to support the different merge strategies you suggest (and more), while being opinionated by default. I also want to point out that you can effectively get different merge strategies by preprocessing opm render --merge input refs yourself, ex. reversing the input ref list(s) to reverse merge preference, or breaking out packages/channels/bundles into separate refs and reordering them with jq. Ultimately I'd like the merge library opm render uses to be extensible to several merge strategies, but opm render itself has only one or two opinionated ways to merge 2+ refs. opm render --merge may be used in the future to handle the multiple-catalog-shared-content issue, which is a real concern when one cluster uses multiple curated but uncoordinated content sources, so it would be better to have a tool for cluster admins that isn't perfect than no tool at all to handle this issue. Perhaps this warrants an EP in itself.

If this is a specific need of the diff command, can we hardcode the correct merge logic directly into the diff command's logic?

For now this is totally fine with me.

cdjohnson · 2021-11-08T20:06:02Z

Is the result of this consistent with how opm serve currently merges the stream? The result should be identical assuming there is no advanced/intelligent merge policy.

joelanford · 2021-11-08T22:02:14Z

Is the result of this consistent with how opm serve currently merges the stream? The result should be identical assuming there is no advanced/intelligent merge policy.

opm serve doesn't merge anything. It loads it all and then errors out (via an internal opm validate call) if it finds any duplicates.

cdjohnson · 2021-11-09T14:21:36Z

@joelanford

opm serve doesn't merge anything. It loads it all and then errors out (via an internal opm validate call) if it finds any duplicates.

It appears to do some sort of merging or deduplication (I didn't do an exhaustive test). Take a look at this FBC:
docker.io/cdjohnson/depcatalogs@sha256:e067cbe0738608c19f66b71828d8dfa75a25790178316b73585cc7dc2967e635

# See that testoperatorc is duplicated
cat index/index.yaml | yq -r 'select(.schema == "olm.package").name'
testoperatora
testoperatorb
testoperatorc
testoperatorc

cat index/index.yaml | yq -r 'select(.schema == "olm.bundle").name'
testoperatora.v1.0.0
testoperatorb.v2.0.0
testoperatorc.v3.0.0
testoperatorc.v3.0.0
testoperatorc.v3.0.1

opm validate passes

When I serve it, it ignores (or merges) the duplicates:
opm serve index &

grpcurl -plaintext localhost:50051 api.Registry/ListBundles | jq '{name: .csvName, version: .version, replaces: .replaces, skipRange: .skipRange, skips: .skips, channelName: .channelName}' | jq -s -c 'sort_by(.name)[]'
{"name":"testoperatora.v1.0.0","version":"1.0.0","replaces":null,"skipRange":null,"skips":null,"channelName":"v1.0"}
{"name":"testoperatorb.v2.0.0","version":"2.0.0","replaces":null,"skipRange":null,"skips":null,"channelName":"v2.0"}
{"name":"testoperatorc.v3.0.0","version":"3.0.0","replaces":null,"skipRange":null,"skips":null,"channelName":"v3.0"}
{"name":"testoperatorc.v3.0.1","version":"3.0.1","replaces":"testoperatorc.v3.0.0","skipRange":"<3.0.1","skips":null,"channelName":"v3.0"}

joelanford · 2021-11-10T14:23:49Z

It appears to do some sort of merging or deduplication

Looking at the code, I think you're right! But I think this is a regression. At one point, I'm fairly sure opm validate was producing and error if it found multiple bundles with the same name in the same package.

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

Now taking the time to look back through the history of the FBC implementation, I didn't find any code that actually prevented duplicates. It looks like the code has always just accepted the last duplicate item (i.e. no merging, just throw away the old thing and keep the new thing when I encounter a duplicate)

I still consider this an error though because a declarative API (which FBC is), should disallow such ambiguity. Declaring "I want A to look like X AND I want A to look like Y" just doesn't make sense.

PR to fix this is #824.

…e key Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>

estroz · 2021-12-16T01:17:07Z

/cc @joelanford

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>

joelanford

Sorry for the delay on the review. I left a few comments related to some hopefully minor refactoring.

However, I am still concerned that doing this type of merging runs counter to the declarative nature of FBC.

It looks like we're merging old on its own, merging new on its own, and then diffing the individually merged models. I question why old and new are ever in a state where they need to be merged in the first place. Shouldn't they both already be fully valid FBCs?

alpha/action/merge.go

…implements as an interface This change as a Merger interface and two concrete types, PreferLast and TwoWay, that merge FBC objects. Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>

joelanford · 2022-01-21T01:57:57Z

alpha/declcfg/merge.go

-	}
+// Merger is an object that will complete merge actions declarative config options
+type Merger interface {
+	MergeDC(*DeclarativeConfig) error


Nit:

Suggested change

MergeDC(*DeclarativeConfig) error

Merge(*DeclarativeConfig) error

Its in the declcfg package and its only parameter is a *DelcarativeConfig, so IMO including DC in the function name is redundant.

joelanford · 2022-01-21T02:15:59Z

alpha/action/diff.go

+		if err != nil {
+			return nil, fmt.Errorf("error setting merge type: %v", err)
+		}
+		if err := a.Merger.MergeDC(oldCfg); err != nil {


I'm still confused by what it means to merge a single *declcfg.DeclarativeConfig.

Shouldn't we be expecting any given individual FBC to be valid?

I would have expected to see something like:

// Validate the old config oldModel, err := declcfg.ToModel(oldCfg) if err != nil { return nil, err } if err := oldModel.Validate(); err != nil { return nil, err } // Validate the new config newModel, err := declcfg.ToModel(newCfg) if err != nil { return nil, err } if err := newModel.Validate(); err != nil { return nil, err } // Merge them together if err := a.Merger.Merge(oldCfg, newCfg); err != nil { return nil, err }

Sorry @joelanford, I think I missed the earlier comment when you mentioned this concern before. I think the use-case for keeping the single argument to the Merge function is that the oldCfg and the newCfg are generated by Render. Since render isn't handling duplicate objects it would be possible for the FBC created by the groups of OldRefs and NewRefs could be invalid. Would changing from Merge to Normalize make more sense here?

Would changing from Merge to Normalize make more sense here?

Perhaps

oldCfg and the newCfg are generated by Render.

Makes sense, but what's the case that led to those needing to be normalized? Shouldn't old and new be valid standalone FBCs?

What I want to avoid is having our exported libraries normalize the possibility of an FBC being invalid and us providing code that can turn an FBC from invalid to valid.

If there's something happening in opm alpha diff that gives rise to the need to normalize some intermediate FBC state, I think all of this is fine, but I'd probably make it internal and unexported.

Just adding some info here for historical purposes -

The merge functionality was public because it is required by oc-mirror, but we have agreed to move this functionality directly into the oc-mirror codebase since the requirement to normalize invalid FBCs is a unique use-case.

Can you clarify? Is the entire merge function moving to oc-mirror or just the single-fbc normalization function?

Just the single-fbc normalization work proposed on this PR was moved to oc-mirror

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>

openshift-ci · 2022-02-26T22:44:21Z

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

grokspawn · 2022-10-19T18:10:01Z

opm diff has moved to oc mirror. From the conversation it appears that team has already extracted the salient bits of this PR for that project. Is there any aspect of this PR which hasn't been moved over and is still needed?

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 Nov 5, 2021

openshift-ci bot requested review from dinhxuanvu and hasbro17 November 5, 2021 21:08

estroz force-pushed the feat/render-merge branch from dcbfd6e to fcea83f Compare November 5, 2021 21:14

estroz force-pushed the feat/render-merge branch from fcea83f to f717b79 Compare November 9, 2021 22:24

estroz changed the title ~~feat(opm): optional merge behavior in render, always merge in diff~~ feat(opm): merge objects of same unique key in alpha diff Nov 10, 2021

estroz marked this pull request as ready for review December 15, 2021 18:15

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 Dec 15, 2021

jpower432 mentioned this pull request Dec 15, 2021

chore: update operator registry dep openshift/oc-mirror#238

Merged

13 tasks

estroz force-pushed the feat/render-merge branch 2 times, most recently from 03ea0e0 to 128a2a0 Compare December 16, 2021 01:04

feat(diff): introduce MergeTypes to merge objects with the same uniqu…

6ad003f

…e key Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>

openshift-ci bot requested a review from joelanford December 16, 2021 01:17

chore: make mergeDC function public

4124d2e

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>

estroz force-pushed the feat/render-merge branch from 128a2a0 to 4124d2e Compare December 17, 2021 19:24

joelanford requested changes Jan 10, 2022

View reviewed changes

alpha/action/merge.go Outdated Show resolved Hide resolved

alpha/action/merge.go Outdated Show resolved Hide resolved

refactor(merge): move merge functionality from action to declcfg and …

1bfb421

…implements as an interface This change as a Merger interface and two concrete types, PreferLast and TwoWay, that merge FBC objects. Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>

joelanford reviewed Jan 21, 2022

View reviewed changes

chore: change MergeDC function to Merge for clarity

1352797

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2022

anik120 closed this Mar 7, 2023

	MergeDC(*DeclarativeConfig) error
	Merge(*DeclarativeConfig) error

feat(opm): merge objects of same unique key in alpha diff #823

feat(opm): merge objects of same unique key in alpha diff #823

Uh oh!

Conversation

estroz commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 5, 2021

Uh oh!

openshift-ci bot commented Nov 5, 2021

Uh oh!

codecov bot commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joelanford commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdjohnson commented Nov 8, 2021

Uh oh!

joelanford commented Nov 8, 2021

Uh oh!

cdjohnson commented Nov 9, 2021

Uh oh!

joelanford commented Nov 10, 2021

Uh oh!

joelanford commented Nov 10, 2021

Uh oh!

estroz commented Dec 16, 2021

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joelanford Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

joelanford Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

jpower432 Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

joelanford Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

joelanford Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

jpower432 Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdjohnson Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

jpower432 Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 26, 2022

Uh oh!

grokspawn commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat(opm): merge objects of same unique key in `alpha diff` #823

feat(opm): merge objects of same unique key in `alpha diff` #823

estroz commented Nov 5, 2021 •

edited

Loading

codecov bot commented Nov 5, 2021 •

edited

Loading

joelanford commented Nov 8, 2021 •

edited

Loading

estroz commented Nov 8, 2021 •

edited

Loading

jpower432 Jan 25, 2022 •

edited

Loading