Mark ScaleTargetInitialized for case of upgrading an inactive ksvc pr…

…e-0.17 (#9110) (#9119)

* Mark ScaleTargetInitialized for case of upgrading an inactive ksvc pre-0.17

- Mark ScaleTargetInitialized if PA is inactive because of no traffic
- Add upgrade and unit tests

* Use const

* Review

* Update preupgrade test

Co-authored-by: Tara Gu <guw@ibm.com>

pkg/reconciler/autoscaling/kpa/kpa.go

@@ -44,7 +44,10 @@ import (
 	corev1listers "k8s.io/client-go/listers/core/v1"
 )
 
-const noPrivateServiceName = "No Private Service Name"
+const (
+	noPrivateServiceName = "No Private Service Name"
+	noTrafficReason      = "NoTraffic"
+)
 
 // podCounts keeps record of various numbers of pods
 // for each revision.
@@ -236,7 +239,11 @@ func reportMetrics(pa *pav1alpha1.PodAutoscaler, pc podCounts) error {
 //    | -1   | >= min | >0    | active     | active     |
 func computeActiveCondition(ctx context.Context, pa *pav1alpha1.PodAutoscaler, pc podCounts) {
 	minReady := activeThreshold(ctx, pa)
-	if pc.ready >= minReady {
+	// In pre-0.17 we could have scaled down normally without ever setting ScaleTargetInitialized.
+	// In this case we'll be in the NoTraffic/inactive state.
+	// TODO(taragu): remove after 0.19
+	alreadyScaledDownSuccessfully := minReady > 0 && pa.Status.GetCondition(pav1alpha1.PodAutoscalerConditionActive).Reason == noTrafficReason
+	if pc.ready >= minReady || alreadyScaledDownSuccessfully {
 		pa.Status.MarkScaleTargetInitialized()
 	}
 
@@ -247,7 +254,7 @@ func computeActiveCondition(ctx context.Context, pa *pav1alpha1.PodAutoscaler, p
 			// We only ever scale to zero while activating if we fail to activate within the progress deadline.
 			pa.Status.MarkInactive("TimedOut", "The target could not be activated.")
 		} else {
-			pa.Status.MarkInactive("NoTraffic", "The target is not receiving traffic.")
+			pa.Status.MarkInactive(noTrafficReason, "The target is not receiving traffic.")
 		}
 
 	case pc.ready < minReady:
@@ -262,7 +269,7 @@ func computeActiveCondition(ctx context.Context, pa *pav1alpha1.PodAutoscaler, p
 			// still need to set it again. Otherwise reconciliation will fail with NewObservedGenFailure
 			// because we cannot go through one iteration of reconciliation without setting
 			// some status.
-			pa.Status.MarkInactive("NoTraffic", "The target is not receiving traffic.")
+			pa.Status.MarkInactive(noTrafficReason, "The target is not receiving traffic.")
 		}
 
 	case pc.ready >= minReady:

pkg/reconciler/autoscaling/kpa/kpa_test.go

@@ -244,8 +244,8 @@ func TestReconcile(t *testing.T) {
 
 	inactiveKPAMinScale := func(g int32) *asv1a1.PodAutoscaler {
 		return kpa(
-			testNamespace, testRevision, WithPASKSNotReady(""),
-			WithNoTraffic("NoTraffic", "The target is not receiving traffic."),
+			testNamespace, testRevision, WithPASKSNotReady(""), WithScaleTargetInitialized,
+			WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
 			withScales(g, unknownScale), WithReachabilityReachable,
 			withMinScale(defaultScale), WithPAStatusService(testRevision),
 			WithPAMetricsService(privateSvc), WithObservedGeneration(1),
@@ -438,7 +438,7 @@ func TestReconcile(t *testing.T) {
 		Name: "pa activates",
 		Key:  key,
 		Objects: []runtime.Object{
-			kpa(testNamespace, testRevision, WithNoTraffic("NoTraffic", "The target is not receiving traffic."),
+			kpa(testNamespace, testRevision, WithScaleTargetInitialized, WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
 				withScales(0, defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)),
 			// SKS is ready here, since its endpoints are populated with Activator endpoints.
 			sks(testNamespace, testRevision, WithProxyMode, WithDeployRef(deployName), WithSKSReady),
@@ -450,7 +450,7 @@ func TestReconcile(t *testing.T) {
 				WithDeployRef(deployName)),
 		}},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
-			Object: kpa(testNamespace, testRevision, WithBufferedTraffic, withScales(0, defaultScale),
+			Object: kpa(testNamespace, testRevision, WithScaleTargetInitialized, WithBufferedTraffic, withScales(0, defaultScale),
 				WithPASKSReady, WithPAMetricsService(privateSvc),
 				WithPAStatusService(testRevision), WithObservedGeneration(1)),
 		}},
@@ -677,8 +677,8 @@ func TestReconcile(t *testing.T) {
 		Ctx: context.WithValue(context.Background(), deciderKey{},
 			decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */, scaling.MinActivators)),
 		Objects: []runtime.Object{
-			kpa(testNamespace, testRevision, withScales(0, 0),
-				WithNoTraffic("NoTraffic", "The target is not receiving traffic."),
+			kpa(testNamespace, testRevision, WithScaleTargetInitialized, withScales(0, 0),
+				WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
 				WithPASKSReady, markOld, WithPAStatusService(testRevision),
 				WithPAMetricsService(privateSvc), WithObservedGeneration(1)),
 			sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady),
@@ -693,8 +693,8 @@ func TestReconcile(t *testing.T) {
 		Ctx: context.WithValue(context.Background(), deciderKey{},
 			decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */, scaling.MinActivators)),
 		Objects: []runtime.Object{
-			kpa(testNamespace, testRevision, withScales(0, 0),
-				WithNoTraffic("NoTraffic", "The target is not receiving traffic."),
+			kpa(testNamespace, testRevision, WithScaleTargetInitialized, withScales(0, 0),
+				WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
 				WithPASKSReady, markOld, WithPAStatusService(testRevision),
 				WithPAMetricsService(privateSvc), WithObservedGeneration(1)),
 			sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady),
@@ -722,7 +722,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: kpa(testNamespace, testRevision, markScaleTargetInitialized, withScales(1, 0),
 				WithPASKSReady, WithPAMetricsService(privateSvc),
-				WithNoTraffic("NoTraffic", "The target is not receiving traffic."),
+				WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
 				WithPAStatusService(testRevision), WithPAMetricsService(privateSvc),
 				WithObservedGeneration(1)),
 		}},
@@ -796,7 +796,7 @@ func TestReconcile(t *testing.T) {
 			defaultMetric,
 		}, underscaledReady...),
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
-			Object: activatingKPAMinScale(underscale),
+			Object: activatingKPAMinScale(underscale, WithScaleTargetInitialized),
 		}},
 		WantPatches: []clientgotesting.PatchActionImpl{
 			minScalePatch,
@@ -909,8 +909,8 @@ func TestReconcile(t *testing.T) {
 			decider(testNamespace, testRevision, unknownScale, /* desiredScale */
 				0 /* ebc */, scaling.MinActivators)),
 		Objects: []runtime.Object{
-			kpa(testNamespace, testRevision, WithPASKSReady,
-				WithNoTraffic("NoTraffic", "The target is not receiving traffic."), WithPAMetricsService(privateSvc),
+			kpa(testNamespace, testRevision, WithScaleTargetInitialized, WithPASKSReady,
+				WithNoTraffic(noTrafficReason, "The target is not receiving traffic."), WithPAMetricsService(privateSvc),
 				withScales(0, -1), WithPAStatusService(testRevision), WithObservedGeneration(1)),
 			sks(testNamespace, testRevision, WithDeployRef(deployName),
 				WithProxyMode, WithSKSReady),
@@ -1056,7 +1056,7 @@ func TestReconcile(t *testing.T) {
 		},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: kpa(testNamespace, testRevision, markScaleTargetInitialized,
-				WithNoTraffic("NoTraffic", "The target is not receiving traffic."),
+				WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
 				withScales(0, -1), WithReachabilityReachable,
 				WithPAMetricsService(privateSvc), WithObservedGeneration(1),
 				WithPASKSNotReady(""),
@@ -1107,6 +1107,30 @@ func TestReconcile(t *testing.T) {
 				WithPAMetricsService(privateSvc), WithObservedGeneration(1),
 			),
 		}},
+	}, {
+		Name: "mark initial scale reached for an existing inactive PA",
+		Key:  key,
+		Ctx: context.WithValue(context.Background(), deciderKey{},
+			decider(testNamespace, testRevision, -1, /* desiredScale */
+				-42 /* ebc */, scaling.MinActivators)),
+		Objects: append([]runtime.Object{
+			kpa(testNamespace, testRevision, WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
+				withScales(0, -1), WithReachabilityReachable, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc),
+				WithPASKSReady,
+			),
+			sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady),
+			metric(testNamespace, testRevision),
+			deploy(testNamespace, testRevision, func(d *appsv1.Deployment) {
+				d.Spec.Replicas = ptr.Int32(2)
+			}),
+		}),
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: kpa(testNamespace, testRevision, WithNoTraffic(noTrafficReason, "The target is not receiving traffic."),
+				WithPASKSReady, markScaleTargetInitialized,
+				withScales(0, -1), WithReachabilityReachable, WithPAStatusService(testRevision),
+				WithPAMetricsService(privateSvc), WithObservedGeneration(1),
+			),
+		}},
 	}}
 
 	table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {

test/upgrade/service_postupgrade_test.go

@@ -140,3 +140,17 @@ func TestCreateNewServicePostUpgrade(t *testing.T) {
 	t.Parallel()
 	createNewService(postUpgradeServiceName, t)
 }
+
+func TestInitialScalePostUpgrade(t *testing.T) {
+	t.Parallel()
+	clients := e2e.Setup(t)
+
+	t.Logf("Getting service %q", initialScaleServiceName)
+	svc, err := clients.ServingClient.Services.Get(initialScaleServiceName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatal("Failed to get Service:", err)
+	}
+	if !svc.IsReady() {
+		t.Fatalf("Post upgrade Service is not ready with reason %q", svc.Status.GetCondition(v1.ServiceConditionRoutesReady).Reason)
+	}
+}

test/upgrade/service_preupgrade_test.go

@@ -90,3 +90,20 @@ func TestBYORevisionPreUpgrade(t *testing.T) {
 		t.Fatal("Failed to create Service:", err)
 	}
 }
+
+func TestInitialScalePreUpgrade(t *testing.T) {
+	t.Parallel()
+	clients := e2e.Setup(t)
+	names := test.ResourceNames{
+		Service: initialScaleServiceName,
+		Image:   test.PizzaPlanet1,
+	}
+
+	resources, err := v1test.CreateServiceReady(t, clients, &names)
+	if err != nil {
+		t.Fatal("Failed to create Service:", err)
+	}
+	if err = e2e.WaitForScaleToZero(t, revisionresourcenames.Deployment(resources.Revision), clients); err != nil {
+		t.Fatal("Could not scale to zero:", err)
+	}
+}

test/upgrade/upgrade.go

@@ -41,6 +41,7 @@ const (
 	scaleToZeroServiceName   = "scale-to-zero-upgrade-service"
 	byoServiceName           = "byo-revision-name-upgrade-test"
 	byoRevName               = byoServiceName + "-" + "rev1"
+	initialScaleServiceName  = "init-scale-service"
 )
 
 // Shamelessly cribbed from conformance/service_test.