refactor and add unit tests for operator reconciler #115
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,344 @@ | ||
| package controllers_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| "github.com/operator-framework/deppy/pkg/deppy" | ||
| "github.com/operator-framework/deppy/pkg/deppy/input" | ||
| rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" | ||
| apimeta "k8s.io/apimachinery/pkg/api/meta" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/rand" | ||
| "k8s.io/utils/pointer" | ||
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
| "github.com/operator-framework/operator-controller/controllers" | ||
| "github.com/operator-framework/operator-controller/internal/resolution" | ||
| operatorutil "github.com/operator-framework/operator-controller/internal/util" | ||
| ) | ||
|
|
||
| var _ = Describe("Reconcile Test", func() { | ||
| var ( | ||
| ctx context.Context | ||
| reconciler *controllers.OperatorReconciler | ||
| ) | ||
| BeforeEach(func() { | ||
| ctx = context.Background() | ||
| reconciler = &controllers.OperatorReconciler{ | ||
| Client: cl, | ||
| Scheme: sch, | ||
| Resolver: resolution.NewOperatorResolver(cl, testEntitySource), | ||
| } | ||
| }) | ||
| When("the operator does not exist", func() { | ||
| It("returns no error", func() { | ||
| res, err := reconciler.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: "non-existent"}}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| }) | ||
| When("the operator exists", func() { | ||
| var ( | ||
| operator *operatorsv1alpha1.Operator | ||
| opKey types.NamespacedName | ||
| ) | ||
| BeforeEach(func() { | ||
| opKey = types.NamespacedName{Name: fmt.Sprintf("operator-test-%s", rand.String(8))} | ||
| }) | ||
| When("the operator specifies a non-existent package", func() { | ||
| var pkgName string | ||
| BeforeEach(func() { | ||
| By("initializing cluster state") | ||
| pkgName = fmt.Sprintf("non-existent-%s", rand.String(6)) | ||
| operator = &operatorsv1alpha1.Operator{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: opKey.Name}, | ||
| Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName}, | ||
| } | ||
| err := cl.Create(ctx, operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| It("sets resolution failure status", func() { | ||
| By("running reconcile") | ||
| res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| // TODO: should resolution failure return an error? | ||
|
There was a problem hiding this comment. we should probably trigger reconcile again with the error that we are setting in the condition. There was a problem hiding this comment. +1, I've got that fixed up in #118. When this merges, I'll rebase that one. |
||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("fetching updated operator after reconcile") | ||
| Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred()) | ||
|
|
||
| By("checking the expected conditions") | ||
| cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady) | ||
| Expect(cond).NotTo(BeNil()) | ||
| Expect(cond.Status).To(Equal(metav1.ConditionFalse)) | ||
| Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed)) | ||
| Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' not found", pkgName))) | ||
| }) | ||
| }) | ||
| When("the operator specifies a valid available package", func() { | ||
| const pkgName = "prometheus" | ||
| BeforeEach(func() { | ||
| By("initializing cluster state") | ||
| operator = &operatorsv1alpha1.Operator{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: opKey.Name}, | ||
| Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName}, | ||
| } | ||
| err := cl.Create(ctx, operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| When("the BundleDeployment does not exist", func() { | ||
| BeforeEach(func() { | ||
| By("running reconcile") | ||
| res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("fetching updated operator after reconcile") | ||
| Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred()) | ||
| }) | ||
| It("results in the expected BundleDeployment", func() { | ||
| bd := &rukpakv1alpha1.BundleDeployment{} | ||
| err := cl.Get(ctx, types.NamespacedName{Name: opKey.Name}, bd) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(bd.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-plain")) | ||
| Expect(bd.Spec.Template.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-registry")) | ||
| Expect(bd.Spec.Template.Spec.Source.Type).To(Equal(rukpakv1alpha1.SourceTypeImage)) | ||
| Expect(bd.Spec.Template.Spec.Source.Image).NotTo(BeNil()) | ||
| Expect(bd.Spec.Template.Spec.Source.Image.Ref).To(Equal("quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed")) | ||
| }) | ||
| It("sets resolution success status", func() { | ||
| cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady) | ||
| Expect(cond).NotTo(BeNil()) | ||
| Expect(cond.Status).To(Equal(metav1.ConditionTrue)) | ||
| Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionSucceeded)) | ||
| Expect(cond.Message).To(Equal("resolution was successful")) | ||
|
There was a problem hiding this comment. (Not related to the PR, but just noticed) We should probably define separate error types and constants for the messages to make it easier for users to check the status message. There was a problem hiding this comment. I'm not opposed to constants for messages we reuse, but I'm pretty sure condition messages are not considered stable or part of the API a consumer is supposed to depend on. There was a problem hiding this comment. I agree with Joe. |
||
| }) | ||
| }) | ||
| When("the expected BundleDeployment already exists", func() { | ||
| BeforeEach(func() { | ||
| By("patching the existing BD") | ||
| err := cl.Create(ctx, &rukpakv1alpha1.BundleDeployment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: opKey.Name, | ||
| OwnerReferences: []metav1.OwnerReference{ | ||
| { | ||
| APIVersion: operatorsv1alpha1.GroupVersion.String(), | ||
| Kind: "Operator", | ||
| Name: operator.Name, | ||
| UID: operator.UID, | ||
| Controller: pointer.Bool(true), | ||
| BlockOwnerDeletion: pointer.Bool(true), | ||
| }, | ||
| }, | ||
| }, | ||
| Spec: rukpakv1alpha1.BundleDeploymentSpec{ | ||
| ProvisionerClassName: "core-rukpak-io-plain", | ||
| Template: &rukpakv1alpha1.BundleTemplate{ | ||
| Spec: rukpakv1alpha1.BundleSpec{ | ||
| ProvisionerClassName: "core-rukpak-io-registry", | ||
| Source: rukpakv1alpha1.BundleSource{ | ||
| Type: rukpakv1alpha1.SourceTypeImage, | ||
| Image: &rukpakv1alpha1.ImageSource{ | ||
| Ref: "quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("running reconcile") | ||
| res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("fetching updated operator after reconcile") | ||
| Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred()) | ||
| }) | ||
| PIt("does not patch the BundleDeployment", func() { | ||
| // TODO: verify that no patch call is made. | ||
| }) | ||
| It("results in the expected BundleDeployment", func() { | ||
| bd := &rukpakv1alpha1.BundleDeployment{} | ||
| err := cl.Get(ctx, types.NamespacedName{Name: opKey.Name}, bd) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(bd.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-plain")) | ||
| Expect(bd.Spec.Template.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-registry")) | ||
| Expect(bd.Spec.Template.Spec.Source.Type).To(Equal(rukpakv1alpha1.SourceTypeImage)) | ||
| Expect(bd.Spec.Template.Spec.Source.Image).NotTo(BeNil()) | ||
| Expect(bd.Spec.Template.Spec.Source.Image.Ref).To(Equal("quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed")) | ||
| }) | ||
| It("sets resolution success status", func() { | ||
| cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady) | ||
| Expect(cond).NotTo(BeNil()) | ||
| Expect(cond.Status).To(Equal(metav1.ConditionTrue)) | ||
| Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionSucceeded)) | ||
| Expect(cond.Message).To(Equal("resolution was successful")) | ||
| }) | ||
| }) | ||
| When("an out-of-date BundleDeployment exists", func() { | ||
| BeforeEach(func() { | ||
| By("creating the expected BD") | ||
| err := cl.Create(ctx, &rukpakv1alpha1.BundleDeployment{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: opKey.Name}, | ||
| Spec: rukpakv1alpha1.BundleDeploymentSpec{ | ||
| ProvisionerClassName: "foo", | ||
| Template: &rukpakv1alpha1.BundleTemplate{ | ||
| Spec: rukpakv1alpha1.BundleSpec{ | ||
| ProvisionerClassName: "bar", | ||
| Source: rukpakv1alpha1.BundleSource{ | ||
| Type: rukpakv1alpha1.SourceTypeHTTP, | ||
| HTTP: &rukpakv1alpha1.HTTPSource{ | ||
| URL: "http://localhost:8080/", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("running reconcile") | ||
| res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("fetching updated operator after reconcile") | ||
| Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred()) | ||
| }) | ||
| It("results in the expected BundleDeployment", func() { | ||
| bd := &rukpakv1alpha1.BundleDeployment{} | ||
| err := cl.Get(ctx, types.NamespacedName{Name: opKey.Name}, bd) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(bd.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-plain")) | ||
| Expect(bd.Spec.Template.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-registry")) | ||
| Expect(bd.Spec.Template.Spec.Source.Type).To(Equal(rukpakv1alpha1.SourceTypeImage)) | ||
| Expect(bd.Spec.Template.Spec.Source.Image).NotTo(BeNil()) | ||
| Expect(bd.Spec.Template.Spec.Source.Image.Ref).To(Equal("quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed")) | ||
| }) | ||
| It("sets resolution success status", func() { | ||
| cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady) | ||
| Expect(cond).NotTo(BeNil()) | ||
| Expect(cond.Status).To(Equal(metav1.ConditionTrue)) | ||
| Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionSucceeded)) | ||
| Expect(cond.Message).To(Equal("resolution was successful")) | ||
| }) | ||
| }) | ||
| }) | ||
| When("the selected bundle's image ref cannot be parsed", func() { | ||
| const pkgName = "badimage" | ||
| BeforeEach(func() { | ||
| By("initializing cluster state") | ||
| operator = &operatorsv1alpha1.Operator{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: opKey.Name}, | ||
| Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName}, | ||
| } | ||
| err := cl.Create(ctx, operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| PIt("sets resolution failure status and returns an error", func() { | ||
| By("running reconcile") | ||
| res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| Expect(err).To(MatchError(ContainSubstring(`error determining bundle path for entity`))) | ||
|
|
||
| By("fetching updated operator after reconcile") | ||
| Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred()) | ||
|
|
||
| By("checking the expected conditions") | ||
| // TODO: there should be a condition update that sets Ready false in this scenario | ||
| }) | ||
| }) | ||
| When("the operator specifies a duplicate package", func() { | ||
| const pkgName = "prometheus" | ||
| BeforeEach(func() { | ||
| By("initializing cluster state") | ||
| err := cl.Create(ctx, &operatorsv1alpha1.Operator{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("orig-%s", opKey.Name)}, | ||
| Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName}, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| operator = &operatorsv1alpha1.Operator{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: opKey.Name}, | ||
| Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName}, | ||
| } | ||
| err = cl.Create(ctx, operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| It("sets resolution failure status", func() { | ||
| By("running reconcile") | ||
| res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
| // TODO: should this scenario return an error? | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("fetching updated operator after reconcile") | ||
| Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred()) | ||
|
|
||
| By("checking the expected conditions") | ||
| cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady) | ||
| Expect(cond).NotTo(BeNil()) | ||
| Expect(cond.Status).To(Equal(metav1.ConditionFalse)) | ||
| Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed)) | ||
| Expect(cond.Message).To(Equal(`duplicate identifier "required package prometheus" in input`)) | ||
| }) | ||
| }) | ||
| AfterEach(func() { | ||
| verifyInvariants(ctx, operator) | ||
|
|
||
| err := cl.Delete(ctx, operator) | ||
| Expect(err).To(Not(HaveOccurred())) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| func verifyInvariants(ctx context.Context, op *operatorsv1alpha1.Operator) { | ||
| key := client.ObjectKeyFromObject(op) | ||
| err := cl.Get(ctx, key, op) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| verifyConditionsInvariants(op) | ||
| } | ||
|
|
||
| func verifyConditionsInvariants(op *operatorsv1alpha1.Operator) { | ||
| // Expect that the operator's set of conditions contains all defined | ||
| // condition types for the Operator API. Every reconcile should always | ||
| // ensure every condition type's status/reason/message reflects the state | ||
| // read during _this_ reconcile call. | ||
| Expect(op.Status.Conditions).To(HaveLen(len(operatorutil.ConditionTypes))) | ||
| for _, t := range operatorutil.ConditionTypes { | ||
| cond := apimeta.FindStatusCondition(op.Status.Conditions, t) | ||
| Expect(cond).To(Not(BeNil())) | ||
| Expect(cond.Status).NotTo(BeEmpty()) | ||
| Expect(cond.Reason).To(BeElementOf(operatorutil.ConditionReasons)) | ||
| Expect(cond.ObservedGeneration).To(Equal(op.GetGeneration())) | ||
| } | ||
| } | ||
|
|
||
| var testEntitySource = input.NewCacheQuerier(map[deppy.Identifier]input.Entity{ | ||
| "operatorhub/prometheus/0.37.0": *input.NewEntity("operatorhub/prometheus/0.37.0", map[string]string{ | ||
| "olm.bundle.path": `"quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"`, | ||
| "olm.channel": `{"channelName":"beta","priority":0}`, | ||
| "olm.package": `{"packageName":"prometheus","version":"0.37.0"}`, | ||
| "olm.gvk": `[]`, | ||
| }), | ||
| "operatorhub/prometheus/0.47.0": *input.NewEntity("operatorhub/prometheus/0.47.0", map[string]string{ | ||
| "olm.bundle.path": `"quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed"`, | ||
| "olm.channel": `{"channelName":"beta","priority":0,"replaces":"prometheusoperator.0.37.0"}`, | ||
| "olm.package": `{"packageName":"prometheus","version":"0.47.0"}`, | ||
| "olm.gvk": `[]`, | ||
| }), | ||
| "operatorhub/badimage/0.1.0": *input.NewEntity("operatorhub/badimage/0.1.0", map[string]string{ | ||
| "olm.bundle.path": `{"name": "quay.io/operatorhubio/badimage:v0.1.0"}`, | ||
| "olm.package": `{"packageName":"badimage","version":"0.1.0"}`, | ||
| "olm.gvk": `[]`, | ||
| }), | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this remain
ConditionTrueas resolution did fail?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConditionTrueis sayingReady: True. Since resolution failed, shouldn't we be sayingReady: false?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I think the structure of how these condition values gets set makes it difficult to reason about. I've fixed this in #107, so as soon as this change lands, I can layer in another PR that simplifies the
Reconcilefunction and makes it more obvious how/when the ready condition is set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I was conflating "Resolution failed" with the type.
OK, I think the confusion is that it's not obvious this is for the "Ready" type, since
TypeReadyis not very local, it's down 30+ lines.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Exactly - That problem will be resolved in #118.