-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Fix slice sharing bug in cgroup manager #70678
Conversation
/sig node |
this should have the 1.13 milestone |
/lgtm |
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.
LGTM. Please create a cherry pick PR to branch-1.12 after this.
/retest |
1 similar comment
/retest |
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 @dashpole for tracking this one down!
Ugh, what an ugly interface in Golang... Time to write another Golang rant about it...
I have a tiny nitpick, but what you have is definitely fine, so OK to ignore my suggestion.
Cheers,
Filipe
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, yujuhong The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What supported versions are affected by this bug? |
this affects 1.11 and 1.12 |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
Why wasn't this marked for backport? |
/lgtm @pires opening cherry picks for 1.11 and 1.12 soon is 1.10 maybe affected as well? 3 major versions are supported at a time |
…8-upstream-release-1.11 Automated cherry pick of #70678: fix slice sharing bug in cgroup manager
…8-upstream-release-1.12 Automated cherry pick of #70678: fix slice sharing bug in cgroup manager
What type of PR is this?
/kind bug
What this PR does / why we need it:
See https://play.golang.org/p/QQOELuf56v8 for an example of what can happen when sharing underlying slices. Slices are only resized when the slice cannot hold the additional elements, and are increased by powers of two. So appending to a slice may or may not create a new slice to hold the input slice.
We just happened to be lucky that the default for
--cgroup-root
didn't trigger this behavior because getting pod cgroup slices ([]string{"kubepods", "burstable"}
->[]string{"kubepods", "burstable", "<pod_uid>"}
always triggered a resize of the former slice, as the size goes from 2 -> 4 each time. But when you specify a cgroup root ([]string{"root", "kubepods", "burstable"}
->[]string{"root", "kubepods", "burstable", "<pod_uid>"}
, the initial slice is already size 4 (because it was constructed by appending to a size 2 slice), and thus does not trigger a resize, and re-uses the parent slice for each child slice. In this way, after appending one pod uid to the QoS cgroup, the second append produces[]string{"root", "kubepods", "burstable", "<pod_uid_1>", "<pod_uid_2>"}
, which is a slice resize (4->8).All of this is to say that we should create a new slice and copy the parent into it when we create a new cgroup. I included my reproduction case as a test case to ensure we don't break this in the future.
Which issue(s) this PR fixes:
Fixes #68416
Does this PR introduce a user-facing change?:
cc @sreis @derekwaynecarr @filbranden @dchen1107 @Random-Liu @yujuhong
/assign @derekwaynecarr @Random-Liu