Merge pull request #123482 from sanposhiho/hpa-containerresource-grad…

…uation graduate HPAContainerMetrics to stable
kubernetes · Mar 7, 2024 · 2ec63e0 · 2ec63e0
2 parents c726b2b + b48b4eb
commit 2ec63e0
Show file tree

Hide file tree

Showing 7 changed files with 22 additions and 221 deletions.
diff --git 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"
@@ -92,7 +90,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
@@ -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
@@ -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.
@@ -139,29 +136,27 @@ func NewHorizontalController(
 	tolerance float64,
 	cpuInitializationPeriod,
 	delayOfInitialReadinessStatus time.Duration,
-	containerResourceMetricsEnabled bool,
 ) *HorizontalController {
 	broadcaster := record.NewBroadcaster(record.WithContext(ctx))
 	broadcaster.StartStructuredLogging(3)
 	broadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: evtNamespacer.Events("")})
 	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(
@@ -476,12 +471,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
@@ -157,8 +157,6 @@ type testCase struct {
 
 	recommendations []timestampedRecommendation
 	hpaSelectors    *selectors.BiMultimap
-
-	containerResourceMetricsEnabled bool
 }
 
 // Needs to be called under a lock.
@@ -783,7 +781,6 @@ func (tc *testCase) setupController(t *testing.T) (*HorizontalController, inform
 		defaultTestingTolerance,
 		defaultTestingCPUInitializationPeriod,
 		defaultTestingDelayOfInitialReadinessStatus,
-		tc.containerResourceMetricsEnabled,
 	)
 	hpaController.hpaListerSynced = alwaysReady
 	if tc.recommendations != nil {
@@ -932,10 +929,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{
@@ -948,46 +944,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,
@@ -1666,9 +1622,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{
@@ -5310,7 +5265,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
@@ -277,6 +277,7 @@ const (
 	// kep: https://kep.k8s.io/1610
 	// alpha: v1.20
 	// beta:  v1.27
+	// GA:  v1.30
 	//
 	// Add support for the HPA to scale based on metrics from individual containers
 	// in target pods
@@ -1046,7 +1047,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, 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
@@ -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"
 )
 
@@ -72,10 +70,6 @@ func (autoscalerStrategy) PrepareForCreate(ctx context.Context, obj runtime.Obje
 
 	// create cannot set status
 	newHPA.Status = autoscaling.HorizontalPodAutoscalerStatus{}
-
-	if !utilfeature.DefaultFeatureGate.Enabled(features.HPAContainerMetrics) {
-		dropContainerMetricSources(newHPA.Spec.Metrics)
-	}
 }
 
 // Validate validates a new autoscaler.
@@ -102,30 +96,10 @@ func (autoscalerStrategy) AllowCreateOnUpdate() bool {
 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
-}
-
 // ValidateUpdate is the default update validation for an end user.
 func (autoscalerStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
 	return validation.ValidateHorizontalPodAutoscalerUpdate(obj.(*autoscaling.HorizontalPodAutoscaler), old.(*autoscaling.HorizontalPodAutoscaler))

diff --git a/pkg/registry/autoscaling/horizontalpodautoscaler/strategy_test.go b/pkg/registry/autoscaling/horizontalpodautoscaler/strategy_test.go