From e42afb927b381f7000e40800e817865df2cb5aa9 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Mon, 2 Oct 2023 13:36:35 +0900 Subject: [PATCH] =?UTF-8?q?removal=E2=9A=A0=EF=B8=8F:=20remove=20the=20ext?= =?UTF-8?q?ernal=20metric=20support?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/v1alpha1/tortoise_webhook.go | 67 -------- controllers/tortoise_controller_test.go | 28 ++-- docs/horizontal.md | 56 +------ pkg/annotation/annotation.go | 3 - pkg/hpa/service.go | 147 ++-------------- pkg/hpa/service_test.go | 183 +------------------- pkg/recommender/recommender.go | 37 +--- pkg/recommender/recommender_test.go | 213 +----------------------- 8 files changed, 51 insertions(+), 683 deletions(-) diff --git a/api/v1alpha1/tortoise_webhook.go b/api/v1alpha1/tortoise_webhook.go index 3fbc2994..0dbc46da 100644 --- a/api/v1alpha1/tortoise_webhook.go +++ b/api/v1alpha1/tortoise_webhook.go @@ -30,19 +30,14 @@ import ( "errors" "fmt" - v2 "k8s.io/api/autoscaling/v2" - corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - "github.com/mercari/tortoise/pkg/annotation" ) // log is for logging in this package. @@ -176,20 +171,6 @@ func (r *Tortoise) ValidateCreate() (admission.Warnings, error) { return nil, fmt.Errorf("%s: tortoise should not have the policies for the container(s) which isn't defined in the deployment, but, it have the policy for the container(s) %v", fieldPath.Child("resourcePolicy"), uselessPolicies) } - hpa, err := ClientService.GetHPAFromUser(ctx, r) - if err != nil { - // Check if HPA really exists or not. - return nil, fmt.Errorf("failed to get the horizontal pod autoscaler defined in %s: %w", fieldPath.Child("targetRefs", "horizontalPodAutoscalerName"), err) - } - if hpa != nil { - for _, c := range containers.List() { - err = validateHPAAnnotations(hpa, c) - if err != nil { - return nil, fmt.Errorf("the horizontal pod autoscaler defined in %s is invalid: %w", fieldPath.Child("targetRefs", "horizontalPodAutoscalerName"), err) - } - } - } - return nil, validateTortoise(r) } @@ -230,51 +211,3 @@ func (r *Tortoise) ValidateDelete() (admission.Warnings, error) { tortoiselog.Info("validate delete", "name", r.Name) return nil, nil } - -func externalMetricNameFromAnnotation(hpa *v2.HorizontalPodAutoscaler, containerName string, k corev1.ResourceName) (string, bool, error) { - var prefix string - var ok bool - switch k { - case corev1.ResourceCPU: - prefix, ok = hpa.GetAnnotations()[annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation] - case corev1.ResourceMemory: - prefix, ok = hpa.GetAnnotations()[annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation] - default: - return "", false, fmt.Errorf("non supported resource type: %s", k) - } - return prefix + containerName, ok, nil -} - -func validateHPAAnnotations(hpa *v2.HorizontalPodAutoscaler, containerName string) error { - externalMetrics := sets.NewString() - for _, m := range hpa.Spec.Metrics { - if m.Type != v2.ExternalMetricSourceType { - continue - } - - if m.External == nil { - // shouldn't reach here - klog.ErrorS(nil, "invalid external metric on HPA", klog.KObj(hpa)) - continue - } - - externalMetrics.Insert(m.External.Metric.Name) - } - - resourceNames := []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory} - for _, rn := range resourceNames { - externalMetricName, ok, err := externalMetricNameFromAnnotation(hpa, containerName, rn) - if err != nil { - return err - } - if !ok { - continue - } - - if !externalMetrics.Has(externalMetricName) { - return fmt.Errorf("HPA doesn't have the external metrics which is defined in the annotations. (The annotation wants an external metric named %s)", externalMetricName) - } - } - - return nil -} diff --git a/controllers/tortoise_controller_test.go b/controllers/tortoise_controller_test.go index 0f18fd47..99576159 100644 --- a/controllers/tortoise_controller_test.go +++ b/controllers/tortoise_controller_test.go @@ -188,8 +188,8 @@ var _ = Describe("Test TortoiseController", func() { wantHPA.Spec.MinReplicas = pointer.Int32(5) wantHPA.Spec.MaxReplicas = 20 for i, m := range wantHPA.Spec.Metrics { - if m.Resource != nil && m.Resource.Name == corev1.ResourceCPU { - wantHPA.Spec.Metrics[i].Resource.Target.AverageUtilization = pointer.Int32(75) + if m.ContainerResource != nil && m.ContainerResource.Name == corev1.ResourceCPU && m.ContainerResource.Container == "app" { + wantHPA.Spec.Metrics[i].ContainerResource.Target.AverageUtilization = pointer.Int32(75) } } @@ -601,8 +601,8 @@ var _ = Describe("Test TortoiseController", func() { wantHPA.Spec.MinReplicas = pointer.Int32(20) wantHPA.Spec.MaxReplicas = 20 for i, m := range wantHPA.Spec.Metrics { - if m.Resource != nil && m.Resource.Name == corev1.ResourceCPU { - wantHPA.Spec.Metrics[i].Resource.Target.AverageUtilization = pointer.Int32(75) + if m.ContainerResource != nil && m.ContainerResource.Name == corev1.ResourceCPU && m.ContainerResource.Container == "app" { + wantHPA.Spec.Metrics[i].ContainerResource.Target.AverageUtilization = pointer.Int32(75) } } @@ -846,11 +846,11 @@ var _ = Describe("Test TortoiseController", func() { wantHPA.Spec.MinReplicas = pointer.Int32(5) wantHPA.Spec.MaxReplicas = 20 for i, m := range wantHPA.Spec.Metrics { - if m.External != nil && m.External.Metric.Name == "datadogmetric@default:mercari-app-cpu-app" { - wantHPA.Spec.Metrics[i].External.Target.Value = resourceQuantityPtr(resource.MustParse("50")) // won't get changed. + if m.ContainerResource != nil && m.ContainerResource.Name == corev1.ResourceCPU && m.ContainerResource.Container == "app" { + wantHPA.Spec.Metrics[i].ContainerResource.Target.AverageUtilization = pointer.Int32(50) // won't get changed. } - if m.External != nil && m.External.Metric.Name == "datadogmetric@default:mercari-app-cpu-istio-proxy" { - wantHPA.Spec.Metrics[i].External.Target.Value = resourceQuantityPtr(resource.MustParse("75")) + if m.ContainerResource != nil && m.ContainerResource.Name == corev1.ResourceCPU && m.ContainerResource.Container == "istio-proxy" { + wantHPA.Spec.Metrics[i].ContainerResource.Target.AverageUtilization = pointer.Int32(75) } } @@ -1151,11 +1151,11 @@ var _ = Describe("Test TortoiseController", func() { wantHPA.Spec.MinReplicas = pointer.Int32(20) wantHPA.Spec.MaxReplicas = 20 for i, m := range wantHPA.Spec.Metrics { - if m.External != nil && m.External.Metric.Name == "datadogmetric@default:mercari-app-cpu-app" { - wantHPA.Spec.Metrics[i].External.Target.Value = resourceQuantityPtr(resource.MustParse("50")) // won't get changed. + if m.ContainerResource != nil && m.ContainerResource.Name == corev1.ResourceCPU && m.ContainerResource.Container == "app" { + wantHPA.Spec.Metrics[i].ContainerResource.Target.AverageUtilization = pointer.Int32(50) // won't get changed. } - if m.External != nil && m.External.Metric.Name == "datadogmetric@default:mercari-app-cpu-istio-proxy" { - wantHPA.Spec.Metrics[i].External.Target.Value = resourceQuantityPtr(resource.MustParse("75")) + if m.ContainerResource != nil && m.ContainerResource.Name == corev1.ResourceCPU && m.ContainerResource.Container == "istio-proxy" { + wantHPA.Spec.Metrics[i].ContainerResource.Target.AverageUtilization = pointer.Int32(75) } } @@ -1793,10 +1793,6 @@ func multiContainerDeploymentWithReplicaNum(replica int32) *v1.Deployment { } } -func resourceQuantityPtr(quantity resource.Quantity) *resource.Quantity { - return &quantity -} - func deleteObj(ctx context.Context, deleteObj client.Object, name string) error { err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "default", Name: name}, deleteObj) if err != nil { diff --git a/docs/horizontal.md b/docs/horizontal.md index c9101dfe..79238b35 100644 --- a/docs/horizontal.md +++ b/docs/horizontal.md @@ -64,59 +64,9 @@ Looking back the above formula, Currently, Tortoise supports: - `type: Resource` metric if Pod has only one container. -- `type: ContainerResource` metric if Pod has only multiple containers. -- `type: External` metric if Pod has the annotations described below. - -##### `type: External` support (will be removed) - -※ This feature will be removed soon since the container resource metrics feature graduated to beta. - -Regarding the `External`, given [the container resource metrics](https://kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale/#container-resource-metrics) is still alpha as of v1.26, -some companies are using the external metrics to fetch the container's resource utilization. - -So, you can let Tortoise regard some external metrics as referring to the container's resource utilization. -You need to give the annotations to your HPA like: - -```yaml -apiVersion: autoscaling/v2 -kind: HorizontalPodAutoscaler -metadata: - name: example-hpa - namespace: example-prod - annotations: - tortoises.autoscaling.mercari.com/container-based-cpu-metric-prefix: "datadogmetric@example-prod:example-workload-cpu-" - tortoises.autoscaling.mercari.com/container-based-memory-metric-prefix: "datadogmetric@example-prod:example-workload-memory-" -spec: - metrics: - - type: External - external: - metric: - name: datadogmetric@example-prod:example-workload-cpu-app # CPU target of the container named "app" - target: - type: Value - value: 60 # Tortoise regards this value as the target utilization for the CPU of the container named "app", and it will adjust this target value. - - type: External - external: - metric: - name: datadogmetric@example-prod:example-workload-cpu-istio-sidecar # CPU target of the container named "istio-sidecar" - target: - type: Value - value: 80 # Tortoise regards this value as the target utilization for the CPU of the container named "istio-sidecar", and it will adjust this target value. - - type: External - external: - metric: - name: datadogmetric@example-prod:example-workload-memory-app # memory target of the container named "app" - target: - type: Value - value: 75 # Tortoise regards this value as the target utilization for the memory of the container named "app", and it will adjust this target value. - - type: External - external: - metric: - name: datadogmetric@example-prod:example-workload-memory-istio-sidecar # memory target of the container named "istio-sidecar" - target: - type: Value - value: 70 # Tortoise regards this value as the target utilization for the memory of the container named "istio-sidecar", and it will adjust this target value. -``` +- `type: ContainerResource` metric if Pod has multiple containers. + +But, if a Pod has only one container but a corresponding HPA doesn't have `type: Resource`, tortoise controller looks for `type: ContainerResource` in HPA next. ### The container right sizing diff --git a/pkg/annotation/annotation.go b/pkg/annotation/annotation.go index 09df670c..00e1edb4 100644 --- a/pkg/annotation/annotation.go +++ b/pkg/annotation/annotation.go @@ -1,9 +1,6 @@ package annotation const ( - HPAContainerBasedCPUExternalMetricNamePrefixAnnotation = "tortoises.autoscaling.mercari.com/container-based-cpu-metric-prefix" - HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation = "tortoises.autoscaling.mercari.com/container-based-memory-metric-prefix" - // TortoiseNameAnnotation - VPA and HPA managed by tortoise have this label. TortoiseNameAnnotation = "tortoises.autoscaling.mercari.com/tortoise-name" diff --git a/pkg/hpa/service.go b/pkg/hpa/service.go index 7ae485a8..5817cd49 100644 --- a/pkg/hpa/service.go +++ b/pkg/hpa/service.go @@ -5,18 +5,15 @@ import ( "errors" "fmt" "math" - "strconv" "time" v1 "k8s.io/api/apps/v1" v2 "k8s.io/api/autoscaling/v2" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" - "k8s.io/klog/v2" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" @@ -40,65 +37,6 @@ func New(c client.Client, replicaReductionFactor float64, upperTargetResourceUti } } -func (c *Service) CreateHPAForSingleContainer(ctx context.Context, tortoise *autoscalingv1alpha1.Tortoise, dm *v1.Deployment) (*v2.HorizontalPodAutoscaler, *autoscalingv1alpha1.Tortoise, error) { - // TODO: make this default HPA spec configurable. - hpa := &v2.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: autoscalingv1alpha1.TortoiseDefaultHPAName(tortoise.Name), - Namespace: tortoise.Namespace, - Annotations: map[string]string{ - annotation.TortoiseNameAnnotation: tortoise.Name, - annotation.ManagedByTortoiseAnnotation: "true", - }, - }, - Spec: v2.HorizontalPodAutoscalerSpec{ - ScaleTargetRef: v2.CrossVersionObjectReference{ - Kind: "Deployment", - Name: tortoise.Spec.TargetRefs.DeploymentName, - APIVersion: "apps/v1", - }, - MinReplicas: pointer.Int32(int32(math.Ceil(float64(dm.Status.Replicas) / 2.0))), - MaxReplicas: dm.Status.Replicas * 2, - Behavior: &v2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &v2.HPAScalingRules{ - Policies: []v2.HPAScalingPolicy{ - { - Type: v2.PercentScalingPolicy, - Value: 2, - PeriodSeconds: 90, - }, - }, - }, - }, - }, - } - - m := make([]v2.MetricSpec, 0, len(tortoise.Spec.ResourcePolicy)) - for _, policy := range tortoise.Spec.ResourcePolicy { - for r, p := range policy.AutoscalingPolicy { - value := pointer.Int32(50) - if p != autoscalingv1alpha1.AutoscalingTypeHorizontal { - value = pointer.Int32(c.upperTargetResourceUtilization) - } - m = append(m, v2.MetricSpec{ - Type: v2.ResourceMetricSourceType, - Resource: &v2.ResourceMetricSource{ - Name: r, - Target: v2.MetricTarget{ - Type: v2.UtilizationMetricType, - AverageUtilization: value, - }, - }, - }) - } - } - hpa.Spec.Metrics = m - tortoise.Status.Targets.HorizontalPodAutoscaler = hpa.Name - - err := c.c.Create(ctx, hpa) - return hpa.DeepCopy(), tortoise, err -} - func (c *Service) InitializeHPA(ctx context.Context, tortoise *autoscalingv1alpha1.Tortoise, dm *v1.Deployment) (*autoscalingv1alpha1.Tortoise, error) { if tortoise.Spec.TargetRefs.HorizontalPodAutoscalerName != nil { // update the existing HPA that the user set on tortoise. @@ -180,23 +118,13 @@ func (c *Service) CreateHPA(ctx context.Context, tortoise *autoscalingv1alpha1.T return nil, tortoise, nil } - if len(dm.Spec.Template.Spec.Containers) == 1 { - return c.CreateHPAForSingleContainer(ctx, tortoise, dm) - } - return c.CreateHPAForMultipleContainer(ctx, tortoise, dm) -} - -func (c *Service) CreateHPAForMultipleContainer(ctx context.Context, tortoise *autoscalingv1alpha1.Tortoise, dm *v1.Deployment) (*v2.HorizontalPodAutoscaler, *autoscalingv1alpha1.Tortoise, error) { - // TODO: make this default HPA spec configurable. hpa := &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: autoscalingv1alpha1.TortoiseDefaultHPAName(tortoise.Name), Namespace: tortoise.Namespace, Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: fmt.Sprintf("datadogmetric@%s:%s-memory-", tortoise.Namespace, tortoise.Spec.TargetRefs.DeploymentName), - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: fmt.Sprintf("datadogmetric@%s:%s-cpu-", tortoise.Namespace, tortoise.Spec.TargetRefs.DeploymentName), - annotation.TortoiseNameAnnotation: tortoise.Name, - annotation.ManagedByTortoiseAnnotation: "true", + annotation.TortoiseNameAnnotation: tortoise.Name, + annotation.ManagedByTortoiseAnnotation: "true", }, }, Spec: v2.HorizontalPodAutoscalerSpec{ @@ -233,23 +161,18 @@ func (c *Service) CreateHPAForMultipleContainer(ctx context.Context, tortoise *a m := make([]v2.MetricSpec, 0, len(tortoise.Spec.ResourcePolicy)) for _, policy := range tortoise.Spec.ResourcePolicy { for r, p := range policy.AutoscalingPolicy { - value := resourceQuantityPtr(resource.MustParse("50")) + value := pointer.Int32(50) if p != autoscalingv1alpha1.AutoscalingTypeHorizontal { - value = resourceQuantityPtr(resource.MustParse(strconv.Itoa(int(c.upperTargetResourceUtilization)))) - } - externalMetricName, err := externalMetricNameFromAnnotation(hpa, policy.ContainerName, r) - if err != nil { - return nil, tortoise, err + value = pointer.Int32(c.upperTargetResourceUtilization) } m = append(m, v2.MetricSpec{ - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: externalMetricName, - }, + Type: v2.ContainerResourceMetricSourceType, + ContainerResource: &v2.ContainerResourceMetricSource{ + Name: r, + Container: policy.ContainerName, Target: v2.MetricTarget{ - Type: v2.ValueMetricType, - Value: value, + Type: v2.UtilizationMetricType, + AverageUtilization: value, }, }, }) @@ -361,33 +284,26 @@ func GetReplicasRecommendation(recommendations []autoscalingv1alpha1.ReplicasRec return 0, errors.New("no recommendation slot") } -func externalMetricNameFromAnnotation(hpa *v2.HorizontalPodAutoscaler, containerName string, k corev1.ResourceName) (string, error) { - var prefix string - switch k { - case corev1.ResourceCPU: - prefix = hpa.GetAnnotations()[annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation] - case corev1.ResourceMemory: - prefix = hpa.GetAnnotations()[annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation] - default: - return "", fmt.Errorf("non supported resource type: %s", k) - } - return prefix + containerName, nil -} - func updateHPATargetValue(hpa *v2.HorizontalPodAutoscaler, containerName string, k corev1.ResourceName, targetValue int32, isSingleContainerDeployment bool) error { for _, m := range hpa.Spec.Metrics { if isSingleContainerDeployment && m.Type == v2.ResourceMetricSourceType && m.Resource.Target.Type == v2.UtilizationMetricType && m.Resource.Name == k { + // If the deployment has only one container, the resource metric is the target. m.Resource.Target.AverageUtilization = pointer.Int32(targetValue) } + } + + // If the deployment has more than one container, the container resource metric is the metric for the container. + // Also, even if the deployment has only one container, the container resource metric might be used as well. + // So, check the container resource metric as well. + for _, m := range hpa.Spec.Metrics { if m.Type != v2.ContainerResourceMetricSourceType { continue } if m.ContainerResource == nil { // shouldn't reach here - klog.ErrorS(nil, "invalid container resource metric", klog.KObj(hpa)) - continue + return fmt.Errorf("invalid container resource metric: %s/%s", hpa.Namespace, hpa.Name) } if m.ContainerResource.Container != containerName || m.ContainerResource.Name != k || m.ContainerResource.Target.AverageUtilization == nil { @@ -397,32 +313,5 @@ func updateHPATargetValue(hpa *v2.HorizontalPodAutoscaler, containerName string, m.ContainerResource.Target.AverageUtilization = &targetValue } - externalMetricName, err := externalMetricNameFromAnnotation(hpa, containerName, k) - if err != nil { - return err - } - - for _, m := range hpa.Spec.Metrics { - if m.Type != v2.ExternalMetricSourceType { - continue - } - - if m.External == nil { - // shouldn't reach here - klog.ErrorS(nil, "invalid external metric", klog.KObj(hpa)) - continue - } - - if m.External.Metric.Name != externalMetricName { - continue - } - - m.External.Target.Value.Set(int64(targetValue)) - } - return nil } - -func resourceQuantityPtr(quantity resource.Quantity) *resource.Quantity { - return &quantity -} diff --git a/pkg/hpa/service_test.go b/pkg/hpa/service_test.go index 601d8750..0f404d83 100644 --- a/pkg/hpa/service_test.go +++ b/pkg/hpa/service_test.go @@ -10,7 +10,6 @@ import ( appv1 "k8s.io/api/apps/v1" v2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,139 +35,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { wantTortoise *autoscalingv1alpha1.Tortoise wantErr bool }{ - { - name: "Basic test case with external metrics", - args: args{ - ctx: context.Background(), - tortoise: &autoscalingv1alpha1.Tortoise{ - Status: autoscalingv1alpha1.TortoiseStatus{ - Targets: autoscalingv1alpha1.TargetsStatus{ - HorizontalPodAutoscaler: "hpa", - }, - Recommendations: autoscalingv1alpha1.Recommendations{ - Horizontal: &autoscalingv1alpha1.HorizontalRecommendations{ - TargetUtilizations: []autoscalingv1alpha1.HPATargetUtilizationRecommendationPerContainer{ - { - ContainerName: "app", - TargetUtilization: map[v1.ResourceName]int32{ - v1.ResourceMemory: 90, - }, - }, - { - ContainerName: "istio-proxy", - TargetUtilization: map[v1.ResourceName]int32{ - v1.ResourceCPU: 80, - }, - }, - }, - MaxReplicas: []autoscalingv1alpha1.ReplicasRecommendation{ - { - From: 0, - To: 2, - Value: 6, - UpdatedAt: now, - WeekDay: pointer.String(now.Weekday().String()), - }, - }, - MinReplicas: []autoscalingv1alpha1.ReplicasRecommendation{ - { - From: 0, - To: 2, - Value: 3, - UpdatedAt: now, - WeekDay: pointer.String(now.Weekday().String()), - }, - }, - }, - }, - }, - }, - now: now.Time, - }, - initialHPA: &v2.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, - }, - Spec: v2.HorizontalPodAutoscalerSpec{ - MinReplicas: ptrInt32(1), - MaxReplicas: 2, - Metrics: []v2.MetricSpec{ - { - Type: v2.ObjectMetricSourceType, - // should be ignored - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-memory-app", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("60")), - }, - }, - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-cpu-istio-proxy", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("50")), - }, - }, - }, - }, - }, - }, - want: &v2.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, - }, - Spec: v2.HorizontalPodAutoscalerSpec{ - MinReplicas: ptrInt32(3), - MaxReplicas: 6, - Metrics: []v2.MetricSpec{ - { - Type: v2.ObjectMetricSourceType, - // should be ignored - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-memory-app", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("90")), - }, - }, - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-cpu-istio-proxy", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("80")), - }, - }, - }, - }, - }, - }, - wantErr: false, - }, { name: "Basic test case with container resource metrics", args: args{ @@ -221,10 +87,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { initialHPA: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(1), @@ -260,10 +122,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { want: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(3), @@ -353,10 +211,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { initialHPA: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(1), @@ -392,10 +246,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { want: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(1), @@ -483,10 +333,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { initialHPA: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(1), @@ -522,10 +368,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { want: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(6), @@ -613,10 +455,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { initialHPA: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(6), @@ -652,10 +490,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { want: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(5), @@ -743,10 +577,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { initialHPA: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(1), @@ -782,10 +612,6 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) { want: &v2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "hpa", - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, }, Spec: v2.HorizontalPodAutoscalerSpec{ MinReplicas: ptrInt32(3), @@ -956,6 +782,15 @@ func TestService_InitializeHPA(t *testing.T) { APIVersion: "apps/v1", }, Behavior: &v2.HorizontalPodAutoscalerBehavior{ + ScaleUp: &v2.HPAScalingRules{ + Policies: []v2.HPAScalingPolicy{ + { + Type: v2.PercentScalingPolicy, + Value: 100, + PeriodSeconds: 60, + }, + }, + }, ScaleDown: &v2.HPAScalingRules{ Policies: []v2.HPAScalingPolicy{ { diff --git a/pkg/recommender/recommender.go b/pkg/recommender/recommender.go index 765d6d0e..67b749fe 100644 --- a/pkg/recommender/recommender.go +++ b/pkg/recommender/recommender.go @@ -17,7 +17,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "github.com/mercari/tortoise/api/v1alpha1" - "github.com/mercari/tortoise/pkg/annotation" ) type Service struct { @@ -328,9 +327,16 @@ func (s *Service) updateHPATargetUtilizationRecommendations(ctx context.Context, func getHPATargetValue(hpa *v2.HorizontalPodAutoscaler, containerName string, k corev1.ResourceName, isSingleContainerDeployment bool) (int32, error) { for _, m := range hpa.Spec.Metrics { if isSingleContainerDeployment && m.Type == v2.ResourceMetricSourceType && m.Resource.Target.Type == v2.UtilizationMetricType && m.Resource.Name == k { + // If the deployment has only one container, the resource metric is the metric for the container. return *m.Resource.Target.AverageUtilization, nil } + } + + // If the deployment has more than one container, the container resource metric is the metric for the container. + // Also, even if the deployment has only one container, the container resource metric might be used instead of resource metric. + // So, check the container resource metric as well. + for _, m := range hpa.Spec.Metrics { if m.Type != v2.ContainerResourceMetricSourceType { continue } @@ -348,35 +354,6 @@ func getHPATargetValue(hpa *v2.HorizontalPodAutoscaler, containerName string, k return *m.ContainerResource.Target.AverageUtilization, nil } - var prefix string - switch k { - case corev1.ResourceCPU: - prefix = hpa.GetAnnotations()[annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation] - case corev1.ResourceMemory: - prefix = hpa.GetAnnotations()[annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation] - default: - return 0, fmt.Errorf("non supported resource type: %s", k) - } - externalMetricName := prefix + containerName - - for _, m := range hpa.Spec.Metrics { - if m.Type != v2.ExternalMetricSourceType { - continue - } - - if m.External == nil { - // shouldn't reach here - klog.ErrorS(nil, "invalid external metric", klog.KObj(hpa)) - continue - } - - if m.External.Metric.Name != externalMetricName { - continue - } - - return int32(m.External.Target.Value.Value()), nil - } - return 0, fmt.Errorf("unsupported hpa: %s, resource name: %s, single container deployment: %v", client.ObjectKeyFromObject(hpa).String(), k, isSingleContainerDeployment) } diff --git a/pkg/recommender/recommender_test.go b/pkg/recommender/recommender_test.go index 8886ff56..e7c064c5 100644 --- a/pkg/recommender/recommender_test.go +++ b/pkg/recommender/recommender_test.go @@ -15,7 +15,6 @@ import ( "k8s.io/utils/pointer" "github.com/mercari/tortoise/api/v1alpha1" - "github.com/mercari/tortoise/pkg/annotation" ) func TestUpdateRecommendation(t *testing.T) { @@ -81,32 +80,14 @@ func TestUpdateRecommendation(t *testing.T) { }, }, hpa: &v2.HorizontalPodAutoscaler{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - // they shouldn't affect. - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, - }, Spec: v2.HorizontalPodAutoscalerSpec{ Metrics: []v2.MetricSpec{ { + // unrelated Type: v2.ObjectMetricSourceType, - // should be ignored - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-memory-app", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("90")), - }, - }, }, { + // unrelated Type: v2.ExternalMetricSourceType, External: &v2.ExternalMetricSource{ Metric: v2.MetricIdentifier{ @@ -239,196 +220,6 @@ func TestUpdateRecommendation(t *testing.T) { }, }, }, - { - name: "if HPA doesn't have the container resource metrics, then the external metrics are used", - args: args{ - tortoise: &v1alpha1.Tortoise{ - Spec: v1alpha1.TortoiseSpec{ - ResourcePolicy: []v1alpha1.ContainerResourcePolicy{ - { - ContainerName: "app", - AutoscalingPolicy: map[corev1.ResourceName]v1alpha1.AutoscalingType{ - corev1.ResourceMemory: v1alpha1.AutoscalingTypeHorizontal, - corev1.ResourceCPU: v1alpha1.AutoscalingTypeVertical, - }, - }, - { - ContainerName: "istio-proxy", - AutoscalingPolicy: map[corev1.ResourceName]v1alpha1.AutoscalingType{ - corev1.ResourceMemory: v1alpha1.AutoscalingTypeVertical, - corev1.ResourceCPU: v1alpha1.AutoscalingTypeHorizontal, - }, - }, - }, - }, - Status: v1alpha1.TortoiseStatus{ - Conditions: v1alpha1.Conditions{ - ContainerRecommendationFromVPA: []v1alpha1.ContainerRecommendationFromVPA{ - - { - ContainerName: "app", - MaxRecommendation: map[corev1.ResourceName]v1alpha1.ResourceQuantity{ - corev1.ResourceMemory: { - Quantity: resource.MustParse("4Gi"), - }, - corev1.ResourceCPU: { - Quantity: resource.MustParse("4"), - }, - }, - }, - { - ContainerName: "istio-proxy", - MaxRecommendation: map[corev1.ResourceName]v1alpha1.ResourceQuantity{ - corev1.ResourceMemory: { - Quantity: resource.MustParse("0.6Gi"), - }, - corev1.ResourceCPU: { - Quantity: resource.MustParse("0.6"), - }, - }, - }, - }, - }, - }, - }, - hpa: &v2.HorizontalPodAutoscaler{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - annotation.HPAContainerBasedMemoryExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-memory-", - annotation.HPAContainerBasedCPUExternalMetricNamePrefixAnnotation: "datadogmetric@echo-prod:echo-cpu-", - }, - }, - Spec: v2.HorizontalPodAutoscalerSpec{ - Metrics: []v2.MetricSpec{ - { - Type: v2.ObjectMetricSourceType, - // should be ignored - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-memory-app", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("60")), - }, - }, - }, - { - Type: v2.ExternalMetricSourceType, - External: &v2.ExternalMetricSource{ - Metric: v2.MetricIdentifier{ - Name: "datadogmetric@echo-prod:echo-cpu-istio-proxy", - }, - Target: v2.MetricTarget{ - Value: resourceQuantityPtr(resource.MustParse("50")), - }, - }, - }, - }, - }, - Status: v2.HorizontalPodAutoscalerStatus{}, - }, - deployment: &v1.Deployment{ - Spec: v1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "app", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse("5Gi"), - corev1.ResourceCPU: resource.MustParse("5"), - }, - }, - }, - { - Name: "istio-proxy", - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse("1Gi"), - corev1.ResourceCPU: resource.MustParse("1"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - want: &v1alpha1.Tortoise{ - Spec: v1alpha1.TortoiseSpec{ - ResourcePolicy: []v1alpha1.ContainerResourcePolicy{ - { - ContainerName: "app", - AutoscalingPolicy: map[corev1.ResourceName]v1alpha1.AutoscalingType{ - corev1.ResourceMemory: v1alpha1.AutoscalingTypeHorizontal, - corev1.ResourceCPU: v1alpha1.AutoscalingTypeVertical, - }, - }, - { - ContainerName: "istio-proxy", - AutoscalingPolicy: map[corev1.ResourceName]v1alpha1.AutoscalingType{ - corev1.ResourceMemory: v1alpha1.AutoscalingTypeVertical, - corev1.ResourceCPU: v1alpha1.AutoscalingTypeHorizontal, - }, - }, - }, - }, - Status: v1alpha1.TortoiseStatus{ - Recommendations: v1alpha1.Recommendations{ - Horizontal: &v1alpha1.HorizontalRecommendations{ - TargetUtilizations: []v1alpha1.HPATargetUtilizationRecommendationPerContainer{ - { - ContainerName: "app", - TargetUtilization: map[corev1.ResourceName]int32{ - corev1.ResourceMemory: 80, - corev1.ResourceCPU: 90, - }, - }, - { - ContainerName: "istio-proxy", - TargetUtilization: map[corev1.ResourceName]int32{ - corev1.ResourceCPU: 90, - corev1.ResourceMemory: 90, - }, - }, - }, - }, - }, - Conditions: v1alpha1.Conditions{ - ContainerRecommendationFromVPA: []v1alpha1.ContainerRecommendationFromVPA{ - { - ContainerName: "app", - MaxRecommendation: map[corev1.ResourceName]v1alpha1.ResourceQuantity{ - corev1.ResourceMemory: { - Quantity: resource.MustParse("4Gi"), - }, - corev1.ResourceCPU: { - Quantity: resource.MustParse("4"), - }, - }, - }, - { - ContainerName: "istio-proxy", - MaxRecommendation: map[corev1.ResourceName]v1alpha1.ResourceQuantity{ - corev1.ResourceMemory: { - Quantity: resource.MustParse("0.6Gi"), - }, - corev1.ResourceCPU: { - Quantity: resource.MustParse("0.6"), - }, - }, - }, - }, - }, - }, - }, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {