-
Notifications
You must be signed in to change notification settings - Fork 109
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(sockmem-plugin): unified solution for TCP memory limitation #365 #366
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 53.31% 53.49% +0.17%
==========================================
Files 437 439 +2
Lines 48155 48322 +167
==========================================
+ Hits 25674 25848 +174
+ Misses 19574 19553 -21
- Partials 2907 2921 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c089dbe
to
86808e6
Compare
Hi @waynepeking348 |
86808e6
to
ea94656
Compare
ea94656
to
c0325aa
Compare
// register qrm memory-handlers registered in adapter | ||
func init() { | ||
var errList []error | ||
errList = append(errList, periodicalhandler.RegisterPeriodicalHandler(qrm.QRMMemoryPluginPeriodicalHandlerGroupName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will it be better for this strategy applied through cgroup manager
or cri
cc @csfldf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csfldf Any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the registration code to DynamicPolicy.Start, and refine it like below:
if p.enableXxx {
err := periodicalhandler.RegisterPeriodicalHandler(...)
if err != nil {
return fmt.Error("xxx failed with error: %v", err);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pls DON'T close and reopen PR arbitrarily, we need to track the comments @lubinszARM |
c12b4d9
to
af42f13
Compare
e446446
to
b9239b6
Compare
Signed-off-by: Robin Lu <robin.lu@bytedance.com>
Signed-off-by: Robin Lu <robin.lu@bytedance.com>
Signed-off-by: Robin Lu <robin.lu@bytedance.com>
Signed-off-by: Robin Lu <robin.lu@bytedance.com>
b9239b6
to
cd1f144
Compare
What type of PR is this?
Features
What this PR does / why we need it:
The feature provides the unified solution for TCP memory limitation in cgroup and global level.
Which issue(s) this PR fixes:
In our production environment, there is a critical case where certain services heavily consumed tcpmem(cgroupv1 pods with default setting have unlimited tcpmem), causing the global tcpmem to reach the limitation. As a result, the network performance of the entire machine was crash.
Special notes for your reviewer:
The feature includes 3 parts:
1, Set global tcpmem limit by changing net.ipv4.tcp_mem.
The default value is 20% of host toal memory.
2, Do nothing under cgroupv2.
3, Set pod tcpmem limit under cgroupv1.
The default value is same with memory.limit_in_bytes.