-
Notifications
You must be signed in to change notification settings - Fork 402
Update/runc 1.3.2 #3274
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
Update/runc 1.3.2 #3274
Conversation
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
|
I understand the background. Due to the following Kubernetes issue, the conversion formula from cgroup v1 shares to cgroup v2 weight has been changed: The fix in runc is as follows. |
saku3
left a comment
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.
@n4mlz
Thanks for the PR!
Overall, it looks good to me.
If you could add comments similar to runc’s, I think it would make the changes even clearer.
| } | ||
|
|
||
| 1 + ((shares.saturating_sub(2)) * 9999) / 262142 | ||
| const MIN_SHARES: u64 = 2; |
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.
Having comments would make it even easier to understand.
|
|
||
| let weight = 1 + ((shares.saturating_sub(2)) * 9999) / 262142; | ||
| weight.min(MAX_CPU_WEIGHT) | ||
| const MIN_SHARES: u64 = 2; |
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.
Having comments would make it even easier to understand.
Signed-off-by: n4mlz <[email protected]>
saku3
left a comment
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.
Thanks!
LGTM once CI passes.
YJDoc2
left a comment
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.
Hey looks good to me, but I'll request you to add the runc release tag link and pr links mentioned by saku3 in a comment here. In future we might not be sure as to why we are using this particular formula , and the git link to pr might also be lost if we run formatting or file movement. The explicit links in comments would help us get the context for this.
|
Also our current CI requires exactly one |
Signed-off-by: n4mlz <[email protected]>
|
Sorry for the delay in my response. I’ve added some comments. Please take a look. |
Description
Type of Change
Testing
Related Issues
Additional Context