diff --git a/cmd/controller/main.go b/cmd/controller/main.go index c7b1105848..bde2ded41b 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -190,7 +190,7 @@ func main() { ctlConf.MinPort, ctlConf.MaxPort, ctlConf.SidecarImage, ctlConf.AlwaysPullSidecar, ctlConf.SidecarCPURequest, ctlConf.SidecarCPULimit, kubeClient, kubeInformerFactory, extClient, agonesClient, agonesInformerFactory) - gsSetController := gameserversets.NewController(wh, health, + gsSetController := gameserversets.NewController(wh, health, gsCounter, kubeClient, extClient, agonesClient, agonesInformerFactory) fleetController := fleets.NewController(wh, health, kubeClient, extClient, agonesClient, agonesInformerFactory) faController := fleetallocation.NewController(wh, allocationMutex, diff --git a/pkg/gameserverallocations/controller_test.go b/pkg/gameserverallocations/controller_test.go index d2a4f7322e..36a350b77d 100644 --- a/pkg/gameserverallocations/controller_test.go +++ b/pkg/gameserverallocations/controller_test.go @@ -39,6 +39,12 @@ import ( "k8s.io/client-go/tools/cache" ) +const ( + defaultNs = "default" + n1 = "node1" + n2 = "node2" +) + var ( gvk = metav1.GroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("GameServerAllocation")) ) diff --git a/pkg/gameserverallocations/helper_test.go b/pkg/gameserverallocations/helper_test.go deleted file mode 100644 index 275c2a144d..0000000000 --- a/pkg/gameserverallocations/helper_test.go +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright 2018 Google Inc. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package gameserverallocations - -const ( - defaultNs = "default" - n1 = "node1" - n2 = "node2" -) diff --git a/pkg/gameservers/prenodecounter.go b/pkg/gameservers/pernodecounter.go similarity index 100% rename from pkg/gameservers/prenodecounter.go rename to pkg/gameservers/pernodecounter.go diff --git a/pkg/gameservers/prenodecounter_test.go b/pkg/gameservers/pernodecounter_test.go similarity index 100% rename from pkg/gameservers/prenodecounter_test.go rename to pkg/gameservers/pernodecounter_test.go diff --git a/pkg/gameserversets/controller.go b/pkg/gameserversets/controller.go index 3b74adcd61..ae28754299 100644 --- a/pkg/gameserversets/controller.go +++ b/pkg/gameserversets/controller.go @@ -16,7 +16,6 @@ package gameserversets import ( "encoding/json" - "sort" "sync" "agones.dev/agones/pkg/apis/stable" @@ -25,6 +24,7 @@ import ( getterv1alpha1 "agones.dev/agones/pkg/client/clientset/versioned/typed/stable/v1alpha1" "agones.dev/agones/pkg/client/informers/externalversions" listerv1alpha1 "agones.dev/agones/pkg/client/listers/stable/v1alpha1" + "agones.dev/agones/pkg/gameservers" "agones.dev/agones/pkg/util/crd" "agones.dev/agones/pkg/util/logfields" "agones.dev/agones/pkg/util/runtime" @@ -66,6 +66,7 @@ const ( // Controller is a the GameServerSet controller type Controller struct { baseLogger *logrus.Entry + counter *gameservers.PerNodeCounter crdGetter v1beta1.CustomResourceDefinitionInterface gameServerGetter getterv1alpha1.GameServersGetter gameServerLister listerv1alpha1.GameServerLister @@ -83,6 +84,7 @@ type Controller struct { func NewController( wh *webhooks.WebHook, health healthcheck.Handler, + counter *gameservers.PerNodeCounter, kubeClient kubernetes.Interface, extClient extclientset.Interface, agonesClient versioned.Interface, @@ -95,6 +97,7 @@ func NewController( c := &Controller{ crdGetter: extClient.ApiextensionsV1beta1().CustomResourceDefinitions(), + counter: counter, gameServerGetter: agonesClient.StableV1alpha1(), gameServerLister: gameServers.Lister(), gameServerSynced: gsInformer.HasSynced, @@ -305,7 +308,8 @@ func (c *Controller) syncGameServerSet(key string) error { list = c.stateCache.forGameServerSet(gsSet).reconcileWithUpdatedServerList(list) - numServersToAdd, toDelete, isPartial := computeReconciliationAction(list, int(gsSet.Spec.Replicas), maxGameServerCreationsPerBatch, maxGameServerDeletionsPerBatch, maxPodPendingCount) + numServersToAdd, toDelete, isPartial := computeReconciliationAction(gsSet.Spec.Scheduling, list, c.counter.Counts(), + int(gsSet.Spec.Replicas), maxGameServerCreationsPerBatch, maxGameServerDeletionsPerBatch, maxPodPendingCount) status := computeStatus(list) fields := logrus.Fields{} @@ -353,33 +357,39 @@ func (c *Controller) syncGameServerSet(key string) error { // computeReconciliationAction computes the action to take to reconcile a game server set set given // the list of game servers that were found and target replica count. -func computeReconciliationAction(list []*v1alpha1.GameServer, targetReplicaCount int, maxCreations int, maxDeletions int, maxPending int) (int, []*v1alpha1.GameServer, bool) { - var upCount int // up == Ready or will become ready +func computeReconciliationAction(strategy v1alpha1.SchedulingStrategy, list []*v1alpha1.GameServer, + counts map[string]gameservers.NodeCount, targetReplicaCount int, maxCreations int, maxDeletions int, + maxPending int) (int, []*v1alpha1.GameServer, bool) { + var upCount int // up == Ready or will become ready + var deleteCount int // number of gameservers to delete // track the number of pods that are being created at any given moment by the GameServerSet // so we can limit it at a throughput that Kubernetes can handle var podPendingCount int // podPending == "up" but don't have a Pod running yet + + var potentialDeletions []*v1alpha1.GameServer var toDelete []*v1alpha1.GameServer scheduleDeletion := func(gs *v1alpha1.GameServer) { - if gs.ObjectMeta.DeletionTimestamp.IsZero() { - toDelete = append(toDelete, gs) - } + toDelete = append(toDelete, gs) + deleteCount-- } handleGameServerUp := func(gs *v1alpha1.GameServer) { if upCount >= targetReplicaCount { - scheduleDeletion(gs) + deleteCount++ } else { upCount++ } + + // Track gameservers that could be potentially deleted + potentialDeletions = append(potentialDeletions, gs) } // pass 1 - count allocated servers only, since those can't be touched for _, gs := range list { if gs.IsAllocated() { upCount++ - continue } } @@ -446,12 +456,17 @@ func computeReconciliationAction(list []*v1alpha1.GameServer, targetReplicaCount } } - if len(toDelete) > maxDeletions { - // we have to pick which GS to delete, let's delete the newest ones first. - sort.Slice(toDelete, func(i, j int) bool { - return toDelete[i].ObjectMeta.CreationTimestamp.After(toDelete[j].ObjectMeta.CreationTimestamp.Time) - }) + if deleteCount > 0 { + if strategy == v1alpha1.Packed { + potentialDeletions = sortGameServersByLeastFullNodes(potentialDeletions, counts) + } else { + potentialDeletions = sortGameServersByNewFirst(potentialDeletions) + } + + toDelete = append(toDelete, potentialDeletions[0:deleteCount]...) + } + if deleteCount > maxDeletions { toDelete = toDelete[0:maxDeletions] partialReconciliation = true } diff --git a/pkg/gameserversets/controller_test.go b/pkg/gameserversets/controller_test.go index 580bfbe094..93b66a7962 100644 --- a/pkg/gameserversets/controller_test.go +++ b/pkg/gameserversets/controller_test.go @@ -22,6 +22,7 @@ import ( "time" "agones.dev/agones/pkg/apis/stable/v1alpha1" + "agones.dev/agones/pkg/gameservers" agtesting "agones.dev/agones/pkg/testing" "agones.dev/agones/pkg/util/webhooks" "github.com/heptiolabs/healthcheck" @@ -159,13 +160,59 @@ func TestComputeReconciliationAction(t *testing.T) { for _, tc := range cases { t.Run(tc.desc, func(t *testing.T) { - toAdd, toDelete, isPartial := computeReconciliationAction(tc.list, tc.targetReplicaCount, maxTestCreationsPerBatch, maxTestDeletionsPerBatch, maxTestPendingPerBatch) + toAdd, toDelete, isPartial := computeReconciliationAction(v1alpha1.Distributed, tc.list, map[string]gameservers.NodeCount{}, + tc.targetReplicaCount, maxTestCreationsPerBatch, maxTestDeletionsPerBatch, maxTestPendingPerBatch) assert.Equal(t, tc.wantNumServersToAdd, toAdd, "# of GameServers to add") assert.Len(t, toDelete, tc.wantNumServersToDelete, "# of GameServers to delete") assert.Equal(t, tc.wantIsPartial, isPartial, "is partial reconciliation") }) } + + t.Run("test packed scale down", func(t *testing.T) { + list := []*v1alpha1.GameServer{ + {ObjectMeta: metav1.ObjectMeta{Name: "gs1"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: "node3"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "gs2"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: "node1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "gs3"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: "node3"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "gs4"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: ""}}, + } + + counts := map[string]gameservers.NodeCount{"node1": {Ready: 1}, "node3": {Ready: 2}} + toAdd, toDelete, isPartial := computeReconciliationAction(v1alpha1.Packed, list, counts, 2, + 1000, 1000, 1000) + + assert.Empty(t, toAdd) + assert.False(t, isPartial, "shouldn't be partial") + + assert.Len(t, toDelete, 2) + assert.Equal(t, "gs4", toDelete[0].ObjectMeta.Name) + assert.Equal(t, "gs2", toDelete[1].ObjectMeta.Name) + }) + + t.Run("test distributed scale down", func(t *testing.T) { + now := metav1.Now() + + list := []*v1alpha1.GameServer{ + {ObjectMeta: metav1.ObjectMeta{Name: "gs1", + CreationTimestamp: metav1.Time{Time: now.Add(10 * time.Second)}}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}}, + {ObjectMeta: metav1.ObjectMeta{Name: "gs2", + CreationTimestamp: now}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}}, + {ObjectMeta: metav1.ObjectMeta{Name: "gs3", + CreationTimestamp: metav1.Time{Time: now.Add(40 * time.Second)}}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}}, + {ObjectMeta: metav1.ObjectMeta{Name: "gs4", + CreationTimestamp: metav1.Time{Time: now.Add(30 * time.Second)}}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}}, + } + + toAdd, toDelete, isPartial := computeReconciliationAction(v1alpha1.Distributed, list, map[string]gameservers.NodeCount{}, + 2, 1000, 1000, 1000) + + assert.Empty(t, toAdd) + assert.False(t, isPartial, "shouldn't be partial") + + assert.Len(t, toDelete, 2) + assert.Equal(t, "gs2", toDelete[0].ObjectMeta.Name) + assert.Equal(t, "gs1", toDelete[1].ObjectMeta.Name) + }) } func TestComputeStatus(t *testing.T) { @@ -573,7 +620,8 @@ func createGameServers(gsSet *v1alpha1.GameServerSet, size int) []v1alpha1.GameS func newFakeController() (*Controller, agtesting.Mocks) { m := agtesting.NewMocks() wh := webhooks.NewWebHook(http.NewServeMux()) - c := NewController(wh, healthcheck.NewHandler(), m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory) + counter := gameservers.NewPerNodeCounter(m.KubeInformerFactory, m.AgonesInformerFactory) + c := NewController(wh, healthcheck.NewHandler(), counter, m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory) c.recorder = m.FakeRecorder return c, m } diff --git a/pkg/gameserversets/gameserversets.go b/pkg/gameserversets/gameserversets.go index effc52965f..64463b9b2d 100644 --- a/pkg/gameserversets/gameserversets.go +++ b/pkg/gameserversets/gameserversets.go @@ -19,66 +19,44 @@ import ( "agones.dev/agones/pkg/apis/stable/v1alpha1" listerv1alpha1 "agones.dev/agones/pkg/client/listers/stable/v1alpha1" + "agones.dev/agones/pkg/gameservers" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" ) -// node is just a convenience data structure for -// keeping relevant GameServer information about Nodes -type node struct { - name string - total int64 - ready []*v1alpha1.GameServer -} - -// filterGameServersOnLeastFullNodes returns a limited list of GameServers, ordered by the nodes -// they are hosted on, with the least utilised Nodes being prioritised -func filterGameServersOnLeastFullNodes(gsList []*v1alpha1.GameServer, limit int32) []*v1alpha1.GameServer { - if limit <= 0 { - return nil - } - - nodeMap := map[string]*node{} - var nodeList []*node - - // count up the number of allocated and ready game servers that exist - // also, since we're already looping through, track all the deletable GameServers - // per node, so we can use this as a shortlist to delete from - for _, gs := range gsList { - if gs.DeletionTimestamp.IsZero() && - (gs.Status.State == v1alpha1.GameServerStateAllocated || gs.Status.State == v1alpha1.GameServerStateReady) { - _, ok := nodeMap[gs.Status.NodeName] - if !ok { - node := &node{name: gs.Status.NodeName} - nodeMap[gs.Status.NodeName] = node - nodeList = append(nodeList, node) - } +// sortGameServersByLeastFullNodes sorts the list of gameservers by which gameservers reside on the least full nodes +func sortGameServersByLeastFullNodes(list []*v1alpha1.GameServer, count map[string]gameservers.NodeCount) []*v1alpha1.GameServer { + sort.Slice(list, func(i, j int) bool { + a := list[i] + b := list[j] + // not scheduled yet/node deleted, put them first + ac, ok := count[a.Status.NodeName] + if !ok { + return true + } - nodeMap[gs.Status.NodeName].total++ - if gs.Status.State == v1alpha1.GameServerStateReady { - nodeMap[gs.Status.NodeName].ready = append(nodeMap[gs.Status.NodeName].ready, gs) - } + bc, ok := count[b.Status.NodeName] + if !ok { + return false } - } - // sort our nodes, least to most - sort.Slice(nodeList, func(i, j int) bool { - return nodeList[i].total < nodeList[j].total + return (ac.Allocated + ac.Ready) < (bc.Allocated + bc.Ready) }) - // we need to get Ready GameServer until we equal or pass limit - result := make([]*v1alpha1.GameServer, 0, limit) + return list +} - for _, n := range nodeList { - result = append(result, n.ready...) +// sortGameServersByNewFirst sorts by newest gameservers first, and returns them +func sortGameServersByNewFirst(list []*v1alpha1.GameServer) []*v1alpha1.GameServer { + sort.Slice(list, func(i, j int) bool { + a := list[i] + b := list[j] - if int32(len(result)) >= limit { - return result - } - } + return a.ObjectMeta.CreationTimestamp.Before(&b.ObjectMeta.CreationTimestamp) + }) - return result + return list } // ListGameServersByGameServerSetOwner lists the GameServers for a given GameServerSet diff --git a/pkg/gameserversets/gameserversets_test.go b/pkg/gameserversets/gameserversets_test.go index 58487e32f7..712ff7389b 100644 --- a/pkg/gameserversets/gameserversets_test.go +++ b/pkg/gameserversets/gameserversets_test.go @@ -17,8 +17,10 @@ package gameserversets import ( "sort" "testing" + "time" "agones.dev/agones/pkg/apis/stable/v1alpha1" + "agones.dev/agones/pkg/gameservers" agtesting "agones.dev/agones/pkg/testing" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,41 +28,43 @@ import ( k8stesting "k8s.io/client-go/testing" ) -func TestFilterGameServersOnLeastFullNodes(t *testing.T) { +func TestSortGameServersByLeastFullNodes(t *testing.T) { t.Parallel() - gsList := []*v1alpha1.GameServer{ - {ObjectMeta: metav1.ObjectMeta{Name: "gs1"}, Status: v1alpha1.GameServerStatus{NodeName: "n1", State: v1alpha1.GameServerStateReady}}, - {ObjectMeta: metav1.ObjectMeta{Name: "gs2"}, Status: v1alpha1.GameServerStatus{NodeName: "n1", State: v1alpha1.GameServerStateStarting}}, - {ObjectMeta: metav1.ObjectMeta{Name: "gs3"}, Status: v1alpha1.GameServerStatus{NodeName: "n2", State: v1alpha1.GameServerStateReady}}, - {ObjectMeta: metav1.ObjectMeta{Name: "gs4"}, Status: v1alpha1.GameServerStatus{NodeName: "n2", State: v1alpha1.GameServerStateReady}}, - {ObjectMeta: metav1.ObjectMeta{Name: "gs5"}, Status: v1alpha1.GameServerStatus{NodeName: "n3", State: v1alpha1.GameServerStateAllocated}}, - {ObjectMeta: metav1.ObjectMeta{Name: "gs6"}, Status: v1alpha1.GameServerStatus{NodeName: "n3", State: v1alpha1.GameServerStateAllocated}}, - {ObjectMeta: metav1.ObjectMeta{Name: "gs7"}, Status: v1alpha1.GameServerStatus{NodeName: "n3", State: v1alpha1.GameServerStateReady}}, + nc := map[string]gameservers.NodeCount{ + "n1": {Ready: 1, Allocated: 0}, + "n2": {Ready: 0, Allocated: 2}, } - t.Run("normal", func(t *testing.T) { - limit := 4 - result := filterGameServersOnLeastFullNodes(gsList, int32(limit)) - assert.Len(t, result, limit) - assert.Equal(t, "gs1", result[0].Name) - assert.Equal(t, "n2", result[1].Status.NodeName) - assert.Equal(t, "n2", result[2].Status.NodeName) - assert.Equal(t, "n3", result[3].Status.NodeName) - }) + list := []*v1alpha1.GameServer{ + {ObjectMeta: metav1.ObjectMeta{Name: "g1"}, Status: v1alpha1.GameServerStatus{NodeName: "n2"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "g2"}, Status: v1alpha1.GameServerStatus{NodeName: ""}}, + {ObjectMeta: metav1.ObjectMeta{Name: "g3"}, Status: v1alpha1.GameServerStatus{NodeName: "n1"}}, + } - t.Run("zero", func(t *testing.T) { - limit := 0 - result := filterGameServersOnLeastFullNodes(gsList, int32(limit)) - assert.Len(t, result, limit) - }) + result := sortGameServersByLeastFullNodes(list, nc) - t.Run("negative", func(t *testing.T) { - limit := -1 - result := filterGameServersOnLeastFullNodes(gsList, int32(limit)) - assert.Len(t, result, 0) - assert.Empty(t, result) - }) + assert.Len(t, result, len(list)) + assert.Equal(t, "g2", result[0].ObjectMeta.Name) + assert.Equal(t, "g3", result[1].ObjectMeta.Name) + assert.Equal(t, "g1", result[2].ObjectMeta.Name) +} + +func TestSortGameServersByNewFirst(t *testing.T) { + now := metav1.Now() + + list := []*v1alpha1.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)}}}, + } + l := len(list) + + result := sortGameServersByNewFirst(list) + assert.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) } func TestListGameServersByGameServerSetOwner(t *testing.T) {