-
Notifications
You must be signed in to change notification settings - Fork 312
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
koordlet: tc plugin for netqos #1920
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7205dd1
to
fa87361
Compare
it will be great if you can add some implement details in doc about tc plugin https://github.com/koordinator-sh/koordinator/blob/main/docs/proposals/koordlet/20231208-support-netqos.md#internal-plugins |
write in this pr? |
278d46a
to
4f2060b
Compare
another one with only proposal is better |
03257aa
to
96ae4e0
Compare
done. and this is proposal pr: #1954 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1920 +/- ##
==========================================
- Coverage 68.57% 67.78% -0.80%
==========================================
Files 430 436 +6
Lines 39383 40192 +809
==========================================
+ Hits 27007 27244 +237
- Misses 10037 10594 +557
- Partials 2339 2354 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
347bd58
to
12deec2
Compare
058a017
to
1c8ced4
Compare
05a8197
to
85a32a3
Compare
go mod tidy? |
newRule.enable = false | ||
} | ||
|
||
p.rule.Update(&newRule) |
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.
check whether rule is updated with reflect.DeepEqual
} | ||
|
||
return apierror.NewAggregate([]error{ | ||
p.ensureFilter(filterKey(strconv.Itoa(SYSTEM_CLASS_MINOR_ID)), genFilterCmd("add", SYSTEM_CLASS_PRIO, SYSTEM_CLASS_MINOR_ID), |
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.
could add/change be seperated? so we don't need add if already exist.
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.
// TODO
} | ||
needLimitAtPodLevel := ing != 0 || egress != 0 | ||
|
||
if pod.Pod.DeletionTimestamp != nil { |
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.
pod may be already deleted before we catch the deleteTimestamp
maybe we should list all rules and delete those danglings.
|
||
minorHex := "" | ||
key := string(uid) | ||
for i := 0; i < 5; i++ { |
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.
maybe we can use other hash algo to avoid the conflicts.
} | ||
|
||
func (p *tcPlugin) DestoryIpset() error { | ||
klog.V(5).Infof("start to delete ipset rules crated by tc plugin.") |
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.
crated -> created
} | ||
|
||
func (p *tcPlugin) DelIptables() error { | ||
klog.V(5).Infof("start to delete iptables rules crated by tc plugin.") |
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.
crated -> created
e08de79
to
72148e2
Compare
code needs rebase |
9a764c5
to
eb5f2af
Compare
Signed-off-by: lucming <[email protected]>
Ⅰ. Describe what this PR does
A plugin for netqos, and based on tc.
other discussion: #1764
btw, there may be some duplicate defination with pr #1843 .
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews
V. Checklist
make test