From 6f697f201e6af8a0af783ae8614ff887bb9f05f1 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Fri, 28 Jul 2023 22:32:30 +0000 Subject: [PATCH] Sort by Priority for strategy Distributed for Game Server Allocation and on Fleet scale down. --- pkg/gameserverallocations/allocation_cache.go | 43 +++++++++++ .../allocation_cache_test.go | 19 ++++- pkg/gameserverallocations/allocator.go | 8 +- pkg/gameserverallocations/allocator_test.go | 30 +++++--- pkg/gameserverallocations/find.go | 34 +++++--- pkg/gameservers/controller_test.go | 1 - pkg/gameserversets/gameserversets.go | 33 ++++++-- pkg/gameserversets/gameserversets_test.go | 77 ++++++++++++++++--- 8 files changed, 205 insertions(+), 40 deletions(-) diff --git a/pkg/gameserverallocations/allocation_cache.go b/pkg/gameserverallocations/allocation_cache.go index 73bb692c84..1710537470 100644 --- a/pkg/gameserverallocations/allocation_cache.go +++ b/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. diff --git a/pkg/gameserverallocations/allocation_cache_test.go b/pkg/gameserverallocations/allocation_cache_test.go index a6071f57a4..0a5e614188 100644 --- a/pkg/gameserverallocations/allocation_cache_test.go +++ b/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) }) diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go index 67db3bdb69..b36c345875 100644 --- a/pkg/gameserverallocations/allocator.go +++ b/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" @@ -531,7 +532,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) diff --git a/pkg/gameserverallocations/allocator_test.go b/pkg/gameserverallocations/allocator_test.go index d94d3ce85a..ddc4ebe24c 100644 --- a/pkg/gameserverallocations/allocator_test.go +++ b/pkg/gameserverallocations/allocator_test.go @@ -680,7 +680,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, @@ -693,7 +693,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, @@ -706,28 +706,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) diff --git a/pkg/gameserverallocations/find.go b/pkg/gameserverallocations/find.go index d0c68aeb05..3a0f23c8ba 100644 --- a/pkg/gameserverallocations/find.go +++ b/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: diff --git a/pkg/gameservers/controller_test.go b/pkg/gameservers/controller_test.go index 39bf8bd2a2..ddacc61ed5 100644 --- a/pkg/gameservers/controller_test.go +++ b/pkg/gameservers/controller_test.go @@ -233,7 +233,6 @@ func TestControllerSyncGameServerWithDevIP(t *testing.T) { gsFixture.ApplyDefaults() gsFixture.Status.State = agonesv1.GameServerStateRequestReady - mocks.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { gameServers := &agonesv1.GameServerList{Items: []agonesv1.GameServer{*gsFixture}} return true, gameServers, nil diff --git a/pkg/gameserversets/gameserversets.go b/pkg/gameserversets/gameserversets.go index 04c406d167..ad909802b0 100644 --- a/pkg/gameserversets/gameserversets.go +++ b/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) }) diff --git a/pkg/gameserversets/gameserversets_test.go b/pkg/gameserversets/gameserversets_test.go index 472a3df897..1662a74f29 100644 --- a/pkg/gameserversets/gameserversets_test.go +++ b/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) {