-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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 pod created failed when resource request is 0 #111544
Conversation
/retest |
/triage accepted |
@jlsong01 Please add some unittest for this PR. Thanks very much. |
edb42a5
to
4d2e7fa
Compare
@jlsong01 Is it more elegant to use |
Add comment at #109191 (comment), I may treat this not fix the bug, but related, for request values are not right even with this patch. |
if rQuant > (nodeInfo.Allocatable.ScalarResources[rName] - nodeInfo.Requested.ScalarResources[rName]) { | ||
|
||
// Skip in case request quantity is zero | ||
if rQuant == 0 { |
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.
let's add this to the head of the loop, it's more efficient.
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.
done
ScalarResources: map[v1.ResourceName]int64{ | ||
kubernetesIOResourceA: 0, | ||
}}), | ||
nodeInfo: framework.NewNodeInfo(newResourcePod(framework.Resource{MilliCPU: 0, Memory: 0})), |
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.
Can we cover this case with (nodeInfo.Allocatable.ScalarResources[rName] - nodeInfo.Requested.ScalarResources[rName]) < 0
, which mocks the bug we met.
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.
Sure. updated the case as allocatable
5 and Requested
6 for extendedResourceA
, which should hit the bug we met.
ce0983a
to
27824fb
Compare
Totally looks good to me, so I labelled with LGTM, but we may get some advices from sig-node expert. Although we changed codes in scheduler, but this is a common function shared by scheduler and kubelet. See kubernetes/pkg/kubelet/lifecycle/predicate.go Line 263 in cb41d50
cc @mrunalp /lgtm |
Besides, we're code freeze now, we may merge this PR in the next release v1.26. |
/assign @Huang-Wei |
@jlsong01 could you revise the description section a bit - as commented in the original issue #109191 (comment): this PR mitigates (when req is 0) instead of fixing the original issue. |
@Huang-Wei The PR's description has been revised. If anything else needed, please let me know :-) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei, jlsong01 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
/sig node
What this PR does / why we need it:
Pod creation should not fail in case request resource is zero.
In terms of tolerating 0-requested req in scheduler, it's a good-to-have and not a root fix to this issue (#109191 (comment))
Which issue(s) this PR fixes:
Relevant to #109191
Special notes for your reviewer:
This PR mitigates (when req is 0) instead of fixing the original issue.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: