OCPBUGS-52169: Workload partitioning of static init containers #2224
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci-robot
added
jira/valid-reference
|
@MarSik: This pull request references Jira Issue OCPBUGS-52169, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@MarSik: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@MarSik: This pull request references Jira Issue OCPBUGS-52169, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In 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 openshift-eng/jira-lifecycle-plugin repository. |
MarSik
force-pushed
the
fix-wp-static-init
branch
from
75934a8 to
5b4180c
Compare
|
@MarSik: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
| @@ -481,15 +541,24 @@ func TestStaticPodManaged(t *testing.T) { | |||
| if container.Resources.Requests.Cpu().String() != "0" && !tc.isGuaranteed { | |||
| t.Errorf("cpu requests should be 0 got %v", container.Resources.Requests.Cpu().String()) | |||
| } | |||
| if container.Resources.Requests.Memory().String() == "0" && !tc.isGuaranteed { | |||
| if container.Resources.Requests.Memory().String() == "0" && !tc.isGuaranteed && !tc.isBestEffort { | |||
| t.Errorf("memory requests were %v but should be %v", container.Resources.Requests.Memory().String(), container.Resources.Requests.Memory().String()) | |||
There was a problem hiding this comment.
Note: I did not touch this, but the Errorf is broken, it uses the same value for both the left and the right side.
MarSik
force-pushed
the
fix-wp-static-init
branch
from
5b4180c to
08c1486
Compare
|
@MarSik: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
note: the commit needs overall this makes sense to me |
|
@rphillips @mrunalp PTAL |
…t containers The pod modification routine that prepares containers for Workload Partitioning quits early when it encounters a container with no resources specified. This causes a logical leak of resource capacity on the node, because the Pod is left untouched and still reports its full resource requests to the scheduler. It is however not using them, because the logic that moves the container to the management partitions works just fine. The end result is lowered node capacity for scheduling. Signed-off-by: Martin Sivak <[email protected]>
MarSik
force-pushed
the
fix-wp-static-init
branch
from
08c1486 to
9f2c1a5
Compare
|
@MarSik: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/retitle OCPBUGS-52169: Workload partitioning of static init containers |
openshift-ci
bot
changed the title
|
/retest-required |
|
@MarSik: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MarSik, rphillips The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| // When both cpu and memory requests are missing, there is nothing | ||
| // to do | ||
| if !cpuOk && !memoryOk { |
There was a problem hiding this comment.
Isn't this missing the case where only limits are set and requests are implied from limits?
What type of PR is this?
/kind bug
What this PR does / why we need it:
The pod modification routine that prepares containers for Workload Partitioning quits early when it encounters a container with no resources specified.
This causes a logical leak of resource capacity on the node, because the Pod is left untouched and still reports its full resource requests to the scheduler. It is however not using them, because the logic that moves the container to the management partitions works just fine.
The end result is lowered node capacity for scheduling.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-52169
Special notes for your reviewer:
Modifies existing downstream only code.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Related to GAd https://github.com/openshift/enhancements/blob/master/enhancements/workload-partitioning/management-workload-partitioning.md#api-server-admission-hook