⚠️ Remove Deppy solver #758

m1kola · 2024-04-17T09:19:56Z

Description

An attempt to remove Deppy and replace it with a simple catalog filtering. This also removes resolution CLI.

With this change we:

No longer check/solving bundle dependencies
No longer check bundle uniqueness on the cluster

Reviewer Checklist

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

netlify · 2024-04-17T09:20:12Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`0b5ae33`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/662fbe218119fd00082a7e14
😎 Deploy Preview	https://deploy-preview-758--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tmshort · 2024-04-17T14:31:50Z

Did you want to create a separate branch for this? e.g. rm-deppy-poc?

m1kola · 2024-04-17T15:08:54Z

Did you want to create a separate branch for this? e.g. rm-deppy-poc?

No, just accidentally removed the remote branch and GH closed the PR.

codecov · 2024-04-17T15:15:43Z

Codecov Report

Attention: Patch coverage is 79.38144% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 64.15%. Comparing base (32ed986) to head (0b5ae33).

Files	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	83.33%	5 Missing and 5 partials ⚠️
internal/controllers/successors.go	72.97%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #758      +/-   ##
==========================================
- Coverage   67.21%   64.15%   -3.06%     
==========================================
  Files          23       16       -7     
  Lines        1467     1303     -164     
==========================================
- Hits          986      836     -150     
+ Misses        415      404      -11     
+ Partials       66       63       -3

Flag	Coverage Δ
e2e	`41.82% <62.88%> (-3.71%)`	⬇️
unit	`56.94% <76.28%> (-4.40%)`	⬇️

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.

tmshort · 2024-04-17T15:30:23Z

But, if this is a PoC, you don't want it merged...?

m1kola · 2024-04-17T15:40:51Z

For now I'll keep in the draft. Eventually we can either split it into smaller PRs (if it is in a good enough state) or close it and use as a reference for proper removal.

internal/controllers/successors.go

internal/controllers/clusterextension_controller.go

internal/controllers/clusterextension_controller_test.go

internal/controllers/clusterextension_controller.go

internal/controllers/successors.go

internal/controllers/clusterextension_controller.go

tmshort · 2024-04-18T17:35:52Z

Looks reasonable at first pass.

varshaprasad96

@m1kola The changes look good, just one nit.

Thanks for cleaning up the solver code and fixing cluster extension to work without it!

varshaprasad96 · 2024-04-18T20:16:26Z

internal/controllers/clusterextension_controller.go

+		if err != nil {
+			return nil, err
+		}
+		upgradeErrorPrefix = fmt.Sprintf("error upgrading from currently installed version %q: ", installedBundleVersion.String())


Nit: upgradeErrorPrefix can be defined as a separate error type globally, and can then be wrapped/formatted to include the bundle version, along with other respective error info like version range, or channel name in the condition. This way it's easier to check for this error with errors.Is for the client.

Wrapping errors makes them part of the API and I believe we currently do not have a use case for type checking. I suggest to leave errors opaque and add types, if/when we need them.

internal/controllers/clusterextension_controller.go

m1kola · 2024-04-19T12:18:56Z

internal/controllers/successors_test.go

@@ -1,7 +1,8 @@
-package variablesources_test
+package controllers_test


It might be easier to review this file with "hide whitespace" option (-w in git diff).

m1kola · 2024-04-19T14:19:27Z

Converted back to draft while we are looking into solving package uniqueness for ClusterExtension.

I had a chat with @joelanford about package uniqueness and we decided that it is best to not allow ClusterExtension to install the same package multiple times.

We will look into whether we can achieve that with ValidatingAdmissionPolicy. In the meantime I'm putting this on hold.

The plan is:

Introduce validating admission for uniqueness
Merge this PR (without uniqueness on the controller level)

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola · 2024-04-29T15:45:13Z

#774 and #785 are now merged. I rebased the PR to solve conflicts. Should be ready for review now.

varshaprasad96

The resolver removals and version filter additions look good to me!
/lgtm

joelanford · 2024-04-29T21:34:25Z

internal/controllers/clusterextension_controller.go

