Skip to content

Commit 49d475a

Browse files
markmandelmarkmandel
committed
[Regression] Fleet scale down didn't adhere to Packed Scheduling
The new Fleet scale up/scale down performance enhancements removed the functionality that the `Packed` strategy is documented to provide -- when scaling down, removing GameServers from least used Nodes first. To maintain performance as well, the set of GameServers only get sorted when scaling down, and only for Packed strategy.
1 parent cf49d9c commit 49d475a

9 files changed

Lines changed: 144 additions & 114 deletions

File tree

‎cmd/controller/main.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func main() {
190190
ctlConf.MinPort, ctlConf.MaxPort, ctlConf.SidecarImage, ctlConf.AlwaysPullSidecar,
191191
ctlConf.SidecarCPURequest, ctlConf.SidecarCPULimit,
192192
kubeClient, kubeInformerFactory, extClient, agonesClient, agonesInformerFactory)
193-
gsSetController := gameserversets.NewController(wh, health,
193+
gsSetController := gameserversets.NewController(wh, health, gsCounter,
194194
kubeClient, extClient, agonesClient, agonesInformerFactory)
195195
fleetController := fleets.NewController(wh, health, kubeClient, extClient, agonesClient, agonesInformerFactory)
196196
faController := fleetallocation.NewController(wh, allocationMutex,

‎pkg/gameserverallocations/controller_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ import (
3939
"k8s.io/client-go/tools/cache"
4040
)
4141

42+
const (
43+
defaultNs = "default"
44+
n1 = "node1"
45+
n2 = "node2"
46+
)
47+
4248
var (
4349
gvk = metav1.GroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("GameServerAllocation"))
4450
)

‎pkg/gameserverallocations/helper_test.go‎

Lines changed: 0 additions & 21 deletions
This file was deleted.

pkg/gameservers/prenodecounter.go renamed to pkg/gameservers/pernodecounter.go

File renamed without changes.

pkg/gameservers/prenodecounter_test.go renamed to pkg/gameservers/pernodecounter_test.go

File renamed without changes.

‎pkg/gameserversets/controller.go‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ package gameserversets
1616

1717
import (
1818
"encoding/json"
19-
"sort"
2019
"sync"
2120

2221
"agones.dev/agones/pkg/apis/stable"
@@ -25,6 +24,7 @@ import (
2524
getterv1alpha1 "agones.dev/agones/pkg/client/clientset/versioned/typed/stable/v1alpha1"
2625
"agones.dev/agones/pkg/client/informers/externalversions"
2726
listerv1alpha1 "agones.dev/agones/pkg/client/listers/stable/v1alpha1"
27+
"agones.dev/agones/pkg/gameservers"
2828
"agones.dev/agones/pkg/util/crd"
2929
"agones.dev/agones/pkg/util/logfields"
3030
"agones.dev/agones/pkg/util/runtime"
@@ -66,6 +66,7 @@ const (
6666
// Controller is a the GameServerSet controller
6767
type Controller struct {
6868
baseLogger *logrus.Entry
69+
counter *gameservers.PerNodeCounter
6970
crdGetter v1beta1.CustomResourceDefinitionInterface
7071
gameServerGetter getterv1alpha1.GameServersGetter
7172
gameServerLister listerv1alpha1.GameServerLister
@@ -83,6 +84,7 @@ type Controller struct {
8384
func NewController(
8485
wh *webhooks.WebHook,
8586
health healthcheck.Handler,
87+
counter *gameservers.PerNodeCounter,
8688
kubeClient kubernetes.Interface,
8789
extClient extclientset.Interface,
8890
agonesClient versioned.Interface,
@@ -95,6 +97,7 @@ func NewController(
9597

9698
c := &Controller{
9799
crdGetter: extClient.ApiextensionsV1beta1().CustomResourceDefinitions(),
100+
counter: counter,
98101
gameServerGetter: agonesClient.StableV1alpha1(),
99102
gameServerLister: gameServers.Lister(),
100103
gameServerSynced: gsInformer.HasSynced,
@@ -305,7 +308,8 @@ func (c *Controller) syncGameServerSet(key string) error {
305308

306309
list = c.stateCache.forGameServerSet(gsSet).reconcileWithUpdatedServerList(list)
307310

308-
numServersToAdd, toDelete, isPartial := computeReconciliationAction(list, int(gsSet.Spec.Replicas), maxGameServerCreationsPerBatch, maxGameServerDeletionsPerBatch, maxPodPendingCount)
311+
numServersToAdd, toDelete, isPartial := computeReconciliationAction(gsSet.Spec.Scheduling, list, c.counter.Counts(),
312+
int(gsSet.Spec.Replicas), maxGameServerCreationsPerBatch, maxGameServerDeletionsPerBatch, maxPodPendingCount)
309313
status := computeStatus(list)
310314
fields := logrus.Fields{}
311315

@@ -353,33 +357,39 @@ func (c *Controller) syncGameServerSet(key string) error {
353357

354358
// computeReconciliationAction computes the action to take to reconcile a game server set set given
355359
// the list of game servers that were found and target replica count.
356-
func computeReconciliationAction(list []*v1alpha1.GameServer, targetReplicaCount int, maxCreations int, maxDeletions int, maxPending int) (int, []*v1alpha1.GameServer, bool) {
357-
var upCount int // up == Ready or will become ready
360+
func computeReconciliationAction(strategy v1alpha1.SchedulingStrategy, list []*v1alpha1.GameServer,
361+
counts map[string]gameservers.NodeCount, targetReplicaCount int, maxCreations int, maxDeletions int,
362+
maxPending int) (int, []*v1alpha1.GameServer, bool) {
363+
var upCount int // up == Ready or will become ready
364+
var deleteCount int // number of gameservers to delete
358365

359366
// track the number of pods that are being created at any given moment by the GameServerSet
360367
// so we can limit it at a throughput that Kubernetes can handle
361368
var podPendingCount int // podPending == "up" but don't have a Pod running yet
369+
370+
var potentialDeletions []*v1alpha1.GameServer
362371
var toDelete []*v1alpha1.GameServer
363372

364373
scheduleDeletion := func(gs *v1alpha1.GameServer) {
365-
if gs.ObjectMeta.DeletionTimestamp.IsZero() {
366-
toDelete = append(toDelete, gs)
367-
}
374+
toDelete = append(toDelete, gs)
375+
deleteCount--
368376
}
369377

370378
handleGameServerUp := func(gs *v1alpha1.GameServer) {
371379
if upCount >= targetReplicaCount {
372-
scheduleDeletion(gs)
380+
deleteCount++
373381
} else {
374382
upCount++
375383
}
384+
385+
// Track gameservers that could be potentially deleted
386+
potentialDeletions = append(potentialDeletions, gs)
376387
}
377388

378389
// pass 1 - count allocated servers only, since those can't be touched
379390
for _, gs := range list {
380391
if gs.IsAllocated() {
381392
upCount++
382-
continue
383393
}
384394
}
385395

@@ -446,12 +456,17 @@ func computeReconciliationAction(list []*v1alpha1.GameServer, targetReplicaCount
446456
}
447457
}
448458

449-
if len(toDelete) > maxDeletions {
450-
// we have to pick which GS to delete, let's delete the newest ones first.
451-
sort.Slice(toDelete, func(i, j int) bool {
452-
return toDelete[i].ObjectMeta.CreationTimestamp.After(toDelete[j].ObjectMeta.CreationTimestamp.Time)
453-
})
459+
if deleteCount > 0 {
460+
if strategy == v1alpha1.Packed {
461+
potentialDeletions = sortGameServersByLeastFullNodes(potentialDeletions, counts)
462+
} else {
463+
potentialDeletions = sortGameServersByNewFirst(potentialDeletions)
464+
}
465+
466+
toDelete = append(toDelete, potentialDeletions[0:deleteCount]...)
467+
}
454468

469+
if deleteCount > maxDeletions {
455470
toDelete = toDelete[0:maxDeletions]
456471
partialReconciliation = true
457472
}

‎pkg/gameserversets/controller_test.go‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"time"
2323

2424
"agones.dev/agones/pkg/apis/stable/v1alpha1"
25+
"agones.dev/agones/pkg/gameservers"
2526
agtesting "agones.dev/agones/pkg/testing"
2627
"agones.dev/agones/pkg/util/webhooks"
2728
"github.com/heptiolabs/healthcheck"
@@ -159,13 +160,59 @@ func TestComputeReconciliationAction(t *testing.T) {
159160

160161
for _, tc := range cases {
161162
t.Run(tc.desc, func(t *testing.T) {
162-
toAdd, toDelete, isPartial := computeReconciliationAction(tc.list, tc.targetReplicaCount, maxTestCreationsPerBatch, maxTestDeletionsPerBatch, maxTestPendingPerBatch)
163+
toAdd, toDelete, isPartial := computeReconciliationAction(v1alpha1.Distributed, tc.list, map[string]gameservers.NodeCount{},
164+
tc.targetReplicaCount, maxTestCreationsPerBatch, maxTestDeletionsPerBatch, maxTestPendingPerBatch)
163165

164166
assert.Equal(t, tc.wantNumServersToAdd, toAdd, "# of GameServers to add")
165167
assert.Len(t, toDelete, tc.wantNumServersToDelete, "# of GameServers to delete")
166168
assert.Equal(t, tc.wantIsPartial, isPartial, "is partial reconciliation")
167169
})
168170
}
171+
172+
t.Run("test packed scale down", func(t *testing.T) {
173+
list := []*v1alpha1.GameServer{
174+
{ObjectMeta: metav1.ObjectMeta{Name: "gs1"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: "node3"}},
175+
{ObjectMeta: metav1.ObjectMeta{Name: "gs2"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: "node1"}},
176+
{ObjectMeta: metav1.ObjectMeta{Name: "gs3"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: "node3"}},
177+
{ObjectMeta: metav1.ObjectMeta{Name: "gs4"}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady, NodeName: ""}},
178+
}
179+
180+
counts := map[string]gameservers.NodeCount{"node1": {Ready: 1}, "node3": {Ready: 2}}
181+
toAdd, toDelete, isPartial := computeReconciliationAction(v1alpha1.Packed, list, counts, 2,
182+
1000, 1000, 1000)
183+
184+
assert.Empty(t, toAdd)
185+
assert.False(t, isPartial, "shouldn't be partial")
186+
187+
assert.Len(t, toDelete, 2)
188+
assert.Equal(t, "gs4", toDelete[0].ObjectMeta.Name)
189+
assert.Equal(t, "gs2", toDelete[1].ObjectMeta.Name)
190+
})
191+
192+
t.Run("test distributed scale down", func(t *testing.T) {
193+
now := metav1.Now()
194+
195+
list := []*v1alpha1.GameServer{
196+
{ObjectMeta: metav1.ObjectMeta{Name: "gs1",
197+
CreationTimestamp: metav1.Time{Time: now.Add(10 * time.Second)}}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}},
198+
{ObjectMeta: metav1.ObjectMeta{Name: "gs2",
199+
CreationTimestamp: now}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}},
200+
{ObjectMeta: metav1.ObjectMeta{Name: "gs3",
201+
CreationTimestamp: metav1.Time{Time: now.Add(40 * time.Second)}}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}},
202+
{ObjectMeta: metav1.ObjectMeta{Name: "gs4",
203+
CreationTimestamp: metav1.Time{Time: now.Add(30 * time.Second)}}, Status: v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady}},
204+
}
205+
206+
toAdd, toDelete, isPartial := computeReconciliationAction(v1alpha1.Distributed, list, map[string]gameservers.NodeCount{},
207+
2, 1000, 1000, 1000)
208+
209+
assert.Empty(t, toAdd)
210+
assert.False(t, isPartial, "shouldn't be partial")
211+
212+
assert.Len(t, toDelete, 2)
213+
assert.Equal(t, "gs2", toDelete[0].ObjectMeta.Name)
214+
assert.Equal(t, "gs1", toDelete[1].ObjectMeta.Name)
215+
})
169216
}
170217

171218
func TestComputeStatus(t *testing.T) {
@@ -573,7 +620,8 @@ func createGameServers(gsSet *v1alpha1.GameServerSet, size int) []v1alpha1.GameS
573620
func newFakeController() (*Controller, agtesting.Mocks) {
574621
m := agtesting.NewMocks()
575622
wh := webhooks.NewWebHook(http.NewServeMux())
576-
c := NewController(wh, healthcheck.NewHandler(), m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory)
623+
counter := gameservers.NewPerNodeCounter(m.KubeInformerFactory, m.AgonesInformerFactory)
624+
c := NewController(wh, healthcheck.NewHandler(), counter, m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory)
577625
c.recorder = m.FakeRecorder
578626
return c, m
579627
}

‎pkg/gameserversets/gameserversets.go‎

Lines changed: 25 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -19,66 +19,44 @@ import (
1919

2020
"agones.dev/agones/pkg/apis/stable/v1alpha1"
2121
listerv1alpha1 "agones.dev/agones/pkg/client/listers/stable/v1alpha1"
22+
"agones.dev/agones/pkg/gameservers"
2223
"github.com/pkg/errors"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2425
"k8s.io/apimachinery/pkg/labels"
2526
)
2627

27-
// node is just a convenience data structure for
28-
// keeping relevant GameServer information about Nodes
29-
type node struct {
30-
name string
31-
total int64
32-
ready []*v1alpha1.GameServer
33-
}
34-
35-
// filterGameServersOnLeastFullNodes returns a limited list of GameServers, ordered by the nodes
36-
// they are hosted on, with the least utilised Nodes being prioritised
37-
func filterGameServersOnLeastFullNodes(gsList []*v1alpha1.GameServer, limit int32) []*v1alpha1.GameServer {
38-
if limit <= 0 {
39-
return nil
40-
}
41-
42-
nodeMap := map[string]*node{}
43-
var nodeList []*node
44-
45-
// count up the number of allocated and ready game servers that exist
46-
// also, since we're already looping through, track all the deletable GameServers
47-
// per node, so we can use this as a shortlist to delete from
48-
for _, gs := range gsList {
49-
if gs.DeletionTimestamp.IsZero() &&
50-
(gs.Status.State == v1alpha1.GameServerStateAllocated || gs.Status.State == v1alpha1.GameServerStateReady) {
51-
_, ok := nodeMap[gs.Status.NodeName]
52-
if !ok {
53-
node := &node{name: gs.Status.NodeName}
54-
nodeMap[gs.Status.NodeName] = node
55-
nodeList = append(nodeList, node)
56-
}
28+
// sortGameServersByLeastFullNodes sorts the list of gameservers by which gameservers reside on the least full nodes
29+
func sortGameServersByLeastFullNodes(list []*v1alpha1.GameServer, count map[string]gameservers.NodeCount) []*v1alpha1.GameServer {
30+
sort.Slice(list, func(i, j int) bool {
31+
a := list[i]
32+
b := list[j]
33+
// not scheduled yet/node deleted, put them first
34+
ac, ok := count[a.Status.NodeName]
35+
if !ok {
36+
return true
37+
}
5738

58-
nodeMap[gs.Status.NodeName].total++
59-
if gs.Status.State == v1alpha1.GameServerStateReady {
60-
nodeMap[gs.Status.NodeName].ready = append(nodeMap[gs.Status.NodeName].ready, gs)
61-
}
39+
bc, ok := count[b.Status.NodeName]
40+
if !ok {
41+
return false
6242
}
63-
}
6443

65-
// sort our nodes, least to most
66-
sort.Slice(nodeList, func(i, j int) bool {
67-
return nodeList[i].total < nodeList[j].total
44+
return (ac.Allocated + ac.Ready) < (bc.Allocated + bc.Ready)
6845
})
6946

70-
// we need to get Ready GameServer until we equal or pass limit
71-
result := make([]*v1alpha1.GameServer, 0, limit)
47+
return list
48+
}
7249

73-
for _, n := range nodeList {
74-
result = append(result, n.ready...)
50+
// sortGameServersByNewFirst sorts by newest gameservers first, and returns them
51+
func sortGameServersByNewFirst(list []*v1alpha1.GameServer) []*v1alpha1.GameServer {
52+
sort.Slice(list, func(i, j int) bool {
53+
a := list[i]
54+
b := list[j]
7555

76-
if int32(len(result)) >= limit {
77-
return result
78-
}
79-
}
56+
return a.ObjectMeta.CreationTimestamp.Before(&b.ObjectMeta.CreationTimestamp)
57+
})
8058

81-
return result
59+
return list
8260
}
8361

8462
// ListGameServersByGameServerSetOwner lists the GameServers for a given GameServerSet

0 commit comments

Comments
 (0)