Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Makes OLMVariableSource responsible for fetching #245

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Makes OLMVariableSource responsible for fetching #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"flag"
"os"

"github.com/operator-framework/deppy/pkg/deppy/solver"
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
"go.uber.org/zap/zapcore"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -33,8 +34,8 @@ import (
catalogd "github.com/operator-framework/catalogd/pkg/apis/core/v1beta1"
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/controllers"
"github.com/operator-framework/operator-controller/internal/resolution"
"github.com/operator-framework/operator-controller/internal/resolution/entitysources"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/olm"
)

var (
Expand Down Expand Up @@ -94,9 +95,12 @@ func main() {
}

if err = (&controllers.OperatorReconciler{
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
Resolver: resolution.NewOperatorResolver(mgr.GetClient(), entitysources.NewCatalogdEntitySource(mgr.GetClient())),
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
Resolver: solver.NewDeppySolver(
entitysources.NewCatalogdEntitySource(mgr.GetClient()),
olm.NewOLMVariableSource(mgr.GetClient()),
),
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "Operator")
os.Exit(1)
Expand Down
5 changes: 2 additions & 3 deletions internal/controllers/operator_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import (

operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/controllers/validators"
"github.com/operator-framework/operator-controller/internal/resolution"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/bundles_and_dependencies"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/entity"
)
Expand All @@ -50,7 +49,7 @@ import (
type OperatorReconciler struct {
client.Client
Scheme *runtime.Scheme
Resolver *resolution.OperatorResolver
Resolver *solver.DeppySolver
}

//+kubebuilder:rbac:groups=operators.operatorframework.io,resources=operators,verbs=get;list;watch
Expand Down Expand Up @@ -122,7 +121,7 @@ func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha
return ctrl.Result{}, nil
}
// run resolution
solution, err := r.Resolver.Resolve(ctx)
solution, err := r.Resolver.Solve(ctx)
if err != nil {
op.Status.InstalledBundleResource = ""
setInstalledStatusConditionUnknown(&op.Status.Conditions, "installation has not been attempted as resolution failed", op.GetGeneration())
Expand Down
5 changes: 3 additions & 2 deletions internal/controllers/operator_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
. "github.com/onsi/gomega"
"github.com/operator-framework/deppy/pkg/deppy"
"github.com/operator-framework/deppy/pkg/deppy/input"
"github.com/operator-framework/deppy/pkg/deppy/solver"
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -21,7 +22,7 @@ import (
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/conditionsets"
"github.com/operator-framework/operator-controller/internal/controllers"
"github.com/operator-framework/operator-controller/internal/resolution"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/olm"
)

var _ = Describe("Operator Controller Test", func() {
Expand All @@ -34,7 +35,7 @@ var _ = Describe("Operator Controller Test", func() {
reconciler = &controllers.OperatorReconciler{
Client: cl,
Scheme: sch,
Resolver: resolution.NewOperatorResolver(cl, testEntitySource),
Resolver: solver.NewDeppySolver(testEntitySource, olm.NewOLMVariableSource(cl)),
}
})
When("the operator does not exist", func() {
Expand Down
43 changes: 0 additions & 43 deletions internal/resolution/resolver.go
View file
Open in desktop

This file was deleted.

23 changes: 14 additions & 9 deletions internal/resolution/resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
. "github.com/onsi/gomega"
"github.com/operator-framework/deppy/pkg/deppy"
"github.com/operator-framework/deppy/pkg/deppy/input"
"github.com/operator-framework/deppy/pkg/deppy/solver"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

"github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/resolution"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/olm"
)

func TestOperatorResolver(t *testing.T) {
Expand Down Expand Up @@ -74,8 +75,9 @@ var _ = Describe("OperatorResolver", func() {
}
client := FakeClient(resources...)
entitySource := input.NewCacheQuerier(testEntityCache)
resolver := resolution.NewOperatorResolver(client, entitySource)
solution, err := resolver.Resolve(context.Background())
variableSource := olm.NewOLMVariableSource(client)
resolver := solver.NewDeppySolver(entitySource, variableSource)
solution, err := resolver.Solve(context.Background())
Expect(err).ToNot(HaveOccurred())
// 2 * required package variables + 2 * bundle variables
Expect(solution.SelectedVariables()).To(HaveLen(4))
Expand All @@ -93,8 +95,9 @@ var _ = Describe("OperatorResolver", func() {
var resources []client.Object
client := FakeClient(resources...)
entitySource := input.NewCacheQuerier(testEntityCache)
resolver := resolution.NewOperatorResolver(client, entitySource)
solution, err := resolver.Resolve(context.Background())
variableSource := olm.NewOLMVariableSource(client)
resolver := solver.NewDeppySolver(entitySource, variableSource)
solution, err := resolver.Solve(context.Background())
Expect(err).ToNot(HaveOccurred())
Expect(solution.SelectedVariables()).To(HaveLen(0))
})
Expand All @@ -110,17 +113,19 @@ var _ = Describe("OperatorResolver", func() {
}
client := FakeClient(resource)
entitySource := FailEntitySource{}
resolver := resolution.NewOperatorResolver(client, entitySource)
solution, err := resolver.Resolve(context.Background())
variableSource := olm.NewOLMVariableSource(client)
resolver := solver.NewDeppySolver(entitySource, variableSource)
solution, err := resolver.Solve(context.Background())
Expect(solution).To(BeNil())
Expect(err).To(HaveOccurred())
})

It("should return an error if the client throws an error", func() {
client := NewFailClientWithError(fmt.Errorf("something bad happened"))
entitySource := input.NewCacheQuerier(testEntityCache)
resolver := resolution.NewOperatorResolver(client, entitySource)
solution, err := resolver.Resolve(context.Background())
variableSource := olm.NewOLMVariableSource(client)
resolver := solver.NewDeppySolver(entitySource, variableSource)
solution, err := resolver.Solve(context.Background())
Expect(solution).To(BeNil())
Expect(err).To(HaveOccurred())
})
Expand Down
14 changes: 10 additions & 4 deletions internal/resolution/variable_sources/olm/olm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/operator-framework/deppy/pkg/deppy"
"github.com/operator-framework/deppy/pkg/deppy/input"
"sigs.k8s.io/controller-runtime/pkg/client"

operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/bundles_and_dependencies"
Expand All @@ -15,20 +16,25 @@ import (
var _ input.VariableSource = &OLMVariableSource{}

type OLMVariableSource struct {
operators []operatorsv1alpha1.Operator
client client.Client
}

func NewOLMVariableSource(operators ...operatorsv1alpha1.Operator) *OLMVariableSource {
func NewOLMVariableSource(cl client.Client) *OLMVariableSource {
return &OLMVariableSource{
operators: operators,
client: cl,
}
}

func (o *OLMVariableSource) GetVariables(ctx context.Context, entitySource input.EntitySource) ([]deppy.Variable, error) {
operatorList := operatorsv1alpha1.OperatorList{}
if err := o.client.List(ctx, &operatorList); err != nil {
return nil, err
}

var inputVariableSources []input.VariableSource

// build required package variable sources
for _, operator := range o.operators {
for _, operator := range operatorList.Items {
rps, err := required_package.NewRequiredPackage(
operator.Spec.PackageName,
required_package.InVersionRange(operator.Spec.Version),
Expand Down
53 changes: 40 additions & 13 deletions internal/resolution/variable_sources/olm/olm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,25 @@ import (
. "github.com/onsi/gomega"
"github.com/operator-framework/deppy/pkg/deppy"
"github.com/operator-framework/deppy/pkg/deppy/input"
"github.com/operator-framework/operator-controller/api/v1alpha1"
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/bundles_and_dependencies"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/crd_constraints"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/olm"
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/required_package"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

func FakeClient(objects ...client.Object) client.Client {
scheme := runtime.NewScheme()
utilruntime.Must(v1alpha1.AddToScheme(scheme))
return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
}

func TestGlobalConstraints(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "OLMVariableSource Suite")
Expand Down Expand Up @@ -59,7 +70,7 @@ func withVersionRange(versionRange string) opOption {
}
}

func operator(name string, opts ...opOption) operatorsv1alpha1.Operator {
func operator(name string, opts ...opOption) *operatorsv1alpha1.Operator {
op := operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -73,7 +84,7 @@ func operator(name string, opts ...opOption) operatorsv1alpha1.Operator {
Fail(err.Error())
}
}
return op
return &op
}

var _ = Describe("OLMVariableSource", func() {
Expand All @@ -84,20 +95,30 @@ var _ = Describe("OLMVariableSource", func() {
})

It("should produce RequiredPackage variables", func() {
olmVariableSource := olm.NewOLMVariableSource(operator("prometheus"), operator("packageA"))
cl := FakeClient(operator("prometheus"), operator("packageA"))

olmVariableSource := olm.NewOLMVariableSource(cl)
variables, err := olmVariableSource.GetVariables(context.Background(), testEntitySource)
Expect(err).ToNot(HaveOccurred())

packageRequiredVariables := filterVariables[*required_package.RequiredPackageVariable](variables)
Expect(packageRequiredVariables).To(HaveLen(2))
Expect(packageRequiredVariables[0].Identifier()).To(Equal(deppy.IdentifierFromString("required package prometheus")))
Expect(packageRequiredVariables[0].BundleEntities()).To(HaveLen(2))
Expect(packageRequiredVariables[1].Identifier()).To(Equal(deppy.IdentifierFromString("required package packageA")))
Expect(packageRequiredVariables[1].BundleEntities()).To(HaveLen(1))
Expect(packageRequiredVariables).To(WithTransform(func(bvars []*required_package.RequiredPackageVariable) map[deppy.Identifier]int {
m1kola marked this conversation as resolved.
Show resolved Hide resolved
out := map[deppy.Identifier]int{}
for _, variable := range bvars {
out[variable.Identifier()] = len(variable.BundleEntities())
}
return out
}, Equal(map[deppy.Identifier]int{
deppy.IdentifierFromString("required package prometheus"): 2,
deppy.IdentifierFromString("required package packageA"): 1,
})))
})

It("should produce BundleVariables variables", func() {
olmVariableSource := olm.NewOLMVariableSource(operator("prometheus"), operator("packageA"))
cl := FakeClient(operator("prometheus"), operator("packageA"))

olmVariableSource := olm.NewOLMVariableSource(cl)
variables, err := olmVariableSource.GetVariables(context.Background(), testEntitySource)
Expect(err).ToNot(HaveOccurred())

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

entityFromCache("operatorhub/prometheus/0.47.0"),
entityFromCache("operatorhub/prometheus/0.37.0"),
entityFromCache("operatorhub/packageA/2.0.0"),
})))
})

It("should produce version filtered BundleVariables variables", func() {
olmVariableSource := olm.NewOLMVariableSource(operator("prometheus", withVersionRange(">0.40.0")), operator("packageA"))
cl := FakeClient(operator("prometheus", withVersionRange(">0.40.0")), operator("packageA"))

olmVariableSource := olm.NewOLMVariableSource(cl)
variables, err := olmVariableSource.GetVariables(context.Background(), testEntitySource)
Expect(err).ToNot(HaveOccurred())

Expand All @@ -129,7 +152,7 @@ var _ = Describe("OLMVariableSource", func() {
out = append(out, variable.BundleEntity().Entity)
}
return out
}, Equal([]*input.Entity{
}, ConsistOf([]*input.Entity{
entityFromCache("operatorhub/prometheus/0.47.0"),
// filtered out
// entityFromCache("operatorhub/prometheus/0.37.0"),
Expand All @@ -138,7 +161,9 @@ var _ = Describe("OLMVariableSource", func() {
})

It("should produce GlobalConstraints variables", func() {
olmVariableSource := olm.NewOLMVariableSource(operator("prometheus"), operator("packageA"))
cl := FakeClient(operator("prometheus"), operator("packageA"))

olmVariableSource := olm.NewOLMVariableSource(cl)
variables, err := olmVariableSource.GetVariables(context.Background(), testEntitySource)
Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -166,7 +191,9 @@ var _ = Describe("OLMVariableSource", func() {
})

It("should return an errors when they occur", func() {
olmVariableSource := olm.NewOLMVariableSource(operator("prometheus"), operator("packageA"))
cl := FakeClient(operator("prometheus"), operator("packageA"))

olmVariableSource := olm.NewOLMVariableSource(cl)
_, err := olmVariableSource.GetVariables(context.Background(), FailEntitySource{})
Expect(err).To(HaveOccurred())
})
Expand Down
Loading