Allowing list based fleet autoscaler to scale up from 0 replicas (#4016)

* Allowing list based fleet autoscaler to scale up from 0 replicas * Omiting discarded 2nd value from range * Fixing unit tests * Moving the list initialization logic from fleet controller to gsset controller * Addressing PR comments * Update pkg/fleetautoscalers/fleetautoscalers.go --------- Co-authored-by: igooch <[email protected]>
googleforgames · Oct 31, 2024 · 0056256 · 0056256
1 parent a27aeab
commit 0056256
Show file tree

Hide file tree

Showing 6 changed files with 181 additions and 10 deletions.
diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go
@@ -355,7 +355,7 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L
 		}
 		return replicas, false, nil
 	case availableCapacity < buffer: // Scale Up
-		if limited && scale == -1 { // Case where we want to scale up but we're already limited by MaxCapacity
+		if limited { // Case where we want to scale up but we're already limited by MaxCapacity.
 			return scaleLimited(scale, f, gameServerLister, nodeCounts, key, isCounter, replicas,
 				capacity, aggCapacity, minCapacity, maxCapacity)
 		}

diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go
@@ -1570,6 +1570,31 @@ func TestApplyListPolicy(t *testing.T) {
 				wantErr:  true,
 			},
 		},
+		"fleet does not have any replicas": {
+			fleet: modifiedFleet(func(f *agonesv1.Fleet) {
+				f.Spec.Template.Spec.Lists = make(map[string]agonesv1.ListStatus)
+				f.Spec.Template.Spec.Lists["gamers"] = agonesv1.ListStatus{
+					Values:   []string{},
+					Capacity: 7}
+				f.Status.Replicas = 0
+				f.Status.ReadyReplicas = 0
+				f.Status.AllocatedReplicas = 0
+				f.Status.Lists = make(map[string]agonesv1.AggregatedListStatus)
+				f.Status.Lists["gamers"] = agonesv1.AggregatedListStatus{}
+			}),
+			featureFlags: string(utilruntime.FeatureCountsAndLists) + "=true",
+			lp: &autoscalingv1.ListPolicy{
+				Key:         "gamers",
+				MaxCapacity: 100,
+				MinCapacity: 10,
+				BufferSize:  intstr.FromInt(10),
+			},
+			want: expected{
+				replicas: 2,
+				limited:  true,
+				wantErr:  false,
+			},
+		},
 		"scale up": {
 			fleet: modifiedFleet(func(f *agonesv1.Fleet) {
 				f.Spec.Template.Spec.Lists = make(map[string]agonesv1.ListStatus)

diff --git a/pkg/gameserversets/controller.go b/pkg/gameserversets/controller.go
@@ -320,7 +320,7 @@ func (c *Controller) syncGameServerSet(ctx context.Context, key string) error {
 		numServersToAdd = 0
 	}
 
-	status := computeStatus(list)
+	status := computeStatus(gsSet, list)
 	fields := logrus.Fields{}
 
 	for _, gs := range list {
@@ -616,7 +616,7 @@ func parallelize(gameServers chan *agonesv1.GameServer, parallelism int, work fu
 
 // syncGameServerSetStatus synchronises the GameServerSet State with active GameServer counts
 func (c *Controller) syncGameServerSetStatus(ctx context.Context, gsSet *agonesv1.GameServerSet, list []*agonesv1.GameServer) error {
-	return c.updateStatusIfChanged(ctx, gsSet, computeStatus(list))
+	return c.updateStatusIfChanged(ctx, gsSet, computeStatus(gsSet, list))
 }
 
 // updateStatusIfChanged updates GameServerSet status if it's different than provided.
@@ -633,8 +633,13 @@ func (c *Controller) updateStatusIfChanged(ctx context.Context, gsSet *agonesv1.
 }
 
 // computeStatus computes the status of the game server set.
-func computeStatus(list []*agonesv1.GameServer) agonesv1.GameServerSetStatus {
+func computeStatus(gsSet *agonesv1.GameServerSet, list []*agonesv1.GameServer) agonesv1.GameServerSetStatus {
 	var status agonesv1.GameServerSetStatus
+
+	// Initialize list status with empty lists from spec
+	if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+		status.Lists = createInitialListStatus(gsSet)
+	}
 	for _, gs := range list {
 		if gs.IsBeingDeleted() {
 			// don't count GS that are being deleted
@@ -687,6 +692,14 @@ func computeStatus(list []*agonesv1.GameServer) agonesv1.GameServerSetStatus {
 	return status
 }
 
+func createInitialListStatus(gsSet *agonesv1.GameServerSet) map[string]agonesv1.AggregatedListStatus {
+	list := make(map[string]agonesv1.AggregatedListStatus)
+	for name := range gsSet.Spec.Template.Spec.Lists {
+		list[name] = agonesv1.AggregatedListStatus{}
+	}
+	return list
+}
+
 // aggregateCounters adds the contents of a CounterStatus map to an AggregatedCounterStatus map.
 func aggregateCounters(aggCounterStatus map[string]agonesv1.AggregatedCounterStatus,
 	counterStatus map[string]agonesv1.CounterStatus,

diff --git a/pkg/gameserversets/controller_test.go b/pkg/gameserversets/controller_test.go
@@ -283,7 +283,7 @@ func TestComputeStatus(t *testing.T) {
 		defer utilruntime.FeatureTestMutex.Unlock()
 
 		require.NoError(t, utilruntime.ParseFeatures(fmt.Sprintf("%s=false", utilruntime.FeatureCountsAndLists)))
-
+		gsSet := defaultFixture()
 		cases := []struct {
 			list       []*agonesv1.GameServer
 			wantStatus agonesv1.GameServerSetStatus
@@ -310,7 +310,7 @@ func TestComputeStatus(t *testing.T) {
 		}
 
 		for _, tc := range cases {
-			assert.Equal(t, tc.wantStatus, computeStatus(tc.list))
+			assert.Equal(t, tc.wantStatus, computeStatus(gsSet, tc.list))
 		}
 	})
 
@@ -320,6 +320,7 @@ func TestComputeStatus(t *testing.T) {
 
 		require.NoError(t, utilruntime.ParseFeatures(fmt.Sprintf("%s=true", utilruntime.FeaturePlayerTracking)))
 
+		gsSet := defaultFixture()
 		var list []*agonesv1.GameServer
 		gs1 := gsWithState(agonesv1.GameServerStateAllocated)
 		gs1.Status.Players = &agonesv1.PlayerStatus{Count: 5, Capacity: 10}
@@ -344,7 +345,7 @@ func TestComputeStatus(t *testing.T) {
 			Lists:    map[string]agonesv1.AggregatedListStatus{},
 		}
 
-		assert.Equal(t, expected, computeStatus(list))
+		assert.Equal(t, expected, computeStatus(gsSet, list))
 	})
 
 	t.Run("counters", func(t *testing.T) {
@@ -353,6 +354,7 @@ func TestComputeStatus(t *testing.T) {
 
 		require.NoError(t, utilruntime.ParseFeatures(fmt.Sprintf("%s=true", utilruntime.FeatureCountsAndLists)))
 
+		gsSet := defaultFixture()
 		var list []*agonesv1.GameServer
 		gs1 := gsWithState(agonesv1.GameServerStateAllocated)
 		gs1.Status.Counters = map[string]agonesv1.CounterStatus{
@@ -407,7 +409,7 @@ func TestComputeStatus(t *testing.T) {
 			Lists: map[string]agonesv1.AggregatedListStatus{},
 		}
 
-		assert.Equal(t, expected, computeStatus(list))
+		assert.Equal(t, expected, computeStatus(gsSet, list))
 	})
 
 	t.Run("lists", func(t *testing.T) {
@@ -416,6 +418,7 @@ func TestComputeStatus(t *testing.T) {
 
 		require.NoError(t, utilruntime.ParseFeatures(fmt.Sprintf("%s=true", utilruntime.FeatureCountsAndLists)))
 
+		gsSet := defaultFixture()
 		var list []*agonesv1.GameServer
 		gs1 := gsWithState(agonesv1.GameServerStateAllocated)
 		gs1.Status.Lists = map[string]agonesv1.ListStatus{
@@ -460,7 +463,45 @@ func TestComputeStatus(t *testing.T) {
 			},
 		}
 
-		assert.Equal(t, expected, computeStatus(list))
+		assert.Equal(t, expected, computeStatus(gsSet, list))
+	})
+
+	t.Run("lists with no gameservers", func(t *testing.T) {
+		utilruntime.FeatureTestMutex.Lock()
+		defer utilruntime.FeatureTestMutex.Unlock()
+
+		require.NoError(t, utilruntime.ParseFeatures(fmt.Sprintf("%s=true", utilruntime.FeatureCountsAndLists)))
+
+		gsSet := defaultFixture()
+		gsSet.Spec.Template.Spec.Lists = map[string]agonesv1.ListStatus{
+			"firstList":  {Capacity: 10, Values: []string{"a", "b"}},
+			"secondList": {Capacity: 1000, Values: []string{"1", "2"}},
+		}
+		var list []*agonesv1.GameServer
+
+		expected := agonesv1.GameServerSetStatus{
+			Replicas:          0,
+			ReadyReplicas:     0,
+			ReservedReplicas:  0,
+			AllocatedReplicas: 0,
+			Counters:          nil,
+			Lists: map[string]agonesv1.AggregatedListStatus{
+				"firstList": {
+					AllocatedCount:    0,
+					AllocatedCapacity: 0,
+					Capacity:          0,
+					Count:             0,
+				},
+				"secondList": {
+					AllocatedCount:    0,
+					AllocatedCapacity: 0,
+					Capacity:          0,
+					Count:             0,
+				},
+			},
+		}
+
+		assert.Equal(t, expected, computeStatus(gsSet, list))
 	})
 }
 

diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go
@@ -1980,12 +1980,23 @@ func defaultFleet(namespace string) *agonesv1.Fleet {
 	return fleetWithGameServerSpec(&gs.Spec, namespace)
 }
 
+// defaultEmptyFleet returns a default fleet configuration with no replicas.
+func defaultEmptyFleet(namespace string) *agonesv1.Fleet {
+	gs := framework.DefaultGameServer(namespace)
+	return fleetWithGameServerSpecAndReplicas(&gs.Spec, namespace, 0)
+}
+
 // fleetWithGameServerSpec returns a fleet with specified gameserver spec
 func fleetWithGameServerSpec(gsSpec *agonesv1.GameServerSpec, namespace string) *agonesv1.Fleet {
+	return fleetWithGameServerSpecAndReplicas(gsSpec, namespace, replicasCount)
+}
+
+// fleetWithGameServerSpecAndReplicas returns a fleet with specified gameserver spec and specified replica count
+func fleetWithGameServerSpecAndReplicas(gsSpec *agonesv1.GameServerSpec, namespace string, replicas int32) *agonesv1.Fleet {
 	return &agonesv1.Fleet{
 		ObjectMeta: metav1.ObjectMeta{GenerateName: "simple-fleet-1.0", Namespace: namespace},
 		Spec: agonesv1.FleetSpec{
-			Replicas: replicasCount,
+			Replicas: replicas,
 			Template: agonesv1.GameServerTemplateSpec{
 				Spec: *gsSpec,
 			},

diff --git a/test/e2e/fleetautoscaler_test.go b/test/e2e/fleetautoscaler_test.go
@@ -1209,6 +1209,87 @@ func TestListAutoscaler(t *testing.T) {
 	}
 }
 
+func TestListAutoscalerWithNoReplicas(t *testing.T) {
+	if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+		t.SkipNow()
+	}
+	t.Parallel()
+
+	ctx := context.Background()
+	client := framework.AgonesClient.AgonesV1()
+	log := e2e.TestLogger(t)
+
+	flt := defaultEmptyFleet(framework.Namespace)
+	flt.Spec.Template.Spec.Lists = map[string]agonesv1.ListStatus{
+		"games": {
+			Capacity: 5,
+		},
+	}
+
+	flt, err := client.Fleets(framework.Namespace).Create(ctx, flt.DeepCopy(), metav1.CreateOptions{})
+	require.NoError(t, err)
+	defer client.Fleets(framework.Namespace).Delete(ctx, flt.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
+	framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas))
+
+	fleetautoscalers := framework.AgonesClient.AutoscalingV1().FleetAutoscalers(framework.Namespace)
+
+	listFas := func(f func(fap *autoscalingv1.FleetAutoscalerPolicy)) *autoscalingv1.FleetAutoscaler {
+		fas := autoscalingv1.FleetAutoscaler{
+			ObjectMeta: metav1.ObjectMeta{Name: flt.ObjectMeta.Name + "-list-autoscaler", Namespace: framework.Namespace},
+			Spec: autoscalingv1.FleetAutoscalerSpec{
+				FleetName: flt.ObjectMeta.Name,
+				Policy: autoscalingv1.FleetAutoscalerPolicy{
+					Type: autoscalingv1.ListPolicyType,
+				},
+				Sync: &autoscalingv1.FleetAutoscalerSync{
+					Type: autoscalingv1.FixedIntervalSyncType,
+					FixedInterval: autoscalingv1.FixedIntervalSync{
+						Seconds: 1,
+					},
+				},
+			},
+		}
+		f(&fas.Spec.Policy)
+		return &fas
+	}
+	testCases := map[string]struct {
+		fas          *autoscalingv1.FleetAutoscaler
+		wantFasErr   bool
+		wantReplicas int32
+	}{
+		"Scale Up to MinCapacity": {
+			fas: listFas(func(fap *autoscalingv1.FleetAutoscalerPolicy) {
+				fap.List = &autoscalingv1.ListPolicy{
+					Key:         "games",
+					BufferSize:  intstr.FromInt(3),
+					MinCapacity: 16,
+					MaxCapacity: 100,
+				}
+			}),
+			wantFasErr:   false,
+			wantReplicas: 4, // Capacity:20
+		},
+	}
+	for name, testCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+
+			fas, err := fleetautoscalers.Create(ctx, testCase.fas, metav1.CreateOptions{})
+			if testCase.wantFasErr {
+				assert.Error(t, err)
+				return
+			}
+			assert.NoError(t, err)
+
+			framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(testCase.wantReplicas))
+			fleetautoscalers.Delete(ctx, fas.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
+
+			// Return to starting 0 replicas
+			framework.ScaleFleet(t, log, flt, 0)
+			framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(0))
+		})
+	}
+}
+
 func TestListAutoscalerAllocated(t *testing.T) {
 	if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
 		t.SkipNow()