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 {