From 9c63e7816b63249d9b1f6c809789348a86998cdd Mon Sep 17 00:00:00 2001 From: Ankit Jha Date: Tue, 4 Aug 2026 23:10:39 +0530 Subject: [PATCH] fix: don't double-count webhook capacity reservations in PercentageRunnersBusy suggestReplicasByPercentageRunnersBusy baselines off the live replica count, which already includes capacity added by an active webhook CapacityReservation from a prior reconcile. computeReplicasWithCache then adds the same reservation again on top. Every reconcile while the reservation stays active, its capacity gets added twice and compounds until maxReplicas or the reservation expires. Subtract the reservation from both the baseline and the busy count before computing the busy fraction, so the returned value is metrics-driven scaling only, and the caller's single addition of reserved is the only place it's counted. Also fixes the no-change branch dereferencing st.replicas directly, a nil-panic risk if unset, in favor of the already nil-safe desiredReplicasBefore. Added a test that runs computeReplicasWithCache across repeated reconciles with an active reservation and checks the result stays flat. Verified against the unpatched code first: replicas climb 4, 5, 6, 7 and plateau, matching the runaway scaling in the issue. Fixes #1962 --- .../actions.summerwind.net/autoscaling.go | 22 ++++- .../autoscaling_test.go | 93 +++++++++++++++++++ .../horizontalrunnerautoscaler_controller.go | 19 ++-- 3 files changed, 120 insertions(+), 14 deletions(-) diff --git a/controllers/actions.summerwind.net/autoscaling.go b/controllers/actions.summerwind.net/autoscaling.go index 06ba54fac2..b31c0b1f17 100644 --- a/controllers/actions.summerwind.net/autoscaling.go +++ b/controllers/actions.summerwind.net/autoscaling.go @@ -23,7 +23,7 @@ const ( defaultScaleDownFactor = 0.7 ) -func (r *HorizontalRunnerAutoscalerReconciler) suggestDesiredReplicas(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler) (*int, error) { +func (r *HorizontalRunnerAutoscalerReconciler) suggestDesiredReplicas(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, reserved int) (*int, error) { if hra.Spec.MinReplicas == nil { return nil, fmt.Errorf("horizontalrunnerautoscaler %s/%s is missing minReplicas", hra.Namespace, hra.Name) } else if hra.Spec.MaxReplicas == nil { @@ -52,7 +52,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestDesiredReplicas(ghc *arcgi case v1alpha1.AutoscalingMetricTypeTotalNumberOfQueuedAndInProgressWorkflowRuns: suggested, err = r.suggestReplicasByQueuedAndInProgressWorkflowRuns(ghc, st, hra, &primaryMetric) case v1alpha1.AutoscalingMetricTypePercentageRunnersBusy: - suggested, err = r.suggestReplicasByPercentageRunnersBusy(ghc, st, hra, primaryMetric) + suggested, err = r.suggestReplicasByPercentageRunnersBusy(ghc, st, hra, primaryMetric, reserved) default: return nil, fmt.Errorf("validating autoscaling metrics: unsupported metric type %q", primaryMetric.Type) } @@ -242,7 +242,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByQueuedAndInProgr return &necessaryReplicas, nil } -func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunnersBusy(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, metrics v1alpha1.MetricSpec) (*int, error) { +func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunnersBusy(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, metrics v1alpha1.MetricSpec, reserved int) (*int, error) { ctx := context.Background() scaleUpThreshold := defaultScaleUpThreshold scaleDownThreshold := defaultScaleDownThreshold @@ -328,6 +328,12 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunner desiredReplicasBefore = *v } + // Exclude reserved capacity: the caller re-adds it, so leaving it in double-counts (#1962). + desiredReplicasBefore -= reserved + if desiredReplicasBefore < 1 { + desiredReplicasBefore = 1 + } + var ( numRunners int numRunnersRegistered int @@ -376,8 +382,14 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunner numTerminatingBusy++ } + // Same reasoning as desiredReplicasBefore above. + busy := numRunnersBusy + numTerminatingBusy - reserved + if busy < 0 { + busy = 0 + } + var desiredReplicas int - fractionBusy := float64(numRunnersBusy+numTerminatingBusy) / float64(desiredReplicasBefore) + fractionBusy := float64(busy) / float64(desiredReplicasBefore) if fractionBusy >= scaleUpThreshold { if scaleUpAdjustment > 0 { desiredReplicas = desiredReplicasBefore + scaleUpAdjustment @@ -391,7 +403,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunner desiredReplicas = int(float64(desiredReplicasBefore) * scaleDownFactor) } } else { - desiredReplicas = *st.replicas + desiredReplicas = desiredReplicasBefore } // NOTES for operators: diff --git a/controllers/actions.summerwind.net/autoscaling_test.go b/controllers/actions.summerwind.net/autoscaling_test.go index 4fde432da0..dbaaa2b97c 100644 --- a/controllers/actions.summerwind.net/autoscaling_test.go +++ b/controllers/actions.summerwind.net/autoscaling_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "net/url" "testing" + "time" "github.com/actions/actions-runner-controller/apis/actions.summerwind.net/v1alpha1" "github.com/actions/actions-runner-controller/github" @@ -13,6 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrlclientfake "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" ) @@ -437,6 +439,97 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { } } +// TestDetermineDesiredReplicas_PercentageRunnersBusyWithWebhookReservation reproduces +// https://github.com/actions/actions-runner-controller/issues/1962: combining the +// PercentageRunnersBusy metric with an active webhook CapacityReservation must not +// double-count the reservation and compound the replica count on every reconcile. +func TestDetermineDesiredReplicas_PercentageRunnersBusyWithWebhookReservation(t *testing.T) { + intPtr := func(v int) *int { + return &v + } + + log := zap.New(func(o *zap.Options) { + o.Development = true + }) + + scheme := runtime.NewScheme() + _ = clientgoscheme.AddToScheme(scheme) + _ = v1alpha1.AddToScheme(scheme) + + server := fake.NewServer( + fake.WithListRunnersResponse(200, fake.RunnersListBody), + ) + defer server.Close() + ghc := newGithubClient(server) + + h := &HorizontalRunnerAutoscalerReconciler{ + Log: log, + Scheme: scheme, + Client: ctrlclientfake.NewClientBuilder().WithScheme(scheme).Build(), + DefaultScaleDownDelay: DefaultScaleDownDelay, + } + + rd := v1alpha1.RunnerDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: "testrd"}, + Spec: v1alpha1.RunnerDeploymentSpec{ + Template: v1alpha1.RunnerTemplate{ + Spec: v1alpha1.RunnerSpec{ + RunnerConfig: v1alpha1.RunnerConfig{Repository: "test/valid"}, + }, + }, + Replicas: intPtr(1), + }, + } + + hra := v1alpha1.HorizontalRunnerAutoscaler{ + Spec: v1alpha1.HorizontalRunnerAutoscalerSpec{ + MinReplicas: intPtr(1), + MaxReplicas: intPtr(20), + Metrics: []v1alpha1.MetricSpec{ + {Type: v1alpha1.AutoscalingMetricTypePercentageRunnersBusy}, + }, + // A webhook-triggered reservation for one queued job's worth of capacity, + // still active across every reconcile below. + CapacityReservations: []v1alpha1.CapacityReservation{ + { + Name: "queued-job", + Replicas: 3, + ExpirationTime: metav1.NewTime(time.Now().Add(time.Hour)), + }, + }, + }, + } + + now := time.Now() + + // minReplicas + the reservation, counted exactly once. + want := *hra.Spec.MinReplicas + 3 + + var got int + for i := 0; i < 5; i++ { + minReplicas, _, _, err := h.getMinReplicas(log, now, hra) + if err != nil { + t.Fatalf("iteration %d: unexpected error: %v", i, err) + } + + st := h.scaleTargetFromRD(context.Background(), rd) + + got, err = h.computeReplicasWithCache(ghc, log, now, st, hra, minReplicas) + if err != nil { + t.Fatalf("iteration %d: unexpected error: %v", i, err) + } + + if got != want { + t.Errorf("iteration %d: got %d desired replicas, want %d (minReplicas+reserved, stable) - a growing sequence means the reservation is being double-counted (#1962)", i, got, want) + } + + // Simulate the controller patching the RunnerDeployment to the newly computed + // replica count before the next reconcile, exactly as Reconcile() does. + rd.Spec.Replicas = intPtr(got) + hra.Status.DesiredReplicas = intPtr(got) + } +} + func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { intPtr := func(v int) *int { return &v diff --git a/controllers/actions.summerwind.net/horizontalrunnerautoscaler_controller.go b/controllers/actions.summerwind.net/horizontalrunnerautoscaler_controller.go index de1411ccfa..a47376f67d 100644 --- a/controllers/actions.summerwind.net/horizontalrunnerautoscaler_controller.go +++ b/controllers/actions.summerwind.net/horizontalrunnerautoscaler_controller.go @@ -475,9 +475,18 @@ func (r *HorizontalRunnerAutoscalerReconciler) getMinReplicas(log logr.Logger, n } func (r *HorizontalRunnerAutoscalerReconciler) computeReplicasWithCache(ghc *arcgithub.Client, log logr.Logger, now time.Time, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, minReplicas int) (int, error) { + var reserved int + + for _, reservation := range hra.Spec.CapacityReservations { + if reservation.ExpirationTime.After(now) { + reserved += reservation.Replicas + } + } + + // reserved is passed in so suggesters can exclude it from their own baseline (#1962). var suggestedReplicas int - v, err := r.suggestDesiredReplicas(ghc, st, hra) + v, err := r.suggestDesiredReplicas(ghc, st, hra, reserved) if err != nil { return 0, err } @@ -488,14 +497,6 @@ func (r *HorizontalRunnerAutoscalerReconciler) computeReplicasWithCache(ghc *arc suggestedReplicas = *v } - var reserved int - - for _, reservation := range hra.Spec.CapacityReservations { - if reservation.ExpirationTime.After(now) { - reserved += reservation.Replicas - } - } - newDesiredReplicas := suggestedReplicas + reserved if newDesiredReplicas < minReplicas {