Remove Entities and EntitySources #413

dtfranz · 2023-09-14T20:01:58Z

Description

Bumps deppy version and removes use of Entity and EntitySource objects. In their place, the CatalogMetadata client is used along with new structs which are wrappers for the declcfg objects.

Resolves #347

joelanford · 2023-09-14T21:25:43Z

internal/resolution/variables/bundle.go

+// A bundle can be present in multiple channels and we need a separate variable
+// with a unique ID for each occurrence.


I know this is the data model we already have, but I'm really not a fan. There should be a single unique ID for each bundle in a particular catalog. I'm worried that creating multiple entities per bundle (one for each channel), will result in very confusing resolution unsat error messages.

Also, generating all of these extra bundle variables and making the variable space larger presumably requires more memory and CPU for resolution.

Not saying we fix this now, but definitely something we should fix soon before it becomes impossible.

@joelanford I wrote this code and I'm not a fan too! :) It is here because this is how it currently works in main: see internal/resolution/entitysources/catalogdsource.go where we create multiple instances of Entity - one per bundle occurnace in package channels.

However since I wrote this I was wondering whether it is actually necessary to have channel aware variables with all the filtering we do before we give it to the solver. I suggest that we leave it like this for now to stay close to what we already have in main, but look into it as a separate effort. I created #414 for this.

whether it is actually necessary to have channel aware variables with all the filtering we do before we give it to the solver.

There are at least two ways to do this:

As you said, when building bundle variables from an Operator, we know whether or not we need to filter by channels, and we know what all of the channel membership is. When we build the operator variable, we would end up with something close to OperatorFoo depends on BundleVariables(<set-of-bundle-variables-that-match-spec-specified-constraints>). So we would be able to filter on package name, bundle version, channel, and upgrade candidates before we even hand a variable off to the resolver. This is pretty much what I did in my PoC starting around here: https://github.com/joelanford/operator-controller/blob/31082f42fa9fbe61a4c9155683d0956e2425b774/internal/resolution/v2/variablesources/operator.go#L71

Another option is to build variables for channels, like: CatFoo/PackageBar/ChannelAlpha depends on BundleVariables(<set of bundles in the channel>). And then if an Operator specifies the alpha channel, then you'd make OperatorFoo depends on CatFoo/PackageBar/ChannelAlpha. (and if it doesn't specify a channel, then intuitively, it would have no dependencies on any channel variables).

One thing to worry about with option (2) is if we ever have an API that let's you specify that the bundle can come from multiple channels. With option 2, what's the preference ordering? Is it first by channel, then by version? Or is it first by version, then by channel? Either way, that preference order is easier to model/implement with option (1) because you can sort a flat list of bundles however you like.

m1kola

I noticed a bunch of "entity" leftovers in names. Probably there are more.

cmd/resolutioncli/main.go

internal/controllers/operator_controller.go

cmd/resolutioncli/main.go

internal/resolution/variables/bundle.go

internal/resolution/variables/installed_package.go

internal/resolution/variables/required_package.go

internal/resolution/variablesources/bundles_and_dependencies.go

internal/resolution/variablesources/installed_package.go

codecov · 2023-09-19T20:08:17Z

Codecov Report

Patch coverage: 91.22% and project coverage change: +0.45% 🎉

Comparison is base (6640aac) 83.97% compared to head (d35618b) 84.42%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   83.97%   84.42%   +0.45%     
==========================================
  Files          27       23       -4     
  Lines        1073      777     -296     
==========================================
- Hits          901      656     -245     
+ Misses        119       86      -33     
+ Partials       53       35      -18

