🌱 refactor catalogmetadata types used for resolution #599

everettraven · 2024-01-29T21:57:51Z

Description

Refactors the types in the internal/catalogmetadata package to reduce the complexity of requiring all the OLM-isms to be conveyed through the single Bundle type. Updates all resolution logic and unit tests as needed.
resolves Deppy: remove assumption about entities being members of channels #176 (?)
Any other issues folks think this resolves?

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

everettraven · 2024-01-29T22:00:05Z

In case anyone looks at this before I get around to it, there is still a good amount of refactoring to do in the unit tests for this change. I've verified manually that the changes still result in the same functionality but I fully expect unit tests to fail as the PR currently stands

Signed-off-by: everettraven <everettraven@gmail.com>

codecov · 2024-01-31T18:36:49Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (ef06f40) 84.53% compared to head (32466fa) 80.98%.
Report is 30 commits behind head on main.

Files	Patch %	Lines
internal/catalogmetadata/client/client.go	66.66%	11 Missing and 1 partial ⚠️
...ternal/catalogmetadata/filter/bundle_predicates.go	28.57%	5 Missing ⚠️
internal/catalogmetadata/types.go	87.50%	3 Missing and 2 partials ⚠️
...nternal/controllers/clusterextension_controller.go	92.50%	3 Missing ⚠️
...al/resolution/variablesources/installed_package.go	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
- Coverage   84.53%   80.98%   -3.55%     
==========================================
  Files          20       20              
  Lines         931      936       +5     
==========================================
- Hits          787      758      -29     
- Misses        100      130      +30     
- Partials       44       48       +4

Flag	Coverage Δ
e2e	`59.29% <46.15%> (-0.54%)`	⬇️
unit	`76.19% <80.26%> (-4.11%)`	⬇️

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

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

cmd/resolutioncli/client.go

stevekuznetsov

I am unfortunately too far from this code to fully grok what is going on here. At the surface level the transform seems reasonable.

m1kola · 2024-02-01T10:50:40Z

internal/catalogmetadata/types.go

