Skip to content

Commit

Permalink
[Regression] Fleet scale down didn't adhere to Packed Scheduling
Browse files Browse the repository at this point in the history
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.

This change implements a library for GameServer sorting that doesn't
use reflection, unlike sort.Slice (much faster), and can be used
with any Comparator, as I expect we'll likely need this again.

This also pulls out the GameServerCounter controller - which keeps
a running total of GameServers per Node, to make this kind of sorting much
faster and easier as we don't have to compute this on every iteration.

To maintain performance as well, the set of GameServers only get sorted when
scaling down, and only for Packed strategy.
  • Loading branch information
markmandel committed Mar 6, 2019
1 parent 0b9af68 commit 406f1b6
Show file tree
Hide file tree
Showing 13 changed files with 263 additions and 211 deletions.
10 changes: 6 additions & 4 deletions cmd/controller/main.go
Expand Up @@ -181,22 +181,24 @@ func main() {

allocationMutex := &sync.Mutex{}

gsCounter := gameservers.NewGameServerCounter(kubeInformerFactory, agonesInformerFactory)

gsController := gameservers.NewController(wh, health,
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,
kubeClient, extClient, agonesClient, agonesInformerFactory)
gasController := gameserverallocations.NewController(wh, health, kubeClient,
kubeInformerFactory, extClient, agonesClient, agonesInformerFactory, topNGSForAllocation)
gasController := gameserverallocations.NewController(wh, health, gsCounter, topNGSForAllocation,
kubeClient, extClient, agonesClient, agonesInformerFactory)
fasController := fleetautoscalers.NewController(wh, health,
kubeClient, extClient, agonesClient, agonesInformerFactory)

rs = append(rs,
wh, gsController, gsSetController, fleetController, faController, fasController, gasController, server)
wh, gsCounter, gsController, gsSetController, fleetController, faController, fasController, gasController, server)

stop := signals.NewStopChannel()

Expand Down
52 changes: 52 additions & 0 deletions pkg/apis/stable/v1alpha1/gameserversort.go
@@ -0,0 +1,52 @@
// Copyright 2019 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 v1alpha1

import "sort"

// GameServerSort sorts gameservers, based on a comparator
type GameServerSort struct {
// List is the GameServer list to sort
List []*GameServer
// Less is the comparator function to use as a sorting function.
// returns true if a < b
Comparator func(a, b *GameServer) bool
}

// Sort sorts the underlying Array and returns it
func (g *GameServerSort) Sort() []*GameServer {
sort.Sort(g)
return g.List
}

// Len is the Len function for sort.Interface
func (g *GameServerSort) Len() int {
return len(g.List)
}

// Less is the Less function for sort.Interface
func (g *GameServerSort) Less(i, j int) bool {
a := g.List[i]
b := g.List[j]

return g.Comparator(a, b)
}

// Swap is the swap function for sort.Interface
func (g *GameServerSort) Swap(i, j int) {
tmp := g.List[j]
g.List[j] = g.List[i]
g.List[i] = tmp
}
37 changes: 37 additions & 0 deletions pkg/apis/stable/v1alpha1/gameserversort_test.go
@@ -0,0 +1,37 @@
// Copyright 2019 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 v1alpha1

import (
"testing"

"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestGameServerSortSort(t *testing.T) {
list := []*GameServer{{ObjectMeta: metav1.ObjectMeta{Name: "c"}},
{ObjectMeta: metav1.ObjectMeta{Name: "b"}}, {ObjectMeta: metav1.ObjectMeta{Name: "a"}}}

sort := GameServerSort{list, func(a, b *GameServer) bool {
return a.ObjectMeta.Name < b.ObjectMeta.Name
}}

result := sort.Sort()
assert.Len(t, result, len(list))
assert.Equal(t, "a", result[0].ObjectMeta.Name)
assert.Equal(t, "b", result[1].ObjectMeta.Name)
assert.Equal(t, "c", result[2].ObjectMeta.Name)
}
32 changes: 13 additions & 19 deletions pkg/gameserverallocations/controller.go
Expand Up @@ -27,6 +27,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"
Expand All @@ -43,7 +44,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
Expand All @@ -61,8 +61,12 @@ var (

// Controller is a the GameServerAllocation controller
type Controller struct {
baseLogger *logrus.Entry
counter *AllocationCounter
baseLogger *logrus.Entry
counter *gameservers.GameServerCounter
readyGameServers gameServerCacheEntry
// Instead of selecting the top one, controller selects a random one
// from the topNGameServerCount of Ready gameservers
topNGameServerCount int
crdGetter v1beta1.CustomResourceDefinitionInterface
gameServerSynced cache.InformerSynced
gameServerGetter getterv1alpha1.GameServersGetter
Expand All @@ -73,10 +77,6 @@ type Controller struct {
workerqueue *workerqueue.WorkerQueue
gsWorkerqueue *workerqueue.WorkerQueue
recorder record.EventRecorder
readyGameServers gameServerCacheEntry
// Instead of selecting the top one, controller selects a random one
// from the topNGameServerCount of Ready gameservers
topNGameServerCount int
}

// gameserver cache to keep the Ready state gameserver.
Expand Down Expand Up @@ -133,7 +133,7 @@ func (e *gameServerCacheEntry) Range(f func(key string, gs *v1alpha1.GameServer)
// findComparator is a comparator function specifically for the
// findReadyGameServerForAllocation method for determining
// scheduling strategy
type findComparator func(bestCount, currentCount NodeCount) bool
type findComparator func(bestCount, currentCount gameservers.NodeCount) bool

var allocationRetry = wait.Backoff{
Steps: 5,
Expand All @@ -145,24 +145,24 @@ var allocationRetry = wait.Backoff{
// NewController returns a controller for a GameServerAllocation
func NewController(wh *webhooks.WebHook,
health healthcheck.Handler,
counter *gameservers.GameServerCounter,
topNGameServerCnt int,
kubeClient kubernetes.Interface,
kubeInformerFactory informers.SharedInformerFactory,
extClient extclientset.Interface,
agonesClient versioned.Interface,
agonesInformerFactory externalversions.SharedInformerFactory,
topNGameServerCnt int,
) *Controller {

agonesInformer := agonesInformerFactory.Stable().V1alpha1()
c := &Controller{
counter: NewAllocationCounter(kubeInformerFactory, agonesInformerFactory),
counter: counter,
topNGameServerCount: topNGameServerCnt,
crdGetter: extClient.ApiextensionsV1beta1().CustomResourceDefinitions(),
gameServerSynced: agonesInformer.GameServers().Informer().HasSynced,
gameServerGetter: agonesClient.StableV1alpha1(),
gameServerLister: agonesInformer.GameServers().Lister(),
gameServerAllocationSynced: agonesInformer.GameServerAllocations().Informer().HasSynced,
gameServerAllocationGetter: agonesClient.StableV1alpha1(),
topNGameServerCount: topNGameServerCnt,
}
c.baseLogger = runtime.NewLoggerWithType(c)
c.workerqueue = workerqueue.NewWorkerQueue(c.syncDelete, c.baseLogger, logfields.GameServerAllocationKey, stable.GroupName+".GameServerAllocationController")
Expand Down Expand Up @@ -217,13 +217,7 @@ func (c *Controller) Run(workers int, stop <-chan struct{}) error {
return err
}

err = c.counter.Run(stop)
if err != nil {
return err
}

c.stop = stop

c.baseLogger.Info("Wait for cache sync")
if !cache.WaitForCacheSync(stop, c.gameServerAllocationSynced, c.gameServerSynced) {
return errors.New("failed to wait for caches to sync")
Expand Down Expand Up @@ -444,7 +438,7 @@ func (c *Controller) syncDelete(key string) error {
// preferred selectors, as well as the passed in comparator
func (c *Controller) findReadyGameServerForAllocation(gsa *v1alpha1.GameServerAllocation, comparator findComparator) (*v1alpha1.GameServer, error) {
// track the best node count
var bestCount *NodeCount
var bestCount *gameservers.NodeCount
// the current GameServer from the node with the most GameServers (allocated, ready)
var bestGS *v1alpha1.GameServer

Expand Down
24 changes: 16 additions & 8 deletions pkg/gameserverallocations/controller_test.go
Expand Up @@ -23,6 +23,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"
applypatch "github.com/evanphx/json-patch"
Expand All @@ -37,6 +38,12 @@ import (
"k8s.io/client-go/tools/cache"
)

const (
defaultNs = "default"
n1 = "node1"
n2 = "node2"
)

var (
gvk = metav1.GroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("GameServerAllocation"))
)
Expand Down Expand Up @@ -185,7 +192,7 @@ func TestControllerAllocate(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

gsa := v1alpha1.GameServerAllocation{ObjectMeta: metav1.ObjectMeta{Name: "gsa-1"},
Expand Down Expand Up @@ -262,7 +269,7 @@ func TestControllerAllocatePriority(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

gas := &v1alpha1.GameServerAllocation{ObjectMeta: metav1.ObjectMeta{Name: "fa-1"},
Expand Down Expand Up @@ -350,7 +357,7 @@ func TestControllerWithoutAllocateMutex(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

wg := sync.WaitGroup{}
Expand Down Expand Up @@ -416,7 +423,7 @@ func TestControllerFindPackedReadyGameServer(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

gs, err := c.findReadyGameServerForAllocation(gsa, packedComparator)
Expand Down Expand Up @@ -478,7 +485,7 @@ func TestControllerFindPackedReadyGameServer(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

gs, err := c.findReadyGameServerForAllocation(prefGsa, packedComparator)
Expand Down Expand Up @@ -543,7 +550,7 @@ func TestControllerFindPackedReadyGameServer(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

gs, err := c.findReadyGameServerForAllocation(gsa, packedComparator)
Expand Down Expand Up @@ -600,7 +607,7 @@ func TestControllerFindDistributedReadyGameServer(t *testing.T) {
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(stop)
err = c.counter.Run(0, stop)
assert.Nil(t, err)

gs, err := c.findReadyGameServerForAllocation(gsa, distributedComparator)
Expand Down Expand Up @@ -807,7 +814,8 @@ func defaultFixtures(gsLen int) (*v1alpha1.Fleet, *v1alpha1.GameServerSet, []v1a
func newFakeController() (*Controller, agtesting.Mocks) {
m := agtesting.NewMocks()
wh := webhooks.NewWebHook("", "")
c := NewController(wh, healthcheck.NewHandler(), m.KubeClient, m.KubeInformerFactory, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory, 1)
counter := gameservers.NewGameServerCounter(m.KubeInformerFactory, m.AgonesInformerFactory)
c := NewController(wh, healthcheck.NewHandler(), counter, 1, m.KubeClient, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory)
c.recorder = m.FakeRecorder
return c, m
}
Expand Down
10 changes: 6 additions & 4 deletions pkg/gameserverallocations/find.go
Expand Up @@ -14,12 +14,14 @@

package gameserverallocations

import "agones.dev/agones/pkg/gameservers"

// packedComparator prioritises Nodes with GameServers that are allocated, and then Nodes with the most
// Ready GameServers -- this will bin pack allocated game servers together.
func packedComparator(bestCount, currentCount NodeCount) bool {
if currentCount.allocated == bestCount.allocated && currentCount.ready > bestCount.ready {
func packedComparator(bestCount, currentCount gameservers.NodeCount) bool {
if currentCount.Allocated == bestCount.Allocated && currentCount.Ready > bestCount.Ready {
return true
} else if currentCount.allocated > bestCount.allocated {
} else if currentCount.Allocated > bestCount.Allocated {
return true
}

Expand All @@ -28,6 +30,6 @@ func packedComparator(bestCount, currentCount NodeCount) bool {

// distributedComparator is the inverse of the packed comparator,
// looking to distribute allocated gameservers on as many nodes as possible.
func distributedComparator(bestCount, currentCount NodeCount) bool {
func distributedComparator(bestCount, currentCount gameservers.NodeCount) bool {
return !packedComparator(bestCount, currentCount)
}
21 changes: 0 additions & 21 deletions pkg/gameserverallocations/helper_test.go

This file was deleted.

0 comments on commit 406f1b6

Please sign in to comment.