feat: configure overload manager #3082
Conversation
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3082 +/- ##
==========================================
+ Coverage 66.54% 66.59% +0.04%
==========================================
Files 157 157
Lines 21956 21976 +20
==========================================
+ Hits 14611 14635 +24
+ Misses 6502 6499 -3
+ Partials 843 842 -1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
| // caclulateMaxHeapSizeBytes calculates the maximum heap size in bytes as 80% of Envoy container memory limits. | ||
| // In case no limits are defined '0' is returned, which means no heap size limit is set. | ||
| func caclulateMaxHeapSizeBytes(envoyResourceRequirements *corev1.ResourceRequirements) uint64 { | ||
| if envoyResourceRequirements == nil || envoyResourceRequirements.Limits == nil { |
There was a problem hiding this comment.
also lets make this method best effort
so it returns a *unit64, when we cannot compute the size lets return nil
and not add a max any FixedHeapConfig for that case
There was a problem hiding this comment.
looks like you already handle 0, suggest changing the API signature to return a ptr instead so the caller can use nil as a way to make a decision
There was a problem hiding this comment.
0 is an illegal value for max_heap_size_bytes, so it means it's disabled. It can be returned in 2 cases:
- No memory limits are defined
- Memory limits are
0, which has the same affect as1
I don't see a value in changing the signature to ptr unless I missed something.
There was a problem hiding this comment.
cool if 0 is disabled, then sgtm, thats not clear from the API doc https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/resource_monitor/fixed_heap/v2alpha/fixed_heap.proto#config-resource-monitor-fixed-heap-v2alpha-fixedheapconfig
There was a problem hiding this comment.
Right, I verified it by defining 0 and got validation error from Envoy at runtime.
|
looks good ! just added one comment around using a ptr to improve the distinction b/w unset/unknown and 0 |
What type of PR is this?
feature
What this PR does / why we need it:
Configure overload manager as recommended in Configuring Envoy as an edge proxy guide with the following:
50000.max_heap_size_bytesto80%ofEnvoycontainer memory limits. in case there are no limitsmax_heap_size_bytesis not being set. This approach has been agreed here.Which issue(s) this PR fixes:
Fixes #1966
Related #1048
Note regarding global downstream connection limits:
latest stable Envoy(v1.29.2) docs suggests to use new overload manager API and also states: "One could also set this limit via specifying an integer through the runtime key overload.global_downstream_max_connections, though this key is deprecated and will be removed in future".
However, when navigating to Downstream connections API page it is stated that it's currently work-in-progress.