Delete stale primary HPAs when autoscalerRef is removed

docs/gitbook/usage/how-it-works.md

@@ -78,6 +78,9 @@ Based on the above configuration, Flagger generates the following Kubernetes obj
 * `deployment/<targetRef.name>-primary`
 * `hpa/<autoscalerRef.name>-primary`
 
+When `autoscalerRef` is removed, Flagger deletes the generated primary HPA if
+it is controlled by the Canary.
+
 The primary deployment is considered the stable release of your app,
 by default all traffic is routed to this version and the target deployment is scaled to zero.
 Flagger will detect changes to the target deployment (including secrets and configmaps)

pkg/canary/hpa_reconciler.go

@@ -3,6 +3,7 @@ package canary
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1"
 	clientset "github.com/fluxcd/flagger/pkg/client/clientset/versioned"
@@ -25,14 +26,50 @@ type HPAReconciler struct {
 }
 
 func (hr *HPAReconciler) ReconcilePrimaryScaler(cd *flaggerv1.Canary, init bool) error {
-	if cd.Spec.AutoscalerRef != nil {
-		if err := hr.reconcilePrimaryHpa(cd, init); err != nil {
-			return err
+	if cd.Spec.AutoscalerRef == nil {
+		return hr.cleanupPrimaryHpas(cd)
+	}
+	return hr.reconcilePrimaryHpa(cd, init)
+}
+
+func (hr *HPAReconciler) cleanupPrimaryHpas(cd *flaggerv1.Canary) error {
+	if cd.UID == "" {
+		return nil
+	}
+
+	hpas, err := hr.kubeClient.AutoscalingV2().HorizontalPodAutoscalers(cd.Namespace).List(context.TODO(), metav1.ListOptions{})
+	if err != nil {
+		return fmt.Errorf("HorizontalPodAutoscaler list query error in namespace %s: %w", cd.Namespace, err)
+	}
+
+	for i := range hpas.Items {
+		hpa := &hpas.Items[i]
+		if !isFlaggerManagedPrimaryHpa(hpa, cd) {
+			continue
 		}
+
+		err := hr.kubeClient.AutoscalingV2().HorizontalPodAutoscalers(cd.Namespace).Delete(context.TODO(), hpa.Name, metav1.DeleteOptions{})
+		if errors.IsNotFound(err) {
+			continue
+		}
+		if err != nil {
+			return fmt.Errorf("deleting HorizontalPodAutoscaler v2 %s.%s failed: %w", hpa.Name, hpa.Namespace, err)
+		}
+
+		hr.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof(
+			"HorizontalPodAutoscaler v2 %s.%s deleted", hpa.Name, cd.Namespace)
 	}
+
 	return nil
 }
 
+func isFlaggerManagedPrimaryHpa(hpa *hpav2.HorizontalPodAutoscaler, cd *flaggerv1.Canary) bool {
+	return cd.UID != "" &&
+		strings.HasSuffix(hpa.Name, "-primary") &&
+		hpa.Spec.ScaleTargetRef.Name == fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) &&
+		metav1.IsControlledBy(hpa, cd)
+}
+
 func (hr *HPAReconciler) reconcilePrimaryHpa(cd *flaggerv1.Canary, init bool) error {
 	hpa, err := hr.kubeClient.AutoscalingV2().HorizontalPodAutoscalers(cd.Namespace).Get(context.TODO(), cd.Spec.AutoscalerRef.Name, metav1.GetOptions{})
 	if err != nil {

pkg/canary/hpa_reconciler_test.go

@@ -8,7 +8,9 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	hpav2 "k8s.io/api/autoscaling/v2"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 )
 
 func Test_reconcilePrimaryHpa(t *testing.T) {
@@ -33,6 +35,94 @@ func Test_reconcilePrimaryHpa(t *testing.T) {
 	require.Error(t, err)
 }
 
+func TestHPAReconciler_ReconcilePrimaryScaler_RemovesManagedHPAWhenAutoscalerRefIsRemoved(t *testing.T) {
+	mocks := newScalerReconcilerFixture(scalerConfig{
+		targetName: "podinfo",
+		scaler:     "HorizontalPodAutoscaler",
+	})
+	mocks.canary.UID = "podinfo-uid"
+	hpaReconciler := mocks.scalerReconciler.(*HPAReconciler)
+	autoscalerRef := *mocks.canary.Spec.AutoscalerRef
+
+	require.NoError(t, hpaReconciler.ReconcilePrimaryScaler(mocks.canary, true))
+
+	primaryHPA, err := mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+	require.True(t, metav1.IsControlledBy(primaryHPA, mocks.canary))
+
+	require.NoError(t, mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Delete(context.TODO(), "podinfo", metav1.DeleteOptions{}))
+	mocks.canary.Spec.AutoscalerRef = nil
+	require.NoError(t, hpaReconciler.ReconcilePrimaryScaler(mocks.canary, true))
+
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.True(t, apierrors.IsNotFound(err))
+
+	// Cleanup is idempotent and the primary HPA stays deleted.
+	require.NoError(t, hpaReconciler.ReconcilePrimaryScaler(mocks.canary, true))
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.True(t, apierrors.IsNotFound(err))
+
+	// Restoring the source HPA and autoscalerRef recreates the managed primary HPA.
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Create(context.TODO(), newScalerReconcilerTestHPAV2(), metav1.CreateOptions{})
+	require.NoError(t, err)
+	mocks.canary.Spec.AutoscalerRef = &autoscalerRef
+	require.NoError(t, hpaReconciler.ReconcilePrimaryScaler(mocks.canary, true))
+
+	primaryHPA, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+	require.True(t, metav1.IsControlledBy(primaryHPA, mocks.canary))
+}
+
+func TestHPAReconciler_ReconcilePrimaryScaler_PreservesUnmanagedHPA(t *testing.T) {
+	mocks := newScalerReconcilerFixture(scalerConfig{
+		targetName: "podinfo",
+		scaler:     "HorizontalPodAutoscaler",
+	})
+	mocks.canary.UID = "podinfo-uid"
+	mocks.canary.Spec.AutoscalerRef = nil
+	hpaReconciler := mocks.scalerReconciler.(*HPAReconciler)
+
+	unmanagedHPA := newScalerReconcilerTestHPAV2()
+	unmanagedHPA.Name = "podinfo-primary"
+	unmanagedHPA.Spec.ScaleTargetRef.Name = "podinfo-primary"
+	_, err := mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Create(context.TODO(), unmanagedHPA, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	require.NoError(t, hpaReconciler.ReconcilePrimaryScaler(mocks.canary, true))
+
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+}
+
+func TestHPAReconciler_ReconcilePrimaryScaler_PreservesHPAOwnedByAnotherCanary(t *testing.T) {
+	mocks := newScalerReconcilerFixture(scalerConfig{
+		targetName: "podinfo",
+		scaler:     "HorizontalPodAutoscaler",
+	})
+	mocks.canary.UID = "podinfo-uid"
+	mocks.canary.Spec.AutoscalerRef = nil
+	hpaReconciler := mocks.scalerReconciler.(*HPAReconciler)
+
+	controller := true
+	foreignHPA := newScalerReconcilerTestHPAV2()
+	foreignHPA.Name = "podinfo-primary"
+	foreignHPA.Spec.ScaleTargetRef.Name = "podinfo-primary"
+	foreignHPA.OwnerReferences = []metav1.OwnerReference{{
+		APIVersion: flaggerv1.SchemeGroupVersion.String(),
+		Kind:       flaggerv1.CanaryKind,
+		Name:       "other-canary",
+		UID:        types.UID("other-canary-uid"),
+		Controller: &controller,
+	}}
+	_, err := mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Create(context.TODO(), foreignHPA, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	require.NoError(t, hpaReconciler.ReconcilePrimaryScaler(mocks.canary, true))
+
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+}
+
 func Test_reconcilePrimaryHpaV2(t *testing.T) {
 	mocks := newScalerReconcilerFixture(scalerConfig{
 		targetName: "podinfo",

pkg/controller/controller.go

@@ -50,6 +50,10 @@ import (
 
 const controllerAgentName = "flagger"
 
+func hpaCleanupSweepKey(canary *flaggerv1.Canary) string {
+	return fmt.Sprintf("%s/%s/%s", canary.Namespace, canary.Name, canary.UID)
+}
+
 // Controller is managing the canary objects and schedules canary deployments
 type Controller struct {
 	kubeConfig           *rest.Config
@@ -63,6 +67,7 @@ type Controller struct {
 	eventRecorder        record.EventRecorder
 	logger               *zap.SugaredLogger
 	canaries             *sync.Map
+	hpaCleanupSweeps     sync.Map
 	jobs                 map[string]CanaryJob
 	recorder             metrics.Recorder
 	notifier             notifier.Interface
@@ -180,6 +185,7 @@ func NewController(
 			if ok {
 				ctrl.logger.Infof("Deleting %s.%s from cache", r.Name, r.Namespace)
 				ctrl.canaries.Delete(fmt.Sprintf("%s.%s", r.Name, r.Namespace))
+				ctrl.hpaCleanupSweeps.Delete(hpaCleanupSweepKey(&r))
 			}
 		},
 	})

pkg/controller/scheduler.go

@@ -189,8 +189,21 @@ func (c *Controller) advanceCanary(name string, namespace string) {
 	}
 
 	var scalerReconciler canary.ScalerReconciler
+	cleanupSweepKey := hpaCleanupSweepKey(cd)
 	if cd.Spec.AutoscalerRef != nil {
+		c.hpaCleanupSweeps.Delete(cleanupSweepKey)
 		scalerReconciler = c.canaryFactory.ScalerReconciler(cd.Spec.AutoscalerRef.Kind)
+	} else if _, cleanupComplete := c.hpaCleanupSweeps.Load(cleanupSweepKey); !cleanupComplete {
+		// Garbage collect a primary autoscaler left behind by a removed
+		// autoscalerRef. Remember successful sweeps to avoid an HPA list on
+		// every analysis tick, but retry failures without blocking the rollout.
+		if hpaReconciler := c.canaryFactory.ScalerReconciler("HorizontalPodAutoscaler"); hpaReconciler != nil {
+			if err := hpaReconciler.ReconcilePrimaryScaler(cd, true); err != nil {
+				c.recordEventWarningf(cd, "%v", err)
+			} else {
+				c.hpaCleanupSweeps.Store(cleanupSweepKey, struct{}{})
+			}
+		}
 	}
 
 	// init Kubernetes router

pkg/controller/scheduler_deployment_test.go

@@ -27,8 +27,13 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	hpav2 "k8s.io/api/autoscaling/v2"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
 
 	flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1"
 	"github.com/fluxcd/flagger/pkg/notifier"
@@ -42,6 +47,153 @@ func TestScheduler_DeploymentInit(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func TestScheduler_DeploymentCleansStaleHPAOnStartup(t *testing.T) {
+	canary := newDeploymentTestCanary()
+	canary.UID = "podinfo-uid"
+	canary.Spec.AutoscalerRef = nil
+	mocks := newDeploymentFixture(canary)
+
+	staleHPA := newDeploymentTestHPA()
+	staleHPA.Name = "podinfo-primary"
+	staleHPA.Spec.ScaleTargetRef.Name = "podinfo-primary"
+	staleHPA.OwnerReferences = []metav1.OwnerReference{
+		*metav1.NewControllerRef(canary, flaggerv1.SchemeGroupVersion.WithKind(flaggerv1.CanaryKind)),
+	}
+	_, err := mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Create(context.TODO(), staleHPA, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	mocks.ctrl.advanceCanary("podinfo", "default")
+
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.True(t, apierrors.IsNotFound(err))
+}
+
+func TestScheduler_DeploymentRemovesPrimaryHPAWhenAutoscalerRefIsRemoved(t *testing.T) {
+	canary := newDeploymentTestCanary()
+	canary.UID = "podinfo-uid"
+	canary.Spec.AutoscalerRef.APIVersion = hpav2.SchemeGroupVersion.String()
+	autoscalerRef := *canary.Spec.AutoscalerRef
+	mocks := newDeploymentFixture(canary)
+
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	mocks.makePrimaryReady(t)
+	mocks.ctrl.advanceCanary("podinfo", "default")
+
+	primaryHPA, err := mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+	require.True(t, metav1.IsControlledBy(primaryHPA, canary))
+
+	updatedCanary, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{})
+	require.NoError(t, err)
+	updatedCanary.Spec.AutoscalerRef = nil
+	_, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Update(context.TODO(), updatedCanary, metav1.UpdateOptions{})
+	require.NoError(t, err)
+	require.NoError(t, mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Delete(context.TODO(), "podinfo", metav1.DeleteOptions{}))
+
+	listCalls := 0
+	fakeClient := mocks.kubeClient.(*fake.Clientset)
+	fakeClient.PrependReactor("list", "horizontalpodautoscalers", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		listCalls++
+		return false, nil, nil
+	})
+
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.True(t, apierrors.IsNotFound(err))
+
+	// The primary workload stays healthy and cleanup remains idempotent.
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.True(t, apierrors.IsNotFound(err))
+	primary, err := mocks.kubeClient.AppsV1().Deployments("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, int32(1), primary.Status.ReadyReplicas)
+	require.Equal(t, 1, listCalls, "successful cleanup should only list HPAs once")
+
+	// Re-enabling autoscaling clears the successful cleanup marker and recreates
+	// the managed primary HPA.
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Create(context.TODO(), newDeploymentTestHPA(), metav1.CreateOptions{})
+	require.NoError(t, err)
+	updatedCanary, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{})
+	require.NoError(t, err)
+	updatedCanary.Spec.AutoscalerRef = &autoscalerRef
+	_, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Update(context.TODO(), updatedCanary, metav1.UpdateOptions{})
+	require.NoError(t, err)
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+
+	// Disabling autoscaling again performs another cleanup sweep.
+	updatedCanary, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{})
+	require.NoError(t, err)
+	updatedCanary.Spec.AutoscalerRef = nil
+	_, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Update(context.TODO(), updatedCanary, metav1.UpdateOptions{})
+	require.NoError(t, err)
+	require.NoError(t, mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Delete(context.TODO(), "podinfo", metav1.DeleteOptions{}))
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.True(t, apierrors.IsNotFound(err))
+	require.Equal(t, 2, listCalls, "removing autoscalerRef again should trigger a new cleanup sweep")
+}
+
+func TestScheduler_DeploymentHPAListFailureDoesNotBlockInitialization(t *testing.T) {
+	canary := newDeploymentTestCanary()
+	canary.UID = "podinfo-uid"
+	canary.Spec.AutoscalerRef = nil
+	mocks := newDeploymentFixture(canary)
+
+	listCalls := 0
+	fakeClient := mocks.kubeClient.(*fake.Clientset)
+	fakeClient.PrependReactor("list", "horizontalpodautoscalers", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		listCalls++
+		return true, nil, apierrors.NewServiceUnavailable("autoscaling/v2 is unavailable")
+	})
+
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	mocks.makePrimaryReady(t)
+	mocks.ctrl.advanceCanary("podinfo", "default")
+
+	require.Equal(t, 2, listCalls, "failed cleanup should be retried on the next reconciliation")
+	require.NoError(t, assertPhase(mocks.flaggerClient, "podinfo", flaggerv1.CanaryPhaseInitialized))
+}
+
+func TestScheduler_DeploymentHPADeleteFailureIsRetriedAndDoesNotBlockReconciliation(t *testing.T) {
+	canary := newDeploymentTestCanary()
+	canary.UID = "podinfo-uid"
+	canary.Spec.AutoscalerRef.APIVersion = hpav2.SchemeGroupVersion.String()
+	mocks := newDeploymentFixture(canary)
+
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	mocks.makePrimaryReady(t)
+	mocks.ctrl.advanceCanary("podinfo", "default")
+
+	updatedCanary, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{})
+	require.NoError(t, err)
+	updatedCanary.Spec.AutoscalerRef = nil
+	_, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Update(context.TODO(), updatedCanary, metav1.UpdateOptions{})
+	require.NoError(t, err)
+	require.NoError(t, mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Delete(context.TODO(), "podinfo", metav1.DeleteOptions{}))
+
+	deleteCalls := 0
+	fakeClient := mocks.kubeClient.(*fake.Clientset)
+	fakeClient.PrependReactor("delete", "horizontalpodautoscalers", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		deleteAction := action.(k8stesting.DeleteAction)
+		if deleteAction.GetName() != "podinfo-primary" {
+			return false, nil, nil
+		}
+		deleteCalls++
+		return true, nil, apierrors.NewServiceUnavailable("autoscaling/v2 delete is unavailable")
+	})
+
+	mocks.ctrl.advanceCanary("podinfo", "default")
+	mocks.ctrl.advanceCanary("podinfo", "default")
+
+	require.Equal(t, 2, deleteCalls, "failed deletion should be retried on the next reconciliation")
+	_, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{})
+	require.NoError(t, err)
+	require.NoError(t, assertPhase(mocks.flaggerClient, "podinfo", flaggerv1.CanaryPhaseInitialized))
+}
+
 func TestScheduler_DeploymentNewRevision(t *testing.T) {
 	mocks := newDeploymentFixture(nil)