From 84c2677130b216d067638015b31f860dbc3c167f Mon Sep 17 00:00:00 2001
From: Ivy Gooch <igooch@google.com>
Date: Thu, 25 May 2023 17:17:37 +0000
Subject: [PATCH] Updates GameServerSet to only aggregate Counters and Lists on
 GameServer States Allocated, Ready, or Reserved

---
 pkg/fleets/controller.go              |  3 ---
 pkg/gameserversets/controller.go      | 12 +++++-------
 pkg/gameserversets/controller_test.go | 22 ++++++++++++----------
 3 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go
index 78d8ba4637..a88925c281 100644
--- a/pkg/fleets/controller.go
+++ b/pkg/fleets/controller.go
@@ -730,9 +730,6 @@ func mergeLists(l1, l2 map[string]agonesv1.AggregatedListStatus) map[string]agon
 
 	for key, val := range l2 {
 		// If the List exists in both maps, aggregate the values.
-		// TODO: Will this cause issues with install/helm/agones/templates/crds/fleet.yaml because in
-		// the CRD we define a maximum capacity and maximum number of values as 1000? (Also a possible
-		// issue with there being a max of 1000 Lists in a Fleet per the CRD?) So far it passes unit tests...
 		if list, ok := l1[key]; ok {
 			list.Capacity += val.Capacity
 			// We do not remove duplicates here.
diff --git a/pkg/gameserversets/controller.go b/pkg/gameserversets/controller.go
index ab6aed73b8..99a392a8a4 100644
--- a/pkg/gameserversets/controller.go
+++ b/pkg/gameserversets/controller.go
@@ -620,10 +620,11 @@ func computeStatus(list []*agonesv1.GameServer) agonesv1.GameServerSetStatus {
 			status.ReservedReplicas++
 		}
 
-		// TODO: Aggregates all Counters and Lists for all GameServer States that are not Deleting. This
-		// means that unlike player tracking below, this will aggregate values during GameServerStateCreating.
-		// Should I mimic player tracking below and only aggregate for cases Ready, Reserved, and Allocated?
-		if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+		// Aggregates all Counters and Lists only for GameServer states Ready, Reserved, or Allocated.
+		if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) &&
+			(gs.Status.State == agonesv1.GameServerStateReady ||
+				gs.Status.State == agonesv1.GameServerStateAllocated ||
+				gs.Status.State == agonesv1.GameServerStateReserved) {
 			status.Counters = aggregateCounters(status.Counters, gs.Status.Counters)
 			status.Lists = aggregateLists(status.Lists, gs.Status.Lists)
 		}
@@ -678,9 +679,6 @@ func aggregateLists(aggListStatus map[string]agonesv1.AggregatedListStatus, list
 
 	for key, val := range listStatus {
 		// If the List exists in both maps, aggregate the values.
-		// TODO: Will this cause issues with install/helm/agones/templates/crds/gameserverset.yaml because in
-		// the CRD we define a maximum capacity and maximum number of values as 1000? (Also a possible
-		// issue with there being a max of 1000 Lists in a GameServerSet per the CRD?)
 		if list, ok := aggListStatus[key]; ok {
 			list.Capacity += val.Capacity
 			// We do not remove duplicates here.
diff --git a/pkg/gameserversets/controller_test.go b/pkg/gameserversets/controller_test.go
index 87a3d4fd8e..f2fda95b5a 100644
--- a/pkg/gameserversets/controller_test.go
+++ b/pkg/gameserversets/controller_test.go
@@ -359,7 +359,8 @@ func TestComputeStatus(t *testing.T) {
 		}
 		gs4 := gsWithState(agonesv1.GameServerStateReady)
 		gs4.Status.Counters = map[string]agonesv1.CounterStatus{
-			"firstCounter": {Count: 15, Capacity: 30},
+			"firstCounter":  {Count: 15, Capacity: 30},
+			"secondCounter": {Count: 20, Capacity: 200},
 		}
 		list = append(list, gs1, gs2, gs3, gs4)
 
@@ -370,12 +371,12 @@ func TestComputeStatus(t *testing.T) {
 			AllocatedReplicas: 1,
 			Counters: map[string]agonesv1.AggregatedCounterStatus{
 				"firstCounter": {
-					Count:    50,
-					Capacity: 85,
+					Count:    30,
+					Capacity: 55,
 				},
 				"secondCounter": {
-					Count:    200,
-					Capacity: 2000,
+					Count:    120,
+					Capacity: 1200,
 				},
 			},
 			Lists: map[string]agonesv1.AggregatedListStatus{},
@@ -407,7 +408,8 @@ func TestComputeStatus(t *testing.T) {
 		}
 		gs4 := gsWithState(agonesv1.GameServerStateReady)
 		gs4.Status.Lists = map[string]agonesv1.ListStatus{
-			"firstList": {Capacity: 30},
+			"firstList":  {Capacity: 30},
+			"secondList": {Capacity: 100, Values: []string{"4"}},
 		}
 		list = append(list, gs1, gs2, gs3, gs4)
 
@@ -419,12 +421,12 @@ func TestComputeStatus(t *testing.T) {
 			Counters:          map[string]agonesv1.AggregatedCounterStatus{},
 			Lists: map[string]agonesv1.AggregatedListStatus{
 				"firstList": {
-					Capacity: 85,
-					Values:   []string{"a", "b", "c", "d"},
+					Capacity: 55,
+					Values:   []string{"a", "b", "c"},
 				},
 				"secondList": {
-					Capacity: 2000,
-					Values:   []string{"1", "2", "3"},
+					Capacity: 1100,
+					Values:   []string{"1", "2", "4"},
 				},
 			},
 		}