reads an attached HPA and sets “Horizontal” to resources managed by t…

…he attached HPA (#215)

api/v1beta2/testdata/mutating/with-hpa-and-istio/after.yaml

@@ -0,0 +1,26 @@
+apiVersion: autoscaling.mercari.com/v1beta2
+kind: Tortoise
+metadata:
+  name: tortoise-sample
+  namespace: default
+spec:
+  updateMode: "Off"
+  deletionPolicy: "NoDelete"
+  targetRefs:
+    horizontalPodAutoscalerName: sample-hpa
+    scaleTargetRef:
+      kind: Deployment
+      name: sample
+  resourcePolicy:
+    - containerName: hoge
+      autoscalingPolicy:
+        cpu: Vertical
+        memory: Horizontal
+    - containerName: nginx
+      autoscalingPolicy:
+        cpu: Horizontal
+        memory: Vertical
+    - containerName: istio-proxy
+      autoscalingPolicy:
+        cpu: Horizontal
+        memory: Vertical

api/v1beta2/testdata/mutating/with-hpa-and-istio/before.yaml

@@ -0,0 +1,11 @@
+apiVersion: autoscaling.mercari.com/v1beta2
+kind: Tortoise
+metadata:
+  name: tortoise-sample
+  namespace: default
+spec:
+  targetRefs:
+    horizontalPodAutoscalerName: sample-hpa
+    scaleTargetRef:
+      kind: Deployment
+      name: sample

api/v1beta2/testdata/mutating/with-hpa-and-istio/deployment.yaml

@@ -0,0 +1,28 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: sample
+  namespace: default
+  labels:
+    app: nginx
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      app: nginx
+  template:
+    metadata:
+      annotations:
+        sidecar.istio.io/inject: "true"
+      labels:
+        app: nginx
+    spec:
+      containers:
+      - name: hoge
+        image: hoge:1.0.0
+        ports:
+        - containerPort: 81
+      - name: nginx
+        image: nginx:1.14.2
+        ports:
+        - containerPort: 80

api/v1beta2/testdata/mutating/with-hpa-and-istio/hpa.yaml

@@ -0,0 +1,34 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: sample-hpa
+  namespace: default
+spec:
+  maxReplicas: 10
+  metrics:
+    - type: ContainerResource
+      containerResource:
+        name: cpu
+        container: nginx
+        target:
+          type: Utilization
+          averageUtilization: 60
+    - type: ContainerResource
+      containerResource:
+        name: memory
+        container: hoge
+        target:
+          type: Utilization
+          averageUtilization: 60
+    - type: ContainerResource
+      containerResource:
+        name: cpu
+        container: istio-proxy
+        target:
+          type: Utilization
+          averageUtilization: 60
+  minReplicas: 3
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: sample

api/v1beta2/testdata/mutating/with-hpa/after.yaml

@@ -0,0 +1,22 @@
+apiVersion: autoscaling.mercari.com/v1beta2
+kind: Tortoise
+metadata:
+  name: tortoise-sample
+  namespace: default
+spec:
+  updateMode: "Off"
+  deletionPolicy: "NoDelete"
+  targetRefs:
+    horizontalPodAutoscalerName: sample-hpa
+    scaleTargetRef:
+      kind: Deployment
+      name: sample
+  resourcePolicy:
+    - containerName: hoge
+      autoscalingPolicy:
+        cpu: Vertical
+        memory: Horizontal
+    - containerName: nginx
+      autoscalingPolicy:
+        cpu: Horizontal
+        memory: Vertical

api/v1beta2/testdata/mutating/with-hpa/before.yaml

@@ -0,0 +1,11 @@
+apiVersion: autoscaling.mercari.com/v1beta2
+kind: Tortoise
+metadata:
+  name: tortoise-sample
+  namespace: default
+spec:
+  targetRefs:
+    horizontalPodAutoscalerName: sample-hpa
+    scaleTargetRef:
+      kind: Deployment
+      name: sample

api/v1beta2/testdata/mutating/with-hpa/deployment.yaml

@@ -0,0 +1,26 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: sample
+  namespace: default
+  labels:
+    app: nginx
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      app: nginx
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+      - name: hoge
+        image: hoge:1.0.0
+        ports:
+        - containerPort: 81
+      - name: nginx
+        image: nginx:1.14.2
+        ports:
+        - containerPort: 80

api/v1beta2/testdata/validating/fail-with-istio/hpa.yaml → api/v1beta2/testdata/mutating/with-hpa/hpa.yaml

@@ -1,7 +1,7 @@
 apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
 metadata:
-  name: sample
+  name: sample-hpa
   namespace: default
 spec:
   maxReplicas: 10
@@ -15,7 +15,7 @@ spec:
           averageUtilization: 60
     - type: ContainerResource
       containerResource:
-        name: cpu
+        name: memory
         container: hoge
         target:
           type: Utilization

api/v1beta2/tortoise_types.go

@@ -82,7 +82,9 @@ type ContainerResourcePolicy struct {
 	// If "Vertical", the resource is vertically scaled.
 	// If "Off", tortoise doesn't scale at all based on that resource.
 	//
-	// The default value is "Horizontal" for cpu, and "Vertical" for memory.
+	// If .spec.TargetRefs.HorizontalPodAutoscalerName is empty, the default value is "Horizontal" for cpu, and "Vertical" for memory.
+	// If .spec.TargetRefs.HorizontalPodAutoscalerName is not empty, by default, it sets "Horizontal" to resources managed by the attached HPA,
+	// and "Vertical" to resources not managed by the attached HPA.
 	// +optional
 	AutoscalingPolicy map[v1.ResourceName]AutoscalingType `json:"autoscalingPolicy,omitempty" protobuf:"bytes,3,opt,name=autoscalingPolicy"`
 }

api/v1beta2/tortoise_webhook.go

@@ -68,6 +68,7 @@ func TortoiseDefaultHPAName(tortoiseName string) string {
 // Default implements webhook.Defaulter so a webhook will be registered for the type
 func (r *Tortoise) Default() {
 	tortoiselog.Info("default", "name", r.Name)
+	ctx := context.Background()
 
 	if r.Spec.UpdateMode == "" {
 		r.Spec.UpdateMode = UpdateModeOff
@@ -77,7 +78,7 @@ func (r *Tortoise) Default() {
 	}
 
 	if r.Spec.TargetRefs.ScaleTargetRef.Kind == "Deployment" {
-		d, err := ClientService.GetDeploymentOnTortoise(context.Background(), r)
+		d, err := ClientService.GetDeploymentOnTortoise(ctx, r)
 		if err != nil {
 			tortoiselog.Error(err, "failed to get deployment")
 			return
@@ -112,6 +113,48 @@ func (r *Tortoise) Default() {
 		}
 	}
 
+	// set the default value to ResourcePolicy
+	if r.Spec.TargetRefs.HorizontalPodAutoscalerName != nil {
+		hpa, err := ClientService.GetHPAFromUser(ctx, r)
+		if err == nil {
+			hpaManagedResourceAndContainer := sets.New[resourceNameAndContainerName]()
+			for _, m := range hpa.Spec.Metrics {
+				if m.Type != v2.ContainerResourceMetricSourceType {
+					continue
+				}
+				hpaManagedResourceAndContainer.Insert(resourceNameAndContainerName{m.ContainerResource.Name, m.ContainerResource.Container})
+			}
+
+			// If the existing HPA is attached, we sets “Horizontal” to resources managed by the attached HPA by default.
+			for i := range r.Spec.ResourcePolicy {
+				_, ok := r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceCPU]
+				if !ok {
+					if hpaManagedResourceAndContainer.Has(resourceNameAndContainerName{v1.ResourceCPU, r.Spec.ResourcePolicy[i].ContainerName}) {
+						// If HPA has the metrics for this container's CPU, we set Horizontal.
+						r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceCPU] = AutoscalingTypeHorizontal
+					} else {
+						// Otherwise, set Vertical.
+						r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceCPU] = AutoscalingTypeVertical
+					}
+				}
+				_, ok = r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceMemory]
+				if !ok {
+					if hpaManagedResourceAndContainer.Has(resourceNameAndContainerName{v1.ResourceMemory, r.Spec.ResourcePolicy[i].ContainerName}) {
+						// If HPA has the metrics for this container's CPU, we set Horizontal.
+						r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceMemory] = AutoscalingTypeHorizontal
+					} else {
+						// Otherwise, set Vertical.
+						r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceMemory] = AutoscalingTypeVertical
+					}
+				}
+			}
+			return
+		}
+		// It shouldn't happen basically err != nil.
+		tortoiselog.Error(err, "failed to get HPA")
+	}
+
+	// If the existing HPA isn't attached, the default policy is Horizontal for CPU and Vertical for Memory.
 	for i := range r.Spec.ResourcePolicy {
 		_, ok := r.Spec.ResourcePolicy[i].AutoscalingPolicy[v1.ResourceCPU]
 		if !ok {

api/v1beta2/tortoise_webhook_test.go

@@ -42,7 +42,7 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-func mutateTest(before, after, deployment string) {
+func mutateTest(before, after, deployment, hpa string) {
 	ctx := context.Background()
 
 	y, err := os.ReadFile(deployment)
@@ -57,6 +57,20 @@ func mutateTest(before, after, deployment string) {
 		Expect(err).NotTo(HaveOccurred())
 	}()
 
+	if hpa != "" {
+		y, err = os.ReadFile(hpa)
+		Expect(err).NotTo(HaveOccurred())
+		hpaObj := &autoscalingv2.HorizontalPodAutoscaler{}
+		err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(hpaObj)
+		Expect(err).NotTo(HaveOccurred())
+		err = k8sClient.Create(ctx, hpaObj)
+		Expect(err).NotTo(HaveOccurred())
+		defer func() {
+			err = k8sClient.Delete(ctx, hpaObj)
+			Expect(err).NotTo(HaveOccurred())
+		}()
+	}
+
 	y, err = os.ReadFile(before)
 	Expect(err).NotTo(HaveOccurred())
 	beforeTortoise := &Tortoise{}
@@ -97,17 +111,19 @@ func validateCreationTest(tortoise, hpa, deployment string, valid bool) {
 		Expect(err).NotTo(HaveOccurred())
 	}()
 
-	y, err = os.ReadFile(hpa)
-	Expect(err).NotTo(HaveOccurred())
-	hpaObj := &autoscalingv2.HorizontalPodAutoscaler{}
-	err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(hpaObj)
-	Expect(err).NotTo(HaveOccurred())
-	err = k8sClient.Create(ctx, hpaObj)
-	Expect(err).NotTo(HaveOccurred())
-	defer func() {
-		err = k8sClient.Delete(ctx, hpaObj)
+	if hpa != "" {
+		y, err = os.ReadFile(hpa)
 		Expect(err).NotTo(HaveOccurred())
-	}()
+		hpaObj := &autoscalingv2.HorizontalPodAutoscaler{}
+		err = yaml.NewYAMLOrJSONDecoder(bytes.NewReader(y), 4096).Decode(hpaObj)
+		Expect(err).NotTo(HaveOccurred())
+		err = k8sClient.Create(ctx, hpaObj)
+		Expect(err).NotTo(HaveOccurred())
+		defer func() {
+			err = k8sClient.Delete(ctx, hpaObj)
+			Expect(err).NotTo(HaveOccurred())
+		}()
+	}
 
 	y, err = os.ReadFile(tortoise)
 	Expect(err).NotTo(HaveOccurred())
@@ -194,9 +210,17 @@ func validateUpdateTest(tortoise, existingTortoise, hpa, deployment string, vali
 
 var _ = Describe("Tortoise Webhook", func() {
 	Context("mutating", func() {
-		It("should mutate a Tortoise", func() {
-			mutateTest(filepath.Join("testdata", "mutating", "with-istio", "before.yaml"), filepath.Join("testdata", "mutating", "with-istio", "after.yaml"), filepath.Join("testdata", "mutating", "with-istio", "deployment.yaml"))
-			mutateTest(filepath.Join("testdata", "mutating", "nothing-to-do", "before.yaml"), filepath.Join("testdata", "mutating", "nothing-to-do", "after.yaml"), filepath.Join("testdata", "mutating", "nothing-to-do", "deployment.yaml"))
+		It("nothing to do", func() {
+			mutateTest(filepath.Join("testdata", "mutating", "nothing-to-do", "before.yaml"), filepath.Join("testdata", "mutating", "nothing-to-do", "after.yaml"), filepath.Join("testdata", "mutating", "nothing-to-do", "deployment.yaml"), "")
+		})
+		It("should mutate a Tortoise with istio", func() {
+			mutateTest(filepath.Join("testdata", "mutating", "with-istio", "before.yaml"), filepath.Join("testdata", "mutating", "with-istio", "after.yaml"), filepath.Join("testdata", "mutating", "with-istio", "deployment.yaml"), "")
+		})
+		It("should mutate a Tortoise with HPA", func() {
+			mutateTest(filepath.Join("testdata", "mutating", "with-hpa", "before.yaml"), filepath.Join("testdata", "mutating", "with-hpa", "after.yaml"), filepath.Join("testdata", "mutating", "with-hpa", "deployment.yaml"), filepath.Join("testdata", "mutating", "with-hpa", "hpa.yaml"))
+		})
+		It("should mutate a Tortoise with HPA and istio", func() {
+			mutateTest(filepath.Join("testdata", "mutating", "with-hpa-and-istio", "before.yaml"), filepath.Join("testdata", "mutating", "with-hpa-and-istio", "after.yaml"), filepath.Join("testdata", "mutating", "with-hpa-and-istio", "deployment.yaml"), filepath.Join("testdata", "mutating", "with-hpa-and-istio", "hpa.yaml"))
 		})
 	})
 	Context("validating(creation)", func() {
@@ -221,8 +245,8 @@ var _ = Describe("Tortoise Webhook", func() {
 		It("invalid: Tortoise has HPA specified, but no Horizoltal in autoscalingPolicy", func() {
 			validateCreationTest(filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "tortoise.yaml"), filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "hpa.yaml"), filepath.Join("testdata", "validating", "hpa-specified-but-no-horizontal", "deployment.yaml"), false)
 		})
-		It("invalidTortoise for the deployment with istio should have istio target", func() {
-			validateCreationTest(filepath.Join("testdata", "validating", "fail-with-istio", "tortoise.yaml"), filepath.Join("testdata", "validating", "fail-with-istio", "hpa.yaml"), filepath.Join("testdata", "validating", "fail-with-istio", "deployment.yaml"), false)
+		It("invalid: Tortoise for the deployment with istio should have istio target", func() {
+			validateCreationTest(filepath.Join("testdata", "validating", "fail-with-istio", "tortoise.yaml"), "", filepath.Join("testdata", "validating", "fail-with-istio", "deployment.yaml"), false)
 		})
 	})
 	Context("validating(updating)", func() {

config/crd/bases/autoscaling.mercari.com_tortoises.yaml

@@ -829,8 +829,12 @@ spec:
                         is scaled. If \"Horizontal\", the resource is horizontally
                         scaled. If \"Vertical\", the resource is vertically scaled.
                         If \"Off\", tortoise doesn't scale at all based on that resource.
-                        \n The default value is \"Horizontal\" for cpu, and \"Vertical\"
-                        for memory."
+                        \n If .spec.TargetRefs.HorizontalPodAutoscalerName is empty,
+                        the default value is \"Horizontal\" for cpu, and \"Vertical\"
+                        for memory. If .spec.TargetRefs.HorizontalPodAutoscalerName
+                        is not empty, by default, it sets \"Horizontal\" to resources
+                        managed by the attached HPA, and \"Vertical\" to resources
+                        not managed by the attached HPA."
                       type: object
                     containerName:
                       description: ContainerName is the name of target container.

manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml

@@ -683,7 +683,7 @@ spec:
                         - Horizontal
                         - Vertical
                         type: string
-                      description: "AutoscalingPolicy specifies how each resource is scaled. If \"Horizontal\", the resource is horizontally scaled. If \"Vertical\", the resource is vertically scaled. If \"Off\", tortoise doesn't scale at all based on that resource. \n The default value is \"Horizontal\" for cpu, and \"Vertical\" for memory."
+                      description: "AutoscalingPolicy specifies how each resource is scaled. If \"Horizontal\", the resource is horizontally scaled. If \"Vertical\", the resource is vertically scaled. If \"Off\", tortoise doesn't scale at all based on that resource. \n If .spec.TargetRefs.HorizontalPodAutoscalerName is empty, the default value is \"Horizontal\" for cpu, and \"Vertical\" for memory. If .spec.TargetRefs.HorizontalPodAutoscalerName is not empty, by default, it sets \"Horizontal\" to resources managed by the attached HPA, and \"Vertical\" to resources not managed by the attached HPA."
                       type: object
                     containerName:
                       description: ContainerName is the name of target container.

manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml

@@ -683,7 +683,7 @@ spec:
                         - Horizontal
                         - Vertical
                         type: string
-                      description: "AutoscalingPolicy specifies how each resource is scaled. If \"Horizontal\", the resource is horizontally scaled. If \"Vertical\", the resource is vertically scaled. If \"Off\", tortoise doesn't scale at all based on that resource. \n The default value is \"Horizontal\" for cpu, and \"Vertical\" for memory."
+                      description: "AutoscalingPolicy specifies how each resource is scaled. If \"Horizontal\", the resource is horizontally scaled. If \"Vertical\", the resource is vertically scaled. If \"Off\", tortoise doesn't scale at all based on that resource. \n If .spec.TargetRefs.HorizontalPodAutoscalerName is empty, the default value is \"Horizontal\" for cpu, and \"Vertical\" for memory. If .spec.TargetRefs.HorizontalPodAutoscalerName is not empty, by default, it sets \"Horizontal\" to resources managed by the attached HPA, and \"Vertical\" to resources not managed by the attached HPA."
                       type: object
                     containerName:
                       description: ContainerName is the name of target container.