From e463cf9c7fe5f9f7a4e5feb968ddd68f288175b8 Mon Sep 17 00:00:00 2001 From: Conor Nolan Date: Thu, 19 Dec 2024 15:56:46 +0000 Subject: [PATCH] Simplify updateBucketCR (#307) --- internal/controller/bucket/create.go | 10 ++--- internal/controller/bucket/delete.go | 4 +- internal/controller/bucket/helpers.go | 63 ++++++++------------------- internal/controller/bucket/update.go | 4 +- 4 files changed, 26 insertions(+), 55 deletions(-) diff --git a/internal/controller/bucket/create.go b/internal/controller/bucket/create.go index 84fc2700..a637c9e3 100644 --- a/internal/controller/bucket/create.go +++ b/internal/controller/bucket/create.go @@ -63,7 +63,7 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext // Of course, this approach does not completely remove the possibility of us finding ourselves in // the above scenario. It only mitigates it. As long as Crossplane persists with its existing logic // then we can only make a "best-effort" to avoid it. - if err := c.updateBucketCR(ctx, bucket, func(_, bucketLatest *v1alpha1.Bucket) UpdateRequired { + if err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { meta.RemoveAnnotations(bucket, meta.AnnotationKeyExternalCreatePending) return NeedsObjectUpdate @@ -195,7 +195,7 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext // Bucket CR while there are no backends for us to create on. if backendCount == 0 { c.log.Info("Failed to find any backend for bucket", consts.KeyBucketName, bucket.Name) - if err := c.updateBucketCR(ctx, bucket, func(_, bucketLatest *v1alpha1.Bucket) UpdateRequired { + if err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { // Although no backends were found for the bucket, we still apply the backend // label to the Bucket CR for each backend that the bucket was intended to be // created on. This is to ensure the bucket will eventually be created on these @@ -243,11 +243,11 @@ func (c *external) waitForCreationAndUpdateBucketCR(ctx context.Context, bucket // 2. The Bucket CR Status with the Ready condition. // 3. The Bucket CR Status Backends with a Ready condition for the backend the bucket // was created on. - err := c.updateBucketCR(ctx, bucket, func(_, bucketLatest *v1alpha1.Bucket) UpdateRequired { + err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { setAllBackendLabels(bucketLatest, allBackendsToCreateOn) return NeedsObjectUpdate - }, func(_, bucketLatest *v1alpha1.Bucket) UpdateRequired { + }, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { bucketLatest.Status.SetConditions(xpv1.Available()) bucketLatest.Status.AtProvider.Backends = v1alpha1.Backends{ beName: &v1alpha1.BackendInfo{ @@ -281,7 +281,7 @@ func (c *external) waitForCreationAndUpdateBucketCR(ctx context.Context, bucket // Update the Bucket CR Status condition to Unavailable. This means the Bucket CR will // not be seen as Ready. If that update is successful, we return the createErr which will // be the most recent error receieved from a backend's failed creation. - if err := c.updateBucketCR(ctx, bucket, func(_, bucketLatest *v1alpha1.Bucket) UpdateRequired { + if err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { bucketLatest.Status.SetConditions(xpv1.Unavailable()) return NeedsStatusUpdate diff --git a/internal/controller/bucket/delete.go b/internal/controller/bucket/delete.go index e08e176e..410b3543 100644 --- a/internal/controller/bucket/delete.go +++ b/internal/controller/bucket/delete.go @@ -105,7 +105,7 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext // the bucket CR. This is done by setting the Disabled flag on the bucket // CR spec. If the deletion is successful or unsuccessful, the bucket CR status must be // updated. - if err := c.updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { + if err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { setBucketStatus(bucketLatest, bucketBackends, providerNames, c.minReplicas) return NeedsStatusUpdate @@ -130,7 +130,7 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext if !bucket.Spec.Disabled { return managed.ExternalDelete{}, nil } - if err := c.updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { + if err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired { c.log.Info("Bucket CRs with non-empty buckets should not be disabled - setting 'disabled' flag to false", consts.KeyBucketName, bucket.Name) bucketLatest.Spec.Disabled = false diff --git a/internal/controller/bucket/helpers.go b/internal/controller/bucket/helpers.go index 7a7dcead..838385a2 100644 --- a/internal/controller/bucket/helpers.go +++ b/internal/controller/bucket/helpers.go @@ -211,70 +211,41 @@ const ( // updateBucketCR updates the Bucket CR and/or the Bucket CR Status by applying a series of callbacks. // The function uses an exponential backoff retry mechanism to handle potential conflicts during updates. // -// The callbacks take two Bucket parameters. Before the callbacks are called, the first Bucket -// parameter will become a DeepCopy of bucket. The second will become the latest version of bucket, as it is fetched -// from the Kube API. Each callback function should aim to update the latest version of the bucket (second parameter) -// with the changes which will be persisted in bucket (and as a result, it's DeepCopy). -// // Callbacks return an UpdateRequired status, depending on whether the update that is performed by the callback // requires a Bucket Status update (NeedsStatusUpdate) or a full Bucket object update (NeedsObjectUpdate). // This enables updateObject to make a decision on whether to perform kubeclient.Status().Update() or // kubeClient.Update() respectively. // -// Callback example 1, updating the latest version of bucket Status with a field from your version of bucket. -// This callback only performs an update to the Bucket Status, so NeedsStatusUpdate is returned to enabled -// updateBucketCR to perform kubeClient.Status().Update(). -// -// func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { -// bucketLatest.Status.SomeField = bucketDeepCopy.Status.SomeField -// -// return NeedsStatusUpdate -// }, -// -// Callback example 2, updating the latest version of bucket Status with a string: -// -// func(_, bucketLatest *v1alpha1.Bucket) { -// bucketLatest.Status.SomeOtherField = "some-value" +// Callback example, updating the latest version of bucket Status with a string, so NeedsStatusUpdate is +// returned to enabled updateBucketCR to perform kubeClient.Status().Update(). // -// return NeedsStatusUpdate -// }, +// func(bucketLatest *v1alpha1.Bucket) { +// bucketLatest.Status.SomeOtherField = "some-value" // -// Callback example 3, updating the latest version of bucket Spec with a field from your version of the bucket. -// This callback performs an update to the Bucket Spec, so NeedsObjectUpdate is returned to enabled updateBucketCR -// to perform a full kubeClient.Update(). +// return NeedsStatusUpdate +// }, // -// func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { -// bucketLatest.Spec.SomeField = bucketDeepCopy.Spec.SomeField +// Example usage with above callback example: // -// return NeedsObjectUpdate -// }, +// err := updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) { +// bucketLatest.Status.SomeOtherField = "some-value" // -// Example usage with above callback example 3: +// return NeedsStatusUpdate +// }) // -// err := updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) { -// bucketLatest.Spec.SomeField = bucketDeepCopy.Spec.SomeField -// -// return NeedsObjectUpdate -// }) -// -// if err != nil { -// // Handle error -// } -func (c *external) updateBucketCR(ctx context.Context, bucket *v1alpha1.Bucket, callbacks ...func(*v1alpha1.Bucket, *v1alpha1.Bucket) UpdateRequired) error { +// if err != nil { +// // Handle error +// } +func (c *external) updateBucketCR(ctx context.Context, bucket *v1alpha1.Bucket, callbacks ...func(*v1alpha1.Bucket) UpdateRequired) error { ctx, span := otel.Tracer("").Start(ctx, "bucket.external.updateBucketCR") defer span.End() - bucketDeepCopy := bucket.DeepCopy() - - nn := types.NamespacedName{Name: bucket.GetName()} - for _, cb := range callbacks { err := retry.OnError(retry.DefaultRetry, resource.IsAPIError, func() error { - if err := c.kubeClient.Get(ctx, nn, bucket); err != nil { + if err := c.kubeClient.Get(ctx, types.NamespacedName{Name: bucket.GetName()}, bucket); err != nil { return err } - - switch cb(bucketDeepCopy, bucket) { + switch cb(bucket) { case NeedsStatusUpdate: return c.kubeClient.Status().Update(ctx, bucket) case NeedsObjectUpdate: diff --git a/internal/controller/bucket/update.go b/internal/controller/bucket/update.go index 24f71d83..dd2a97da 100644 --- a/internal/controller/bucket/update.go +++ b/internal/controller/bucket/update.go @@ -86,7 +86,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Bucket CR Status in all cases to represent the conditions of each individual bucket. cls := c.backendStore.GetBackendS3Clients(allBackendsToUpdateOn) if err := c.updateBucketCR(ctx, bucket, - func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { + func(bucketLatest *v1alpha1.Bucket) UpdateRequired { setBucketStatus(bucketLatest, bucketBackends, allBackendsToUpdateOn, c.minReplicas) return NeedsStatusUpdate @@ -100,7 +100,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // The buckets have been updated successfully on all backends, so we need to update the // Bucket CR Spec accordingly. err := c.updateBucketCR(ctx, bucket, - func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { + func(bucketLatest *v1alpha1.Bucket) UpdateRequired { if bucketLatest.ObjectMeta.Labels == nil { bucketLatest.ObjectMeta.Labels = map[string]string{} }