From d08cde04602811dfd759d223e2a2798bc720d4d0 Mon Sep 17 00:00:00 2001 From: Sascha Schwarze Date: Mon, 5 Feb 2024 11:37:20 +0100 Subject: [PATCH 1/2] Prevent a PodAutoscaler's DesiredScale to turn to -1 --- pkg/reconciler/autoscaling/kpa/kpa.go | 10 ++- pkg/reconciler/autoscaling/kpa/kpa_test.go | 97 +++++++++++++++++++++- 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 6c185949d534..a7dd17851544 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 { + logger.Debugf("Ignoring change of desiredScale from %d to %d", *pa.Status.DesiredScale, pc.want) + } else { + pa.Status.DesiredScale = ptr.Int32(int32(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 f16450cc44f2..5129eefd1c84 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" @@ -601,7 +602,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ // SKS does not exist, so we're just creating and have no status. Object: kpa(testNamespace, testRevision, WithPASKSNotReady("No Private Service Name"), - WithBufferedTraffic, WithPAMetricsService(privateSvc), withScales(0, unknownScale), + WithBufferedTraffic, WithPAMetricsService(privateSvc), withScales(0, defaultScale), WithObservedGeneration(1)), }}, WantCreates: []runtime.Object{ @@ -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) + } + }) + } +} From 87a5c849cf4f478c923ce08214595a248ed532aa Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 27 Mar 2024 17:06:36 -0400 Subject: [PATCH 2/2] use correct ptr package --- pkg/reconciler/autoscaling/kpa/kpa_test.go | 29 +++++++++++----------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 5129eefd1c84..e4df8601e893 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -55,7 +55,6 @@ 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" @@ -2019,41 +2018,41 @@ func TestComputeStatus(t *testing.T) { pcReady: 0, pcWant: 1, - wantActualScale: pointer.Int32(0), - wantDesiredScale: pointer.Int32(1), + wantActualScale: ptr.Int32(0), + wantDesiredScale: ptr.Int32(1), }, { name: "ready", - haveActual: pointer.Int32(0), - haveDesiredScale: pointer.Int32(1), + haveActual: ptr.Int32(0), + haveDesiredScale: ptr.Int32(1), pcReady: 1, pcWant: 1, - wantActualScale: pointer.Int32(1), - wantDesiredScale: pointer.Int32(1), + wantActualScale: ptr.Int32(1), + wantDesiredScale: ptr.Int32(1), }, { name: "stable", - haveActual: pointer.Int32(1), - haveDesiredScale: pointer.Int32(1), + haveActual: ptr.Int32(1), + haveDesiredScale: ptr.Int32(1), pcReady: 1, pcWant: 1, - wantActualScale: pointer.Int32(1), - wantDesiredScale: pointer.Int32(1), + wantActualScale: ptr.Int32(1), + wantDesiredScale: ptr.Int32(1), }, { name: "no metrics", - haveActual: pointer.Int32(1), - haveDesiredScale: pointer.Int32(2), + haveActual: ptr.Int32(1), + haveDesiredScale: ptr.Int32(2), pcReady: 2, pcWant: -1, - wantActualScale: pointer.Int32(2), - wantDesiredScale: pointer.Int32(2), + wantActualScale: ptr.Int32(2), + wantDesiredScale: ptr.Int32(2), }} tc := &testConfigStore{config: defaultConfig()}