Skip to content

Commit

Permalink
Counters & Lists: Consolidate priorities sorting
Browse files Browse the repository at this point in the history 
Doing work on googleforgames#3649, realised the `priorities` sorting implementation
between a `GameServerAllocation` and `Fleet/GameServerSet` was
different.

Once used available capacity, and the latter used current capacity.

To make things simpler, both from an implementation point of view, and
documentation and user understanding, this PR consolidates everything
into using _available capacity_ across the board.

This also includes:
* Some refactoring to consolidate Counter and List priority sorting
logic as much as possible.
* Fixes a bug in `Packed` GameServerSet scale down logic.
* Fixes for tests since logic changed.

Additional context: googleforgames#3649 (comment)
  • Loading branch information
@markmandel
markmandel committed Mar 9, 2024
1 parent e9af555 commit 6e1c648
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 231 deletions.
84 changes: 84 additions & 0 deletions pkg/apis/agones/v1/gameserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -981,3 +981,87 @@ func MergeRemoveDuplicates(list1 []string, list2 []string) []string {
}
return uniqueList
}

// CompareCountAndListPriorities compares two game servers based on a list of CountsAndLists Priorities using available
// capacity as the comparison.
func (gs *GameServer) CompareCountAndListPriorities(priorities []Priority, other *GameServer) *bool {
for _, priority := range priorities {
res := gs.compareCountAndListPriority(&priority, other)
if res != nil {
// reverse if descending
if priority.Order == GameServerPriorityDescending {
flip := !*res
return &flip
}

return res
}
}

return nil
}

// compareCountAndListPriority 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 true if gs1 < gs2; false if gs1 > gs2; nil if gs1 == gs2; nil if neither gamer server has the Priority.
// If only one game server has the Priority, prefer that server. I.e. nil < gsX when Priority
// Order is Descending (3, 2, 1, 0, nil), and nil > gsX when Order is Ascending (0, 1, 2, 3, nil).
func (gs *GameServer) compareCountAndListPriority(p *Priority, other *GameServer) *bool {
var gs1ok, gs2ok bool
t := true
f := false
switch p.Type {
case GameServerPriorityCounter:
// Check if both game servers contain the Counter.
counter1, ok1 := gs.Status.Counters[p.Key]
counter2, ok2 := other.Status.Counters[p.Key]
// If both game servers have the Counter
if ok1 && ok2 {
availCapacity1 := counter1.Capacity - counter1.Count
availCapacity2 := counter2.Capacity - counter2.Count
if availCapacity1 < availCapacity2 {
return &t
}
if availCapacity1 > availCapacity2 {
return &f
}
if availCapacity1 == availCapacity2 {
return nil
}
}
gs1ok = ok1
gs2ok = ok2
case GameServerPriorityList:
// Check if both game servers contain the List.
list1, ok1 := gs.Status.Lists[p.Key]
list2, ok2 := other.Status.Lists[p.Key]
// If both game servers have the List
if ok1 && ok2 {
availCapacity1 := list1.Capacity - int64(len(list1.Values))
availCapacity2 := list2.Capacity - int64(len(list2.Values))
if availCapacity1 < availCapacity2 {
return &t
}
if availCapacity1 > availCapacity2 {
return &f
}
if availCapacity1 == availCapacity2 {
return nil
}
}
gs1ok = ok1
gs2ok = ok2
}
// If only one game server has the Priority, prefer that server. I.e. nil < gsX when Order is
// Descending (3, 2, 1, 0, nil), and nil > gsX when Order is Ascending (0, 1, 2, 3, nil).
if (gs1ok && p.Order == GameServerPriorityDescending) ||
(gs2ok && p.Order == GameServerPriorityAscending) {
return &f
}
if (gs1ok && p.Order == GameServerPriorityAscending) ||
(gs2ok && p.Order == GameServerPriorityDescending) {
return &t
}
// If neither game server has the Priority
return nil
}
4 changes: 2 additions & 2 deletions pkg/fleetautoscalers/fleetautoscalers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ func TestApplyCounterPolicy(t *testing.T) {
}}}},
},
want: expected{
replicas: 2,
replicas: 1,
limited: false,
wantErr: false,
},
Expand Down Expand Up @@ -1729,7 +1729,7 @@ func TestApplyListPolicy(t *testing.T) {
}}}},
},
want: expected{
replicas: 3,
replicas: 4,
limited: false,
wantErr: false,
},
Expand Down
103 changes: 4 additions & 99 deletions pkg/gameserverallocations/allocation_cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,24 +225,8 @@ func (c *AllocationCache) ListSortedGameServers(gsa *allocationv1.GameServerAllo

// if we end up here, then break the tie with Counter or List Priority.
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
}
}
if res := gs1.CompareCountAndListPriorities(gsa.Spec.Priorities, gs2); res != nil {
return *res
}
}

