From 74fa98ce647deb7981be027caa2bc35100ea29e2 Mon Sep 17 00:00:00 2001
From: Armon Dadgar <armon.dadgar@gmail.com>
Date: Thu, 13 Aug 2015 18:28:09 -0700
Subject: [PATCH] scheduler: make diff less nasty

---
 scheduler/service_sched.go | 19 +++++++++----------
 scheduler/util.go          | 29 +++++++++++++++++++----------
 scheduler/util_test.go     |  7 ++++++-
 3 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/scheduler/service_sched.go b/scheduler/service_sched.go
index 527ae1c10..6fd78cd99 100644
--- a/scheduler/service_sched.go
+++ b/scheduler/service_sched.go
@@ -128,36 +128,35 @@ func (s *ServiceScheduler) computeJobAllocs(job *structs.Job) error {
 	}
 
 	// Diff the required and existing allocations
-	place, update, migrate, evict, ignore := diffAllocs(job, tainted, groups, allocs)
-	s.logger.Printf("[DEBUG] sched: %#v: need %d placements, %d updates, %d migrations, %d evictions, %d ignored allocs",
-		s.eval, len(place), len(update), len(migrate), len(evict), len(ignore))
+	diff := diffAllocs(job, tainted, groups, allocs)
+	s.logger.Printf("[DEBUG] sched: %#v: %#v", s.eval, diff)
 
 	// Add all the evicts
-	for _, e := range evict {
+	for _, e := range diff.evict {
 		s.plan.AppendEvict(e.Alloc)
 	}
 
 	// For simplicity, we treat all migrates as an evict + place.
 	// XXX: This could probably be done more intelligently?
-	for _, e := range migrate {
+	for _, e := range diff.migrate {
 		s.plan.AppendEvict(e.Alloc)
 	}
-	place = append(place, migrate...)
+	diff.place = append(diff.place, diff.migrate...)
 
 	// For simplicity, we treat all updates as an evict + place.
 	// XXX: This should be done with rolling in-place updates instead.
-	for _, e := range update {
+	for _, e := range diff.update {
 		s.plan.AppendEvict(e.Alloc)
 	}
-	place = append(place, update...)
+	diff.place = append(diff.place, diff.update...)
 
 	// Nothing remaining to do if placement is not required
-	if len(place) == 0 {
+	if len(diff.place) == 0 {
 		return nil
 	}
 
 	// Compute the placements
-	return s.computePlacements(job, place)
+	return s.computePlacements(job, diff.place)
 }
 
 func (s *ServiceScheduler) computePlacements(job *structs.Job, place []allocTuple) error {
diff --git a/scheduler/util.go b/scheduler/util.go
index 2f9afc639..689b62564 100644
--- a/scheduler/util.go
+++ b/scheduler/util.go
@@ -26,16 +26,25 @@ func materializeTaskGroups(job *structs.Job) map[string]*structs.TaskGroup {
 	return out
 }
 
+// diffResult is used to return the sets that result from the diff
+type diffResult struct {
+	place, update, migrate, evict, ignore []allocTuple
+}
+
+func (d *diffResult) GoString() string {
+	return fmt.Sprintf("allocs: (place %d) (update %d) (migrate %d) (evict %d) (ignore %d)",
+		len(d.place), len(d.update), len(d.migrate), len(d.evict), len(d.ignore))
+}
+
 // diffAllocs is used to do a set difference between the target allocations
 // and the existing allocations. This returns 5 sets of results, the list of
 // named task groups that need to be placed (no existing allocation), the
 // allocations that need to be updated (job definition is newer), allocs that
 // need to be migrated (node is draining), the allocs that need to be evicted
 // (no longer required), and those that should be ignored.
-func diffAllocs(job *structs.Job,
-	taintedNodes map[string]bool,
-	required map[string]*structs.TaskGroup,
-	allocs []*structs.Allocation) (place, update, migrate, evict, ignore []allocTuple) {
+func diffAllocs(job *structs.Job, taintedNodes map[string]bool,
+	required map[string]*structs.TaskGroup, allocs []*structs.Allocation) *diffResult {
+	result := &diffResult{}
 
 	// Scan the existing updates
 	existing := make(map[string]struct{})
@@ -49,7 +58,7 @@ func diffAllocs(job *structs.Job,
 
 		// If not required, we evict
 		if !ok {
-			evict = append(evict, allocTuple{
+			result.evict = append(result.evict, allocTuple{
 				Name:      name,
 				TaskGroup: tg,
 				Alloc:     exist,
@@ -59,7 +68,7 @@ func diffAllocs(job *structs.Job,
 
 		// If we are on a tainted node, we must migrate
 		if taintedNodes[exist.NodeID] {
-			migrate = append(migrate, allocTuple{
+			result.migrate = append(result.migrate, allocTuple{
 				Name:      name,
 				TaskGroup: tg,
 				Alloc:     exist,
@@ -72,7 +81,7 @@ func diffAllocs(job *structs.Job,
 		// if the job definition has changed in a way that affects
 		// this allocation and potentially ignore it.
 		if job.ModifyIndex != exist.Job.ModifyIndex {
-			update = append(update, allocTuple{
+			result.update = append(result.update, allocTuple{
 				Name:      name,
 				TaskGroup: tg,
 				Alloc:     exist,
@@ -81,7 +90,7 @@ func diffAllocs(job *structs.Job,
 		}
 
 		// Everything is up-to-date
-		ignore = append(ignore, allocTuple{
+		result.ignore = append(result.ignore, allocTuple{
 			Name:      name,
 			TaskGroup: tg,
 			Alloc:     exist,
@@ -97,13 +106,13 @@ func diffAllocs(job *structs.Job,
 		// is an existing allocation, we would have checked for a potential
 		// update or ignore above.
 		if !ok {
-			place = append(place, allocTuple{
+			result.place = append(result.place, allocTuple{
 				Name:      name,
 				TaskGroup: tg,
 			})
 		}
 	}
-	return
+	return result
 }
 
 // readyNodesInDCs returns all the ready nodes in the given datacenters
diff --git a/scheduler/util_test.go b/scheduler/util_test.go
index ce16dd579..176024396 100644
--- a/scheduler/util_test.go
+++ b/scheduler/util_test.go
@@ -75,7 +75,12 @@ func TestDiffAllocs(t *testing.T) {
 		},
 	}
 
-	place, update, migrate, evict, ignore := diffAllocs(job, tainted, required, allocs)
+	diff := diffAllocs(job, tainted, required, allocs)
+	place := diff.place
+	update := diff.update
+	migrate := diff.migrate
+	evict := diff.evict
+	ignore := diff.ignore
 
 	// We should update the first alloc
 	if len(update) != 1 || update[0].Alloc != allocs[0] {