Adds edge and test cases to applyCounterOrListPolicy

cmd/controller/main.go

@@ -224,7 +224,7 @@ func main() {
 		kubeClient, extClient, agonesClient, agonesInformerFactory)
 	fleetController := fleets.NewController(health, kubeClient, extClient, agonesClient, agonesInformerFactory)
 	fasController := fleetautoscalers.NewController(health,
-		kubeClient, extClient, agonesClient, agonesInformerFactory)
+		kubeClient, extClient, agonesClient, agonesInformerFactory, gsCounter)
 
 	rs = append(rs,
 		gsCounter, gsController, gsSetController, fleetController, fasController)

pkg/apis/agones/v1/common.go

@@ -34,18 +34,18 @@ const (
 	ErrContainerPortRequired    = "ContainerPort must be defined for Dynamic and Static PortPolicies"
 	ErrContainerPortPassthrough = "ContainerPort cannot be specified with Passthrough PortPolicy"
 	ErrContainerNameInvalid     = "Container must be empty or the name of a container in the pod template"
-	// GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be incremented at Allocation.
-	GameServerAllocationIncrement string = "Increment"
-	// GameServerAllocationDecrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation.
-	GameServerAllocationDecrement string = "Decrement"
-	// GameServerAllocationPriorityCounter is a PriorityType for sorting Game Servers by Counter
-	GameServerAllocationPriorityCounter string = "Counter"
-	// GameServerAllocationPriorityList is a PriorityType for sorting Game Servers by List
-	GameServerAllocationPriorityList string = "List"
-	// GameServerAllocationAscending is a Priority Order where the smaller count is preferred in sorting.
-	GameServerAllocationAscending string = "Ascending"
-	// GameServerAllocationDescending is a Priority Order where the larger count is preferred in sorting.
-	GameServerAllocationDescending string = "Descending"
+	// GameServerPriorityIncrement is a Counter Action that indiciates the Counter's Count should be incremented at Allocation.
+	GameServerPriorityIncrement string = "Increment"
+	// GameServerPriorityDecrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation.
+	GameServerPriorityDecrement string = "Decrement"
+	// GameServerPriorityCounter is a PriorityType for sorting Game Servers by Counter
+	GameServerPriorityCounter string = "Counter"
+	// GameServerPriorityList is a PriorityType for sorting Game Servers by List
+	GameServerPriorityList string = "List"
+	// GameServerPriorityAscending is a Priority Order where the smaller count is preferred in sorting.
+	GameServerPriorityAscending string = "Ascending"
+	// GameServerPriorityDescending is a Priority Order where the larger count is preferred in sorting.
+	GameServerPriorityDescending string = "Descending"
 )
 
 // AggregatedPlayerStatus stores total player tracking values
