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
kubelet: fix nil crash in allocateRemainingFrom #113021
Conversation
956a5ff
to
2514486
Compare
// Needs to allocate additional devices. | ||
if m.allocatedDevices[resource] == nil { | ||
m.allocatedDevices[resource] = sets.NewString() | ||
} | ||
|
||
// Allocates from reusableDevices list first. |
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.
it was not clear to me how allocateRemainingFrom used m.allocatedDevices. Maybe we should change from a functor to a function, or redefine the functor to be passed a variable rather than use a closure?
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.
The functor is reused in other places. I don't think we should change the logic here since the functor is keeping a count. This is the simplest change to prevent the crash.
/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
if there is an easy to add tests, that would be great. The logic itself looks OK
I don't think we should add a test for a nil check. |
if allocateRemainingFrom(reusableDevices) { | ||
return allocated, nil | ||
} | ||
|
||
// Needs to allocate additional devices. | ||
if m.allocatedDevices[resource] == nil { | ||
m.allocatedDevices[resource] = sets.NewString() | ||
} |
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.
Would it make sense to move this check up the stack to prevent crashes in other places? Ideally it should be done as soon as the resource value is known.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrunalp, rphillips, SergeyKanzhelev 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 type of PR is this?
/kind bug
What this PR does / why we need it:
allocateRemainingFrom uses
allocatedDevices
which needs to be allocated. The function calls allocateRemainingFrom before the nil check. Reverse the order of operations so the set is allocated prior to calling allocateRemainingFrom.Which issue(s) this PR fixes:
Fixes #112936
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: