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{{