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

resolve operators on catalog availability change #216

Merged
merged 4 commits into from
Jun 2, 2023
Merged

resolve operators on catalog availability change #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82a834a
resolve operators on catalog availability change
ankitathomas May 16, 2023
1544404
removing catalog state check for operator reconcile
ankitathomas May 17, 2023
b688885
e2e tests for catalog watch
ankitathomas May 19, 2023
67caff4
use smaller test index image, cleanup after tests
ankitathomas May 26, 2023
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
7 changes: 7 additions & 0 deletions config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ rules:
verbs:
- list
- watch
- apiGroups:
- catalogd.operatorframework.io
resources:
- catalogs
verbs:
- list
- watch
- apiGroups:
- catalogd.operatorframework.io
resources:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ go 1.19

require (
github.com/blang/semver/v4 v4.0.0
github.com/go-logr/logr v1.2.3
github.com/onsi/ginkgo/v2 v2.8.3
github.com/onsi/gomega v1.27.1
github.com/operator-framework/catalogd v0.2.0
Expand All @@ -26,7 +27,6 @@ require (
github.com/evanphx/json-patch/v5 v5.6.0 // indirect
github.com/fsnotify/fsnotify v1.6.0 // indirect
github.com/go-air/gini v1.0.4 // indirect
github.com/go-logr/logr v1.2.3 // indirect
github.com/go-logr/zapr v1.2.3 // indirect
github.com/go-openapi/jsonpointer v0.19.5 // indirect
github.com/go-openapi/jsonreference v0.20.0 // indirect
Expand Down
34 changes: 32 additions & 2 deletions internal/controllers/operator_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"context"
"fmt"

"github.com/go-logr/logr"
catalogd "github.com/operator-framework/catalogd/pkg/apis/core/v1beta1"
"github.com/operator-framework/deppy/pkg/deppy/solver"
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
"k8s.io/apimachinery/pkg/api/equality"
Expand All @@ -32,11 +34,13 @@ import (
"k8s.io/utils/pointer"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"

"github.com/operator-framework/operator-controller/internal/controllers/validators"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

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 @@ -57,6 +61,7 @@ type OperatorReconciler struct {

//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=bundlemetadata,verbs=list;watch
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=packages,verbs=list;watch
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=catalogs,verbs=list;watch

func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
l := log.FromContext(ctx).WithName("operator-controller")
Expand Down Expand Up @@ -287,6 +292,8 @@ func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha
func (r *OperatorReconciler) SetupWithManager(mgr ctrl.Manager) error {
err := ctrl.NewControllerManagedBy(mgr).
For(&operatorsv1alpha1.Operator{}).
Watches(source.NewKindWithCache(&catalogd.Catalog{}, mgr.GetCache()),
handler.EnqueueRequestsFromMapFunc(operatorRequestsForCatalog(context.TODO(), mgr.GetClient(), mgr.GetLogger()))).
Owns(&rukpakv1alpha1.BundleDeployment{}).
Complete(r)

Expand Down Expand Up @@ -422,3 +429,26 @@ func setInstalledStatusConditionUnknown(conditions *[]metav1.Condition, message
ObservedGeneration: generation,
})
}

// Generate reconcile requests for all operators affected by a catalog change
func operatorRequestsForCatalog(ctx context.Context, c client.Reader, logger logr.Logger) handler.MapFunc {
return func(object client.Object) []reconcile.Request {
// no way of associating an operator to a catalog so create reconcile requests for everything
operators := operatorsv1alpha1.OperatorList{}
err := c.List(ctx, &operators)
if err != nil {
logger.Error(err, "unable to enqueue operators for catalog reconcile")
return nil
}
var requests []reconcile.Request
for _, op := range operators.Items {
requests = append(requests, reconcile.Request{
NamespacedName: types.NamespacedName{
Namespace: op.GetNamespace(),
Name: op.GetName(),
},
})
}
return requests
}
}
109 changes: 98 additions & 11 deletions test/e2e/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
catalogd "github.com/operator-framework/catalogd/pkg/apis/core/v1beta1"
operatorv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
"k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -52,30 +53,35 @@ var _ = Describe("Operator Install", func() {
Image: &catalogd.ImageSource{
// (TODO): Set up a local image registry, and build and store a test catalog in it
// to use in the test suite
Ref: "quay.io/operatorhubio/catalog:latest",
Ref: "quay.io/olmtest/e2e-index:single-package-fbc", //generated from: "quay.io/operatorhubio/catalog:latest",
},
},
},
}
})
It("resolves the specified package with correct bundle path", func() {
err := c.Create(ctx, operatorCatalog)
Expect(err).ToNot(HaveOccurred())
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: "test-catalog"}, operatorCatalog)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(len(operatorCatalog.Status.Conditions)).To(Equal(1))
g.Expect(operatorCatalog.Status.Conditions[0].Message).To(ContainSubstring("successfully unpacked the catalog image"))
cond := apimeta.FindStatusCondition(operatorCatalog.Status.Conditions, catalogd.TypeUnpacked)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
g.Expect(cond.Reason).To(Equal(catalogd.ReasonUnpackSuccessful))
g.Expect(cond.Message).To(ContainSubstring("successfully unpacked the catalog image"))
}).WithTimeout(5 * time.Minute).WithPolling(defaultPoll).Should(Succeed())
})
It("resolves the specified package with correct bundle path", func() {

By("creating the Operator resource")
err := c.Create(ctx, operator)
err = c.Create(ctx, operator)
Expect(err).ToNot(HaveOccurred())

By("eventually reporting a successful resolution and bundle path")
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operator.Name}, operator)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(len(operator.Status.Conditions)).To(Equal(2))

cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorv1alpha1.TypeResolved)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
Expand All @@ -97,17 +103,98 @@ var _ = Describe("Operator Install", func() {
bd := rukpakv1alpha1.BundleDeployment{}
err = c.Get(ctx, types.NamespacedName{Name: operatorName}, &bd)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(len(bd.Status.Conditions)).To(Equal(2))
g.Expect(bd.Status.Conditions[0].Reason).To(Equal("UnpackSuccessful"))
g.Expect(bd.Status.Conditions[1].Reason).To(Equal("InstallationSucceeded"))

cond = apimeta.FindStatusCondition(bd.Status.Conditions, rukpakv1alpha1.TypeHasValidBundle)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
g.Expect(cond.Reason).To(Equal(rukpakv1alpha1.ReasonUnpackSuccessful))

cond = apimeta.FindStatusCondition(bd.Status.Conditions, rukpakv1alpha1.TypeInstalled)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
g.Expect(cond.Reason).To(Equal(rukpakv1alpha1.ReasonInstallationSucceeded))
}).WithTimeout(defaultTimeout).WithPolling(defaultPoll).Should(Succeed())
})
It("resolves again when a new catalog is available", func() {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can avoid all the long timeouts here by simplifying the test expectations. I think all we really want to assert is "whenever a catalog is created/updated/deleted, all of the existing operators are reconciled". Is there something that would show up in the Operator CR that we could use to verify that a reconcile happened after the catalog event?

That might be organized something like this:

Describe("reconcile Operators on catalog events")
   - BeforeEach() -> create 3 operators
   - AfterEach() -> delete 3 operators
   - When("catalog is created")
        - It("reconciles 3 operators")
   - When("catalog exists")
       - BeforeEach() -> create a catalog
       - AfterEach() -> delete the catalog
       - When("catalog is updated")
           - It("reconciles 3 operators")
       - When("catalog is deleted")
           - It("reconciles 3 operators")

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe even this is too much to do in the e2e? Is there a way to test the controller and the watches without a full-blown e2e?

If so, we could verify all these scenarios there, and then just focus on the happy path in the e2e?

I'm honestly not sure what's the best option here, but I think we should really focus on keeping our overall e2e run as short (in terms of overall run time) as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it would be ideal if we could test this outside of the e2e tests, but as far as I am aware there isn't a good way to test watches being triggered without actually starting the controller and performing operations against a cluster that would trigger those watches. I think it could be done with envtest but would likely involve a lot of stubbing out resources and would essentially boil down to the question of "do we prefer complex unit tests or longer running e2e tests?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a minimal catalog cuts the wait down by a good chunk, so the e2e suite shouldn't be as long lived. I'm not sure if we'd want to expose resolution/reconcile details on the operator CR just for the sake of CI, in case it can be confusing to a user.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're concerned about adding to the status API, how about examining the logs. We control the logs, and the tests can examine them for particular log entries. It wouldn't necessarily be exposing the logs as an API since it would be for e2e/unit tests, which can be modified when the logs are.

Copy link
Member

@joelanford joelanford May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The changes in this PR boil down to an updated controller configuration with a new watch that adds some extra stuff to the workqueue and calls a reconciler. So I'm suggesting it might be possible to isolate that specific behavior if we refactored a little bit (e.g. what if instead of SetupWithManager being a method on the reconciler, we made it a function that we could pass a reconciler to). In our main.go, we'd pass the real reconciler, but in a test, we'd pass a fake reconciler that does the "did I get triggered" assertions

Then we could put some envtest-based unit tests together alongside the reconciler and we would essentially split the testing into two chunks:

  1. Does the controller configuration enqueue the expected reconcile requests?
  2. Does the reconciler handle reconcile requests in the expected way?

We'd still want an e2e that runs through some scenarios of the integrated/real main.go, but we would avoid the combinatoric problem here, which would save lots of time in e2e runs.

Copy link
Member

@joelanford joelanford May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

how about examining the logs

Yeah +1 if the unit test path isn't taken. We do this in the SDK helm-operator e2e tests.

https://github.com/operator-framework/operator-sdk/blob/5347d9375658cab9117e7ac8f691d35bb9154ff9/test/e2e/helm/cluster_test.go#L125-L131

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm opening another issue to track the refactoring #247. I don't think it makes sense to block this PR on the refactor, I'm in favor of having that in a separate PR.

Eventually(func(g Gomega) {
// target package should not be present on cluster
err := c.Get(ctx, types.NamespacedName{Name: pkgName}, &catalogd.Package{})
Expect(errors.IsNotFound(err)).To(BeTrue())
}).WithTimeout(5 * time.Minute).WithPolling(defaultPoll).Should(Succeed())

By("creating the Operator resource")
err := c.Create(ctx, operator)
Expect(err).ToNot(HaveOccurred())

By("failing to find Operator during resolution")
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operator.Name}, operator)
g.Expect(err).ToNot(HaveOccurred())
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorv1alpha1.TypeResolved)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionFalse))
g.Expect(cond.Reason).To(Equal(operatorv1alpha1.ReasonResolutionFailed))
g.Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' not found", pkgName)))
}).WithTimeout(defaultTimeout).WithPolling(defaultPoll).Should(Succeed())

By("creating an Operator catalog with the desired package")
err = c.Create(ctx, operatorCatalog)
Expect(err).ToNot(HaveOccurred())
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operatorCatalog.Name}, operatorCatalog)
g.Expect(err).ToNot(HaveOccurred())
cond := apimeta.FindStatusCondition(operatorCatalog.Status.Conditions, catalogd.TypeUnpacked)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
g.Expect(cond.Reason).To(Equal(catalogd.ReasonUnpackSuccessful))
}).WithTimeout(5 * time.Minute).WithPolling(defaultPoll).Should(Succeed())

By("eventually resolving the package successfully")
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operator.Name}, operator)
g.Expect(err).ToNot(HaveOccurred())
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorv1alpha1.TypeResolved)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
g.Expect(cond.Reason).To(Equal(operatorv1alpha1.ReasonSuccess))
}).WithTimeout(defaultTimeout).WithPolling(defaultPoll).Should(Succeed())
})
AfterEach(func() {
err := c.Delete(ctx, operatorCatalog)
err := c.Delete(ctx, operator)
Expect(err).ToNot(HaveOccurred())
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operatorName}, &operatorv1alpha1.Operator{})
Expect(errors.IsNotFound(err)).To(BeTrue())
}).WithTimeout(defaultTimeout).WithPolling(defaultPoll).Should(Succeed())

err = c.Delete(ctx, operatorCatalog)
Expect(err).ToNot(HaveOccurred())
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operatorCatalog.Name}, &catalogd.Catalog{})
Expect(errors.IsNotFound(err)).To(BeTrue())
}).WithTimeout(defaultTimeout).WithPolling(defaultPoll).Should(Succeed())

// speed up delete without waiting for gc
err = c.DeleteAllOf(ctx, &catalogd.BundleMetadata{})
Expect(err).ToNot(HaveOccurred())
err = c.Delete(ctx, operator)
err = c.DeleteAllOf(ctx, &catalogd.Package{})
Expect(err).ToNot(HaveOccurred())

Eventually(func(g Gomega) {
// ensure resource cleanup
packages := &catalogd.PackageList{}
err = c.List(ctx, packages)
Expect(err).To(BeNil())
Expect(packages.Items).To(BeEmpty())

bmd := &catalogd.BundleMetadataList{}
err = c.List(ctx, bmd)
Expect(err).To(BeNil())
Expect(bmd.Items).To(BeEmpty())

err = c.Get(ctx, types.NamespacedName{Name: operatorName}, &rukpakv1alpha1.BundleDeployment{})
Expect(errors.IsNotFound(err)).To(BeTrue())
}).WithTimeout(5 * time.Minute).WithPolling(defaultPoll).Should(Succeed())
})
})
})