remove VPA updator disablement (#396)

mercari · Mar 29, 2024 · 1ed8fe1 · 1ed8fe1
1 parent 097a5c9
commit 1ed8fe1
Show file tree

Hide file tree

Showing 15 changed files with 1 addition and 118 deletions.
diff --git a/api/autoscaling/v2/testdata/mutating/mutate-by-one-recommendation/tortoise.yaml b/api/autoscaling/v2/testdata/mutating/mutate-by-one-recommendation/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/mutating/mutate-by-recommendations-back-to-normal/tortoise.yaml b/api/autoscaling/v2/testdata/mutating/mutate-by-recommendations-back-to-normal/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/mutating/mutate-by-recommendations/tortoise.yaml b/api/autoscaling/v2/testdata/mutating/mutate-by-recommendations/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/mutating/no-mutate-by-recommendations-when-dryrun/tortoise.yaml b/api/autoscaling/v2/testdata/mutating/no-mutate-by-recommendations-when-dryrun/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/mutating/no-tortoise-for-this-hpa/tortoise.yaml b/api/autoscaling/v2/testdata/mutating/no-tortoise-for-this-hpa/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/validating/hpa-with-auto-being-deleted/tortoise.yaml b/api/autoscaling/v2/testdata/validating/hpa-with-auto-being-deleted/tortoise.yaml
@@ -44,8 +44,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/validating/hpa-with-auto-existing/tortoise.yaml b/api/autoscaling/v2/testdata/validating/hpa-with-auto-existing/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/autoscaling/v2/testdata/validating/hpa-with-off/tortoise.yaml b/api/autoscaling/v2/testdata/validating/hpa-with-off/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/core/v1/testdata/mutating/auto-tortoise-no-dp/tortoise.yaml b/api/core/v1/testdata/mutating/auto-tortoise-no-dp/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/core/v1/testdata/mutating/auto-tortoise-no-replicaset/tortoise.yaml b/api/core/v1/testdata/mutating/auto-tortoise-no-replicaset/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/api/core/v1/testdata/mutating/auto-tortoise-no-tortoise/tortoise.yaml b/api/core/v1/testdata/mutating/auto-tortoise-no-tortoise/tortoise.yaml
@@ -44,8 +44,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerResourceRequests:
     - containerName: nginx

diff --git a/api/core/v1/testdata/mutating/auto-tortoise/tortoise.yaml b/api/core/v1/testdata/mutating/auto-tortoise/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerResourceRequests:
     - containerName: nginx

diff --git a/api/core/v1/testdata/mutating/off-tortoise/tortoise.yaml b/api/core/v1/testdata/mutating/off-tortoise/tortoise.yaml
@@ -43,8 +43,6 @@ status:
     verticalPodAutoscalers: 
     - name: tortoise-monitor-sample
       role: Monitor
-    - name: tortoise-updater-sample
-      role: Updater
   conditions:
     containerRecommendationFromVPA:
     - containerName: echo

diff --git a/controllers/tortoise_controller.go b/controllers/tortoise_controller.go
@@ -147,14 +147,6 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
 
-	// Previously, we had VPA called "updator vpa". We don't need it anymore so we disable it here if Tortoise still has.
-	// This logic can be removed later.
-	err = r.VpaService.DisableTortoiseUpdaterVPA(ctx, tortoise)
-	if err != nil {
-		logger.Error(err, "delete updater VPA created by tortoise", "tortoise", req.NamespacedName)
-		return ctrl.Result{}, err
-	}
-
 	// TODO: stop depending on deployment.
 	// https://github.com/mercari/tortoise/issues/129
 	//
@@ -315,10 +307,6 @@ func (r *TortoiseReconciler) deleteVPAAndHPA(ctx context.Context, tortoise *auto
 	if err != nil {
 		return fmt.Errorf("delete monitor VPA created by tortoise: %w", err)
 	}
-	err = r.VpaService.DeleteTortoiseUpdaterVPA(ctx, tortoise)
-	if err != nil {
-		return fmt.Errorf("delete updater VPA created by tortoise: %w", err)
-	}
 	return nil
 }
 

diff --git a/pkg/vpa/service.go b/pkg/vpa/service.go
@@ -35,16 +35,11 @@ func New(c *rest.Config, recorder record.EventRecorder) (*Service, error) {
 }
 
 const tortoiseMonitorVPANamePrefix = "tortoise-monitor-"
-const tortoiseUpdaterVPANamePrefix = "tortoise-updater-"
 
 func TortoiseMonitorVPAName(tortoiseName string) string {
 	return tortoiseMonitorVPANamePrefix + tortoiseName
 }
 
-func TortoiseUpdaterVPAName(tortoiseName string) string {
-	return tortoiseUpdaterVPANamePrefix + tortoiseName
-}
-
 func (c *Service) DeleteTortoiseMonitorVPA(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise) error {
 	if tortoise.Spec.DeletionPolicy == autoscalingv1beta3.DeletionPolicyNoDelete {
 		return nil
@@ -65,80 +60,6 @@ func (c *Service) DeleteTortoiseMonitorVPA(ctx context.Context, tortoise *autosc
 	return nil
 }
 
-func (c *Service) GetTortoiseUpdaterVPA(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise) (*v1.VerticalPodAutoscaler, error) {
-	vpa, err := c.c.AutoscalingV1().VerticalPodAutoscalers(tortoise.Namespace).Get(ctx, TortoiseUpdaterVPAName(tortoise.Name), metav1.GetOptions{})
-	if err != nil {
-		return nil, fmt.Errorf("failed to get updater vpa on tortoise: %w", err)
-	}
-	return vpa, nil
-}
-
-func (c *Service) DeleteTortoiseUpdaterVPA(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise) error {
-	if tortoise.Spec.DeletionPolicy == autoscalingv1beta3.DeletionPolicyNoDelete {
-		return nil
-	}
-
-	vpa, err := c.c.AutoscalingV1().VerticalPodAutoscalers(tortoise.Namespace).Get(ctx, TortoiseUpdaterVPAName(tortoise.Name), metav1.GetOptions{})
-	if err != nil {
-		if apierrors.IsNotFound(err) {
-			// already deleted
-			return nil
-		}
-		return fmt.Errorf("failed to get vpa: %w", err)
-	}
-
-	if err := c.c.AutoscalingV1().VerticalPodAutoscalers(tortoise.Namespace).Delete(ctx, vpa.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
-		return fmt.Errorf("failed to delete vpa: %w", err)
-	}
-	return nil
-}
-
-// DisableTortoiseUpdaterVPA is to disable the updater VPA by removing the recommendation from the VPA.
-func (c *Service) DisableTortoiseUpdaterVPA(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise) error {
-	// we only want to record metric once in every reconcile loop.
-	updateFn := func() error {
-		oldVPA, err := c.GetTortoiseUpdaterVPA(ctx, tortoise)
-		if err != nil {
-			if apierrors.IsNotFound(err) {
-				return nil
-			}
-			return fmt.Errorf("get tortoise VPA: %w", err)
-		}
-		if oldVPA.Status.Recommendation == nil {
-			// Already no recommendation.
-			return nil
-		}
-
-		// Remove the recommendation from the VPA.
-		oldVPA.Status.Recommendation.ContainerRecommendations = nil
-
-		// If VPA CRD in the cluster hasn't got the status subresource yet, this will update the status as well.
-		newVPA2, err := c.c.AutoscalingV1().VerticalPodAutoscalers(oldVPA.Namespace).Update(ctx, oldVPA, metav1.UpdateOptions{})
-		if err != nil {
-			return fmt.Errorf("update VPA MinReplicas (%s/%s): %w", oldVPA.Namespace, oldVPA.Name, err)
-		}
-		newVPA2.Status = oldVPA.Status
-
-		// Then, we update VPA status (Recommendation).
-		_, err = c.c.AutoscalingV1().VerticalPodAutoscalers(oldVPA.Namespace).UpdateStatus(ctx, newVPA2, metav1.UpdateOptions{})
-		if err != nil {
-			if apierrors.IsNotFound(err) {
-				// Ignore it. Probably it's because VPA CRD hasn't got the status subresource yet.
-				return nil
-			}
-			return fmt.Errorf("update VPA (%s/%s) status: %w", newVPA2.Namespace, newVPA2.Name, err)
-		}
-
-		return nil
-	}
-
-	if err := retry.RetryOnConflict(retry.DefaultRetry, updateFn); err != nil {
-		return fmt.Errorf("update VPA status: %w", err)
-	}
-
-	return nil
-}
-
 // UpdateVPAContainerResourcePolicy is update VPA to have appropriate container policies based on tortoises' resource policy.
 func (c *Service) UpdateVPAContainerResourcePolicy(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise, vpa *v1.VerticalPodAutoscaler) (*v1.VerticalPodAutoscaler, error) {
 	retVPA := &v1.VerticalPodAutoscaler{}
@@ -213,7 +134,7 @@ func (c *Service) CreateTortoiseMonitorVPA(ctx context.Context, tortoise *autosc
 func (c *Service) GetTortoiseMonitorVPA(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise) (*v1.VerticalPodAutoscaler, bool, error) {
 	vpa, err := c.c.AutoscalingV1().VerticalPodAutoscalers(tortoise.Namespace).Get(ctx, TortoiseMonitorVPAName(tortoise.Name), metav1.GetOptions{})
 	if err != nil {
-		return nil, false, fmt.Errorf("failed to get updater vpa on tortoise: %w", err)
+		return nil, false, fmt.Errorf("failed to get monitor vpa on tortoise: %w", err)
 	}
 
 	return vpa, isMonitorVPAReady(vpa, tortoise), nil