From 598dd1d9fe140f00ab030a6e7447a35264f67d73 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Thu, 25 Feb 2021 10:32:09 +0900 Subject: [PATCH] Fix incorrect DESIRED on `kubectl get hra (#353) `kubectl get horizontalrunnerautoscalers.actions.summerwind.dev` shows HRA.status.desiredReplicas as the DESIRED count. However the value had been not taking capacityReservations into account, which resulted in showing incorrect count when you used webhook-based autoscaler, or capacityReservations API directly. This fixes that. --- .../horizontalrunnerautoscaler_controller.go | 8 ++++---- controllers/integration_test.go | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/controllers/horizontalrunnerautoscaler_controller.go b/controllers/horizontalrunnerautoscaler_controller.go index c62f2e87..e007cbe6 100644 --- a/controllers/horizontalrunnerautoscaler_controller.go +++ b/controllers/horizontalrunnerautoscaler_controller.go @@ -132,16 +132,16 @@ func (r *HorizontalRunnerAutoscalerReconciler) Reconcile(req ctrl.Request) (ctrl var updated *v1alpha1.HorizontalRunnerAutoscaler - if hra.Status.DesiredReplicas == nil || *hra.Status.DesiredReplicas != *replicas { + if hra.Status.DesiredReplicas == nil || *hra.Status.DesiredReplicas != newDesiredReplicas { updated = hra.DeepCopy() - if (hra.Status.DesiredReplicas == nil && *replicas > 1) || - (hra.Status.DesiredReplicas != nil && *replicas > *hra.Status.DesiredReplicas) { + if (hra.Status.DesiredReplicas == nil && newDesiredReplicas > 1) || + (hra.Status.DesiredReplicas != nil && newDesiredReplicas > *hra.Status.DesiredReplicas) { updated.Status.LastSuccessfulScaleOutTime = &metav1.Time{Time: time.Now()} } - updated.Status.DesiredReplicas = replicas + updated.Status.DesiredReplicas = &newDesiredReplicas } if replicasFromCache == nil { diff --git a/controllers/integration_test.go b/controllers/integration_test.go index 55db2edf..1f85744e 100644 --- a/controllers/integration_test.go +++ b/controllers/integration_test.go @@ -484,6 +484,7 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { Expect(err).NotTo(HaveOccurred(), "failed to get test HorizontalRunnerAutoscaler resource") ExpectRunnerSetsManagedReplicasCountEventuallyEquals(ctx, ns.Name, 1, "runners after HRA force update for scale-down") + ExpectHRADesiredReplicasEquals(ctx, ns.Name, name, 1, "runner deployment desired replicas") } // Scale-up to 2 replicas on first pull_request create webhook event @@ -491,12 +492,14 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { env.SendUserPullRequestEvent("test", "valid", "main", "created") ExpectRunnerSetsCountEventuallyEquals(ctx, ns.Name, 1, "runner sets after webhook") ExpectRunnerSetsManagedReplicasCountEventuallyEquals(ctx, ns.Name, 2, "runners after first webhook event") + ExpectHRADesiredReplicasEquals(ctx, ns.Name, name, 2, "runner deployment desired replicas") } // Scale-up to 3 replicas on second pull_request create webhook event { env.SendUserPullRequestEvent("test", "valid", "main", "created") ExpectRunnerSetsManagedReplicasCountEventuallyEquals(ctx, ns.Name, 3, "runners after second webhook event") + ExpectHRADesiredReplicasEquals(ctx, ns.Name, name, 3, "runner deployment desired replicas") } }) @@ -598,6 +601,18 @@ var _ = Context("INTEGRATION: Inside of a new namespace", func() { }) }) +func ExpectHRADesiredReplicasEquals(ctx context.Context, ns, name string, desired int, optionalDescriptions ...interface{}) { + var rd actionsv1alpha1.HorizontalRunnerAutoscaler + + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, &rd) + + ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to get test HRA resource") + + replicas := rd.Status.DesiredReplicas + + ExpectWithOffset(1, *replicas).To(Equal(desired), optionalDescriptions...) +} + func (env *testEnvironment) ExpectRegisteredNumberCountEventuallyEquals(want int, optionalDescriptions ...interface{}) { EventuallyWithOffset( 1,