From 492239d3ee7e338858496078bd6198b8d01f3d10 Mon Sep 17 00:00:00 2001
From: Alex Dadgar <alex.dadgar@gmail.com>
Date: Tue, 25 Jul 2017 11:27:47 -0700
Subject: [PATCH] Improve multiple group handling in a deployment

This PR resolves a bug in which a job with multiple task groups would
create new deployment objects each, thus clearing out all other task
groups deployment state.
---
 scheduler/reconcile.go      | 11 ++++++--
 scheduler/reconcile_test.go | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/scheduler/reconcile.go b/scheduler/reconcile.go
index 9e094d1fd..4c8dc1282 100644
--- a/scheduler/reconcile.go
+++ b/scheduler/reconcile.go
@@ -435,9 +435,14 @@ func (a *allocReconciler) computeGroup(group string, all allocSet) bool {
 	}
 
 	// Create a new deployment if necessary
-	if a.deployment == nil && strategy != nil && dstate.DesiredTotal != 0 {
-		a.deployment = structs.NewDeployment(a.job)
-		a.result.deployment = a.deployment
+	if !existingDeployment && strategy != nil && dstate.DesiredTotal != 0 {
+		// A previous group may have made the deployment already
+		if a.deployment == nil {
+			a.deployment = structs.NewDeployment(a.job)
+			a.result.deployment = a.deployment
+		}
+
+		// Attach the groups deployment state to the deployment
 		a.deployment.TaskGroups[group] = dstate
 	}
 
diff --git a/scheduler/reconcile_test.go b/scheduler/reconcile_test.go
index caa55720e..85b823fd7 100644
--- a/scheduler/reconcile_test.go
+++ b/scheduler/reconcile_test.go
@@ -1990,6 +1990,62 @@ func TestReconciler_NewCanaries(t *testing.T) {
 	assertNamesHaveIndexes(t, intRange(0, 1), placeResultsToNames(r.place))
 }
 
+// Tests the reconciler creates new canaries when the job changes for multiple
+// task groups
+func TestReconciler_NewCanaries_MultiTG(t *testing.T) {
+	job := mock.Job()
+	job.TaskGroups[0].Update = canaryUpdate
+	job.TaskGroups = append(job.TaskGroups, job.TaskGroups[0].Copy())
+	job.TaskGroups[0].Name = "tg2"
+
+	// Create 10 allocations from the old job for each tg
+	var allocs []*structs.Allocation
+	for j := 0; j < 2; j++ {
+		for i := 0; i < 10; i++ {
+			alloc := mock.Alloc()
+			alloc.Job = job
+			alloc.JobID = job.ID
+			alloc.NodeID = structs.GenerateUUID()
+			alloc.Name = structs.AllocName(job.ID, job.TaskGroups[j].Name, uint(i))
+			alloc.TaskGroup = job.TaskGroups[j].Name
+			allocs = append(allocs, alloc)
+		}
+	}
+
+	reconciler := NewAllocReconciler(testLogger(), allocUpdateFnDestructive, false, job.ID, job, nil, allocs, nil)
+	r := reconciler.Compute()
+
+	newD := structs.NewDeployment(job)
+	newD.StatusDescription = structs.DeploymentStatusDescriptionRunningNeedsPromotion
+	state := &structs.DeploymentState{
+		DesiredCanaries: 2,
+		DesiredTotal:    10,
+	}
+	newD.TaskGroups[job.TaskGroups[0].Name] = state
+	newD.TaskGroups[job.TaskGroups[1].Name] = state.Copy()
+
+	// Assert the correct results
+	assertResults(t, r, &resultExpectation{
+		createDeployment:  newD,
+		deploymentUpdates: nil,
+		place:             4,
+		inplace:           0,
+		stop:              0,
+		desiredTGUpdates: map[string]*structs.DesiredUpdates{
+			job.TaskGroups[0].Name: {
+				Canary: 2,
+				Ignore: 10,
+			},
+			job.TaskGroups[1].Name: {
+				Canary: 2,
+				Ignore: 10,
+			},
+		},
+	})
+
+	assertNamesHaveIndexes(t, intRange(0, 1, 0, 1), placeResultsToNames(r.place))
+}
+
 // Tests the reconciler creates new canaries when the job changes and scales up
 func TestReconciler_NewCanaries_ScaleUp(t *testing.T) {
 	// Scale the job up to 15