storage capacity: patch vs. update of CSIStorageCapacity

[pohly]

capacity.go currently uses Update to change the capacity value. It might be more efficient to use Patch:

• write a benchmark
• compare both approaches

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[pohly]

/remove-lifecycle stale
/lifecycle frozen

[pohly]

The benchmark is in PR #706.

Here's a preliminary patch for using patching (sic!):

diff --git a/pkg/capacity/capacity.go b/pkg/capacity/capacity.go
index 815b00679..8516164dd 100644
--- a/pkg/capacity/capacity.go
+++ b/pkg/capacity/capacity.go
@@ -25,6 +25,7 @@ import (
 	"sort"
 	"sync"
 	"time"
+	"strings"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/kubernetes-csi/external-provisioner/pkg/capacity/topology"
@@ -45,6 +46,7 @@ import (
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/component-base/metrics"
 	"k8s.io/klog/v2"
+	k8stypes "k8s.io/apimachinery/pkg/types"
 )
 
 const (
@@ -648,15 +650,24 @@ func (c *Controller) syncCapacity(ctx context.Context, item workItem) error {
 		return nil
 	} else {
 		// Update existing object. Must not modify object in the informer cache.
-		capacity := capacity.DeepCopy()
-		capacity.Capacity = quantity
-		capacity.MaximumVolumeSize = maximumVolumeSize
-		if c.owner != nil && !c.isOwnedByUs(capacity) {
-			capacity.OwnerReferences = append(capacity.OwnerReferences, *c.owner)
-		}
+		// capacity := capacity.DeepCopy()
+		// capacity.Capacity = quantity
+		// capacity.MaximumVolumeSize = maximumVolumeSize
+		// if c.owner != nil && !c.isOwnedByUs(capacity) {
+		// 	capacity.OwnerReferences = append(capacity.OwnerReferences, *c.owner)
+		// }
 		var err error
 		klog.V(5).Infof("Capacity Controller: updating %s for %+v, new capacity %v, new maximumVolumeSize %v", capacity.Name, item, quantity, maximumVolumeSize)
-		capacity, err = c.client.StorageV1beta1().CSIStorageCapacities(capacity.Namespace).Update(ctx, capacity, metav1.UpdateOptions{})
+		ops := []string {
+			fmt.Sprintf(`{"op":"replace","path":"/capacity","value":"%d"}`, quantity.Value()),
+		}
+		if maximumVolumeSize != nil {
+			ops = append(ops, fmt.Sprintf(`{"op":"replace","path":"/maximumVolumeSize","value":"%d"}`, maximumVolumeSize.Value()))
+		} else if capacity.MaximumVolumeSize != nil {
+			ops = append(ops, `{"op":"remove","path":"/maximumVolumeSize"}`)
+		}
+		patch := "[" + strings.Join(ops, ",") + "]"
+		capacity, err := c.client.StorageV1beta1().CSIStorageCapacities(capacity.Namespace).Patch(ctx, capacity.Name, k8stypes.JSONPatchType, []byte(patch), metav1.PatchOptions{}, "")
 		if err != nil {
 			return fmt.Errorf("update CSIStorageCapacity for %+v: %v", item, err)
 		}

It's not complete because it doesn't handle adding the ownerref, but that is irrelevant for the benchmark.

Comparing the baseline against the modified version shows no improvement:

$ $GOPATH/bin/benchstat /tmp/log /tmp/log-patch 
name               old time/op  new time/op  delta
CapacityUpdate-36  3.93ms ± 1%  3.92ms ± 1%   ~     (p=0.548 n=5+5)

I've not tried to measured load on the apiserver. Processing the patch must be more work, so I would expect the load to become higher.

My conclusion is that the time for updating is limited by roundtrip times and overhead of request processing. Making the payload for a single request smaller doesn't help here, probably because a single CSIStorageCapacity object isn't large to begin with.

I therefore suggest to close this issue here by merging the benchmark PR, without changing the implementation.