From 71bbdccacf99dba1a71ce74d86991d9a704eed61 Mon Sep 17 00:00:00 2001 From: carlosrejano <59321132+carlosrejano@users.noreply.github.com> Date: Sat, 7 Sep 2024 17:25:39 +0200 Subject: [PATCH] fix(appset): Retry on conflict when updating status (#19663) * fix(appset): Retry on conflict when updating status # Context: When updating the status of the applicationset object it can happen that it fails due to a conflict since the resourceVersion has changed due to a different update. This makes the reconcile fails and we need to wait until the following reconcile loop until it updates the relevant status fields and hope that the update calls don't fail again due a conflict. It can even happen that it gets stuck constantly due to this erriors. A better approach I would say is retrying when there is a conflict error with the newest version of the object, so we make sure we update the object with the latest version always. This has been raised in issue #19535 that failing due to conflicts can make the reconcile not able to proceed. # What does this PR? - Wraps all the `Update().Status` calls inside a retry function that will retry when the update fails due a conflict. - Adds appset to fake client subresources, if not the client can not correctly determine the status subresource. Refer to: https://github.com/kubernetes-sigs/controller-runtime/issues/2386, and https://github.com/kubernetes-sigs/controller-runtime/issues/2362. Signed-off-by: Carlos Rejano * fixup! fix(appset): Retry on conflict when updating status --------- Signed-off-by: Carlos Rejano Signed-off-by: carlosrejano <59321132+carlosrejano@users.noreply.github.com> Co-authored-by: Carlos Rejano --- .../controllers/applicationset_controller.go | 120 ++++++++++++------ .../applicationset_controller_test.go | 2 +- 2 files changed, 83 insertions(+), 39 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 1ba523ef74b1d..70fa60e97a0f6 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -32,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -427,20 +428,29 @@ func (r *ApplicationSetReconciler) setApplicationSetStatusCondition(ctx context. if needToUpdateConditions || len(applicationSet.Status.Conditions) < len(newConditions) { // fetch updated Application Set object before updating it - namespacedName := types.NamespacedName{Namespace: applicationSet.Namespace, Name: applicationSet.Name} - if err := r.Get(ctx, namespacedName, applicationSet); err != nil { - if client.IgnoreNotFound(err) != nil { - return nil + // DefaultRetry will retry 5 times with a backoff factor of 1, jitter of 0.1 and a duration of 10ms + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + namespacedName := types.NamespacedName{Namespace: applicationSet.Namespace, Name: applicationSet.Name} + updatedAppset := &argov1alpha1.ApplicationSet{} + if err := r.Get(ctx, namespacedName, updatedAppset); err != nil { + if client.IgnoreNotFound(err) != nil { + return nil + } + return fmt.Errorf("error fetching updated application set: %w", err) } - return fmt.Errorf("error fetching updated application set: %w", err) - } - applicationSet.Status.SetConditions( - newConditions, evaluatedTypes, - ) + updatedAppset.Status.SetConditions( + newConditions, evaluatedTypes, + ) - // Update the newly fetched object with new set of conditions - err := r.Client.Status().Update(ctx, applicationSet) + // Update the newly fetched object with new set of conditions + err := r.Client.Status().Update(ctx, updatedAppset) + if err != nil { + return err + } + updatedAppset.DeepCopyInto(applicationSet) + return nil + }) if err != nil && !apierr.IsNotFound(err) { return fmt.Errorf("unable to set application set condition: %w", err) } @@ -1249,15 +1259,29 @@ func (r *ApplicationSetReconciler) migrateStatus(ctx context.Context, appset *ar } if update { - namespacedName := types.NamespacedName{Namespace: appset.Namespace, Name: appset.Name} - if err := r.Client.Status().Update(ctx, appset); err != nil { - return fmt.Errorf("unable to set application set status: %w", err) - } - if err := r.Get(ctx, namespacedName, appset); err != nil { - if client.IgnoreNotFound(err) != nil { - return nil + // DefaultRetry will retry 5 times with a backoff factor of 1, jitter of 0.1 and a duration of 10ms + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + namespacedName := types.NamespacedName{Namespace: appset.Namespace, Name: appset.Name} + updatedAppset := &argov1alpha1.ApplicationSet{} + if err := r.Get(ctx, namespacedName, updatedAppset); err != nil { + if client.IgnoreNotFound(err) != nil { + return nil + } + return fmt.Errorf("error fetching updated application set: %w", err) } - return fmt.Errorf("error fetching updated application set: %w", err) + + updatedAppset.Status.ApplicationStatus = appset.Status.ApplicationStatus + + // Update the newly fetched object with new set of ApplicationStatus + err := r.Client.Status().Update(ctx, updatedAppset) + if err != nil { + return err + } + updatedAppset.DeepCopyInto(appset) + return nil + }) + if err != nil && !apierr.IsNotFound(err) { + return fmt.Errorf("unable to set application set condition: %w", err) } } return nil @@ -1272,21 +1296,31 @@ func (r *ApplicationSetReconciler) updateResourcesStatus(ctx context.Context, lo statuses = append(statuses, status) } appset.Status.Resources = statuses + // DefaultRetry will retry 5 times with a backoff factor of 1, jitter of 0.1 and a duration of 10ms + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + namespacedName := types.NamespacedName{Namespace: appset.Namespace, Name: appset.Name} + updatedAppset := &argov1alpha1.ApplicationSet{} + if err := r.Get(ctx, namespacedName, updatedAppset); err != nil { + if client.IgnoreNotFound(err) != nil { + return nil + } + return fmt.Errorf("error fetching updated application set: %w", err) + } + + updatedAppset.Status.Resources = appset.Status.Resources - namespacedName := types.NamespacedName{Namespace: appset.Namespace, Name: appset.Name} - err := r.Client.Status().Update(ctx, appset) + // Update the newly fetched object with new status resources + err := r.Client.Status().Update(ctx, updatedAppset) + if err != nil { + return err + } + updatedAppset.DeepCopyInto(appset) + return nil + }) if err != nil { logCtx.Errorf("unable to set application set status: %v", err) return fmt.Errorf("unable to set application set status: %w", err) } - - if err := r.Get(ctx, namespacedName, appset); err != nil { - if client.IgnoreNotFound(err) != nil { - return nil - } - return fmt.Errorf("error fetching updated application set: %w", err) - } - return nil } @@ -1321,20 +1355,30 @@ func (r *ApplicationSetReconciler) setAppSetApplicationStatus(ctx context.Contex for i := range applicationStatuses { applicationSet.Status.SetApplicationStatus(applicationStatuses[i]) } + // DefaultRetry will retry 5 times with a backoff factor of 1, jitter of 0.1 and a duration of 10ms + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + updatedAppset := &argov1alpha1.ApplicationSet{} + if err := r.Get(ctx, namespacedName, updatedAppset); err != nil { + if client.IgnoreNotFound(err) != nil { + return nil + } + return fmt.Errorf("error fetching updated application set: %w", err) + } + + updatedAppset.Status.ApplicationStatus = applicationSet.Status.ApplicationStatus - // Update the newly fetched object with new set of ApplicationStatus - err := r.Client.Status().Update(ctx, applicationSet) + // Update the newly fetched object with new set of ApplicationStatus + err := r.Client.Status().Update(ctx, updatedAppset) + if err != nil { + return err + } + updatedAppset.DeepCopyInto(applicationSet) + return nil + }) if err != nil { logCtx.Errorf("unable to set application set status: %v", err) return fmt.Errorf("unable to set application set status: %w", err) } - - if err := r.Get(ctx, namespacedName, applicationSet); err != nil { - if client.IgnoreNotFound(err) != nil { - return nil - } - return fmt.Errorf("error fetching updated application set: %w", err) - } } return nil diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index f11131dc33ef0..d5813d5f114a8 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -2397,7 +2397,7 @@ func TestSetApplicationSetStatusCondition(t *testing.T) { argoObjs := []runtime.Object{} for _, testCase := range testCases { - client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&testCase.appset).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build() + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&testCase.appset).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).WithStatusSubresource(&testCase.appset).Build() metrics := appsetmetrics.NewFakeAppsetMetrics(client) r := ApplicationSetReconciler{