Skip to content

Commit

Permalink
fix(appset): Retry on conflict when updating status (argoproj#19663)
Browse files Browse the repository at this point in the history 
* 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 argoproj#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:
    kubernetes-sigs/controller-runtime#2386,
    and
    kubernetes-sigs/controller-runtime#2362.

Signed-off-by: Carlos Rejano <carlos.rejano@adevinta.com>

* fixup! fix(appset): Retry on conflict when updating status

---------

Signed-off-by: Carlos Rejano <carlos.rejano@adevinta.com>
Signed-off-by: carlosrejano <59321132+carlosrejano@users.noreply.github.com>
Co-authored-by: Carlos Rejano <carlos.rejano@adevinta.com>
  • Loading branch information
@carlosrejano
carlosrejano and Carlos Rejano committed Sep 7, 2024
1 parent 4736657 commit 71bbdcc
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 39 deletions.
120 changes: 82 additions & 38 deletions applicationset/controllers/applicationset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion applicationset/controllers/applicationset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down

0 comments on commit 71bbdcc

Please sign in to comment.