PodAutoscaler Active Condition should not affect Reachability (knativ…

…e#14309) (knative#515)

* Don't change reachability if the PA is not active

* drop unused helper method

(cherry picked from commit 9ffab17)

Co-authored-by: Dave Protasowski <dprotaso@gmail.com>

pkg/apis/serving/v1/revision_helpers.go

@@ -128,11 +128,6 @@ func (r *Revision) GetRoutingStateModified() time.Time {
 	return parsed
 }
 
-// IsReachable returns whether or not the revision can be reached by a route.
-func (r *Revision) IsReachable() bool {
-	return RoutingState(r.Labels[serving.RoutingStateLabelKey]) == RoutingStateActive
-}
-
 // GetProtocol returns the app level network protocol.
 func (r *Revision) GetProtocol() net.ProtocolType {
 	ports := r.Spec.GetContainer().Ports

pkg/apis/serving/v1/revision_helpers_test.go

@@ -22,7 +22,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	net "knative.dev/networking/pkg/apis/networking"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
@@ -106,38 +105,6 @@ func TestIsActivationRequired(t *testing.T) {
 	}
 }
 
-func TestRevisionIsReachable(t *testing.T) {
-	tests := []struct {
-		name   string
-		labels map[string]string
-		want   bool
-	}{{
-		name:   "has serving state label",
-		labels: map[string]string{serving.RoutingStateLabelKey: "active"},
-		want:   true,
-	}, {
-		name:   "empty route annotation",
-		labels: map[string]string{serving.RoutingStateLabelKey: ""},
-		want:   false,
-	}, {
-		name:   "no route annotation",
-		labels: nil,
-		want:   false,
-	}}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			rev := Revision{ObjectMeta: metav1.ObjectMeta{Labels: tt.labels}}
-
-			got := rev.IsReachable()
-
-			if got != tt.want {
-				t.Errorf("IsReachable = %t, want: %t", got, tt.want)
-			}
-		})
-	}
-}
-
 func TestRevisionGetProtocol(t *testing.T) {
 	containerWithPortName := func(name string) corev1.Container {
 		return corev1.Container{Ports: []corev1.ContainerPort{{Name: name}}}

pkg/reconciler/revision/resources/pa.go

@@ -20,6 +20,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"knative.dev/pkg/apis"
 	"knative.dev/pkg/kmeta"
 	autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
@@ -44,27 +45,30 @@ func MakePA(rev *v1.Revision) *autoscalingv1alpha1.PodAutoscaler {
 				Name:       names.Deployment(rev),
 			},
 			ProtocolType: rev.GetProtocol(),
-			Reachability: func() autoscalingv1alpha1.ReachabilityType {
-				// If the Revision has failed to become Ready, then mark the PodAutoscaler as unreachable.
-				if rev.Status.GetCondition(v1.RevisionConditionReady).IsFalse() {
-					// Make sure that we don't do this when a newly failing revision is
-					// marked reachable by outside forces.
-					if !rev.IsReachable() {
-						return autoscalingv1alpha1.ReachabilityUnreachable
-					}
-				}
+			Reachability: reachability(rev),
+		},
+	}
+}
 
-				// We don't know the reachability if the revision has just been created
-				// or it is activating.
-				if rev.Status.GetCondition(v1.RevisionConditionActive).IsUnknown() {
-					return autoscalingv1alpha1.ReachabilityUnknown
-				}
+func reachability(rev *v1.Revision) autoscalingv1alpha1.ReachabilityType {
+	// check infra failures
+	conds := []apis.ConditionType{
+		v1.RevisionConditionResourcesAvailable,
+		v1.RevisionConditionContainerHealthy,
+	}
 
-				if rev.IsReachable() {
-					return autoscalingv1alpha1.ReachabilityReachable
-				}
-				return autoscalingv1alpha1.ReachabilityUnreachable
-			}(),
-		},
+	for _, cond := range conds {
+		if c := rev.Status.GetCondition(cond); c != nil && c.IsFalse() {
+			return autoscalingv1alpha1.ReachabilityUnreachable
+		}
+	}
+
+	switch rev.GetRoutingState() {
+	case v1.RoutingStateActive:
+		return autoscalingv1alpha1.ReachabilityReachable
+	case v1.RoutingStateReserve:
+		return autoscalingv1alpha1.ReachabilityUnreachable
+	default:
+		return autoscalingv1alpha1.ReachabilityUnknown
 	}
 }