Skip to content

Commit

Permalink
fix(appset): Retry on conflict when updating status
Browse files Browse the repository at this point in the history 
  # 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 <[email protected]>
  • Loading branch information
@carlosrejano
carlosrejano authored and Carlos Rejano committed Aug 23, 2024
1 parent a0a5a18 commit f60af7b
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 39 deletions.
117 changes: 79 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 @@ -422,20 +423,28 @@ 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
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 @@ -1244,15 +1253,28 @@ 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
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 conditions
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 @@ -1267,21 +1289,30 @@ func (r *ApplicationSetReconciler) updateResourcesStatus(ctx context.Context, lo
statuses = append(statuses, status)
}
appset.Status.Resources = statuses
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)
}

namespacedName := types.NamespacedName{Namespace: appset.Namespace, Name: appset.Name}
err := r.Client.Status().Update(ctx, appset)
updatedAppset.Status.Resources = appset.Status.Resources

// Update the newly fetched object with new set of conditions
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 @@ -1316,20 +1347,30 @@ func (r *ApplicationSetReconciler) setAppSetApplicationStatus(ctx context.Contex
for i := range applicationStatuses {
applicationSet.Status.SetApplicationStatus(applicationStatuses[i])
}

// Update the newly fetched object with new set of ApplicationStatus
err := r.Client.Status().Update(ctx, applicationSet)
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 conditions
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 @@ -2375,7 +2375,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()

r := ApplicationSetReconciler{
Client: client,
Expand Down

0 comments on commit f60af7b

Please sign in to comment.