🐛: Improve Catalog Selection Testing + Fix small issue #1174
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1174 +/- ##
==========================================
+ Coverage 75.33% 76.93% +1.60%
==========================================
Files 35 35
Lines 1934 1934
==========================================
+ Hits 1457 1488 +31
+ Misses 331 311 -20
+ Partials 146 135 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -114,7 +114,7 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx | |||
| if len(resolvedBundles) != 0 { | |||
| // We've already found one or more package candidates | |||
| currentIsDeprecated := isDeprecated(thisBundle, thisDeprecation) | |||
| priorIsDeprecated := isDeprecated(*resolvedBundles[0], priorDeprecation) // Slice index doesn't matter; the whole slice is either deprecated or not | |||
| priorIsDeprecated := isDeprecated(*resolvedBundles[len(resolvedBundles)-1], priorDeprecation) | |||
There was a problem hiding this comment.
Am I correct in understanding that the expectation is that if the previously installed package is deprecated we expect it to be at the end of the slice of resolved bundles?
How would this work if there are more than one deprecated resolved bundles and the previously installed bundle is of higher semver than other deprecated bundles?
There was a problem hiding this comment.
The package at the end of the slice isn't the currently installed package, it's just the latest package we found as an acceptable candidate from this or another catalog.
But regardless, in your scenario, if the user isn't filtering out packages of a lower version (UpgradeConstraintPolicyIgnore=true) and our resolver finds multiple packages that are also deprecated then we would have an ambiguous resolution i.e. we will not decide for the user which package in which catalog we will downgrade to.
internal/resolve/catalog_test.go
Outdated
| } | ||
| return fbc, nil | ||
| }, | ||
| } | ||
| r := CatalogResolver{WalkCatalogsFunc: w.WalkCatalogs} | ||
|
|
||
| t.Run("when catalog b has a newer version that matches the range", func(t *testing.T) { | ||
| t.Run("when package is deprecated in all but one catalog", func(t *testing.T) { |
There was a problem hiding this comment.
Based on the test string, this seems like it is when the entire package is deprecated in all but one catalog but seems to be testing when a specific bundle is deprecated in all but one catalog.
What happens if there are multiple bundles, some with deprecations and some without?
As an example:
- Given package
fooand version range1.x - Given catalog A contains package
foowith bundles1.0.0,1.1.0marked as deprecated but1.2.0,1.3.0not deprecated - Given catalog B contains package
foowith bundles1.0.0,1.1.0and1.2.0marked as deprecated but1.3.0and1.4.0not deprecated
What happens?
There was a problem hiding this comment.
RETROACTIVE EDIT - fixed this comment to reflect the actual behavior
You're right on the wording, I'm conflating package and bundle here. I'll fix that :)
In your scenario we would give an ambiguous resolution because we have multiple resolved candidates from multiple catalogs:
1.3.0from catalogA1.4.0from catalogB
The deprecated bundles won't be considered at all since we encountered greater than zero non-deprecated bundles.
In more fine-grained detail:
- We check catalog A
- filter and sort the bundle candidates
- top bundle:
1.3.0- not deprecated and highest version - add it to the resolved list.
- We check catalog B
- filter and sort the bundle candidates
- top bundle:
1.4.0- not deprecated and highest version - add it to the list
So we finish resolution with candidates 1.3.0(A), 1.4.0(B), and the error generated would be matching packages found in multiple catalogs: [A B]
You made me realize though, and maybe this was your intent, we need to adjust the error when there are multiple resolved packages in one catalog. But then again, my understanding from the RFC was that we were removing the highest-semver decision entirely, but looking at this again it seems like we should keep highest-semver when we only have one catalog to choose from, right?
There was a problem hiding this comment.
Within a catalog, it should be highest semver, there should only be one return from a given catalog. Since we support automatic updates, this has to be the case. The ambiguity comes from when there are two operators in different catalogs, and we're not sure which operator the user meant, regardless of semver.
So, in @everettraven's example, we should have:
- Picked
foo-1.3.0from catalogA: 1.0.0 and 1.1.0 are deprecated, and thus ignored when 1.2.0 is found. 1.3.0 takes precedence over 1.2.0 due to semver. - Picked
foo-1.4.0from catalogB. 1.0.0, 1.1.0, and 1.2.0 are deprecated, and thus ignored when 1.3.0 is found. 1.4.0 takes precedence over 1.3.0 due to semver.
Now, there are two bundles with the same name, but different versions.
So, I don;'t think we can completely get rid of semver, because we might run into the the situation you have (i.e. one catalog with multiple candidates), and we should pick the highest semver (that fulfills the version requirement).
So, perhaps the resolvedBundles array needs to be a map indexed by catalog? Or something similar? Still have to deal with deprecated though. Before these changes, only one bundle was ever returned, either from 1 catalog or n catalogs. I think we need to choose the best bundle from each catalog, and then make sure we don't have ambiguity after that process.
There was a problem hiding this comment.
TL;DR: pick the best bundle from each catalog; if there are multiple results, then look at deprecation status. If there are multiple non-deprecated or all bundles are deprecated and there are multiple deprecated, then there's ambiguity, and we error out.
So, if there is a deprecated bundle from catalog A, and a non-deprecated bundle from catalog B, the bundle from catalog B wins.
There was a problem hiding this comment.
Apologies to you both - I confused myself here and forgot some key detail.
When we're looking at one catalog, if we get multiple bundle candidates, we sort them by semver and deprecation.
That means that each time we run this func, we're already choosing the best candidate per catalog, and there's no change needed.
The key thing here is that if more than one catalog is giving us back a "best candidate", and all the candidates share deprecated or non-deprecated status, that's when we give the error. The code already takes care of this well, AFAICT.
Of course if either of you still think I'm missing something here let me know! In the meantime though I'll add another test case to hopefully make this clearer.
There was a problem hiding this comment.
There was a problem hiding this comment.
All of this sounds and looks good to me. I mostly wanted to verify my understanding of the expected behaviors, so thank you both for the detailed answers!
There was a problem hiding this comment.
/lgtm
Unless @everettraven's issues have not been resolved.
We initially assumed that the resolvedBundle index we check doesn't matter when it comes to checking for prior package deprecations in the resolver but the index needs to match the prior result or the bundle names may not match and give a false negative. Also improved the unit test cases (which caught the above issue). Signed-off-by: dtfranz <[email protected]>
dtfranz
force-pushed
the
improve-ambiguous-res-tests
branch
from
96019b6 to
1b3eb02
Compare
We initially assumed that the resolvedBundle index we check doesn't matter when it comes to checking for prior package deprecations in the resolver but the index needs to match the prior result or the bundle names may not match and give a false negative.
Also improved the unit test cases (which caught the above issue).
Description
Reviewer Checklist