Sort by Priority for strategy Distributed

pkg/gameserverallocations/allocation_cache.go

@@ -262,6 +262,49 @@ func (c *AllocationCache) ListSortedGameServers(gsa *allocationv1.GameServerAllo
 	return list
 }
 
+// ListSortedGameServersPriorities sorts and returns a list of game servers based on the
+// list of Priorities.
+func (c *AllocationCache) ListSortedGameServersPriorities(gsa *allocationv1.GameServerAllocation) []*agonesv1.GameServer {
+	list := c.getGameServers()
+	if list == nil {
+		return []*agonesv1.GameServer{}
+	}
+
+	sort.Slice(list, func(i, j int) bool {
+		gs1 := list[i]
+		gs2 := list[j]
+
+		// TODO: Should we add FeatureStateAllocationFilter here too?
+
+		if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) && (gsa != nil) {
+			for _, priority := range gsa.Spec.Priorities {
+				res := compareGameServers(&priority, gs1, gs2)
+				switch priority.Order {
+				case agonesv1.GameServerPriorityAscending:
+					if res == -1 {
+						return true
+					}
+					if res == 1 {
+						return false
+					}
+				case agonesv1.GameServerPriorityDescending:
+					if res == -1 {
+						return false
+					}
+					if res == 1 {
+						return true
+					}
+				}
+			}
+		}
+
+		// finally sort lexicographically, so we have a stable order
+		return gs1.GetObjectMeta().GetName() < gs2.GetObjectMeta().GetName()
+	})
+
+	return list
+}
+
 // compareGameServers compares two game servers based on a CountsAndLists Priority using available
 // capacity (Capacity - Count for Counters, and Capacity - len(Values) for Lists) as the comparison.
 // Returns -1 if gs1 < gs2; 1 if gs1 > gs2; 0 if gs1 == gs2; 0 if neither gamer server has the Priority.

pkg/gameserverallocations/allocation_cache_test.go

@@ -221,7 +221,7 @@ func TestAllocationCacheListSortedGameServers(t *testing.T) {
 	}
 }
 
-func TestAllocationCacheCompareGameServers(t *testing.T) {
+func TestListSortedGameServersPriorities(t *testing.T) {
 	t.Parallel()
 	runtime.FeatureTestMutex.Lock()
 	defer runtime.FeatureTestMutex.Unlock()
@@ -472,6 +472,21 @@ func TestAllocationCacheCompareGameServers(t *testing.T) {
 			},
 			want: []*agonesv1.GameServer{&gs2, &gs4, &gs1, &gs3, &gs5, &gs6},
 		},
+		"Sort lexigraphically as no game server has the priority": {
+			list: []agonesv1.GameServer{gs6, gs5, gs4, gs3, gs2, gs1},
+			gsa: &allocationv1.GameServerAllocation{
+				Spec: allocationv1.GameServerAllocationSpec{
+					Priorities: []agonesv1.Priority{
+						{
+							Type:  "Counter",
+							Key:   "sayers",
+							Order: "Ascending",
+						},
+					},
+				},
+			},
+			want: []*agonesv1.GameServer{&gs1, &gs2, &gs3, &gs4, &gs5, &gs6},
+		},
 	}
 
 	for testName, testScenario := range testScenarios {
@@ -493,7 +508,7 @@ func TestAllocationCacheCompareGameServers(t *testing.T) {
 			err = cache.counter.Run(ctx, 0)
 			assert.Nil(t, err)
 
-			got := cache.ListSortedGameServers(testScenario.gsa)
+			got := cache.ListSortedGameServersPriorities(testScenario.gsa)
 
 			assert.Equal(t, testScenario.want, got)
 		})

pkg/gameserverallocations/allocator.go

