calculate the recommendation based on a correct resource request (#313)

mercari · Feb 6, 2024 · e3c408a · e3c408a
1 parent 83e3194
commit e3c408a
Show file tree

Hide file tree

Showing 24 changed files with 373 additions and 65 deletions.
diff --git a/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-add-another-horizontal/after/tortoise.yaml
@@ -21,8 +21,8 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"

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
@@ -21,12 +21,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
-          cpu: "4"
-          memory: 4Gi
+          cpu: "3"
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal-2/after/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal-2/after/tortoise.yaml
@@ -31,12 +31,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "3"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
-          cpu: "4"
-          memory: 4Gi
+          cpu: "3"
+          memory: 3Gi
     containerRecommendationFromVPA:
     - containerName: app
       maxRecommendation:

diff --git a/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/tortoise.yaml b/controllers/testdata/mutable-autoscalingpolicy-remove-horizontal/after/tortoise.yaml
@@ -30,12 +30,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "3"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
-          cpu: "4"
-          memory: 4Gi
+          cpu: "3"
+          memory: 3Gi
     containerRecommendationFromVPA:
     - containerName: app
       maxRecommendation:

diff --git a/controllers/testdata/reconcile-for-the-istio-enabled-pod-working/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-istio-enabled-pod-working/after/tortoise.yaml
@@ -22,11 +22,11 @@ status:
       - containerName: "app"
         resource:
           cpu: "10"
-          memory: 10Gi
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/...llers/testdata/reconcile-for-the-multiple-containers-pod-all-vertical/after/tortoise.yaml b/...llers/testdata/reconcile-for-the-multiple-containers-pod-all-vertical/after/tortoise.yaml
@@ -21,12 +21,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "3"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
-          cpu: "4"
-          memory: 4Gi
+          cpu: "3"
+          memory: 3Gi
     containerRecommendationFromVPA:
     - containerName: app
       maxRecommendation:

diff --git a/...s/testdata/reconcile-for-the-multiple-containers-pod-during-emergency/after/tortoise.yaml b/...s/testdata/reconcile-for-the-multiple-containers-pod-during-emergency/after/tortoise.yaml
@@ -22,12 +22,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/.../testdata/reconcile-for-the-multiple-containers-pod-during-emergency/before/tortoise.yaml b/.../testdata/reconcile-for-the-multiple-containers-pod-during-emergency/before/tortoise.yaml
@@ -22,12 +22,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/.../testdata/reconcile-for-the-multiple-containers-pod-emergency-started/after/tortoise.yaml b/.../testdata/reconcile-for-the-multiple-containers-pod-emergency-started/after/tortoise.yaml
@@ -22,12 +22,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/testdata/reconcile-for-the-multiple-containers-pod-one-off/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-multiple-containers-pod-one-off/after/tortoise.yaml
@@ -21,8 +21,8 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"

diff --git a/...estdata/reconcile-for-the-multiple-containers-pod-suggested-too-small/after/tortoise.yaml b/...estdata/reconcile-for-the-multiple-containers-pod-suggested-too-small/after/tortoise.yaml
@@ -22,11 +22,11 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 10Mi
       - containerName: "istio-proxy"
         resource:
           cpu: "100m"
-          memory: "100Mi"
+          memory: "11Mi"
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/testdata/reconcile-for-the-multiple-containers-pod-working/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-multiple-containers-pod-working/after/tortoise.yaml
@@ -21,12 +21,12 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "10"
-          memory: 10Gi
+          cpu: "6"
+          memory: 3Gi
       - containerName: "istio-proxy"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/testdata/reconcile-for-the-single-container-pod-backtonormal/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-single-container-pod-backtonormal/after/tortoise.yaml
@@ -19,7 +19,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/...lers/testdata/reconcile-for-the-single-container-pod-during-emergency/after/tortoise.yaml b/...lers/testdata/reconcile-for-the-single-container-pod-during-emergency/after/tortoise.yaml
@@ -19,7 +19,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/...ers/testdata/reconcile-for-the-single-container-pod-during-emergency/before/tortoise.yaml b/...ers/testdata/reconcile-for-the-single-container-pod-during-emergency/before/tortoise.yaml
@@ -19,7 +19,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/...ers/testdata/reconcile-for-the-single-container-pod-emergency-started/after/tortoise.yaml b/...ers/testdata/reconcile-for-the-single-container-pod-emergency-started/after/tortoise.yaml
@@ -19,7 +19,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/...stdata/reconcile-for-the-single-container-pod-gathering-data-finished/after/tortoise.yaml b/...stdata/reconcile-for-the-single-container-pod-gathering-data-finished/after/tortoise.yaml
@@ -18,7 +18,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/testdata/reconcile-for-the-single-container-pod-hpa-changed/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-single-container-pod-hpa-changed/after/tortoise.yaml
@@ -18,7 +18,7 @@ status:
     containerResourceRequests:
       - containerName: "app"
         resource:
-          cpu: "4"
+          cpu: "3"
           memory: 4Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"

diff --git a/...ollers/testdata/reconcile-for-the-single-container-pod-partly-working/after/tortoise.yaml b/...ollers/testdata/reconcile-for-the-single-container-pod-partly-working/after/tortoise.yaml
@@ -18,7 +18,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/testdata/reconcile-for-the-single-container-pod-working/after/tortoise.yaml b/controllers/testdata/reconcile-for-the-single-container-pod-working/after/tortoise.yaml
@@ -18,7 +18,7 @@ status:
       - containerName: "app"
         resource:
           cpu: "4"
-          memory: 4Gi
+          memory: 3Gi
     tortoiseConditions:
     - message: "HPA target utilization is updated"
       reason: HPATargetUtilizationUpdated

diff --git a/controllers/tortoise_controller.go b/controllers/tortoise_controller.go
@@ -139,16 +139,17 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		return ctrl.Result{}, err
 	}
 	currentReplicaNum := dm.Status.Replicas
-	acr, err := r.DeploymentService.GetResourceRequests(dm)
-	if err != nil {
-		logger.Error(err, "failed to get resource requests in deployment", "tortoise", req.NamespacedName, "deployment", klog.KObj(dm))
-		return ctrl.Result{}, err
-	}
-
-	tortoise.Status.Conditions.ContainerResourceRequests = acr
 
-	// === Finish the part depending on deployment ===
-	// From here, we shouldn't use `dm` anymore.
+	if tortoise.Spec.UpdateMode == autoscalingv1beta3.UpdateModeOff || tortoise.Status.Conditions.ContainerResourceRequests == nil {
+		// If the update mode is off, we have to update ContainerResourceRequests from the deployment directly.
+		// If it's not off, ContainerResourceRequests should be updated in UpdateVPAFromTortoiseRecommendation in the last reconciliation.
+		acr, err := r.DeploymentService.GetResourceRequests(dm)
+		if err != nil {
+			logger.Error(err, "failed to get resource requests in deployment", "tortoise", req.NamespacedName, "deployment", klog.KObj(dm))
+			return ctrl.Result{}, err
+		}
+		tortoise.Status.Conditions.ContainerResourceRequests = acr
+	}
 
 	hpa, err := r.HpaService.GetHPAOnTortoiseSpec(ctx, tortoise)
 	if err != nil {
@@ -236,7 +237,7 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		return ctrl.Result{}, err
 	}
 
