From 61173f3c7983a15933350354503641f9c699b475 Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Mon, 8 Apr 2024 23:49:35 +0000 Subject: [PATCH 1/9] Set Minimums for DesiredCapacity and Buffer to 1 --- pkg/fleetautoscalers/fleetautoscalers.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index f1df1d8bbf..54d6dfab0f 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -321,8 +321,21 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L } // The desired TOTAL capacity based on the Aggregated Allocated Counts (see applyBufferPolicy for explanation) desiredCapacity := int64(math.Ceil(float64(aggAllocatedCount*100) / float64(100-bufferPercent))) + + // Ensures desiredCapacity is at least 1 + if desiredCapacity < 1 { + desiredCapacity = 1 + } + // Convert into a desired AVAILABLE capacity aka the buffer buffer = desiredCapacity - aggAllocatedCount + + // Ensures buffer is at least 1 if desiredCapacity calculation results in a value less than or equal to aggAllocatedCount + if buffer < 1 { + buffer = 1 + desiredCapacity = aggAllocatedCount + buffer + } + } // Current available capacity across the TOTAL fleet From 9649961ad4039bc36b4f6194aec3b8513455191a Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Tue, 9 Apr 2024 00:23:59 +0000 Subject: [PATCH 2/9] fix lint --- pkg/fleetautoscalers/fleetautoscalers.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index 54d6dfab0f..9c2198f2f5 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -333,7 +333,6 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L // Ensures buffer is at least 1 if desiredCapacity calculation results in a value less than or equal to aggAllocatedCount if buffer < 1 { buffer = 1 - desiredCapacity = aggAllocatedCount + buffer } } From 9ae00f1863d261bf3384bcdb9ce99776c661a63d Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Thu, 11 Apr 2024 02:19:49 +0000 Subject: [PATCH 3/9] Fix TestApplyListPolicy --- pkg/fleetautoscalers/fleetautoscalers_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go index 175971f660..6b194a9c37 100644 --- a/pkg/fleetautoscalers/fleetautoscalers_test.go +++ b/pkg/fleetautoscalers/fleetautoscalers_test.go @@ -24,17 +24,18 @@ import ( "net/http/httptest" "testing" - 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" "github.com/stretchr/testify/assert" admregv1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" k8stesting "k8s.io/client-go/testing" + + 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" ) const ( @@ -1950,7 +1951,7 @@ func TestApplyListPolicy(t *testing.T) { }}}}, }, want: expected{ - replicas: 0, + replicas: 1, limited: false, wantErr: false, }, @@ -2097,7 +2098,7 @@ func TestApplyListPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: true, + limited: false, wantErr: false, }, }, From 2982cf5c4457e36d19d2dcdc2fb3ada73d2601d8 Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Thu, 11 Apr 2024 18:00:51 +0000 Subject: [PATCH 4/9] review changes --- pkg/fleetautoscalers/fleetautoscalers.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index 9c2198f2f5..db109b3080 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -28,6 +28,10 @@ import ( "strings" "time" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/uuid" + agonesv1 "agones.dev/agones/pkg/apis/agones/v1" autoscalingv1 "agones.dev/agones/pkg/apis/autoscaling/v1" listeragonesv1 "agones.dev/agones/pkg/client/listers/agones/v1" @@ -35,9 +39,6 @@ import ( "agones.dev/agones/pkg/gameservers" gssets "agones.dev/agones/pkg/gameserversets" "agones.dev/agones/pkg/util/runtime" - "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/uuid" ) var tlsConfig = &tls.Config{} @@ -321,16 +322,10 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L } // The desired TOTAL capacity based on the Aggregated Allocated Counts (see applyBufferPolicy for explanation) desiredCapacity := int64(math.Ceil(float64(aggAllocatedCount*100) / float64(100-bufferPercent))) - - // Ensures desiredCapacity is at least 1 - if desiredCapacity < 1 { - desiredCapacity = 1 - } - // Convert into a desired AVAILABLE capacity aka the buffer buffer = desiredCapacity - aggAllocatedCount - // Ensures buffer is at least 1 if desiredCapacity calculation results in a value less than or equal to aggAllocatedCount + // Ensures buffer is at least 1 if buffer < 1 { buffer = 1 } From 615d377426c198f99c07bb7cfb1d51c97f77088c Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Thu, 18 Apr 2024 19:10:44 +0000 Subject: [PATCH 5/9] Prevent scaling down to zero replicas in scaleDown logic --- pkg/fleetautoscalers/fleetautoscalers.go | 16 ++++++++-------- pkg/fleetautoscalers/fleetautoscalers_test.go | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index db109b3080..7a9a914f58 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -324,12 +324,6 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L desiredCapacity := int64(math.Ceil(float64(aggAllocatedCount*100) / float64(100-bufferPercent))) // Convert into a desired AVAILABLE capacity aka the buffer buffer = desiredCapacity - aggAllocatedCount - - // Ensures buffer is at least 1 - if buffer < 1 { - buffer = 1 - } - } // Current available capacity across the TOTAL fleet @@ -352,8 +346,14 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L return scaleLimited(scale, f, gameServerLister, nodeCounts, key, isCounter, replicas, capacity, aggCapacity, minCapacity, maxCapacity) } - return scaleDown(f, gameServerLister, nodeCounts, key, isCounter, replicas, aggCount, - aggCapacity, minCapacity, buffer) + if replicas > 1 { + result, isLimited, err := scaleDown(f, gameServerLister, nodeCounts, key, isCounter, replicas, aggCount, + aggCapacity, minCapacity, buffer) + if result <= 1 { + return 1, true, nil + } + return result, isLimited, err + } } if isCounter { diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go index 6b194a9c37..0b1f9bee74 100644 --- a/pkg/fleetautoscalers/fleetautoscalers_test.go +++ b/pkg/fleetautoscalers/fleetautoscalers_test.go @@ -1952,7 +1952,7 @@ func TestApplyListPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: false, + limited: true, wantErr: false, }, }, @@ -2098,7 +2098,7 @@ func TestApplyListPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: false, + limited: true, wantErr: false, }, }, From 451b4f8b94f20179fdcf34fa34af5df85c14fb48 Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Thu, 18 Apr 2024 23:30:41 +0000 Subject: [PATCH 6/9] modify TestApplyCounterPolicy --- pkg/fleetautoscalers/fleetautoscalers_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go index 0b1f9bee74..830bf7e1df 100644 --- a/pkg/fleetautoscalers/fleetautoscalers_test.go +++ b/pkg/fleetautoscalers/fleetautoscalers_test.go @@ -939,7 +939,7 @@ func TestApplyCounterPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: false, + limited: true, wantErr: false, }, }, @@ -1269,7 +1269,7 @@ func TestApplyCounterPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: false, + limited: true, wantErr: false, }, }, From 956194ae256bbc1a4ce0a78d5e6441cfdc65bfa0 Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Fri, 19 Apr 2024 19:40:47 +0000 Subject: [PATCH 7/9] remove number of replicas check --- pkg/fleetautoscalers/fleetautoscalers.go | 12 +++++------- pkg/fleetautoscalers/fleetautoscalers_test.go | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index 7a9a914f58..01bac49934 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -346,14 +346,12 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L return scaleLimited(scale, f, gameServerLister, nodeCounts, key, isCounter, replicas, capacity, aggCapacity, minCapacity, maxCapacity) } - if replicas > 1 { - result, isLimited, err := scaleDown(f, gameServerLister, nodeCounts, key, isCounter, replicas, aggCount, - aggCapacity, minCapacity, buffer) - if result <= 1 { - return 1, true, nil - } - return result, isLimited, err + result, isLimited, err := scaleDown(f, gameServerLister, nodeCounts, key, isCounter, replicas, aggCount, + aggCapacity, minCapacity, buffer) + if result < 1 { + return 1, true, nil } + return result, isLimited, err } if isCounter { diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go index 830bf7e1df..0b1f9bee74 100644 --- a/pkg/fleetautoscalers/fleetautoscalers_test.go +++ b/pkg/fleetautoscalers/fleetautoscalers_test.go @@ -939,7 +939,7 @@ func TestApplyCounterPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: true, + limited: false, wantErr: false, }, }, @@ -1269,7 +1269,7 @@ func TestApplyCounterPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: true, + limited: false, wantErr: false, }, }, From 1115b59b34321cc8c42a5dc7f2503c643ae41715 Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Wed, 24 Apr 2024 16:48:24 +0000 Subject: [PATCH 8/9] modified scaleDown method and used helper function --- pkg/fleetautoscalers/fleetautoscalers.go | 25 ++++++++++--------- pkg/fleetautoscalers/fleetautoscalers_test.go | 2 +- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index 01bac49934..8c7ec36143 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -346,12 +346,8 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L return scaleLimited(scale, f, gameServerLister, nodeCounts, key, isCounter, replicas, capacity, aggCapacity, minCapacity, maxCapacity) } - result, isLimited, err := scaleDown(f, gameServerLister, nodeCounts, key, isCounter, replicas, aggCount, + return scaleDown(f, gameServerLister, nodeCounts, key, isCounter, replicas, aggCount, aggCapacity, minCapacity, buffer) - if result < 1 { - return 1, true, nil - } - return result, isLimited, err } if isCounter { @@ -475,8 +471,8 @@ func scaleDown(f *agonesv1.Fleet, gameServerLister listeragonesv1.GameServerList nodeCounts map[string]gameservers.NodeCount, key string, isCounter bool, replicas int32, aggCount, aggCapacity, minCapacity, buffer int64) (int32, bool, error) { // Exit early if we're already at MinCapacity to avoid calling getSortedGameServers if unnecessary - if aggCapacity == minCapacity { - return replicas, true, nil + if aggCapacity <= minCapacity { + return max(replicas, 1), true, nil } // We first need to get the individual game servers in order of deletion on scale down, as any @@ -491,6 +487,9 @@ func scaleDown(f *agonesv1.Fleet, gameServerLister listeragonesv1.GameServerList // "Remove" one game server at a time in order of potential deletion. (Not actually removed here, // that's done later, if possible, by the fleetautoscaler controller.) for _, gs := range gameServers { + if replicas == 1 { + return 1, true, nil + } replicas-- switch isCounter { case true: @@ -526,11 +525,13 @@ func scaleDown(f *agonesv1.Fleet, gameServerLister listeragonesv1.GameServerList return replicas, true, nil } } + return max(replicas, 1), false, nil +} - // We are not currently able to scale down to zero replicas, so one replica is the minimum allowed. - if replicas < 1 { - replicas = 1 +// Helper function to return maximum of two int32 values +func max(a, b int32) int32 { + if a > b { + return a } - - return replicas, false, nil + return b } diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go index 0b1f9bee74..a99c831707 100644 --- a/pkg/fleetautoscalers/fleetautoscalers_test.go +++ b/pkg/fleetautoscalers/fleetautoscalers_test.go @@ -939,7 +939,7 @@ func TestApplyCounterPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: false, + limited: true, wantErr: false, }, }, From 8b616404dac540cda5782884517e3f80829fd9c6 Mon Sep 17 00:00:00 2001 From: Kalaiselvi Murugesan Date: Thu, 9 May 2024 17:07:30 +0000 Subject: [PATCH 9/9] suggested changes --- pkg/fleetautoscalers/fleetautoscalers.go | 25 ++++++++++--------- pkg/fleetautoscalers/fleetautoscalers_test.go | 2 +- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pkg/fleetautoscalers/fleetautoscalers.go b/pkg/fleetautoscalers/fleetautoscalers.go index 8c7ec36143..5ef77e0edf 100644 --- a/pkg/fleetautoscalers/fleetautoscalers.go +++ b/pkg/fleetautoscalers/fleetautoscalers.go @@ -320,6 +320,12 @@ func applyCounterOrListPolicy(c *autoscalingv1.CounterPolicy, l *autoscalingv1.L if err != nil { return 0, false, err } + // If the Aggregated Allocated Counts is 0 then desired capacity gets calculated as 0. If the + // capacity of 1 replica is equal to or greater than minimum capacity we can exit early. + if aggAllocatedCount <= 0 && capacity >= minCapacity { + return 1, true, nil + } + // The desired TOTAL capacity based on the Aggregated Allocated Counts (see applyBufferPolicy for explanation) desiredCapacity := int64(math.Ceil(float64(aggAllocatedCount*100) / float64(100-bufferPercent))) // Convert into a desired AVAILABLE capacity aka the buffer @@ -471,8 +477,8 @@ func scaleDown(f *agonesv1.Fleet, gameServerLister listeragonesv1.GameServerList nodeCounts map[string]gameservers.NodeCount, key string, isCounter bool, replicas int32, aggCount, aggCapacity, minCapacity, buffer int64) (int32, bool, error) { // Exit early if we're already at MinCapacity to avoid calling getSortedGameServers if unnecessary - if aggCapacity <= minCapacity { - return max(replicas, 1), true, nil + if aggCapacity == minCapacity { + return replicas, true, nil } // We first need to get the individual game servers in order of deletion on scale down, as any @@ -487,9 +493,6 @@ func scaleDown(f *agonesv1.Fleet, gameServerLister listeragonesv1.GameServerList // "Remove" one game server at a time in order of potential deletion. (Not actually removed here, // that's done later, if possible, by the fleetautoscaler controller.) for _, gs := range gameServers { - if replicas == 1 { - return 1, true, nil - } replicas-- switch isCounter { case true: @@ -525,13 +528,11 @@ func scaleDown(f *agonesv1.Fleet, gameServerLister listeragonesv1.GameServerList return replicas, true, nil } } - return max(replicas, 1), false, nil -} -// Helper function to return maximum of two int32 values -func max(a, b int32) int32 { - if a > b { - return a + // We are not currently able to scale down to zero replicas, so one replica is the minimum allowed. + if replicas < 1 { + replicas = 1 } - return b + + return replicas, false, nil } diff --git a/pkg/fleetautoscalers/fleetautoscalers_test.go b/pkg/fleetautoscalers/fleetautoscalers_test.go index a99c831707..0b1f9bee74 100644 --- a/pkg/fleetautoscalers/fleetautoscalers_test.go +++ b/pkg/fleetautoscalers/fleetautoscalers_test.go @@ -939,7 +939,7 @@ func TestApplyCounterPolicy(t *testing.T) { }, want: expected{ replicas: 1, - limited: true, + limited: false, wantErr: false, }, },