fix: retry all status updates due to controller contention (#114)

* fix: retry all status updates due to controller contention Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com> * fix: retry all status updates due to controller contention Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com> * test: add coverage Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com> --------- Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
validator-labs · Nov 16, 2023 · 35f03a4 · 35f03a4
1 parent acb9902
commit 35f03a4
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 31 deletions.
diff --git a/internal/controller/validationresult_controller.go b/internal/controller/validationresult_controller.go
@@ -32,12 +32,8 @@ import (
 	"github.com/spectrocloud-labs/validator/pkg/constants"
 )
 
-const (
-	// ValidationResultHash is used to determine whether to re-emit updates to a validation result sink.
-	ValidationResultHash = "validator/validation-result-hash"
-
-	statusUpdateRetries = 3
-)
+// ValidationResultHash is used to determine whether to re-emit updates to a validation result sink.
+const ValidationResultHash = "validator/validation-result-hash"
 
 var (
 	vr        *v1alpha1.ValidationResult
@@ -84,7 +80,7 @@ func (r *ValidationResultReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		// Always update the ValidationResult's Status with a retry due to race condition with
 		// SafeUpdateValidationResult, which also updates the VR's Status and is continuously
 		// being called by the validator plugins.
-		for i := 0; i < statusUpdateRetries; i++ {
+		for i := 0; i < constants.StatusUpdateRetries; i++ {
 			if err := r.updateStatus(ctx); err == nil {
 				break
 			}

diff --git a/internal/test/controller_runtime_client.go b/internal/test/controller_runtime_client.go
@@ -33,12 +33,23 @@ func (m SubResourceMock) Patch(ctx context.Context, obj client.Object, patch cli
 
 type ClientMock struct {
 	CreateErrors []error
+	GetErrors    []error
 	UpdateErrors []error
 	SubResourceMock
 }
 
 func (m ClientMock) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	return nil
+	var err error
+	var errs []error
+	if m.GetErrors != nil {
+		err, errs = m.GetErrors[0], m.GetErrors[1:]
+	}
+	m = ClientMock{
+		CreateErrors: m.CreateErrors,
+		GetErrors:    errs,
+		UpdateErrors: m.UpdateErrors,
+	}
+	return err
 }
 func (m ClientMock) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
 	return nil
@@ -51,6 +62,7 @@ func (m ClientMock) Create(ctx context.Context, obj client.Object, opts ...clien
 	}
 	m = ClientMock{
 		CreateErrors: errs,
+		GetErrors:    m.GetErrors,
 		UpdateErrors: m.UpdateErrors,
 	}
 	return err
@@ -66,6 +78,7 @@ func (m ClientMock) Update(ctx context.Context, obj client.Object, opts ...clien
 	}
 	m = ClientMock{
 		CreateErrors: m.CreateErrors,
+		GetErrors:    m.GetErrors,
 		UpdateErrors: errs,
 	}
 	return err

diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go
@@ -3,4 +3,5 @@ package constants
 const (
 	ValidationRulePrefix string = "validation"
 	ValidatorConfig      string = "validator-config"
+	StatusUpdateRetries  int    = 3
 )
diff --git a/pkg/validationresult/validation_result.go b/pkg/validationresult/validation_result.go
@@ -52,40 +52,54 @@ func HandleNewValidationResult(c client.Client, vr *v1alpha1.ValidationResult, l
 		return err
 	}
 
+	var err error
+	nn := ktypes.NamespacedName{Name: vr.Name, Namespace: vr.Namespace}
+
 	// Update the ValidationResult's status
-	vr.Status = v1alpha1.ValidationResultStatus{
-		State: v1alpha1.ValidationInProgress,
-	}
-	if err := c.Status().Update(context.Background(), vr); err != nil {
-		l.V(0).Error(err, "failed to update ValidationResult status", "name", vr.Name, "namespace", vr.Namespace)
-		return err
+	for i := 0; i < constants.StatusUpdateRetries; i++ {
+		if err := c.Get(context.Background(), nn, vr); err != nil {
+			l.V(0).Error(err, "failed to get ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
+			return err
+		}
+		vr.Status = v1alpha1.ValidationResultStatus{State: v1alpha1.ValidationInProgress}
+		err = c.Status().Update(context.Background(), vr)
+		if err == nil {
+			return nil
+		}
+		l.V(1).Info("warning: failed to update ValidationResult status", "name", vr.Name, "namespace", vr.Namespace, "error", err.Error())
 	}
-
-	return nil
+	return err
 }
 
 // SafeUpdateValidationResult updates the overall validation result, ensuring
 // that the overall validation status remains failed if a single rule fails
 func SafeUpdateValidationResult(c client.Client, nn ktypes.NamespacedName, res *types.ValidationResult, resErr error, l logr.Logger) {
-
+	var err error
 	vr := &v1alpha1.ValidationResult{}
-	if err := c.Get(context.Background(), nn, vr); err != nil {
-		l.V(0).Error(err, "failed to get ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
-		return
-	}
 
-	updateValidationResult(vr, res, resErr)
+	for i := 0; i < constants.StatusUpdateRetries; i++ {
+		if err := c.Get(context.Background(), nn, vr); err != nil {
+			l.V(0).Error(err, "failed to get ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
+			return
+		}
+
+		updateValidationResult(vr, res, resErr)
+
+		err = c.Status().Update(context.Background(), vr)
+		if err != nil {
+			l.V(1).Info("warning: failed to update ValidationResult", "name", nn.Name, "namespace", nn.Namespace, "error", err.Error())
+			continue
+		}
 
-	if err := c.Status().Update(context.Background(), vr); err != nil {
-		l.V(0).Error(err, "failed to update ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
+		l.V(0).Info(
+			"Updated ValidationResult", "state", res.State, "reason", res.Condition.ValidationRule,
+			"message", res.Condition.Message, "details", res.Condition.Details,
+			"failures", res.Condition.Failures, "time", res.Condition.LastValidationTime,
+		)
 		return
 	}
 
-	l.V(0).Info(
-		"Updated ValidationResult", "state", res.State, "reason", res.Condition.ValidationRule,
-		"message", res.Condition.Message, "details", res.Condition.Details,
-		"failures", res.Condition.Failures, "time", res.Condition.LastValidationTime,
-	)
+	l.V(0).Error(err, "failed to update ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
 }
 
 // updateValidationResult updates the ValidationResult for the active validation rule

diff --git a/pkg/validationresult/validation_result_test.go b/pkg/validationresult/validation_result_test.go
@@ -101,15 +101,24 @@ func TestHandleNewValidationResult(t *testing.T) {
 			res:      vr([]corev1.ConditionStatus{corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
 			expected: errors.New("creation failed"),
 		},
+		{
+			name: "Fail (get)",
+			client: test.ClientMock{
+				GetErrors:       []error{errors.New("get failed")},
+				SubResourceMock: test.SubResourceMock{},
+			},
+			res:      vr([]corev1.ConditionStatus{corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
+			expected: errors.New("get failed"),
+		},
 		{
 			name: "Fail (status update)",
 			client: test.ClientMock{
 				SubResourceMock: test.SubResourceMock{
-					UpdateErrors: []error{errors.New("update failed")},
+					UpdateErrors: []error{errors.New("status update failed")},
 				},
 			},
 			res:      vr(nil, v1alpha1.ValidationSucceeded, nil),
-			expected: errors.New("update failed"),
+			expected: errors.New("status update failed"),
 		},
 	}
 	for _, c := range cs {
@@ -121,6 +130,48 @@ func TestHandleNewValidationResult(t *testing.T) {
 	}
 }
 
+func TestSafeUpdateValidationResult(t *testing.T) {
+	cs := []struct {
+		name   string
+		client test.ClientMock
+		nn     ktypes.NamespacedName
+		res    *types.ValidationResult
+		resErr error
+	}{
+		{
+			name:   "Pass",
+			client: test.ClientMock{},
+			nn:     ktypes.NamespacedName{Name: "", Namespace: ""},
+			res:    res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr: nil,
+		},
+		{
+			name: "Fail (get)",
+			client: test.ClientMock{
+				GetErrors: []error{errors.New("get failed")},
+			},
+			nn:     ktypes.NamespacedName{Name: "", Namespace: ""},
+			res:    res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr: errors.New("get failed"),
+		},
+		{
+			name: "Fail (update)",
+			client: test.ClientMock{
+				SubResourceMock: test.SubResourceMock{
+					UpdateErrors: []error{errors.New("status update failed")},
+				},
+			},
+			nn:     ktypes.NamespacedName{Name: "", Namespace: ""},
+			res:    res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr: errors.New("status update failed"),
+		},
+	}
+	for _, c := range cs {
+		t.Log(c.name)
+		SafeUpdateValidationResult(c.client, c.nn, c.res, c.resErr, logr.Logger{})
+	}
+}
+
 func TestUpdateValidationResult(t *testing.T) {
 	cs := []struct {
 		name       string