wip

pkg/apis/serving/k8s_lifecycle.go

@@ -51,28 +51,40 @@ func TransformDeploymentStatus(ds *appsv1.DeploymentStatus) *duckv1.Status {
 	// below, we'll overwrite this.
 	depCondSet.Manage(s).MarkTrue(DeploymentConditionReplicaSetReady)
 
-	for _, cond := range ds.Conditions {
-		// TODO(jonjohnsonjr): Should we care about appsv1.DeploymentAvailable here?
-		switch cond.Type {
-		case appsv1.DeploymentProgressing:
-			switch cond.Status {
-			case corev1.ConditionUnknown:
-				depCondSet.Manage(s).MarkUnknown(DeploymentConditionProgressing, cond.Reason, cond.Message)
-			case corev1.ConditionTrue:
-				depCondSet.Manage(s).MarkTrue(DeploymentConditionProgressing)
-			case corev1.ConditionFalse:
-				depCondSet.Manage(s).MarkFalse(DeploymentConditionProgressing, cond.Reason, cond.Message)
+	conds := []appsv1.DeploymentConditionType{
+		appsv1.DeploymentProgressing,
+		appsv1.DeploymentReplicaFailure,
+	}
+
+	for _, wantType := range conds {
+		for _, cond := range ds.Conditions {
+			if wantType != cond.Type {
+				continue
 			}
-		case appsv1.DeploymentReplicaFailure:
-			switch cond.Status {
-			case corev1.ConditionUnknown:
-				depCondSet.Manage(s).MarkUnknown(DeploymentConditionReplicaSetReady, cond.Reason, cond.Message)
-			case corev1.ConditionTrue:
-				depCondSet.Manage(s).MarkFalse(DeploymentConditionReplicaSetReady, cond.Reason, cond.Message)
-			case corev1.ConditionFalse:
-				depCondSet.Manage(s).MarkTrue(DeploymentConditionReplicaSetReady)
+
+			switch cond.Type {
+			case appsv1.DeploymentProgressing:
+				switch cond.Status {
+				case corev1.ConditionUnknown:
+					depCondSet.Manage(s).MarkUnknown(DeploymentConditionProgressing, cond.Reason, cond.Message)
+				case corev1.ConditionTrue:
+					depCondSet.Manage(s).MarkTrue(DeploymentConditionProgressing)
+				case corev1.ConditionFalse:
+					depCondSet.Manage(s).MarkFalse(DeploymentConditionProgressing, cond.Reason, cond.Message)
+				}
+			case appsv1.DeploymentReplicaFailure:
+				switch cond.Status {
+				case corev1.ConditionUnknown:
+					depCondSet.Manage(s).MarkUnknown(DeploymentConditionReplicaSetReady, cond.Reason, cond.Message)
+				case corev1.ConditionTrue:
+					depCondSet.Manage(s).MarkFalse(DeploymentConditionReplicaSetReady, cond.Reason, cond.Message)
+				case corev1.ConditionFalse:
+					depCondSet.Manage(s).MarkTrue(DeploymentConditionReplicaSetReady)
+				}
 			}
+
 		}
 	}
+
 	return s
 }

pkg/apis/serving/v1/revision_helpers.go

@@ -19,7 +19,6 @@ package v1
 import (
 	"time"
 
-	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	net "knative.dev/networking/pkg/apis/networking"
 	"knative.dev/pkg/kmeta"
@@ -144,9 +143,3 @@ func (rs *RevisionStatus) IsActivationRequired() bool {
 	c := revisionCondSet.Manage(rs).GetCondition(RevisionConditionActive)
 	return c != nil && c.Status != corev1.ConditionTrue
 }
-
-// IsReplicaSetFailure returns true if the deployment replicaset failed to create
-func (rs *RevisionStatus) IsReplicaSetFailure(deploymentStatus *appsv1.DeploymentStatus) bool {
-	ds := serving.TransformDeploymentStatus(deploymentStatus)
-	return ds != nil && ds.GetCondition(serving.DeploymentConditionReplicaSetReady).IsFalse()
-}

pkg/apis/serving/v1/revision_helpers_test.go

@@ -20,7 +20,6 @@ import (
 	"testing"
 	"time"
 
-	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 
@@ -268,45 +267,3 @@ func TestSetRoutingState(t *testing.T) {
 		t.Error("Expected default value for unparsable annotationm but got:", got)
 	}
 }
-
-func TestIsReplicaSetFailure(t *testing.T) {
-	revisionStatus := RevisionStatus{}
-	cases := []struct {
-		name                string
-		status              appsv1.DeploymentStatus
-		IsReplicaSetFailure bool
-	}{{
-		name:   "empty deployment status should not be a failure",
-		status: appsv1.DeploymentStatus{},
-	}, {
-		name: "Ready deployment status should not be a failure",
-		status: appsv1.DeploymentStatus{
-			Conditions: []appsv1.DeploymentCondition{{
-				Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue,
-			}},
-		},
-	}, {
-		name: "ReplicasetFailure true should be a failure",
-		status: appsv1.DeploymentStatus{
-			Conditions: []appsv1.DeploymentCondition{{
-				Type: appsv1.DeploymentReplicaFailure, Status: corev1.ConditionTrue,
-			}},
-		},
-		IsReplicaSetFailure: true,
-	}, {
-		name: "ReplicasetFailure false should not be a failure",
-		status: appsv1.DeploymentStatus{
-			Conditions: []appsv1.DeploymentCondition{{
-				Type: appsv1.DeploymentReplicaFailure, Status: corev1.ConditionFalse,
-			}},
-		},
-	}}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			if got, want := revisionStatus.IsReplicaSetFailure(&tc.status), tc.IsReplicaSetFailure; got != want {
-				t.Errorf("IsReplicaSetFailure = %v, want: %v", got, want)
-			}
-		})
-	}
-}

pkg/apis/serving/v1/revision_lifecycle.go

@@ -197,6 +197,11 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA
 		rs.MarkContainerHealthyTrue()
 	}
 
+	// Mark resource unavailable if we don't have a Service Name and the deployment is ready
+	if rs.GetCondition(RevisionConditionResourcesAvailable).IsTrue() && ps.ServiceName == "" {
+		rs.MarkResourcesAvailableUnknown(ReasonDeploying, "")
+	}
+
 	switch cond.Status {
 	case corev1.ConditionUnknown:
 		rs.MarkActiveUnknown(cond.Reason, cond.Message)
@@ -222,14 +227,6 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA
 		rs.MarkActiveFalse(cond.Reason, cond.Message)
 	case corev1.ConditionTrue:
 		rs.MarkActiveTrue()
-
-		// Precondition for PA being active is SKS being active and
-		// that implies that |service.endpoints| > 0.
-		//
-		// Note: This is needed for backwards compatibility as we're adding the new
-		// ScaleTargetInitialized condition to gate readiness.
-		rs.MarkResourcesAvailableTrue()
-		rs.MarkContainerHealthyTrue()
 	}
 }
 

pkg/apis/serving/v1/revision_lifecycle_test.go

@@ -536,6 +536,7 @@ func TestPropagateAutoscalerStatus(t *testing.T) {
 
 	// PodAutoscaler becomes ready, making us active.
 	r.PropagateAutoscalerStatus(&autoscalingv1alpha1.PodAutoscalerStatus{
+		ServiceName: "some-service",
 		Status: duckv1.Status{
 			Conditions: duckv1.Conditions{{
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
@@ -551,6 +552,7 @@ func TestPropagateAutoscalerStatus(t *testing.T) {
 
 	// PodAutoscaler flipping back to Unknown causes Active become ongoing immediately.
 	r.PropagateAutoscalerStatus(&autoscalingv1alpha1.PodAutoscalerStatus{
+		ServiceName: "some-service",
 		Status: duckv1.Status{
 			Conditions: duckv1.Conditions{{
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
@@ -566,6 +568,7 @@ func TestPropagateAutoscalerStatus(t *testing.T) {
 
 	// PodAutoscaler becoming unready makes Active false, but doesn't affect readiness.
 	r.PropagateAutoscalerStatus(&autoscalingv1alpha1.PodAutoscalerStatus{
+		ServiceName: "some-service",
 		Status: duckv1.Status{
 			Conditions: duckv1.Conditions{{
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,
@@ -671,6 +674,7 @@ func TestPropagateAutoscalerStatusRace(t *testing.T) {
 
 	// The PodAutoscaler might have been ready but it's scaled down already.
 	r.PropagateAutoscalerStatus(&autoscalingv1alpha1.PodAutoscalerStatus{
+		ServiceName: "some-service",
 		Status: duckv1.Status{
 			Conditions: duckv1.Conditions{{
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionReady,

pkg/reconciler/autoscaling/kpa/kpa_test.go

@@ -221,7 +221,7 @@ func markScaleTargetInitialized(pa *autoscalingv1alpha1.PodAutoscaler) {
 
 func kpa(ns, n string, opts ...PodAutoscalerOption) *autoscalingv1alpha1.PodAutoscaler {
 	rev := newTestRevision(ns, n)
-	kpa := revisionresources.MakePA(rev)
+	kpa := revisionresources.MakePA(rev, nil)
 	kpa.Generation = 1
 	kpa.Annotations[autoscaling.ClassAnnotationKey] = "kpa.autoscaling.knative.dev"
 	kpa.Annotations[autoscaling.MetricAnnotationKey] = "concurrency"
@@ -1303,7 +1303,7 @@ func TestGlobalResyncOnUpdateAutoscalerConfigMap(t *testing.T) {
 	rev := newTestRevision(testNamespace, testRevision)
 	newDeployment(ctx, t, fakedynamicclient.Get(ctx), testRevision+"-deployment", 3)
 
-	kpa := revisionresources.MakePA(rev)
+	kpa := revisionresources.MakePA(rev, nil)
 	sks := aresources.MakeSKS(kpa, nv1a1.SKSOperationModeServe, minActivators)
 	sks.Status.PrivateServiceName = "bogus"
 	sks.Status.InitializeConditions()
@@ -1372,7 +1372,7 @@ func TestReconcileDeciderCreatesAndDeletes(t *testing.T) {
 
 	newDeployment(ctx, t, fakedynamicclient.Get(ctx), testRevision+"-deployment", 3)
 
-	kpa := revisionresources.MakePA(rev)
+	kpa := revisionresources.MakePA(rev, nil)
 	sks := sks(testNamespace, testRevision, WithDeployRef(kpa.Spec.ScaleTargetRef.Name), WithSKSReady)
 	fakenetworkingclient.Get(ctx).NetworkingV1alpha1().ServerlessServices(testNamespace).Create(ctx, sks, metav1.CreateOptions{})
 	fakeservingclient.Get(ctx).AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(ctx, kpa, metav1.CreateOptions{})
@@ -1446,7 +1446,7 @@ func TestUpdate(t *testing.T) {
 	fakekubeclient.Get(ctx).CoreV1().Pods(testNamespace).Create(ctx, pod, metav1.CreateOptions{})
 	fakefilteredpodsinformer.Get(ctx, serving.RevisionUID).Informer().GetIndexer().Add(pod)
 
-	kpa := revisionresources.MakePA(rev)
+	kpa := revisionresources.MakePA(rev, nil)
 	kpa.SetDefaults(context.Background())
 	fakeservingclient.Get(ctx).AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(ctx, kpa, metav1.CreateOptions{})
 	fakepainformer.Get(ctx).Informer().GetIndexer().Add(kpa)
@@ -1525,7 +1525,7 @@ func TestControllerCreateError(t *testing.T) {
 			createErr: want,
 		})
 
-	kpa := revisionresources.MakePA(newTestRevision(testNamespace, testRevision))
+	kpa := revisionresources.MakePA(newTestRevision(testNamespace, testRevision), nil)
 	fakeservingclient.Get(ctx).AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(ctx, kpa, metav1.CreateOptions{})
 	fakepainformer.Get(ctx).Informer().GetIndexer().Add(kpa)
 
@@ -1568,7 +1568,7 @@ func TestControllerUpdateError(t *testing.T) {
 			createErr: want,
 		})
 
-	kpa := revisionresources.MakePA(newTestRevision(testNamespace, testRevision))
+	kpa := revisionresources.MakePA(newTestRevision(testNamespace, testRevision), nil)
 	fakeservingclient.Get(ctx).AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(ctx, kpa, metav1.CreateOptions{})
 	fakepainformer.Get(ctx).Informer().GetIndexer().Add(kpa)
 
@@ -1610,7 +1610,7 @@ func TestControllerGetError(t *testing.T) {
 			getErr: want,
 		})
 
-	kpa := revisionresources.MakePA(newTestRevision(testNamespace, testRevision))
+	kpa := revisionresources.MakePA(newTestRevision(testNamespace, testRevision), nil)
 	fakeservingclient.Get(ctx).AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(ctx, kpa, metav1.CreateOptions{})
 	fakepainformer.Get(ctx).Informer().GetIndexer().Add(kpa)
 
@@ -1649,7 +1649,7 @@ func TestScaleFailure(t *testing.T) {
 
 	// Only put the KPA in the lister, which will prompt failures scaling it.
 	rev := newTestRevision(testNamespace, testRevision)
-	kpa := revisionresources.MakePA(rev)
+	kpa := revisionresources.MakePA(rev, nil)
 	fakepainformer.Get(ctx).Informer().GetIndexer().Add(kpa)
 
 	newDeployment(ctx, t, fakedynamicclient.Get(ctx), testRevision+"-deployment", 3)

pkg/reconciler/autoscaling/kpa/scaler_test.go

@@ -640,7 +640,7 @@ func TestDisableScaleToZero(t *testing.T) {
 
 func newKPA(ctx context.Context, t *testing.T, servingClient clientset.Interface, revision *v1.Revision) *autoscalingv1alpha1.PodAutoscaler {
 	t.Helper()
-	pa := revisionresources.MakePA(revision)
+	pa := revisionresources.MakePA(revision, nil)
 	pa.Status.InitializeConditions()
 	_, err := servingClient.AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(ctx, pa, metav1.CreateOptions{})
 	if err != nil {

pkg/reconciler/revision/cruds.go

@@ -116,6 +116,6 @@ func (c *Reconciler) createImageCache(ctx context.Context, rev *v1.Revision, con
 }
 
 func (c *Reconciler) createPA(ctx context.Context, rev *v1.Revision) (*autoscalingv1alpha1.PodAutoscaler, error) {
-	pa := resources.MakePA(rev)
+	pa := resources.MakePA(rev, nil)
 	return c.client.AutoscalingV1alpha1().PodAutoscalers(pa.Namespace).Create(ctx, pa, metav1.CreateOptions{})
 }

pkg/reconciler/revision/reconcile_resources.go

@@ -49,44 +49,31 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1.Revision)
 		// Deployment does not exist. Create it.
 		rev.Status.MarkResourcesAvailableUnknown(v1.ReasonDeploying, "")
 		rev.Status.MarkContainerHealthyUnknown(v1.ReasonDeploying, "")
-		deployment, err = c.createDeployment(ctx, rev)
-		if err != nil {
+
+		if _, err = c.createDeployment(ctx, rev); err != nil {
 			return fmt.Errorf("failed to create deployment %q: %w", deploymentName, err)
 		}
+
 		logger.Infof("Created deployment %q", deploymentName)
+		return nil
 	} else if err != nil {
 		return fmt.Errorf("failed to get deployment %q: %w", deploymentName, err)
 	} else if !metav1.IsControlledBy(deployment, rev) {
 		// Surface an error in the revision's status, and return an error.
 		rev.Status.MarkResourcesAvailableFalse(v1.ReasonNotOwned, v1.ResourceNotOwnedMessage("Deployment", deploymentName))
 		return fmt.Errorf("revision: %q does not own Deployment: %q", rev.Name, deploymentName)
-	} else {
-		// The deployment exists, but make sure that it has the shape that we expect.
-		deployment, err = c.checkAndUpdateDeployment(ctx, rev, deployment)
-		if err != nil {
-			return fmt.Errorf("failed to update deployment %q: %w", deploymentName, err)
-		}
-
-		// Now that we have a Deployment, determine whether there is any relevant
-		// status to surface in the Revision.
-		//
-		// TODO(jonjohnsonjr): Should we check Generation != ObservedGeneration?
-		// The autoscaler mutates the deployment pretty often, which would cause us
-		// to flip back and forth between Ready and Unknown every time we scale up
-		// or down.
-		if !rev.Status.IsActivationRequired() {
-			rev.Status.PropagateDeploymentStatus(&deployment.Status)
-		}
 	}
 
-	// If the replicaset is failing we assume its an error we have to surface
-	if rev.Status.IsReplicaSetFailure(&deployment.Status) {
-		rev.Status.PropagateDeploymentStatus(&deployment.Status)
-		return nil
+	// The deployment exists, but make sure that it has the shape that we expect.
+	deployment, err = c.checkAndUpdateDeployment(ctx, rev, deployment)
+	if err != nil {
+		return fmt.Errorf("failed to update deployment %q: %w", deploymentName, err)
 	}
 
+	rev.Status.PropagateDeploymentStatus(&deployment.Status)
+
 	// If a container keeps crashing (no active pods in the deployment although we want some)
-	if *deployment.Spec.Replicas > 0 && deployment.Status.AvailableReplicas == 0 {
+	if *deployment.Spec.Replicas != 0 && deployment.Status.AvailableReplicas == 0 {
 		pods, err := c.kubeclient.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{LabelSelector: metav1.FormatLabelSelector(deployment.Spec.Selector)})
 		if err != nil {
 			logger.Errorw("Error getting pods", zap.Error(err))
@@ -151,6 +138,10 @@ func (c *Reconciler) reconcileImageCache(ctx context.Context, rev *v1.Revision)
 
 func (c *Reconciler) reconcilePA(ctx context.Context, rev *v1.Revision) error {
 	ns := rev.Namespace
+
+	deploymentName := resourcenames.Deployment(rev)
+	deployment, _ := c.deploymentLister.Deployments(ns).Get(deploymentName)
+
 	paName := resourcenames.PA(rev)
 	logger := logging.FromContext(ctx)
 	logger.Info("Reconciling PA: ", paName)
@@ -173,7 +164,7 @@ func (c *Reconciler) reconcilePA(ctx context.Context, rev *v1.Revision) error {
 
 	// Perhaps tha PA spec changed underneath ourselves?
 	// We no longer require immutability, so need to reconcile PA each time.
-	tmpl := resources.MakePA(rev)
+	tmpl := resources.MakePA(rev, deployment)
 	logger.Debugf("Desired PASpec: %#v", tmpl.Spec)
 	if !equality.Semantic.DeepEqual(tmpl.Spec, pa.Spec) {
 		diff, _ := kmp.SafeDiff(tmpl.Spec, pa.Spec) // Can't realistically fail on PASpec.