Skip to content

Commit 5de49a1

Browse files
vadasambarvadasambar
committed
feat: support --scale-down-delay-after-* per nodegroup
Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: update scale down status after every scale up - move scaledown delay status to cluster state/registry - enable scale down if `ScaleDownDelayTypeLocal` is enabled - add new funcs on cluster state to get and update scale down delay status - use timestamp instead of booleans to track scale down delay status Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: use existing fields on clusterstate - uses `scaleUpRequests`, `scaleDownRequests` and `scaleUpFailures` instead of `ScaleUpDelayStatus` - changed the above existing fields a little to make them more convenient for use - moved initializing scale down delay processor to static autoscaler (because clusterstate is not available in main.go) Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: remove note saying only `scale-down-after-add` is supported - because we are supporting all the flags Signed-off-by: vadasambar <surajrbanakar@gmail.com> fix: evaluate `scaleDownInCooldown` the old way only if `ScaleDownDelayTypeLocal` is set to `false` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: remove line saying `--scale-down-delay-type-local` is only supported for `--scale-down-delay-after-add` - because it is not true anymore - we are supporting all `--scale-down-delay-after-*` flags per nodegroup Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: fix clusterstate tests failing Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: move back initializing processors logic to from static autoscaler to main - we don't want to initialize processors in static autoscaler because anyone implementing an alternative to static_autoscaler has to initialize the processors - and initializing specific processors is making static autoscaler aware of an implementation detail which might not be the best practice Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: revert changes related to `clusterstate` - since I am going with observer pattern Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: add observer interface for state of scaling - to implement observer pattern for tracking state of scale up/downs (as opposed to using clusterstate to do the same) - refactor `ScaleDownCandidatesDelayProcessor` to use fields from the new observer Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: remove params passed to `clearScaleUpFailures` - not needed anymore Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: revert clusterstate tests - approach has changed - I am not making any changes in clusterstate now Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: add accidentally deleted lines for clusterstate test Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: implement `Add` fn for scale state observer - to easily add new observers - re-word comments - remove redundant params from `NewDefaultScaleDownCandidatesProcessor` Signed-off-by: vadasambar <surajrbanakar@gmail.com> fix: CI complaining because no comments on fn definitions Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: initialize parent `ScaleDownCandidatesProcessor` - instead of `ScaleDownCandidatesSortingProcessor` and `ScaleDownCandidatesDelayProcessor` separately Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: add scale state notifier to list of default processors - initialize processors for `NewDefaultScaleDownCandidatesProcessor` outside and pass them to the fn - this allows more flexibility Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: add observer interface - create a separate observer directory - implement `RegisterScaleUp` function in the clusterstate - TODO: resolve syntax errors Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: use `scaleStateNotifier` in place of `clusterstate` - delete leftover `scale_stateA_observer.go` (new one is already present in `observers` directory) - register `clustertstate` with `scaleStateNotifier` - use `Register` instead of `Add` function in `scaleStateNotifier` - fix `go build` - wip: fixing tests Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: fix syntax errors - add utils package `pointers` for converting `time` to pointer (without having to initialize a new variable) Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: wip track scale down failures along with scale up failures - I was tracking scale up failures but not scale down failures - fix copyright year 2017 -> 2023 for the new `pointers` package Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: register failed scale down with scale state notifier - wip writing tests for `scale_down_candidates_delay_processor` - fix CI lint errors - remove test file for `scale_down_candidates_processor` (there is not much to test as of now) Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: wip tests for `ScaleDownCandidatesDelayProcessor` Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: add unit tests for `ScaleDownCandidatesDelayProcessor` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: don't track scale up failures in `ScaleDownCandidatesDelayProcessor` - not needed Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: better doc comments for `TestGetScaleDownCandidates` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: don't ignore error in `NGChangeObserver` - return it instead and let the caller decide what to do with it Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: change pointers to values in `NGChangeObserver` interface - easier to work with - remove `expectedAddTime` param from `RegisterScaleUp` (not needed for now) - add tests for clusterstate's `RegisterScaleUp` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: conditions in `GetScaleDownCandidates` - set scale down in cool down if the number of scale down candidates is 0 Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: use `ng1` instead of `ng2` in existing test Signed-off-by: vadasambar <surajrbanakar@gmail.com> feat: wip static autoscaler tests Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: assign directly instead of using `sdProcessor` variable - variable is not needed Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: first working test for static autoscaler Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: continue working on static autoscaler tests Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: wip second static autoscaler test Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: remove `Println` used for debugging Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: add static_autoscaler tests for scale down delay per nodegroup flags Signed-off-by: vadasambar <surajrbanakar@gmail.com> chore: rebase off the latest `master` - change scale state observer interface's `RegisterFailedScaleup` to reflect latest changes around clusterstate's `RegisterFailedScaleup` in `master` Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: fix clusterstate test failing Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: fix failing orchestrator test Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: rename `defaultScaleDownCandidatesProcessor` -> `combinedScaleDownCandidatesProcessor` - describes the processor better Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: replace `NGChangeObserver` -> `NodeGroupChangeObserver` - makes it easier to understand for someone not familiar with the codebase Signed-off-by: vadasambar <surajrbanakar@gmail.com> docs: reword code comment `after` -> `for which` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: don't return error from `RegisterScaleDown` - not needed as of now (no implementer function returns a non-nil error for this function) Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: address review comments around ng change observer interface - change dir structure of nodegroup change observer package - stop returning errors wherever it is not needed in the nodegroup change observer interface - rename `NGChangeObserver` -> `NodeGroupChangeObserver` interface (makes it easier to understand) Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: make nodegroupchange observer thread-safe Signed-off-by: vadasambar <surajrbanakar@gmail.com> docs: add TODO to consider using multiple mutexes in nodegroupchange observer Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: use `time.Now()` directly instead of assigning a variable to it Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: share code for checking if there was a recent scale-up/down/failure Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: convert `ScaleDownCandidatesDelayProcessor` into table tests Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: change scale state notifier's `Register()` -> `RegisterForNotifications()` - makes it easier to understand what the function does Signed-off-by: vadasambar <surajrbanakar@gmail.com> test: replace scale state notifier `Register` -> `RegisterForNotifications` in test - to fix syntax errors since it is already renamed in the actual code Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: remove `clusterStateRegistry` from `delete_in_batch` tests - not needed anymore since we have `scaleStateNotifier` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: address PR review comments Signed-off-by: vadasambar <surajrbanakar@gmail.com> fix: add empty `RegisterFailedScaleDown` for clusterstate - fix syntax error in static autoscaler test Signed-off-by: vadasambar <surajrbanakar@gmail.com>
1 parent cdb24d8 commit 5de49a1

20 files changed

+798
-90
lines changed

‎cluster-autoscaler/clusterstate/clusterstate.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -188,13 +188,8 @@ func (csr *ClusterStateRegistry) Stop() {
188188
close(csr.interrupt)
189189
}
190190

191-
// RegisterOrUpdateScaleUp registers scale-up for give node group or changes requested node increase
192-
// count.
193-
// If delta is positive then number of new nodes requested is increased; Time and expectedAddTime
194-
// are reset.
195-
// If delta is negative the number of new nodes requested is decreased; Time and expectedAddTime are
196-
// left intact.
197-
func (csr *ClusterStateRegistry) RegisterOrUpdateScaleUp(nodeGroup cloudprovider.NodeGroup, delta int, currentTime time.Time) {
191+
// RegisterScaleUp registers scale-up for give node group
192+
func (csr *ClusterStateRegistry) RegisterScaleUp(nodeGroup cloudprovider.NodeGroup, delta int, currentTime time.Time) {
198193
csr.Lock()
199194
defer csr.Unlock()
200195
csr.registerOrUpdateScaleUpNoLock(nodeGroup, delta, currentTime)
@@ -246,7 +241,14 @@ func (csr *ClusterStateRegistry) registerOrUpdateScaleUpNoLock(nodeGroup cloudpr
246241
}
247242

248243
// RegisterScaleDown registers node scale down.
249-
func (csr *ClusterStateRegistry) RegisterScaleDown(request *ScaleDownRequest) {
244+
func (csr *ClusterStateRegistry) RegisterScaleDown(nodeGroup cloudprovider.NodeGroup,
245+
nodeName string, currentTime time.Time, expectedDeleteTime time.Time) {
246+
request := &ScaleDownRequest{
247+
NodeGroup: nodeGroup,
248+
NodeName: nodeName,
249+
Time: currentTime,
250+
ExpectedDeleteTime: expectedDeleteTime,
251+
}
250252
csr.Lock()
251253
defer csr.Unlock()
252254
csr.scaleDownRequests = append(csr.scaleDownRequests, request)
@@ -310,16 +312,21 @@ func (csr *ClusterStateRegistry) backoffNodeGroup(nodeGroup cloudprovider.NodeGr
310312
// RegisterFailedScaleUp should be called after getting error from cloudprovider
311313
// when trying to scale-up node group. It will mark this group as not safe to autoscale
312314
// for some time.
313-
func (csr *ClusterStateRegistry) RegisterFailedScaleUp(nodeGroup cloudprovider.NodeGroup, reason metrics.FailedScaleUpReason, errorMessage, gpuResourceName, gpuType string, currentTime time.Time) {
315+
func (csr *ClusterStateRegistry) RegisterFailedScaleUp(nodeGroup cloudprovider.NodeGroup, reason string, errorMessage, gpuResourceName, gpuType string, currentTime time.Time) {
314316
csr.Lock()
315317
defer csr.Unlock()
316-
csr.registerFailedScaleUpNoLock(nodeGroup, reason, cloudprovider.InstanceErrorInfo{
318+
csr.registerFailedScaleUpNoLock(nodeGroup, metrics.FailedScaleUpReason(reason), cloudprovider.InstanceErrorInfo{
317319
ErrorClass: cloudprovider.OtherErrorClass,
318320
ErrorCode: string(reason),
319321
ErrorMessage: errorMessage,
320322
}, gpuResourceName, gpuType, currentTime)
321323
}
322324

325+
// RegisterFailedScaleDown records failed scale-down for a nodegroup.
326+
// We don't need to implement this function for cluster state registry
327+
func (csr *ClusterStateRegistry) RegisterFailedScaleDown(_ cloudprovider.NodeGroup, _ string, _ time.Time) {
328+
}
329+
323330
func (csr *ClusterStateRegistry) registerFailedScaleUpNoLock(nodeGroup cloudprovider.NodeGroup, reason metrics.FailedScaleUpReason, errorInfo cloudprovider.InstanceErrorInfo, gpuResourceName, gpuType string, currentTime time.Time) {
324331
csr.scaleUpFailures[nodeGroup.Id()] = append(csr.scaleUpFailures[nodeGroup.Id()], ScaleUpFailure{NodeGroup: nodeGroup, Reason: reason, Time: currentTime})
325332
metrics.RegisterFailedScaleUp(reason, gpuResourceName, gpuType)

‎cluster-autoscaler/clusterstate/clusterstate_test.go

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
testprovider "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/test"
3232
"k8s.io/autoscaler/cluster-autoscaler/clusterstate/api"
3333
"k8s.io/autoscaler/cluster-autoscaler/clusterstate/utils"
34+
3435
"k8s.io/autoscaler/cluster-autoscaler/utils/taints"
3536
. "k8s.io/autoscaler/cluster-autoscaler/utils/test"
3637
"k8s.io/client-go/kubernetes/fake"
@@ -75,7 +76,7 @@ func TestOKWithScaleUp(t *testing.T) {
7576
MaxTotalUnreadyPercentage: 10,
7677
OkTotalUnreadyCount: 1,
7778
}, fakeLogRecorder, newBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: time.Minute}))
78-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 4, time.Now())
79+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 4, time.Now())
7980
err := clusterstate.UpdateNodes([]*apiv1.Node{ng1_1, ng2_1}, nil, now)
8081
assert.NoError(t, err)
8182
assert.True(t, clusterstate.IsClusterHealthy())
@@ -122,7 +123,7 @@ func TestEmptyOK(t *testing.T) {
122123
assert.False(t, clusterstate.HasNodeGroupStartedScaleUp("ng1"))
123124

124125
provider.AddNodeGroup("ng1", 0, 10, 3)
125-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 3, now.Add(-3*time.Second))
126+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 3, now.Add(-3*time.Second))
126127
// clusterstate.scaleUpRequests["ng1"].Time = now.Add(-3 * time.Second)
127128
// clusterstate.scaleUpRequests["ng1"].ExpectedAddTime = now.Add(1 * time.Minute)
128129
err = clusterstate.UpdateNodes([]*apiv1.Node{}, nil, now)
@@ -161,7 +162,7 @@ func TestHasNodeGroupStartedScaleUp(t *testing.T) {
161162
assert.False(t, clusterstate.HasNodeGroupStartedScaleUp("ng1"))
162163

163164
provider.AddNodeGroup("ng1", 0, 5, tc.initialSize+tc.delta)
164-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), tc.delta, now.Add(-3*time.Second))
165+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), tc.delta, now.Add(-3*time.Second))
165166
err = clusterstate.UpdateNodes([]*apiv1.Node{}, nil, now)
166167
assert.NoError(t, err)
167168
assert.True(t, clusterstate.IsNodeGroupScalingUp("ng1"))
@@ -450,7 +451,7 @@ func TestExpiredScaleUp(t *testing.T) {
450451
MaxTotalUnreadyPercentage: 10,
451452
OkTotalUnreadyCount: 1,
452453
}, fakeLogRecorder, newBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 2 * time.Minute}))
453-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 4, now.Add(-3*time.Minute))
454+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 4, now.Add(-3*time.Minute))
454455
err := clusterstate.UpdateNodes([]*apiv1.Node{ng1_1}, nil, now)
455456
assert.NoError(t, err)
456457
assert.True(t, clusterstate.IsClusterHealthy())
@@ -476,13 +477,7 @@ func TestRegisterScaleDown(t *testing.T) {
476477
OkTotalUnreadyCount: 1,
477478
}, fakeLogRecorder, newBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 15 * time.Minute}))
478479
now := time.Now()
479-
480-
clusterstate.RegisterScaleDown(&ScaleDownRequest{
481-
NodeGroup: provider.GetNodeGroup("ng1"),
482-
NodeName: "ng1-1",
483-
ExpectedDeleteTime: now.Add(time.Minute),
484-
Time: now,
485-
})
480+
clusterstate.RegisterScaleDown(provider.GetNodeGroup("ng1"), "ng1-1", now.Add(time.Minute), now)
486481
assert.Equal(t, 1, len(clusterstate.scaleDownRequests))
487482
clusterstate.updateScaleRequests(now.Add(5 * time.Minute))
488483
assert.Equal(t, 0, len(clusterstate.scaleDownRequests))
@@ -794,7 +789,7 @@ func TestScaleUpBackoff(t *testing.T) {
794789
}, fakeLogRecorder, newBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 120 * time.Second}))
795790

796791
// After failed scale-up, node group should be still healthy, but should backoff from scale-ups
797-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 1, now.Add(-180*time.Second))
792+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 1, now.Add(-180*time.Second))
798793
err := clusterstate.UpdateNodes([]*apiv1.Node{ng1_1, ng1_2, ng1_3}, nil, now)
799794
assert.NoError(t, err)
800795
assert.True(t, clusterstate.IsClusterHealthy())
@@ -826,7 +821,7 @@ func TestScaleUpBackoff(t *testing.T) {
826821
assert.Equal(t, NodeGroupScalingSafety{SafeToScale: true, Healthy: true}, clusterstate.NodeGroupScaleUpSafety(ng1, now))
827822

828823
// Another failed scale up should cause longer backoff
829-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 1, now.Add(-121*time.Second))
824+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 1, now.Add(-121*time.Second))
830825

831826
err = clusterstate.UpdateNodes([]*apiv1.Node{ng1_1, ng1_2, ng1_3}, nil, now)
832827
assert.NoError(t, err)
@@ -860,7 +855,7 @@ func TestScaleUpBackoff(t *testing.T) {
860855
}, clusterstate.NodeGroupScaleUpSafety(ng1, now))
861856

862857
// The backoff should be cleared after a successful scale-up
863-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 1, now)
858+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 1, now)
864859
ng1_4 := BuildTestNode("ng1-4", 1000, 1000)
865860
SetNodeReadyState(ng1_4, true, now.Add(-1*time.Minute))
866861
provider.AddNode("ng1", ng1_4)
@@ -935,6 +930,7 @@ func TestUpdateScaleUp(t *testing.T) {
935930

936931
provider := testprovider.NewTestCloudProvider(nil, nil)
937932
provider.AddNodeGroup("ng1", 1, 10, 5)
933+
provider.AddNodeGroup("ng2", 1, 10, 5)
938934
fakeClient := &fake.Clientset{}
939935
fakeLogRecorder, _ := utils.NewStatusMapRecorder(fakeClient, "kube-system", kube_record.NewFakeRecorder(5), false, "my-cool-configmap")
940936
clusterstate := NewClusterStateRegistry(
@@ -948,29 +944,30 @@ func TestUpdateScaleUp(t *testing.T) {
948944
nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 10 * time.Second}),
949945
)
950946

951-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 100, now)
947+
// Test cases for `RegisterScaleUp`
948+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 100, now)
952949
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].Increase, 100)
953950
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].Time, now)
954951
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].ExpectedAddTime, now.Add(10*time.Second))
955952

956953
// expect no change of times on negative delta
957-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), -20, later)
954+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), -20, later)
958955
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].Increase, 80)
959956
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].Time, now)
960957
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].ExpectedAddTime, now.Add(10*time.Second))
961958

962959
// update times on positive delta
963-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), 30, later)
960+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), 30, later)
964961
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].Increase, 110)
965962
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].Time, later)
966963
assert.Equal(t, clusterstate.scaleUpRequests["ng1"].ExpectedAddTime, later.Add(10*time.Second))
967964

968965
// if we get below 0 scalup is deleted
969-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), -200, now)
966+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), -200, now)
970967
assert.Nil(t, clusterstate.scaleUpRequests["ng1"])
971968

972969
// If new scalup is registered with negative delta nothing should happen
973-
clusterstate.RegisterOrUpdateScaleUp(provider.GetNodeGroup("ng1"), -200, now)
970+
clusterstate.RegisterScaleUp(provider.GetNodeGroup("ng1"), -200, now)
974971
assert.Nil(t, clusterstate.scaleUpRequests["ng1"])
975972
}
976973

@@ -986,9 +983,9 @@ func TestScaleUpFailures(t *testing.T) {
986983
fakeLogRecorder, _ := utils.NewStatusMapRecorder(fakeClient, "kube-system", kube_record.NewFakeRecorder(5), false, "my-cool-configmap")
987984
clusterstate := NewClusterStateRegistry(provider, ClusterStateRegistryConfig{}, fakeLogRecorder, newBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 15 * time.Minute}))
988985

989-
clusterstate.RegisterFailedScaleUp(provider.GetNodeGroup("ng1"), metrics.Timeout, "", "", "", now)
990-
clusterstate.RegisterFailedScaleUp(provider.GetNodeGroup("ng2"), metrics.Timeout, "", "", "", now)
991-
clusterstate.RegisterFailedScaleUp(provider.GetNodeGroup("ng1"), metrics.APIError, "", "", "", now.Add(time.Minute))
986+
clusterstate.RegisterFailedScaleUp(provider.GetNodeGroup("ng1"), string(metrics.Timeout), "", "", "", now)
987+
clusterstate.RegisterFailedScaleUp(provider.GetNodeGroup("ng2"), string(metrics.Timeout), "", "", "", now)
988+
clusterstate.RegisterFailedScaleUp(provider.GetNodeGroup("ng1"), string(metrics.APIError), "", "", "", now.Add(time.Minute))
992989

993990
failures := clusterstate.GetScaleUpFailures()
994991
assert.Equal(t, map[string][]ScaleUpFailure{

‎cluster-autoscaler/config/autoscaling_options.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,9 @@ type AutoscalingOptions struct {
158158
ScaleDownDelayAfterDelete time.Duration
159159
// ScaleDownDelayAfterFailure sets the duration before the next scale down attempt if scale down results in an error
160160
ScaleDownDelayAfterFailure time.Duration
161+
// ScaleDownDelayTypeLocal sets if the --scale-down-delay-after-* flags should be applied locally per nodegroup
162+
// or globally across all nodegroups
163+
ScaleDownDelayTypeLocal bool
161164
// ScaleDownNonEmptyCandidatesCount is the maximum number of non empty nodes
162165
// considered at once as candidates for scale down.
163166
ScaleDownNonEmptyCandidatesCount int

‎cluster-autoscaler/config/const.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,17 @@ const (
4040
DefaultMaxNodeProvisionTimeKey = "maxnodeprovisiontime"
4141
// DefaultIgnoreDaemonSetsUtilizationKey identifies IgnoreDaemonSetsUtilization autoscaling option
4242
DefaultIgnoreDaemonSetsUtilizationKey = "ignoredaemonsetsutilization"
43-
// DefaultScaleDownUnneededTime identifies ScaleDownUnneededTime autoscaling option
43+
44+
// DefaultScaleDownUnneededTime is the default time duration for which CA waits before deleting an unneeded node
4445
DefaultScaleDownUnneededTime = 10 * time.Minute
4546
// DefaultScaleDownUnreadyTime identifies ScaleDownUnreadyTime autoscaling option
4647
DefaultScaleDownUnreadyTime = 20 * time.Minute
4748
// DefaultScaleDownUtilizationThreshold identifies ScaleDownUtilizationThreshold autoscaling option
4849
DefaultScaleDownUtilizationThreshold = 0.5
4950
// DefaultScaleDownGpuUtilizationThreshold identifies ScaleDownGpuUtilizationThreshold autoscaling option
5051
DefaultScaleDownGpuUtilizationThreshold = 0.5
52+
// DefaultScaleDownDelayAfterFailure is the default value for ScaleDownDelayAfterFailure autoscaling option
53+
DefaultScaleDownDelayAfterFailure = 3 * time.Minute
54+
// DefaultScanInterval is the default scan interval for CA
55+
DefaultScanInterval = 10 * time.Second
5156
)

‎cluster-autoscaler/core/scaledown/actuation/actuator.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222

2323
apiv1 "k8s.io/api/core/v1"
2424
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
25-
"k8s.io/autoscaler/cluster-autoscaler/clusterstate"
2625
"k8s.io/autoscaler/cluster-autoscaler/context"
2726
"k8s.io/autoscaler/cluster-autoscaler/core/scaledown"
2827
"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/budgets"
@@ -31,6 +30,7 @@ import (
3130
"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/status"
3231
"k8s.io/autoscaler/cluster-autoscaler/core/utils"
3332
"k8s.io/autoscaler/cluster-autoscaler/metrics"
33+
"k8s.io/autoscaler/cluster-autoscaler/observers/nodegroupchange"
3434
"k8s.io/autoscaler/cluster-autoscaler/simulator"
3535
"k8s.io/autoscaler/cluster-autoscaler/simulator/clustersnapshot"
3636
"k8s.io/autoscaler/cluster-autoscaler/simulator/drainability/rules"
@@ -45,7 +45,6 @@ import (
4545
// Actuator is responsible for draining and deleting nodes.
4646
type Actuator struct {
4747
ctx *context.AutoscalingContext
48-
clusterState *clusterstate.ClusterStateRegistry
4948
nodeDeletionTracker *deletiontracker.NodeDeletionTracker
5049
nodeDeletionScheduler *GroupDeletionScheduler
5150
deleteOptions options.NodeDeleteOptions
@@ -66,8 +65,8 @@ type actuatorNodeGroupConfigGetter interface {
6665
}
6766

6867
// NewActuator returns a new instance of Actuator.
69-
func NewActuator(ctx *context.AutoscalingContext, csr *clusterstate.ClusterStateRegistry, ndt *deletiontracker.NodeDeletionTracker, deleteOptions options.NodeDeleteOptions, drainabilityRules rules.Rules, configGetter actuatorNodeGroupConfigGetter) *Actuator {
70-
ndb := NewNodeDeletionBatcher(ctx, csr, ndt, ctx.NodeDeletionBatcherInterval)
68+
func NewActuator(ctx *context.AutoscalingContext, scaleStateNotifier nodegroupchange.NodeGroupChangeObserver, ndt *deletiontracker.NodeDeletionTracker, deleteOptions options.NodeDeleteOptions, drainabilityRules rules.Rules, configGetter actuatorNodeGroupConfigGetter) *Actuator {
69+
ndb := NewNodeDeletionBatcher(ctx, scaleStateNotifier, ndt, ctx.NodeDeletionBatcherInterval)
7170
legacyFlagDrainConfig := SingleRuleDrainConfig(ctx.MaxGracefulTerminationSec)
7271
var evictor Evictor
7372
if len(ctx.DrainPriorityConfig) > 0 {
@@ -77,7 +76,6 @@ func NewActuator(ctx *context.AutoscalingContext, csr *clusterstate.ClusterState
7776
}
7877
return &Actuator{
7978
ctx: ctx,
80-
clusterState: csr,
8179
nodeDeletionTracker: ndt,
8280
nodeDeletionScheduler: NewGroupDeletionScheduler(ctx, ndt, ndb, evictor),
8381
budgetProcessor: budgets.NewScaleDownBudgetProcessor(ctx),
@@ -102,7 +100,7 @@ func (a *Actuator) ClearResultsNotNewerThan(t time.Time) {
102100
func (a *Actuator) StartDeletion(empty, drain []*apiv1.Node) (*status.ScaleDownStatus, errors.AutoscalerError) {
103101
a.nodeDeletionScheduler.ReportMetrics()
104102
deletionStartTime := time.Now()
105-
defer func() { metrics.UpdateDuration(metrics.ScaleDownNodeDeletion, time.Now().Sub(deletionStartTime)) }()
103+
defer func() { metrics.UpdateDuration(metrics.ScaleDownNodeDeletion, time.Since(deletionStartTime)) }()
106104

107105
results, ts := a.nodeDeletionTracker.DeletionResults()
108106
scaleDownStatus := &status.ScaleDownStatus{NodeDeleteResults: results, NodeDeleteResultsAsOf: ts}

‎cluster-autoscaler/core/scaledown/actuation/actuator_test.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import (
4040
"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/deletiontracker"
4141
"k8s.io/autoscaler/cluster-autoscaler/core/scaledown/status"
4242
. "k8s.io/autoscaler/cluster-autoscaler/core/test"
43+
"k8s.io/autoscaler/cluster-autoscaler/observers/nodegroupchange"
4344
"k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupconfig"
4445
"k8s.io/autoscaler/cluster-autoscaler/simulator/utilization"
4546
kube_util "k8s.io/autoscaler/cluster-autoscaler/utils/kubernetes"
@@ -1186,13 +1187,16 @@ func TestStartDeletion(t *testing.T) {
11861187
wantScaleDownStatus.ScaledDownNodes = append(wantScaleDownStatus.ScaledDownNodes, statusScaledDownNode)
11871188
}
11881189

1190+
scaleStateNotifier := nodegroupchange.NewNodeGroupChangeObserversList()
1191+
scaleStateNotifier.Register(csr)
1192+
11891193
// Create Actuator, run StartDeletion, and verify the error.
11901194
ndt := deletiontracker.NewNodeDeletionTracker(0)
1191-
ndb := NewNodeDeletionBatcher(&ctx, csr, ndt, 0*time.Second)
1195+
ndb := NewNodeDeletionBatcher(&ctx, scaleStateNotifier, ndt, 0*time.Second)
11921196
legacyFlagDrainConfig := SingleRuleDrainConfig(ctx.MaxGracefulTerminationSec)
11931197
evictor := Evictor{EvictionRetryTime: 0, PodEvictionHeadroom: DefaultPodEvictionHeadroom, shutdownGracePeriodByPodPriority: legacyFlagDrainConfig, fullDsEviction: false}
11941198
actuator := Actuator{
1195-
ctx: &ctx, clusterState: csr, nodeDeletionTracker: ndt,
1199+
ctx: &ctx, nodeDeletionTracker: ndt,
11961200
nodeDeletionScheduler: NewGroupDeletionScheduler(&ctx, ndt, ndb, evictor),
11971201
budgetProcessor: budgets.NewScaleDownBudgetProcessor(&ctx),
11981202
configGetter: nodegroupconfig.NewDefaultNodeGroupConfigProcessor(ctx.NodeGroupDefaults),
@@ -1424,12 +1428,14 @@ func TestStartDeletionInBatchBasic(t *testing.T) {
14241428
t.Fatalf("Couldn't set up autoscaling context: %v", err)
14251429
}
14261430
csr := clusterstate.NewClusterStateRegistry(provider, clusterstate.ClusterStateRegistryConfig{}, ctx.LogRecorder, NewBackoff(), nodegroupconfig.NewDefaultNodeGroupConfigProcessor(config.NodeGroupAutoscalingOptions{MaxNodeProvisionTime: 15 * time.Minute}))
1431+
scaleStateNotifier := nodegroupchange.NewNodeGroupChangeObserversList()
1432+
scaleStateNotifier.Register(csr)
14271433
ndt := deletiontracker.NewNodeDeletionTracker(0)
1428-
ndb := NewNodeDeletionBatcher(&ctx, csr, ndt, deleteInterval)
1434+
ndb := NewNodeDeletionBatcher(&ctx, scaleStateNotifier, ndt, deleteInterval)
14291435
legacyFlagDrainConfig := SingleRuleDrainConfig(ctx.MaxGracefulTerminationSec)
14301436
evictor := Evictor{EvictionRetryTime: 0, PodEvictionHeadroom: DefaultPodEvictionHeadroom, shutdownGracePeriodByPodPriority: legacyFlagDrainConfig}
14311437
actuator := Actuator{
1432-
ctx: &ctx, clusterState: csr, nodeDeletionTracker: ndt,
1438+
ctx: &ctx, nodeDeletionTracker: ndt,
14331439
nodeDeletionScheduler: NewGroupDeletionScheduler(&ctx, ndt, ndb, evictor),
14341440
budgetProcessor: budgets.NewScaleDownBudgetProcessor(&ctx),
14351441
}

0 commit comments

Comments
 (0)