add default conditions to PA to avoid potential race conditions (2nd attempt) #16078

nader-ziada · 2025-09-10T17:53:51Z

add default conditions to PA to avoid potential race conditions
between revision and autoscaler

Proposed Changes

Add default conditions in PodAutoscaler to avoid any potential race conditions

codecov · 2025-09-10T17:59:07Z

Codecov Report

❌ Patch coverage is 18.18182% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.06%. Comparing base (0902825) to head (1bf2b31).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/reconciler/testing/v1/factory.go	0.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16078      +/-   ##
==========================================
- Coverage   80.13%   80.06%   -0.07%     
==========================================
  Files         214      214              
  Lines       16887    16907      +20     
==========================================
+ Hits        13532    13537       +5     
- Misses       2996     3011      +15     
  Partials      359      359

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nader-ziada · 2025-09-11T02:18:41Z

/retest

nader-ziada · 2025-09-11T11:50:01Z

/test all

nader-ziada · 2025-09-11T17:48:42Z

/test all

nader-ziada · 2025-09-11T19:28:25Z

/retest

nader-ziada · 2025-09-12T03:47:57Z

/retest

nader-ziada · 2025-09-12T11:50:20Z

/test all

nader-ziada · 2025-09-12T14:43:19Z

@dprotaso can you take a look, I think I fixed the issue. ran the tests multiple times to check if its flaky

dprotaso · 2025-09-12T15:19:52Z

@nader-ziada what was the issue?

nader-ziada · 2025-09-12T15:26:38Z

the code was not expecting a condition, so assumed it failed even though it was the pending one, check the 2nd commit

dprotaso · 2025-09-15T15:11:25Z

pkg/apis/serving/v1/revision_lifecycle.go

+		scaleTargetInitializedCondReason := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized).GetReason()
+		if !ps.IsScaleTargetInitialized() && !resUnavailable && ps.ServiceName != "" && scaleTargetInitializedCondReason != autoscalingv1alpha1.PendingReason {


Can you clarify what is happening here? Generally we're not looking at the Reason string but Status.

To get here PA::Ready=False are we trying to set the right error message?

Another way to think about this can we just look at ScaleTargetInitialized=False to simplify this? Technically not IsScaleTargetInitialized is true when it is set to Unknown and False

I think the new situation here is that the condition is now there on the creation of the resource with the generated defaulting, so I wasn't sure how else to differentiate between the default Unknown (with reason Pending) vs Unknown because something happened.

default Unknown (with reason Pending) vs Unknown because something happened.

I would view these two scenarios as being the same

dprotaso · 2025-09-15T15:17:17Z

pkg/reconciler/autoscaling/kpa/kpa.go

+	cond := pa.Status.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionReady)
+	if cond.IsUnknown() && cond.GetReason() == autoscalingv1alpha1.PendingReason {
+		// still at default PA condition, no need to mark anything
+		return
+	}


Can you comment why we added this? It's not clear why we want to gate setting the Active condition on Ready all of a sudden

I'm basically trying to skip the check if its the default condition with Reason = Pending that is now set by default when the resource is created, which is different that just Unknown that would cause the PA to be set to Inactive

which is different that just Unknown that would cause the PA to be set to Inactive

Unknown means it's deploying or doing whatever. The reason doesn't really matter. If we were setting the status to Inactive while it was unknown then I'd probably continue doing that.

Was this triggering some failures?

yes, it was setting the PA ready to false which made the revision get into the "Initial scale was never achieved" error

here: https://github.com/knative/serving/blob/main/pkg/apis/serving/v1/revision_lifecycle.go#L214

nader-ziada · 2025-09-17T12:35:33Z

/retest

nader-ziada · 2025-09-17T15:05:21Z

/test all

nader-ziada · 2025-09-17T17:00:46Z

/retest

nader-ziada · 2025-09-17T19:35:39Z

/test all

nader-ziada · 2025-09-26T01:54:08Z

/retest

nader-ziada · 2025-09-26T12:10:07Z

/test all

nader-ziada · 2025-09-26T14:30:40Z

/retest

nader-ziada · 2025-09-26T16:03:50Z

/test istio-latest-no-mesh

nader-ziada · 2025-09-26T17:26:13Z

@dprotaso @dsimansk i think this is ready for review now, passed the last 4 runs of tests

dprotaso · 2025-09-30T20:52:27Z

pkg/reconciler/revision/table_test.go

+func withRevisionConditionsGivenPADefault(r *v1.Revision) {
+	WithInitRevConditions(r)
+	r.Status.MarkActiveUnknown("Deploying", "")
+}
+


This looks identical to allUnknownConditions can we clean up the extra function and simplify the diff?

https://github.com/nader-ziada/serving/blob/f1341d21581b613facc929cb3ed0bad197eafd53/pkg/reconciler/revision/table_test.go#L905-L909

yeah, will remove that

dprotaso · 2025-09-30T20:54:55Z

pkg/apis/serving/v1/revision_lifecycle.go


-	if cond == nil {
+	sksCondition := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionSKSReady)
+	if cond == nil || (cond.IsUnknown() && sksCondition != nil && sksCondition.IsUnknown()) {


Can you explain why we need the extra clauses?

I would think we could simply have cond == nil || cond.IsUnknown()

I believe that there were cases now that the pa has conditions at the start, it was not waiting for the service to get ready, so I was trying to make sure to simulate the previous cases before defaulting pa conditions

dprotaso · 2025-09-30T20:56:16Z

pkg/apis/serving/v1/revision_lifecycle.go

 		// 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() {


Can you comment on why we want to add an additional check here?

Probably worth updating the comment as well

same as other comment, I believe that there were cases now that the pa has conditions at the start, it was not waiting for the service to get ready, so I was trying to make sure to simulate the previous cases before defaulting pa conditions

between revision and autoscaler fix revision lifecyle check for conditions

nader-ziada · 2025-09-30T22:19:49Z

/retest

nader-ziada · 2025-10-01T18:27:23Z

@dprotaso added comments

dprotaso · 2025-10-02T16:06:44Z

/lgtm
/approve

knative-prow · 2025-10-02T16:06:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nader-ziada

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative#16078) between revision and autoscaler fix revision lifecyle check for conditions

#1598) * Add default conditions to create PA to avoid potential race conditions (knative#16078) between revision and autoscaler fix revision lifecyle check for conditions * Regen release files --------- Co-authored-by: Nader Ziada <[email protected]>

…onditions (knative#16078)" This reverts commit 561f348.

…onditions (#16078)" (#16118) This reverts commit 561f348.

knative-prow bot requested review from dsimansk and skonto September 10, 2025 17:53

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 10, 2025

nader-ziada changed the title ~~add initialize conditions to MakePA to avoid potential race conditions (2nd attempt)~~ add default conditions to PA to avoid potential race conditions (2nd attempt) Sep 10, 2025

nader-ziada force-pushed the pa-race branch 2 times, most recently from f9d8138 to cc8c4c2 Compare September 12, 2025 01:40

nader-ziada force-pushed the pa-race branch from cc8c4c2 to 9351ff9 Compare September 12, 2025 13:13

dprotaso reviewed Sep 15, 2025

View reviewed changes

nader-ziada changed the title ~~add default conditions to PA to avoid potential race conditions (2nd attempt)~~ [wip] add default conditions to PA to avoid potential race conditions (2nd attempt) Sep 15, 2025

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2025

This was referenced Sep 15, 2025

add initialize conditions to MakePA to avoid potential race conditions #16037

Merged

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

Merged

nader-ziada force-pushed the pa-race branch 3 times, most recently from c09b0d0 to d390044 Compare September 16, 2025 22:33

nader-ziada force-pushed the pa-race branch from d390044 to 88de82b Compare September 17, 2025 13:46

nader-ziada force-pushed the pa-race branch from 6b815f7 to 2474400 Compare September 25, 2025 22:26

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2025

nader-ziada force-pushed the pa-race branch from 2474400 to 674c400 Compare September 26, 2025 13:49

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2025

nader-ziada changed the title ~~[wip] add default conditions to PA to avoid potential race conditions (2nd attempt)~~ add default conditions to PA to avoid potential race conditions (2nd attempt) Sep 26, 2025

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2025

nader-ziada force-pushed the pa-race branch from 674c400 to f1341d2 Compare September 29, 2025 16:24

dprotaso reviewed Sep 30, 2025

View reviewed changes

Add default conditions to create PA to avoid potential race conditions

1bf2b31

between revision and autoscaler fix revision lifecyle check for conditions

nader-ziada force-pushed the pa-race branch from f1341d2 to 1bf2b31 Compare September 30, 2025 21:36

knative-prow bot assigned dprotaso Oct 2, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2025

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2025

knative-prow bot merged commit 561f348 into knative:main Oct 2, 2025
111 of 113 checks passed

dsimansk pushed a commit to dsimansk/serving that referenced this pull request Oct 3, 2025

Add default conditions to create PA to avoid potential race conditions (

82e78ef

knative#16078) between revision and autoscaler fix revision lifecyle check for conditions

dprotaso added a commit to dprotaso/serving that referenced this pull request Oct 6, 2025

Revert "Add default conditions to create PA to avoid potential race c…

3200fd8

…onditions (knative#16078)" This reverts commit 561f348.

dprotaso mentioned this pull request Oct 6, 2025

Revert "Add default conditions to create PA to avoid potential race conditions (#16078)" #16118

Merged

knative-prow bot pushed a commit that referenced this pull request Oct 7, 2025

Revert "Add default conditions to create PA to avoid potential race c…

84893c2

…onditions (#16078)" (#16118) This reverts commit 561f348.

		scaleTargetInitializedCondReason := ps.GetCondition(autoscalingv1alpha1.PodAutoscalerConditionScaleTargetInitialized).GetReason()
		if !ps.IsScaleTargetInitialized() && !resUnavailable && ps.ServiceName != "" && scaleTargetInitializedCondReason != autoscalingv1alpha1.PendingReason {

add default conditions to PA to avoid potential race conditions (2nd attempt) #16078

add default conditions to PA to avoid potential race conditions (2nd attempt) #16078

Uh oh!

Conversation

nader-ziada commented Sep 10, 2025

Uh oh!

codecov bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nader-ziada commented Sep 11, 2025

Uh oh!

nader-ziada commented Sep 11, 2025

Uh oh!

nader-ziada commented Sep 11, 2025

Uh oh!

nader-ziada commented Sep 11, 2025

Uh oh!

nader-ziada commented Sep 12, 2025

Uh oh!

nader-ziada commented Sep 12, 2025

Uh oh!

nader-ziada commented Sep 12, 2025

Uh oh!

dprotaso commented Sep 12, 2025

Uh oh!

nader-ziada commented Sep 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nader-ziada commented Sep 17, 2025

Uh oh!

nader-ziada commented Sep 17, 2025

Uh oh!

nader-ziada commented Sep 17, 2025

Uh oh!

nader-ziada commented Sep 17, 2025

Uh oh!

nader-ziada commented Sep 26, 2025

Uh oh!

nader-ziada commented Sep 26, 2025

Uh oh!

nader-ziada commented Sep 26, 2025

Uh oh!

nader-ziada commented Sep 26, 2025

Uh oh!

nader-ziada commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nader-ziada commented Sep 30, 2025

Uh oh!

nader-ziada commented Oct 1, 2025

Uh oh!

dprotaso commented Oct 2, 2025

Uh oh!

codecov bot commented Sep 10, 2025 •

edited

Loading