diff --git a/api/v1beta1/tortoise_webhook.go b/api/v1beta1/tortoise_webhook.go index 0163fed0..767ca37b 100644 --- a/api/v1beta1/tortoise_webhook.go +++ b/api/v1beta1/tortoise_webhook.go @@ -261,6 +261,13 @@ func (r *Tortoise) ValidateUpdate(old runtime.Object) (admission.Warnings, error } } + if hasHorizontal(oldTortoise) && !hasHorizontal(r) && r.Spec.DeletionPolicy == DeletionPolicyNoDelete { + // TODO: add test for this. + + // Old has horizontal, but the new one doesn't have any. + return nil, fmt.Errorf("%s: no horizontal policy exists. It will cause the deletion of HPA and you need to specify DeleteAll to allow the deletion.", fieldPath.Child("targetRefs", "resourcePolicy", "autoscalingPolicy")) + } + if reflect.DeepEqual(oldTortoise.Spec.ResourcePolicy, r.Spec.ResourcePolicy) { return nil, fmt.Errorf("%s: immutable field get changed", fieldPath.Child("resourcePolicy")) } diff --git a/api/v1beta1/tortoise_webhook_test.go b/api/v1beta1/tortoise_webhook_test.go index 1d94f224..b64b57d9 100644 --- a/api/v1beta1/tortoise_webhook_test.go +++ b/api/v1beta1/tortoise_webhook_test.go @@ -131,7 +131,7 @@ func validateTest(tortoise, hpa, deployment string, valid bool) { } } -var _ = Describe("MarkdownTortoise Webhook", func() { +var _ = Describe("Tortoise Webhook", func() { Context("mutating", func() { It("should mutate a Tortoise", func() { mutateTest(filepath.Join("testdata", "mutating", "before.yaml"), filepath.Join("testdata", "mutating", "after.yaml"), filepath.Join("testdata", "mutating", "deployment.yaml")) diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/hpa.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/hpa.yaml new file mode 100644 index 00000000..ed8c2e55 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/hpa.yaml @@ -0,0 +1,52 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-hpa-mercari + namespace: default +spec: + behavior: + scaleDown: + policies: + - periodSeconds: 90 + type: Percent + value: 2 + selectPolicy: Max + scaleUp: + policies: + - periodSeconds: 60 + type: Percent + value: 100 + selectPolicy: Max + stabilizationWindowSeconds: 0 + maxReplicas: 20 + metrics: + - containerResource: + container: app + name: cpu + target: + averageUtilization: 50 + type: Utilization + type: ContainerResource + - containerResource: + container: istio-proxy + name: cpu + target: + averageUtilization: 75 + type: Utilization + type: ContainerResource + - containerResource: + container: istio-proxy + name: memory + target: + averageUtilization: 75 + type: Utilization + type: ContainerResource + minReplicas: 5 + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + currentMetrics: null + desiredReplicas: 0 diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/tortoise.yaml new file mode 100644 index 00000000..7871be02 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/tortoise.yaml @@ -0,0 +1,93 @@ +metadata: + name: mercari + namespace: default +spec: + resourcePolicy: + - autoscalingPolicy: + cpu: Horizontal + memory: Vertical + containerName: app + - autoscalingPolicy: + cpu: Horizontal + memory: Horizontal + containerName: istio-proxy + targetRefs: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + conditions: + containerRecommendationFromVPA: + - containerName: app + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + - containerName: istio-proxy + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + tortoiseConditions: null + recommendations: + horizontal: + maxReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 20 + minReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 5 + targetUtilizations: + - containerName: app + targetUtilization: + cpu: 50 + - containerName: istio-proxy + targetUtilization: + cpu: 75 + memory: 75 + vertical: + containerResourceRecommendation: + - RecommendedResource: + cpu: "6" + memory: 3Gi + containerName: app + - RecommendedResource: + cpu: "4" + memory: 4Gi + containerName: istio-proxy + targets: + deployment: "" + horizontalPodAutoscaler: tortoise-hpa-mercari + verticalPodAutoscalers: + - name: tortoise-updater-mercari + role: Updater + - name: tortoise-monitor-mercari + role: Monitor + tortoisePhase: Working diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/vpa-Monitor.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/vpa-Monitor.yaml new file mode 100644 index 00000000..73acfb27 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/vpa-Monitor.yaml @@ -0,0 +1,44 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-monitor-mercari + namespace: default +spec: + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: "Off" +status: + conditions: + - lastTransitionTime: null + status: "True" + type: RecommendationProvided + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/vpa-Updater.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/vpa-Updater.yaml new file mode 100644 index 00000000..66bc7d20 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/vpa-Updater.yaml @@ -0,0 +1,48 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-updater-mercari + namespace: default +spec: + recommenders: + - name: tortoise-controller + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: Auto +status: + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "6" + memory: 3Gi + target: + cpu: "6" + memory: 3Gi + uncappedTarget: + cpu: "6" + memory: 3Gi + upperBound: + cpu: "6" + memory: 3Gi + - containerName: istio-proxy + lowerBound: + cpu: "4" + memory: 4Gi + target: + cpu: "4" + memory: 4Gi + uncappedTarget: + cpu: "4" + memory: 4Gi + upperBound: + cpu: "4" + memory: 4Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/deployment.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/deployment.yaml new file mode 100644 index 00000000..04ac49e3 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/deployment.yaml @@ -0,0 +1,29 @@ +metadata: + name: mercari-app + namespace: default +spec: + selector: + matchLabels: + app: mercari + strategy: {} + template: + metadata: + creationTimestamp: null + labels: + app: mercari + spec: + containers: + - image: awesome-mercari-app-image + name: app + resources: + requests: + cpu: "10" + memory: 10Gi + - image: awesome-istio-proxy-image + name: istio-proxy + resources: + requests: + cpu: "4" + memory: 4Gi +status: + replicas: 10 diff --git a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/hpa.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/hpa.yaml similarity index 73% rename from controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/hpa.yaml rename to controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/hpa.yaml index e9283d94..d4c64cb0 100644 --- a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/hpa.yaml +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/hpa.yaml @@ -21,12 +21,20 @@ spec: stabilizationWindowSeconds: 0 maxReplicas: 100 metrics: - - resource: + - containerResource: + container: app name: cpu target: - averageUtilization: 80 + averageUtilization: 50 type: Utilization - type: Resource + type: ContainerResource + - containerResource: + container: istio-proxy + name: cpu + target: + averageUtilization: 50 + type: Utilization + type: ContainerResource minReplicas: 1 scaleTargetRef: apiVersion: apps/v1 diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/tortoise.yaml new file mode 100644 index 00000000..5369d4cf --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/tortoise.yaml @@ -0,0 +1,84 @@ +metadata: + name: mercari + namespace: default +spec: + resourcePolicy: + - autoscalingPolicy: + cpu: Horizontal + memory: Vertical + containerName: app + - autoscalingPolicy: + cpu: Horizontal + memory: Horizontal + containerName: istio-proxy + targetRefs: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + conditions: + containerRecommendationFromVPA: + - containerName: app + maxRecommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + recommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + - containerName: istio-proxy + maxRecommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + recommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + tortoiseConditions: null + recommendations: + horizontal: + maxReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 15 + minReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 3 + targetUtilizations: + - containerName: app + targetUtilization: + cpu: 50 + - containerName: istio-proxy + targetUtilization: + cpu: 50 + vertical: + containerResourceRecommendation: null + targets: + deployment: "" + horizontalPodAutoscaler: tortoise-hpa-mercari + verticalPodAutoscalers: + - name: tortoise-updater-mercari + role: Updater + - name: tortoise-monitor-mercari + role: Monitor + tortoisePhase: Working diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/vpa-Monitor.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/vpa-Monitor.yaml new file mode 100644 index 00000000..73acfb27 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/vpa-Monitor.yaml @@ -0,0 +1,44 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-monitor-mercari + namespace: default +spec: + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: "Off" +status: + conditions: + - lastTransitionTime: null + status: "True" + type: RecommendationProvided + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/vpa-Updater.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/vpa-Updater.yaml new file mode 100644 index 00000000..28c9ee17 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/before/vpa-Updater.yaml @@ -0,0 +1,20 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-updater-mercari + namespace: default +spec: + recommenders: + - name: tortoise-controller + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: Auto +status: {} diff --git a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/hpa.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/hpa.yaml similarity index 83% rename from controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/hpa.yaml rename to controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/hpa.yaml index e9283d94..b76a3d33 100644 --- a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/hpa.yaml +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/hpa.yaml @@ -19,15 +19,16 @@ spec: value: 100 selectPolicy: Max stabilizationWindowSeconds: 0 - maxReplicas: 100 + maxReplicas: 20 metrics: - - resource: + - containerResource: + container: app name: cpu target: - averageUtilization: 80 + averageUtilization: 50 type: Utilization - type: Resource - minReplicas: 1 + type: ContainerResource + minReplicas: 5 scaleTargetRef: apiVersion: apps/v1 kind: Deployment diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/tortoise.yaml new file mode 100644 index 00000000..902c0442 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/tortoise.yaml @@ -0,0 +1,91 @@ +metadata: + name: mercari + namespace: default +spec: + resourcePolicy: + - autoscalingPolicy: + cpu: "Horizontal" + memory: "Vertical" + containerName: app + - autoscalingPolicy: + cpu: "Vertical" + memory: "Vertical" + containerName: istio-proxy + targetRefs: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + conditions: + containerRecommendationFromVPA: + - containerName: app + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + - containerName: istio-proxy + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + tortoiseConditions: null + recommendations: + horizontal: + maxReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:47Z" + value: 20 + minReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:47Z" + value: 5 + targetUtilizations: + - containerName: app + targetUtilization: + cpu: 50 + - containerName: istio-proxy + targetUtilization: {} + vertical: + containerResourceRecommendation: + - RecommendedResource: + cpu: "6" + memory: 3Gi + containerName: app + - RecommendedResource: + cpu: "3" + memory: 3Gi + containerName: istio-proxy + targets: + deployment: "" + horizontalPodAutoscaler: "tortoise-hpa-mercari" + verticalPodAutoscalers: + - name: tortoise-updater-mercari + role: Updater + - name: tortoise-monitor-mercari + role: Monitor + tortoisePhase: Working diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/vpa-Monitor.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/vpa-Monitor.yaml new file mode 100644 index 00000000..73acfb27 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/vpa-Monitor.yaml @@ -0,0 +1,44 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-monitor-mercari + namespace: default +spec: + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: "Off" +status: + conditions: + - lastTransitionTime: null + status: "True" + type: RecommendationProvided + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/vpa-Updater.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/vpa-Updater.yaml new file mode 100644 index 00000000..5ac22555 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/after/vpa-Updater.yaml @@ -0,0 +1,48 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-updater-mercari + namespace: default +spec: + recommenders: + - name: tortoise-controller + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: Auto +status: + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "6" + memory: 3Gi + target: + cpu: "6" + memory: 3Gi + uncappedTarget: + cpu: "6" + memory: 3Gi + upperBound: + cpu: "6" + memory: 3Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + uncappedTarget: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "3" + memory: 3Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/deployment.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/deployment.yaml new file mode 100644 index 00000000..04ac49e3 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/deployment.yaml @@ -0,0 +1,29 @@ +metadata: + name: mercari-app + namespace: default +spec: + selector: + matchLabels: + app: mercari + strategy: {} + template: + metadata: + creationTimestamp: null + labels: + app: mercari + spec: + containers: + - image: awesome-mercari-app-image + name: app + resources: + requests: + cpu: "10" + memory: 10Gi + - image: awesome-istio-proxy-image + name: istio-proxy + resources: + requests: + cpu: "4" + memory: 4Gi +status: + replicas: 10 diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/tortoise.yaml new file mode 100644 index 00000000..24c54b34 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/tortoise.yaml @@ -0,0 +1,89 @@ +metadata: + name: mercari + namespace: default +spec: + resourcePolicy: + - autoscalingPolicy: + cpu: "Horizontal" # This is updated from Vertical. + memory: "Vertical" + containerName: app + - autoscalingPolicy: + cpu: "Vertical" + memory: "Vertical" + containerName: istio-proxy + targetRefs: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + conditions: + containerRecommendationFromVPA: + - containerName: app + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + - containerName: istio-proxy + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + tortoiseConditions: null + recommendations: + horizontal: + maxReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:47Z" + value: 20 + minReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:47Z" + value: 5 + targetUtilizations: + - containerName: app + targetUtilization: {} + - containerName: istio-proxy + targetUtilization: {} + vertical: + containerResourceRecommendation: + - RecommendedResource: + cpu: "3" + memory: 3Gi + containerName: app + - RecommendedResource: + cpu: "3" + memory: 3Gi + containerName: istio-proxy + targets: + deployment: "" + verticalPodAutoscalers: + - name: tortoise-updater-mercari + role: Updater + - name: tortoise-monitor-mercari + role: Monitor + tortoisePhase: Working diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/vpa-Monitor.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/vpa-Monitor.yaml new file mode 100644 index 00000000..73acfb27 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/vpa-Monitor.yaml @@ -0,0 +1,44 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-monitor-mercari + namespace: default +spec: + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: "Off" +status: + conditions: + - lastTransitionTime: null + status: "True" + type: RecommendationProvided + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/vpa-Updater.yaml b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/vpa-Updater.yaml new file mode 100644 index 00000000..28c9ee17 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-no-hpa-and-add-horizontal/before/vpa-Updater.yaml @@ -0,0 +1,20 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-updater-mercari + namespace: default +spec: + recommenders: + - name: tortoise-controller + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: Auto +status: {} diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/tortoise.yaml new file mode 100644 index 00000000..d2c249f4 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/tortoise.yaml @@ -0,0 +1,90 @@ +metadata: + name: mercari + namespace: default +spec: + resourcePolicy: + - autoscalingPolicy: + cpu: Vertical + memory: Vertical + containerName: app + - autoscalingPolicy: + cpu: Vertical + memory: Vertical + containerName: istio-proxy + targetRefs: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + conditions: + containerRecommendationFromVPA: + - containerName: app + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + - containerName: istio-proxy + maxRecommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + recommendation: + cpu: + quantity: "3" + updatedAt: null + memory: + quantity: 3Gi + updatedAt: null + tortoiseConditions: null + recommendations: + horizontal: + maxReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 20 + minReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 5 + targetUtilizations: + - containerName: app + targetUtilization: {} + - containerName: istio-proxy + targetUtilization: {} + vertical: + containerResourceRecommendation: + - RecommendedResource: + cpu: "3" + memory: 3Gi + containerName: app + - RecommendedResource: + cpu: "3" + memory: 3Gi + containerName: istio-proxy + targets: + deployment: "mercari-app" + horizontalPodAutoscaler: tortoise-hpa-mercari + verticalPodAutoscalers: + - name: tortoise-updater-mercari + role: Updater + - name: tortoise-monitor-mercari + role: Monitor + tortoisePhase: Working diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/vpa-Monitor.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/vpa-Monitor.yaml new file mode 100644 index 00000000..73acfb27 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/vpa-Monitor.yaml @@ -0,0 +1,44 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-monitor-mercari + namespace: default +spec: + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: "Off" +status: + conditions: + - lastTransitionTime: null + status: "True" + type: RecommendationProvided + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/vpa-Updater.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/vpa-Updater.yaml new file mode 100644 index 00000000..858222f9 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/vpa-Updater.yaml @@ -0,0 +1,48 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-updater-mercari + namespace: default +spec: + recommenders: + - name: tortoise-controller + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: Auto +status: + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + uncappedTarget: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "3" + memory: 3Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + uncappedTarget: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "3" + memory: 3Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/deployment.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/deployment.yaml new file mode 100644 index 00000000..04ac49e3 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/deployment.yaml @@ -0,0 +1,29 @@ +metadata: + name: mercari-app + namespace: default +spec: + selector: + matchLabels: + app: mercari + strategy: {} + template: + metadata: + creationTimestamp: null + labels: + app: mercari + spec: + containers: + - image: awesome-mercari-app-image + name: app + resources: + requests: + cpu: "10" + memory: 10Gi + - image: awesome-istio-proxy-image + name: istio-proxy + resources: + requests: + cpu: "4" + memory: 4Gi +status: + replicas: 10 diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/hpa.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/hpa.yaml new file mode 100644 index 00000000..d4c64cb0 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/hpa.yaml @@ -0,0 +1,45 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-hpa-mercari + namespace: default +spec: + behavior: + scaleDown: + policies: + - periodSeconds: 90 + type: Percent + value: 2 + selectPolicy: Max + scaleUp: + policies: + - periodSeconds: 60 + type: Percent + value: 100 + selectPolicy: Max + stabilizationWindowSeconds: 0 + maxReplicas: 100 + metrics: + - containerResource: + container: app + name: cpu + target: + averageUtilization: 50 + type: Utilization + type: ContainerResource + - containerResource: + container: istio-proxy + name: cpu + target: + averageUtilization: 50 + type: Utilization + type: ContainerResource + minReplicas: 1 + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + currentMetrics: null + desiredReplicas: 0 diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/tortoise.yaml new file mode 100644 index 00000000..6bedb0a6 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/tortoise.yaml @@ -0,0 +1,84 @@ +metadata: + name: mercari + namespace: default +spec: + resourcePolicy: + - autoscalingPolicy: + cpu: Vertical + memory: Vertical + containerName: app + - autoscalingPolicy: + cpu: Vertical + memory: Vertical + containerName: istio-proxy + targetRefs: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app +status: + conditions: + containerRecommendationFromVPA: + - containerName: app + maxRecommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + recommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + - containerName: istio-proxy + maxRecommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + recommendation: + cpu: + quantity: "0" + updatedAt: null + memory: + quantity: "0" + updatedAt: null + tortoiseConditions: null + recommendations: + horizontal: + maxReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 15 + minReplicas: + - from: 0 + timezone: Local + to: 24 + updatedAt: "2023-10-06T01:15:46Z" + value: 3 + targetUtilizations: + - containerName: app + targetUtilization: + cpu: 50 + - containerName: istio-proxy + targetUtilization: + cpu: 50 + vertical: + containerResourceRecommendation: null + targets: + deployment: mercari-app + horizontalPodAutoscaler: tortoise-hpa-mercari + verticalPodAutoscalers: + - name: tortoise-updater-mercari + role: Updater + - name: tortoise-monitor-mercari + role: Monitor + tortoisePhase: Working diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/vpa-Monitor.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/vpa-Monitor.yaml new file mode 100644 index 00000000..73acfb27 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/vpa-Monitor.yaml @@ -0,0 +1,44 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-monitor-mercari + namespace: default +spec: + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: "Off" +status: + conditions: + - lastTransitionTime: null + status: "True" + type: RecommendationProvided + recommendation: + containerRecommendations: + - containerName: app + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi + - containerName: istio-proxy + lowerBound: + cpu: "3" + memory: 3Gi + target: + cpu: "3" + memory: 3Gi + upperBound: + cpu: "5" + memory: 5Gi diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/vpa-Updater.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/vpa-Updater.yaml new file mode 100644 index 00000000..28c9ee17 --- /dev/null +++ b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/before/vpa-Updater.yaml @@ -0,0 +1,20 @@ +metadata: + annotations: + tortoise.autoscaling.mercari.com/managed-by-tortoise: "true" + tortoises.autoscaling.mercari.com/tortoise-name: mercari + name: tortoise-updater-mercari + namespace: default +spec: + recommenders: + - name: tortoise-controller + resourcePolicy: + containerPolicies: + - containerName: app + - containerName: istio-proxy + targetRef: + apiVersion: apps/v1 + kind: Deployment + name: mercari-app + updatePolicy: + updateMode: Auto +status: {} diff --git a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/tortoise.yaml index 7953890e..7797b75f 100644 --- a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/tortoise.yaml +++ b/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/after/tortoise.yaml @@ -80,8 +80,6 @@ status: memory: 4Gi containerName: istio-proxy targets: - deployment: "" - horizontalPodAutoscaler: tortoise-hpa-mercari verticalPodAutoscalers: - name: tortoise-updater-mercari role: Updater diff --git a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/tortoise.yaml b/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/tortoise.yaml index 6fb80a8e..238f072a 100644 --- a/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/tortoise.yaml +++ b/controllers/testdata/reconcile-for-the-multiple-containers-pod-all-off/before/tortoise.yaml @@ -72,8 +72,6 @@ status: vertical: containerResourceRecommendation: null targets: - deployment: "" - horizontalPodAutoscaler: tortoise-hpa-mercari verticalPodAutoscalers: - name: tortoise-updater-mercari role: Updater diff --git a/controllers/tortoise_controller.go b/controllers/tortoise_controller.go index a7d9e4dc..324cd89b 100644 --- a/controllers/tortoise_controller.go +++ b/controllers/tortoise_controller.go @@ -98,6 +98,14 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{RequeueAfter: r.Interval}, nil } + defer func() { + tortoise = r.TortoiseService.RecordReconciliationFailure(tortoise, reterr, now) + _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now, false) + if err != nil { + logger.Error(err, "update Tortoise status", "tortoise", req.NamespacedName) + } + }() + // tortoise is not deleted. Make sure finalizer is added to tortoise. if err := r.TortoiseService.AddFinalizer(ctx, tortoise); err != nil { return ctrl.Result{}, fmt.Errorf("add finalizer: %w", err) @@ -109,15 +117,6 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{RequeueAfter: requeueAfter}, nil } - // Need to register defer after `ShouldReconcileTortoiseNow` because it updates the tortoise status in it. - defer func() { - tortoise = r.TortoiseService.RecordReconciliationFailure(tortoise, reterr, now) - _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now) - if err != nil { - logger.Error(err, "update Tortoise status", "tortoise", req.NamespacedName) - } - }() - dm, err := r.DeploymentService.GetDeploymentOnTortoise(ctx, tortoise) if err != nil { logger.Error(err, "failed to get deployment", "tortoise", req.NamespacedName) @@ -135,6 +134,8 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{RequeueAfter: r.Interval}, nil } + // TODO: If HPA is modified, we need to change the tortoise phase. + // How to scale during that time? _, err = r.HpaService.UpdateHPASpecFromTortoiseAutoscalingPolicy(ctx, tortoise) if err != nil { if !apierrors.IsNotFound(err) { @@ -142,6 +143,8 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{}, err } + logger.V(4).Info("HPA doesn't exist for this tortoise. We may need to recreate hpa", "tortoise", req.NamespacedName) + // If not found, it's one of: // - the user don't specify Horizontal in any autoscalingPolicy. // - In that case, we don't need to create an initial HPA. @@ -164,7 +167,7 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ if !ready { logger.V(4).Info("VPA created by tortoise isn't ready yet", "tortoise", req.NamespacedName) tortoise.Status.TortoisePhase = autoscalingv1beta1.TortoisePhaseInitializing - _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now) + _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now, true) if err != nil { logger.Error(err, "update Tortoise status", "tortoise", req.NamespacedName) return ctrl.Result{}, err @@ -187,7 +190,7 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{}, err } - _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now) + _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now, true) if err != nil { logger.Error(err, "update Tortoise status", "tortoise", req.NamespacedName) return ctrl.Result{}, err @@ -212,7 +215,7 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ return ctrl.Result{}, err } - _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now) + _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now, true) if err != nil { logger.Error(err, "update Tortoise status", "tortoise", req.NamespacedName) return ctrl.Result{}, err @@ -262,7 +265,7 @@ func (r *TortoiseReconciler) initializeVPAAndHPA(ctx context.Context, tortoise * if err != nil { return fmt.Errorf("create tortoise updater VPA: %w", err) } - _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now) + _, err = r.TortoiseService.UpdateTortoiseStatus(ctx, tortoise, now, true) if err != nil { return fmt.Errorf("update Tortoise status: %w", err) } diff --git a/controllers/tortoise_controller_test.go b/controllers/tortoise_controller_test.go index 87725057..59b4ca5d 100644 --- a/controllers/tortoise_controller_test.go +++ b/controllers/tortoise_controller_test.go @@ -37,27 +37,32 @@ func newResource(path string) resources { updaterVPAPath := fmt.Sprintf("%s/vpa-Updater.yaml", path) monitorVPAPath := fmt.Sprintf("%s/vpa-Monitor.yaml", path) + var tortoise *v1beta1.Tortoise y, err := os.ReadFile(tortoisePath) - Expect(err).NotTo(HaveOccurred()) - tortoise := &v1beta1.Tortoise{} - err = yaml.Unmarshal(y, tortoise) - Expect(err).NotTo(HaveOccurred()) + if err == nil { + tortoise = &v1beta1.Tortoise{} + err = yaml.Unmarshal(y, tortoise) + Expect(err).NotTo(HaveOccurred()) + } + var vpa *autoscalingv1.VerticalPodAutoscaler y, err = os.ReadFile(updaterVPAPath) - Expect(err).NotTo(HaveOccurred()) - vpa := &autoscalingv1.VerticalPodAutoscaler{} - err = yaml.Unmarshal(y, vpa) - Expect(err).NotTo(HaveOccurred()) + if err == nil { + vpa = &autoscalingv1.VerticalPodAutoscaler{} + err = yaml.Unmarshal(y, vpa) + Expect(err).NotTo(HaveOccurred()) + } + var vpa2 *autoscalingv1.VerticalPodAutoscaler y, err = os.ReadFile(monitorVPAPath) - Expect(err).NotTo(HaveOccurred()) - vpa2 := &autoscalingv1.VerticalPodAutoscaler{} - err = yaml.Unmarshal(y, vpa2) - Expect(err).NotTo(HaveOccurred()) + if err == nil { + vpa2 = &autoscalingv1.VerticalPodAutoscaler{} + err = yaml.Unmarshal(y, vpa2) + Expect(err).NotTo(HaveOccurred()) + } var deploy *v1.Deployment y, err = os.ReadFile(deploymentPath) - // maybe deployment file is not exist if err == nil { deploy = &v1.Deployment{} err = yaml.Unmarshal(y, deploy) @@ -66,9 +71,7 @@ func newResource(path string) resources { var hpa *v2.HorizontalPodAutoscaler y, err = os.ReadFile(hpaPath) - // maybe hpa file is not exist if err == nil { - Expect(err).NotTo(HaveOccurred()) hpa = &v2.HorizontalPodAutoscaler{} err = yaml.Unmarshal(y, hpa) Expect(err).NotTo(HaveOccurred()) @@ -84,7 +87,6 @@ func newResource(path string) resources { }, } } - func createDeploymentWithStatus(ctx context.Context, k8sClient client.Client, deploy *v1.Deployment) { err := k8sClient.Create(ctx, deploy.DeepCopy()) Expect(err).NotTo(HaveOccurred()) @@ -147,6 +149,7 @@ func startController(ctx context.Context) func() { }) Expect(err).ShouldNot(HaveOccurred()) + // We only reconcile once. tortoiseService, err := tortoise.New(mgr.GetClient(), record.NewFakeRecorder(10), 1, "Asia/Tokyo", 1000*time.Minute, "weekly") Expect(err).ShouldNot(HaveOccurred()) cli, err := vpa.New(mgr.GetConfig(), record.NewFakeRecorder(10)) @@ -204,6 +207,7 @@ var _ = Describe("Test TortoiseController", func() { initializeResourcesFromFiles(ctx, k8sClient, filepath.Join(path, "before")) stopFunc = startController(ctx) tc := testCase{want: newResource(filepath.Join(path, "after"))} + time.Sleep(1 * time.Second) Eventually(func(g Gomega) { gotTortoise := &v1beta1.Tortoise{} err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "default", Name: "mercari"}, gotTortoise) @@ -213,6 +217,12 @@ var _ = Describe("Test TortoiseController", func() { gotHPA = &v2.HorizontalPodAutoscaler{} err = k8sClient.Get(ctx, client.ObjectKey{Namespace: "default", Name: "tortoise-hpa-mercari"}, gotHPA) g.Expect(err).ShouldNot(HaveOccurred()) + } else { + // HPA should not exist. + gotHPA = &v2.HorizontalPodAutoscaler{} + err = k8sClient.Get(ctx, client.ObjectKey{Namespace: "default", Name: "tortoise-hpa-mercari"}, gotHPA) + Expect(apierrors.IsNotFound(err)).To(Equal(true)) + gotHPA = nil } gotUpdaterVPA := &autoscalingv1.VerticalPodAutoscaler{} err = k8sClient.Get(ctx, client.ObjectKey{Namespace: "default", Name: "tortoise-updater-mercari"}, gotUpdaterVPA) @@ -279,6 +289,17 @@ var _ = Describe("Test TortoiseController", func() { runTest(filepath.Join("testdata", "reconcile-for-the-multiple-containers-pod-emergency")) }) }) + Context("mutable AutoscalingPolicy", func() { + It("Tortoise get Horizontal and create HPA", func() { + runTest(filepath.Join("testdata", "mutable-autoscalingpolicy-no-hpa-and-add-horizontal")) + }) + It("Tortoise get another Horizontal and modify the existing HPA", func() { + runTest(filepath.Join("testdata", "mutable-autoscalingpolicy-add-another-horizontal")) + }) + It("Horizontal is removed and modify the existing HPA", func() { + runTest(filepath.Join("testdata", "mutable-autoscalingpolicy-remove-horizontal")) + }) + }) Context("DeletionPolicy is handled correctly", func() { It("[DeletionPolicy = DeleteAll] delete HPA and VPAs when Tortoise is deleted", func() { resource := initializeResourcesFromFiles(ctx, k8sClient, "testdata/deletion-policy-all/before") diff --git a/pkg/hpa/service.go b/pkg/hpa/service.go index 56029151..5b29ceaf 100644 --- a/pkg/hpa/service.go +++ b/pkg/hpa/service.go @@ -19,6 +19,7 @@ import ( "k8s.io/client-go/util/retry" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" autoscalingv1beta1 "github.com/mercari/tortoise/api/v1beta1" "github.com/mercari/tortoise/pkg/annotation" @@ -43,12 +44,16 @@ func New(c client.Client, recorder record.EventRecorder, replicaReductionFactor } func (c *Service) InitializeHPA(ctx context.Context, tortoise *autoscalingv1beta1.Tortoise, dm *v1.Deployment) (*autoscalingv1beta1.Tortoise, error) { + logger := log.FromContext(ctx) // if all policy is off or Vertical, we don't need HPA. if !hasHorizontal(tortoise) { + logger.V(4).Info("no horizontal policy, no need to create HPA") return tortoise, nil } if tortoise.Spec.TargetRefs.HorizontalPodAutoscalerName != nil { + logger.V(4).Info("user specified the existing HPA, no need to create HPA") + // update the existing HPA that the user set on tortoise. tortoise, err := c.giveAnnotationsOnExistingHPA(ctx, tortoise) if err != nil { @@ -59,6 +64,7 @@ func (c *Service) InitializeHPA(ctx context.Context, tortoise *autoscalingv1beta return tortoise, nil } + logger.V(4).Info("no existing HPA specified, creating HPA") // create default HPA. _, tortoise, err := c.CreateHPA(ctx, tortoise, dm) @@ -97,7 +103,7 @@ func (c *Service) giveAnnotationsOnExistingHPA(ctx context.Context, tortoise *au } func (c *Service) DeleteHPACreatedByTortoise(ctx context.Context, tortoise *autoscalingv1beta1.Tortoise) error { - if tortoise.Spec.TargetRefs.HorizontalPodAutoscalerName != nil || tortoise.Spec.DeletionPolicy == autoscalingv1beta1.DeletionPolicyNoDelete { + if tortoise.Spec.DeletionPolicy == autoscalingv1beta1.DeletionPolicyNoDelete { // A user specified the existing HPA and tortoise didn't create HPA by itself. return nil } @@ -180,15 +186,17 @@ func (c *Service) addHPAMetricsFromTortoiseAutoscalingPolicy(ctx context.Context } // remove metrics - for i, m := range currenthpa.Spec.Metrics { + newMetrics := []v2.MetricSpec{} + for _, m := range currenthpa.Spec.Metrics { if m.Type != v2.ContainerResourceMetricSourceType { continue } - if needToRemoveFromHPA.Has(resourceNameAndContainerName{m.ContainerResource.Name, m.ContainerResource.Container}) { - currenthpa.Spec.Metrics = append(currenthpa.Spec.Metrics[:i], currenthpa.Spec.Metrics[i+1:]...) + if !needToRemoveFromHPA.Has(resourceNameAndContainerName{m.ContainerResource.Name, m.ContainerResource.Container}) { + newMetrics = append(newMetrics, m) continue } } + currenthpa.Spec.Metrics = newMetrics return currenthpa } @@ -312,6 +320,14 @@ func (c *Service) ChangeHPAFromTortoiseRecommendation(tortoise *autoscalingv1bet } func (c *Service) UpdateHPASpecFromTortoiseAutoscalingPolicy(ctx context.Context, tortoise *autoscalingv1beta1.Tortoise) (*v2.HorizontalPodAutoscaler, error) { + if !hasHorizontal(tortoise) { + err := c.DeleteHPACreatedByTortoise(ctx, tortoise) + if err != nil { + return nil, fmt.Errorf("delete hpa created by tortoise: %w", err) + } + return nil, nil + } + hpa := &v2.HorizontalPodAutoscaler{} if err := c.c.Get(ctx, types.NamespacedName{Namespace: tortoise.Namespace, Name: tortoise.Status.Targets.HorizontalPodAutoscaler}, hpa); err != nil { return nil, fmt.Errorf("failed to get hpa on tortoise: %w", err) diff --git a/pkg/hpa/service_test.go b/pkg/hpa/service_test.go index 09445da1..d7d79abf 100644 --- a/pkg/hpa/service_test.go +++ b/pkg/hpa/service_test.go @@ -10,6 +10,7 @@ import ( appv1 "k8s.io/api/apps/v1" v2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" "k8s.io/utils/pointer" @@ -1394,6 +1395,124 @@ func TestService_UpdateHPASpecFromTortoiseAutoscalingPolicy(t *testing.T) { }, }, }, + { + name: "remove all metrics from hpa", + args: args{ + tortoise: &autoscalingv1beta1.Tortoise{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tortoise", + Namespace: "default", + }, + Spec: autoscalingv1beta1.TortoiseSpec{ + DeletionPolicy: autoscalingv1beta1.DeletionPolicyDeleteAll, + ResourcePolicy: []autoscalingv1beta1.ContainerResourcePolicy{ + { + ContainerName: "app", + AutoscalingPolicy: map[v1.ResourceName]v1beta1.AutoscalingType{ + v1.ResourceMemory: v1beta1.AutoscalingTypeVertical, + v1.ResourceCPU: v1beta1.AutoscalingTypeVertical, + }, + }, + { + ContainerName: "istio-proxy", + AutoscalingPolicy: map[v1.ResourceName]v1beta1.AutoscalingType{ + v1.ResourceMemory: v1beta1.AutoscalingTypeVertical, + v1.ResourceCPU: v1beta1.AutoscalingTypeVertical, + }, + }, + }, + TargetRefs: autoscalingv1beta1.TargetRefs{ + HorizontalPodAutoscalerName: pointer.String("existing-hpa"), + ScaleTargetRef: autoscalingv1beta1.CrossVersionObjectReference{ + Kind: "Deployment", + Name: "deployment", + }, + }, + }, + Status: autoscalingv1beta1.TortoiseStatus{ + Targets: autoscalingv1beta1.TargetsStatus{ + HorizontalPodAutoscaler: "hpa", + }, + }, + }, + dm: &appv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment", + Namespace: "default", + }, + Spec: appv1.DeploymentSpec{ + Replicas: pointer.Int32(4), + Template: v1.PodTemplateSpec{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "app", + }, + }, + }, + }, + }, + Status: appv1.DeploymentStatus{ + Replicas: 4, + }, + }, + }, + initialHPA: &v2.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hpa", + Namespace: "default", + Annotations: map[string]string{ + annotation.TortoiseNameAnnotation: "tortoise", + annotation.ManagedByTortoiseAnnotation: "true", + }, + }, + Spec: v2.HorizontalPodAutoscalerSpec{ + MinReplicas: ptrInt32(1), + MaxReplicas: 2, + ScaleTargetRef: v2.CrossVersionObjectReference{ + Kind: "Deployment", + Name: "deployment", + APIVersion: "apps/v1", + }, + Metrics: []v2.MetricSpec{ + { + Type: v2.ContainerResourceMetricSourceType, + ContainerResource: &v2.ContainerResourceMetricSource{ + Name: v1.ResourceCPU, + Container: "app", + Target: v2.MetricTarget{ + AverageUtilization: pointer.Int32(50), + Type: v2.UtilizationMetricType, + }, + }, + }, + { + Type: v2.ContainerResourceMetricSourceType, + ContainerResource: &v2.ContainerResourceMetricSource{ + Name: v1.ResourceCPU, + Container: "istio-proxy", + Target: v2.MetricTarget{ + AverageUtilization: pointer.Int32(50), + Type: v2.UtilizationMetricType, + }, + }, + }, + }, + Behavior: &v2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &v2.HPAScalingRules{ + Policies: []v2.HPAScalingPolicy{ + { + Type: v2.PercentScalingPolicy, + Value: 2, + PeriodSeconds: 90, + }, + }, + }, + }, + }, + }, + afterHPA: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1406,12 +1525,21 @@ func TestService_UpdateHPASpecFromTortoiseAutoscalingPolicy(t *testing.T) { t.Errorf("Service.UpdateHPASpecFromTortoiseAutoscalingPolicy() error = %v, wantErr %v", err, tt.wantErr) return } + if tt.afterHPA == nil { + // hpa should be removed + hpa := &v2.HorizontalPodAutoscaler{} + err = c.c.Get(context.Background(), client.ObjectKey{Name: tt.initialHPA.Name, Namespace: tt.initialHPA.Namespace}, hpa) + if err == nil || !apierrors.IsNotFound(err) { + t.Errorf("hpa should be removed") + } + return + } + hpa := &v2.HorizontalPodAutoscaler{} err = c.c.Get(context.Background(), client.ObjectKey{Name: tt.afterHPA.Name, Namespace: tt.afterHPA.Namespace}, hpa) if err != nil { t.Errorf("get hpa error = %v", err) } - if d := cmp.Diff(tt.afterHPA, hpa, cmpopts.IgnoreFields(v2.HorizontalPodAutoscaler{}, "TypeMeta"), cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion")); d != "" { t.Errorf("Service.UpdateHPASpecFromTortoiseAutoscalingPolicy() hpa diff = %v", d) } diff --git a/pkg/recommender/recommender.go b/pkg/recommender/recommender.go index 6ffaaafa..676a1bf5 100644 --- a/pkg/recommender/recommender.go +++ b/pkg/recommender/recommender.go @@ -307,7 +307,7 @@ func (s *Service) updateHPATargetUtilizationRecommendations(ctx context.Context, targetMap[k] = s.upperTargetResourceUtilization } } - logger.Info("HPA target utilization recommendation is created", "current target utilization", targetValue, "recommended target utilization", targetMap[k], "upper usage", upperUsage) + logger.Info("HPA target utilization recommendation is created", "current target utilization", targetValue, "recommended target utilization", targetMap[k], "upper usage", upperUsage, "container name", r.ContainerName, "resource name", k) } newHPATargetUtilizationRecommendationPerContainer = append(newHPATargetUtilizationRecommendationPerContainer, v1beta1.HPATargetUtilizationRecommendationPerContainer{ ContainerName: r.ContainerName, diff --git a/pkg/tortoise/tortoise.go b/pkg/tortoise/tortoise.go index 37610b94..4c0d3041 100644 --- a/pkg/tortoise/tortoise.go +++ b/pkg/tortoise/tortoise.go @@ -292,7 +292,7 @@ func (s *Service) RemoveFinalizer(ctx context.Context, tortoise *v1beta1.Tortois return nil } -func (s *Service) UpdateTortoiseStatus(ctx context.Context, originalTortoise *v1beta1.Tortoise, now time.Time) (*v1beta1.Tortoise, error) { +func (s *Service) UpdateTortoiseStatus(ctx context.Context, originalTortoise *v1beta1.Tortoise, now time.Time, timeRecord bool) (*v1beta1.Tortoise, error) { logger := log.FromContext(ctx) logger.V(4).Info("update tortoise status", "tortoise", klog.KObj(originalTortoise)) retTortoise := &v1beta1.Tortoise{} @@ -317,7 +317,9 @@ func (s *Service) UpdateTortoiseStatus(ctx context.Context, originalTortoise *v1 return originalTortoise, err } - s.updateLastTimeUpdateTortoise(originalTortoise, now) + if timeRecord { + s.updateLastTimeUpdateTortoise(originalTortoise, now) + } return originalTortoise, nil }