Sync up with the autoscaler in serving for 1.15 release

.github/workflows/kind-e2e.yaml

@@ -35,7 +35,7 @@ env:
   YTT_VERSION: 0.40.1
   KO_FLAGS: --platform=linux/amd64
   GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-  KNATIVE_SERVING_BRANCH: main
+  KNATIVE_SERVING_BRANCH: release-1.15
   KNATIVE_DIR: /home/runner/work/serving-progressive-rollout
   KNATIVE_SERVING_DIR: /home/runner/work/serving-progressive-rollout/serving
 

go.mod

@@ -15,7 +15,7 @@ require (
 	k8s.io/code-generator v0.29.2
 	k8s.io/kube-openapi v0.0.0-20231010175941-2dd684a91f00
 	knative.dev/caching v0.0.0-20240716132144-989f54c83776
-	knative.dev/hack v0.0.0-20240704013904-b9799599afcf
+	knative.dev/hack v0.0.0-20240719133331-9c9eed6f6679
 	knative.dev/networking v0.0.0-20240716111826-bab7f2a3e556
 	knative.dev/pkg v0.0.0-20240716082220-4355f0c73608
 	knative.dev/serving v0.42.0

go.sum

@@ -801,8 +801,8 @@ k8s.io/utils v0.0.0-20240102154912-e7106e64919e h1:eQ/4ljkx21sObifjzXwlPKpdGLrCf
 k8s.io/utils v0.0.0-20240102154912-e7106e64919e/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 knative.dev/caching v0.0.0-20240716132144-989f54c83776 h1:2nINnWuXtb9e2nG/EJxSCeghcmu6qmvmomJ7woiP5Is=
 knative.dev/caching v0.0.0-20240716132144-989f54c83776/go.mod h1:Uj74eO9rLiK1eb8wmDBED1hJBZQ7MJ9cvq/d8Ktsm3c=
-knative.dev/hack v0.0.0-20240704013904-b9799599afcf h1:n92FmZRywgtHso7pFAku7CW0qvRAs1hXtMQqO0R6eiE=
-knative.dev/hack v0.0.0-20240704013904-b9799599afcf/go.mod h1:yk2OjGDsbEnQjfxdm0/HJKS2WqTLEFg/N6nUs6Rqx3Q=
+knative.dev/hack v0.0.0-20240719133331-9c9eed6f6679 h1:tvbANb4KIO91DT1FGR4yCLA5E0qAmIeQ3DAGOkZGg4k=
+knative.dev/hack v0.0.0-20240719133331-9c9eed6f6679/go.mod h1:R0ritgYtjLDO9527h5vb5X6gfvt5LCrJ55BNbVDsWiY=
 knative.dev/networking v0.0.0-20240716111826-bab7f2a3e556 h1:9OTyJkrjiFh/burZiti3WucGv8Qtt91VJTnXfO5dC2g=
 knative.dev/networking v0.0.0-20240716111826-bab7f2a3e556/go.mod h1:1PosUDkXqoHNzYxtLIwa7LFqSsIXBShHOseAb6XBeEU=
 knative.dev/pkg v0.0.0-20240716082220-4355f0c73608 h1:BOiRzcnRS9Z5ruxlCiS/K1/Hb5bUN0X4W3xCegdcYQE=

pkg/reconciler/autoscaling/kpa/kpa.go

@@ -21,9 +21,8 @@ import (
 	"fmt"
 	"math"
 
-	goStats "go.opencensus.io/stats"
+	"go.opencensus.io/stats"
 	"go.uber.org/zap"
-	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
 
 	nv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/logging"
@@ -229,7 +228,15 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *autoscalingv1alph
 }
 
 func computeStatus(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler, spa *autoscalingv1.StagePodAutoscaler, 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, spa, pc)
@@ -242,7 +249,7 @@ func reportMetrics(pa *autoscalingv1alpha1.PodAutoscaler, pc podCounts) {
 
 	ctx := metrics.RevisionContext(pa.Namespace, serviceLabel, configLabel, pa.Name)
 
-	stats := []goStats.Measurement{
+	stats := []stats.Measurement{
 		actualPodCountM.M(int64(pc.ready)), notReadyPodCountM.M(int64(pc.notReady)),
 		pendingPodCountM.M(int64(pc.pending)), terminatingPodCountM.M(int64(pc.terminating)),
 	}
@@ -358,19 +365,3 @@ func computeNumActivators(readyPods int, decider *scaling.Decider) int32 {
 
 	return int32(math.Max(minActivators, math.Ceil(capacityToCover/decider.Spec.ActivatorCapacity)))
 }
-
-// GetScaleBounds returns the min and the max scales for the current stage.
-func GetScaleBounds(asConfig *autoscalerconfig.Config, pa *autoscalingv1alpha1.PodAutoscaler,
-	spa *autoscalingv1.StagePodAutoscaler) (int32, int32) {
-	min, max := pa.ScaleBounds(asConfig)
-	if spa != nil {
-		minS, maxS := spa.ScaleBounds()
-		if minS != nil && min > *minS {
-			min = *minS
-		}
-		if maxS != nil && max > *maxS {
-			max = *maxS
-		}
-	}
-	return min, max
-}

pkg/reconciler/autoscaling/kpa/kpa_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 The Knative Authors
+Copyright 2023 The Knative Authors
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -43,10 +43,6 @@ import (
 
 	networkingclient "knative.dev/networking/pkg/client/injection/client"
 	filteredinformerfactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
-	_ "knative.dev/pkg/metrics/testing"
-	_ "knative.dev/pkg/system/testing"
-	_ "knative.dev/serving-progressive-rollout/pkg/client/injection/informers/serving/v1/rolloutorchestrator/fake"
-	_ "knative.dev/serving-progressive-rollout/pkg/client/injection/informers/serving/v1/stagepodautoscaler/fake"
 	servingclient "knative.dev/serving/pkg/client/injection/client"
 	"knative.dev/serving/pkg/client/injection/ducks/autoscaling/v1alpha1/podscalable"
 	pareconciler "knative.dev/serving/pkg/client/injection/reconciler/autoscaling/v1alpha1/podautoscaler"
@@ -73,9 +69,13 @@ import (
 	"knative.dev/pkg/kmeta"
 	"knative.dev/pkg/logging"
 	"knative.dev/pkg/metrics/metricstest"
+	_ "knative.dev/pkg/metrics/testing"
 	"knative.dev/pkg/ptr"
 	"knative.dev/pkg/reconciler"
 	"knative.dev/pkg/system"
+	_ "knative.dev/pkg/system/testing"
+	_ "knative.dev/serving-progressive-rollout/pkg/client/injection/informers/serving/v1/rolloutorchestrator/fake"
+	_ "knative.dev/serving-progressive-rollout/pkg/client/injection/informers/serving/v1/stagepodautoscaler/fake"
 	"knative.dev/serving/pkg/apis/autoscaling"
 	autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
 	"knative.dev/serving/pkg/apis/serving"
@@ -103,9 +103,6 @@ const (
 	paStableWindow           = 45 * time.Second
 	progressDeadline         = 121 * time.Second
 	stableWindow             = 5 * time.Minute
-	testNamespace            = "test-namespace"
-	testRevision             = "test-revision"
-	key                      = testNamespace + "/" + testRevision
 )
 
 func defaultConfigMapData() map[string]string {
@@ -606,7 +603,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{
@@ -1032,7 +1029,7 @@ func TestReconcile(t *testing.T) {
 				-42 /* ebc */)),
 		Objects: append([]runtime.Object{
 			kpa(testNamespace, testRevision, WithPASKSReady, WithBufferedTraffic,
-				withScales(20, defaultScale), withInitialScaleB(20), WithReachabilityReachable,
+				withScales(20, defaultScale), withInitialScale(20), WithReachabilityReachable,
 				WithPAStatusService(testRevision), WithPAMetricsService(privateSvc),
 			),
 			sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithNumActivators(scaledAct)),
@@ -1043,7 +1040,7 @@ func TestReconcile(t *testing.T) {
 		}, makeReadyPods(20, testNamespace, testRevision)...),
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: kpa(testNamespace, testRevision, WithPASKSReady, WithTraffic,
-				markScaleTargetInitialized, withScales(20, 20), withInitialScaleB(20), WithReachabilityReachable,
+				markScaleTargetInitialized, withScales(20, 20), withInitialScale(20), WithReachabilityReachable,
 				WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1),
 			),
 		}},
@@ -1921,10 +1918,6 @@ func TestResolveScrapeTarget(t *testing.T) {
 	}
 }
 
-func withInitialScaleB(initScale int) PodAutoscalerOption {
-	return withInitialScale(initScale)
-}
-
 func withInitialScale(initScale int) PodAutoscalerOption {
 	return func(pa *autoscalingv1alpha1.PodAutoscaler) {
 		pa.Annotations = kmeta.UnionMaps(
@@ -2007,19 +2000,108 @@ 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:  ptr.Int32(0),
+		wantDesiredScale: ptr.Int32(1),
+	}, {
+		name: "ready",
+
+		haveActual:       ptr.Int32(0),
+		haveDesiredScale: ptr.Int32(1),
+
+		pcReady: 1,
+		pcWant:  1,
+
+		wantActualScale:  ptr.Int32(1),
+		wantDesiredScale: ptr.Int32(1),
+	}, {
+		name: "stable",
+
+		haveActual:       ptr.Int32(1),
+		haveDesiredScale: ptr.Int32(1),
+
+		pcReady: 1,
+		pcWant:  1,
+
+		wantActualScale:  ptr.Int32(1),
+		wantDesiredScale: ptr.Int32(1),
+	}, {
+		name: "no metrics",
+
+		haveActual:       ptr.Int32(1),
+		haveDesiredScale: ptr.Int32(2),
+
+		pcReady: 2,
+		pcWant:  -1,
+
+		wantActualScale:  ptr.Int32(2),
+		wantDesiredScale: ptr.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, nil, 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)
+			}
+		})
+	}
+}
+
 func TestReconcileWithNSR(t *testing.T) {
 	testRevision := "testRevision1"
 	testNamespace := "testNamespace1"
 	key := testNamespace + "/" + testRevision
 	deployName := testRevision + "-deployment"
 	const (
 		defaultScale = 19
-		unknownScale = scaleUnknown
-		underscale   = defaultScale - 1
-		overscale    = defaultScale + 1
-
-		defaultAct = 3 // 1-10 ready pods + 200 TBC
-		scaledAct  = 4 // 11 or 12 ready pods + 200 TBC
+		defaultAct   = 3 // 1-10 ready pods + 200 TBC
 	)
 	privateSvc := names.PrivateService(testRevision)
 

pkg/reconciler/autoscaling/kpa/scaler.go

@@ -25,14 +25,14 @@ import (
 	"knative.dev/pkg/apis/duck"
 	"knative.dev/pkg/injection/clients/dynamicclient"
 	"knative.dev/pkg/logging"
-	autoscalingv1 "knative.dev/serving-progressive-rollout/pkg/apis/serving/v1"
 
 	netapis "knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	nethttp "knative.dev/networking/pkg/http"
 	netheader "knative.dev/networking/pkg/http/header"
 	netprober "knative.dev/networking/pkg/prober"
 	pkgnet "knative.dev/pkg/network"
+	autoscalingv1 "knative.dev/serving-progressive-rollout/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/activator"
 	autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
 	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
@@ -141,6 +141,10 @@ func activatorProbe(pa *autoscalingv1alpha1.PodAutoscaler, transport http.RoundT
 }
 
 func lastPodRetention(pa *autoscalingv1alpha1.PodAutoscaler, cfg *autoscalerconfig.Config) time.Duration {
+	// if revision is unreachable, no need to account for last pod retention
+	if pa.Spec.Reachability == autoscalingv1alpha1.ReachabilityUnreachable {
+		return 0
+	}
 	d, ok := pa.ScaleToZeroPodRetention()
 	if ok {
 		return d
@@ -322,6 +326,22 @@ func (ks *scaler) applyScale(ctx context.Context, pa *autoscalingv1alpha1.PodAut
 	return nil
 }
 
+// GetScaleBounds returns the min and the max scales for the current stage.
+func GetScaleBounds(asConfig *autoscalerconfig.Config, pa *autoscalingv1alpha1.PodAutoscaler,
+	spa *autoscalingv1.StagePodAutoscaler) (int32, int32) {
+	min, max := pa.ScaleBounds(asConfig)
+	if spa != nil {
+		minS, maxS := spa.ScaleBounds()
+		if minS != nil && min > *minS {
+			min = *minS
+		}
+		if maxS != nil && max > *maxS {
+			max = *maxS
+		}
+	}
+	return min, max
+}
+
 // scale attempts to scale the given PA's target reference to the desired scale.
 func (ks *scaler) scale(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler, spa *autoscalingv1.StagePodAutoscaler, sks *netv1alpha1.ServerlessService, desiredScale int32) (int32, error) {
 	asConfig := config.FromContext(ctx).Autoscaler

pkg/reconciler/autoscaling/kpa/scaler_test.go

@@ -26,11 +26,10 @@ import (
 	"testing"
 	"time"
 
-	autoscalingv1 "knative.dev/serving-progressive-rollout/pkg/apis/serving/v1"
-	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
-
 	// These are the fake informers we want setup.
 	fakedynamicclient "knative.dev/pkg/injection/clients/dynamicclient/fake"
+	autoscalingv1 "knative.dev/serving-progressive-rollout/pkg/apis/serving/v1"
+	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
 	fakeservingclient "knative.dev/serving/pkg/client/injection/client/fake"
 	podscalable "knative.dev/serving/pkg/client/injection/ducks/autoscaling/v1alpha1/podscalable/fake"
 
@@ -65,6 +64,12 @@ import (
 	. "knative.dev/serving/pkg/testing"
 )
 
+const (
+	testNamespace = "test-namespace"
+	testRevision  = "test-revision"
+	key           = testNamespace + "/" + testRevision
+)
+
 func TestScaler(t *testing.T) {
 	const activationTimeout = progressDeadline + activationTimeoutBuffer
 	tests := []struct {
@@ -355,6 +360,33 @@ func TestScaler(t *testing.T) {
 			paMarkInactive(k, time.Now().Add(-gracePeriod))
 			WithReachabilityReachable(k)
 		},
+	}, {
+		label:         "scale to zero ignore last pod retention if  revision is unreachable",
+		startReplicas: 1,
+		scaleTo:       0,
+		wantReplicas:  0,
+		wantScaling:   true,
+		paMutation: func(k *autoscalingv1alpha1.PodAutoscaler) {
+			paMarkInactive(k, time.Now().Add(-gracePeriod))
+			WithReachabilityUnreachable(k)
+		},
+		configMutator: func(c *config.Config) {
+			c.Autoscaler.ScaleToZeroPodRetentionPeriod = 10 * gracePeriod
+		},
+	}, {
+		label:         "can't scale to zero if revision is unreachable, but before deadline",
+		startReplicas: 1,
+		scaleTo:       0,
+		wantReplicas:  0,
+		wantScaling:   false,
+		paMutation: func(k *autoscalingv1alpha1.PodAutoscaler) {
+			paMarkInactive(k, time.Now().Add(-gracePeriod+time.Second))
+			WithReachabilityUnreachable(k)
+		},
+		configMutator: func(c *config.Config) {
+			c.Autoscaler.ScaleToZeroPodRetentionPeriod = 10 * gracePeriod
+		},
+		wantCBCount: 1,
 	}, {
 		label:         "ignore minScale if unreachable",
 		startReplicas: 10,
@@ -673,6 +705,7 @@ func newRevision(ctx context.Context, t *testing.T, servingClient clientset.Inte
 
 func newDeployment(ctx context.Context, t *testing.T, dynamicClient dynamic.Interface, name string, replicas int) *appsv1.Deployment {
 	t.Helper()
+
 	uns := &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"apiVersion": "apps/v1",