Merge pull request kubernetes-sigs#8918 from sbueringer/pr-cache-mapper

k8s-ci-robot · web-flow · commit fd0dc6087684 · 2023-06-27T02:46:32.000-07:00
🌱 util: cache list calls in cluster to objects mapper
diff --git a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
@@ -158,7 +158,7 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	}
 
 	// Look up the owner of this kubeadm config if there is one
-	configOwner, err := bsutil.GetConfigOwnerFromCache(ctx, r.Client, config)
+	configOwner, err := bsutil.GetTypedConfigOwner(ctx, r.Client, config)
 	if apierrors.IsNotFound(err) {
 		// Could not find the owner yet, this is not an error and will rereconcile when the owner gets set.
 		return ctrl.Result{}, nil
diff --git a/bootstrap/util/configowner.go b/bootstrap/util/configowner.go
@@ -127,15 +127,15 @@ func (co ConfigOwner) KubernetesVersion() string {
 
 // GetConfigOwner returns the Unstructured object owning the current resource
 // using the uncached unstructured client. For performance-sensitive uses,
-// consider GetConfigOwnerCached.
+// consider GetTypedConfigOwner.
 func GetConfigOwner(ctx context.Context, c client.Client, obj metav1.Object) (*ConfigOwner, error) {
 	return getConfigOwner(ctx, c, obj, GetOwnerByRef)
 }
 
-// GetConfigOwnerFromCache returns the Unstructured object owning the current
+// GetTypedConfigOwner returns the Unstructured object owning the current
 // resource. The implementation ensures a typed client is used, so the objects are read from the cache.
-func GetConfigOwnerFromCache(ctx context.Context, c client.Client, obj metav1.Object) (*ConfigOwner, error) {
-	return getConfigOwner(ctx, c, obj, GetOwnerByRefFromCache)
+func GetTypedConfigOwner(ctx context.Context, c client.Client, obj metav1.Object) (*ConfigOwner, error) {
+	return getConfigOwner(ctx, c, obj, GetTypedOwnerByRef)
 }
 
 func getConfigOwner(ctx context.Context, c client.Client, obj metav1.Object, getFn func(context.Context, client.Client, *corev1.ObjectReference) (*ConfigOwner, error)) (*ConfigOwner, error) {
@@ -183,9 +183,9 @@ func GetOwnerByRef(ctx context.Context, c client.Client, ref *corev1.ObjectRefer
 	return &ConfigOwner{obj}, nil
 }
 
-// GetOwnerByRefFromCache finds and returns the owner by looking at the object
+// GetTypedOwnerByRef finds and returns the owner by looking at the object
 // reference. The implementation ensures a typed client is used, so the objects are read from the cache.
-func GetOwnerByRefFromCache(ctx context.Context, c client.Client, ref *corev1.ObjectReference) (*ConfigOwner, error) {
+func GetTypedOwnerByRef(ctx context.Context, c client.Client, ref *corev1.ObjectReference) (*ConfigOwner, error) {
 	obj, err := c.Scheme().New(ref.GroupVersionKind())
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to construct object of type %s", ref.GroupVersionKind())
diff --git a/bootstrap/util/configowner_test.go b/bootstrap/util/configowner_test.go
@@ -175,7 +175,7 @@ func TestGetConfigOwner(t *testing.T) {
 		doTests(t, GetConfigOwner)
 	})
 	t.Run("cached", func(t *testing.T) {
-		doTests(t, GetConfigOwnerFromCache)
+		doTests(t, GetTypedConfigOwner)
 	})
 }
 
diff --git a/docs/book/src/developer/providers/migrations/v1.4-to-v1.5.md b/docs/book/src/developer/providers/migrations/v1.4-to-v1.5.md
@@ -36,7 +36,8 @@ maintainers of providers and consumers of our Go API.
 - clusterctl move is adding the new annotation `clusterctl.cluster.x-k8s.io/delete-for-move` before object deletion.
 - Providers running CAPI release-0.3 clusterctl upgrade tests should set `WorkloadKubernetesVersion` field to the maximum workload cluster kubernetes version supported by the old providers in `ClusterctlUpgradeSpecInput`. For more information, please see: https://github.com/kubernetes-sigs/cluster-api/pull/8518#issuecomment-1508064859 
 - Introduced function `CollectInfrastructureLogs` at the `ClusterLogCollector` interface in `test/framework/cluster_proxy.go` to allow collecting infrastructure related logs during tests.
-- A `GetConfigOwnerFromCache` function has been added to the `sigs.k8s.io./cluster-api/bootstrap/util` package. It is equivalent to `GetConfigOwner` except that it uses the cached typed client instead of the uncached unstructured client, so `GetConfigOwnerFromCache` is expected to be more performant.
+- A `GetTypedConfigOwner` function has been added to the `sigs.k8s.io./cluster-api/bootstrap/util` package. It is equivalent to `GetConfigOwner` except that it uses the cached typed client instead of the uncached unstructured client, so `GetTypedConfigOwner` is expected to be more performant.
+- `ClusterToObjectsMapper` in `sigs.k8s.io./cluster-api/util` has been deprecated, please use `ClusterToTypedObjectsMapper` instead.
 
 ### Suggested changes for providers
 
diff --git a/exp/internal/controllers/machinepool_controller.go b/exp/internal/controllers/machinepool_controller.go
@@ -74,7 +74,7 @@ type MachinePoolReconciler struct {
 }
 
 func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToMachinePools, err := util.ClusterToObjectsMapper(mgr.GetClient(), &expv1.MachinePoolList{}, mgr.GetScheme())
+	clusterToMachinePools, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &expv1.MachinePoolList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go
@@ -92,7 +92,7 @@ type Reconciler struct {
 }
 
 func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToMachines, err := util.ClusterToObjectsMapper(mgr.GetClient(), &clusterv1.MachineList{}, mgr.GetScheme())
+	clusterToMachines, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &clusterv1.MachineList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go
@@ -75,7 +75,7 @@ type Reconciler struct {
 }
 
 func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToMachineDeployments, err := util.ClusterToObjectsMapper(mgr.GetClient(), &clusterv1.MachineDeploymentList{}, mgr.GetScheme())
+	clusterToMachineDeployments, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &clusterv1.MachineDeploymentList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go
@@ -92,7 +92,7 @@ type Reconciler struct {
 }
 
 func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToMachineSets, err := util.ClusterToObjectsMapper(mgr.GetClient(), &clusterv1.MachineSetList{}, mgr.GetScheme())
+	clusterToMachineSets, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &clusterv1.MachineSetList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/main.go b/main.go
@@ -367,13 +367,12 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {
 			Unstructured: true,
 		},
 	})
+	if err != nil {
+		setupLog.Error(err, "unable to create unstructured caching client")
+		os.Exit(1)
+	}
 
 	if feature.Gates.Enabled(feature.ClusterTopology) {
-		if err != nil {
-			setupLog.Error(err, "unable to create unstructured caching client", "controller", "ClusterTopology")
-			os.Exit(1)
-		}
-
 		if err := (&controllers.ClusterClassReconciler{
 			Client:                    mgr.GetClient(),
 			APIReader:                 mgr.GetAPIReader(),
diff --git a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller.go b/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller.go
@@ -143,7 +143,7 @@ func (r *DockerMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Re
 
 // SetupWithManager will add watches for this controller.
 func (r *DockerMachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToDockerMachinePools, err := util.ClusterToObjectsMapper(mgr.GetClient(), &infraexpv1.DockerMachinePoolList{}, mgr.GetScheme())
+	clusterToDockerMachinePools, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &infraexpv1.DockerMachinePoolList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go b/test/infrastructure/docker/internal/controllers/dockermachine_controller.go
@@ -422,7 +422,7 @@ func (r *DockerMachineReconciler) reconcileDelete(ctx context.Context, machine *
 
 // SetupWithManager will add watches for this controller.
 func (r *DockerMachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToDockerMachines, err := util.ClusterToObjectsMapper(mgr.GetClient(), &infrav1.DockerMachineList{}, mgr.GetScheme())
+	clusterToDockerMachines, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &infrav1.DockerMachineList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go b/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go
@@ -1130,7 +1130,7 @@ func (r *InMemoryMachineReconciler) reconcileDeleteControllerManager(ctx context
 
 // SetupWithManager will add watches for this controller.
 func (r *InMemoryMachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	clusterToInMemoryMachines, err := util.ClusterToObjectsMapper(mgr.GetClient(), &infrav1.InMemoryMachineList{}, mgr.GetScheme())
+	clusterToInMemoryMachines, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &infrav1.InMemoryMachineList{}, mgr.GetScheme())
 	if err != nil {
 		return err
 	}
diff --git a/util/util.go b/util/util.go
@@ -470,6 +470,10 @@ func (k KubeAwareAPIVersions) Less(i, j int) bool {
 // ClusterToObjectsMapper returns a mapper function that gets a cluster and lists all objects for the object passed in
 // and returns a list of requests.
 // NB: The objects are required to have `clusterv1.ClusterNameLabel` applied.
+//
+// Deprecated: This function is deprecated and will be removed in a future release, use ClusterToTypedObjectsMapper instead.
+// The problem with this function is that it uses UnstructuredList to retrieve objects, with the default client configuration
+// this will lead to uncached List calls, which is a major performance issue.
 func ClusterToObjectsMapper(c client.Client, ro client.ObjectList, scheme *runtime.Scheme) (handler.MapFunc, error) {
 	gvk, err := apiutil.GVKForObject(ro, scheme)
 	if err != nil {
@@ -513,6 +517,72 @@ func ClusterToObjectsMapper(c client.Client, ro client.ObjectList, scheme *runti
 	}, nil
 }
 
+// ClusterToTypedObjectsMapper returns a mapper function that gets a cluster and lists all objects for the object passed in
+// and returns a list of requests.
+// Note: This function uses the passed in typed ObjectList and thus with the default client configuration all list calls
+// will be cached.
+// NB: The objects are required to have `clusterv1.ClusterNameLabel` applied.
+func ClusterToTypedObjectsMapper(c client.Client, ro client.ObjectList, scheme *runtime.Scheme) (handler.MapFunc, error) {
+	gvk, err := apiutil.GVKForObject(ro, scheme)
+	if err != nil {
+		return nil, err
+	}
+
+	// Note: we create the typed ObjectList once here, so we don't have to use
+	// reflection in every execution of the actual event handler.
+	obj, err := scheme.New(gvk)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to construct object of type %s", gvk)
+	}
+	objectList, ok := obj.(client.ObjectList)
+	if !ok {
+		return nil, errors.Errorf("expected objject to be a client.ObjectList, is actually %T", obj)
+	}
+
+	isNamespaced, err := isAPINamespaced(gvk, c.RESTMapper())
+	if err != nil {
+		return nil, err
+	}
+
+	return func(ctx context.Context, o client.Object) []ctrl.Request {
+		cluster, ok := o.(*clusterv1.Cluster)
+		if !ok {
+			return nil
+		}
+
+		listOpts := []client.ListOption{
+			client.MatchingLabels{
+				clusterv1.ClusterNameLabel: cluster.Name,
+			},
+		}
+
+		if isNamespaced {
+			listOpts = append(listOpts, client.InNamespace(cluster.Namespace))
+		}
+
+		objectList = objectList.DeepCopyObject().(client.ObjectList)
+		if err := c.List(ctx, objectList, listOpts...); err != nil {
+			return nil
+		}
+
+		objects, err := meta.ExtractList(objectList)
+		if err != nil {
+			return nil
+		}
+
+		results := []ctrl.Request{}
+		for _, obj := range objects {
+			// Note: We don't check if the type cast succeeds as all items in an client.ObjectList
+			// are client.Objects.
+			o := obj.(client.Object)
+			results = append(results, ctrl.Request{
+				NamespacedName: client.ObjectKey{Namespace: o.GetNamespace(), Name: o.GetName()},
+			})
+		}
+		return results
+	}, nil
+}
+
 // isAPINamespaced detects if a GroupVersionKind is namespaced.
 func isAPINamespaced(gk schema.GroupVersionKind, restmapper meta.RESTMapper) (bool, error) {
 	restMapping, err := restmapper.RESTMapping(schema.GroupKind{Group: gk.Group, Kind: gk.Kind})
diff --git a/util/util_test.go b/util/util_test.go
@@ -739,7 +739,7 @@ func TestClusterToObjectsMapper(t *testing.T) {
 		restMapper.Add(gvk, meta.RESTScopeNamespace)
 
 		client := fake.NewClientBuilder().WithObjects(tc.objects...).WithRESTMapper(restMapper).Build()
-		f, err := ClusterToObjectsMapper(client, tc.input, scheme)
+		f, err := ClusterToTypedObjectsMapper(client, tc.input, scheme)
 		g.Expect(err != nil, err).To(Equal(tc.expectError))
 		g.Expect(f(ctx, cluster)).To(ConsistOf(tc.output))
 	}

Original file line number	Diff line number	Diff line change
`@@ -158,7 +158,7 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques`
`158`	`158`	`}`
`159`	`159`
`160`	`160`	`// Look up the owner of this kubeadm config if there is one`
`161`		`- configOwner, err := bsutil.GetConfigOwnerFromCache(ctx, r.Client, config)`
	`161`	`+ configOwner, err := bsutil.GetTypedConfigOwner(ctx, r.Client, config)`
`162`	`162`	`if apierrors.IsNotFound(err) {`
`163`	`163`	`// Could not find the owner yet, this is not an error and will rereconcile when the owner gets set.`
`164`	`164`	`return ctrl.Result{}, nil`
Original file line number	Diff line number	Diff line change
`@@ -175,7 +175,7 @@ func TestGetConfigOwner(t *testing.T) {`
`175`	`175`	`doTests(t, GetConfigOwner)`
`176`	`176`	`})`
`177`	`177`	`t.Run("cached", func(t *testing.T) {`
`178`		`- doTests(t, GetConfigOwnerFromCache)`
	`178`	`+ doTests(t, GetTypedConfigOwner)`
`179`	`179`	`})`
`180`	`180`	`}`
`181`	`181`
Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ type MachinePoolReconciler struct {`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {`
`77`		`- clusterToMachinePools, err := util.ClusterToObjectsMapper(mgr.GetClient(), &expv1.MachinePoolList{}, mgr.GetScheme())`
	`77`	`+ clusterToMachinePools, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &expv1.MachinePoolList{}, mgr.GetScheme())`
`78`	`78`	`if err != nil {`
`79`	`79`	`return err`
`80`	`80`	`}`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ type Reconciler struct {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {`
`95`		`- clusterToMachines, err := util.ClusterToObjectsMapper(mgr.GetClient(), &clusterv1.MachineList{}, mgr.GetScheme())`
	`95`	`+ clusterToMachines, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &clusterv1.MachineList{}, mgr.GetScheme())`
`96`	`96`	`if err != nil {`
`97`	`97`	`return err`
`98`	`98`	`}`
Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,7 @@ type Reconciler struct {`
`75`	`75`	`}`
`76`	`76`
`77`	`77`	`func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {`
`78`		`- clusterToMachineDeployments, err := util.ClusterToObjectsMapper(mgr.GetClient(), &clusterv1.MachineDeploymentList{}, mgr.GetScheme())`
	`78`	`+ clusterToMachineDeployments, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &clusterv1.MachineDeploymentList{}, mgr.GetScheme())`
`79`	`79`	`if err != nil {`
`80`	`80`	`return err`
`81`	`81`	`}`
Original file line number	Diff line number	Diff line change
`@@ -143,7 +143,7 @@ func (r *DockerMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Re`
`143`	`143`
`144`	`144`	`// SetupWithManager will add watches for this controller.`
`145`	`145`	`func (r *DockerMachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {`
`146`		`- clusterToDockerMachinePools, err := util.ClusterToObjectsMapper(mgr.GetClient(), &infraexpv1.DockerMachinePoolList{}, mgr.GetScheme())`
	`146`	`+ clusterToDockerMachinePools, err := util.ClusterToTypedObjectsMapper(mgr.GetClient(), &infraexpv1.DockerMachinePoolList{}, mgr.GetScheme())`
`147`	`147`	`if err != nil {`
`148`	`148`	`return err`
`149`	`149`	`}`