Prevent a PodAutoscaler's DesiredScale to turn to -1 #14866

SaschaSchwarze0 · 2024-02-05T10:42:56Z

Proposed Changes

This changed the Knative PodAutoscaler logic to never change the DesiredScale from a non-negative to a negative value.
This happens when leading AutoScaler restarted and it has no metrics. Moments later once it has metrics, it will change it back.
While this has no functional impact, it causes such a change for every revision which leads to four Kubernetes API calls (update PodAutoscaler in autoscaler, update Revision in controller, and then again PodAutoscaler in autoscaler, update Revision in controller) which has massive impact as QPS slows down overall progression inside these two controllers.

Release Note

The autoscaler now keeps the desiredScale of a PodAutoscaler at its current value while it initializes and therefore has not yet metrics

knative-prow · 2024-02-05T10:43:06Z

Hi @SaschaSchwarze0. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/reconciler/autoscaling/kpa/kpa.go

skonto · 2024-02-05T12:12:54Z

/ok-to-test

cc @dprotaso @psschwei

skonto · 2024-02-05T13:08:17Z

It seems that the failing test Name: "sks does not exist", expects to set pa.DesiredScale to -1 instead of the original value (11).

SaschaSchwarze0 · 2024-02-05T13:54:26Z

It seems that the failing test Name: "sks does not exist", expects to set pa.DesiredScale to -1 instead of the original value (11).

Yep, now that I don't have this change guarded by a flag anymore, I have to look at the results of all unit tests. Will check.

SaschaSchwarze0 · 2024-02-06T07:50:44Z

I changed the test case. It was actually running into that code path where there are no metrics and now that means that it keeps the desiredScale rather than setting it to -1.

SaschaSchwarze0 · 2024-02-07T14:15:08Z

/easycla

codecov · 2024-02-07T14:51:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.05%. Comparing base (17df219) to head (87a5c84).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14866      +/-   ##
==========================================
+ Coverage   84.02%   84.05%   +0.03%     
==========================================
  Files         213      213              
  Lines       16796    16803       +7     
==========================================
+ Hits        14112    14124      +12     
+ Misses       2329     2323       -6     
- Partials      355      356       +1

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

pkg/reconciler/autoscaling/kpa/kpa.go

psschwei

/lgtm

dprotaso · 2024-02-15T18:17:53Z

I've been reviewing #14607 & #14573 which address bugs in the same part of the codebase as this PR.

Those PRs fix the problem but they would introduce regressions if merged as is - so it's not solved yet. I'm creating e2e tests to catch these regressions - it would be good to run them against this PR when they land.

It sorta feels like there might a 'holistic' solution for those PRs where we want to distinguish "I have no data for this revision" and "I have no data for this revision and I haven't tried to sample any"

The latter scenario sounds seems to match what this PR is trying to fix.

I'm going to hold this PR for now until I wrap up the other reviews and the e2e tests

/hold

SaschaSchwarze0 · 2024-03-19T14:20:33Z

@dprotaso can you give an update where we are standing here ?

dprotaso · 2024-03-26T20:51:34Z

pkg/reconciler/autoscaling/kpa/kpa.go

+	// later back to the correct value. The following condition omits this.
+	if pc.want > -1 || pa.Status.DesiredScale == nil || *pa.Status.DesiredScale < 0 {
+		pa.Status.DesiredScale = ptr.Int32(int32(pc.want))
+	} else {
+		logger.Debugf("Ignoring change of desiredScale from %d to %d", *pa.Status.DesiredScale, pc.want)
+	}


Any thoughts on flipping the condition and the blocks around?

I found it easier to reason about this code when I realized the else condition is

pc.want == -1 && pa.Status.DesiredScale != nil && pa.Status.DesiredScale >= 0

I understand it both ways. 8-) Changed it.

dprotaso · 2024-03-26T20:54:11Z

pkg/reconciler/autoscaling/kpa/kpa.go

@@ -219,7 +219,15 @@ func (c *Reconciler) reconcileDecider(ctx context.Context, pa *autoscalingv1alph
 }

 func computeStatus(ctx context.Context, pa *autoscalingv1alpha1.PodAutoscaler, pc podCounts, logger *zap.SugaredLogger) {
-	pa.Status.DesiredScale, pa.Status.ActualScale = ptr.Int32(int32(pc.want)), ptr.Int32(int32(pc.ready))
+	pa.Status.ActualScale = ptr.Int32(int32(pc.ready))


I was wondering why ActualScale is different then I saw we are deriving this number from querying the informer cache which is sync'd at startup

Correct, that value is never incorrect.

dprotaso · 2024-03-27T20:00:08Z

/hold cancel
/lgtm

dprotaso · 2024-03-27T20:00:21Z

/approve

knative-prow · 2024-03-27T20:00:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, SaschaSchwarze0

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

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

dprotaso · 2024-03-27T21:07:08Z

linter failed - I just updated the PR to use the right ptr package

/lgtm

dprotaso · 2024-03-27T21:53:54Z

kourier-tls flakes - #15052
/retest

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2024

knative-prow bot added the area/API API objects and controllers label Feb 5, 2024

knative-prow bot requested review from KauzClay and ReToCode February 5, 2024 10:43

knative-prow bot added the area/autoscale label Feb 5, 2024

skonto reviewed Feb 5, 2024

View reviewed changes

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 5, 2024

SaschaSchwarze0 force-pushed the sascha-14669-ignore-pa-desiredscale-change-to-negative branch from e3f5c11 to e3d76a8 Compare February 6, 2024 07:44

psschwei reviewed Feb 7, 2024

View reviewed changes

pkg/reconciler/autoscaling/kpa/kpa.go Show resolved Hide resolved

psschwei reviewed Feb 8, 2024

View reviewed changes

knative-prow bot assigned psschwei Feb 8, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2024

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2024

dprotaso reviewed Mar 26, 2024

View reviewed changes

Prevent a PodAutoscaler's DesiredScale to turn to -1

d08cde0

SaschaSchwarze0 force-pushed the sascha-14669-ignore-pa-desiredscale-change-to-negative branch from e3d76a8 to d08cde0 Compare March 27, 2024 16:17

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024

SaschaSchwarze0 requested a review from dprotaso March 27, 2024 16:17

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024

knative-prow bot assigned dprotaso Mar 27, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2024

use correct ptr package

87a5c84

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024

knative-prow bot merged commit 3e2243d into knative:main Mar 27, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent a PodAutoscaler's DesiredScale to turn to -1 #14866

Prevent a PodAutoscaler's DesiredScale to turn to -1 #14866

SaschaSchwarze0 commented Feb 5, 2024

knative-prow bot commented Feb 5, 2024

skonto commented Feb 5, 2024 •

edited

Loading

skonto commented Feb 5, 2024 •

edited

Loading

SaschaSchwarze0 commented Feb 5, 2024

SaschaSchwarze0 commented Feb 6, 2024

SaschaSchwarze0 commented Feb 7, 2024

codecov bot commented Feb 7, 2024 •

edited

Loading

psschwei left a comment

dprotaso commented Feb 15, 2024

SaschaSchwarze0 commented Mar 19, 2024

dprotaso Mar 26, 2024

SaschaSchwarze0 Mar 27, 2024

dprotaso Mar 26, 2024 •

edited

Loading

SaschaSchwarze0 Mar 27, 2024

dprotaso commented Mar 27, 2024

dprotaso commented Mar 27, 2024

knative-prow bot commented Mar 27, 2024

dprotaso commented Mar 27, 2024

dprotaso commented Mar 27, 2024 •

edited

Loading

Prevent a PodAutoscaler's DesiredScale to turn to -1 #14866

Prevent a PodAutoscaler's DesiredScale to turn to -1 #14866

Conversation

SaschaSchwarze0 commented Feb 5, 2024

Proposed Changes

knative-prow bot commented Feb 5, 2024

skonto commented Feb 5, 2024 • edited Loading

skonto commented Feb 5, 2024 • edited Loading

SaschaSchwarze0 commented Feb 5, 2024

SaschaSchwarze0 commented Feb 6, 2024

SaschaSchwarze0 commented Feb 7, 2024

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

psschwei left a comment

Choose a reason for hiding this comment

dprotaso commented Feb 15, 2024

SaschaSchwarze0 commented Mar 19, 2024

dprotaso Mar 26, 2024

Choose a reason for hiding this comment

SaschaSchwarze0 Mar 27, 2024

Choose a reason for hiding this comment

dprotaso Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

SaschaSchwarze0 Mar 27, 2024

Choose a reason for hiding this comment

dprotaso commented Mar 27, 2024

dprotaso commented Mar 27, 2024

knative-prow bot commented Mar 27, 2024

dprotaso commented Mar 27, 2024

dprotaso commented Mar 27, 2024 • edited Loading

skonto commented Feb 5, 2024 •

edited

Loading

skonto commented Feb 5, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

dprotaso Mar 26, 2024 •

edited

Loading

dprotaso commented Mar 27, 2024 •

edited

Loading