From 66ac19550561fca68056d736bf300d53c0aae2e7 Mon Sep 17 00:00:00 2001
From: Karl Isenberg <karlisenberg@google.com>
Date: Mon, 31 Jul 2023 11:04:19 -0700
Subject: [PATCH] WIP: Address feedback

---
 .../controllers/reconciler_base.go            | 27 ++++++++++---------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/pkg/reconcilermanager/controllers/reconciler_base.go b/pkg/reconcilermanager/controllers/reconciler_base.go
index c07953537..fcf718ed0 100644
--- a/pkg/reconcilermanager/controllers/reconciler_base.go
+++ b/pkg/reconcilermanager/controllers/reconciler_base.go
@@ -624,9 +624,12 @@ func (r *reconcilerBase) validateCACertSecret(ctx context.Context, namespace, ca
 
 func (r *reconcilerBase) validateAnnotations(_ context.Context, rs client.Object) error {
 	autoscalingStrategy := reconcilerAutoscalingStrategy(rs)
-	if autoscalingStrategy != metadata.ReconcilerAutoscalingStrategyAuto &&
-		autoscalingStrategy != metadata.ReconcilerAutoscalingStrategyRecommend &&
-		autoscalingStrategy != metadata.ReconcilerAutoscalingStrategyDisabled {
+	switch autoscalingStrategy {
+	case metadata.ReconcilerAutoscalingStrategyAuto,
+		metadata.ReconcilerAutoscalingStrategyRecommend,
+		metadata.ReconcilerAutoscalingStrategyDisabled:
+		// valid
+	default:
 		return errors.Errorf("annotation %q has invalid value %q, must be one of %q, %q, or %q",
 			metadata.ReconcilerAutoscalingStrategyAnnotationKey,
 			autoscalingStrategy,
@@ -739,16 +742,14 @@ func (r *reconcilerBase) upsertVerticalPodAutoscaler(ctx context.Context, strate
 	case metadata.ReconcilerAutoscalingStrategyDisabled:
 		// delete if VPA is installed
 		if !vpaEnabled {
-			r.logger(ctx).Info("Managed object delete skipped - not enabled",
-				logFieldObjectRef, vpaRef.String(),
-				logFieldObjectKind, "VerticalPodAutoscaler")
+			// nothing to delete - CRD not installed
 			return vpaRef, false, nil
 		}
 		return vpaRef, false, r.deleteVerticalPodAutoscaler(ctx, reconcilerRef)
 	case metadata.ReconcilerAutoscalingStrategyAuto, metadata.ReconcilerAutoscalingStrategyRecommend:
 		// upsert if VPA is installed
 		if !vpaEnabled {
-			r.logger(ctx).Info("Managed object upsert skipped - not enabled",
+			r.logger(ctx).Info("Managed object upsert skipped - VerticalPodAutoscaler CRD/APIService not installed",
 				logFieldObjectRef, vpaRef.String(),
 				logFieldObjectKind, "VerticalPodAutoscaler")
 			return vpaRef, false, nil
@@ -757,7 +758,6 @@ func (r *reconcilerBase) upsertVerticalPodAutoscaler(ctx context.Context, strate
 		// shouldn't happen - invalid strategy should be caught by validation
 		return vpaRef, false, errors.Errorf("invalid reconciler autoscaling strategy: %v", strategy)
 	}
-	autoscale := (strategy == metadata.ReconcilerAutoscalingStrategyAuto)
 	vpa := &autoscalingv1.VerticalPodAutoscaler{}
 	vpa.Name = vpaRef.Name
 	vpa.Namespace = vpaRef.Namespace
@@ -769,10 +769,11 @@ func (r *reconcilerBase) upsertVerticalPodAutoscaler(ctx context.Context, strate
 			Name: reconcilerRef.Name,
 		}
 		var updateMode autoscalingv1.UpdateMode
-		if autoscale {
-			updateMode = autoscalingv1.UpdateModeAuto // Evict and Recreate as-needed
-		} else {
-			updateMode = autoscalingv1.UpdateModeOff // Recommend only
+		switch strategy {
+		case metadata.ReconcilerAutoscalingStrategyAuto:
+			updateMode = autoscalingv1.UpdateModeAuto
+		default: // Recommend
+			updateMode = autoscalingv1.UpdateModeOff
 		}
 		vpa.Spec.UpdatePolicy = &autoscalingv1.PodUpdatePolicy{
 			UpdateMode: &updateMode,
@@ -791,7 +792,7 @@ func (r *reconcilerBase) upsertVerticalPodAutoscaler(ctx context.Context, strate
 			logFieldObjectKind, "VerticalPodAutoscaler",
 			logFieldOperation, op)
 	}
-	return vpaRef, autoscale, nil
+	return vpaRef, (strategy == metadata.ReconcilerAutoscalingStrategyAuto), nil
 }
 
 func (r *reconcilerBase) isVPAEnabled() (bool, error) {