-	_, updated, err := r.VpaService.UpdateVPAFromTortoiseRecommendation(ctx, tortoise, currentReplicaNum, now)
+	_, tortoise, updated, err := r.VpaService.UpdateVPAFromTortoiseRecommendation(ctx, tortoise, currentReplicaNum, now)
 	if err != nil {
 		logger.Error(err, "update VPA based on the recommendation in tortoise", "tortoise", req.NamespacedName)
 		return ctrl.Result{}, err

diff --git a/pkg/deployment/service.go b/pkg/deployment/service.go
@@ -57,6 +57,7 @@ func (c *Service) RolloutRestart(ctx context.Context, dm *v1.Deployment, tortois
 	return nil
 }
 
+// GetResourceRequests returns the resource requests of the containers in the deployment.
 func (c *Service) GetResourceRequests(dm *v1.Deployment) ([]autoscalingv1beta3.ContainerResourceRequests, error) {
 	actualContainerResource := []autoscalingv1beta3.ContainerResourceRequests{}
 	for _, c := range dm.Spec.Template.Spec.Containers {

diff --git a/pkg/vpa/service.go b/pkg/vpa/service.go
@@ -232,7 +232,7 @@ func (c *Service) CreateTortoiseMonitorVPA(ctx context.Context, tortoise *autosc
 
 // UpdateVPAFromTortoiseRecommendation updates VPA with the recommendation from Tortoise.
 // In the second return value, it returns true if the Pods should be updated with new resources.
-func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise, replica int32, now time.Time) (*v1.VerticalPodAutoscaler, bool, error) {
+func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise, replica int32, now time.Time) (*v1.VerticalPodAutoscaler, *autoscalingv1beta3.Tortoise, bool, error) {
 	newVPA := &v1.VerticalPodAutoscaler{}
 	// At the end of this function, this will be true:
 	// - if UpdateMode is Off, this will be always false.
@@ -250,6 +250,7 @@ func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, torto
 		}
 		newVPA = oldVPA.DeepCopy()
 		newRecommendations := make([]v1.RecommendedContainerResources, 0, len(tortoise.Status.Recommendations.Vertical.ContainerResourceRecommendation))
+		newRequests := make([]autoscalingv1beta3.ContainerResourceRequests, 0, len(tortoise.Status.Recommendations.Vertical.ContainerResourceRecommendation))
 		for _, r := range tortoise.Status.Recommendations.Vertical.ContainerResourceRecommendation {
 			if !metricsRecorded {
 				// only record metrics once in every reconcile loop.
@@ -272,6 +273,10 @@ func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, torto
 				UpperBound:     r.RecommendedResource,
 				UncappedTarget: r.RecommendedResource,
 			})
+			newRequests = append(newRequests, autoscalingv1beta3.ContainerResourceRequests{
+				ContainerName: r.ContainerName,
+				Resource:      r.RecommendedResource,
+			})
 		}
 		metricsRecorded = true
 		if tortoise.Spec.UpdateMode == autoscalingv1beta3.UpdateModeOff {
@@ -316,6 +321,10 @@ func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, torto
 			}
 		}
 
+		// The recommendation will be applied to VPA and the deployment will be restarted with the new resources.
+		// Update the request recorded in the status, which will be used in the next reconcile loop.
+		tortoise.Status.Conditions.ContainerResourceRequests = newRequests
+
 		newVPA.Status.Conditions = []v1.VerticalPodAutoscalerCondition{
 			{
 				Type:               v1.RecommendationProvided,
@@ -359,7 +368,7 @@ func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, torto
 	}
 
 	if err := retry.RetryOnConflict(retry.DefaultRetry, updateFn); err != nil {
-		return newVPA, podShouldBeUpdatedWithNewResource, fmt.Errorf("update VPA status: %w", err)
+		return newVPA, tortoise, podShouldBeUpdatedWithNewResource, fmt.Errorf("update VPA status: %w", err)
 	}
 
 	if tortoise.Spec.UpdateMode != autoscalingv1beta3.UpdateModeOff && podShouldBeUpdatedWithNewResource {
@@ -378,7 +387,7 @@ func (c *Service) UpdateVPAFromTortoiseRecommendation(ctx context.Context, torto
 		}
 	}
 
-	return newVPA, podShouldBeUpdatedWithNewResource, nil
+	return newVPA, tortoise, podShouldBeUpdatedWithNewResource, nil
 }
 
 func recommendationIncreaseAnyResource(oldVPA, newVPA *v1.VerticalPodAutoscaler) bool {