Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9312

awels · 2023-02-23T18:15:06Z

What this PR does / why we need it:
Setting the ratio for mem/cpu to 1 allows the pod to be created under any combination of mem/cpu request/limit ratio. Added functional test to demonstrate various ratios work without issue.

This is a quick fix for just hotplug attachment pod containers for issue #9302 We should investigate a better option that doesn't waste some resources for resource constrained environments.

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:

Release note:

NONE

mhenriks

My concern with this is mostly that it is's incomplete. It does not fully address #9302

What would a proper fix look like? Can the LimitRanges in effect be discovered? Can we detect this error and recreate the pod? Do we need to extend the KubeVirt API to allow our request/limit ration be configured?

And if we want to do a temp fix, should the 1:1 ratio be applied to all containers?

What do you think @vladikr @davidvossel @xpivarc?

mhenriks · 2023-02-23T18:46:51Z

pkg/virt-controller/services/renderresources.go

 	}
 }

-func hotplugContainerMinimalLimits() k8sv1.ResourceList {
+func hotplugContainerMinimalResources() k8sv1.ResourceList {


I prefer maintaining the existing pattern of *MinimalRequests and *MinimalLimits functions. In this case the request function can call the limit function. I just feel that is a pattern we will want to go back to eventually with a proper fix

The name seemed wrong for what it was doing, that is why I changed it. I have no issues keeping the original naming it just didn't seem to fit with this particular change, that is why I changed the name.

Changed the name as requested

awels · 2023-02-27T15:40:25Z

/retest-required

xpivarc · 2023-03-01T15:13:52Z

tests/storage/hotplug.go

@@ -1158,6 +1162,205 @@ var _ = SIGDescribe("Hotplug", func() {
 		})
 	})

