From 2955cadbf951e3197a9e173a3e06e31bb96a69b1 Mon Sep 17 00:00:00 2001
From: Anshul Pundir <anshul.pundir@docker.com>
Date: Mon, 23 Jul 2018 16:19:37 -0700
Subject: [PATCH] [orchestrator] Fix task sorting.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
---
 manager/orchestrator/task.go      | 20 ++++++++-------
 manager/orchestrator/task_test.go | 41 +++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/manager/orchestrator/task.go b/manager/orchestrator/task.go
index 8173dbd15f..f9a3feadf5 100644
--- a/manager/orchestrator/task.go
+++ b/manager/orchestrator/task.go
@@ -9,6 +9,7 @@ import (
 	"github.com/docker/swarmkit/identity"
 	"github.com/docker/swarmkit/manager/constraint"
 	"github.com/docker/swarmkit/protobuf/ptypes"
+	google_protobuf "github.com/gogo/protobuf/types"
 )
 
 // NewTask creates a new task.
@@ -143,6 +144,14 @@ func InvalidNode(n *api.Node) bool {
 		n.Spec.Availability == api.NodeAvailabilityDrain
 }
 
+func taskTimestamp(t *api.Task) *google_protobuf.Timestamp {
+	if t.Status.AppliedAt != nil {
+		return t.Status.AppliedAt
+	}
+
+	return t.Status.Timestamp
+}
+
 // TasksByTimestamp sorts tasks by applied timestamp if available, otherwise
 // status timestamp.
 type TasksByTimestamp []*api.Task
@@ -159,15 +168,8 @@ func (t TasksByTimestamp) Swap(i, j int) {
 
 // Less implements the Less method for sorting.
 func (t TasksByTimestamp) Less(i, j int) bool {
-	iTimestamp := t[i].Status.Timestamp
-	if t[i].Status.AppliedAt != nil {
-		iTimestamp = t[i].Status.AppliedAt
-	}
-
-	jTimestamp := t[j].Status.Timestamp
-	if t[j].Status.AppliedAt != nil {
-		iTimestamp = t[j].Status.AppliedAt
-	}
+	iTimestamp := taskTimestamp(t[i])
+	jTimestamp := taskTimestamp(t[j])
 
 	if iTimestamp == nil {
 		return true
diff --git a/manager/orchestrator/task_test.go b/manager/orchestrator/task_test.go
index d7d2fbbbde..6ac210b1c2 100644
--- a/manager/orchestrator/task_test.go
+++ b/manager/orchestrator/task_test.go
@@ -1,7 +1,10 @@
 package orchestrator
 
 import (
+	google_protobuf "github.com/gogo/protobuf/types"
 	"github.com/stretchr/testify/assert"
+	"sort"
+	"strconv"
 	"testing"
 
 	"github.com/docker/swarmkit/api"
@@ -105,3 +108,41 @@ func TestIsTaskDirtyPlacementConstraintsOnly(t *testing.T) {
 	service.Spec.Task.Placement.Constraints = nil
 	assert.False(t, IsTaskDirtyPlacementConstraintsOnly(service.Spec.Task, task))
 }
+
+// Test Task sorting, which is currently based on
+// Status.AppliedAt, and then on Status.Timestamp.
+func TestTaskSort(t *testing.T) {
+	var tasks []*api.Task
+	size := 5
+	seconds := int64(size)
+	for i := 0; i < size; i++ {
+		task := &api.Task{
+			ID: "id_" + strconv.Itoa(i),
+			Status: api.TaskStatus{
+				Timestamp: &google_protobuf.Timestamp{Seconds: seconds},
+			},
+		}
+
+		seconds--
+		tasks = append(tasks, task)
+	}
+
+	sort.Sort(TasksByTimestamp(tasks))
+	for i, task := range tasks {
+		expected := &google_protobuf.Timestamp{Seconds: int64(i + 1)}
+		assert.Equal(t, expected, task.Status.Timestamp)
+		assert.Equal(t, "id_"+strconv.Itoa(size-(i+1)), task.ID)
+	}
+
+	for i, task := range tasks {
+		task.Status.AppliedAt = &google_protobuf.Timestamp{Seconds: int64(size - i)}
+	}
+
+	sort.Sort(TasksByTimestamp(tasks))
+	sort.Sort(TasksByTimestamp(tasks))
+	for i, task := range tasks {
+		expected := &google_protobuf.Timestamp{Seconds: int64(i + 1)}
+		assert.Equal(t, expected, task.Status.AppliedAt)
+		assert.Equal(t, "id_"+strconv.Itoa(i), task.ID)
+	}
+}