Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support overload manager #2062

Closed

Conversation

tmsnan
Copy link
Contributor

@tmsnan tmsnan commented Oct 25, 2023

What type of PR is this?
support overload manager

What this PR does / why we need it:

Which issue(s) this PR fixes:

Related #1966 #1048

Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
@tmsnan tmsnan requested a review from a team as a code owner October 25, 2023 03:39
@tmsnan
Copy link
Contributor Author

tmsnan commented Oct 25, 2023

How to set the approximate fixed heap value of the system through a script, i need help.

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: Patch coverage is 90.90909% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.17%. Comparing base (e4d9d7e) to head (ef78873).
Report is 507 commits behind head on main.

Files Patch % Lines
internal/cmd/egctl/translate.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2062      +/-   ##
==========================================
+ Coverage   65.04%   65.17%   +0.13%     
==========================================
  Files          99       99              
  Lines       14536    14542       +6     
==========================================
+ Hits         9455     9478      +23     
+ Misses       4494     4480      -14     
+ Partials      587      584       -3     

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

// After the feature is enabled, following two overload actions are configured to Envoy:
// 1.Shrink heap action is executed when 95% of the maximum heap size is reached.
// 2.Envoy will stop accepting requests when 98% of the maximum heap size is reached.
OverloadManager *OverloadManager `json:"overloadManager,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo we shouldnt expose envoyisms such as overloadManager directly, but do it in a way that is relevant to the EG user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right,the MaxHeapSizeBytes value should be exposed directly to enable the overload manager. I'm still thinking about how to set this value specifically.

@@ -213,6 +222,14 @@ type EnvoyProxyList struct {
Items []EnvoyProxy `json:"items"`
}

// OverloadManager defines the configuration for the Overload Manager in envoy.
type OverloadManager struct {
// The appropriate number of bytes can be different from system to system.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldnt add this API field if we cannot suggest a prescriptive way for a user to come up with a value to insert

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The google VRP example uses a 2GiB max heap size when executing a docker container with 3GB of memory.

Proposal:

  • By default, set MaxHeapSize to 70% of the envoy container's memory limit (which is known to EG, since it's generating the PodTemplate). If the limit is not set - don't configure anything.
  • Allow users to override this setting - configuring a custom percent.

@arkodg , @tmsnan - WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @guydc agree with the approach ! since EG creates the envoy proxy, we have the the exact memory specs for the envoy proxy fleet
imo we'll need to gather some more data before we decide on the the max heap and threshold values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chatted with @KBaichoo who advised that we could consider a tighter limit like 80% to optimize resources, thanks !

@tmsnan tmsnan marked this pull request as draft October 26, 2023 01:52
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Nov 25, 2023
@github-actions github-actions bot removed the stale label Feb 15, 2024
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants