From 65aa63c455521e26699a006ba52a28279ae9c7e3 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Fri, 21 Jun 2024 11:28:43 -0400 Subject: [PATCH] fix: reduce unnecessary caching and remove predicate from dynamic watches Our cache configuration resulted in watching all cluster-scoped resources for each cluster-scoped resource that shows up in a ClusterExtension. Now, we only watch: - All ClusterExtensions - All ClusterCatalogs - All objects in the systemNamespace - All other objects that specifically match the dependentPredicate This commit also removes the dependent predicate so that we reconcile for any change to managed resources. This is important to ensure that future features (like health checking) can account for any change to managed operator objects. Signed-off-by: Joe Lanford --- cmd/manager/main.go | 8 +++-- .../clusterextension_controller.go | 35 +++++++++---------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index a91a51b86..7d5e36857 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/server" + catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" registryv1handler "github.com/operator-framework/rukpak/pkg/handler" "github.com/operator-framework/rukpak/pkg/provisioner/registry" @@ -127,12 +128,13 @@ func main() { LeaderElectionID: "9c4404e7.operatorframework.io", Cache: crcache.Options{ ByObject: map[client.Object]crcache.ByObject{ - &ocv1alpha1.ClusterExtension{}: {}, + &ocv1alpha1.ClusterExtension{}: {Label: k8slabels.Everything()}, + &catalogd.ClusterCatalog{}: {Label: k8slabels.Everything()}, }, DefaultNamespaces: map[string]crcache.Config{ - systemNamespace: {}, - crcache.AllNamespaces: {LabelSelector: dependentSelector}, + systemNamespace: {LabelSelector: k8slabels.Everything()}, }, + DefaultLabelSelector: dependentSelector, }, // LeaderElectionReleaseOnCancel defines if the leader should step down voluntarily // when the Manager ends. This requires the binary to immediately end when the diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index b7572fe89..951cb0af3 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -45,6 +45,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" apimachyaml "k8s.io/apimachinery/pkg/util/yaml" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" crcontroller "sigs.k8s.io/controller-runtime/pkg/controller" @@ -63,7 +64,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/property" rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2" registryv1handler "github.com/operator-framework/rukpak/pkg/handler" - helmpredicate "github.com/operator-framework/rukpak/pkg/helm-operator-plugins/predicate" rukpaksource "github.com/operator-framework/rukpak/pkg/source" "github.com/operator-framework/rukpak/pkg/storage" "github.com/operator-framework/rukpak/pkg/util" @@ -357,7 +357,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp source.Kind(r.cache, obj, crhandler.EnqueueRequestForOwner(r.Scheme(), r.RESTMapper(), ext, crhandler.OnlyControllerOwner()), - helmpredicate.DependentPredicateFuncs[client.Object](), ), ); err != nil { return err @@ -572,24 +571,24 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error { controller, err := ctrl.NewControllerManagedBy(mgr). For(&ocv1alpha1.ClusterExtension{}). Watches(&catalogd.ClusterCatalog{}, - crhandler.EnqueueRequestsFromMapFunc(clusterExtensionRequestsForCatalog(mgr.GetClient(), mgr.GetLogger()))). - WithEventFilter(predicate.Funcs{ - UpdateFunc: func(ue event.UpdateEvent) bool { - oldObject, isOldCatalog := ue.ObjectOld.(*catalogd.ClusterCatalog) - newObject, isNewCatalog := ue.ObjectNew.(*catalogd.ClusterCatalog) - - if !isOldCatalog || !isNewCatalog { - return true - } + crhandler.EnqueueRequestsFromMapFunc(clusterExtensionRequestsForCatalog(mgr.GetClient(), mgr.GetLogger())), + builder.WithPredicates(predicate.Funcs{ + UpdateFunc: func(ue event.UpdateEvent) bool { + oldObject, isOldCatalog := ue.ObjectOld.(*catalogd.ClusterCatalog) + newObject, isNewCatalog := ue.ObjectNew.(*catalogd.ClusterCatalog) + + if !isOldCatalog || !isNewCatalog { + return true + } - if oldObject.Status.ResolvedSource != nil && newObject.Status.ResolvedSource != nil { - if oldObject.Status.ResolvedSource.Image != nil && newObject.Status.ResolvedSource.Image != nil { - return oldObject.Status.ResolvedSource.Image.ResolvedRef != newObject.Status.ResolvedSource.Image.ResolvedRef + if oldObject.Status.ResolvedSource != nil && newObject.Status.ResolvedSource != nil { + if oldObject.Status.ResolvedSource.Image != nil && newObject.Status.ResolvedSource.Image != nil { + return oldObject.Status.ResolvedSource.Image.ResolvedRef != newObject.Status.ResolvedSource.Image.ResolvedRef + } } - } - return true - }, - }). + return true + }, + })). Build(r) if err != nil {