-}
-
-func (b *Bundle) RequiredPackages() ([]PackageRequired, error) {
-	if err := b.loadRequiredPackages(); err != nil {


I think we had these load* to avoid unmarshaling properties multiple times. E.g. RequiredPackages can be called 10 times during resolution process and ideally we don't want to unmarshal 10 times.

Is that a realistic example though? I looked at all the references to this function and there were a total of 2 with one being in tests. The only reference to this function getting called outside of tests is:

operator-controller/internal/resolution/variablesources/bundle.go

Line 66 in d51dfe6

requiredPackages, _ := bundle.RequiredPackages()

and that function is called once for every bundle, which means every bundle is "loading" the required packages a singular time and that is it. Doesn't seem worth the additional complexity to optimize this when in the current state of the world it only gets called once for each bundle and it isn't taking advantage of that optimization.

That being said, you are more knowledgable in this space so if you have other insights that prove my assumptions wrong I'm happy to continue the discussion and make any optimizations that are worthwhile.

that function is called once for every bundle

It is not always the case. Some functions can potentially be called multiple times on the single bundle. While RequiredPackages is guarded by the visited set here and most likely won't be called multiple times in the code as it currently stands, other functions might still get multiple calls. For example, I believe Version will be called every time we do filtering here.

Overall we do quite a lot of filtering on allBundles. Every time we filter on a property that needs unmarshaling - we can benefit from the optimisation.

I personally don't see where we do enough filtering within the existing logic on properties that need unmarshaling that make me concerned about unmarshaling more than once but I'm also not that well versed on how intensive unmarshaling is.

Not a hill I'm willing to die on so I added it back in (although didn't recreate the load* functions as it feels redundant IMO): 94fe3df

m1kola · 2024-02-01T11:07:51Z

internal/catalogmetadata/client/client.go

+							bundle.Deprecation = &declcfg.DeprecationEntry{
+								Reference: entry.Reference,
+								Message:   entry.Message,
+							}


If I remember our conversation from January correctly, we decided to get rid of fields like InChannels and try to keep original FBC/declcfg stucts as much as possible. I see that InChannels is now gone, but Deprecation field on Bundle and other structs was added. I think it is the same same concept.

Should we explore adding some maps in places where we need to do lookups of deprecation entries? Or something similar what you did to channels (using filters).

I similarly recall the conversation as keeping the wrapper as simple as possible if one is necessary (at the very least we need one for that catalog name since the declcfg.* representations don't store catalog information). I recall @joelanford mentioning adding the deprecation entry to the wrapper as well in previous conversations so I took that approach as it was one less set of information to deal with after the fact. I'm not opposed to either of the approaches though so this is more or less a "let's pick one and I'll implement it" situation.

The benefit of this approach is that there is no lookup cost after the set of packages, channels, and bundles are returned but there are definitely other solutions where the lookup cost is negligible that I'm open to exploring.

I don't remember the context of my suggestion of putting the deprecation entry in the wrapper, but I think I'd suggest separating it out and keeping the structure similar to the FBC structure.

m1kola · 2024-02-01T11:29:27Z

internal/resolution/variablesources/installed_package.go

+		channels := filter.Filter[catalogmetadata.Channel](allChannels, filter.And[catalogmetadata.Channel](
+			func(entity *catalogmetadata.Channel) bool {
+				return entity.Package == clusterExtension.Spec.PackageName
+			},
+			func(entity *catalogmetadata.Channel) bool {
+				if clusterExtension.Spec.Channel != "" {
+					return entity.Name == clusterExtension.Spec.Channel
+				}
+				return true
+			},
+		))


Wondering if we can build a map of package -> channel once and re-used it multiple times to avoid iterating trough all the channel date for each call to this function.

We certainly could, but I wasn't sure if that optimization is worth the overhead / additional complexity within the client logic. I'm definitely open to discussing optimization in this PR, but also want to make sure we aren't prematurely trying to optimize and dig ourselves into a different hole of complexity. Especially since there has been mention of potentially not even needing to have a sat solver that could result in needing to refactor a large chunk of this logic again.

Signed-off-by: everettraven <everettraven@gmail.com>

openshift-merge-robot · 2024-02-21T22:59:07Z

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-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 Jan 29, 2024

refactor catalogmetadata types used for resolution

e37bb1c

Signed-off-by: everettraven <everettraven@gmail.com>

everettraven force-pushed the updates/resolution branch from 71fe718 to e37bb1c Compare January 31, 2024 18:30

everettraven changed the title ~~WIP: reduce complexity of bundle type used to build resolution variables~~ 🌱 refactor catalogmetadata types used for resolution Jan 31, 2024

everettraven marked this pull request as ready for review January 31, 2024 18:43

everettraven requested a review from a team as a code owner January 31, 2024 18:43

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 Jan 31, 2024

stevekuznetsov reviewed Jan 31, 2024

View reviewed changes

cmd/resolutioncli/client.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Jan 31, 2024

View reviewed changes

m1kola reviewed Feb 1, 2024

View reviewed changes

everettraven added 2 commits February 2, 2024 09:22

add lazy loading/caching back

94fe3df

Signed-off-by: everettraven <everettraven@gmail.com>

better type checking

32466fa

Signed-off-by: everettraven <everettraven@gmail.com>

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024

joelanford mentioned this pull request Feb 29, 2024

✨ Integrate App with Extension reconciler #625

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 refactor catalogmetadata types used for resolution #599

🌱 refactor catalogmetadata types used for resolution #599

everettraven commented Jan 29, 2024 •

edited

everettraven commented Jan 29, 2024

codecov bot commented Jan 31, 2024 •

edited

stevekuznetsov left a comment

m1kola Feb 1, 2024

everettraven Feb 1, 2024 •

edited

m1kola Feb 2, 2024 •

edited

everettraven Feb 2, 2024

m1kola Feb 1, 2024

everettraven Feb 1, 2024

joelanford Apr 3, 2024

m1kola Feb 1, 2024

everettraven Feb 1, 2024

openshift-merge-robot commented Feb 21, 2024

🌱 refactor catalogmetadata types used for resolution #599

Are you sure you want to change the base?

🌱 refactor catalogmetadata types used for resolution #599

Conversation

everettraven commented Jan 29, 2024 • edited

Description

Reviewer Checklist

everettraven commented Jan 29, 2024

codecov bot commented Jan 31, 2024 • edited

Codecov Report

stevekuznetsov left a comment

Choose a reason for hiding this comment

m1kola Feb 1, 2024

Choose a reason for hiding this comment

everettraven Feb 1, 2024 • edited

Choose a reason for hiding this comment

m1kola Feb 2, 2024 • edited

Choose a reason for hiding this comment

everettraven Feb 2, 2024

Choose a reason for hiding this comment

m1kola Feb 1, 2024

Choose a reason for hiding this comment

everettraven Feb 1, 2024

Choose a reason for hiding this comment

joelanford Apr 3, 2024

Choose a reason for hiding this comment

m1kola Feb 1, 2024

Choose a reason for hiding this comment

everettraven Feb 1, 2024

Choose a reason for hiding this comment

openshift-merge-robot commented Feb 21, 2024

everettraven commented Jan 29, 2024 •

edited

codecov bot commented Jan 31, 2024 •

edited

everettraven Feb 1, 2024 •

edited

m1kola Feb 2, 2024 •

edited