From 07e0a80216cb46d2ffd0a65802724ebadc1c75e1 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Sat, 24 Feb 2024 08:22:28 +0000 Subject: [PATCH 1/2] graduate HPAContainerMetrics to stable --- .../app/autoscaling.go | 3 - pkg/apis/autoscaling/validation/validation.go | 6 +- pkg/controller/podautoscaler/horizontal.go | 39 +++---- .../podautoscaler/horizontal_test.go | 56 +-------- pkg/features/kube_features.go | 3 +- .../horizontalpodautoscaler/strategy.go | 40 +------ .../horizontalpodautoscaler/strategy_test.go | 110 ------------------ 7 files changed, 24 insertions(+), 233 deletions(-) delete mode 100644 pkg/registry/autoscaling/horizontalpodautoscaler/strategy_test.go diff --git a/cmd/kube-controller-manager/app/autoscaling.go b/cmd/kube-controller-manager/app/autoscaling.go index 8e12f16ad9f7b..4d8ecd2830437 100644 --- a/cmd/kube-controller-manager/app/autoscaling.go +++ b/cmd/kube-controller-manager/app/autoscaling.go @@ -22,14 +22,12 @@ package app import ( "context" - "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/dynamic" "k8s.io/client-go/scale" "k8s.io/controller-manager/controller" "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/controller/podautoscaler" "k8s.io/kubernetes/pkg/controller/podautoscaler/metrics" - "k8s.io/kubernetes/pkg/features" resourceclient "k8s.io/metrics/pkg/client/clientset/versioned/typed/metrics/v1beta1" "k8s.io/metrics/pkg/client/custom_metrics" @@ -91,7 +89,6 @@ func startHPAControllerWithMetricsClient(ctx context.Context, controllerContext controllerContext.ComponentConfig.HPAController.HorizontalPodAutoscalerTolerance, controllerContext.ComponentConfig.HPAController.HorizontalPodAutoscalerCPUInitializationPeriod.Duration, controllerContext.ComponentConfig.HPAController.HorizontalPodAutoscalerInitialReadinessDelay.Duration, - feature.DefaultFeatureGate.Enabled(features.HPAContainerMetrics), ).Run(ctx, int(controllerContext.ComponentConfig.HPAController.ConcurrentHorizontalPodAutoscalerSyncs)) return nil, true, nil } diff --git a/pkg/apis/autoscaling/validation/validation.go b/pkg/apis/autoscaling/validation/validation.go index 516f795995c8f..bd986816cfd96 100644 --- a/pkg/apis/autoscaling/validation/validation.go +++ b/pkg/apis/autoscaling/validation/validation.go @@ -317,11 +317,7 @@ func validateMetricSpec(spec autoscaling.MetricSpec, fldPath *field.Path) field. expectedField = "external" case autoscaling.ContainerResourceMetricSourceType: if spec.ContainerResource == nil { - if utilfeature.DefaultFeatureGate.Enabled(features.HPAContainerMetrics) { - allErrs = append(allErrs, field.Required(fldPath.Child("containerResource"), "must populate information for the given metric source")) - } else { - allErrs = append(allErrs, field.Required(fldPath.Child("containerResource"), "must populate information for the given metric source (only allowed when HPAContainerMetrics feature is enabled)")) - } + allErrs = append(allErrs, field.Required(fldPath.Child("containerResource"), "must populate information for the given metric source")) } expectedField = "containerResource" default: diff --git a/pkg/controller/podautoscaler/horizontal.go b/pkg/controller/podautoscaler/horizontal.go index 282d54462ce75..5703be9eeaf3d 100644 --- a/pkg/controller/podautoscaler/horizontal.go +++ b/pkg/controller/podautoscaler/horizontal.go @@ -119,9 +119,6 @@ type HorizontalController struct { // Storage of HPAs and their selectors. hpaSelectors *selectors.BiMultimap hpaSelectorsMux sync.Mutex - - // feature gates - containerResourceMetricsEnabled bool } // NewHorizontalController creates a new HorizontalController. @@ -138,7 +135,6 @@ func NewHorizontalController( tolerance float64, cpuInitializationPeriod, delayOfInitialReadinessStatus time.Duration, - containerResourceMetricsEnabled bool, ) *HorizontalController { broadcaster := record.NewBroadcaster() broadcaster.StartStructuredLogging(0) @@ -146,21 +142,20 @@ func NewHorizontalController( recorder := broadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "horizontal-pod-autoscaler"}) hpaController := &HorizontalController{ - eventRecorder: recorder, - scaleNamespacer: scaleNamespacer, - hpaNamespacer: hpaNamespacer, - downscaleStabilisationWindow: downscaleStabilisationWindow, - monitor: monitor.New(), - queue: workqueue.NewNamedRateLimitingQueue(NewDefaultHPARateLimiter(resyncPeriod), "horizontalpodautoscaler"), - mapper: mapper, - recommendations: map[string][]timestampedRecommendation{}, - recommendationsLock: sync.Mutex{}, - scaleUpEvents: map[string][]timestampedScaleEvent{}, - scaleUpEventsLock: sync.RWMutex{}, - scaleDownEvents: map[string][]timestampedScaleEvent{}, - scaleDownEventsLock: sync.RWMutex{}, - hpaSelectors: selectors.NewBiMultimap(), - containerResourceMetricsEnabled: containerResourceMetricsEnabled, + eventRecorder: recorder, + scaleNamespacer: scaleNamespacer, + hpaNamespacer: hpaNamespacer, + downscaleStabilisationWindow: downscaleStabilisationWindow, + monitor: monitor.New(), + queue: workqueue.NewNamedRateLimitingQueue(NewDefaultHPARateLimiter(resyncPeriod), "horizontalpodautoscaler"), + mapper: mapper, + recommendations: map[string][]timestampedRecommendation{}, + recommendationsLock: sync.Mutex{}, + scaleUpEvents: map[string][]timestampedScaleEvent{}, + scaleUpEventsLock: sync.RWMutex{}, + scaleDownEvents: map[string][]timestampedScaleEvent{}, + scaleDownEventsLock: sync.RWMutex{}, + hpaSelectors: selectors.NewBiMultimap(), } hpaInformer.Informer().AddEventHandlerWithResyncPeriod( @@ -475,12 +470,6 @@ func (a *HorizontalController) computeReplicasForMetric(ctx context.Context, hpa return 0, "", time.Time{}, condition, fmt.Errorf("failed to get %s resource metric value: %v", spec.Resource.Name, err) } case autoscalingv2.ContainerResourceMetricSourceType: - if !a.containerResourceMetricsEnabled { - // If the container resource metrics feature is disabled but the object has the one, - // that means the user enabled the feature once, - // created some HPAs with the container resource metrics, and disabled it finally. - return 0, "", time.Time{}, condition, fmt.Errorf("ContainerResource metric type is not supported: disabled by the feature gate") - } replicaCountProposal, timestampProposal, metricNameProposal, condition, err = a.computeStatusForContainerResourceMetric(ctx, specReplicas, spec, hpa, selector, status) if err != nil { return 0, "", time.Time{}, condition, fmt.Errorf("failed to get %s container metric value: %v", spec.ContainerResource.Container, err) diff --git a/pkg/controller/podautoscaler/horizontal_test.go b/pkg/controller/podautoscaler/horizontal_test.go index 706df3112a9a0..9fb40dbb23867 100644 --- a/pkg/controller/podautoscaler/horizontal_test.go +++ b/pkg/controller/podautoscaler/horizontal_test.go @@ -156,8 +156,6 @@ type testCase struct { recommendations []timestampedRecommendation hpaSelectors *selectors.BiMultimap - - containerResourceMetricsEnabled bool } // Needs to be called under a lock. @@ -780,7 +778,6 @@ func (tc *testCase) setupController(t *testing.T) (*HorizontalController, inform defaultTestingTolerance, defaultTestingCPUInitializationPeriod, defaultTestingDelayOfInitialReadinessStatus, - tc.containerResourceMetricsEnabled, ) hpaController.hpaListerSynced = alwaysReady if tc.recommendations != nil { @@ -929,10 +926,9 @@ func TestScaleUpContainer(t *testing.T) { Container: "container1", }, }}, - reportedLevels: []uint64{300, 500, 700}, - reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, - useMetricsAPI: true, - containerResourceMetricsEnabled: true, + reportedLevels: []uint64{300, 500, 700}, + reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, + useMetricsAPI: true, expectedReportedReconciliationActionLabel: monitor.ActionLabelScaleUp, expectedReportedReconciliationErrorLabel: monitor.ErrorLabelNone, expectedReportedMetricComputationActionLabels: map[autoscalingv2.MetricSourceType]monitor.ActionLabel{ @@ -945,46 +941,6 @@ func TestScaleUpContainer(t *testing.T) { tc.runTest(t) } -func TestContainerMetricWithTheFeatureGateDisabled(t *testing.T) { - // In this test case, the container metrics will be ignored - // and only the CPUTarget will be taken into consideration. - - tc := testCase{ - minReplicas: 2, - maxReplicas: 6, - specReplicas: 3, - statusReplicas: 3, - expectedDesiredReplicas: 4, - CPUTarget: 30, - verifyCPUCurrent: true, - metricsTarget: []autoscalingv2.MetricSpec{{ - Type: autoscalingv2.ContainerResourceMetricSourceType, - ContainerResource: &autoscalingv2.ContainerResourceMetricSource{ - Name: v1.ResourceCPU, - Target: autoscalingv2.MetricTarget{ - Type: autoscalingv2.UtilizationMetricType, - AverageUtilization: pointer.Int32(10), - }, - Container: "container1", - }, - }}, - reportedLevels: []uint64{300, 400, 500}, - reportedCPURequests: []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")}, - expectedReportedReconciliationActionLabel: monitor.ActionLabelScaleUp, - expectedReportedReconciliationErrorLabel: monitor.ErrorLabelInternal, - expectedReportedMetricComputationActionLabels: map[autoscalingv2.MetricSourceType]monitor.ActionLabel{ - autoscalingv2.ResourceMetricSourceType: monitor.ActionLabelScaleUp, - autoscalingv2.ContainerResourceMetricSourceType: monitor.ActionLabelNone, - }, - expectedReportedMetricComputationErrorLabels: map[autoscalingv2.MetricSourceType]monitor.ErrorLabel{ - autoscalingv2.ResourceMetricSourceType: monitor.ErrorLabelNone, - autoscalingv2.ContainerResourceMetricSourceType: monitor.ErrorLabelInternal, - }, - } - - tc.runTest(t) -} - func TestScaleUpUnreadyLessScale(t *testing.T) { tc := testCase{ minReplicas: 2, @@ -1663,9 +1619,8 @@ func TestScaleDownContainerResource(t *testing.T) { }, }, }}, - useMetricsAPI: true, - containerResourceMetricsEnabled: true, - recommendations: []timestampedRecommendation{}, + useMetricsAPI: true, + recommendations: []timestampedRecommendation{}, expectedReportedReconciliationActionLabel: monitor.ActionLabelScaleDown, expectedReportedReconciliationErrorLabel: monitor.ErrorLabelNone, expectedReportedMetricComputationActionLabels: map[autoscalingv2.MetricSourceType]monitor.ActionLabel{ @@ -5305,7 +5260,6 @@ func TestMultipleHPAs(t *testing.T) { defaultTestingTolerance, defaultTestingCPUInitializationPeriod, defaultTestingDelayOfInitialReadinessStatus, - false, ) hpaController.scaleUpEvents = scaleUpEventsMap hpaController.scaleDownEvents = scaleDownEventsMap diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index f9f7c70e57c2a..69885ec3c5bae 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -280,6 +280,7 @@ const ( // kep: https://kep.k8s.io/1610 // alpha: v1.20 // beta: v1.27 + // beta: v1.30 // // Add support for the HPA to scale based on metrics from individual containers // in target pods @@ -994,7 +995,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS GracefulNodeShutdownBasedOnPodPriority: {Default: true, PreRelease: featuregate.Beta}, - HPAContainerMetrics: {Default: true, PreRelease: featuregate.Beta}, + HPAContainerMetrics: {Default: true, PreRelease: featuregate.GA}, HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go b/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go index 7e6fadf662dcf..59cc45f647bf4 100644 --- a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go +++ b/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go @@ -22,11 +22,9 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/apis/autoscaling" "k8s.io/kubernetes/pkg/apis/autoscaling/validation" - "k8s.io/kubernetes/pkg/features" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) @@ -67,16 +65,7 @@ func (autoscalerStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.S } // PrepareForCreate clears fields that are not allowed to be set by end users on creation. -func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { - newHPA := obj.(*autoscaling.HorizontalPodAutoscaler) - - // create cannot set status - newHPA.Status = autoscaling.HorizontalPodAutoscalerStatus{} - - if !utilfeature.DefaultFeatureGate.Enabled(features.HPAContainerMetrics) { - dropContainerMetricSources(newHPA.Spec.Metrics) - } -} +func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {} // Validate validates a new autoscaler. func (autoscalerStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { @@ -99,32 +88,7 @@ func (autoscalerStrategy) AllowCreateOnUpdate() bool { } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. -func (autoscalerStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { - newHPA := obj.(*autoscaling.HorizontalPodAutoscaler) - oldHPA := old.(*autoscaling.HorizontalPodAutoscaler) - if !utilfeature.DefaultFeatureGate.Enabled(features.HPAContainerMetrics) && !hasContainerMetricSources(oldHPA) { - dropContainerMetricSources(newHPA.Spec.Metrics) - } - // Update is not allowed to set status - newHPA.Status = oldHPA.Status -} - -// dropContainerMetricSources ensures all container resource metric sources are nil -func dropContainerMetricSources(metrics []autoscaling.MetricSpec) { - for i := range metrics { - metrics[i].ContainerResource = nil - } -} - -// hasContainerMetricSources returns true if the hpa has any container resource metric sources -func hasContainerMetricSources(hpa *autoscaling.HorizontalPodAutoscaler) bool { - for i := range hpa.Spec.Metrics { - if hpa.Spec.Metrics[i].ContainerResource != nil { - return true - } - } - return false -} +func (autoscalerStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {} // ValidateUpdate is the default update validation for an end user. func (autoscalerStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { diff --git a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy_test.go b/pkg/registry/autoscaling/horizontalpodautoscaler/strategy_test.go deleted file mode 100644 index 8bc41be06121d..0000000000000 --- a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy_test.go +++ /dev/null @@ -1,110 +0,0 @@ -/* -Copyright 2015 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package horizontalpodautoscaler - -import ( - "context" - "testing" - - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/kubernetes/pkg/apis/autoscaling" - "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/features" - "k8s.io/utils/pointer" -) - -func makeTestContainerMetricsHPA(hasContainerMetric bool) *autoscaling.HorizontalPodAutoscaler { - testHPA := &autoscaling.HorizontalPodAutoscaler{ - Spec: autoscaling.HorizontalPodAutoscalerSpec{ - Metrics: []autoscaling.MetricSpec{}, - }, - } - if hasContainerMetric { - testHPA.Spec.Metrics = append(testHPA.Spec.Metrics, autoscaling.MetricSpec{ - Type: autoscaling.ContainerResourceMetricSourceType, - ContainerResource: &autoscaling.ContainerResourceMetricSource{ - Name: core.ResourceCPU, - Container: "test-container", - Target: autoscaling.MetricTarget{ - Type: autoscaling.UtilizationMetricType, - AverageUtilization: pointer.Int32Ptr(30), - }, - }, - }) - } - return testHPA -} - -func TestCreateWithFeatureEnabled(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HPAContainerMetrics, true)() - testHPA := makeTestContainerMetricsHPA(true) - Strategy.PrepareForCreate(context.Background(), testHPA) - if testHPA.Spec.Metrics[0].ContainerResource == nil { - t.Errorf("container metrics was set to nil") - } -} - -func TestCreateWithFeatureDisabled(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HPAContainerMetrics, false)() - testHPA := makeTestContainerMetricsHPA(true) - Strategy.PrepareForCreate(context.Background(), testHPA) - if testHPA.Spec.Metrics[0].ContainerResource != nil { - t.Errorf("container metrics is not nil") - } -} - -func TestAutoscalerStatusStrategy_PrepareForUpdate(t *testing.T) { - for _, tc := range []struct { - name string - featureEnabled bool - old bool - expectedNew bool - }{ - { - name: "feature disabled with existing container metrics", - featureEnabled: false, - old: true, - expectedNew: true, - }, - { - name: "feature enabled with no container metrics", - featureEnabled: true, - old: false, - expectedNew: true, - }, - { - name: "feature enabled with existing container metrics", - featureEnabled: true, - old: true, - expectedNew: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HPAContainerMetrics, tc.featureEnabled)() - oldHPA := makeTestContainerMetricsHPA(tc.old) - newHPA := makeTestContainerMetricsHPA(true) - Strategy.PrepareForUpdate(context.Background(), newHPA, oldHPA) - if tc.expectedNew && newHPA.Spec.Metrics[0].ContainerResource == nil { - t.Errorf("container metric source is nil") - } - if !tc.expectedNew && newHPA.Spec.Metrics[0].ContainerResource != nil { - t.Errorf("container metric source is not nil") - } - }) - } -} From b48b4ebc6976aa9a373596ea6c36aa6f347ab567 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Sat, 2 Mar 2024 04:51:00 +0000 Subject: [PATCH 2/2] address reviews --- pkg/features/kube_features.go | 4 ++-- .../horizontalpodautoscaler/strategy.go | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 69885ec3c5bae..570f41836e07c 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -280,7 +280,7 @@ const ( // kep: https://kep.k8s.io/1610 // alpha: v1.20 // beta: v1.27 - // beta: v1.30 + // GA: v1.30 // // Add support for the HPA to scale based on metrics from individual containers // in target pods @@ -995,7 +995,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS GracefulNodeShutdownBasedOnPodPriority: {Default: true, PreRelease: featuregate.Beta}, - HPAContainerMetrics: {Default: true, PreRelease: featuregate.GA}, + HPAContainerMetrics: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go b/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go index 59cc45f647bf4..8558440f7aaad 100644 --- a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go +++ b/pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go @@ -65,7 +65,12 @@ func (autoscalerStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.S } // PrepareForCreate clears fields that are not allowed to be set by end users on creation. -func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {} +func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { + newHPA := obj.(*autoscaling.HorizontalPodAutoscaler) + + // create cannot set status + newHPA.Status = autoscaling.HorizontalPodAutoscalerStatus{} +} // Validate validates a new autoscaler. func (autoscalerStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { @@ -88,7 +93,12 @@ func (autoscalerStrategy) AllowCreateOnUpdate() bool { } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. -func (autoscalerStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {} +func (autoscalerStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { + newHPA := obj.(*autoscaling.HorizontalPodAutoscaler) + oldHPA := old.(*autoscaling.HorizontalPodAutoscaler) + // Update is not allowed to set status + newHPA.Status = oldHPA.Status +} // ValidateUpdate is the default update validation for an end user. func (autoscalerStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {