From e9eef049933c6154677f9070786b7ea0bb66f361 Mon Sep 17 00:00:00 2001
From: Yusuke Kuoka <ykuoka@gmail.com>
Date: Thu, 25 Feb 2021 11:08:00 +0900
Subject: [PATCH] Fix old HRA capacity reservations not cleaned up (#354)

Similar to #348 for #346, but for HRA.Spec.CapacityReservations usually modified by the webhook-based autoscaler controller.
This patch tries to fix that by improving the webhook-based autoscaler controller to omit expired reservations on updating HRA spec.
---
 .../horizontal_runner_autoscaler_webhook.go   | 18 ++++++++-
 ...rizontal_runner_autoscaler_webhook_test.go | 39 +++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/controllers/horizontal_runner_autoscaler_webhook.go b/controllers/horizontal_runner_autoscaler_webhook.go
index eb883765..e6e6d92e 100644
--- a/controllers/horizontal_runner_autoscaler_webhook.go
+++ b/controllers/horizontal_runner_autoscaler_webhook.go
@@ -381,7 +381,9 @@ func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) tryScaleUp(ctx contex
 		amount = target.ScaleUpTrigger.Amount
 	}
 
-	copy.Spec.CapacityReservations = append(copy.Spec.CapacityReservations, v1alpha1.CapacityReservation{
+	capacityReservations := getValidCapacityReservations(copy)
+
+	copy.Spec.CapacityReservations = append(capacityReservations, v1alpha1.CapacityReservation{
 		ExpirationTime: metav1.Time{Time: time.Now().Add(target.ScaleUpTrigger.Duration.Duration)},
 		Replicas:       amount,
 	})
@@ -395,6 +397,20 @@ func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) tryScaleUp(ctx contex
 	return nil
 }
 
+func getValidCapacityReservations(autoscaler *v1alpha1.HorizontalRunnerAutoscaler) []v1alpha1.CapacityReservation {
+	var capacityReservations []v1alpha1.CapacityReservation
+
+	now := time.Now()
+
+	for _, reservation := range autoscaler.Spec.CapacityReservations {
+		if reservation.ExpirationTime.Time.After(now) {
+			capacityReservations = append(capacityReservations, reservation)
+		}
+	}
+
+	return capacityReservations
+}
+
 func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) SetupWithManager(mgr ctrl.Manager) error {
 	name := "webhookbasedautoscaler"
 	if autoscaler.Name != "" {
diff --git a/controllers/horizontal_runner_autoscaler_webhook_test.go b/controllers/horizontal_runner_autoscaler_webhook_test.go
index d57ff50c..f57fadb5 100644
--- a/controllers/horizontal_runner_autoscaler_webhook_test.go
+++ b/controllers/horizontal_runner_autoscaler_webhook_test.go
@@ -9,6 +9,7 @@ import (
 	actionsv1alpha1 "github.com/summerwind/actions-runner-controller/api/v1alpha1"
 	"io"
 	"io/ioutil"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	"net/http"
@@ -17,6 +18,7 @@ import (
 	"os"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"testing"
+	"time"
 )
 
 var (
@@ -111,6 +113,43 @@ func TestWebhookPing(t *testing.T) {
 	)
 }
 
+func TestGetValidCapacityReservations(t *testing.T) {
+	now := time.Now()
+
+	hra := &actionsv1alpha1.HorizontalRunnerAutoscaler{
+		Spec: actionsv1alpha1.HorizontalRunnerAutoscalerSpec{
+			CapacityReservations: []actionsv1alpha1.CapacityReservation{
+				{
+					ExpirationTime: metav1.Time{Time: now.Add(-time.Second)},
+					Replicas:       1,
+				},
+				{
+					ExpirationTime: metav1.Time{Time: now},
+					Replicas:       2,
+				},
+				{
+					ExpirationTime: metav1.Time{Time: now.Add(time.Second)},
+					Replicas:       3,
+				},
+			},
+		},
+	}
+
+	revs := getValidCapacityReservations(hra)
+
+	var count int
+
+	for _, r := range revs {
+		count += r.Replicas
+	}
+
+	want := 3
+
+	if count != want {
+		t.Errorf("want %d, got %d", want, count)
+	}
+}
+
 func installTestLogger(webhook *HorizontalRunnerAutoscalerGitHubWebhook) *bytes.Buffer {
 	logs := &bytes.Buffer{}