Use RetryOnError

securesign · Nov 11, 2024 · 85655be · 85655be
1 parent 5cb7c50
commit 85655be
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 26 deletions.
diff --git a/internal/controller/common/action/base_action.go b/internal/controller/common/action/base_action.go
@@ -7,9 +7,9 @@ import (
 	"maps"
 	"reflect"
 	"strconv"
-	"strings"
 	"time"
 
+	"github.com/securesign/operator/internal/apis"
 	"github.com/securesign/operator/internal/controller/annotations"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apiErrors "k8s.io/apimachinery/pkg/api/errors"
@@ -24,9 +24,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
-// OptimisticLockErrorMsg - ignore update error: https://github.com/kubernetes/kubernetes/issues/28149
-const OptimisticLockErrorMsg = "the object has been modified; please apply your changes to the latest version and try again"
-
 type EnsureOption func(current client.Object, expected client.Object) error
 
 type BaseAction struct {
@@ -51,34 +48,53 @@ func (action *BaseAction) Continue() *Result {
 	return nil
 }
 
-func (action *BaseAction) StatusUpdate(ctx context.Context, obj client2.Object) *Result {
-	if err := action.Client.Status().Update(ctx, obj); err != nil {
-		if strings.Contains(err.Error(), OptimisticLockErrorMsg) {
-			return &Result{Result: reconcile.Result{RequeueAfter: 1 * time.Second}, Err: nil}
+func (action *BaseAction) StatusUpdate(ctx context.Context, expected client2.Object) *Result {
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		var (
+			current client2.Object
+			ok      bool
+		)
+		if current, ok = expected.DeepCopyObject().(client2.Object); !ok {
+			return errors.New("can't create DeepCopy object")
 		}
-		return action.Failed(err)
-	}
-	// Requeue will be caused by update
-	return &Result{Result: reconcile.Result{Requeue: false}}
+
+		if getErr := action.Client.Get(ctx, client2.ObjectKeyFromObject(expected), current); getErr != nil {
+			return getErr
+		}
+
+		currentStatus := reflect.ValueOf(current).Elem().FieldByName("Status")
+		expectedStatus := reflect.ValueOf(expected).Elem().FieldByName("Status")
+		if currentStatus == reflect.ValueOf(nil) {
+			// object without Status
+			return errors.New("can't find Status field on expected object")
+		}
+		if !expectedStatus.IsValid() || !currentStatus.IsValid() {
+			return errors.New("status is not valid")
+		}
+		if !currentStatus.CanSet() {
+			return errors.New("can't set expected Status to current object")
+		}
+
+		currentStatus.Set(expectedStatus)
+
+		return action.Client.Status().Update(ctx, current)
+
+	})
+	return &Result{Result: reconcile.Result{Requeue: false}, Err: err}
 }
 
 func (action *BaseAction) Failed(err error) *Result {
 	action.Logger.Error(err, "error during action execution")
+	// If the returned error is non-nil, the Result is ignored and the request will be
+	// requeued using exponential backoff.
 	return &Result{
-		Result: reconcile.Result{RequeueAfter: time.Duration(5) * time.Second},
-		Err:    err,
+		Err: err,
 	}
 }
 
-func (action *BaseAction) FailedWithStatusUpdate(ctx context.Context, err error, instance client2.Object) *Result {
-	if e := action.Client.Status().Update(ctx, instance); e != nil {
-		if strings.Contains(err.Error(), OptimisticLockErrorMsg) {
-			return &Result{Result: reconcile.Result{RequeueAfter: 1 * time.Second}, Err: err}
-		}
-		err = errors.Join(e, err)
-	}
-	// Requeue will be caused by update
-	return &Result{Result: reconcile.Result{Requeue: false}, Err: err}
+func (action *BaseAction) FailedWithStatusUpdate(ctx context.Context, err error, instance apis.ConditionsAwareObject) *Result {
+	action.StatusUpdate(ctx, instance)
+	return action.Failed(err)
 }
 
 func (action *BaseAction) Return() *Result {

diff --git a/internal/controller/tsa/actions/ntpMonitoring_test.go b/internal/controller/tsa/actions/ntpMonitoring_test.go
@@ -174,6 +174,7 @@ func Test_NTPHandle(t *testing.T) {
 
 				g.Expect(instance.Status.NTPMonitoring.Config.NtpConfigRef.Name).To(Equal(cm.Name), "Config Map name mismatch")
 
+				g.Expect(client.Get(context.TODO(), types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()}, instance)).To(Succeed())
 				instance.Spec.NTPMonitoring.Config.NumServers = 2
 				err = client.Update(context.TODO(), instance)
 				g.Expect(err).NotTo(HaveOccurred(), "Error updating instance")
@@ -205,6 +206,7 @@ func Test_NTPHandle(t *testing.T) {
 
 				oldConfigMapName := instance.Status.NTPMonitoring.Config.NtpConfigRef.Name
 
+				g.Expect(client.Get(context.TODO(), types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()}, instance)).To(Succeed())
 				instance.Spec.NTPMonitoring.Config.NumServers = 2
 				err = client.Update(context.TODO(), instance)
 				g.Expect(err).NotTo(HaveOccurred(), "Error updating instance")

diff --git a/internal/testing/action/result.go b/internal/testing/action/result.go
@@ -17,13 +17,12 @@ func StatusUpdate() *action.Result {
 
 func Failed(err error) *action.Result {
 	return &action.Result{
-		Result: reconcile.Result{RequeueAfter: time.Duration(5) * time.Second},
-		Err:    err,
+		Err: err,
 	}
 }
 
 func FailedWithStatusUpdate(err error) *action.Result {
-	return &action.Result{Result: reconcile.Result{Requeue: false}, Err: err}
+	return Failed(err)
 }
 
 func Return() *action.Result {