From accca15aab88d897ee41c9d643a48f62a546a213 Mon Sep 17 00:00:00 2001 From: Elie CHARRA Date: Thu, 20 Jun 2024 14:19:43 +0200 Subject: [PATCH] fix: retry failed updates (#48) --- internal/controller/context_controller.go | 3 +- .../controller/context_controller_test.go | 35 ++++++++++++++++ internal/controller/policy_controller.go | 2 +- internal/controller/policy_controller_test.go | 34 +++++++++++++++ internal/controller/run_controller.go | 1 - internal/controller/space_controller.go | 2 +- internal/controller/space_controller_test.go | 41 +++++++++++-------- internal/controller/stack_controller.go | 3 +- internal/controller/stack_controller_test.go | 41 +++++++++++-------- 9 files changed, 119 insertions(+), 43 deletions(-) diff --git a/internal/controller/context_controller.go b/internal/controller/context_controller.go index d8b27f0..91f1786 100644 --- a/internal/controller/context_controller.go +++ b/internal/controller/context_controller.go @@ -229,8 +229,7 @@ func (r *ContextReconciler) handleUpdateContext(ctx context.Context, context *v1 spaceliftUpdatedContext, err := r.SpaceliftContextRepository.Update(ctx, context) if err != nil { logger.Error(err, "Unable to update the context in spacelift") - // TODO: Implement better error handling and retry errors that could be retried - return ctrl.Result{}, nil + return ctrl.Result{}, err } context.SetContext(spaceliftUpdatedContext) diff --git a/internal/controller/context_controller_test.go b/internal/controller/context_controller_test.go index 80c46da..5b7f256 100644 --- a/internal/controller/context_controller_test.go +++ b/internal/controller/context_controller_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/pkg/errors" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" "go.uber.org/zap/zaptest/observer" @@ -551,6 +552,40 @@ func (s *ContextControllerTestSuite) TestContextCreation_OK() { s.Assert().Equal("test-context-id", context.Status.Id) } +func (s *ContextControllerTestSuite) TestContextUpdate_UnableToCreateOnSpacelift() { + + s.FakeSpaceliftContextRepo.EXPECT().Get(mock.Anything, mock.Anything).Times(2). + Return(nil, nil) + failedUpdate := s.FakeSpaceliftContextRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). + Return(nil, errors.New("error on first update")) + s.FakeSpaceliftContextRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). + Return(&models.Context{Id: "test-context-id"}, nil).NotBefore(failedUpdate) + + s.Logs.TakeAll() + context, err := s.CreateTestContext() + s.Require().NoError(err) + defer s.DeleteContext(context) + + var logs *observer.ObservedLogs + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Unable to update the context in spacelift") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) + + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Context updated") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) + s.Assert().Equal("test-context-id", logs.All()[0].ContextMap()[logging.ContextId]) + + context, err = s.ContextRepo.Get(s.Context(), types.NamespacedName{ + Namespace: context.Namespace, + Name: context.ObjectMeta.Name, + }) + s.Require().NoError(err) + s.Assert().Equal("test-context-id", context.Status.Id) +} + func (s *ContextControllerTestSuite) TestContextUpdate_OK() { s.FakeSpaceliftContextRepo.EXPECT().Get(mock.Anything, mock.Anything).Once(). diff --git a/internal/controller/policy_controller.go b/internal/controller/policy_controller.go index abb4d20..8ed81b3 100644 --- a/internal/controller/policy_controller.go +++ b/internal/controller/policy_controller.go @@ -158,7 +158,7 @@ func (r *PolicyReconciler) handleUpdatePolicy(ctx context.Context, policy *v1bet spaceliftUpdatedPolicy, err := r.SpaceliftPolicyRepository.Update(ctx, policy) if err != nil { logger.Error(err, "Unable to update the policy in spacelift") - return ctrl.Result{}, nil + return ctrl.Result{}, err } res, err := r.updatePolicyStatus(ctx, policy, *spaceliftUpdatedPolicy) diff --git a/internal/controller/policy_controller_test.go b/internal/controller/policy_controller_test.go index 6afd88b..df7b223 100644 --- a/internal/controller/policy_controller_test.go +++ b/internal/controller/policy_controller_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/pkg/errors" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" "go.uber.org/zap/zaptest/observer" @@ -269,6 +270,39 @@ func (s *PolicyControllerSuite) TestPolicyCreation_OK() { s.Assert().Equal("test-policy-id", policy.Status.Id) } +func (s *PolicyControllerSuite) TestPolicyUpdate_UnableToCreateOnSpacelift() { + + s.FakeSpaceliftPolicyRepo.EXPECT().Get(mock.Anything, mock.Anything).Times(2). + Return(nil, nil) + failedUpdate := s.FakeSpaceliftPolicyRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). + Return(nil, errors.New("error on first update")) + s.FakeSpaceliftPolicyRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). + Return(&models.Policy{Id: "test-policy-id"}, nil).NotBefore(failedUpdate) + + policy, err := s.CreateTestPolicy() + s.Require().NoError(err) + defer s.DeletePolicy(policy) + + var logs *observer.ObservedLogs + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Unable to update the policy in spacelift") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) + + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Policy updated") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) + s.Assert().Equal("test-policy-id", logs.All()[0].ContextMap()[logging.PolicyId]) + + policy, err = s.PolicyRepo.Get(s.Context(), types.NamespacedName{ + Namespace: policy.Namespace, + Name: policy.ObjectMeta.Name, + }) + s.Require().NoError(err) + s.Assert().Equal("test-policy-id", policy.Status.Id) +} + func (s *PolicyControllerSuite) TestPolicyUpdate_OK() { s.FakeSpaceliftPolicyRepo.EXPECT().Get(mock.Anything, mock.Anything).Once(). diff --git a/internal/controller/run_controller.go b/internal/controller/run_controller.go index c42bdaf..eedac12 100644 --- a/internal/controller/run_controller.go +++ b/internal/controller/run_controller.go @@ -111,7 +111,6 @@ func (r *RunReconciler) handleNewRun(ctx context.Context, run *v1beta1.Run, stac spaceliftRun, err := r.SpaceliftRunRepository.Create(ctx, stack) if err != nil { logger.Error(err, "Unable to create the run in spacelift") - // TODO: Implement better error handling and retry errors that could be retried return ctrl.Result{}, nil } diff --git a/internal/controller/space_controller.go b/internal/controller/space_controller.go index 0380573..c8ea46b 100644 --- a/internal/controller/space_controller.go +++ b/internal/controller/space_controller.go @@ -111,7 +111,7 @@ func (r *SpaceReconciler) handleUpdateSpace(ctx context.Context, space *v1beta1. spaceliftUpdatedSpace, err := r.SpaceliftSpaceRepository.Update(ctx, space) if err != nil { logger.Error(err, "Unable to update the space in spacelift") - return ctrl.Result{}, nil + return ctrl.Result{}, err } res, err := r.updateSpaceStatus(ctx, space, *spaceliftUpdatedSpace) diff --git a/internal/controller/space_controller_test.go b/internal/controller/space_controller_test.go index 5bcf5f2..bd7e76c 100644 --- a/internal/controller/space_controller_test.go +++ b/internal/controller/space_controller_test.go @@ -150,33 +150,38 @@ func (s *SpaceControllerSuite) TestSpaceCreation_Success() { } func (s *SpaceControllerSuite) TestSpaceUpdate_UnableToUpdateOnSpacelift() { - s.FakeSpaceliftSpaceRepo.EXPECT().Get(mock.Anything, mock.Anything).Once(). + s.FakeSpaceliftSpaceRepo.EXPECT().Get(mock.Anything, mock.Anything).Times(2). + Return(nil, nil) + failedUpdate := s.FakeSpaceliftSpaceRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). + Return(nil, fmt.Errorf("unable to update resource on spacelift")) + s.FakeSpaceliftSpaceRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). Return(&models.Space{ ID: "test-space-generated-id", - }, nil) - s.FakeSpaceliftSpaceRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). - Return(nil, fmt.Errorf("unable to update resource on spacelift")) + }, nil).NotBefore(failedUpdate) s.Logs.TakeAll() space, err := s.CreateTestSpace() s.Require().NoError(err) defer s.DeleteSpace(space) - // Make sure we don't update the space ID - s.Require().Never(func() bool { - space, err := s.SpaceRepo.Get(s.Context(), types.NamespacedName{ - Namespace: space.Namespace, - Name: space.ObjectMeta.Name, - }) - s.Require().NoError(err) - return space.Status.Id != "" - }, 3*time.Second, integration.DefaultInterval) + var logs *observer.ObservedLogs + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Unable to update the space in spacelift") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) - // Check that the error has been logged - logs := s.Logs.FilterMessage("Unable to update the space in spacelift") - s.Require().Equal(1, logs.Len()) - logs = s.Logs.FilterMessage("Space updated") - s.Require().Equal(0, logs.Len()) + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Space updated") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) + s.Assert().Equal("test-space-generated-id", logs.All()[0].ContextMap()[logging.SpaceId]) + + space, err = s.SpaceRepo.Get(s.Context(), types.NamespacedName{ + Namespace: space.Namespace, + Name: space.ObjectMeta.Name, + }) + s.Require().NoError(err) + s.Assert().Equal("test-space-generated-id", space.Status.Id) } func (s *SpaceControllerSuite) TestSpaceUpdate_OK() { diff --git a/internal/controller/stack_controller.go b/internal/controller/stack_controller.go index 4bb6fb2..709edbc 100644 --- a/internal/controller/stack_controller.go +++ b/internal/controller/stack_controller.go @@ -157,8 +157,7 @@ func (r *StackReconciler) handleUpdateStack(ctx context.Context, stack *v1beta1. spaceliftUpdatedStack, err := r.SpaceliftStackRepository.Update(ctx, stack) if err != nil { logger.Error(err, "Unable to update the stack in spacelift") - // TODO: Implement better error handling and retry errors that could be retried - return ctrl.Result{}, nil + return ctrl.Result{}, err } res, err := r.updateStackStatus(ctx, stack, *spaceliftUpdatedStack) diff --git a/internal/controller/stack_controller_test.go b/internal/controller/stack_controller_test.go index d373daa..32eab1c 100644 --- a/internal/controller/stack_controller_test.go +++ b/internal/controller/stack_controller_test.go @@ -297,33 +297,38 @@ func (s *StackControllerSuite) TestStackCreationWithSpaceName_OK() { } func (s *StackControllerSuite) TestStackUpdate_UnableToUpdateOnSpacelift() { - s.FakeSpaceliftStackRepo.EXPECT().Get(mock.Anything, mock.Anything).Once(). + s.FakeSpaceliftStackRepo.EXPECT().Get(mock.Anything, mock.Anything).Times(2). + Return(nil, nil) + failedUpdate := s.FakeSpaceliftStackRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). + Return(nil, fmt.Errorf("unable to update resource on spacelift")) + s.FakeSpaceliftStackRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). Return(&models.Stack{ Id: "test-stack-generated-id", - }, nil) - s.FakeSpaceliftStackRepo.EXPECT().Update(mock.Anything, mock.Anything).Once(). - Return(nil, fmt.Errorf("unable to update resource on spacelift")) + }, nil).NotBefore(failedUpdate) s.Logs.TakeAll() stack, err := s.CreateTestStack() s.Require().NoError(err) defer s.DeleteStack(stack) - // Make sure we don't update the stack ID - s.Require().Never(func() bool { - stack, err := s.StackRepo.Get(s.Context(), types.NamespacedName{ - Namespace: stack.Namespace, - Name: stack.ObjectMeta.Name, - }) - s.Require().NoError(err) - return stack.Status.Id != "" - }, 3*time.Second, integration.DefaultInterval) + var logs *observer.ObservedLogs + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Unable to update the stack in spacelift") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) - // Check that the error has been logged - logs := s.Logs.FilterMessage("Unable to update the stack in spacelift") - s.Require().Equal(1, logs.Len()) - logs = s.Logs.FilterMessage("Stack updated") - s.Require().Equal(0, logs.Len()) + s.Require().Eventually(func() bool { + logs = s.Logs.FilterMessage("Stack updated") + return logs.Len() == 1 + }, integration.DefaultTimeout, integration.DefaultInterval) + s.Assert().Equal("test-stack-generated-id", logs.All()[0].ContextMap()[logging.StackId]) + + stack, err = s.StackRepo.Get(s.Context(), types.NamespacedName{ + Namespace: stack.Namespace, + Name: stack.ObjectMeta.Name, + }) + s.Require().NoError(err) + s.Assert().Equal("test-stack-generated-id", stack.Status.Id) } func (s *StackControllerSuite) TestStackUpdate_OK() {