Makes `OLMVariableSource` responsible for fetching #245

m1kola · 2023-06-01T13:54:24Z

Description

OLMVariableSource now contains the client and makes requests to get available operators.

Doing this:

Makes OperatorResolver wrapper around deppy solver unnecessary: we can use solver.DeppySolver directly in the controller. I kept it for now, but I think we can remove it as a follow up and replaced by solver.DeppySolver directly.
Makes it easier to implement Dependency Library + CLI #206 since OperatorResolver no longer contains logic (it does not instantiate variable sources and underlying solver in the Resolve method). But further refactoring still required in order to achieve the above task (e.g. need to get rid of this)

Reviewer Checklist

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

internal/resolution/resolver.go

m1kola · 2023-06-01T14:06:31Z

internal/resolution/variable_sources/olm/olm_test.go

@@ -109,15 +130,17 @@ var _ = Describe("OLMVariableSource", func() {
 				out = append(out, variable.BundleEntity().Entity)
 			}
 			return out
-		}, Equal([]*input.Entity{
+		}, ConsistOf([]*input.Entity{


I believe the order of variables returned from olmVariableSource.GetVariables is not important here.
Since it is now using a client to fetch operators - order is different now and tests were relying on order.

This sounds like an important question to resolve at the public-facing level - if the order is not guaranteed, it should be noted in the API docs. If it is, a simple sort-in-place can be added.

This sounds like an important question to resolve at the public-facing level - if the order is not guaranteed, it should be noted in the API docs.

We are not defining an API here - we are implementing a contract/interface required by Deppy. Solver will be calling GetVariables and as far as I see it doesn't require variable s to be sorted in a specific way.

https://github.com/operator-framework/deppy/blob/9a7655cd364b3900d394f81a752731200e2d01a4/pkg/deppy/input/variable_source.go#L9-L12

IMO nether sorting variables nor documenting ordering on the operator-controller side is necessary.

Solver will be calling GetVariables and as far as I see it doesn't require variable s to be sorted in a specific way.

Want to clarify: there are some places where the order is very important for the resolution process. For example, we want to select latest possible version for the solution.

But I think here it is not important. However thinking about debuggability - it still might be a good idea to have some stable order.

I think I was partially wrong here:

olmVariableSource.GetVariables now returns in different order. This is mostly due to a fake client: sigs.k8s.io/controller-runtime/pkg/client/fake doesn't guarantee stable order.

I looked into this a bit more. It looks like the fake client does not guarantee order (at least I do not see anything about this in docs), but de facto it does sorting (we hit this code eventually).

So in the test we add prometheus and then packageA into the fake, but when GetVariables calls the client - it returns packageA first and prometheus because the result list gets sorted.

Our options here:

Change ordering of the expected results taking undocumented sorting of fake client into account. This will make the diff minimal. But I think this is not ideal - it will lead to a hard to understand failure, if underlying implementation of the fake client changes.

Do sorting in GetVariables after calling the client. This is only required for making the test simplier. I think it is not right to introduce sorting in the code which we test just to make the test code a little bit simplier.

Change tests (what we currently have in this PR) to make sure that we do not rely on order provided by the fake client. I think this is the best approach given that we do not need to guarantee order from GetVariables.

Create a fake client which maintains order of objects provided in the constructor when returning a List result. Not sure this has any benefit: we will have to introduce extra testing code to avoid changing existing testing code (assertions).

However thinking about debuggability - it still might be a good idea to have some stable order.

Since we now know that fake client returns in deterministic order (not documented, but still) - I think there is little to no benefit in sorting.

If sort order is immaterial except for testing, I agree that your third option is the best.

stevekuznetsov

I'm not super-familiar with all the internals here but the purpose of this refactor looks great!

internal/controllers/operator_controller_test.go

internal/resolution/resolver.go

internal/resolution/resolver_test.go

internal/resolution/variable_sources/olm/olm_test.go

stevekuznetsov · 2023-06-01T16:44:44Z

internal/resolution/variable_sources/olm/olm_test.go

@@ -109,15 +130,17 @@ var _ = Describe("OLMVariableSource", func() {
 				out = append(out, variable.BundleEntity().Entity)
 			}
 			return out
-		}, Equal([]*input.Entity{
+		}, ConsistOf([]*input.Entity{


This sounds like an important question to resolve at the public-facing level - if the order is not guaranteed, it should be noted in the API docs. If it is, a simple sort-in-place can be added.

`OLMVariableSource` now contains the client and makes requests to get available operators. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

varshaprasad96

/lgtm

The changes look good to me. But I'm curious about the extensibility of variable source. Previously, since we had a wrapper around Deppy's Resolver, we could technically pass multiple entity sources for the resolution. Ie, in here, the OperatorResolver struct could contain multiple entity Sources, and the resolve method could accordingly transform them into variableSources. Now looking at it, I realize that we are restricting the resolver to fetch just one variable and entity source.

The other aspect which seems evident now, is the need for entitySource [issue]. EntitySource seems to be getting transformed to variable source with GetVariables in Resolve. This means there is technically no need for both of them to be passed. This has been discussed before but with this change it seems more evident :)

m1kola · 2023-06-08T15:07:02Z

@varshaprasad96 I think a lot of this will change soon-ish. We need to agree on the method of building problem for the solver. @perdasilva presented an RFC on this topic during the recent community meeting.

For now I'm trying to work with what we have today. For example in #246 I created a couple of composite variable sources to be able to combine variable sources (see internal/resolution/variable_sources/olm/composite.go here and internal/resolution/variable_sources/olm/olm.go here for how it used). But I expect this to be replaced eventually with different method of building the resolution problem.

varshaprasad96 · 2023-06-08T15:17:01Z

@m1kola that's fair. The accumulator perspective which I was thinking is similar to as you mentioned here. Which is why I was curious on whether it would make sense to move it to top level and extend the possibility of having multiple variable sources directly. But agreed, since this is a changing API with new architectures, most of this thought might look differently in future.

m1kola commented Jun 1, 2023

View reviewed changes

internal/resolution/resolver.go Outdated Show resolved Hide resolved

m1kola commented Jun 1, 2023

View reviewed changes

m1kola requested review from perdasilva and joelanford June 1, 2023 14:06

stevekuznetsov reviewed Jun 1, 2023

View reviewed changes

m1kola mentioned this pull request Jun 1, 2023

PoC: Resolution CLI #246

Merged

4 tasks

m1kola force-pushed the variable_source_cleanup branch from c4523a6 to 754fc5a Compare June 2, 2023 10:29

m1kola requested a review from stevekuznetsov June 2, 2023 12:54

m1kola force-pushed the variable_source_cleanup branch from 754fc5a to b397e9d Compare June 2, 2023 15:59

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

m1kola force-pushed the variable_source_cleanup branch from b397e9d to d8de4f8 Compare June 2, 2023 16:02

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

Makes OLMVariableSource responsible for fetching

365253c

`OLMVariableSource` now contains the client and makes requests to get available operators. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola force-pushed the variable_source_cleanup branch from d8de4f8 to 365253c Compare June 8, 2023 12:17

varshaprasad96 approved these changes Jun 8, 2023

View reviewed changes

openshift-ci bot assigned varshaprasad96 Jun 8, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023

m1kola merged commit b418973 into operator-framework:main Jun 8, 2023
5 checks passed

m1kola deleted the variable_source_cleanup branch June 8, 2023 15:08

ankitathomas mentioned this pull request Jun 15, 2023

Update resolution package tests #191

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes `OLMVariableSource` responsible for fetching #245

Makes `OLMVariableSource` responsible for fetching #245

m1kola commented Jun 1, 2023

m1kola Jun 1, 2023

stevekuznetsov Jun 1, 2023

m1kola Jun 2, 2023

m1kola Jun 7, 2023

m1kola Jun 8, 2023

stevekuznetsov Jun 8, 2023

stevekuznetsov left a comment

stevekuznetsov Jun 1, 2023

varshaprasad96 left a comment

m1kola commented Jun 8, 2023

varshaprasad96 commented Jun 8, 2023 •

edited

Loading

Makes OLMVariableSource responsible for fetching #245

Makes OLMVariableSource responsible for fetching #245

Conversation

m1kola commented Jun 1, 2023

Description

Reviewer Checklist

m1kola Jun 1, 2023

Choose a reason for hiding this comment

stevekuznetsov Jun 1, 2023

Choose a reason for hiding this comment

m1kola Jun 2, 2023

Choose a reason for hiding this comment

m1kola Jun 7, 2023

Choose a reason for hiding this comment

m1kola Jun 8, 2023

Choose a reason for hiding this comment

stevekuznetsov Jun 8, 2023

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

stevekuznetsov Jun 1, 2023

Choose a reason for hiding this comment

varshaprasad96 left a comment

Choose a reason for hiding this comment

m1kola commented Jun 8, 2023

varshaprasad96 commented Jun 8, 2023 • edited Loading

Makes `OLMVariableSource` responsible for fetching #245

Makes `OLMVariableSource` responsible for fetching #245

varshaprasad96 commented Jun 8, 2023 •

edited

Loading