fix: retry failed updates (#48)

spacelift-io · Jun 20, 2024 · accca15 · accca15
1 parent 4e8f184
commit accca15
Show file tree

Hide file tree

Showing 9 changed files with 119 additions and 43 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() {