@@ -236,15 +236,15 @@ type Priority struct {
 func (p *Priority) Validate(f string) ([]metav1.StatusCause, bool) {
 	var causes []metav1.StatusCause
 
-	if !(p.PriorityType == GameServerAllocationPriorityCounter || p.PriorityType == GameServerAllocationPriorityList) {
+	if !(p.PriorityType == GameServerPriorityCounter || p.PriorityType == GameServerPriorityList) {
 		causes = append(causes, metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueInvalid,
 			Message: "Priority.Sort must be either `Counter` or `List`",
 			Field:   f,
 		})
 	}
 
-	if !(p.Order == GameServerAllocationAscending || p.Order == GameServerAllocationDescending || p.Order == "") {
+	if !(p.Order == GameServerPriorityAscending || p.Order == GameServerPriorityDescending || p.Order == "") {
 		causes = append(causes, metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueInvalid,
 			Message: "Priority.Order must be either `Ascending` or `Descending`",

pkg/apis/agones/v1/gameserver.go

@@ -914,15 +914,15 @@ func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) {
 
 // UpdateCount increments or decrements a CounterStatus on a Game Server by the given amount.
 func (gs *GameServer) UpdateCount(name string, action string, amount int64) error {
-	if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) {
-		return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Allocation action must be one of %s or %s", name, action, amount, GameServerAllocationIncrement, GameServerAllocationDecrement)
+	if !(action == GameServerPriorityIncrement || action == GameServerPriorityDecrement) {
+		return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Allocation action must be one of %s or %s", name, action, amount, GameServerPriorityIncrement, GameServerPriorityDecrement)
 	}
 	if amount < 0 {
 		return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Amount must be greater than 0", name, action, amount)
 	}
 	if counter, ok := gs.Status.Counters[name]; ok {
 		cnt := counter.Count
-		if action == GameServerAllocationIncrement {
+		if action == GameServerPriorityIncrement {
 			cnt += amount
 			// only check for Count > Capacity when incrementing
 			if cnt > counter.Capacity {

pkg/apis/autoscaling/v1/fleetautoscaler.go

@@ -429,7 +429,7 @@ func (c *CounterPolicy) ValidateCounterPolicy(causes []metav1.StatusCause) []met
 			causes = append(causes, metav1.StatusCause{
 				Type:    metav1.CauseTypeFieldValueInvalid,
 				Field:   "bufferSize",
-				Message: "bufferSize must be bigger than 0",
+				Message: "bufferSize must be bigger than 0", // TODO: Should we allow bufferSize to be Zero (in case of Scaling down to 0)? In case of BufferPercentage MaxCapacity can be set to 0, in case of BufferInt Max Capacity must be >= 2.
 			})
 		}
 		if c.MaxCapacity < int64(c.BufferSize.IntValue()) {

pkg/fleetautoscalers/controller.go

@@ -30,6 +30,7 @@ import (
 	"agones.dev/agones/pkg/client/informers/externalversions"
 	listeragonesv1 "agones.dev/agones/pkg/client/listers/agones/v1"
 	listerautoscalingv1 "agones.dev/agones/pkg/client/listers/autoscaling/v1"
+	"agones.dev/agones/pkg/gameservers"
 	"agones.dev/agones/pkg/util/crd"
 	"agones.dev/agones/pkg/util/logfields"
 	"agones.dev/agones/pkg/util/runtime"
@@ -76,6 +77,7 @@ type Extensions struct {
 type Controller struct {
 	baseLogger            *logrus.Entry
 	clock                 clock.WithTickerAndDelayedExecution
+	counter               *gameservers.PerNodeCounter
 	crdGetter             apiextclientv1.CustomResourceDefinitionInterface
 	fasThreads            map[types.UID]fasThread
 	fasThreadMutex        sync.Mutex
@@ -87,6 +89,7 @@ type Controller struct {
 	fleetAutoscalerSynced cache.InformerSynced
 	workerqueue           *workerqueue.WorkerQueue
 	recorder              record.EventRecorder
+	gameServerLister      listeragonesv1.GameServerLister
 }
 
 // NewController returns a controller for a FleetAutoscaler
@@ -95,12 +98,16 @@ func NewController(
 	kubeClient kubernetes.Interface,
 	extClient extclientset.Interface,
 	agonesClient versioned.Interface,
-	agonesInformerFactory externalversions.SharedInformerFactory) *Controller {
+	agonesInformerFactory externalversions.SharedInformerFactory,
+	counter *gameservers.PerNodeCounter) *Controller {
 
 	autoscaler := agonesInformerFactory.Autoscaling().V1().FleetAutoscalers()
 	fleetInformer := agonesInformerFactory.Agones().V1().Fleets()
+	gameServers := agonesInformerFactory.Agones().V1().GameServers()
+
 	c := &Controller{
 		clock:                 clock.RealClock{},
+		counter:               counter,
 		crdGetter:             extClient.ApiextensionsV1().CustomResourceDefinitions(),
 		fasThreads:            map[types.UID]fasThread{},
 		fasThreadMutex:        sync.Mutex{},
@@ -110,6 +117,7 @@ func NewController(
 		fleetAutoscalerGetter: agonesClient.AutoscalingV1(),
 		fleetAutoscalerLister: autoscaler.Lister(),
 		fleetAutoscalerSynced: autoscaler.Informer().HasSynced,
+		gameServerLister:      gameServers.Lister(),
 	}
 	c.baseLogger = runtime.NewLoggerWithType(c)
 	c.workerqueue = workerqueue.NewWorkerQueueWithRateLimiter(c.syncFleetAutoscaler, c.baseLogger, logfields.FleetAutoscalerKey, autoscaling.GroupName+".FleetAutoscalerController", workerqueue.FastRateLimiter(3*time.Second))
@@ -326,7 +334,7 @@ func (c *Controller) syncFleetAutoscaler(ctx context.Context, key string) error
 	}
 
 	currentReplicas := fleet.Status.Replicas
-	desiredReplicas, scalingLimited, err := computeDesiredFleetSize(fas, fleet)
+	desiredReplicas, scalingLimited, err := computeDesiredFleetSize(fas, fleet, c.gameServerLister, c.counter.Counts())
 	if err != nil {
 		c.recorder.Eventf(fas, corev1.EventTypeWarning, "FleetAutoscaler",
 			"Error calculating desired fleet size on FleetAutoscaler %s. Error: %s", fas.ObjectMeta.Name, err.Error())

pkg/fleetautoscalers/controller_test.go

@@ -24,8 +24,10 @@ import (
 	"testing"
 	"time"
 
+	"agones.dev/agones/pkg/apis"
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	autoscalingv1 "agones.dev/agones/pkg/apis/autoscaling/v1"
+	"agones.dev/agones/pkg/gameservers"
 	agtesting "agones.dev/agones/pkg/testing"
 	utilruntime "agones.dev/agones/pkg/util/runtime"
 	"agones.dev/agones/pkg/util/webhooks"
@@ -693,7 +695,7 @@ func TestControllerSyncFleetAutoscaler(t *testing.T) {
 
 		err := c.syncFleetAutoscaler(ctx, "default/fas-1")
 		if assert.NotNil(t, err) {
-			assert.Equal(t, "error calculating autoscaling fleet: fleet-1: wrong policy type, should be one of: Buffer, Webhook", err.Error())
+			assert.Equal(t, "error calculating autoscaling fleet: fleet-1: wrong policy type, should be one of: Buffer, Webhook, Counter, List", err.Error())
 		}
 	})
 
@@ -1189,7 +1191,11 @@ func defaultFixtures() (*autoscalingv1.FleetAutoscaler, *agonesv1.Fleet) {
 		},
 		Spec: agonesv1.FleetSpec{
 			Replicas: 8,
-			Template: agonesv1.GameServerTemplateSpec{},
+			Template: agonesv1.GameServerTemplateSpec{
+				Spec: agonesv1.GameServerSpec{
+					Scheduling: apis.Packed,
+				},
+			},
 		},
 		Status: agonesv1.FleetStatus{
 			Replicas:          5,
@@ -1245,7 +1251,8 @@ func defaultWebhookFixtures() (*autoscalingv1.FleetAutoscaler, *agonesv1.Fleet)
 // newFakeController returns a controller, backed by the fake Clientset
 func newFakeController() (*Controller, agtesting.Mocks) {
 	m := agtesting.NewMocks()
-	c := NewController(healthcheck.NewHandler(), m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory)
+	counter := gameservers.NewPerNodeCounter(m.KubeInformerFactory, m.AgonesInformerFactory)
+	c := NewController(healthcheck.NewHandler(), m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory, counter)
 	c.recorder = m.FakeRecorder
 	return c, m
 }