Revert "add initialize conditions to MakePA to avoid potential race conditions (#16037)" (#16075)

This reverts commit 12777f6.

Author: dprotaso

config/core/300-resources/podautoscaler.yaml

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

docs/serving-api.md

@@ -459,7 +459,6 @@ 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>
@@ -472,7 +471,6 @@ 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,17 +112,14 @@ 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", "lastTransitionTime": "1970-01-01T00:00:00Z"}, {"type":"Ready", "status": "Unknown", "reason": "Pending", "message": "Waiting for controller", "lastTransitionTime": "1970-01-01T00:00:00Z"}, {"type":"SKSReady", "status": "Unknown", "reason": "Pending", "message": "Waiting for controller", "lastTransitionTime": "1970-01-01T00:00:00Z"}, {"type":"ScaleTargetInitialized", "status": "Unknown", "reason": "Pending", "message": "Waiting for controller", "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_test.go

@@ -726,7 +726,6 @@ 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

@@ -92,7 +92,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "first-reconcile",
 				// The first reconciliation Populates the following status properties.
-				WithLogURL, withRevisionConditionsGivenPADefault, MarkDeploying("Deploying"),
+				WithLogURL, allUnknownConditions, MarkDeploying("Deploying"),
 				withDefaultContainerStatuses(), WithRevisionObservedGeneration(1)),
 		}},
 		Key: "foo/first-reconcile",
@@ -256,7 +256,7 @@ func TestReconcile(t *testing.T) {
 		Name: "pa is ready",
 		Objects: []runtime.Object{
 			Revision("foo", "pa-ready",
-				WithLogURL, withRevisionConditionsGivenPADefault),
+				WithLogURL, allUnknownConditions),
 			pa("foo", "pa-ready", WithPASKSReady, WithTraffic,
 				WithScaleTargetInitialized, WithPAStatusService("new-stuff"), WithReachabilityUnknown),
 			deploy(t, "foo", "pa-ready"),
@@ -340,7 +340,7 @@ func TestReconcile(t *testing.T) {
 		},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "pa-inactive",
-				WithLogURL, withDefaultContainerStatuses(), MarkContainerHealthyUnknown(""),
+				WithLogURL, withDefaultContainerStatuses(), MarkDeploying(""),
 				// 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),
@@ -351,7 +351,7 @@ func TestReconcile(t *testing.T) {
 	}, {
 		Name: "pa is not ready with initial scale zero, but ServiceName still empty, so not marking resources available false",
 		Objects: []runtime.Object{
-			Revision("foo", "pa-inactive", withRevisionConditionsGivenPADefault,
+			Revision("foo", "pa-inactive", allUnknownConditions,
 				WithLogURL,
 				MarkDeploying(v1.ReasonDeploying),
 				WithRevisionObservedGeneration(1)),
@@ -363,7 +363,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			// We should not mark resources unavailable if ServiceName is empty
 			Object: Revision("foo", "pa-inactive",
-				WithLogURL, withDefaultContainerStatuses(), withRevisionConditionsGivenPADefault,
+				WithLogURL, withDefaultContainerStatuses(), allUnknownConditions,
 				MarkInactive("NoTraffic", "This thing is inactive."),
 				MarkDeploying(v1.ReasonDeploying),
 				WithRevisionObservedGeneration(1)),
@@ -403,7 +403,7 @@ func TestReconcile(t *testing.T) {
 		// Protocol type is the only thing that can be changed on PA
 		Objects: []runtime.Object{
 			Revision("foo", "fix-mutated-pa",
-				withRevisionConditionsGivenPADefault,
+				allUnknownConditions,
 				WithLogURL, MarkRevisionReady,
 				WithRoutingState(v1.RoutingStateActive, fc)),
 			pa("foo", "fix-mutated-pa", WithProtocolType(networking.ProtocolH2C),
@@ -414,7 +414,7 @@ func TestReconcile(t *testing.T) {
 		},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "fix-mutated-pa",
-				WithLogURL, withRevisionConditionsGivenPADefault,
+				WithLogURL, allUnknownConditions,
 				// When our reconciliation has to change the service
 				// we should see the following mutations to status.
 
@@ -693,7 +693,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "image-pull-secrets",
 				WithImagePullSecrets("foo-secret"),
-				WithLogURL, withRevisionConditionsGivenPADefault, MarkDeploying("Deploying"), withDefaultContainerStatuses(), WithRevisionObservedGeneration(1)),
+				WithLogURL, allUnknownConditions, MarkDeploying("Deploying"), withDefaultContainerStatuses(), WithRevisionObservedGeneration(1)),
 		}},
 		Key: "foo/image-pull-secrets",
 	}, {
@@ -714,7 +714,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "first-reconcile", WithRevisionInitContainers(),
 				// The first reconciliation Populates the following status properties.
-				WithLogURL, withRevisionConditionsGivenPADefault, MarkDeploying("Deploying"),
+				WithLogURL, allUnknownConditions, MarkDeploying("Deploying"),
 				withDefaultContainerStatuses(), withInitContainerStatuses(), WithRevisionObservedGeneration(1)),
 		}},
 		Key: "foo/first-reconcile",
@@ -736,7 +736,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: Revision("foo", "first-reconcile",
 				// The first reconciliation Populates the following status properties.
-				WithLogURL, withRevisionConditionsGivenPADefault, MarkDeploying("Deploying"),
+				WithLogURL, allUnknownConditions, MarkDeploying("Deploying"),
 				withDefaultContainerStatuses(), WithRevisionObservedGeneration(1), WithRevisionPVC()),
 		}},
 		Key: "foo/first-reconcile",
@@ -905,15 +905,6 @@ func allUnknownConditions(r *v1.Revision) {
 	MarkActivating("Deploying", "")(r)
 }
 
-// Revision Unknown conditions but with the generated default PA conditions
-// taken into consideration
-func withRevisionConditionsGivenPADefault(r *v1.Revision) {
-	WithInitRevConditions(r)
-	r.Status.MarkActiveUnknown("Pending", "Waiting for controller")
-	r.Status.MarkResourcesAvailableUnknown("Pending", "Waiting for controller")
-	r.Status.MarkContainerHealthyUnknown("Pending", "Waiting for controller")
-}
-
 type configOption func(*config.Config)
 
 type deploymentOption func(*appsv1.Deployment)

pkg/reconciler/testing/v1/factory.go

@@ -25,7 +25,6 @@ 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"
 
@@ -96,28 +95,6 @@ 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)