From 05b7ca7b8bc7e7b2917b5b8e56a996be6e64478a Mon Sep 17 00:00:00 2001 From: Anjali-Chauhan1 Date: Fri, 2 Oct 2026 18:30:32 +0530 Subject: [PATCH] Don't mark idle revisions as ProgressDeadlineExceeded A revision was marked ResourcesAvailable=False with reason ProgressDeadlineExceeded ("Initial scale was never achieved") whenever its PodAutoscaler was Ready=False with the scale target not yet initialized. That state also occurs for a freshly created revision whose PA is merely inactive because it has no traffic (e.g. initial-scale 0), so revisions were spuriously failed seconds after creation and recovered later. Only treat the PA as having missed its progress deadline when it reports the TimedOut reason, which the KPA sets solely when activation times out. To keep that signal reliable, the KPA now preserves TimedOut on subsequent reconciles until the PA activates again, instead of rewriting it to NoTraffic on the next pass, which would otherwise silently clear a genuine failure on the revision. Signed-off-by: Anjali-Chauhan1 --- pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go | 6 ++ pkg/apis/serving/v1/revision_lifecycle.go | 4 +- .../serving/v1/revision_lifecycle_test.go | 71 +++++++++++++++++++ pkg/reconciler/autoscaling/kpa/kpa.go | 18 +++-- pkg/reconciler/autoscaling/kpa/kpa_test.go | 32 +++++++++ pkg/reconciler/revision/table_test.go | 36 +++++++++- 6 files changed, 161 insertions(+), 6 deletions(-) diff --git a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go b/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go index 9cff0ab9f547..01db97a78426 100644 --- a/pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go +++ b/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 diff --git a/pkg/apis/serving/v1/revision_lifecycle.go b/pkg/apis/serving/v1/revision_lifecycle.go index fe10f1494468..daf2a71d428c 100644 --- a/pkg/apis/serving/v1/revision_lifecycle.go +++ b/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") } diff --git a/pkg/apis/serving/v1/revision_lifecycle_test.go b/pkg/apis/serving/v1/revision_lifecycle_test.go index 32ce9b20b09b..043cb3f01ccd 100644 --- a/pkg/apis/serving/v1/revision_lifecycle_test.go +++ b/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() diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 05ca47b07c69..67b1afa41b3c 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/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 diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 797af201c9aa..b2e771d41bb6 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/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 diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 32c86aadac68..aeae5dfe5510 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/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{{