From a7b9c0865746f864e6b00bcdae2b62309d1358a5 Mon Sep 17 00:00:00 2001
From: Bryan Venteicher <bryanventeicher@gmail.com>
Date: Tue, 10 Sep 2024 10:12:19 -0500
Subject: [PATCH] Use cached Get() of PVCs in Volume controller

The uncached Get() of every VM's PVCs on each reconcile causes an
unacceptable perf regression.

The memory limit bump fallout from this change will be addressed later.

For now, keep the deferred instance-storage specific cache Watch(). It
only matches IS PVCs via the label selector its cost is negligible but
can be removed in a later commit.
---
 .../volume/volume_controller.go               |  6 +----
 .../volume/volume_controller_unit_test.go     | 22 -------------------
 2 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/controllers/virtualmachine/volume/volume_controller.go b/controllers/virtualmachine/volume/volume_controller.go
index 117c37ed9..ae5feacf7 100644
--- a/controllers/virtualmachine/volume/volume_controller.go
+++ b/controllers/virtualmachine/volume/volume_controller.go
@@ -70,7 +70,6 @@ func AddToManager(ctx *pkgctx.ControllerManagerContext, mgr manager.Manager) err
 	r := NewReconciler(
 		ctx,
 		mgr.GetClient(),
-		mgr.GetAPIReader(),
 		ctrl.Log.WithName("controllers").WithName("volume"),
 		record.New(mgr.GetEventRecorderFor(controllerNameLong)),
 		ctx.VMProvider,
@@ -177,14 +176,12 @@ func AddToManager(ctx *pkgctx.ControllerManagerContext, mgr manager.Manager) err
 func NewReconciler(
 	ctx context.Context,
 	client client.Client,
-	reader client.Reader,
 	logger logr.Logger,
 	recorder record.Recorder,
 	vmProvider providers.VirtualMachineProviderInterface) *Reconciler {
 	return &Reconciler{
 		Context:    ctx,
 		Client:     client,
-		reader:     reader,
 		logger:     logger,
 		recorder:   recorder,
 		VMProvider: vmProvider,
@@ -195,7 +192,6 @@ var _ reconcile.Reconciler = &Reconciler{}
 
 type Reconciler struct {
 	client.Client
-	reader     client.Reader
 	Context    context.Context
 	logger     logr.Logger
 	recorder   record.Recorder
@@ -666,7 +662,7 @@ func (r *Reconciler) processAttachments(
 			if volume.PersistentVolumeClaim.ClaimName == attachment.Spec.VolumeName {
 				volumeStatus := attachmentToVolumeStatus(volume.Name, attachment)
 				volumeStatus.Used = existingManagedVolUsage[volume.Name]
-				if err := updateVolumeStatusWithLimit(ctx, r.reader, *volume.PersistentVolumeClaim, &volumeStatus); err != nil {
+				if err := updateVolumeStatusWithLimit(ctx, r.Client, *volume.PersistentVolumeClaim, &volumeStatus); err != nil {
 					ctx.Logger.Error(err, "failed to get volume status limit")
 				}
 				volumeStatuses = append(volumeStatuses, volumeStatus)
diff --git a/controllers/virtualmachine/volume/volume_controller_unit_test.go b/controllers/virtualmachine/volume/volume_controller_unit_test.go
index a4838fedf..caad41abe 100644
--- a/controllers/virtualmachine/volume/volume_controller_unit_test.go
+++ b/controllers/virtualmachine/volume/volume_controller_unit_test.go
@@ -45,24 +45,6 @@ func (f *testFailClient) Create(ctx context.Context, obj client.Object, opts ...
 	return apierrors.NewForbidden(schema.GroupResource{}, "", errors.New("insufficient quota for creating PVC"))
 }
 
-type noPVCReader struct {
-	client.Client
-}
-
-func (c *noPVCReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	if _, ok := obj.(*corev1.PersistentVolumeClaim); ok {
-		panic("Don't Get() PersistentVolumeClaim with this Client!")
-	}
-	return c.Client.Get(ctx, key, obj, opts...)
-}
-
-func (c *noPVCReader) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
-	if _, ok := list.(*corev1.PersistentVolumeClaimList); ok {
-		panic("Don't List() PersistentVolumeClaim with this Client!")
-	}
-	return c.Client.List(ctx, list, opts...)
-}
-
 func unitTests() {
 	Describe(
 		"Reconcile",
@@ -164,12 +146,8 @@ func unitTestsReconcile() {
 				}).
 			Build()
 
-		// Any PVC Get/List should not use this client so we don't start an informer.
-		noPVCClient := &noPVCReader{ctx.Client}
-
 		reconciler = volume.NewReconciler(
 			ctx,
-			noPVCClient,
 			ctx.Client,
 			ctx.Logger,
 			ctx.Recorder,