Flag	Coverage Δ
e2e	`66.92% <80.70%> (+5.09%)`	⬆️
unit	`76.68% <88.18%> (-3.66%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...al/resolution/variablesources/installed_package.go	`65.21% <53.84%> (-6.22%)`	⬇️
...nal/resolution/variablesources/required_package.go	`74.41% <90.00%> (+0.60%)`	⬆️
internal/controllers/operator_controller.go	`80.81% <90.90%> (+0.10%)`	⬆️
...lution/variablesources/bundles_and_dependencies.go	`96.00% <93.33%> (+4.16%)`	⬆️
cmd/manager/main.go	`70.58% <100.00%> (+0.58%)`	⬆️
internal/catalogmetadata/client/client.go	`89.47% <100.00%> (ø)`
internal/controllers/variable_source.go	`100.00% <100.00%> (ø)`
internal/resolution/variables/bundle.go	`100.00% <100.00%> (ø)`
internal/resolution/variables/installed_package.go	`100.00% <100.00%> (ø)`
internal/resolution/variables/required_package.go	`100.00% <100.00%> (ø)`
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dtfranz · 2023-09-19T20:14:18Z

internal/controllers/operator_controller_test.go

@@ -575,43 +580,6 @@ var _ = Describe("Operator Controller Test", func() {
 				})
 			})
 		})
-		When("the selected bundle's image ref cannot be parsed", func() {


Removed this test as the bundle path retrieval no longer has an error path with the new struct.

ncdc · 2023-09-19T20:22:00Z

@dtfranz please update the description to say either "resolves" or "fixes" instead of "addresses". The former will result in the associated issue being closed when the PR is merged. That doesn't happen with "addresses". Thanks.

internal/catalogmetadata/client/client.go

ncdc

I am out of time for today, but wanted to submit what I had for now. Can try to review more tomorrow.

cmd/manager/main.go

cmd/resolutioncli/catalogmetadata_reader.go

cmd/resolutioncli/variable_source.go

internal/catalogmetadata/types_test.go

internal/catalogmetadata/types.go

joelanford · 2023-09-20T03:54:36Z

It looks like this may actually resolve the issue from #419 as well. In this PR (or maybe a follow-up is okay), can we update the controller unit test and the e2e test to add a new alpha channel for the prometheus package that contains a non-latest version, and then update the relevant assertions to make sure that 0.65.1 is still the resolved bundle?

See:

m1kola · 2023-09-20T12:30:55Z

Converted back to draft to avoud accidental merge. ~~It includes changes from #423 +~~ some feedback still need to be addressed.

joelanford · 2023-09-20T23:40:15Z

We probably also address #258 here

Is this that TODO comment I asked if there was a issue for. Definitely seems related. Seems like the propertyByType function is still lossy because, for duplicate property types, only the last one is found/kept/used.

dtfranz · 2023-09-21T00:08:59Z

It looks like this may actually resolve the issue from #419 as well. In this PR (or maybe a follow-up is okay), can we update the controller unit test and the e2e test to add a new alpha channel for the prometheus package that contains a non-latest version, and then update the relevant assertions to make sure that 0.65.1 is still the resolved bundle?

The changes are pretty big here already, I'd like to take care of this in a follow-up if that's alright. I think that you are correct though, that this should resolve that issue.

internal/resolution/variables/installed_package_test.go

internal/resolution/variables/required_package_test.go

joelanford

A few copy/pastas, but LGTM

Signed-off-by: Mikalai Radchuk <[email protected]> Signed-off-by: Daniel Franz<[email protected]>

Signed-off-by: Mikalai Radchuk <[email protected]>

* Removes provides+required GVK and associated tests. GVK related code was a dead weight since were never set GVK properties on bundle entity. * Remove entity + entitysource and associated util pkgs Signed-off-by: dtfranz <[email protected]>

Signed-off-by: dtfranz <[email protected]>

m1kola · 2023-09-21T10:26:01Z

I believe all the remaining feedback was addressed. Will convert back to ready for review shortly.

We probably also address #258 here

Is this that TODO comment I asked if there was a issue for. Definitely seems related. Seems like the propertyByType function is still lossy because, for duplicate property types, only the last one is found/kept/used.

@joelanford not exactly, but it is related. In main right now catalogd entity source is reading only a subset of bundle properties and re-writes this subset into bundle entity properties (often in different format). So bundle entity properties != bundle properties.

With new implementation we have all the properties from catalog's metadata "as is" in Bundle struct without modifications and they are ready to be consumed. TODO was about reading them incorrectly in some instances (e.g. when properties with the same type appear multiple times).

The issue from TODO does not affect this PR or even master as we currently do not use any props with multiple instances. However it is nice to address. I created #427 for this.

internal/resolution/variablesources/bundles_and_dependencies_test.go

Signed-off-by: dtfranz <[email protected]>

m1kola

Looks good to me!

P.S. Not submitting this as "Approve" review beucase I was heavily involved in this and it is not right to approve my own code .

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 Sep 14, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2023

joelanford reviewed Sep 14, 2023

View reviewed changes

m1kola mentioned this pull request Sep 15, 2023

Reduce number of resolution variables to one per bundle #414

Closed

m1kola reviewed Sep 15, 2023

View reviewed changes

dtfranz force-pushed the remove_entity_soruces_dfranz branch 6 times, most recently from 8414cde to 5f1d0df Compare September 19, 2023 20:05

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

dtfranz marked this pull request as ready for review September 19, 2023 20:05

dtfranz requested a review from a team as a code owner September 19, 2023 20:05

dtfranz changed the title ~~WIP: Remove Entities and EntitySources~~ Remove Entities and EntitySources Sep 19, 2023

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 19, 2023

dtfranz commented Sep 19, 2023

View reviewed changes

ncdc reviewed Sep 19, 2023

View reviewed changes

internal/catalogmetadata/client/client.go Outdated Show resolved Hide resolved

dtfranz force-pushed the remove_entity_soruces_dfranz branch from 5f1d0df to d3dd487 Compare September 19, 2023 20:28

ncdc reviewed Sep 19, 2023

View reviewed changes

joelanford reviewed Sep 20, 2023

View reviewed changes

internal/catalogmetadata/types.go Outdated Show resolved Hide resolved

m1kola mentioned this pull request Sep 20, 2023

Fixes semver range parsing for required packages #423

Merged

4 tasks

m1kola force-pushed the remove_entity_soruces_dfranz branch from d3dd487 to 60c02a1 Compare September 20, 2023 12:28

m1kola marked this pull request as draft September 20, 2023 12:30

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 Sep 20, 2023

m1kola force-pushed the remove_entity_soruces_dfranz branch from 60c02a1 to 59c7dc1 Compare September 20, 2023 12:38

dtfranz force-pushed the remove_entity_soruces_dfranz branch from c16b861 to 57ebe86 Compare September 20, 2023 20:39

joelanford reviewed Sep 21, 2023

View reviewed changes

internal/resolution/variables/installed_package_test.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 21, 2023

View reviewed changes

internal/resolution/variables/required_package_test.go Outdated Show resolved Hide resolved

joelanford approved these changes Sep 21, 2023

View reviewed changes

m1kola and others added 5 commits September 21, 2023 10:45

Updates codebase to use catalogmetadata package

d8ae4fb

Signed-off-by: Mikalai Radchuk <[email protected]> Signed-off-by: Daniel Franz<[email protected]>

Updates resolutioncli to remove entity source

fc5dace

Signed-off-by: Mikalai Radchuk <[email protected]>

Removes entity sources and GVK related code

d180207

* Removes provides+required GVK and associated tests. GVK related code was a dead weight since were never set GVK properties on bundle entity. * Remove entity + entitysource and associated util pkgs Signed-off-by: dtfranz <[email protected]>

Deppy version bump, remove remaining entity/entitysource vestigials

66c7be9

Signed-off-by: dtfranz <[email protected]>

Add interface for client mocking

c02fe9b

Signed-off-by: dtfranz <[email protected]>

m1kola mentioned this pull request Sep 21, 2023

Bug: Non-latest version installed in packages with multiple channels #419

Closed

m1kola force-pushed the remove_entity_soruces_dfranz branch from 57ebe86 to 832502d Compare September 21, 2023 10:00

m1kola marked this pull request as ready for review September 21, 2023 10:26

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 21, 2023

m1kola reviewed Sep 21, 2023

View reviewed changes

internal/resolution/variablesources/bundles_and_dependencies_test.go Outdated Show resolved Hide resolved

m1kola force-pushed the remove_entity_soruces_dfranz branch from 832502d to 9d88cb0 Compare September 21, 2023 10:40

m1kola reviewed Sep 21, 2023

View reviewed changes

internal/resolution/variablesources/bundles_and_dependencies_test.go Outdated Show resolved Hide resolved

m1kola force-pushed the remove_entity_soruces_dfranz branch 2 times, most recently from ad3dd46 to 97e80f6 Compare September 21, 2023 10:55

Fix tests, bump catalogd, address comments.

d35618b

Signed-off-by: dtfranz <[email protected]>

m1kola force-pushed the remove_entity_soruces_dfranz branch from 97e80f6 to d35618b Compare September 21, 2023 11:00

m1kola reviewed Sep 21, 2023

View reviewed changes

joelanford added this pull request to the merge queue Sep 21, 2023

Merged via the queue into operator-framework:main with commit 52a1e28 Sep 21, 2023
12 checks passed

joelanford mentioned this pull request Sep 21, 2023

fix: do not include channel name or priority in bundle sorting during resolution #420

Merged

4 tasks

m1kola mentioned this pull request Sep 21, 2023

Ensure the FBC --> entity translation for bundle properties is not lossy #258

Closed

joelanford mentioned this pull request Oct 17, 2023

4.14 OLM 1.0 Tech Preview procedures openshift/openshift-docs#66036

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Entities and EntitySources #413

Remove Entities and EntitySources #413

dtfranz commented Sep 14, 2023 •

edited

Loading

joelanford Sep 14, 2023

m1kola Sep 15, 2023

joelanford Sep 15, 2023

m1kola left a comment

codecov bot commented Sep 19, 2023 •

edited

Loading

dtfranz Sep 19, 2023

ncdc commented Sep 19, 2023

ncdc left a comment

joelanford commented Sep 20, 2023

m1kola commented Sep 20, 2023 •

edited

Loading

joelanford commented Sep 20, 2023

dtfranz commented Sep 21, 2023

joelanford left a comment

m1kola commented Sep 21, 2023

m1kola left a comment

		// A bundle can be present in multiple channels and we need a separate variable
		// with a unique ID for each occurrence.

Remove Entities and EntitySources #413

Remove Entities and EntitySources #413

Conversation

dtfranz commented Sep 14, 2023 • edited Loading

Description

joelanford Sep 14, 2023

Choose a reason for hiding this comment

m1kola Sep 15, 2023

Choose a reason for hiding this comment

joelanford Sep 15, 2023

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

dtfranz Sep 19, 2023

Choose a reason for hiding this comment

ncdc commented Sep 19, 2023

ncdc left a comment

Choose a reason for hiding this comment

joelanford commented Sep 20, 2023

m1kola commented Sep 20, 2023 • edited Loading

joelanford commented Sep 20, 2023

dtfranz commented Sep 21, 2023

joelanford left a comment

Choose a reason for hiding this comment

m1kola commented Sep 21, 2023

m1kola left a comment

Choose a reason for hiding this comment

dtfranz commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

m1kola commented Sep 20, 2023 •

edited

Loading