From e3f5c1141d4474c58ba97b80a522ebfce29b4d07 Mon Sep 17 00:00:00 2001
From: Sascha Schwarze <schwarzs@de.ibm.com>
Date: Mon, 5 Feb 2024 11:37:20 +0100
Subject: [PATCH] Prevent a PodAutoscaler's DesiredScale to turn to -1

---
 pkg/reconciler/autoscaling/kpa/kpa.go      | 10 ++-
 pkg/reconciler/autoscaling/kpa/kpa_test.go | 95 ++++++++++++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go
index 6c185949d534..7163d6da8ecd 100644
--- a/pkg/reconciler/autoscaling/kpa/kpa.go
+++ b/pkg/reconciler/autoscaling/kpa/kpa.go
@@ -219,7 +219,15 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *autoscalingv1alph
 }
 
 func computeStatus(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler, pc podCounts, logger *zap.SugaredLogger) {
-	pa.Status.DesiredScale, pa.Status.ActualScale = ptr.Int32(int32(pc.want)), ptr.Int32(int32(pc.ready))
+	pa.Status.ActualScale = ptr.Int32(int32(pc.ready))
+
+	// When the autoscaler just restarted, it does not yet have metrics and would change the desiredScale to -1 and moments
+	// later back to the correct value. The following condition omits this.
+	if pc.want > -1 || pa.Status.DesiredScale == nil || *pa.Status.DesiredScale < 0 {
+		pa.Status.DesiredScale = ptr.Int32(int32(pc.want))
+	} else {
+		logger.Debugf("Ignoring change of desiredScale from %d to %d", *pa.Status.DesiredScale, pc.want)
+	}
 
 	reportMetrics(pa, pc)
 	computeActiveCondition(ctx, pa, pc)
diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go
index c45d96449c6c..f13197aa11db 100644
--- a/pkg/reconciler/autoscaling/kpa/kpa_test.go
+++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go
@@ -55,6 +55,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	clientgotesting "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"
 
 	"github.com/google/go-cmp/cmp"
 	"go.opencensus.io/resource"
@@ -1996,3 +1997,97 @@ func TestComputeActivatorNum(t *testing.T) {
 		})
 	}
 }
+
+func TestComputeStatus(t *testing.T) {
+	cases := []struct {
+		name string
+
+		haveActual       *int32
+		haveDesiredScale *int32
+
+		pcReady int
+		pcWant  int
+
+		wantActualScale  *int32
+		wantDesiredScale *int32
+	}{{
+		name: "initial",
+
+		haveActual:       nil,
+		haveDesiredScale: nil,
+
+		pcReady: 0,
+		pcWant:  1,
+
+		wantActualScale:  pointer.Int32(0),
+		wantDesiredScale: pointer.Int32(1),
+	}, {
+		name: "ready",
+
+		haveActual:       pointer.Int32(0),
+		haveDesiredScale: pointer.Int32(1),
+
+		pcReady: 1,
+		pcWant:  1,
+
+		wantActualScale:  pointer.Int32(1),
+		wantDesiredScale: pointer.Int32(1),
+	}, {
+		name: "stable",
+
+		haveActual:       pointer.Int32(1),
+		haveDesiredScale: pointer.Int32(1),
+
+		pcReady: 1,
+		pcWant:  1,
+
+		wantActualScale:  pointer.Int32(1),
+		wantDesiredScale: pointer.Int32(1),
+	}, {
+		name: "no metrics",
+
+		haveActual:       pointer.Int32(1),
+		haveDesiredScale: pointer.Int32(2),
+
+		pcReady: 2,
+		pcWant:  -1,
+
+		wantActualScale:  pointer.Int32(2),
+		wantDesiredScale: pointer.Int32(2),
+	}}
+
+	tc := &testConfigStore{config: defaultConfig()}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			ctx := tc.ToContext(context.Background())
+
+			pa := &autoscalingv1alpha1.PodAutoscaler{
+				Status: autoscalingv1alpha1.PodAutoscalerStatus{
+					ActualScale:  c.haveActual,
+					DesiredScale: c.haveDesiredScale,
+				},
+			}
+			pc := podCounts{
+				ready: c.pcReady,
+				want:  c.pcWant,
+			}
+
+			computeStatus(ctx, pa, pc, logging.FromContext(ctx))
+
+			if c.wantActualScale == nil && pa.Status.ActualScale != nil || c.wantActualScale != nil && pa.Status.ActualScale == nil {
+				t.Errorf("Unexpected ActualScale. Want: %v, Got: %v", c.wantActualScale, pa.Status.ActualScale)
+			}
+			if c.wantActualScale != nil && pa.Status.ActualScale != nil && *c.wantActualScale != *pa.Status.ActualScale {
+				t.Errorf("Unexpected ActualScale. Want: %d, Got: %d", *c.wantActualScale, *pa.Status.ActualScale)
+			}
+
+			if c.wantDesiredScale == nil && pa.Status.DesiredScale != nil || c.wantDesiredScale != nil && pa.Status.DesiredScale == nil {
+				t.Errorf("Unexpected DesiredScale. Want: %v, Got: %v", c.wantDesiredScale, pa.Status.DesiredScale)
+			}
+			if c.wantDesiredScale != nil && pa.Status.DesiredScale != nil && *c.wantDesiredScale != *pa.Status.DesiredScale {
+				t.Errorf("Unexpected DesiredScale. Want: %d, Got: %d", *c.wantDesiredScale, *pa.Status.DesiredScale)
+			}
+		})
+	}
+}