From 4fd2671239fc1d44d8ff779607cd5508dd816733 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Wed, 9 Oct 2019 09:15:41 +0200 Subject: [PATCH 01/16] .Update added test to check if watcher is disabled in dry-run mode --- scaler/scale_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/scaler/scale_test.go b/scaler/scale_test.go index cd1a3593..59964edb 100644 --- a/scaler/scale_test.go +++ b/scaler/scale_test.go @@ -217,3 +217,46 @@ func TestScale_DownDryRun(t *testing.T) { result := scaler.scale(1, 4, true) assert.NotEqual(t, scaleFailed, result.state) } + +func TestScale_DryRun(t *testing.T) { + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + metrics, mocks := NewMockedMetrics(mockCtrl) + + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + + sObjName := "any" + cfg := Config{Name: sObjName, MinCount: 1, MaxCount: 5, WatcherInterval: time.Millisecond * 100} + scaler, err := cfg.New(scaTgt, metrics) + require.NoError(t, err) + + addedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) + addedScalingTickets.EXPECT().Inc() + plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) + plannedButSkippedGauge.EXPECT().Set(float64(1)) + appliedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) + appliedScalingTickets.EXPECT().Inc() + ignoredCounter := mock_metrics.NewMockCounter(mockCtrl) + ignoredCounter.EXPECT().Inc() + gomock.InOrder( + mocks.scalingTicketCount.EXPECT().WithLabelValues("added").Return(addedScalingTickets), + scaTgt.EXPECT().GetScalingObjectCount(sObjName).Return(uint(1), nil), + scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil), + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), + mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(appliedScalingTickets), + mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), + mocks.scaleResultCounter.EXPECT().WithLabelValues("ignored").Return(ignoredCounter), + ) + scaler.Run() + defer func() { + scaler.Stop() + scaler.Join() + }() + + err = scaler.ScaleTo(2, true) + assert.NoError(t, err) + + // needed to give the ticketprocessor some time to start working + time.Sleep(time.Second * 1) +} From c6046a52ee9d55cf82a9771ff069d558939df54c Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Wed, 9 Oct 2019 15:24:37 +0200 Subject: [PATCH 02/16] .Refactor move desiredScale and scaleObjectWatcher out of scaler 1 --- scaler/scale.go | 5 ++-- scaler/scale_test.go | 7 ++--- scaler/scalerImpl.go | 17 +++++++++++ scaler/scaler_test.go | 67 +++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/scaler/scale.go b/scaler/scale.go index 185265ce..6a9aa34a 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -9,6 +9,7 @@ import ( // scaleState represents the state of a scaling type scaleState string +// TODO: Check which states are still needed and which can be removed const ( // scaleUnknown means the scale process was completed successfully scaleUnknown scaleState = "unknown" @@ -152,9 +153,7 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR s.logger.Info().Str("scalingObject", sObjName).Msgf("Scale %s by %d to %d.", scaleTypeStr, diff, newCount) s.metrics.plannedButSkippedScalingOpen.WithLabelValues(scaleTypeStr).Set(0) - // Set the new scalingObject count - s.desiredScale.setValue(newCount) - err = s.scalingTarget.AdjustScalingObjectCount(s.scalingObject.Name, s.scalingObject.MinCount, s.scalingObject.MaxCount, currentCount, newCount) + err = s.scalingTarget.AdjustScalingObjectCount(s.scalingObject.name, s.scalingObject.minCount, s.scalingObject.maxCount, currentCount, newCount) if err != nil { return scaleResult{ state: scaleFailed, diff --git a/scaler/scale_test.go b/scaler/scale_test.go index 59964edb..6b88b6a6 100644 --- a/scaler/scale_test.go +++ b/scaler/scale_test.go @@ -55,7 +55,6 @@ func TestScale_Up(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(0), uint(2)).Return(nil) result := scaler.scale(2, 0, false) - assert.Equal(t, uint(2), scaler.desiredScale.value) assert.NotEqual(t, scaleFailed, result.state) // scale up - max hit @@ -65,7 +64,6 @@ func TestScale_Up(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(0), uint(5)).Return(nil) result = scaler.scale(6, 0, false) - assert.Equal(t, uint(5), scaler.desiredScale.value) assert.NotEqual(t, scaleFailed, result.state) } @@ -90,7 +88,6 @@ func TestScale_Down(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(4), uint(1)).Return(nil) result := scaler.scale(1, 4, false) - assert.Equal(t, uint(1), scaler.desiredScale.value) assert.NotEqual(t, scaleFailed, result.state) // scale up - min hit @@ -100,7 +97,6 @@ func TestScale_Down(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(2), uint(1)).Return(nil) result = scaler.scale(0, 2, false) - assert.Equal(t, uint(1), scaler.desiredScale.value) assert.NotEqual(t, scaleFailed, result.state) } @@ -220,6 +216,9 @@ func TestScale_DownDryRun(t *testing.T) { func TestScale_DryRun(t *testing.T) { + // TODO: Remove this comment: + // This test fails because of the scaleObjectWatcher, which calls GetScalingObjectCount too often + mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() metrics, mocks := NewMockedMetrics(mockCtrl) diff --git a/scaler/scalerImpl.go b/scaler/scalerImpl.go index 524cdbf5..8c85ad76 100644 --- a/scaler/scalerImpl.go +++ b/scaler/scalerImpl.go @@ -50,6 +50,10 @@ func (s *Scaler) scaleTicketProcessor(ticketChan <-chan ScalingTicket) { func (s *Scaler) applyScaleTicket(ticket ScalingTicket) { ticket.start() result := s.scaleTo(ticket.desiredCount, ticket.dryRun) + if err := updateDesiredScale(result, &s.desiredScale); err != nil { + s.logger.Error().Err(err).Msg("Failed updating desired scale.") + } + ticket.complete(result.state) s.numOpenScalingTickets-- @@ -62,6 +66,19 @@ func (s *Scaler) applyScaleTicket(ticket ScalingTicket) { s.logger.Info().Msgf("Ticket applied. Scaling was %s (%s). New count is %d. Scaling in %f .", result.state, result.stateDescription, result.newCount, dur.Seconds()) } +func updateDesiredScale(sResult scaleResult, desiredScale *optionalValue) error { + if desiredScale == nil { + return fmt.Errorf("desiredScale parameter is nil") + } + + if sResult.state != scaleDone { + return nil + } + + desiredScale.setValue(sResult.newCount) + return nil +} + func updateScaleResultMetric(result scaleResult, scaleResultCounter m.CounterVec) { switch result.state { diff --git a/scaler/scaler_test.go b/scaler/scaler_test.go index 58b3111e..27a0534a 100644 --- a/scaler/scaler_test.go +++ b/scaler/scaler_test.go @@ -134,7 +134,7 @@ func Test_OpenScalingTicket(t *testing.T) { assert.Equal(t, uint(0), ticket.desiredCount) } -func Test_ApplyScalingTicket(t *testing.T) { +func Test_ApplyScalingTicket_NoScale_DeadJob(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() metrics, mocks := NewMockedMetrics(mockCtrl) @@ -158,8 +158,47 @@ func Test_ApplyScalingTicket(t *testing.T) { mocks.scaleResultCounter.EXPECT().WithLabelValues("ignored").Return(ignoredCounter) mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()) - ticket := NewScalingTicket(0, false) + ticket := NewScalingTicket(10, false) scaler.applyScaleTicket(ticket) + assert.False(t, scaler.desiredScale.isKnown) + assert.Equal(t, uint(0), scaler.desiredScale.value) +} + +// TODO: Add this test +// func Test_ApplyScalingTicket_NoScale_DryRun(t *testing.T) { + +func Test_ApplyScalingTicket_Scale(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + metrics, mocks := NewMockedMetrics(mockCtrl) + + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) + plannedButSkippedGauge.EXPECT().Set(float64(0)) + scalingTicketCounter := mock_metrics.NewMockCounter(mockCtrl) + scalingTicketCounter.EXPECT().Inc() + doneCounter := mock_metrics.NewMockCounter(mockCtrl) + doneCounter.EXPECT().Inc() + + gomock.InOrder( + scaTgt.EXPECT().GetScalingObjectCount("any").Return(uint(0), nil), + scaTgt.EXPECT().IsScalingObjectDead("any").Return(false, nil), + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), + scaTgt.EXPECT().AdjustScalingObjectCount("any", uint(1), uint(10), uint(0), uint(5)).Return(nil), + mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(scalingTicketCounter), + mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), + mocks.scaleResultCounter.EXPECT().WithLabelValues("done").Return(doneCounter), + ) + + cfg := Config{Name: "any", WatcherInterval: time.Second * 5, MinCount: 1, MaxCount: 10} + scaler, err := cfg.New(scaTgt, metrics) + require.NoError(t, err) + require.NotNil(t, scaler) + + ticket := NewScalingTicket(5, false) + scaler.applyScaleTicket(ticket) + assert.True(t, scaler.desiredScale.isKnown) + assert.Equal(t, uint(5), scaler.desiredScale.value) } func Test_OpenAndApplyScalingTicket(t *testing.T) { @@ -211,3 +250,27 @@ func Test_OpenAndApplyScalingTicket(t *testing.T) { err = scaler.openScalingTicket(0, false) assert.NoError(t, err) } + +func Test_UpdateDesiredScale(t *testing.T) { + err := updateDesiredScale(scaleResult{}, nil) + assert.Error(t, err) + + desiredScale := optionalValue{} + err = updateDesiredScale(scaleResult{}, &desiredScale) + assert.NoError(t, err) + assert.False(t, desiredScale.isKnown) + assert.Equal(t, uint(0), desiredScale.value) + + desiredScale = optionalValue{} + desiredScale.setValue(10) + err = updateDesiredScale(scaleResult{}, &desiredScale) + assert.NoError(t, err) + assert.True(t, desiredScale.isKnown) + assert.Equal(t, uint(10), desiredScale.value) + + desiredScale = optionalValue{} + err = updateDesiredScale(scaleResult{state: scaleDone, newCount: 10}, &desiredScale) + assert.NoError(t, err) + assert.True(t, desiredScale.isKnown) + assert.Equal(t, uint(10), desiredScale.value) +} From 7aa085edd74fab191da9073f98f17252e5719332 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Wed, 9 Oct 2019 22:25:01 +0200 Subject: [PATCH 03/16] .Update docu for dry-run mode added --- doc/DryRunMode.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 doc/DryRunMode.md diff --git a/doc/DryRunMode.md b/doc/DryRunMode.md new file mode 100644 index 00000000..4d3894c8 --- /dev/null +++ b/doc/DryRunMode.md @@ -0,0 +1,10 @@ +# Dry Run Mode + +The following table shows how sokar behaves in case the dry run mode is activated. + +| Feature | Dry Run Mode Active | Dry Run Mode Deactivated | +| :-------------------------------------------------------------------------------------------------------------------------------------------------- | :----------------------------------------------------------------- | :----------------------- | +| Automatic Scaling | Deactivated | Active | +| Manual Scaling | Possible | Not Possible | +| ScaleObjectWatcher | Deactivated | Active | +| PlanedButSkippedScalingOpen
_(The metric `sokar_sca_planned_but_skipped_scaling_open`,
for more information see [Metrics.md](../Metrics.md))_ | Set to 1 if a scaling was skipped
Set to 0 after manual scaling | Stays 0 | From 93c492e3d3bbffaf1d5ef535895d14a185081f46 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Wed, 9 Oct 2019 22:53:17 +0200 Subject: [PATCH 04/16] .Refactor removed needless code --- scaler/scale.go | 20 --------- scaler/scale_test.go | 100 ++++++++++++++++++++----------------------- sokar/sokarImpl.go | 1 + 3 files changed, 47 insertions(+), 74 deletions(-) diff --git a/scaler/scale.go b/scaler/scale.go index 6a9aa34a..3f9e2996 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -9,7 +9,6 @@ import ( // scaleState represents the state of a scaling type scaleState string -// TODO: Check which states are still needed and which can be removed const ( // scaleUnknown means the scale process was completed successfully scaleUnknown scaleState = "unknown" @@ -70,28 +69,9 @@ func checkScalingPolicy(desiredCount uint, min uint, max uint) policyCheckResult return result } -// trueIfNil returns a scaleResult filled in with an appropriate error message in case the given scaler is nil -func trueIfNil(s *Scaler) (result scaleResult, ok bool) { - ok = false - result = scaleResult{state: scaleUnknown} - - if s == nil { - ok = true - result = scaleResult{ - state: scaleFailed, - stateDescription: "Scaler is nil", - newCount: 0, - } - } - return result, ok -} - // scale scales the scalingObject from currentCount to desiredCount. // Internally it is checked if a scaling is needed and if the scaling policy is valid. func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleResult { - if r, ok := trueIfNil(s); ok { - return r - } sObjName := s.scalingObject.Name min := s.scalingObject.MinCount diff --git a/scaler/scale_test.go b/scaler/scale_test.go index 6b88b6a6..3269ffde 100644 --- a/scaler/scale_test.go +++ b/scaler/scale_test.go @@ -159,15 +159,6 @@ func TestScaleBy_CheckScalingPolicy(t *testing.T) { assert.True(t, chk.maxPolicyViolated) } -func TestScaleBy_trueIfNil(t *testing.T) { - _, ok := trueIfNil(nil) - assert.True(t, ok) - - scaler := &Scaler{} - _, ok = trueIfNil(scaler) - assert.False(t, ok) -} - func TestScale_UpDryRun(t *testing.T) { mockCtrl := gomock.NewController(t) @@ -214,48 +205,49 @@ func TestScale_DownDryRun(t *testing.T) { assert.NotEqual(t, scaleFailed, result.state) } -func TestScale_DryRun(t *testing.T) { - - // TODO: Remove this comment: - // This test fails because of the scaleObjectWatcher, which calls GetScalingObjectCount too often - - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - metrics, mocks := NewMockedMetrics(mockCtrl) - - scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) - - sObjName := "any" - cfg := Config{Name: sObjName, MinCount: 1, MaxCount: 5, WatcherInterval: time.Millisecond * 100} - scaler, err := cfg.New(scaTgt, metrics) - require.NoError(t, err) - - addedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) - addedScalingTickets.EXPECT().Inc() - plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) - plannedButSkippedGauge.EXPECT().Set(float64(1)) - appliedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) - appliedScalingTickets.EXPECT().Inc() - ignoredCounter := mock_metrics.NewMockCounter(mockCtrl) - ignoredCounter.EXPECT().Inc() - gomock.InOrder( - mocks.scalingTicketCount.EXPECT().WithLabelValues("added").Return(addedScalingTickets), - scaTgt.EXPECT().GetScalingObjectCount(sObjName).Return(uint(1), nil), - scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil), - mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), - mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(appliedScalingTickets), - mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), - mocks.scaleResultCounter.EXPECT().WithLabelValues("ignored").Return(ignoredCounter), - ) - scaler.Run() - defer func() { - scaler.Stop() - scaler.Join() - }() - - err = scaler.ScaleTo(2, true) - assert.NoError(t, err) - - // needed to give the ticketprocessor some time to start working - time.Sleep(time.Second * 1) -} +//func TestScale_DryRun(t *testing.T) { +// +// // TODO: Remove this comment: +// // This test fails because of the scaleObjectWatcher, which calls GetScalingObjectCount too often +// +// mockCtrl := gomock.NewController(t) +// defer mockCtrl.Finish() +// metrics, mocks := NewMockedMetrics(mockCtrl) +// +// scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) +// +// sObjName := "any" +// cfg := Config{Name: sObjName, MinCount: 1, MaxCount: 5, WatcherInterval: time.Millisecond * 100} +// scaler, err := cfg.New(scaTgt, metrics) +// require.NoError(t, err) +// +// addedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) +// addedScalingTickets.EXPECT().Inc() +// plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) +// plannedButSkippedGauge.EXPECT().Set(float64(1)) +// appliedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) +// appliedScalingTickets.EXPECT().Inc() +// ignoredCounter := mock_metrics.NewMockCounter(mockCtrl) +// ignoredCounter.EXPECT().Inc() +// gomock.InOrder( +// mocks.scalingTicketCount.EXPECT().WithLabelValues("added").Return(addedScalingTickets), +// scaTgt.EXPECT().GetScalingObjectCount(sObjName).Return(uint(1), nil), +// scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil), +// mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), +// mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(appliedScalingTickets), +// mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), +// mocks.scaleResultCounter.EXPECT().WithLabelValues("ignored").Return(ignoredCounter), +// ) +// scaler.Run() +// defer func() { +// scaler.Stop() +// scaler.Join() +// }() +// +// err = scaler.ScaleTo(2, true) +// assert.NoError(t, err) +// +// // needed to give the ticketprocessor some time to start working +// time.Sleep(time.Second * 1) +//} +// diff --git a/sokar/sokarImpl.go b/sokar/sokarImpl.go index 7d9adf86..f7ed8581 100644 --- a/sokar/sokarImpl.go +++ b/sokar/sokarImpl.go @@ -67,6 +67,7 @@ func (sk *Sokar) triggerScale(dryRunOnly bool, scaleValue float32, planFun func( plannedJobCount := planFun(scaleValue, preScaleJobCount) sk.metrics.plannedJobCount.Set(float64(plannedJobCount)) + // TODO: Move this into the scaler. The scaler should provide the information about the last scale. if !dryRunOnly { sk.lastScaleAction = time.Now() } From 5964f1f591307a75dfe6e256198a532df98317f3 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Wed, 9 Oct 2019 23:12:20 +0200 Subject: [PATCH 05/16] .Refactor split scale method into execution an preparation part --- scaler/scale.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scaler/scale.go b/scaler/scale.go index 3f9e2996..a277c594 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -117,6 +117,15 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR } } + return s.executeScale(currentCount, newCount, dryRun) +} + +func (s *Scaler) executeScale(currentCount, newCount uint, dryRun bool) scaleResult { + sObjName := s.scalingObject.name + min := s.scalingObject.minCount + max := s.scalingObject.maxCount + + diff := helper.SubUint(newCount, currentCount) scaleTypeStr := amountToScaleType(diff) if dryRun { @@ -133,7 +142,7 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR s.logger.Info().Str("scalingObject", sObjName).Msgf("Scale %s by %d to %d.", scaleTypeStr, diff, newCount) s.metrics.plannedButSkippedScalingOpen.WithLabelValues(scaleTypeStr).Set(0) - err = s.scalingTarget.AdjustScalingObjectCount(s.scalingObject.name, s.scalingObject.minCount, s.scalingObject.maxCount, currentCount, newCount) + err := s.scalingTarget.AdjustScalingObjectCount(sObjName, min, max, currentCount, newCount) if err != nil { return scaleResult{ state: scaleFailed, From 639180fc995780fc381b8bb62d4f4f708ecf3588 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 00:36:37 +0200 Subject: [PATCH 06/16] .Fix needed adjustments after rebase --- scaler/scale.go | 6 +++--- scaler/scaler.go | 10 ---------- scaler/scaler_test.go | 12 ++++++------ 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/scaler/scale.go b/scaler/scale.go index a277c594..9cc58083 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -121,9 +121,9 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR } func (s *Scaler) executeScale(currentCount, newCount uint, dryRun bool) scaleResult { - sObjName := s.scalingObject.name - min := s.scalingObject.minCount - max := s.scalingObject.maxCount + sObjName := s.scalingObject.Name + min := s.scalingObject.MinCount + max := s.scalingObject.MaxCount diff := helper.SubUint(newCount, currentCount) scaleTypeStr := amountToScaleType(diff) diff --git a/scaler/scaler.go b/scaler/scaler.go index 82a007a5..2da711fc 100644 --- a/scaler/scaler.go +++ b/scaler/scaler.go @@ -48,16 +48,6 @@ type Scaler struct { scalingObjectWatcherPaused bool } -// Config is the configuration for the Scaler -type Config struct { - Name string - MinCount uint - MaxCount uint - Logger zerolog.Logger - MaxOpenScalingTickets uint - WatcherInterval time.Duration -} - // Option represents an option for the Scaler type Option func(c *Scaler) diff --git a/scaler/scaler_test.go b/scaler/scaler_test.go index 27a0534a..8531635b 100644 --- a/scaler/scaler_test.go +++ b/scaler/scaler_test.go @@ -179,19 +179,19 @@ func Test_ApplyScalingTicket_Scale(t *testing.T) { scalingTicketCounter.EXPECT().Inc() doneCounter := mock_metrics.NewMockCounter(mockCtrl) doneCounter.EXPECT().Inc() + sObjName := "any" gomock.InOrder( - scaTgt.EXPECT().GetScalingObjectCount("any").Return(uint(0), nil), - scaTgt.EXPECT().IsScalingObjectDead("any").Return(false, nil), + scaTgt.EXPECT().GetScalingObjectCount(sObjName).Return(uint(0), nil), + scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil), mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), - scaTgt.EXPECT().AdjustScalingObjectCount("any", uint(1), uint(10), uint(0), uint(5)).Return(nil), + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(10), uint(0), uint(5)).Return(nil), mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(scalingTicketCounter), mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), mocks.scaleResultCounter.EXPECT().WithLabelValues("done").Return(doneCounter), ) - - cfg := Config{Name: "any", WatcherInterval: time.Second * 5, MinCount: 1, MaxCount: 10} - scaler, err := cfg.New(scaTgt, metrics) + sObj := ScalingObject{Name: sObjName, MinCount: 1, MaxCount: 10} + scaler, err := New(scaTgt, sObj, metrics) require.NoError(t, err) require.NotNil(t, scaler) From 6bc7e94fc3db7a479294c2deaa92a690ef5b2c51 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 00:44:50 +0200 Subject: [PATCH 07/16] .Update the scaler now has a dry-run mode flag --- main.go | 5 +++-- main_test.go | 8 ++++---- scaler/scaler.go | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/main.go b/main.go index 7f721899..31a80a16 100644 --- a/main.go +++ b/main.go @@ -63,7 +63,7 @@ func main() { logger.Info().Msgf("Scaling Target: %s", scalingTarget.String()) logger.Info().Msg("5. Setup: Scaler") - scaler := helper.Must(setupScaler(cfg.ScaleObject.Name, cfg.ScaleObject.MinCount, cfg.ScaleObject.MaxCount, cfg.Scaler.WatcherInterval, scalingTarget, loggingFactory)).(*scaler.Scaler) + scaler := helper.Must(setupScaler(cfg.ScaleObject.Name, cfg.ScaleObject.MinCount, cfg.ScaleObject.MaxCount, cfg.Scaler.WatcherInterval, scalingTarget, loggingFactory, cfg.DryRunMode)).(*scaler.Scaler) logger.Info().Msg("6. Setup: CapacityPlanner") @@ -259,7 +259,7 @@ func setupScalingTarget(cfg config.Scaler, logF logging.LoggerFactory) (scaler.S } // setupScaler creates and configures the Scaler. Internally nomad is used as scaling target. -func setupScaler(scalingObjName string, min uint, max uint, watcherInterval time.Duration, scalingTarget scaler.ScalingTarget, logF logging.LoggerFactory) (*scaler.Scaler, error) { +func setupScaler(scalingObjName string, min uint, max uint, watcherInterval time.Duration, scalingTarget scaler.ScalingTarget, logF logging.LoggerFactory, dryRunMode bool) (*scaler.Scaler, error) { if logF == nil { return nil, fmt.Errorf("Logging factory is nil") @@ -276,6 +276,7 @@ func setupScaler(scalingObjName string, min uint, max uint, watcherInterval time scaler.NewMetrics(), scaler.WithLogger(logF.NewNamedLogger("sokar.scaler")), scaler.WatcherInterval(watcherInterval), + scaler.DryRunMode(dryRunMode), ) if err != nil { return nil, fmt.Errorf("Failed setting up scaler: %s", err) diff --git a/main_test.go b/main_test.go index 26838f52..cdf4722b 100644 --- a/main_test.go +++ b/main_test.go @@ -53,16 +53,16 @@ func Test_SetupScaler_Failures(t *testing.T) { logF := mock_logging.NewMockLoggerFactory(mockCtrl) // no logging factory - scaler, err := setupScaler("any", 0, 1, time.Second*1, nil, nil) + scaler, err := setupScaler("any", 0, 1, time.Second*1, nil, nil, false) assert.Error(t, err) assert.Nil(t, scaler) - scaler, err = setupScaler("any", 0, 1, time.Second*1, nil, logF) + scaler, err = setupScaler("any", 0, 1, time.Second*1, nil, logF, false) assert.Error(t, err) assert.Nil(t, scaler) // invalid watcher-interval - scaler, err = setupScaler("any", 0, 1, time.Second*0, nil, nil) + scaler, err = setupScaler("any", 0, 1, time.Second*0, nil, nil, false) assert.Error(t, err) assert.Nil(t, scaler) } @@ -75,7 +75,7 @@ func Test_SetupScaler(t *testing.T) { scalingTarget := mock_scaler.NewMockScalingTarget(mockCtrl) logF.EXPECT().NewNamedLogger(gomock.Any()).Times(1) - scaler, err := setupScaler("any", 0, 1, time.Second*1, scalingTarget, logF) + scaler, err := setupScaler("any", 0, 1, time.Second*1, scalingTarget, logF, false) assert.NoError(t, err) assert.NotNil(t, scaler) } diff --git a/scaler/scaler.go b/scaler/scaler.go index 2da711fc..e6cc4a1b 100644 --- a/scaler/scaler.go +++ b/scaler/scaler.go @@ -19,6 +19,10 @@ type Scaler struct { // ScalingObject represents the ScalingObject and relevant meta data scalingObject ScalingObject + // dryRunMode active/ not active. In dry run mode no automatic scaling will + // executed. For more information see ../doc/DryRunMode.md + dryRunMode bool + // watcherInterval the interval the Scaler will check if // the scalingObject count still matches the desired state. watcherInterval time.Duration @@ -73,6 +77,15 @@ func WatcherInterval(interval time.Duration) Option { } } +// DryRunMode can be used to activate/ deactivate the dry run mode. +// In dry run mode no automatic scaling will executed. +// For more information see ../doc/DryRunMode.md +func DryRunMode(enable bool) Option { + return func(s *Scaler) { + s.dryRunMode = enable + } +} + // New creates a new instance of a scaler using the given // ScalingTarget to send scaling events to. func New(scalingTarget ScalingTarget, scalingObject ScalingObject, metrics Metrics, options ...Option) (*Scaler, error) { @@ -87,6 +100,7 @@ func New(scalingTarget ScalingTarget, scalingObject ScalingObject, metrics Metri maxOpenScalingTickets: maxOpenScalingTickets, metrics: metrics, desiredScale: optionalValue{isKnown: false}, + dryRunMode: false, } // apply the options From 95fe56f71658a22abc32052ee12c6c74a7de7f97 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 01:19:25 +0200 Subject: [PATCH 08/16] .Update added way to overrule if scaler is in dry-run mode --- scaler/scale.go | 23 +++-- scaler/scale_test.go | 159 +++++++++++++++++++++-------------- scaler/scaler.go | 6 +- scaler/scalerImpl.go | 48 +++++------ scaler/scalingTicket.go | 12 +-- scaler/scalingTicket_test.go | 4 +- 6 files changed, 150 insertions(+), 102 deletions(-) diff --git a/scaler/scale.go b/scaler/scale.go index 9cc58083..a93be371 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -71,7 +71,8 @@ func checkScalingPolicy(desiredCount uint, min uint, max uint) policyCheckResult // scale scales the scalingObject from currentCount to desiredCount. // Internally it is checked if a scaling is needed and if the scaling policy is valid. -func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleResult { +// If the force flag is true then even in dry-run mode the scaling will be applied. +func (s *Scaler) scale(desiredCount uint, currentCount uint, force bool) scaleResult { sObjName := s.scalingObject.Name min := s.scalingObject.MinCount @@ -84,6 +85,7 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR return scaleResult{ state: scaleFailed, stateDescription: fmt.Sprintf("Error obtaining if scalingObject is dead: %s.", err.Error()), + newCount: currentCount, } } @@ -91,6 +93,7 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR return scaleResult{ state: scaleIgnored, stateDescription: fmt.Sprintf("ScalingObject '%s' is dead. Can't scale", sObjName), + newCount: currentCount, } } @@ -117,10 +120,16 @@ func (s *Scaler) scale(desiredCount uint, currentCount uint, dryRun bool) scaleR } } - return s.executeScale(currentCount, newCount, dryRun) + return s.executeScale(currentCount, newCount, force) } -func (s *Scaler) executeScale(currentCount, newCount uint, dryRun bool) scaleResult { +// executeScale just executes the wanted scale, even if it is not needed in case currentCount == newCount. +// The only thing that is checked is if the scaler is in dry run mode or not. +// In dry-run mode the scaling is not applied by actually scaling the scalingObject, only a metric is +// updated, that reflects the fact that a scaling was skipped/ ignored. +// Only the force flag can be used to override this behavior. If the force flag is true then even in +// dry-run mode the scaling will be applied. +func (s *Scaler) executeScale(currentCount, newCount uint, force bool) scaleResult { sObjName := s.scalingObject.Name min := s.scalingObject.MinCount max := s.scalingObject.MaxCount @@ -128,8 +137,9 @@ func (s *Scaler) executeScale(currentCount, newCount uint, dryRun bool) scaleRes diff := helper.SubUint(newCount, currentCount) scaleTypeStr := amountToScaleType(diff) - if dryRun { - s.logger.Info().Str("scalingObject", sObjName).Msgf("Skip scale %s by %d to %d (DryRun).", scaleTypeStr, diff, newCount) + // the force flag can overrule the dry run mode + if s.dryRunMode && !force { + s.logger.Info().Str("scalingObject", sObjName).Msgf("Skip scale %s by %d to %d (DryRun, force=%v).", scaleTypeStr, diff, newCount, force) s.metrics.plannedButSkippedScalingOpen.WithLabelValues(scaleTypeStr).Set(1) return scaleResult{ @@ -139,7 +149,7 @@ func (s *Scaler) executeScale(currentCount, newCount uint, dryRun bool) scaleRes } } - s.logger.Info().Str("scalingObject", sObjName).Msgf("Scale %s by %d to %d.", scaleTypeStr, diff, newCount) + s.logger.Info().Str("scalingObject", sObjName).Msgf("Scale %s by %d to %d (force=%v).", scaleTypeStr, diff, newCount, force) s.metrics.plannedButSkippedScalingOpen.WithLabelValues(scaleTypeStr).Set(0) err := s.scalingTarget.AdjustScalingObjectCount(sObjName, min, max, currentCount, newCount) @@ -147,6 +157,7 @@ func (s *Scaler) executeScale(currentCount, newCount uint, dryRun bool) scaleRes return scaleResult{ state: scaleFailed, stateDescription: fmt.Sprintf("Error adjusting scalingObject count to %d: %s.", newCount, err.Error()), + newCount: currentCount, } } diff --git a/scaler/scale_test.go b/scaler/scale_test.go index 3269ffde..ccfd9fa1 100644 --- a/scaler/scale_test.go +++ b/scaler/scale_test.go @@ -11,6 +11,81 @@ import ( mock_scaler "github.com/thomasobenaus/sokar/test/scaler" ) +func Test_ExecuteScale_NoDryRun(t *testing.T) { + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + sObjName := "any" + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + metrics, mocks := NewMockedMetrics(mockCtrl) + + plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) + plannedButSkippedGauge.EXPECT().Set(float64(0)).Times(3) + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge).Times(3) + + sObj := ScalingObject{Name: sObjName, MinCount: 0, MaxCount: 2} + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(false)) + require.NoError(t, err) + + // error + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(0), uint(2), uint(1), uint(1)).Return(fmt.Errorf("err")) + result := scaler.executeScale(1, 1, false) + assert.Equal(t, scaleFailed, result.state) + assert.Equal(t, uint(1), result.newCount) + + // no scale, success + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(0), uint(2), uint(1), uint(1)).Return(nil) + result = scaler.executeScale(1, 1, false) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(1), result.newCount) + + // scale up, success + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(0), uint(2), uint(1), uint(2)).Return(nil) + result = scaler.executeScale(1, 2, false) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(2), result.newCount) + + // scale down, success + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(0), uint(2), uint(2), uint(1)).Return(nil) + plannedButSkippedGauge.EXPECT().Set(float64(0)) + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("DOWN").Return(plannedButSkippedGauge) + result = scaler.executeScale(2, 1, false) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(1), result.newCount) +} + +func Test_ExecuteScale_DryRun(t *testing.T) { + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + sObjName := "any" + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + metrics, mocks := NewMockedMetrics(mockCtrl) + + plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) + plannedButSkippedGauge.EXPECT().Set(float64(1)) + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge) + + sObj := ScalingObject{Name: sObjName, MinCount: 0, MaxCount: 2} + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(true)) + require.NoError(t, err) + + // no scale, dry run + result := scaler.executeScale(1, 2, false) + assert.Equal(t, scaleIgnored, result.state) + assert.Equal(t, uint(1), result.newCount) + + // scale up, dry run but force + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(0), uint(2), uint(1), uint(2)).Return(nil) + plannedButSkippedGauge.EXPECT().Set(float64(0)) + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge) + result = scaler.executeScale(1, 2, true) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(2), result.newCount) +} + func TestScale_ScalingObjectDead(t *testing.T) { mockCtrl := gomock.NewController(t) @@ -25,13 +100,15 @@ func TestScale_ScalingObjectDead(t *testing.T) { // dead scalingObject - error scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, fmt.Errorf("internal error")) - result := scaler.scale(2, 0, false) + result := scaler.scale(2, 1, false) assert.Equal(t, scaleFailed, result.state) + assert.Equal(t, uint(1), result.newCount) // dead scalingObject scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(true, nil) - result = scaler.scale(2, 0, false) + result = scaler.scale(2, 1, false) assert.Equal(t, scaleIgnored, result.state) + assert.Equal(t, uint(1), result.newCount) } func TestScale_Up(t *testing.T) { @@ -55,7 +132,8 @@ func TestScale_Up(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(0), uint(2)).Return(nil) result := scaler.scale(2, 0, false) - assert.NotEqual(t, scaleFailed, result.state) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(2), result.newCount) // scale up - max hit polViolatedCounter := mock_metrics.NewMockCounter(mockCtrl) @@ -64,7 +142,8 @@ func TestScale_Up(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(0), uint(5)).Return(nil) result = scaler.scale(6, 0, false) - assert.NotEqual(t, scaleFailed, result.state) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(5), result.newCount) } func TestScale_Down(t *testing.T) { @@ -88,7 +167,8 @@ func TestScale_Down(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(4), uint(1)).Return(nil) result := scaler.scale(1, 4, false) - assert.NotEqual(t, scaleFailed, result.state) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(1), result.newCount) // scale up - min hit polViolatedCounter := mock_metrics.NewMockCounter(mockCtrl) @@ -97,7 +177,8 @@ func TestScale_Down(t *testing.T) { scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(2), uint(1)).Return(nil) result = scaler.scale(0, 2, false) - assert.NotEqual(t, scaleFailed, result.state) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(1), result.newCount) } func TestScale_NoScale(t *testing.T) { @@ -113,11 +194,10 @@ func TestScale_NoScale(t *testing.T) { scaler, err := New(scaTgt, sObj, metrics) require.NoError(t, err) - // scale down scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) result := scaler.scale(2, 2, false) - assert.False(t, scaler.desiredScale.isKnown) - assert.NotEqual(t, scaleFailed, result.state) + assert.Equal(t, scaleIgnored, result.state) + assert.Equal(t, uint(2), result.newCount) } func TestScaleBy_CheckScalingPolicy(t *testing.T) { @@ -169,7 +249,7 @@ func TestScale_UpDryRun(t *testing.T) { sObjName := "any" sObj := ScalingObject{Name: sObjName, MinCount: 1, MaxCount: 5} - scaler, err := New(scaTgt, sObj, metrics) + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(true)) require.NoError(t, err) plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) @@ -178,8 +258,9 @@ func TestScale_UpDryRun(t *testing.T) { // scale up scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) - result := scaler.scale(2, 0, true) - assert.NotEqual(t, scaleFailed, result.state) + result := scaler.scale(2, 1, false) + assert.Equal(t, scaleIgnored, result.state) + assert.Equal(t, uint(1), result.newCount) } func TestScale_DownDryRun(t *testing.T) { @@ -192,7 +273,7 @@ func TestScale_DownDryRun(t *testing.T) { sObjName := "any" sObj := ScalingObject{Name: sObjName, MinCount: 1, MaxCount: 5} - scaler, err := New(scaTgt, sObj, metrics) + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(true)) require.NoError(t, err) plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) @@ -201,53 +282,7 @@ func TestScale_DownDryRun(t *testing.T) { // scale down scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) - result := scaler.scale(1, 4, true) - assert.NotEqual(t, scaleFailed, result.state) + result := scaler.scale(1, 4, false) + assert.Equal(t, scaleIgnored, result.state) + assert.Equal(t, uint(4), result.newCount) } - -//func TestScale_DryRun(t *testing.T) { -// -// // TODO: Remove this comment: -// // This test fails because of the scaleObjectWatcher, which calls GetScalingObjectCount too often -// -// mockCtrl := gomock.NewController(t) -// defer mockCtrl.Finish() -// metrics, mocks := NewMockedMetrics(mockCtrl) -// -// scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) -// -// sObjName := "any" -// cfg := Config{Name: sObjName, MinCount: 1, MaxCount: 5, WatcherInterval: time.Millisecond * 100} -// scaler, err := cfg.New(scaTgt, metrics) -// require.NoError(t, err) -// -// addedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) -// addedScalingTickets.EXPECT().Inc() -// plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) -// plannedButSkippedGauge.EXPECT().Set(float64(1)) -// appliedScalingTickets := mock_metrics.NewMockCounter(mockCtrl) -// appliedScalingTickets.EXPECT().Inc() -// ignoredCounter := mock_metrics.NewMockCounter(mockCtrl) -// ignoredCounter.EXPECT().Inc() -// gomock.InOrder( -// mocks.scalingTicketCount.EXPECT().WithLabelValues("added").Return(addedScalingTickets), -// scaTgt.EXPECT().GetScalingObjectCount(sObjName).Return(uint(1), nil), -// scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil), -// mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), -// mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(appliedScalingTickets), -// mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), -// mocks.scaleResultCounter.EXPECT().WithLabelValues("ignored").Return(ignoredCounter), -// ) -// scaler.Run() -// defer func() { -// scaler.Stop() -// scaler.Join() -// }() -// -// err = scaler.ScaleTo(2, true) -// assert.NoError(t, err) -// -// // needed to give the ticketprocessor some time to start working -// time.Sleep(time.Second * 1) -//} -// diff --git a/scaler/scaler.go b/scaler/scaler.go index e6cc4a1b..2f043052 100644 --- a/scaler/scaler.go +++ b/scaler/scaler.go @@ -130,9 +130,9 @@ func (s *Scaler) GetCount() (uint, error) { } // ScaleTo will scale the scalingObject to the desired count. -func (s *Scaler) ScaleTo(desiredCount uint, dryRun bool) error { - s.logger.Info().Msgf("Scale to %d requested (dryRun=%t).", desiredCount, dryRun) - return s.openScalingTicket(desiredCount, dryRun) +func (s *Scaler) ScaleTo(desiredCount uint, force bool) error { + s.logger.Info().Msgf("Scale to %d requested (force=%t).", desiredCount, force) + return s.openScalingTicket(desiredCount, force) } // GetName returns the name of this component diff --git a/scaler/scalerImpl.go b/scaler/scalerImpl.go index 8c85ad76..c3ca89e5 100644 --- a/scaler/scalerImpl.go +++ b/scaler/scalerImpl.go @@ -46,26 +46,6 @@ func (s *Scaler) scaleTicketProcessor(ticketChan <-chan ScalingTicket) { s.logger.Info().Msg("ScaleTicketProcessor closed.") } -// applyScaleTicket applies the given ScalingTicket by issuing and tracking the scaling action. -func (s *Scaler) applyScaleTicket(ticket ScalingTicket) { - ticket.start() - result := s.scaleTo(ticket.desiredCount, ticket.dryRun) - if err := updateDesiredScale(result, &s.desiredScale); err != nil { - s.logger.Error().Err(err).Msg("Failed updating desired scale.") - } - - ticket.complete(result.state) - s.numOpenScalingTickets-- - - s.metrics.scalingTicketCount.WithLabelValues("applied").Inc() - - dur, _ := ticket.processingDuration() - s.metrics.scalingDurationSeconds.Observe(float64(dur.Seconds())) - updateScaleResultMetric(result, s.metrics.scaleResultCounter) - - s.logger.Info().Msgf("Ticket applied. Scaling was %s (%s). New count is %d. Scaling in %f .", result.state, result.stateDescription, result.newCount, dur.Seconds()) -} - func updateDesiredScale(sResult scaleResult, desiredScale *optionalValue) error { if desiredScale == nil { return fmt.Errorf("desiredScale parameter is nil") @@ -98,7 +78,7 @@ func updateScaleResultMetric(result scaleResult, scaleResultCounter m.CounterVec } // openScalingTicket opens based on the desired count a ScalingTicket -func (s *Scaler) openScalingTicket(desiredCount uint, dryRun bool) error { +func (s *Scaler) openScalingTicket(desiredCount uint, force bool) error { if s.numOpenScalingTickets > s.maxOpenScalingTickets { s.metrics.scalingTicketCount.WithLabelValues("rejected").Inc() @@ -110,11 +90,31 @@ func (s *Scaler) openScalingTicket(desiredCount uint, dryRun bool) error { s.metrics.scalingTicketCount.WithLabelValues("added").Inc() // TODO: Add metric "open scaling tickets" s.numOpenScalingTickets++ - s.scaleTicketChan <- NewScalingTicket(desiredCount, dryRun) + s.scaleTicketChan <- NewScalingTicket(desiredCount, force) return nil } -func (s *Scaler) scaleTo(desiredCount uint, dryRun bool) scaleResult { +// applyScaleTicket applies the given ScalingTicket by issuing and tracking the scaling action. +func (s *Scaler) applyScaleTicket(ticket ScalingTicket) { + ticket.start() + result := s.scaleTo(ticket.desiredCount, ticket.force) + if err := updateDesiredScale(result, &s.desiredScale); err != nil { + s.logger.Error().Err(err).Msg("Failed updating desired scale.") + } + + ticket.complete(result.state) + s.numOpenScalingTickets-- + + s.metrics.scalingTicketCount.WithLabelValues("applied").Inc() + + dur, _ := ticket.processingDuration() + s.metrics.scalingDurationSeconds.Observe(float64(dur.Seconds())) + updateScaleResultMetric(result, s.metrics.scaleResultCounter) + + s.logger.Info().Msgf("Ticket applied. Scaling was %s (%s). New count is %d. Scaling in %f .", result.state, result.stateDescription, result.newCount, dur.Seconds()) +} + +func (s *Scaler) scaleTo(desiredCount uint, force bool) scaleResult { scalingObjectName := s.scalingObject.Name currentCount, err := s.scalingTarget.GetScalingObjectCount(scalingObjectName) if err != nil { @@ -124,5 +124,5 @@ func (s *Scaler) scaleTo(desiredCount uint, dryRun bool) scaleResult { } } - return s.scale(desiredCount, currentCount, dryRun) + return s.scale(desiredCount, currentCount, force) } diff --git a/scaler/scalingTicket.go b/scaler/scalingTicket.go index 35feb500..7eea4061 100644 --- a/scaler/scalingTicket.go +++ b/scaler/scalingTicket.go @@ -18,23 +18,25 @@ type ScalingTicket struct { // (failed or successful) completedAt *time.Time - // If this flag is true, then no scaling is executed in the end. - // The scaler just checks against the scaling policy if a scaling would be needed. - dryRun bool + // In case the scaler is in dry-run mode usually the scaling is not applied by actually scaling the scalingObject. + // Only a metric is updated, that reflects the fact that a scaling was skipped/ ignored. + // With this force flag this behavior overridden. If the force flag is true then even in + // dry-run mode the scaling will be applied. + force bool desiredCount uint state scaleState } // NewScalingTicket creates and opens/ issues a new ScalingTicket -func NewScalingTicket(desiredCount uint, dryRun bool) ScalingTicket { +func NewScalingTicket(desiredCount uint, force bool) ScalingTicket { return ScalingTicket{ issuedAt: time.Now(), startedAt: nil, completedAt: nil, desiredCount: desiredCount, state: scaleNotStarted, - dryRun: dryRun, + force: force, } } diff --git a/scaler/scalingTicket_test.go b/scaler/scalingTicket_test.go index ea739399..cdd3438b 100644 --- a/scaler/scalingTicket_test.go +++ b/scaler/scalingTicket_test.go @@ -15,9 +15,9 @@ func TestNewScalingTicket(t *testing.T) { assert.Equal(t, uint(0), sj.desiredCount) assert.Nil(t, sj.startedAt) assert.Nil(t, sj.completedAt) - assert.False(t, sj.dryRun) + assert.False(t, sj.force) sj = NewScalingTicket(0, true) - assert.True(t, sj.dryRun) + assert.True(t, sj.force) } func Test_Start(t *testing.T) { From 8e220a17c024df10e3da55f394c0a2439ce815ac Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 01:58:38 +0200 Subject: [PATCH 09/16] .Update moved decision if scale is allowed into separate method --- scaler/scale.go | 7 ++++++- scaler/scale_test.go | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/scaler/scale.go b/scaler/scale.go index a93be371..fb69d004 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -2,6 +2,7 @@ package scaler import ( "fmt" + "time" "github.com/thomasobenaus/sokar/helper" ) @@ -138,7 +139,7 @@ func (s *Scaler) executeScale(currentCount, newCount uint, force bool) scaleResu scaleTypeStr := amountToScaleType(diff) // the force flag can overrule the dry run mode - if s.dryRunMode && !force { + if !isScalePermitted(s.dryRunMode, force) { s.logger.Info().Str("scalingObject", sObjName).Msgf("Skip scale %s by %d to %d (DryRun, force=%v).", scaleTypeStr, diff, newCount, force) s.metrics.plannedButSkippedScalingOpen.WithLabelValues(scaleTypeStr).Set(1) @@ -167,3 +168,7 @@ func (s *Scaler) executeScale(currentCount, newCount uint, force bool) scaleResu newCount: newCount, } } + +func isScalePermitted(dryRun, force bool) bool { + return !dryRun || force +} diff --git a/scaler/scale_test.go b/scaler/scale_test.go index ccfd9fa1..77bd5e0a 100644 --- a/scaler/scale_test.go +++ b/scaler/scale_test.go @@ -286,3 +286,10 @@ func TestScale_DownDryRun(t *testing.T) { assert.Equal(t, scaleIgnored, result.state) assert.Equal(t, uint(4), result.newCount) } + +func Test_IsScalePermitted(t *testing.T) { + assert.True(t, isScalePermitted(true, true)) + assert.True(t, isScalePermitted(false, true)) + assert.True(t, isScalePermitted(false, false)) + assert.False(t, isScalePermitted(true, false)) +} From 992dc7195920af8cd183a964f9ae568edd5a4bd1 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 01:59:33 +0200 Subject: [PATCH 10/16] .Update moved knowledge about last scaling action into scaler --- scaler/scale.go | 5 +++++ scaler/scale_test.go | 32 ++++++++++++++++++++++++++++ scaler/scaler.go | 14 ++++++++++++ scaler/scaler_test.go | 3 +++ sokar/iface/scaler_IF.go | 5 ++++- sokar/scaleBy_test.go | 5 +++++ sokar/sokar.go | 9 -------- sokar/sokarImpl.go | 11 +++------- sokar/sokar_test.go | 41 +++++++++++++++++++++++++++++++++--- test/sokar/mock_scaler_IF.go | 23 ++++++++++++++++---- 10 files changed, 123 insertions(+), 25 deletions(-) diff --git a/scaler/scale.go b/scaler/scale.go index fb69d004..18593859 100644 --- a/scaler/scale.go +++ b/scaler/scale.go @@ -75,6 +75,11 @@ func checkScalingPolicy(desiredCount uint, min uint, max uint) policyCheckResult // If the force flag is true then even in dry-run mode the scaling will be applied. func (s *Scaler) scale(desiredCount uint, currentCount uint, force bool) scaleResult { + // memorize the time the scaling started + if isScalePermitted(s.dryRunMode, force) { + s.lastScaleAction = time.Now() + } + sObjName := s.scalingObject.Name min := s.scalingObject.MinCount max := s.scalingObject.MaxCount diff --git a/scaler/scale_test.go b/scaler/scale_test.go index 77bd5e0a..1d0dac4a 100644 --- a/scaler/scale_test.go +++ b/scaler/scale_test.go @@ -3,6 +3,7 @@ package scaler import ( "fmt" "testing" + "time" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -261,6 +262,35 @@ func TestScale_UpDryRun(t *testing.T) { result := scaler.scale(2, 1, false) assert.Equal(t, scaleIgnored, result.state) assert.Equal(t, uint(1), result.newCount) + oneDayAgo := time.Now().Add(time.Hour * -24) + assert.WithinDuration(t, oneDayAgo, scaler.lastScaleAction, time.Second*1) +} + +func TestScale_DryRunForce(t *testing.T) { + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + metrics, mocks := NewMockedMetrics(mockCtrl) + + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + + sObjName := "any" + sObj := ScalingObject{Name: sObjName, MinCount: 1, MaxCount: 5} + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(true)) + require.NoError(t, err) + + plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) + plannedButSkippedGauge.EXPECT().Set(float64(0)) + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge) + scaTgt.EXPECT().AdjustScalingObjectCount(sObjName, uint(1), uint(5), uint(1), uint(2)).Return(nil) + + // scale up + scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil) + result := scaler.scale(2, 1, true) + assert.Equal(t, scaleDone, result.state) + assert.Equal(t, uint(2), result.newCount) + fiveMsAgo := time.Now().Add(time.Millisecond * -5) + assert.WithinDuration(t, fiveMsAgo, scaler.lastScaleAction, time.Second*1) } func TestScale_DownDryRun(t *testing.T) { @@ -285,6 +315,8 @@ func TestScale_DownDryRun(t *testing.T) { result := scaler.scale(1, 4, false) assert.Equal(t, scaleIgnored, result.state) assert.Equal(t, uint(4), result.newCount) + oneDayAgo := time.Now().Add(time.Hour * -24) + assert.WithinDuration(t, oneDayAgo, scaler.lastScaleAction, time.Second*1) } func Test_IsScalePermitted(t *testing.T) { diff --git a/scaler/scaler.go b/scaler/scaler.go index 2f043052..e754c9d6 100644 --- a/scaler/scaler.go +++ b/scaler/scaler.go @@ -8,6 +8,8 @@ import ( "github.com/rs/zerolog" ) +var oneDayAgo = time.Now().Add(time.Hour * -24) + // Scaler is a component responsible for scaling a scalingObject type Scaler struct { logger zerolog.Logger @@ -23,6 +25,11 @@ type Scaler struct { // executed. For more information see ../doc/DryRunMode.md dryRunMode bool + // LastScaleAction represents that point in time + // when the scaler was triggered to execute a scaling + // action the last time + lastScaleAction time.Time + // watcherInterval the interval the Scaler will check if // the scalingObject count still matches the desired state. watcherInterval time.Duration @@ -101,6 +108,7 @@ func New(scalingTarget ScalingTarget, scalingObject ScalingObject, metrics Metri metrics: metrics, desiredScale: optionalValue{isKnown: false}, dryRunMode: false, + lastScaleAction: oneDayAgo, } // apply the options @@ -162,3 +170,9 @@ func (s *Scaler) Stop() error { func (s *Scaler) Join() { s.wg.Wait() } + +// GetTimeOfLastScaleAction returns that point in time where the most recent +// scaling STARTED. +func (s *Scaler) GetTimeOfLastScaleAction() time.Time { + return time.Now() +} diff --git a/scaler/scaler_test.go b/scaler/scaler_test.go index 8531635b..7a1cfc83 100644 --- a/scaler/scaler_test.go +++ b/scaler/scaler_test.go @@ -30,6 +30,9 @@ func Test_New(t *testing.T) { assert.NotNil(t, scaler.stopChan) assert.NotNil(t, scaler.scaleTicketChan) assert.NotNil(t, scaler.scalingTarget) + + oneDayAgo := time.Now().Add(time.Hour * -24) + assert.WithinDuration(t, oneDayAgo, scaler.lastScaleAction, time.Second*1) } func Test_GetCount(t *testing.T) { diff --git a/sokar/iface/scaler_IF.go b/sokar/iface/scaler_IF.go index 764de553..77c5cefc 100644 --- a/sokar/iface/scaler_IF.go +++ b/sokar/iface/scaler_IF.go @@ -1,7 +1,10 @@ package sokar +import "time" + // Scaler is a component that is able to scale a scaling-object type Scaler interface { - ScaleTo(count uint, dryRun bool) error + ScaleTo(count uint, force bool) error GetCount() (uint, error) + GetTimeOfLastScaleAction() time.Time } diff --git a/sokar/scaleBy_test.go b/sokar/scaleBy_test.go index 1b51ce96..4edd7956 100644 --- a/sokar/scaleBy_test.go +++ b/sokar/scaleBy_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/golang/mock/gomock" "github.com/julienschmidt/httprouter" @@ -94,6 +95,7 @@ func Test_ScaleByPercentage_HTTPHandler_OK(t *testing.T) { params := []httprouter.Param{httprouter.Param{Key: PathPartValue, Value: "10"}} gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(nil), ) @@ -129,6 +131,7 @@ func Test_ScaleByPercentage_HTTPHandler_IntError(t *testing.T) { params := []httprouter.Param{httprouter.Param{Key: PathPartValue, Value: "10"}} gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(fmt.Errorf("Failed to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), @@ -196,6 +199,7 @@ func Test_ScaleByValue_HTTPHandler_OK(t *testing.T) { params := []httprouter.Param{httprouter.Param{Key: PathPartValue, Value: "10"}} gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(nil), ) @@ -231,6 +235,7 @@ func Test_ScaleByValue_HTTPHandler_IntError(t *testing.T) { params := []httprouter.Param{httprouter.Param{Key: PathPartValue, Value: "10"}} gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(fmt.Errorf("Failed to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), diff --git a/sokar/sokar.go b/sokar/sokar.go index cdc48279..94fe96bb 100644 --- a/sokar/sokar.go +++ b/sokar/sokar.go @@ -3,14 +3,11 @@ package sokar import ( "fmt" "sync" - "time" "github.com/rs/zerolog" sokarIF "github.com/thomasobenaus/sokar/sokar/iface" ) -var oneDayAgo = time.Now().Add(time.Hour * -24) - // Sokar component that can be used to scale scaling-objects (jobs /instances). type Sokar struct { logger zerolog.Logger @@ -25,11 +22,6 @@ type Sokar struct { // the needed commands to the scaling target (i.e. nomad) scaler sokarIF.Scaler - // LastScaleAction represents that point in time - // when the scaler was triggered to execute a scaling - // action the last time - lastScaleAction time.Time - // metrics is a collection of metrics used by the sokar metrics Metrics @@ -71,7 +63,6 @@ func (cfg *Config) New(scaleEventEmitter sokarIF.ScaleEventEmitter, capacityPlan stopChan: make(chan struct{}, 1), metrics: metrics, logger: cfg.Logger, - lastScaleAction: oneDayAgo, dryRunMode: cfg.DryRunMode, }, nil } diff --git a/sokar/sokarImpl.go b/sokar/sokarImpl.go index f7ed8581..27bd7160 100644 --- a/sokar/sokarImpl.go +++ b/sokar/sokarImpl.go @@ -2,7 +2,6 @@ package sokar import ( "fmt" - "time" sokarIF "github.com/thomasobenaus/sokar/sokar/iface" ) @@ -45,7 +44,7 @@ func (sk *Sokar) handleScaleEvent(scaleEvent sokarIF.ScaleEvent) { } } -func (sk *Sokar) triggerScale(dryRunOnly bool, scaleValue float32, planFun func(scaleValue float32, currentScale uint) uint) error { +func (sk *Sokar) triggerScale(force bool, scaleValue float32, planFun func(scaleValue float32, currentScale uint) uint) error { scaleDown := scaleValueToScaleDir(scaleValue) @@ -57,7 +56,7 @@ func (sk *Sokar) triggerScale(dryRunOnly bool, scaleValue float32, planFun func( sk.metrics.preScaleJobCount.Set(float64(preScaleJobCount)) // Don't scale if sokar is in cool down mode - if sk.capacityPlanner.IsCoolingDown(sk.lastScaleAction, scaleDown) { + if sk.capacityPlanner.IsCoolingDown(sk.scaler.GetTimeOfLastScaleAction(), scaleDown) { sk.metrics.skippedScalingDuringCooldownTotal.Inc() sk.logger.Info().Msg("Skip scale event. Sokar is cooling down.") return nil @@ -67,11 +66,7 @@ func (sk *Sokar) triggerScale(dryRunOnly bool, scaleValue float32, planFun func( plannedJobCount := planFun(scaleValue, preScaleJobCount) sk.metrics.plannedJobCount.Set(float64(plannedJobCount)) - // TODO: Move this into the scaler. The scaler should provide the information about the last scale. - if !dryRunOnly { - sk.lastScaleAction = time.Now() - } - err = sk.scaler.ScaleTo(plannedJobCount, dryRunOnly) + err = sk.scaler.ScaleTo(plannedJobCount, force) // HACK: For now we ignore all rejected scaling tickets if err != nil { diff --git a/sokar/sokar_test.go b/sokar/sokar_test.go index 8a4eb9e2..4190453f 100644 --- a/sokar/sokar_test.go +++ b/sokar/sokar_test.go @@ -33,9 +33,6 @@ func Test_New(t *testing.T) { assert.NotNil(t, sokar.scaler) assert.NotNil(t, sokar.stopChan) assert.NotNil(t, sokar.metrics) - - oneDayAgo := time.Now().Add(time.Hour * -24) - assert.WithinDuration(t, oneDayAgo, sokar.lastScaleAction, time.Second*1) } func Test_HandleScaleEvent(t *testing.T) { @@ -59,6 +56,7 @@ func Test_HandleScaleEvent(t *testing.T) { event := sokarIF.ScaleEvent{ScaleFactor: scaleFactor} gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), capaPlannerIF.EXPECT().Plan(scaleFactor, uint(0)).Return(scaleTo), scalerIF.EXPECT().ScaleTo(scaleTo, false), @@ -117,6 +115,7 @@ func Test_TriggerScale_Scale(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), metricMocks.plannedJobCount.EXPECT().Set(float64(scaleTo)), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(nil), @@ -150,6 +149,7 @@ func Test_TriggerScale_Cooldown(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(true), metricMocks.skippedScalingDuringCooldownTotal.EXPECT().Inc(), ) @@ -161,6 +161,40 @@ func Test_TriggerScale_Cooldown(t *testing.T) { sokar.triggerScale(false, scaleFactor, planFunc) } +func Test_TriggerScale_NoCooldown(t *testing.T) { + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + evEmitterIF := mock_sokar.NewMockScaleEventEmitter(mockCtrl) + scalerIF := mock_sokar.NewMockScaler(mockCtrl) + capaPlannerIF := mock_sokar.NewMockCapacityPlanner(mockCtrl) + metrics, metricMocks := NewMockedMetrics(mockCtrl) + + cfg := Config{} + sokar, err := cfg.New(evEmitterIF, capaPlannerIF, scalerIF, metrics) + require.NotNil(t, sokar) + require.NoError(t, err) + + currentScale := uint(0) + scaleFactor := float32(1) + scaleTo := uint(1) + gomock.InOrder( + scalerIF.EXPECT().GetCount().Return(currentScale, nil), + metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now().Add(time.Hour*-1)), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + metricMocks.plannedJobCount.EXPECT().Set(float64(1)), + scalerIF.EXPECT().ScaleTo(scaleTo, false), + ) + + planFunc := func(scaleValue float32, currentScale uint) uint { + return scaleTo + } + + sokar.triggerScale(false, scaleFactor, planFunc) +} + func Test_TriggerScale_ErrGettingJobCount(t *testing.T) { mockCtrl := gomock.NewController(t) @@ -212,6 +246,7 @@ func Test_TriggerScale_ErrScaleTo(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), + scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), metricMocks.plannedJobCount.EXPECT().Set(float64(scaleTo)), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(fmt.Errorf("Unable to scale")), diff --git a/test/sokar/mock_scaler_IF.go b/test/sokar/mock_scaler_IF.go index 287a9adb..3245c62f 100644 --- a/test/sokar/mock_scaler_IF.go +++ b/test/sokar/mock_scaler_IF.go @@ -7,6 +7,7 @@ package mock_sokar import ( gomock "github.com/golang/mock/gomock" reflect "reflect" + time "time" ) // MockScaler is a mock of Scaler interface @@ -33,17 +34,17 @@ func (m *MockScaler) EXPECT() *MockScalerMockRecorder { } // ScaleTo mocks base method -func (m *MockScaler) ScaleTo(count uint, dryRun bool) error { +func (m *MockScaler) ScaleTo(count uint, force bool) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ScaleTo", count, dryRun) + ret := m.ctrl.Call(m, "ScaleTo", count, force) ret0, _ := ret[0].(error) return ret0 } // ScaleTo indicates an expected call of ScaleTo -func (mr *MockScalerMockRecorder) ScaleTo(count, dryRun interface{}) *gomock.Call { +func (mr *MockScalerMockRecorder) ScaleTo(count, force interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScaleTo", reflect.TypeOf((*MockScaler)(nil).ScaleTo), count, dryRun) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScaleTo", reflect.TypeOf((*MockScaler)(nil).ScaleTo), count, force) } // GetCount mocks base method @@ -60,3 +61,17 @@ func (mr *MockScalerMockRecorder) GetCount() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCount", reflect.TypeOf((*MockScaler)(nil).GetCount)) } + +// GetTimeOfLastScaleAction mocks base method +func (m *MockScaler) GetTimeOfLastScaleAction() time.Time { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetTimeOfLastScaleAction") + ret0, _ := ret[0].(time.Time) + return ret0 +} + +// GetTimeOfLastScaleAction indicates an expected call of GetTimeOfLastScaleAction +func (mr *MockScalerMockRecorder) GetTimeOfLastScaleAction() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTimeOfLastScaleAction", reflect.TypeOf((*MockScaler)(nil).GetTimeOfLastScaleAction)) +} From a69b96ac9fbab7fb3f828f4b8750db2170152f53 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 02:38:56 +0200 Subject: [PATCH 11/16] .Update renamed dryRun in force --- sokar/scaleBy.go | 6 ++++-- sokar/scaleBy_test.go | 8 ++++---- sokar/sokarImpl.go | 3 ++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/sokar/scaleBy.go b/sokar/scaleBy.go index 81a8e51a..716b791a 100644 --- a/sokar/scaleBy.go +++ b/sokar/scaleBy.go @@ -30,7 +30,8 @@ func (sk *Sokar) ScaleByPercentage(w http.ResponseWriter, r *http.Request, ps ht } percentageFract := float32(percentage) / 100.00 - err = sk.triggerScale(false, percentageFract, planScaleByPercentage) + // this is used in manual (override) mode --> force has to be true + err = sk.triggerScale(true, percentageFract, planScaleByPercentage) if err != nil { sk.logger.Error().Err(err).Msg("Unable to trigger scale") http.Error(w, err.Error(), http.StatusInternalServerError) @@ -60,7 +61,8 @@ func (sk *Sokar) ScaleByValue(w http.ResponseWriter, r *http.Request, ps httprou return } - err = sk.triggerScale(false, float32(value), planScaleByValue) + // this is used in manual (override) mode --> force has to be true + err = sk.triggerScale(true, float32(value), planScaleByValue) if err != nil { sk.logger.Error().Err(err).Msg("Unable to trigger scale") http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/sokar/scaleBy_test.go b/sokar/scaleBy_test.go index 4edd7956..35310e44 100644 --- a/sokar/scaleBy_test.go +++ b/sokar/scaleBy_test.go @@ -97,7 +97,7 @@ func Test_ScaleByPercentage_HTTPHandler_OK(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), - scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(nil), + scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(nil), ) metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)) metricMocks.plannedJobCount.EXPECT().Set(float64(scaleTo)) @@ -133,7 +133,7 @@ func Test_ScaleByPercentage_HTTPHandler_IntError(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), - scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(fmt.Errorf("Failed to scale")), + scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(fmt.Errorf("Failed to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), ) metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)) @@ -201,7 +201,7 @@ func Test_ScaleByValue_HTTPHandler_OK(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), - scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(nil), + scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(nil), ) metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)) metricMocks.plannedJobCount.EXPECT().Set(float64(scaleTo)) @@ -237,7 +237,7 @@ func Test_ScaleByValue_HTTPHandler_IntError(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), - scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(fmt.Errorf("Failed to scale")), + scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(fmt.Errorf("Failed to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), ) metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)) diff --git a/sokar/sokarImpl.go b/sokar/sokarImpl.go index 27bd7160..73babb4c 100644 --- a/sokar/sokarImpl.go +++ b/sokar/sokarImpl.go @@ -38,7 +38,8 @@ func (sk *Sokar) handleScaleEvent(scaleEvent sokarIF.ScaleEvent) { sk.metrics.scaleEventsTotal.Inc() sk.metrics.scaleFactor.Set(float64(scaleFactor)) - err := sk.triggerScale(sk.dryRunMode, scaleFactor, sk.capacityPlanner.Plan) + // this method is used for automatic mode only --> force has to be false + err := sk.triggerScale(false, scaleFactor, sk.capacityPlanner.Plan) if err != nil { sk.logger.Error().Err(err).Msg("Failed to scale.") } From 0cda6086440afb91c9ca216250fd2edddc76e9a8 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Thu, 10 Oct 2019 02:59:19 +0200 Subject: [PATCH 12/16] .Update disabled ScaleObjectWatcher in dry run mode --- scaler/scaler.go | 10 +++++-- scaler/scaler_test.go | 66 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/scaler/scaler.go b/scaler/scaler.go index e754c9d6..91bf1de7 100644 --- a/scaler/scaler.go +++ b/scaler/scaler.go @@ -152,8 +152,14 @@ func (s *Scaler) GetName() string { func (s *Scaler) Run() { // handler that processes incoming scaling tickets go s.scaleTicketProcessor(s.scaleTicketChan) - // handler that checks periodically if the desired count is still valid - go s.scalingObjectWatcher(s.watcherInterval) + + if s.dryRunMode { + s.logger.Info().Msg("Don't start the ScalingObjectWatcher in dry-run mode.") + } else { + // handler that checks periodically if the desired count is still valid + go s.scalingObjectWatcher(s.watcherInterval) + s.logger.Info().Msg("ScalingObjectWatcher started.") + } } // Stop tears down scaler diff --git a/scaler/scaler_test.go b/scaler/scaler_test.go index 7a1cfc83..3a385552 100644 --- a/scaler/scaler_test.go +++ b/scaler/scaler_test.go @@ -124,7 +124,7 @@ func Test_OpenScalingTicket(t *testing.T) { scalingTicketCounter := mock_metrics.NewMockCounter(mockCtrl) scalingTicketCounter.EXPECT().Inc().Times(2) mocks.scalingTicketCount.EXPECT().WithLabelValues("added").Return(scalingTicketCounter) - err = scaler.openScalingTicket(0, false) + err = scaler.openScalingTicket(1, false) assert.NoError(t, err) assert.Equal(t, uint(1), scaler.numOpenScalingTickets) assert.Len(t, scaler.scaleTicketChan, 1) @@ -134,7 +134,8 @@ func Test_OpenScalingTicket(t *testing.T) { assert.Error(t, err) ticket := <-scaler.scaleTicketChan - assert.Equal(t, uint(0), ticket.desiredCount) + assert.Equal(t, uint(1), ticket.desiredCount) + assert.False(t, ticket.force) } func Test_ApplyScalingTicket_NoScale_DeadJob(t *testing.T) { @@ -167,8 +168,65 @@ func Test_ApplyScalingTicket_NoScale_DeadJob(t *testing.T) { assert.Equal(t, uint(0), scaler.desiredScale.value) } -// TODO: Add this test -// func Test_ApplyScalingTicket_NoScale_DryRun(t *testing.T) { +func Test_ApplyScalingTicket_NoScale_DryRun(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + metrics, mocks := NewMockedMetrics(mockCtrl) + + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + plannedButSkippedGauge := mock_metrics.NewMockGauge(mockCtrl) + plannedButSkippedGauge.EXPECT().Set(float64(1)) + scalingTicketCounter := mock_metrics.NewMockCounter(mockCtrl) + scalingTicketCounter.EXPECT().Inc() + doneCounter := mock_metrics.NewMockCounter(mockCtrl) + doneCounter.EXPECT().Inc() + sObjName := "any" + + gomock.InOrder( + scaTgt.EXPECT().GetScalingObjectCount(sObjName).Return(uint(1), nil), + scaTgt.EXPECT().IsScalingObjectDead(sObjName).Return(false, nil), + mocks.plannedButSkippedScalingOpen.EXPECT().WithLabelValues("UP").Return(plannedButSkippedGauge), + mocks.scalingTicketCount.EXPECT().WithLabelValues("applied").Return(scalingTicketCounter), + mocks.scalingDurationSeconds.EXPECT().Observe(gomock.Any()), + mocks.scaleResultCounter.EXPECT().WithLabelValues("ignored").Return(doneCounter), + ) + sObj := ScalingObject{Name: sObjName, MinCount: 1, MaxCount: 10} + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(true)) + require.NoError(t, err) + require.NotNil(t, scaler) + + ticket := NewScalingTicket(5, false) + scaler.applyScaleTicket(ticket) + assert.False(t, scaler.desiredScale.isKnown) + assert.Equal(t, uint(0), scaler.desiredScale.value) +} + +func Test_ApplyScalingTicket_NoScaleObjectWatcherInDryRunMode(t *testing.T) { + // This test was added to ensure that the ScaleObjectWatcher does not + // run in dry-run mode. Why, see: https://github.com/ThomasObenaus/sokar/issues/98. + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + metrics, _ := NewMockedMetrics(mockCtrl) + + scaTgt := mock_scaler.NewMockScalingTarget(mockCtrl) + sObj := ScalingObject{Name: "any", MinCount: 1, MaxCount: 10} + scaler, err := New(scaTgt, sObj, metrics, DryRunMode(true), WatcherInterval(time.Millisecond*100)) + require.NoError(t, err) + require.NotNil(t, scaler) + + scaler.Run() + defer func() { + scaler.Stop() + scaler.Join() + }() + + // give the (potential) watcher some time + time.Sleep(time.Millisecond * 200) + + // hint: This test would fail in case a running ScaleObjectWatcher would + // call a method of the mocked ScalingTarget (e.g. GetCount or Scale) +} func Test_ApplyScalingTicket_Scale(t *testing.T) { mockCtrl := gomock.NewController(t) From dbbeb69956dd761d5cec1066638d08d264aa8b41 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Fri, 11 Oct 2019 00:24:40 +0200 Subject: [PATCH 13/16] .Update now the capacityplanner returns the cooldown time left --- capacityPlanner/capacityPlannerImpl.go | 6 +++--- capacityPlanner/capacityPlanner_test.go | 19 +++++++++++++------ sokar/iface/capacity_planner_IF.go | 2 +- sokar/sokarImpl.go | 4 ++-- test/sokar/mock_capacity_planner_IF.go | 5 +++-- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/capacityPlanner/capacityPlannerImpl.go b/capacityPlanner/capacityPlannerImpl.go index 5f0859bf..63a428be 100644 --- a/capacityPlanner/capacityPlannerImpl.go +++ b/capacityPlanner/capacityPlannerImpl.go @@ -23,7 +23,7 @@ func (cp *CapacityPlanner) Plan(scaleFactor float32, currentScale uint) uint { // IsCoolingDown returns true if the CapacityPlanner thinks that currently a new scaling // would not be a good idea. -func (cp *CapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) bool { +func (cp *CapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (bool, time.Duration) { now := time.Now() dur := cp.upScaleCooldownPeriod @@ -32,8 +32,8 @@ func (cp *CapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bo } if timeOfLastScale.Add(dur).After(now) { - return true + return true, timeOfLastScale.Add(dur).Sub(now) } - return false + return false, time.Second * 0 } diff --git a/capacityPlanner/capacityPlanner_test.go b/capacityPlanner/capacityPlanner_test.go index e9fa6690..51b67e19 100644 --- a/capacityPlanner/capacityPlanner_test.go +++ b/capacityPlanner/capacityPlanner_test.go @@ -1,6 +1,7 @@ package capacityPlanner import ( + "fmt" "testing" "time" @@ -101,27 +102,33 @@ func Test_IsCoolingDown(t *testing.T) { require.NotNil(t, capa) lastScale := time.Now() - result := capa.IsCoolingDown(lastScale, false) + result, timeLeft := capa.IsCoolingDown(lastScale, false) assert.True(t, result) + assert.InEpsilon(t, time.Second*10, timeLeft, 0.1, fmt.Sprintf("left %s", timeLeft.String())) - result = capa.IsCoolingDown(lastScale, true) + result, timeLeft = capa.IsCoolingDown(lastScale, true) assert.True(t, result) + assert.InEpsilon(t, time.Second*20, timeLeft, 0.1, fmt.Sprintf("left %s", timeLeft.String())) // Upscaling lastScale = time.Now().Add(time.Second * -11) - result = capa.IsCoolingDown(lastScale, false) + result, timeLeft = capa.IsCoolingDown(lastScale, false) assert.False(t, result) + assert.Equal(t, time.Second*0, timeLeft, fmt.Sprintf("left %s", timeLeft.String())) lastScale = time.Now().Add(time.Second * -9) - result = capa.IsCoolingDown(lastScale, false) + result, timeLeft = capa.IsCoolingDown(lastScale, false) assert.True(t, result) + assert.InEpsilon(t, time.Second*1, timeLeft, 0.1, fmt.Sprintf("left %s", timeLeft.String())) // Downscaling lastScale = time.Now().Add(time.Second * -21) - result = capa.IsCoolingDown(lastScale, true) + result, timeLeft = capa.IsCoolingDown(lastScale, true) assert.False(t, result) + assert.Equal(t, time.Second*0, timeLeft, fmt.Sprintf("left %s", timeLeft.String())) lastScale = time.Now().Add(time.Second * -19) - result = capa.IsCoolingDown(lastScale, true) + result, timeLeft = capa.IsCoolingDown(lastScale, true) assert.True(t, result) + assert.InEpsilon(t, time.Second*1, timeLeft, 0.1, fmt.Sprintf("left %s", timeLeft.String())) } diff --git a/sokar/iface/capacity_planner_IF.go b/sokar/iface/capacity_planner_IF.go index 4219eb8a..546e6b81 100644 --- a/sokar/iface/capacity_planner_IF.go +++ b/sokar/iface/capacity_planner_IF.go @@ -11,5 +11,5 @@ type CapacityPlanner interface { // IsCoolingDown returns true if the CapacityPlanner thinks that // its currently not a good idea to apply the wanted scaling event. - IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) bool + IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (bool, time.Duration) } diff --git a/sokar/sokarImpl.go b/sokar/sokarImpl.go index 73babb4c..14c43536 100644 --- a/sokar/sokarImpl.go +++ b/sokar/sokarImpl.go @@ -57,9 +57,9 @@ func (sk *Sokar) triggerScale(force bool, scaleValue float32, planFun func(scale sk.metrics.preScaleJobCount.Set(float64(preScaleJobCount)) // Don't scale if sokar is in cool down mode - if sk.capacityPlanner.IsCoolingDown(sk.scaler.GetTimeOfLastScaleAction(), scaleDown) { + if cooldown, timeleft := sk.capacityPlanner.IsCoolingDown(sk.scaler.GetTimeOfLastScaleAction(), scaleDown); cooldown { sk.metrics.skippedScalingDuringCooldownTotal.Inc() - sk.logger.Info().Msg("Skip scale event. Sokar is cooling down.") + sk.logger.Info().Msgf("Skip scale event. Sokar is cooling down (time left=%s).", timeleft.String()) return nil } diff --git a/test/sokar/mock_capacity_planner_IF.go b/test/sokar/mock_capacity_planner_IF.go index 11e87c93..3d4f37b2 100644 --- a/test/sokar/mock_capacity_planner_IF.go +++ b/test/sokar/mock_capacity_planner_IF.go @@ -48,11 +48,12 @@ func (mr *MockCapacityPlannerMockRecorder) Plan(scaleFactor, currentScale interf } // IsCoolingDown mocks base method -func (m *MockCapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) bool { +func (m *MockCapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (bool, time.Duration) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsCoolingDown", timeOfLastScale, scaleDown) ret0, _ := ret[0].(bool) - return ret0 + ret1, _ := ret[1].(time.Duration) + return ret0, ret1 } // IsCoolingDown indicates an expected call of IsCoolingDown From b469830836cd182231a960cc653bf2292d984aa3 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Fri, 11 Oct 2019 00:26:21 +0200 Subject: [PATCH 14/16] .Fix return the time of last scale action in GetTimeOfLastScaleAction instead of returning time.Now --- scaler/scaler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scaler/scaler.go b/scaler/scaler.go index 91bf1de7..c0df60d9 100644 --- a/scaler/scaler.go +++ b/scaler/scaler.go @@ -180,5 +180,5 @@ func (s *Scaler) Join() { // GetTimeOfLastScaleAction returns that point in time where the most recent // scaling STARTED. func (s *Scaler) GetTimeOfLastScaleAction() time.Time { - return time.Now() + return s.lastScaleAction } From aab4d4f7268979a145a683620a7115b9ede69071 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Fri, 11 Oct 2019 00:30:22 +0200 Subject: [PATCH 15/16] .Fix fixed tests --- sokar/scaleBy_test.go | 8 ++++---- sokar/sokar_test.go | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/sokar/scaleBy_test.go b/sokar/scaleBy_test.go index 35310e44..305b682b 100644 --- a/sokar/scaleBy_test.go +++ b/sokar/scaleBy_test.go @@ -96,7 +96,7 @@ func Test_ScaleByPercentage_HTTPHandler_OK(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(nil), ) metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)) @@ -132,7 +132,7 @@ func Test_ScaleByPercentage_HTTPHandler_IntError(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(fmt.Errorf("Failed to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), ) @@ -200,7 +200,7 @@ func Test_ScaleByValue_HTTPHandler_OK(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(nil), ) metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)) @@ -236,7 +236,7 @@ func Test_ScaleByValue_HTTPHandler_IntError(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), scalerIF.EXPECT().ScaleTo(scaleTo, true).Return(fmt.Errorf("Failed to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), ) diff --git a/sokar/sokar_test.go b/sokar/sokar_test.go index 4190453f..31b87312 100644 --- a/sokar/sokar_test.go +++ b/sokar/sokar_test.go @@ -57,7 +57,7 @@ func Test_HandleScaleEvent(t *testing.T) { gomock.InOrder( scalerIF.EXPECT().GetCount().Return(currentScale, nil), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), capaPlannerIF.EXPECT().Plan(scaleFactor, uint(0)).Return(scaleTo), scalerIF.EXPECT().ScaleTo(scaleTo, false), ) @@ -116,7 +116,7 @@ func Test_TriggerScale_Scale(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), metricMocks.plannedJobCount.EXPECT().Set(float64(scaleTo)), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(nil), ) @@ -150,7 +150,7 @@ func Test_TriggerScale_Cooldown(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(true), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(true, time.Second*0), metricMocks.skippedScalingDuringCooldownTotal.EXPECT().Inc(), ) @@ -183,7 +183,7 @@ func Test_TriggerScale_NoCooldown(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now().Add(time.Hour*-1)), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), metricMocks.plannedJobCount.EXPECT().Set(float64(1)), scalerIF.EXPECT().ScaleTo(scaleTo, false), ) @@ -247,7 +247,7 @@ func Test_TriggerScale_ErrScaleTo(t *testing.T) { scalerIF.EXPECT().GetCount().Return(currentScale, nil), metricMocks.preScaleJobCount.EXPECT().Set(float64(currentScale)), scalerIF.EXPECT().GetTimeOfLastScaleAction().Return(time.Now()), - capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false), + capaPlannerIF.EXPECT().IsCoolingDown(gomock.Any(), false).Return(false, time.Second*0), metricMocks.plannedJobCount.EXPECT().Set(float64(scaleTo)), scalerIF.EXPECT().ScaleTo(scaleTo, false).Return(fmt.Errorf("Unable to scale")), metricMocks.failedScalingTotal.EXPECT().Inc(), From 90ca036af0393086981b4ce5084fc66354dc2153 Mon Sep 17 00:00:00 2001 From: Thomas Obenaus Date: Fri, 11 Oct 2019 01:02:16 +0200 Subject: [PATCH 16/16] .Update docu added for the CapacityPlanner interface --- capacityPlanner/capacityPlannerImpl.go | 4 +++- sokar/iface/capacity_planner_IF.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/capacityPlanner/capacityPlannerImpl.go b/capacityPlanner/capacityPlannerImpl.go index 63a428be..60faad97 100644 --- a/capacityPlanner/capacityPlannerImpl.go +++ b/capacityPlanner/capacityPlannerImpl.go @@ -23,7 +23,7 @@ func (cp *CapacityPlanner) Plan(scaleFactor float32, currentScale uint) uint { // IsCoolingDown returns true if the CapacityPlanner thinks that currently a new scaling // would not be a good idea. -func (cp *CapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (bool, time.Duration) { +func (cp *CapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (cooldownActive bool, cooldownTimeLeft time.Duration) { now := time.Now() dur := cp.upScaleCooldownPeriod @@ -31,9 +31,11 @@ func (cp *CapacityPlanner) IsCoolingDown(timeOfLastScale time.Time, scaleDown bo dur = cp.downScaleCooldownPeriod } + // still cooling down if timeOfLastScale.Add(dur).After(now) { return true, timeOfLastScale.Add(dur).Sub(now) } + // not cooling down any more return false, time.Second * 0 } diff --git a/sokar/iface/capacity_planner_IF.go b/sokar/iface/capacity_planner_IF.go index 546e6b81..903ef3b9 100644 --- a/sokar/iface/capacity_planner_IF.go +++ b/sokar/iface/capacity_planner_IF.go @@ -11,5 +11,5 @@ type CapacityPlanner interface { // IsCoolingDown returns true if the CapacityPlanner thinks that // its currently not a good idea to apply the wanted scaling event. - IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (bool, time.Duration) + IsCoolingDown(timeOfLastScale time.Time, scaleDown bool) (cooldownActive bool, cooldownTimeLeft time.Duration) }