app: Don't attempt to transfer leadership before promoting the other …

…node

This handles the case of a two-node cluster with one voter and one spare, and
the voter being shutdown gracefully.

app/app.go

@@ -258,12 +258,36 @@ func (a *App) Handover(ctx context.Context) error {
 	}
 	defer cli.Close()
 
+	// Possibly transfer our role.
+	nodes, err := cli.Cluster(ctx)
+	if err != nil {
+		return fmt.Errorf("cluster servers: %w", err)
+	}
+
+	changes := a.makeRolesChanges(nodes)
+
+	role, candidates := changes.Handover(a.id)
+
+	if role != -1 {
+		for i, node := range candidates {
+			if err := cli.Assign(ctx, node.ID, role); err != nil {
+				a.warn("promote %s from %s to %s: %v", node.Address, node.Role, role, err)
+				if i == len(candidates)-1 {
+					// We could not promote any node
+					return fmt.Errorf("could not promote any online node to %s", role)
+				}
+				continue
+			}
+			a.debug("promoted %s from %s to %s", node.Address, node.Role, role)
+			break
+		}
+	}
+
 	// Check if we are the current leader and transfer leadership if so.
 	leader, err := cli.Leader(ctx)
 	if err != nil {
 		return fmt.Errorf("leader address: %w", err)
 	}
-
 	if leader != nil && leader.Address == a.address {
 		if err := cli.Transfer(ctx, 0); err != nil {
 			return fmt.Errorf("transfer leadership: %w", err)
@@ -275,35 +299,11 @@ func (a *App) Handover(ctx context.Context) error {
 		defer cli.Close()
 	}
 
-	// Possibly transfer our role.
-	nodes, err := cli.Cluster(ctx)
-	if err != nil {
-		return fmt.Errorf("cluster servers: %w", err)
-	}
-
-	changes := a.makeRolesChanges(nodes)
-
-	role, candidates := changes.Handover(a.id)
-	if role == -1 {
-		return nil
-	}
-
-	for i, node := range candidates {
-		if err := cli.Assign(ctx, node.ID, role); err != nil {
-			a.warn("promote %s from %s to %s: %v", node.Address, node.Role, role, err)
-			if i == len(candidates)-1 {
-				// We could not promote any node
-				return fmt.Errorf("could not promote any online node to %s", role)
-			}
-			continue
+	// Demote ourselves if we have promoted someone else.
+	if role != -1 {
+		if err := cli.Assign(ctx, a.ID(), client.Spare); err != nil {
+			return fmt.Errorf("demote ourselves: %w", err)
 		}
-		a.debug("promoted %s from %s to %s", node.Address, node.Role, role)
-		break
-	}
-
-	// Demote ourselves.
-	if err := cli.Assign(ctx, a.ID(), client.Spare); err != nil {
-		return fmt.Errorf("demote ourselves: %w", err)
 	}
 
 	return nil

app/app_test.go

@@ -291,6 +291,41 @@ func TestHandover_Voter(t *testing.T) {
 	assert.Equal(t, client.Voter, cluster[3].Role)
 }
 
+// In a two-node cluster only one of them is a voter. When Handover() is called
+// on the voter, the role and leadership are transfered.
+func TestHandover_TwoNodes(t *testing.T) {
+	n := 2
+	apps := make([]*app.App, n)
+
+	for i := 0; i < n; i++ {
+		addr := fmt.Sprintf("127.0.0.1:900%d", i+1)
+		options := []app.Option{app.WithAddress(addr)}
+		if i > 0 {
+			options = append(options, app.WithCluster([]string{"127.0.0.1:9001"}))
+		}
+
+		app, cleanup := newApp(t, options...)
+		defer cleanup()
+
+		require.NoError(t, app.Ready(context.Background()))
+
+		apps[i] = app
+	}
+
+	err := apps[0].Handover(context.Background())
+	require.NoError(t, err)
+
+	cli, err := apps[1].Leader(context.Background())
+	require.NoError(t, err)
+	defer cli.Close()
+
+	cluster, err := cli.Cluster(context.Background())
+	require.NoError(t, err)
+
+	assert.Equal(t, client.Spare, cluster[0].Role)
+	assert.Equal(t, client.Voter, cluster[1].Role)
+}
+
 // Transfer voting rights to another online node. Failure domains are taken
 // into account.
 func TestHandover_VoterHonorFailureDomain(t *testing.T) {