Skip to content

Commit

Permalink
deprecation: remove the external metric support
Browse files Browse the repository at this point in the history
  • Loading branch information
@sanposhiho
sanposhiho committed Oct 2, 2023
1 parent 320f4dc commit 7f71e51
Show file tree
Hide file tree
Showing 8 changed files with 51 additions and 674 deletions.
67 changes: 0 additions & 67 deletions api/v1alpha1/tortoise_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
}
24 changes: 12 additions & 12 deletions controllers/tortoise_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}

Expand Down
56 changes: 3 additions & 53 deletions docs/horizontal.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 0 additions & 3 deletions pkg/annotation/annotation.go
View file
Original file line number Diff line number Diff line change
@@ -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"

Expand Down
Loading

0 comments on commit 7f71e51

Please sign in to comment.