diff --git a/api/v1beta2/testdata/mutating/with-hpa-and-istio/after.yaml b/api/v1beta2/testdata/mutating/with-hpa-and-istio/after.yaml new file mode 100644 index 00000000..62f160d2 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/api/v1beta2/testdata/mutating/with-hpa-and-istio/before.yaml b/api/v1beta2/testdata/mutating/with-hpa-and-istio/before.yaml new file mode 100644 index 00000000..20b154ff --- /dev/null +++ b/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 diff --git a/api/v1beta2/testdata/mutating/with-hpa-and-istio/deployment.yaml b/api/v1beta2/testdata/mutating/with-hpa-and-istio/deployment.yaml new file mode 100644 index 00000000..5161c071 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/api/v1beta2/testdata/mutating/with-hpa-and-istio/hpa.yaml b/api/v1beta2/testdata/mutating/with-hpa-and-istio/hpa.yaml new file mode 100644 index 00000000..0bc6c614 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/api/v1beta2/testdata/mutating/with-hpa/after.yaml b/api/v1beta2/testdata/mutating/with-hpa/after.yaml new file mode 100644 index 00000000..2abbb74e --- /dev/null +++ b/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 \ No newline at end of file diff --git a/api/v1beta2/testdata/mutating/with-hpa/before.yaml b/api/v1beta2/testdata/mutating/with-hpa/before.yaml new file mode 100644 index 00000000..20b154ff --- /dev/null +++ b/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 diff --git a/api/v1beta2/testdata/mutating/with-hpa/deployment.yaml b/api/v1beta2/testdata/mutating/with-hpa/deployment.yaml new file mode 100644 index 00000000..451ac49a --- /dev/null +++ b/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 \ No newline at end of file diff --git a/api/v1beta2/testdata/validating/fail-with-istio/hpa.yaml b/api/v1beta2/testdata/mutating/with-hpa/hpa.yaml similarity index 93% rename from api/v1beta2/testdata/validating/fail-with-istio/hpa.yaml rename to api/v1beta2/testdata/mutating/with-hpa/hpa.yaml index 9668268b..96cf429d 100644 --- a/api/v1beta2/testdata/validating/fail-with-istio/hpa.yaml +++ b/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 diff --git a/api/v1beta2/tortoise_types.go b/api/v1beta2/tortoise_types.go index 798f2e3e..966bac86 100644 --- a/api/v1beta2/tortoise_types.go +++ b/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"` } diff --git a/api/v1beta2/tortoise_webhook.go b/api/v1beta2/tortoise_webhook.go index 9f9cf355..6b1ec66b 100644 --- a/api/v1beta2/tortoise_webhook.go +++ b/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 { diff --git a/api/v1beta2/tortoise_webhook_test.go b/api/v1beta2/tortoise_webhook_test.go index f5569cec..e63b5cef 100644 --- a/api/v1beta2/tortoise_webhook_test.go +++ b/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() { diff --git a/config/crd/bases/autoscaling.mercari.com_tortoises.yaml b/config/crd/bases/autoscaling.mercari.com_tortoises.yaml index 233c4732..bba93e71 100644 --- a/config/crd/bases/autoscaling.mercari.com_tortoises.yaml +++ b/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. diff --git a/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml b/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml index ae6c4fc1..0ff7c618 100644 --- a/manifests/crd/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml +++ b/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. diff --git a/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml b/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml index 97817e38..9e078c9a 100644 --- a/manifests/default/apiextensions.k8s.io_v1_customresourcedefinition_tortoises.autoscaling.mercari.com.yaml +++ b/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.