Add default conditions to create PA to avoid potential race conditions (knative#16078)

between revision and autoscaler

fix revision lifecyle check for conditions

Author: nader-ziada

config/core/300-resources/podautoscaler.yaml

@@ -125,9 +125,32 @@ spec:
             status:
               description: Status communicates the observed state of the PodAutoscaler (from the controller).
               type: object
-              required:
-                - metricsServiceName
-                - serviceName
+              default:
+                conditions:
+                  - lastTransitionTime: "1970-01-01T00:00:00Z"
+                    message: Waiting for controller
+                    reason: Pending
+                    severity: ""
+                    status: Unknown
+                    type: Active
+                  - lastTransitionTime: "1970-01-01T00:00:00Z"
+                    message: Waiting for controller
+                    reason: Pending
+                    severity: ""
+                    status: Unknown
+                    type: Ready
+                  - lastTransitionTime: "1970-01-01T00:00:00Z"
+                    message: Waiting for controller
+                    reason: Pending
+                    severity: ""
+                    status: Unknown
+                    type: SKSReady
+                  - lastTransitionTime: "1970-01-01T00:00:00Z"
+                    message: Waiting for controller
+                    reason: Pending
+                    severity: ""
+                    status: Unknown
+                    type: ScaleTargetInitialized
               properties:
                 actualScale:
                   description: ActualScale shows the actual number of replicas for the revision.

docs/serving-api.md

@@ -459,6 +459,7 @@ string
 </em>
 </td>
 <td>
+<em>(Optional)</em>
 <p>ServiceName is the K8s Service name that serves the revision, scaled by this PA.
 The service is created and owned by the ServerlessService object owned by this PA.</p>
 </td>
@@ -471,6 +472,7 @@ string
 </em>
 </td>
 <td>
+<em>(Optional)</em>
 <p>MetricsServiceName is the K8s Service name that provides revision metrics.
 The service is managed by the PA object.</p>
 </td>

pkg/apis/autoscaling/v1alpha1/pa_types.go

@@ -112,14 +112,17 @@ const (
 
 // PodAutoscalerStatus communicates the observed state of the PodAutoscaler (from the controller).
 type PodAutoscalerStatus struct {
+	// +kubebuilder:default={"conditions": {{"type":"Active", "status": "Unknown", "reason": "Pending",  "message": "Waiting for controller", "severity": "", "lastTransitionTime": "1970-01-01T00:00:00Z"}, {"type":"Ready", "status": "Unknown", "reason": "Pending", "message": "Waiting for controller", "severity": "", "lastTransitionTime": "1970-01-01T00:00:00Z"}, {"type":"SKSReady", "status": "Unknown", "reason": "Pending", "message": "Waiting for controller", "severity": "", "lastTransitionTime": "1970-01-01T00:00:00Z"}, {"type":"ScaleTargetInitialized", "status": "Unknown", "reason": "Pending", "message": "Waiting for controller", "severity": "", "lastTransitionTime": "1970-01-01T00:00:00Z"}}}
 	duckv1.Status `json:",inline"`
 
 	// ServiceName is the K8s Service name that serves the revision, scaled by this PA.
 	// The service is created and owned by the ServerlessService object owned by this PA.
+	// +optional
 	ServiceName string `json:"serviceName"`
 
 	// MetricsServiceName is the K8s Service name that provides revision metrics.
 	// The service is managed by the PA object.
+	// +optional
 	MetricsServiceName string `json:"metricsServiceName"`
 
 	// DesiredScale shows the current desired number of replicas for the revision.

pkg/apis/serving/v1/revision_lifecycle.go

@@ -184,7 +184,8 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA
 		rs.DesiredReplicas = ps.DesiredScale
 	}
 
-	if cond == nil {
+	sksCondition := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionSKSReady)
+	if cond == nil || (cond.IsUnknown() && sksCondition != nil && sksCondition.IsUnknown()) {
 		rs.MarkActiveUnknown(ReasonDeploying, "")
 
 		if !resUnavailable {
@@ -217,6 +218,7 @@ func (rs *RevisionStatus) PropagateAutoscalerStatus(ps *autoscalingv1alpha1.PodA
 		// 2. Initial scale was never achieved, which means we failed to progress
 		//    towards initial scale during the progress deadline period and scaled to 0
 		//		failing to activate.
+		// 3. Need to make sure to wait for SKS to be ready, or at least fail.
 		// So mark the revision as failed at that point.
 		// See #8922 for details. When we try to scale to 0, we force the Deployment's
 		// Progress status to become `true`, since successful scale down means
@@ -226,7 +228,7 @@ 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 != "" {
+		if !ps.IsScaleTargetInitialized() && !resUnavailable && ps.ServiceName != "" && !sksCondition.IsUnknown() {
 			rs.MarkResourcesAvailableFalse(ReasonProgressDeadlineExceeded,
 				"Initial scale was never achieved")
 		}

pkg/apis/serving/v1/revision_lifecycle_test.go

@@ -640,6 +640,9 @@ func TestPropagateAutoscalerStatusNoProgress(t *testing.T) {
 			}, {
 				Type:   autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized,
 				Status: corev1.ConditionUnknown,
+			}, {
+				Type:   autoscalingv1alpha1.PodAutoscalerConditionSKSReady,
+				Status: corev1.ConditionFalse,
 			}},
 		},
 	})
@@ -726,6 +729,7 @@ func TestPropagateAutoscalerStatusReplicas(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			tc.ps.InitializeConditions()
 			r.PropagateAutoscalerStatus(&tc.ps)
 
 			if !cmp.Equal(tc.wantActualReplicas, r.ActualReplicas) {

pkg/reconciler/revision/table_test.go

@@ -312,6 +312,7 @@ func TestReconcile(t *testing.T) {
 				WithPAStatusService("pa-inactive"),
 				WithNoTraffic("NoTraffic", "This thing is inactive."),
 				WithScaleTargetInitialized,
+				WithPASKSNotReady(""),
 				WithReachabilityUnreachable),
 			readyDeploy(deploy(t, "foo", "pa-inactive")),
 			image("foo", "pa-inactive"),
@@ -335,13 +336,14 @@ func TestReconcile(t *testing.T) {
 				WithRevisionObservedGeneration(1)),
 			pa("foo", "pa-inactive",
 				WithNoTraffic("NoTraffic", "This thing is inactive."),
-				WithPAStatusService("pa-inactive")),
+				WithPAStatusService("pa-inactive"),
+				WithPASKSReady),
 			readyDeploy(deploy(t, "foo", "pa-inactive")),
 			image("foo", "pa-inactive"),
 		},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "pa-inactive",
-				WithLogURL, withDefaultContainerStatuses(), MarkDeploying(""),
+				WithLogURL, withDefaultContainerStatuses(), MarkContainerHealthyUnknown(""),
 				// 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),

pkg/reconciler/testing/v1/factory.go

@@ -25,6 +25,7 @@ import (
 	fakenetworkingclient "knative.dev/networking/pkg/client/injection/client/fake"
 	fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
 	fakedynamicclient "knative.dev/pkg/injection/clients/dynamicclient/fake"
+	autoscalingv1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	fakeservingclient "knative.dev/serving/pkg/client/injection/client/fake"
 
@@ -95,6 +96,28 @@ func MakeFactory(ctor Ctor) rtesting.Factory {
 				return false, nil, nil
 			},
 		)
+
+		client.PrependReactor("create", "podautoscalers", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
+			if createAction, ok := action.(ktesting.CreateAction); ok {
+				if pa, ok := createAction.GetObject().(*autoscalingv1alpha1.PodAutoscaler); ok {
+					// Initialize conditions first
+					pa.Status.InitializeConditions()
+
+					// Set all conditions to match kubebuilder defaults: status="Unknown", reason="Deploying"
+					// This matches the behavior defined in the +kubebuilder:default annotation
+					condSet := pa.GetConditionSet()
+					manager := condSet.Manage(&pa.Status)
+
+					// Set each condition to Unknown with "Deploying" reason to match kubebuilder defaults
+					manager.MarkUnknown(autoscalingv1alpha1.PodAutoscalerConditionActive, "Pending", "Waiting for controller")
+					manager.MarkUnknown(autoscalingv1alpha1.PodAutoscalerConditionReady, "Pending", "Waiting for controller")
+					manager.MarkUnknown(autoscalingv1alpha1.PodAutoscalerConditionSKSReady, "Pending", "Waiting for controller")
+					manager.MarkUnknown(autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized, "Pending", "Waiting for controller")
+				}
+			}
+			return false, nil, nil
+		})
+
 		// This is needed by the Configuration controller tests, which
 		// use GenerateName to produce Revisions.
 		rtesting.PrependGenerateNameReactor(&client.Fake)