+	Context("with limit range in namespace", func() {


I think a unit test would be sufficient. WDYT?

I think a functional test is better (yes it is slower) but we can demonstrate this actually works for various combinations of ratios in a real cluster. A unit test would just show us that indeed the ratio is 1 to 1. But we are interested in does the functionality (hotplugging volumes) still work when various combinations of ratios are defined in the namespace.

awels · 2023-03-01T15:51:02Z

There will be a follow up PR of this that adds an API to allow one to specify the exact request/limit for both cpu and memory for all the different support containers. So this PR does NOT completely fix #9302 just the hotplug attachment pod container.

xpivarc · 2023-03-01T15:56:13Z

My concern with this is mostly that it is's incomplete. It does not fully address #9302

I am not sure what you mean by this.

What would a proper fix look like? Can the LimitRanges in effect be discovered? Can we detect this error and recreate the pod? Do we need to extend the KubeVirt API to allow our request/limit ration be configured?

And if we want to do a temp fix, should the 1:1 ratio be applied to all containers?

What do you think @vladikr @davidvossel @xpivarc?

I am not fun of tighter integration in Kubevirt.

awels · 2023-03-01T16:07:28Z

My concern with this is mostly that it is's incomplete. It does not fully address #9302

I am not sure what you mean by this.

So #9302 is about more than just the hotplug attachment pod container, there are other containers in KubeVirt with a similar ratio that will break if you apply the LimitRange with a requestLimitRatio. This particular PR just addresses hotplug, not the other containers. I will be making a much more extensive follow up PR that addresses everything.

What would a proper fix look like? Can the LimitRanges in effect be discovered? Can we detect this error and recreate the pod? Do we need to extend the KubeVirt API to allow our request/limit ration be configured?
And if we want to do a temp fix, should the 1:1 ratio be applied to all containers?
What do you think @vladikr @davidvossel @xpivarc?

I am not fun of tighter integration in Kubevirt.

mhenriks · 2023-03-01T16:23:10Z

Assuming other containers will be addressed in subsequent pr

/lgtm
/approve

kubevirt-bot · 2023-03-01T16:23:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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:

~~OWNERS~~ [mhenriks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

xpivarc · 2023-03-01T16:28:06Z

My concern with this is mostly that it is's incomplete. It does not fully address #9302

I am not sure what you mean by this.

So #9302 is about more than just the hotplug attachment pod container, there are other containers in KubeVirt with a similar ratio that will break if you apply the LimitRange with a requestLimitRatio. This particular PR just addresses hotplug, not the other containers. I will be making a much more extensive follow up PR that addresses everything.

What would a proper fix look like? Can the LimitRanges in effect be discovered? Can we detect this error and recreate the pod? Do we need to extend the KubeVirt API to allow our request/limit ration be configured?
And if we want to do a temp fix, should the 1:1 ratio be applied to all containers?
What do you think @vladikr @davidvossel @xpivarc?

I am not fun of tighter integration in Kubevirt.

Oh I missed that it is a general issue.

xpivarc

/hold

xpivarc · 2023-03-01T16:30:01Z

tests/storage/hotplug.go

+			_, err = virtClient.VirtualMachineInstance(vm.Namespace).Get(context.Background(), vm.Name, &metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+		},
+			Entry("[Serial]1 to 1 cpu and mem ratio", float64(1), float64(1)),


You need a serial label as well.

I am not sure I follow. all the entries have a [serial] label

We now use gingko labels. Please stand by I am searching for a reference.

See https://github.com/kubevirt/kubevirt/blob/main/tests/canary_upgrade_test.go#L49 the argument to Desribe

So you need
Entry("[Serial]1 to 1 cpu and mem ratio", Serial, float64(1), float64(1)),

… 1 to 1 ratio Setting the ratio for mem/cpu to 1 allows the pod to be created under any combination of mem/cpu request/limit ratio. Added functional test to demonstrate various ratios work without issue. Signed-off-by: Alexander Wels <awels@redhat.com>

Signed-off-by: Alexander Wels <awels@redhat.com>

xpivarc · 2023-03-01T17:50:54Z

/hold cancel
/lgtm

kubevirt-bot · 2023-03-01T18:32:10Z

@awels: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-fossa	`5002793`	link	false	`/test pull-kubevirt-fossa`

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. I understand the commands that are listed here.

awels · 2023-03-01T21:01:32Z

/test pull-kubevirt-e2e-kind-1.23-vgpu

awels · 2023-03-02T14:02:02Z

/cherrypick release-0.59

kubevirt-bot · 2023-03-02T14:02:51Z

@awels: new pull request created: #9352

In response to this:

/cherrypick release-0.59

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.

awels · 2023-03-02T14:03:03Z

/cherrypick release-0.58

kubevirt-bot · 2023-03-02T14:03:56Z

@awels: #9312 failed to apply on top of branch "release-0.58":

Applying: Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio
Using index info to reconstruct a base tree...
M	pkg/virt-controller/services/renderresources.go
M	tests/storage/hotplug.go
Falling back to patching base and 3-way merge...
Auto-merging tests/storage/hotplug.go
CONFLICT (content): Merge conflict in tests/storage/hotplug.go
Auto-merging pkg/virt-controller/services/renderresources.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-0.58

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.

…_to_1 Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio

…_to_1 Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio Signed-off-by: Alexander Wels <awels@redhat.com>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Feb 23, 2023

kubevirt-bot requested review from 0xFelix and kbidarkar February 23, 2023 18:21

mhenriks reviewed Feb 23, 2023

View reviewed changes

xpivarc reviewed Mar 1, 2023

View reviewed changes

kubevirt-bot assigned mhenriks Mar 1, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2023

xpivarc reviewed Mar 1, 2023

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2023

awels added 3 commits March 1, 2023 11:45

Revert change to function name

3b1e4e5

Signed-off-by: Alexander Wels <awels@redhat.com>

Add serial lable to tests

5002793

Signed-off-by: Alexander Wels <awels@redhat.com>

awels force-pushed the set_hotplug_cpu_mem_ratio_to_1 branch from e043bbf to 5002793 Compare March 1, 2023 17:45

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2023

kubevirt-bot assigned xpivarc Mar 1, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2023

kubevirt-bot merged commit 3cdcfab into kubevirt:main Mar 1, 2023

kubevirt-bot mentioned this pull request Mar 2, 2023

[release-0.59] Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9352

Closed

awels pushed a commit to awels/kubevirt that referenced this pull request Mar 2, 2023

Merge pull request kubevirt#9312 from awels/set_hotplug_cpu_mem_ratio…

d4a406f

…_to_1 Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio

awels mentioned this pull request Mar 2, 2023

[release-0.58] Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9353

Merged

awels pushed a commit to awels/kubevirt that referenced this pull request Mar 2, 2023

Merge pull request kubevirt#9312 from awels/set_hotplug_cpu_mem_ratio…

58427ca

…_to_1 Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio

awels mentioned this pull request Mar 2, 2023

[release-0.59] Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9354

Merged

awels mentioned this pull request Mar 2, 2023

[release-0.53] Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9312

Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9312

awels commented Feb 23, 2023

mhenriks left a comment •

edited

Loading

mhenriks Feb 23, 2023

awels Feb 23, 2023

awels Mar 1, 2023

awels commented Feb 27, 2023

xpivarc Mar 1, 2023

awels Mar 1, 2023 •

edited

Loading

awels commented Mar 1, 2023

xpivarc commented Mar 1, 2023

awels commented Mar 1, 2023

mhenriks commented Mar 1, 2023

kubevirt-bot commented Mar 1, 2023

xpivarc commented Mar 1, 2023

xpivarc left a comment

xpivarc Mar 1, 2023

awels Mar 1, 2023

xpivarc Mar 1, 2023

xpivarc Mar 1, 2023

xpivarc Mar 1, 2023

awels Mar 1, 2023

xpivarc commented Mar 1, 2023

kubevirt-bot commented Mar 1, 2023 •

edited

Loading

awels commented Mar 1, 2023

awels commented Mar 2, 2023

kubevirt-bot commented Mar 2, 2023

awels commented Mar 2, 2023

kubevirt-bot commented Mar 2, 2023

Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9312

Set mem/cpu request and limits on hotplug attachment pod container to 1 to 1 ratio #9312

Conversation

awels commented Feb 23, 2023

mhenriks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels commented Feb 27, 2023

Choose a reason for hiding this comment

awels Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

awels commented Mar 1, 2023

xpivarc commented Mar 1, 2023

awels commented Mar 1, 2023

mhenriks commented Mar 1, 2023

kubevirt-bot commented Mar 1, 2023

xpivarc commented Mar 1, 2023

xpivarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc commented Mar 1, 2023

kubevirt-bot commented Mar 1, 2023 • edited Loading

awels commented Mar 1, 2023

awels commented Mar 2, 2023

kubevirt-bot commented Mar 2, 2023

awels commented Mar 2, 2023

kubevirt-bot commented Mar 2, 2023

mhenriks left a comment •

edited

Loading

awels Mar 1, 2023 •

edited

Loading

kubevirt-bot commented Mar 1, 2023 •

edited

Loading