kubelet: fix nil crash in allocateRemainingFrom #113021
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/kubelet
sig/node
rphillips
force-pushed
the
fixes/112936
branch
from
956a5ff
to
2514486
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
| // 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.
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.
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 |
k8s-ci-robot
added
triage/accepted
k8s-ci-robot
added
the
lgtm
|
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.
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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it:
allocateRemainingFrom uses
allocatedDeviceswhich 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.: