Change default target burst capacity #12774
Conversation
More discussion in: knative#11926 knative#12241 With the current defaults, setting min-scale=2 causes activator to flip in/out of path on each request. This PR sets the default TBC to 210 so as to not be a direct multiple of the container concurrency target default, and thus make it harder to trigger the activator flipageddon Signed-off-by: Paul S. Schweigert <[email protected]>
knative-prow-robot
added
size/S
Codecov Report
@@ Coverage Diff @@
## main #12774 +/- ##
=======================================
Coverage 87.46% 87.46%
=======================================
Files 196 196
Lines 9754 9760 +6
=======================================
+ Hits 8531 8537 +6
Misses 935 935
Partials 288 288
Continue to review full report at Codecov.
|
Signed-off-by: Paul S. Schweigert <[email protected]>
|
/retest |
|
Just out of curiosity - since I haven't been following in detail is there still a scenario (but now less likely) where a single request will flip the activator in/out of the path? |
|
/lgtm /hold for a few more eyes feel free to unhold if there's no input tomorrow |
knative-prow-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, psschwei 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:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
Hmmm... I guess any combination of |
Signed-off-by: Paul S. Schweigert <[email protected]>
|
/retest |
| MaxScaleUpRate: 1000, | ||
| MaxScaleDownRate: 2, | ||
| TargetBurstCapacity: 200, | ||
| TargetUtilization: defaultTargetUtilization, |
There was a problem hiding this comment.
looks good in general, but not sure about the alignment change. I think go lint would complain about this reformatting
There was a problem hiding this comment.
That was done by gofmt... think the alignment blocks are delineated by comments (which is why L48-51 got realigned). If I move the comment to the end of the line (instead of on a new line before) the indentation remains the same.
| @@ -112,6 +112,7 @@ func defaultConfigMapData() map[string]string { | |||
| "scale-to-zero-grace-period": gracePeriod.String(), | |||
| "tick-interval": "2s", | |||
| "min-scale": "0", | |||
| "target-burst-capacity": "200", | |||
There was a problem hiding this comment.
Shouldn't this be 211 then now, too ?
There was a problem hiding this comment.
I borrowed this from #12241 which didn't change the TBC value for the tests.
Before I saw that, I tried using TBC=211 and it caused a bunch of the tests to fail. I didn't dig too deep into it, so it could be that the tests are tuned to the 200 value (though that's just speculation, I really didn't look into for more than a few minutes).
If we'd like to get the tests passing with the updated TBC value, I can work on that (one thing I did find was increasing the minActivators by one (2 -> 3) fixed two-thirds of the errors, though not sure why exactly).
Well, unless cc=1 and min-scale=211, but for all intents and purposes... |
Lol - at least it's documented in this PR discussion |
knative-prow-robot
added
lgtm
|
/lgtm |
|
/retest |
|
/test istio-latest-no-mesh-tls_serving_main |
|
/test istio-latest-no-mesh_serving_main |
Signed-off-by: Paul S. Schweigert [email protected]
With the current TBC defaults (200), setting min-scale=2 causes activator to flip in/out of path on each request. This PR sets the default TBC to 210 so as to not be a direct multiple of the container concurrency target default, and thus make it harder to trigger activator flipageddon.
More discussion in (where we considered also setting TBC at
-1to always keep the activator in path unless overridden before a lazy consensus on just bumping the default slightly):#11926
#12241
/assign @dprotaso @rhuss @evankanderson @nader-ziada