Skip to content

Commit

Permalink
fix: wrap errors in ReplaceDeployment, don’t hide errconflicts (#2108)
Browse files Browse the repository at this point in the history
also logs now include old and new deployment keys
  • Loading branch information
matt2e authored Jul 18, 2024
1 parent 60acb0b commit b5bab9a
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 10 deletions.
2 changes: 1 addition & 1 deletion backend/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
22 changes: 13 additions & 9 deletions backend/controller/dal/dal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
}
}

Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions buildengine/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b5bab9a

Please sign in to comment.