diff --git a/internal/controller/validationresult_controller.go b/internal/controller/validationresult_controller.go index 77a9d85e..e087c381 100644 --- 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 index 7f9187a2..c0998f19 100644 --- 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 index 7fb276c1..7888d409 100644 --- 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 index c4419dfb..4b833ddd 100644 --- 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 index cc4e3fb2..b9aa93e6 100644 --- 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