@@ -25,6 +25,7 @@ import (
 
 	"agones.dev/agones/pkg/allocation/converters"
 	pb "agones.dev/agones/pkg/allocation/go"
+	"agones.dev/agones/pkg/apis"
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	allocationv1 "agones.dev/agones/pkg/apis/allocation/v1"
 	multiclusterv1 "agones.dev/agones/pkg/apis/multicluster/v1"
@@ -532,7 +533,12 @@ func (c *Allocator) ListenAndAllocate(ctx context.Context, updateWorkerCount int
 			requestCount++
 
 			if list == nil {
-				list = c.allocationCache.ListSortedGameServers(req.gsa)
+				if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) || req.gsa.Spec.Scheduling == apis.Packed {
+					list = c.allocationCache.ListSortedGameServers(req.gsa)
+				} else {
+					// If FeatureCountsAndLists and Scheduling == Distributed, sort game servers by Priorities
+					list = c.allocationCache.ListSortedGameServersPriorities(req.gsa)
+				}
 			}
 
 			gs, index, err := findGameServerForAllocation(req.gsa, list)

pkg/gameserverallocations/allocator_test.go

@@ -683,7 +683,7 @@ func TestAllocatorRunLocalAllocationsCountsAndLists(t *testing.T) {
 
 	READY := agonesv1.GameServerStateReady
 
-	gsa1 := &allocationv1.GameServerAllocation{
+	gsaAscending := &allocationv1.GameServerAllocation{
 		ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs},
 		Spec: allocationv1.GameServerAllocationSpec{
 			Scheduling: apis.Packed,
@@ -696,7 +696,7 @@ func TestAllocatorRunLocalAllocationsCountsAndLists(t *testing.T) {
 					Order: agonesv1.GameServerPriorityAscending},
 			},
 		}}
-	gsa2 := &allocationv1.GameServerAllocation{
+	gsaDescending := &allocationv1.GameServerAllocation{
 		ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs},
 		Spec: allocationv1.GameServerAllocationSpec{
 			Scheduling: apis.Packed,
@@ -709,28 +709,40 @@ func TestAllocatorRunLocalAllocationsCountsAndLists(t *testing.T) {
 					Order: agonesv1.GameServerPriorityDescending},
 			},
 		}}
+	gsaDistributed := &allocationv1.GameServerAllocation{
+		ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs},
+		Spec: allocationv1.GameServerAllocationSpec{
+			Scheduling: apis.Distributed,
+			Selectors: []allocationv1.GameServerSelector{{
+				GameServerState: &READY,
+			}},
+			Priorities: []agonesv1.Priority{
+				{Type: agonesv1.GameServerPriorityCounter,
+					Key:   "foo",
+					Order: agonesv1.GameServerPriorityDescending},
+			},
+		}}
 
-	// line up 3 in a batch (first sort by Ascending then Descending then Ascending)
-	j1 := request{gsa: gsa1.DeepCopy(), response: make(chan response)}
+	j1 := request{gsa: gsaDescending.DeepCopy(), response: make(chan response)}
 	a.pendingRequests <- j1
-	j2 := request{gsa: gsa2.DeepCopy(), response: make(chan response)}
+	j2 := request{gsa: gsaAscending.DeepCopy(), response: make(chan response)}
 	a.pendingRequests <- j2
-	j3 := request{gsa: gsa1.DeepCopy(), response: make(chan response)}
+	j3 := request{gsa: gsaDistributed.DeepCopy(), response: make(chan response)}
 	a.pendingRequests <- j3
 
-	go a.ListenAndAllocate(ctx, 3)
+	go a.ListenAndAllocate(ctx, 5)
 
 	res1 := <-j1.response
 	assert.NoError(t, res1.err)
 	assert.NotNil(t, res1.gs)
 	assert.Equal(t, agonesv1.GameServerStateAllocated, res1.gs.Status.State)
-	assert.Equal(t, gs3.ObjectMeta.Name, res1.gs.ObjectMeta.Name)
+	assert.Equal(t, gs1.ObjectMeta.Name, res1.gs.ObjectMeta.Name)
 
 	res2 := <-j2.response
 	assert.NoError(t, res2.err)
 	assert.NotNil(t, res2.gs)
 	assert.Equal(t, agonesv1.GameServerStateAllocated, res2.gs.Status.State)
-	assert.Equal(t, gs1.ObjectMeta.Name, res2.gs.ObjectMeta.Name)
+	assert.Equal(t, gs3.ObjectMeta.Name, res2.gs.ObjectMeta.Name)
 
 	res3 := <-j3.response
 	assert.NoError(t, res3.err)

pkg/gameserverallocations/find.go

@@ -20,6 +20,7 @@ import (
 	"agones.dev/agones/pkg/apis"
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	allocationv1 "agones.dev/agones/pkg/apis/allocation/v1"
+	"agones.dev/agones/pkg/util/runtime"
 	"github.com/pkg/errors"
 )
 
