Bug 1731263: Re-enable preemption test with fixes#23050
Bug 1731263: Re-enable preemption test with fixes#23050ravisantoshgudimetla wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
size/M
ravisantoshgudimetla
force-pushed
the
fix-tests
branch
2 times, most recently
from
01e77c0 to
e746b49
Compare
| milliCPU = milliCPU * 40 / 100 | ||
| memAllocatable, found := currentNode.Status.Allocatable["memory"] | ||
| // Just to be tolerant use 0.6 of resources available on the node | ||
| milliCPU = int64(float64(milliCPU-currentCpuUsage) * float64(0.6)) |
| } | ||
| if milliCPU <= 0 { | ||
| milliCPU = 0 | ||
| } |
There was a problem hiding this comment.
how will the low priority pod be preempted if it doesn't make any resource requests (i.e. runs best-effort)?
There was a problem hiding this comment.
As I have mentioned in the comment, we shouldn't enter this but if we're it means we're going past what can be allocated on the node.
There was a problem hiding this comment.
then why not just fail cleanly at this point rather than in some weird way that could be mistaken as a flake?
There was a problem hiding this comment.
That's true. The only thing I had in my mind back then was to ensure that tests won't fail because of -ve values being assigned to cpu and memory. I will remove it after the tests successfully pass.
ravisantoshgudimetla
force-pushed
the
fix-tests
branch
2 times, most recently
from
8b6c3be to
4014009
Compare
openshift-ci-robot
added
size/L
|
/retest |
ravisantoshgudimetla
force-pushed
the
fix-tests
branch
from
4014009 to
705c81e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ravisantoshgudimetla The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ravisantoshgudimetla
force-pushed
the
fix-tests
branch
2 times, most recently
from
b3954cf to
689355f
Compare
ravisantoshgudimetla
force-pushed
the
fix-tests
branch
from
689355f to
7db1d5e
Compare
|
@ravisantoshgudimetla: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| memory = int64(float64(memory-currentMemUsage) * float64(0.6)) | ||
| podRes = v1.ResourceList{} | ||
| // If a node is already heavily utilized let not's create a pod there. | ||
| if milliCPU <= 0 { |
There was a problem hiding this comment.
and I need to add memory check as well.
| continue | ||
| } | ||
| podRes[v1.ResourceCPU] = *resource.NewMilliQuantity(int64(milliCPU), resource.DecimalSI) | ||
| podRes[v1.ResourceMemory] = *resource.NewQuantity(int64(memory), resource.BinarySI) |
There was a problem hiding this comment.
The test is currently failing because of this change and I will revert it. Without this we were getting the following error:
skip [k8s.io/kubernetes/test/e2e/scheduling/preemption.go:234]: We need atleast two pods to be created butall nodes are already heavily utilized, so preemption tests cannot be run
Perhaps we need to increase the node size now since we're skipping those tests?
openshift-ci-robot
added
the
needs-rebase
|
@ravisantoshgudimetla: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retitle Bug 1731263: Re-enable preemption test with fixes |
openshift-ci-robot
changed the title
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@ravisantoshgudimetla: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@ravisantoshgudimetla will you be able to rebase this PR, or @damemi feel free to pick it up |
|
Rebased in #23645, this can be closed |
|
/close |
|
@soltysh: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
6 participants
Re-enable the preemption tests with fixes to ensure they can pass.
/cc @sjenning @wking