Don't mark idle revisions as ProgressDeadlineExceeded

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -37,6 +37,12 @@ var podCondSet = apis.NewLivingConditionSet(
 	PodAutoscalerConditionSKSReady,
 )
 
+// ReasonTimedOut is set when the target failed to activate within the progress deadline.
+const ReasonTimedOut = "TimedOut"
+
+// ReasonNoTraffic is set when the target is inactive because it is not receiving traffic.
+const ReasonNoTraffic = "NoTraffic"
+
 // GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
 func (*PodAutoscaler) GetConditionSet() apis.ConditionSet {
 	return podCondSet

pkg/apis/serving/v1/revision_lifecycle.go

@@ -228,7 +228,9 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA
 		// ScaleTargetInitialized down the road, we would have marked resources
 		// unavailable here, and have no way of recovering later.
 		// If the ResourcesAvailable is already false, don't override the message.
-		if !ps.IsScaleTargetInitialized() && !resUnavailable && ps.ServiceName != "" {
+		// Only a TimedOut PA indicates a missed deadline; NoTraffic is just idle (#16694).
+		if !ps.IsScaleTargetInitialized() && !resUnavailable && ps.ServiceName != "" &&
+			cond.Reason == autoscalingv1alpha1.ReasonTimedOut {
 			rs.MarkResourcesAvailableFalse(ReasonProgressDeadlineExceeded,
 				"Initial scale was never achieved")
 		}

pkg/apis/serving/v1/revision_lifecycle_test.go

@@ -675,6 +675,7 @@ func TestPropagateAutoscalerStatusNoProgress(t *testing.T) {
 			Conditions: duckv1.Conditions{{
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
 				Status: corev1.ConditionFalse,
+				Reason: autoscalingv1alpha1.ReasonTimedOut,
 			}, {
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized,
 				Status: corev1.ConditionUnknown,
@@ -697,6 +698,7 @@ func TestPropagateAutoscalerStatusNoProgress(t *testing.T) {
 			Conditions: duckv1.Conditions{{
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
 				Status: corev1.ConditionFalse,
+				Reason: autoscalingv1alpha1.ReasonTimedOut,
 			}, {
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized,
 				Status: corev1.ConditionUnknown,
@@ -710,6 +712,75 @@ func TestPropagateAutoscalerStatusNoProgress(t *testing.T) {
 	}
 }
 
+func TestPropagateAutoscalerStatusNoTrafficIsNotProgressDeadline(t *testing.T) {
+	r := &RevisionStatus{}
+	r.InitializeConditions()
+	apistest.CheckConditionOngoing(r, RevisionConditionReady, t)
+
+	// PodAutoscaler is inactive because it has no traffic, not because it timed out.
+	r.PropagateAutoscalerStatus(&autoscalingv1alpha1.PodAutoscalerStatus{
+		ServiceName: "testRevision",
+		Status: duckv1.Status{
+			Conditions: duckv1.Conditions{{
+				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
+				Status: corev1.ConditionFalse,
+				Reason: autoscalingv1alpha1.ReasonNoTraffic,
+			}, {
+				Type:   autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized,
+				Status: corev1.ConditionUnknown,
+			}},
+		},
+	})
+	apistest.CheckConditionFailed(r, RevisionConditionActive, t)
+
+	cond := r.GetCondition(RevisionConditionResourcesAvailable)
+	if cond.IsFalse() {
+		t.Errorf("ResourcesAvailable = False, want not-False; reason = %q", cond.Reason)
+	}
+	if got, notWant := cond.Reason, ReasonProgressDeadlineExceeded; got == notWant {
+		t.Error("NoTraffic PA status was mistaken for ProgressDeadlineExceeded")
+	}
+}
+
+func TestPropagateAutoscalerStatusTimedOutAcrossReconciles(t *testing.T) {
+	r := &RevisionStatus{}
+	r.InitializeConditions()
+
+	ds := &appsv1.DeploymentStatus{
+		Conditions: []appsv1.DeploymentCondition{{
+			Type:   appsv1.DeploymentProgressing,
+			Status: corev1.ConditionTrue,
+		}, {
+			Type:   appsv1.DeploymentAvailable,
+			Status: corev1.ConditionTrue,
+		}},
+	}
+	ps := &autoscalingv1alpha1.PodAutoscalerStatus{
+		ServiceName: "testRevision",
+		Status: duckv1.Status{
+			Conditions: duckv1.Conditions{{
+				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
+				Status: corev1.ConditionFalse,
+				Reason: autoscalingv1alpha1.ReasonTimedOut,
+			}, {
+				Type:   autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized,
+				Status: corev1.ConditionUnknown,
+			}},
+		},
+	}
+
+	for i := range 3 {
+		r.PropagateDeploymentStatus(ds)
+		r.PropagateAutoscalerStatus(ps)
+
+		cond := r.GetCondition(RevisionConditionResourcesAvailable)
+		if !cond.IsFalse() || cond.Reason != ReasonProgressDeadlineExceeded {
+			t.Errorf("reconcile %d: ResourcesAvailable = %s/%s, want False/%s",
+				i, cond.Status, cond.Reason, ReasonProgressDeadlineExceeded)
+		}
+	}
+}
+
 func TestPropagateAutoscalerStatusRace(t *testing.T) {
 	r := &RevisionStatus{}
 	r.InitializeConditions()

pkg/reconciler/autoscaling/kpa/kpa.go

@@ -49,7 +49,7 @@ import (
 
 const (
 	noPrivateServiceName = "No Private Service Name"
-	noTrafficReason      = "NoTraffic"
+	noTrafficReason      = autoscalingv1alpha1.ReasonNoTraffic
 	minActivators        = 2
 )
 
@@ -292,9 +292,9 @@ func computeActiveCondition(ctx context.Context, pa *autoscalingv1alpha1.PodAuto
 	switch {
 	// Need to check for minReady = 0 because in the initialScale 0 case, pc.want will be -1.
 	case pc.want == 0 || minReady == 0:
-		if pa.Status.IsActivating() && minReady > 0 {
+		if (pa.Status.IsActivating() && minReady > 0) || hasTimedOut(pa) {
 			// 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.")
+			pa.Status.MarkInactive(autoscalingv1alpha1.ReasonTimedOut, "The target could not be activated.")
 		} else {
 			pa.Status.MarkInactive(noTrafficReason, "The target is not receiving traffic.")
 		}
@@ -311,7 +311,11 @@ func computeActiveCondition(ctx context.Context, pa *autoscalingv1alpha1.PodAuto
 			// 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(noTrafficReason, "The target is not receiving traffic.")
+			if hasTimedOut(pa) {
+				pa.Status.MarkInactive(autoscalingv1alpha1.ReasonTimedOut, "The target could not be activated.")
+			} else {
+				pa.Status.MarkInactive(noTrafficReason, "The target is not receiving traffic.")
+			}
 		}
 
 	case pc.ready >= minReady:
@@ -321,6 +325,12 @@ func computeActiveCondition(ctx context.Context, pa *autoscalingv1alpha1.PodAuto
 	}
 }
 
+// hasTimedOut returns true if the PA previously failed to activate and has not reached initial scale since.
+func hasTimedOut(pa *autoscalingv1alpha1.PodAutoscaler) bool {
+	return pa.Status.IsInactive() && !pa.Status.IsScaleTargetInitialized() &&
+		pa.Status.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionActive).GetReason() == autoscalingv1alpha1.ReasonTimedOut
+}
+
 // activeThreshold returns the scale required for the pa to be marked Active
 func activeThreshold(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler) int {
 	asConfig := config.FromContext(ctx).Autoscaler

pkg/reconciler/autoscaling/kpa/kpa_test.go

@@ -808,6 +808,38 @@ func TestReconcile(t *testing.T) {
 			Name:  deployName,
 			Patch: []byte(`[{"op":"add","path":"/spec/replicas","value":0}]`),
 		}},
+	}, {
+		Name: "activation failure is preserved after scaling to zero",
+		Key:  key,
+		Ctx: context.WithValue(context.Background(), deciderKey{},
+			decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */)),
+		Objects: []runtime.Object{
+			kpa(testNamespace, testRevision, withScales(0, 0),
+				WithNoTraffic(autoscalingv1alpha1.ReasonTimedOut, "The target could not be activated."),
+				WithPASKSReady, markOld, WithPAStatusService(testRevision),
+				WithPAMetricsService(privateSvc), WithObservedGeneration(1)),
+			sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady),
+			metric(testNamespace, testRevision),
+			deploy(testNamespace, testRevision, func(d *appsv1.Deployment) {
+				d.Spec.Replicas = ptr.Int32(0)
+			}),
+		},
+	}, {
+		Name: "activation failure is preserved with unknown desired scale",
+		Key:  key,
+		Ctx: context.WithValue(context.Background(), deciderKey{},
+			decider(testNamespace, testRevision, unknownScale, 0 /* ebc */)),
+		Objects: []runtime.Object{
+			kpa(testNamespace, testRevision, withScales(0, 0),
+				WithNoTraffic(autoscalingv1alpha1.ReasonTimedOut, "The target could not be activated."),
+				WithPASKSReady, markOld, WithPAStatusService(testRevision),
+				WithPAMetricsService(privateSvc), WithObservedGeneration(1)),
+			sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady),
+			metric(testNamespace, testRevision),
+			deploy(testNamespace, testRevision, func(d *appsv1.Deployment) {
+				d.Spec.Replicas = ptr.Int32(0)
+			}),
+		},
 	}, {
 		Name: "want=-1, underscaled, PA inactive",
 		// No-op

pkg/reconciler/revision/table_test.go

@@ -399,16 +399,44 @@ func TestReconcile(t *testing.T) {
 				readyDeploy(deploy(t, "foo", "pa-inactive")),
 				image("foo", "pa-inactive"),
 			},
+			WantUpdates: []clientgotesting.UpdateActionImpl{{
+				Object: pa("foo", "pa-inactive",
+					WithNoTraffic("NoTraffic", "This thing is inactive."),
+					WithPAStatusService("pa-inactive")),
+			}},
 			WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 				Object: Revision("foo", "pa-inactive",
 					WithLogURL, withDefaultContainerStatuses(), MarkDeploying(""),
+					markResourcesAvailableUnknown(v1.ReasonDeploying),
 					// When we reconcile an "all ready" revision when the PA
 					// is inactive, we should see the following change.
 					MarkInactive("NoTraffic", "This thing is inactive."),
+					WithRevisionObservedGeneration(1)),
+			}},
+			Key: "foo/pa-inactive",
+		},
+		{
+			Name: "pa timed out while activating",
+			// The PA timed out activating, so resources are marked unavailable.
+			Objects: []runtime.Object{
+				Revision("foo", "pa-timed-out",
+					WithLogURL,
+					WithRevisionObservedGeneration(1)),
+				pa("foo", "pa-timed-out",
+					WithReachability(autoscalingv1alpha1.ReachabilityUnreachable),
+					WithNoTraffic(autoscalingv1alpha1.ReasonTimedOut, "The target could not be activated."),
+					WithPAStatusService("pa-timed-out")),
+				readyDeploy(deploy(t, "foo", "pa-timed-out")),
+				image("foo", "pa-timed-out"),
+			},
+			WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+				Object: Revision("foo", "pa-timed-out",
+					WithLogURL, withDefaultContainerStatuses(), MarkDeploying(""),
+					MarkInactive(autoscalingv1alpha1.ReasonTimedOut, "The target could not be activated."),
 					WithRevisionObservedGeneration(1),
 					MarkResourcesUnavailable(v1.ReasonProgressDeadlineExceeded, "Initial scale was never achieved")),
 			}},
-			Key: "foo/pa-inactive",
+			Key: "foo/pa-timed-out",
 		},
 		{
 			Name: "pa is not ready with initial scale zero, but ServiceName still empty, so not marking resources available false",
@@ -1077,6 +1105,12 @@ func withDefaultContainerStatuses() RevisionOption {
 	}
 }
 
+func markResourcesAvailableUnknown(reason string) RevisionOption {
+	return func(r *v1.Revision) {
+		r.Status.MarkResourcesAvailableUnknown(reason, "")
+	}
+}
+
 func withInitContainerStatuses() RevisionOption {
 	return func(r *v1.Revision) {
 		r.Status.InitContainerStatuses = []v1.ContainerStatus{{