@@ -50,18 +51,29 @@ func findGameServerForAllocation(gsa *allocationv1.GameServerAllocation, list []
 	case apis.Distributed:
 		// randomised looping - make a list of indices, and then randomise them
 		// as we don't want to change the order of the gameserver slice
-		l := len(list)
-		indices := make([]int, l)
-		for i := 0; i < l; i++ {
-			indices[i] = i
-		}
-		rand.Shuffle(l, func(i, j int) {
-			indices[i], indices[j] = indices[j], indices[i]
-		})
+		if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+			l := len(list)
+			indices := make([]int, l)
+			for i := 0; i < l; i++ {
+				indices[i] = i
+			}
+			rand.Shuffle(l, func(i, j int) {
+				indices[i], indices[j] = indices[j], indices[i]
+			})
 
-		loop = func(list []*agonesv1.GameServer, f func(i int, gs *agonesv1.GameServer)) {
-			for _, i := range indices {
-				f(i, list[i])
+			loop = func(list []*agonesv1.GameServer, f func(i int, gs *agonesv1.GameServer)) {
+				for _, i := range indices {
+					f(i, list[i])
+				}
+			}
+		} else {
+			// For FeatureCountsAndLists we do not do randomized looping -- instead choose the game
+			// server based on the list of Priorities. (The order in which the game servers were sorted
+			// in ListSortedGameServersPriorities.)
+			loop = func(list []*agonesv1.GameServer, f func(i int, gs *agonesv1.GameServer)) {
+				for i, gs := range list {
+					f(i, gs)
+				}
 			}
 		}
 	default:

pkg/gameserverallocations/find_test.go

@@ -323,6 +323,12 @@ func TestFindGameServerForAllocationPacked(t *testing.T) {
 func TestFindGameServerForAllocationDistributed(t *testing.T) {
 	t.Parallel()
 
+	// TODO: remove when `CountsAndLists` feature flag is moved to stable.
+	// NOTE: CountsAndLists has different behavior for Distributed, and the game server list is not random.
+	runtime.FeatureTestMutex.Lock()
+	defer runtime.FeatureTestMutex.Unlock()
+	assert.NoError(t, runtime.ParseFeatures(string(runtime.FeatureCountsAndLists)+"=false"))
+
 	controller, m := newFakeController()
 	c := controller.allocator.allocationCache
 	labels := map[string]string{"role": "gameserver"}

pkg/gameserversets/gameserversets.go

@@ -46,7 +46,7 @@ func SortGameServersByStrategy(strategy apis.SchedulingStrategy, list []*agonesv
 	if strategy == apis.Packed {
 		return sortGameServersByLeastFullNodes(list, counts, priorities)
 	}
-	return sortGameServersByNewFirst(list)
+	return sortGameServersByNewFirst(list, priorities)
 }
 
 // SortGameServersByLeastFullNodes sorts the list of gameservers by which gameservers reside on the least full nodes
@@ -87,7 +87,7 @@ func sortGameServersByLeastFullNodes(list []*agonesv1.GameServer, count map[stri
 		}
 
 		// if the Nodes have the same count and CountsAndLists flag is enabled, sort based on CountsAndLists Priorities.
-		if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) && (priorities != nil) {
+		if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
 			for _, priority := range priorities {
 				res := compareGameServerCapacity(&priority, a, b)
 				switch priority.Order {
@@ -120,14 +120,35 @@ func sortGameServersByLeastFullNodes(list []*agonesv1.GameServer, count map[stri
 	return list
 }
 
-// sortGameServersByNewFirst sorts by newest gameservers first, and returns them
-func sortGameServersByNewFirst(list []*agonesv1.GameServer) []*agonesv1.GameServer {
+// sortGameServersByNewFirst sorts by newest gameservers first.
+// If FeatureCountsAndLists is enabled, sort by Priority first, then tie break with newest gameservers.
+func sortGameServersByNewFirst(list []*agonesv1.GameServer, priorities []agonesv1.Priority) []*agonesv1.GameServer {
 	sort.Slice(list, func(i, j int) bool {
 		a := list[i]
 		b := list[j]
 
-		// TODO: Sort by Priority First, then tie break with NewFirst
-
+		if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+			for _, priority := range priorities {
+				res := compareGameServerCapacity(&priority, a, b)
+				switch priority.Order {
+				case agonesv1.GameServerPriorityAscending:
+					if res == -1 {
+						return true
+					}
+					if res == 1 {
+						return false
+					}
+				case agonesv1.GameServerPriorityDescending:
+					if res == -1 {
+						return false
+					}
+					if res == 1 {
+						return true
+					}
+				}
+			}
+		}
+		// TODO: Do we still want to keep this as the tie-breaker?
 		return a.ObjectMeta.CreationTimestamp.Before(&b.ObjectMeta.CreationTimestamp)
 	})
 

pkg/gameserversets/gameserversets_test.go

@@ -134,20 +134,77 @@ func TestSortGameServersByLeastFullNodes(t *testing.T) {
 }
 
 func TestSortGameServersByNewFirst(t *testing.T) {
+	t.Parallel()
+
+	utilruntime.FeatureTestMutex.Lock()
+	defer utilruntime.FeatureTestMutex.Unlock()
+
+	require.NoError(t, utilruntime.ParseFeatures(string(utilruntime.FeatureCountsAndLists)+"=true"))
+
 	now := metav1.Now()
 
-	list := []*agonesv1.GameServer{
-		{ObjectMeta: metav1.ObjectMeta{Name: "g1", CreationTimestamp: metav1.Time{Time: now.Add(10 * time.Second)}}},
-		{ObjectMeta: metav1.ObjectMeta{Name: "g2", CreationTimestamp: now}},
-		{ObjectMeta: metav1.ObjectMeta{Name: "g3", CreationTimestamp: metav1.Time{Time: now.Add(30 * time.Second)}}},
+	gs1 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "g1", CreationTimestamp: metav1.Time{Time: now.Add(10 * time.Second)}}}
+	gs2 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "g2", CreationTimestamp: now}}
+	gs3 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "g3", CreationTimestamp: metav1.Time{Time: now.Add(30 * time.Second)}}}
+	gs4 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "g4", CreationTimestamp: metav1.Time{Time: now.Add(30 * time.Second)}},
+		Status: agonesv1.GameServerStatus{
+			Counters: map[string]agonesv1.CounterStatus{
+				"bar": {
+					Count:    0,
+					Capacity: 100,
+				}}}}
+	gs5 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "g5", CreationTimestamp: now},
+		Status: agonesv1.GameServerStatus{
+			Counters: map[string]agonesv1.CounterStatus{
+				"bar": {
+					Count:    0,
+					Capacity: 100,
+				}}}}
+	gs6 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "g6", CreationTimestamp: now},
+		Status: agonesv1.GameServerStatus{
+			Counters: map[string]agonesv1.CounterStatus{
+				"bar": {
+					Count:    0,
+					Capacity: 1000,
+				}}}}
+
+	testScenarios := map[string]struct {
+		list       []*agonesv1.GameServer
+		priorities []agonesv1.Priority
+		want       []*agonesv1.GameServer
+	}{
+		"No priorities, sort by creation time": {
+			list:       []*agonesv1.GameServer{&gs1, &gs2, &gs3},
+			priorities: nil,
+			want:       []*agonesv1.GameServer{&gs2, &gs1, &gs3},
+		},
+		"Descending priorities": {
+			list: []*agonesv1.GameServer{&gs4, &gs6, &gs5},
+			priorities: []agonesv1.Priority{{
+				Type:  "Counter",
+				Key:   "bar",
+				Order: "Descending",
+			}},
+			want: []*agonesv1.GameServer{&gs6, &gs5, &gs4},
+		},
+		"Ascending priorities": {
+			list: []*agonesv1.GameServer{&gs4, &gs5, &gs6},
+			priorities: []agonesv1.Priority{{
+				Type:  "Counter",
+				Key:   "bar",
+				Order: "Ascending",
+			}},
+			want: []*agonesv1.GameServer{&gs5, &gs4, &gs6},
+		},
 	}
-	l := len(list)
 
-	result := sortGameServersByNewFirst(list)
-	require.Len(t, result, l)
-	assert.Equal(t, "g2", result[0].ObjectMeta.Name)
-	assert.Equal(t, "g1", result[1].ObjectMeta.Name)
-	assert.Equal(t, "g3", result[2].ObjectMeta.Name)
+	for testName, testScenario := range testScenarios {
+		t.Run(testName, func(t *testing.T) {
+
+			result := sortGameServersByNewFirst(testScenario.list, testScenario.priorities)
+			assert.Equal(t, testScenario.want, result)
+		})
+	}
 }
 
 func TestListGameServersByGameServerSetOwner(t *testing.T) {