Fix review findings

Author: sbueringer

internal/util/client/client.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/klog/v2"
@@ -34,13 +35,14 @@ import (
 )
 
 var (
-	// waitForCacheTimeout is the timeout used when waiting for the cache to become up-to-date.
-	waitForCacheTimeout = 10 * time.Second
-
-	// waitForCacheInterval is the timeout used when waiting for the cache to become up-to-date.
-	// This interval seems pretty low, but based on tests it's realistic that the cache is up-to-date
-	// that quickly.
-	waitForCacheInterval = 100 * time.Microsecond
+	// waitBackoff is the timeout used when waiting for the cache to become up-to-date.
+	// This adds up to ~ 10 seconds max wait duration.
+	waitBackoff = wait.Backoff{
+		Duration: 25 * time.Microsecond,
+		Cap:      2 * time.Second,
+		Factor:   1.2,
+		Steps:    63,
+	}
 )
 
 // WaitForCacheToBeUpToDate waits until the cache is up-to-date in the sense of that the cache contains
@@ -127,10 +129,15 @@ func waitFor[T client.Object](ctx context.Context, c client.Client, action strin
 		return nil
 	}
 
+	var o any = objs[0]
+	if _, ok := o.(*unstructured.Unstructured); ok {
+		return errors.Errorf("failed to wait for up-to-date objects in the cache after %s: Unstructured is not supported", action)
+	}
+
 	// All objects have the same type, so we can just take the GVK of the first object.
 	objGVK, err := apiutil.GVKForObject(objs[0], c.Scheme())
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "failed to wait for up-to-date objects in the cache after %s", action)
 	}
 
 	log := ctrl.LoggerFrom(ctx)
@@ -147,7 +154,7 @@ func waitFor[T client.Object](ctx context.Context, c client.Client, action strin
 	now := time.Now()
 
 	var pollErrs []error
-	err = wait.PollUntilContextTimeout(ctx, waitForCacheInterval, waitForCacheTimeout, true, func(ctx context.Context) (bool, error) {
+	err = wait.ExponentialBackoffWithContext(ctx, waitBackoff, func(ctx context.Context) (bool, error) {
 		pollErrs = nil
 
 		for _, desiredObj := range desiredObjects {
@@ -177,7 +184,7 @@ func waitFor[T client.Object](ctx context.Context, c client.Client, action strin
 		var errSuffix string
 		if err != nil {
 			if wait.Interrupted(err) {
-				errSuffix = fmt.Sprintf(": timed out after %s", waitForCacheTimeout)
+				errSuffix = ": timed out"
 			} else {
 				errSuffix = fmt.Sprintf(": %s", err.Error())
 			}

internal/util/client/client_test.go

@@ -26,8 +26,10 @@ import (
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -38,7 +40,12 @@ import (
 
 func Test_WaitForCacheToBeUpToDate(t *testing.T) {
 	// Modify timeout to speed up test
-	waitForCacheTimeout = 1 * time.Second
+	waitBackoff = wait.Backoff{
+		Duration: 25 * time.Microsecond,
+		Cap:      2 * time.Second,
+		Factor:   1.2,
+		Steps:    5,
+	}
 
 	tests := []struct {
 		name            string
@@ -92,7 +99,7 @@ func Test_WaitForCacheToBeUpToDate(t *testing.T) {
 				machine("machine-4", "4", nil),
 			},
 			clientResponses: map[client.ObjectKey][]client.Object{},
-			wantErr: "failed to wait for up-to-date Machine objects in the cache after Machine creation: timed out after 1s: [" +
+			wantErr: "failed to wait for up-to-date Machine objects in the cache after Machine creation: timed out: [" +
 				"machines.cluster.x-k8s.io \"machine-1\" not found, " +
 				"machines.cluster.x-k8s.io \"machine-2\" not found, " +
 				"machines.cluster.x-k8s.io \"machine-3\" not found, " +
@@ -201,7 +208,12 @@ func Test_WaitForCacheToBeUpToDate(t *testing.T) {
 
 func Test_WaitForObjectsToBeDeletedFromTheCache(t *testing.T) {
 	// Modify timeout to speed up test
-	waitForCacheTimeout = 1 * time.Second
+	waitBackoff = wait.Backoff{
+		Duration: 25 * time.Microsecond,
+		Cap:      2 * time.Second,
+		Factor:   1.2,
+		Steps:    5,
+	}
 
 	tests := []struct {
 		name            string
@@ -212,6 +224,13 @@ func Test_WaitForObjectsToBeDeletedFromTheCache(t *testing.T) {
 		{
 			name: "no-op if no objects are passed in",
 		},
+		{
+			name: "error if Unstructured is used",
+			objs: []client.Object{
+				&unstructured.Unstructured{},
+			},
+			wantErr: "failed to wait for up-to-date objects in the cache after Machine deletion: Unstructured is not supported",
+		},
 		{
 			name: "success if objects are going away instantly (not found)",
 			objs: []client.Object{
@@ -306,7 +325,7 @@ func Test_WaitForObjectsToBeDeletedFromTheCache(t *testing.T) {
 					machine("machine-4", "7", nil),
 				},
 			},
-			wantErr: "failed to wait for up-to-date Machine objects in the cache after Machine deletion: timed out after 1s: [" +
+			wantErr: "failed to wait for up-to-date Machine objects in the cache after Machine deletion: timed out: [" +
 				"default/machine-1 still exists, " +
 				"default/machine-2 still exists, " +
 				"default/machine-3 still exists, " +

internal/util/client/metrics.go

@@ -30,6 +30,6 @@ var (
 	waitDurationMetric = prometheus.NewHistogramVec(prometheus.HistogramOpts{
 		Name:    "capi_client_cache_wait_duration_seconds",
 		Help:    "Duration that we waited for the cache to be up-to-date in seconds, broken down by kind and status",
-		Buckets: []float64{0.00001, 0.000025, 0.00005, 0.0001, 0.00025, 0.0005, 0.001, 0.005, 0.025, 0.05, 0.1, 0.2, 0.4, 0.6, 0.8, 1.0},
+		Buckets: []float64{0.00001, 0.000025, 0.00005, 0.0001, 0.00025, 0.0005, 0.001, 0.005, 0.025, 0.05, 0.1, 0.2, 0.4, 0.6, 0.8, 1.0, 2.5, 5, 10},
 	}, []string{"kind", "status"})
 )