From b5bab9a44186137bb46c5a1af152733ec6dd1641 Mon Sep 17 00:00:00 2001
From: Matt Toohey <mtoohey@block.xyz>
Date: Thu, 18 Jul 2024 11:22:58 +1000
Subject: [PATCH] =?UTF-8?q?fix:=20wrap=20errors=20in=20ReplaceDeployment,?=
 =?UTF-8?q?=20don=E2=80=99t=20hide=20errconflicts=20(#2108)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

also logs now include old and new deployment keys
---
 backend/controller/controller.go |  2 +-
 backend/controller/dal/dal.go    | 22 +++++++++++++---------
 buildengine/deploy.go            |  1 +
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/backend/controller/controller.go b/backend/controller/controller.go
index 4208993e8a..78c756af63 100644
--- a/backend/controller/controller.go
+++ b/backend/controller/controller.go
@@ -526,7 +526,7 @@ func (s *Service) ReplaceDeploy(ctx context.Context, c *connect.Request[ftlv1.Re
 		if errors.Is(err, dalerrs.ErrNotFound) {
 			logger.Errorf(err, "Deployment not found: %s", newDeploymentKey)
 			return nil, connect.NewError(connect.CodeNotFound, errors.New("deployment not found"))
-		} else if errors.Is(err, dalerrs.ErrConflict) {
+		} else if errors.Is(err, dal.ErrReplaceDeploymentAlreadyActive) {
 			logger.Infof("Reusing deployment: %s", newDeploymentKey)
 		} else {
 			logger.Errorf(err, "Could not replace deployment: %s", newDeploymentKey)
diff --git a/backend/controller/dal/dal.go b/backend/controller/dal/dal.go
index daddfa85f6..a0fd3b0c3c 100644
--- a/backend/controller/dal/dal.go
+++ b/backend/controller/dal/dal.go
@@ -724,24 +724,28 @@ func (d *DAL) SetDeploymentReplicas(ctx context.Context, key model.DeploymentKey
 	return nil
 }
 
+var ErrReplaceDeploymentAlreadyActive = errors.New("deployment already active")
+
 // ReplaceDeployment replaces an old deployment of a module with a new deployment.
+//
+// returns ErrReplaceDeploymentAlreadyActive if the new deployment is already active.
 func (d *DAL) ReplaceDeployment(ctx context.Context, newDeploymentKey model.DeploymentKey, minReplicas int) (err error) {
 	// Start the transaction
 	tx, err := d.db.Begin(ctx)
 	if err != nil {
-		return dalerrs.TranslatePGError(err)
+		return fmt.Errorf("replace deployment failed to begin transaction for %v: %w", newDeploymentKey, dalerrs.TranslatePGError(err))
 	}
 
 	defer tx.CommitOrRollback(ctx, &err)
 	newDeployment, err := tx.GetDeployment(ctx, newDeploymentKey)
 	if err != nil {
-		return dalerrs.TranslatePGError(err)
+		return fmt.Errorf("replace deployment failed to get deployment for %v: %w", newDeploymentKey, dalerrs.TranslatePGError(err))
 	}
 
 	// must be called before deploymentWillDeactivate for the old deployment
 	err = d.deploymentWillActivate(ctx, tx, newDeploymentKey)
 	if err != nil {
-		return dalerrs.TranslatePGError(err)
+		return fmt.Errorf("replace deployment failed willActivate trigger for %v: %w", newDeploymentKey, dalerrs.TranslatePGError(err))
 	}
 
 	// If there's an existing deployment, set its desired replicas to 0
@@ -750,23 +754,23 @@ func (d *DAL) ReplaceDeployment(ctx context.Context, newDeploymentKey model.Depl
 	if err == nil {
 		count, err := tx.ReplaceDeployment(ctx, oldDeployment.Key, newDeploymentKey, int32(minReplicas))
 		if err != nil {
-			return dalerrs.TranslatePGError(err)
+			return fmt.Errorf("replace deployment failed to replace min replicas from %v to %v: %w", oldDeployment.Key, newDeploymentKey, dalerrs.TranslatePGError(err))
 		}
 		if count == 1 {
-			return fmt.Errorf("deployment already exists: %w", dalerrs.ErrConflict)
+			return fmt.Errorf("replace deployment failed: deployment already exists from %v to %v: %w", oldDeployment.Key, newDeploymentKey, ErrReplaceDeploymentAlreadyActive)
 		}
 		err = d.deploymentWillDeactivate(ctx, tx, oldDeployment.Key)
 		if err != nil {
-			return dalerrs.TranslatePGError(err)
+			return fmt.Errorf("replace deployment failed willDeactivate trigger from %v to %v: %w", oldDeployment.Key, newDeploymentKey, dalerrs.TranslatePGError(err))
 		}
 		replacedDeploymentKey = optional.Some(oldDeployment.Key)
 	} else if !dalerrs.IsNotFound(err) {
-		return dalerrs.TranslatePGError(err)
+		return fmt.Errorf("replace deployment failed to get existing deployment for %v: %w", newDeploymentKey, dalerrs.TranslatePGError(err))
 	} else {
 		// Set the desired replicas for the new deployment
 		err = tx.SetDeploymentDesiredReplicas(ctx, newDeploymentKey, int32(minReplicas))
 		if err != nil {
-			return dalerrs.TranslatePGError(err)
+			return fmt.Errorf("replace deployment failed to set replicas for %v: %w", newDeploymentKey, dalerrs.TranslatePGError(err))
 		}
 	}
 
@@ -778,7 +782,7 @@ func (d *DAL) ReplaceDeployment(ctx context.Context, newDeploymentKey model.Depl
 		Replaced:      replacedDeploymentKey,
 	})
 	if err != nil {
-		return dalerrs.TranslatePGError(err)
+		return fmt.Errorf("replace deployment failed to create event: %w", dalerrs.TranslatePGError(err))
 	}
 
 	return nil
diff --git a/buildengine/deploy.go b/buildengine/deploy.go
index 329a8b086b..767918cb46 100644
--- a/buildengine/deploy.go
+++ b/buildengine/deploy.go
@@ -100,6 +100,7 @@ func Deploy(ctx context.Context, module Module, replicas int32, waitForDeployOnl
 		if err != nil {
 			return err
 		}
+		logger.Debugf("Deployment %s became ready", resp.Msg.DeploymentKey)
 	}
 
 	return nil