Expand All @@ -266,24 +250,8 @@ func (c *AllocationCache) ListSortedGameServersPriorities(gsa *allocationv1.Game
gs2 := list[j]

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
}
}
if res := gs1.CompareCountAndListPriorities(gsa.Spec.Priorities, gs2); res != nil {
return *res
}
}

Expand All @@ -294,69 +262,6 @@ func (c *AllocationCache) ListSortedGameServersPriorities(gsa *allocationv1.Game
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.
// If only one game server has the Priority, prefer that server. I.e. nil < gsX when Priority
// Order is Descending (3, 2, 1, 0, nil), and nil > gsX when Order is Ascending (0, 1, 2, 3, nil).
func compareGameServers(p *agonesv1.Priority, gs1, gs2 *agonesv1.GameServer) int {
var gs1ok, gs2ok bool
switch p.Type {
case agonesv1.GameServerPriorityCounter:
// Check if both game servers contain the Counter.
counter1, ok1 := gs1.Status.Counters[p.Key]
counter2, ok2 := gs2.Status.Counters[p.Key]
// If both game servers have the Counter
if ok1 && ok2 {
availCapacity1 := counter1.Capacity - counter1.Count
availCapacity2 := counter2.Capacity - counter2.Count
if availCapacity1 < availCapacity2 {
return -1
}
if availCapacity1 > availCapacity2 {
return 1
}
if availCapacity1 == availCapacity2 {
return 0
}
}
gs1ok = ok1
gs2ok = ok2
case agonesv1.GameServerPriorityList:
// Check if both game servers contain the List.
list1, ok1 := gs1.Status.Lists[p.Key]
list2, ok2 := gs2.Status.Lists[p.Key]
// If both game servers have the List
if ok1 && ok2 {
availCapacity1 := list1.Capacity - int64(len(list1.Values))
availCapacity2 := list2.Capacity - int64(len(list2.Values))
if availCapacity1 < availCapacity2 {
return -1
}
if availCapacity1 > availCapacity2 {
return 1
}
if availCapacity1 == availCapacity2 {
return 0
}
}
gs1ok = ok1
gs2ok = ok2
}
// If only one game server has the Priority, prefer that server. I.e. nil < gsX when Order is
// Descending (3, 2, 1, 0, nil), and nil > gsX when Order is Ascending (0, 1, 2, 3, nil).
if (gs1ok && p.Order == agonesv1.GameServerPriorityDescending) ||
(gs2ok && p.Order == agonesv1.GameServerPriorityAscending) {
return 1
}
if (gs1ok && p.Order == agonesv1.GameServerPriorityAscending) ||
(gs2ok && p.Order == agonesv1.GameServerPriorityDescending) {
return -1
}
// If neither game server has the Priority
return 0
}

// SyncGameServers synchronises the GameServers to Gameserver cache. This is called when a failure
// happened during the allocation. This method will sync and make sure the cache is up to date.
func (c *AllocationCache) SyncGameServers(ctx context.Context, key string) error {
Expand Down
123 changes: 9 additions & 114 deletions pkg/gameserversets/gameserversets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,31 +86,15 @@ func sortGameServersByPackedStrategy(list []*agonesv1.GameServer, count map[stri
return false
}

// if the Nodes have the same count and CountsAndLists flag is enabled, sort based on CountsAndLists Priorities.
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
}
if a.Status.NodeName == b.Status.NodeName {
// See if Count and List priorities can be used as a tie-breaker within the node
if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
if res := a.CompareCountAndListPriorities(priorities, b); res != nil {
return *res
}
}
}

// if the gameservers are on the same node, then sort lexigraphically for a stable sort
if a.Status.NodeName == b.Status.NodeName {
// Sort lexicographically for a stable sort within the node
return a.GetObjectMeta().GetName() < b.GetObjectMeta().GetName()
}
// if both Nodes have the same count, one node is emptied first (packed scheduling behavior)
Expand All @@ -121,31 +105,15 @@ func sortGameServersByPackedStrategy(list []*agonesv1.GameServer, count map[stri
}

// sortGameServersByDistributedStrategy sorts by newest gameservers first.
// If FeatureCountsAndLists is enabled, sort by Priority first, then tie break with newest gameservers.
// If FeatureCountsAndLists is enabled, sort by Priority first, then tie-break with newest gameservers.
func sortGameServersByDistributedStrategy(list []*agonesv1.GameServer, priorities []agonesv1.Priority) []*agonesv1.GameServer {
sort.Slice(list, func(i, j int) bool {
a := list[i]
b := list[j]

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
}
}
if res := a.CompareCountAndListPriorities(priorities, b); res != nil {
return *res
}
}

Expand All @@ -172,76 +140,3 @@ func ListGameServersByGameServerSetOwner(gameServerLister listerv1.GameServerLis

return result, nil
}

// compareGameServerCapacity compares two game servers based on a CountsAndLists Priority. First
// compares by Capacity, and then compares by Count or length of the List Values if Capacity is equal.
// NOTE: This does NOT sort by available capacity.
// Returns -1 if gs1 < gs2; 1 if gs1 > gs2; 0 if gs1 == gs2; 0 if neither gamer server has the Priority.
// If only one game server has the Priority, prefer that server. I.e. nil < gsX when Priority
// Order is Descending (3, 2, 1, 0, nil), and nil > gsX when Order is Ascending (0, 1, 2, 3, nil).
func compareGameServerCapacity(p *agonesv1.Priority, gs1, gs2 *agonesv1.GameServer) int {
var gs1ok, gs2ok bool
switch p.Type {
case agonesv1.GameServerPriorityCounter:
// Check if both game servers contain the Counter.
counter1, ok1 := gs1.Status.Counters[p.Key]
counter2, ok2 := gs2.Status.Counters[p.Key]
// If both game servers have the Counter
if ok1 && ok2 {
if counter1.Capacity < counter2.Capacity {
return -1
}
if counter1.Capacity > counter2.Capacity {
return 1
}
if counter1.Capacity == counter2.Capacity {
if counter1.Count < counter2.Count {
return -1
}
if counter1.Count > counter2.Count {
return 1
}
return 0
}
}
gs1ok = ok1
gs2ok = ok2
case agonesv1.GameServerPriorityList:
// Check if both game servers contain the List.
list1, ok1 := gs1.Status.Lists[p.Key]
list2, ok2 := gs2.Status.Lists[p.Key]
// If both game servers have the List
if ok1 && ok2 {
if list1.Capacity < list2.Capacity {
return -1
}
if list1.Capacity > list2.Capacity {
return 1
}
if list1.Capacity == list2.Capacity {
if len(list1.Values) < len(list2.Values) {
return -1
}
if len(list1.Values) > len(list2.Values) {
return 1
}
return 0
}
}
gs1ok = ok1
gs2ok = ok2
}
// If only one game server has the Priority, prefer that server. I.e. nil < gsX when Order is
// Descending (3, 2, 1, 0, nil), and nil > gsX when Order is Ascending (0, 1, 2, 3, nil).
// No Order specified "" is the same as Ascending.
if (gs1ok && p.Order == agonesv1.GameServerPriorityDescending) ||
(gs2ok && p.Order == agonesv1.GameServerPriorityAscending) {
return 1
}
if (gs1ok && p.Order == agonesv1.GameServerPriorityAscending) ||
(gs2ok && p.Order == agonesv1.GameServerPriorityDescending) {
return -1
}
// If neither game server has the Priority
return 0
}
Loading

0 comments on commit 6e1c648

Please sign in to comment.