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
Restore default hotplug attachment pod request to original values #10720
Restore default hotplug attachment pod request to original values #10720
Conversation
As a quick fix we modified the requests and limit ratio to be 1. However this is not ideal since it reserves a lot of cpu and memory for that container that doesn't really do anything at all. Its purpose is to attach a volume to the node. We added the ability to set the requests and limits for all the supporting containers in (kubevirt#9422). At this point anyone can set the ratio to what they want. So lets restore the defaults to the original values, so that people that don't care don't end up using a lot of memory for no reason. Signed-off-by: Alexander Wels <awels@redhat.com>
/test pull-kubevirt-unit-test |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xpivarc 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 |
/test pull-kubevirt-e2e-k8s-1.26-sig-storage |
@@ -4070,7 +4070,7 @@ var _ = Describe("Template", func() { | |||
Expect(memLimit).To(Equal(expMemLimitQ.Value())) | |||
expCpuReqQ := resource.MustParse("100m") | |||
Expect(cpuReq).To(Equal(expCpuReqQ.Value())) | |||
expMemReqQ := resource.MustParse("80M") | |||
expMemReqQ := resource.MustParse("2M") |
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.
I'd expect expCpuReqQ := resource.MustParse("100m")
to be expCpuReqQ := resource.MustParse("10m")
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.
Sorry, reading my own comment I realize this is too criptic.
What I mean is that here the expected memory request was reduced back to 2M
.
I would expect the same to happen for the cpu request too, IOW to be rolled back to 10m
, but it is not.
Can you explain? Thanks
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.
No you are right, have to check why just the one, and not the other.
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.
So turns out the unscaled value of 10m and 100m is both '1' and thus matches, instead I need to use MilliValue, and that fixes the issue.
so that the scaled values don't match. 10m and 100m are 1 when calling Value, but 10 and 100 when calling MilliValue. Signed-off-by: Alexander Wels <awels@redhat.com>
/test pull-kubevirt-e2e-k8s-1.27-sig-network |
Thank you! |
What this PR does / why we need it:
As a quick fix we modified the requests and limit ratio to be 1. However this is not ideal since it reserves a lot of cpu and memory for that container that doesn't really do anything at all. Its purpose is to attach a volume to the node.
We added the ability to set the requests and limits for all the supporting containers in (#9422). At this point anyone can set the ratio to what they want. So lets restore the defaults to the original values, so that people that don't care don't end up using a lot of memory for no reason.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: