From fef092b4172d6307592255765d45c0faa241eddb Mon Sep 17 00:00:00 2001 From: Mikkel Oscar Lyderik Larsen Date: Sat, 27 Feb 2021 00:29:19 +0100 Subject: [PATCH] hpa: Don't scale down if at least one metric was invalid Signed-off-by: Mikkel Oscar Lyderik Larsen --- pkg/controller/podautoscaler/horizontal.go | 6 +- .../podautoscaler/horizontal_test.go | 70 ++----------------- 2 files changed, 10 insertions(+), 66 deletions(-) diff --git a/pkg/controller/podautoscaler/horizontal.go b/pkg/controller/podautoscaler/horizontal.go index f83a50681ea..665542a12a7 100644 --- a/pkg/controller/podautoscaler/horizontal.go +++ b/pkg/controller/podautoscaler/horizontal.go @@ -288,8 +288,10 @@ func (a *HorizontalController) computeReplicasForMetrics(hpa *autoscalingv2.Hori } } - // If all metrics are invalid return error and set condition on hpa based on first invalid metric. - if invalidMetricsCount >= len(metricSpecs) { + // If all metrics are invalid or some are invalid and we would scale down, + // return an error and set the condition of the hpa based on the first invalid metric. + // Otherwise set the condition as scaling active as we're going to scale + if invalidMetricsCount >= len(metricSpecs) || (invalidMetricsCount > 0 && replicas < specReplicas) { setCondition(hpa, invalidMetricCondition.Type, invalidMetricCondition.Status, invalidMetricCondition.Reason, invalidMetricCondition.Message) return 0, "", statuses, time.Time{}, fmt.Errorf("invalid metrics (%v invalid out of %v), first error is: %v", invalidMetricsCount, len(metricSpecs), invalidMetricError) } diff --git a/pkg/controller/podautoscaler/horizontal_test.go b/pkg/controller/podautoscaler/horizontal_test.go index 347b306acad..01a34786f2e 100644 --- a/pkg/controller/podautoscaler/horizontal_test.go +++ b/pkg/controller/podautoscaler/horizontal_test.go @@ -1679,64 +1679,6 @@ func TestScaleDownIncludeUnreadyPods(t *testing.T) { tc.runTest(t) } -func TestScaleDownOneMetricInvalid(t *testing.T) { - tc := testCase{ - minReplicas: 2, - maxReplicas: 6, - specReplicas: 5, - statusReplicas: 5, - expectedDesiredReplicas: 3, - CPUTarget: 50, - metricsTarget: []autoscalingv2.MetricSpec{ - { - Type: "CheddarCheese", - }, - }, - reportedLevels: []uint64{100, 300, 500, 250, 250}, - reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, - useMetricsAPI: true, - recommendations: []timestampedRecommendation{}, - } - - tc.runTest(t) -} - -func TestScaleDownOneMetricEmpty(t *testing.T) { - tc := testCase{ - minReplicas: 2, - maxReplicas: 6, - specReplicas: 5, - statusReplicas: 5, - expectedDesiredReplicas: 3, - CPUTarget: 50, - metricsTarget: []autoscalingv2.MetricSpec{ - { - Type: autoscalingv2.ExternalMetricSourceType, - External: &autoscalingv2.ExternalMetricSource{ - Metric: autoscalingv2.MetricIdentifier{ - Name: "qps", - Selector: &metav1.LabelSelector{}, - }, - Target: autoscalingv2.MetricTarget{ - Type: autoscalingv2.AverageValueMetricType, - AverageValue: resource.NewMilliQuantity(1000, resource.DecimalSI), - }, - }, - }, - }, - reportedLevels: []uint64{100, 300, 500, 250, 250}, - reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, - useMetricsAPI: true, - recommendations: []timestampedRecommendation{}, - } - _, _, _, testEMClient, _ := tc.prepareTestClient(t) - testEMClient.PrependReactor("list", "*", func(action core.Action) (handled bool, ret runtime.Object, err error) { - return true, &emapi.ExternalMetricValueList{}, fmt.Errorf("something went wrong") - }) - tc.testEMClient = testEMClient - tc.runTest(t) -} - func TestScaleDownIgnoreHotCpuPods(t *testing.T) { tc := testCase{ minReplicas: 2, @@ -4113,10 +4055,10 @@ func TestNoScaleDownOneMetricInvalid(t *testing.T) { reportedLevels: []uint64{100, 300, 500, 250, 250}, reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, useMetricsAPI: true, + recommendations: []timestampedRecommendation{}, expectedConditions: []autoscalingv1.HorizontalPodAutoscalerCondition{ - {Type: autoscalingv1.AbleToScale, Status: v1.ConditionTrue, Reason: "ScaleDownStabilized"}, - {Type: autoscalingv1.ScalingActive, Status: v1.ConditionTrue, Reason: "ValidMetricFound"}, - {Type: autoscalingv1.ScalingLimited, Status: v1.ConditionFalse, Reason: "DesiredWithinRange"}, + {Type: autoscalingv1.AbleToScale, Status: v1.ConditionTrue, Reason: "SucceededGetScale"}, + {Type: autoscalingv1.ScalingActive, Status: v1.ConditionFalse, Reason: "InvalidMetricSourceType"}, }, } @@ -4148,10 +4090,10 @@ func TestNoScaleDownOneMetricEmpty(t *testing.T) { reportedLevels: []uint64{100, 300, 500, 250, 250}, reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, useMetricsAPI: true, + recommendations: []timestampedRecommendation{}, expectedConditions: []autoscalingv1.HorizontalPodAutoscalerCondition{ - {Type: autoscalingv1.AbleToScale, Status: v1.ConditionTrue, Reason: "ScaleDownStabilized"}, - {Type: autoscalingv1.ScalingActive, Status: v1.ConditionTrue, Reason: "ValidMetricFound"}, - {Type: autoscalingv1.ScalingLimited, Status: v1.ConditionFalse, Reason: "DesiredWithinRange"}, + {Type: autoscalingv1.AbleToScale, Status: v1.ConditionTrue, Reason: "SucceededGetScale"}, + {Type: autoscalingv1.ScalingActive, Status: v1.ConditionFalse, Reason: "FailedGetExternalMetric"}, }, } _, _, _, testEMClient, _ := tc.prepareTestClient(t)