update HPA correctly when changing the spec (#372)

api/autoscaling/v2/webhook_suite_test.go

@@ -164,7 +164,7 @@ var _ = BeforeSuite(func() {
 	eventRecorder := mgr.GetEventRecorderFor("tortoise-controller")
 	tortoiseService, err := tortoise.New(mgr.GetClient(), eventRecorder, config.RangeOfMinMaxReplicasRecommendationHours, config.TimeZone, config.TortoiseUpdateInterval, config.GatheringDataPeriodType)
 	Expect(err).NotTo(HaveOccurred())
-	hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, 100, time.Hour, 1000, 10000, "")
+	hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, 100, time.Hour, 1000, 10000, 3, "")
 	Expect(err).NotTo(HaveOccurred())
 
 	hpaWebhook := New(tortoiseService, hpaService)

controllers/tortoise_controller_test.go

@@ -245,7 +245,7 @@ func startController(ctx context.Context) func() {
 	Expect(err).ShouldNot(HaveOccurred())
 	cli, err := vpa.New(mgr.GetConfig(), recorder)
 	Expect(err).ShouldNot(HaveOccurred())
-	hpaS, err := hpa.New(mgr.GetClient(), recorder, 0.95, 90, 25, time.Hour, 1000, 10000, ".*-exclude-metric")
+	hpaS, err := hpa.New(mgr.GetClient(), recorder, 0.95, 90, 25, time.Hour, 1000, 10000, 3, ".*-exclude-metric")
 	Expect(err).ShouldNot(HaveOccurred())
 	reconciler := &TortoiseReconciler{
 		Scheme:             scheme,

main.go

@@ -155,7 +155,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, config.HPATargetUtilizationMaxIncrease, config.HPATargetUtilizationUpdateInterval, config.MaximumMinReplicas, config.MaximumMaxReplicas, config.HPAExternalMetricExclusionRegex)
+	hpaService, err := hpa.New(mgr.GetClient(), eventRecorder, config.ReplicaReductionFactor, config.MaximumTargetResourceUtilization, config.HPATargetUtilizationMaxIncrease, config.HPATargetUtilizationUpdateInterval, config.MaximumMinReplicas, config.MaximumMaxReplicas, int32(config.MinimumMinReplicas), config.HPAExternalMetricExclusionRegex)
 	if err != nil {
 		setupLog.Error(err, "unable to start hpa service")
 		os.Exit(1)

pkg/hpa/service.go

@@ -37,6 +37,7 @@ type Service struct {
 	tortoiseHPATargetUtilizationMaxIncrease    int
 	recorder                                   record.EventRecorder
 	tortoiseHPATargetUtilizationUpdateInterval time.Duration
+	minimumMinReplicas                         int32
 	maximumMinReplica                          int32
 	maximumMaxReplica                          int32
 	externalMetricExclusionRegex               *regexp.Regexp
@@ -50,6 +51,7 @@ func New(
 	tortoiseHPATargetUtilizationMaxIncrease int,
 	tortoiseHPATargetUtilizationUpdateInterval time.Duration,
 	maximumMinReplica, maximumMaxReplica int32,
+	minimumMinReplicas int32,
 	externalMetricExclusionRegex string,
 ) (*Service, error) {
 	var regex *regexp.Regexp
@@ -70,6 +72,7 @@ func New(
 		recorder:                                recorder,
 		tortoiseHPATargetUtilizationUpdateInterval: tortoiseHPATargetUtilizationUpdateInterval,
 		maximumMinReplica:                          maximumMinReplica,
+		minimumMinReplicas:                         minimumMinReplicas,
 		maximumMaxReplica:                          maximumMaxReplica,
 		externalMetricExclusionRegex:               regex,
 	}, nil
@@ -256,8 +259,8 @@ func (c *Service) CreateHPA(ctx context.Context, tortoise *autoscalingv1beta3.To
 				Name:       tortoise.Spec.TargetRefs.ScaleTargetRef.Name,
 				APIVersion: tortoise.Spec.TargetRefs.ScaleTargetRef.APIVersion,
 			},
-			MinReplicas: ptr.To[int32](int32(math.Ceil(float64(replicaNum) / 2.0))),
-			MaxReplicas: replicaNum * 2,
+			MinReplicas: ptr.To[int32](c.minimumMinReplicas),
+			MaxReplicas: c.maximumMaxReplica,
 			Behavior:    globalRecommendedHPABehavior,
 		},
 	}
@@ -513,6 +516,7 @@ func (c *Service) UpdateHPASpecFromTortoiseAutoscalingPolicy(
 			c.recorder.Event(tortoise, corev1.EventTypeNormal, event.HPACreated, fmt.Sprintf("Initialized a HPA %s/%s because tortoise has resource to scale horizontally", tortoise.Namespace, tortoise.Status.Targets.HorizontalPodAutoscaler))
 			return tortoise, nil
 		}
+
 		return tortoise, fmt.Errorf("failed to get hpa on tortoise: %w", err)
 	}
 
@@ -530,9 +534,10 @@ func (c *Service) UpdateHPASpecFromTortoiseAutoscalingPolicy(
 			return fmt.Errorf("failed to get hpa on tortoise: %w", err)
 		}
 
+		// update only metrics
 		hpa.Spec.Metrics = newhpa.Spec.Metrics
 
-		return c.c.Update(ctx, newhpa)
+		return c.c.Update(ctx, hpa)
 	}
 
 	if err := retry.RetryOnConflict(retry.DefaultRetry, updateFn); err != nil {

pkg/hpa/service_test.go

@@ -2290,7 +2290,7 @@ func TestClient_UpdateHPAFromTortoiseRecommendation(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			c, err := New(fake.NewClientBuilder().WithRuntimeObjects(tt.initialHPA).Build(), record.NewFakeRecorder(10), 0.95, 90, 50, time.Hour, 1000, 10001, tt.excludeMetricRegex)
+			c, err := New(fake.NewClientBuilder().WithRuntimeObjects(tt.initialHPA).Build(), record.NewFakeRecorder(10), 0.95, 90, 50, time.Hour, 1000, 10001, 3, tt.excludeMetricRegex)
 			if err != nil {
 				t.Fatalf("New() error = %v", err)
 			}
@@ -2371,8 +2371,8 @@ func TestService_InitializeHPA(t *testing.T) {
 					Namespace: "default",
 				},
 				Spec: v2.HorizontalPodAutoscalerSpec{
-					MinReplicas: ptrInt32(2),
-					MaxReplicas: 8,
+					MinReplicas: ptrInt32(3),
+					MaxReplicas: 1000,
 					Metrics: []v2.MetricSpec{
 						{
 							Type: v2.ContainerResourceMetricSourceType,
@@ -2513,12 +2513,12 @@ func TestService_InitializeHPA(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			c, err := New(fake.NewClientBuilder().Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 100, 1000, "")
+			c, err := New(fake.NewClientBuilder().Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 100, 1000, 3, "")
 			if err != nil {
 				t.Fatalf("New() error = %v", err)
 			}
 			if tt.initialHPA != nil {
-				c, err = New(fake.NewClientBuilder().WithRuntimeObjects(tt.initialHPA).Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 100, 1000, "")
+				c, err = New(fake.NewClientBuilder().WithRuntimeObjects(tt.initialHPA).Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 100, 1000, 3, "")
 				if err != nil {
 					t.Fatalf("New() error = %v", err)
 				}
@@ -4071,12 +4071,12 @@ func TestService_UpdateHPASpecFromTortoiseAutoscalingPolicy(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			c, err := New(fake.NewClientBuilder().Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 1000, 10000, "")
+			c, err := New(fake.NewClientBuilder().Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 1000, 10000, 3, "")
 			if err != nil {
 				t.Fatalf("New() error = %v", err)
 			}
 			if tt.initialHPA != nil {
-				c, err = New(fake.NewClientBuilder().WithRuntimeObjects(tt.initialHPA).Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 1000, 10000, "")
+				c, err = New(fake.NewClientBuilder().WithRuntimeObjects(tt.initialHPA).Build(), record.NewFakeRecorder(10), 0.95, 90, 100, time.Hour, 1000, 10000, 3, "")
 				if err != nil {
 					t.Fatalf("New() error = %v", err)
 				}

pkg/recommender/recommender.go

@@ -47,8 +47,8 @@ type Service struct {
 }
 
 func New(
-	MaxReplicasRecommendationMultiplier float64,
-	MinReplicasRecommendationMultiplier float64,
+	maxReplicasRecommendationMultiplier float64,
+	minReplicasRecommendationMultiplier float64,
 	maximumTargetResourceUtilization int,
 	minimumTargetResourceUtilization int,
 	minimumMinReplicas int,
@@ -83,8 +83,8 @@ func New(
 
 	return &Service{
 		eventRecorder:                       eventRecorder,
-		MaxReplicasRecommendationMultiplier: MaxReplicasRecommendationMultiplier,
-		MinReplicasRecommendationMultiplier: MinReplicasRecommendationMultiplier,
+		MaxReplicasRecommendationMultiplier: maxReplicasRecommendationMultiplier,
+		MinReplicasRecommendationMultiplier: minReplicasRecommendationMultiplier,
 		maximumTargetResourceUtilization:    int32(maximumTargetResourceUtilization),
 		minimumTargetResourceUtilization:    int32(minimumTargetResourceUtilization),
 		minimumMinReplicas:                  int32(minimumMinReplicas),