-	clusterExtensionList := ocv1alpha1.ClusterExtensionList{}
-	if err := r.Client.List(ctx, &clusterExtensionList); err != nil {
+
+	installedBundle, err := r.installedBundle(ctx, allBundles, ext)


At some point, I know we talked about adding some labels or annotations to the BD for the installed package name and version and using that in order to derive successors.

Looking on the removed code in the installed_package variable source, I see that we never made that change. So I think it's fine to stick with that same logic like we are in this PR. But I also think we should capture a follow-up issue (if we don't already have one), to implement the installed bundle detection that way.

Somewhat related: I don't think we should have to find the existing installed bundle in the catalog in order to be able to upgrade from it.

At some point, I know we talked about adding some labels or annotations to the BD for the installed package name and version and using that in order to derive successors.

Looking on the removed code in the installed_package variable source, I see that we never made that change. So I think it's fine to stick with that same logic like we are in this PR. But I also think we should capture a follow-up issue (if we don't already have one), to implement the installed bundle detection that way.

@joelanford I can't find the thread in upstream slack, but I think we ended up deciding against using labels and in favour of digests which were supposed to support non-image sources. We have this issue for non-image bundle types support and the idea with digests captured here.

Somewhat related: I don't think we should have to find the existing installed bundle in the catalog in order to be able to upgrade from it.

I might be missing something, but in order to support both server and legacy semantics we seem to need the following data:

Package name (for both server and legacy)

Current version (for both server and legacy)

Bundle name (for legacy)

Now that we have #679 we might be able to get this info from ClusterExtensions's status, but we probably don't want to to read from ClusterExtensions's status while reconciling ClusterExtensions.

Even if we store this data in labels and use on upgrades - it will be an equivalent of looking at ClusterExtensions's status.

Agreed we should not read from ClusterExtension status. I think @varshaprasad96 is planning to include that information as labels on the Helm release secret. Prior to the helm integration landing, we could include that information as BD labels or annotations.

I could have sworn we added BD annotations for that info already, but maybe not.

@joelanford I think you did it in your PoC a while ago and I had a draft PR which we then closed in favour of the digest idea, but we didn't get to implement the the digest idea.

joelanford

Left one comment about capturing a follow-up. Otherwise LGTM to merge this as is.

grokspawn · 2024-05-01T15:13:08Z

Left one comment about capturing a follow-up. Otherwise LGTM to merge this as is.

I'd merge this, but I'd like to make sure that a follow-up need is captured somewhere first. @m1kola is there an issue/pr for this yet? Could you create one (if not already existing) and mention it here?

m1kola · 2024-05-01T15:19:49Z

I'd merge this, but I'd like to make sure that a follow-up need is captured somewhere first. @m1kola is there an issue/pr for this yet? Could you create one (if not already existing) and mention it here?

@grokspawn we already have something that captures this. See #758 (comment) for more details.

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 Apr 17, 2024

m1kola closed this Apr 17, 2024

m1kola deleted the remove_resolver branch April 17, 2024 10:23

m1kola reopened this Apr 17, 2024

m1kola force-pushed the remove_resolver branch from 849ca7e to f9ac71e Compare April 17, 2024 14:54

m1kola force-pushed the remove_resolver branch from f9ac71e to 9921b8e Compare April 17, 2024 15:10

m1kola force-pushed the remove_resolver branch from 9921b8e to 48c65a5 Compare April 17, 2024 16:15

m1kola commented Apr 17, 2024

View reviewed changes

internal/controllers/successors.go Show resolved Hide resolved

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

internal/controllers/clusterextension_controller_test.go Show resolved Hide resolved

varshaprasad96 reviewed Apr 17, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

internal/controllers/successors.go Show resolved Hide resolved

m1kola force-pushed the remove_resolver branch 5 times, most recently from 4f85d7a to 95d809a Compare April 18, 2024 16:28

m1kola commented Apr 18, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

varshaprasad96 previously approved these changes Apr 18, 2024

View reviewed changes

m1kola commented Apr 19, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

m1kola dismissed varshaprasad96’s stale review via 7393944 April 19, 2024 11:23

m1kola force-pushed the remove_resolver branch from 95d809a to 7393944 Compare April 19, 2024 11:23

m1kola changed the title ~~⚠️ PoC: Remove Deppy solver~~ ⚠️ Remove Deppy solver Apr 19, 2024

m1kola marked this pull request as ready for review April 19, 2024 12:16

m1kola requested a review from a team as a code owner April 19, 2024 12:16

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 Apr 19, 2024

m1kola commented Apr 19, 2024

View reviewed changes

m1kola marked this pull request as draft April 19, 2024 14:16

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 Apr 19, 2024

This was referenced Apr 22, 2024

🌱 Enable admissionregistration.k8s.io/v1beta1 #771

Merged

✨ Add ValidatingAdmissionPolicy to enforce package uniqueness across ClusterExtension #774

Merged

m1kola force-pushed the remove_resolver branch 2 times, most recently from 28733c2 to d7d0522 Compare April 25, 2024 15:29

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

m1kola added 2 commits April 29, 2024 17:34

Replace solver with simple catalog filtering

6b701a7

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

Remove solver leftovers

0b5ae33

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola force-pushed the remove_resolver branch from d7d0522 to 0b5ae33 Compare April 29, 2024 15:34

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2024

m1kola marked this pull request as ready for review April 29, 2024 15:45

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

m1kola requested a review from varshaprasad96 April 29, 2024 15:47

varshaprasad96 approved these changes Apr 29, 2024

View reviewed changes

openshift-ci bot assigned varshaprasad96 Apr 29, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2024

joelanford reviewed Apr 29, 2024

View reviewed changes

joelanford approved these changes Apr 29, 2024

View reviewed changes

m1kola mentioned this pull request Apr 30, 2024

[epic] Port relevant Extension reconciler logic over to ClusterExtension #747

Closed

m1kola added this pull request to the merge queue May 2, 2024

Merged via the queue into operator-framework:main with commit e37a2e6 May 2, 2024
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Remove Deppy solver #758

⚠️ Remove Deppy solver #758

m1kola commented Apr 17, 2024 •

edited

Loading

netlify bot commented Apr 17, 2024 •

edited

Loading

tmshort commented Apr 17, 2024

m1kola commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

tmshort commented Apr 17, 2024

m1kola commented Apr 17, 2024

tmshort commented Apr 18, 2024

varshaprasad96 left a comment

varshaprasad96 Apr 18, 2024

m1kola Apr 19, 2024

m1kola Apr 19, 2024

m1kola commented Apr 19, 2024

m1kola commented Apr 29, 2024

varshaprasad96 left a comment

joelanford Apr 29, 2024

m1kola Apr 30, 2024

joelanford May 1, 2024

m1kola May 1, 2024

joelanford left a comment

grokspawn commented May 1, 2024

m1kola commented May 1, 2024

		@@ -1,7 +1,8 @@
		package variablesources_test
		package controllers_test

⚠️ Remove Deppy solver #758

⚠️ Remove Deppy solver #758

Conversation

m1kola commented Apr 17, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Apr 17, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

tmshort commented Apr 17, 2024

m1kola commented Apr 17, 2024 • edited Loading

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

tmshort commented Apr 17, 2024

m1kola commented Apr 17, 2024

tmshort commented Apr 18, 2024

varshaprasad96 left a comment

Choose a reason for hiding this comment

varshaprasad96 Apr 18, 2024

Choose a reason for hiding this comment

m1kola Apr 19, 2024

Choose a reason for hiding this comment

m1kola Apr 19, 2024

Choose a reason for hiding this comment

m1kola commented Apr 19, 2024

m1kola commented Apr 29, 2024

varshaprasad96 left a comment

Choose a reason for hiding this comment

joelanford Apr 29, 2024

Choose a reason for hiding this comment

m1kola Apr 30, 2024

Choose a reason for hiding this comment

joelanford May 1, 2024

Choose a reason for hiding this comment

m1kola May 1, 2024

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

grokspawn commented May 1, 2024

m1kola commented May 1, 2024

m1kola commented Apr 17, 2024 •

edited

Loading

netlify bot commented Apr 17, 2024 •

edited

Loading

m1kola commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading