Change KubeVirt VM CPU assignment to not overwrite cpu alloc ratio by soer3n · Pull Request #1906 · kubermatic/machine-controller

soer3n · Mar 10, 2025

What this PR does / why we need it:
This pr changes the assignment of cpu resources for kubevirt virtual machines to be able to use kubevirt cpuAllocationRatio feature by introducing a new field to the providerSpec called vCPUs.

Which issue(s) this PR fixes:

Fixes #1905

What type of PR is this?

/kind feature

Special notes for your reviewer:

Does this PR introduce a user-facing change? Then add your Release Note here:

A new field `cloudProviderSpec.virtualMachine.template.vcpus` was added to kubevirt provider. When `vcpus.cores` is set, the cpus for the virtual machine will be configured in `spec.template.spec.domain.cpu` instead of setting resource requests and limits explicit. The webhook validation will fail if both `vcpus` and `cpus` are configured for a machine.

Documentation:

NONE

Signed-off-by: soer3n <srenhenning@googlemail.com>

ahmedwaleedmalik · Mar 12, 2025

pkg/cloudprovider/provider/kubevirt/provider.go

+	if isDedicatedVCPURequested(machine.Spec.Annotations) {
+		cpusAsUint64, err := strconv.ParseUint(c.CPUs, 0, 64)
+
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse cpu cores: %w", err)
+		}
+
+		virtualMachine.Spec.Template.Spec.Domain.CPU = &kubevirtcorev1.CPU{
+			Cores: uint32(cpusAsUint64),
+		}
+	}


If this annotation is consumed in machine-controller then this should be configured at machine.Annotations not in the spec.

ahmedwaleedmalik · Mar 12, 2025

sdk/cloudprovider/kubevirt/types.go

 )

+const (
+	UseDedicatedKubevirtVCPUAnnotationKey = "kubermatic.io/use-dedicated-kubevirt-cpu"


Suggested change

UseDedicatedKubevirtVCPUAnnotationKey = "kubermatic.io/use-dedicated-kubevirt-cpu"

UseDedicatedKubeVirtVCPUAnnotationKey = "kubermatic.io/use-dedicated-kubevirt-cpu"

ahmedwaleedmalik · Mar 12, 2025

pkg/cloudprovider/provider/kubevirt/provider.go

+
+	if isDedicatedVCPURequested(machine.Spec.Annotations) {
+		cpusAsUint64, err := strconv.ParseUint(c.CPUs, 0, 64)
+


No need for space here

ahmedwaleedmalik · Mar 12, 2025

Please update the release note and mention the annotation explicitly that you have introduced
This is a bug or a feature request and not a chore
I have updated the title. Please follow proper casing

moadqassem · Mar 12, 2025

sdk/cloudprovider/kubevirt/types.go

 )

+const (
+	UseDedicatedKubevirtVCPUAnnotationKey = "kubermatic.io/use-dedicated-kubevirt-cpu"


I think it is better if you actually use the RawConfig instead of annotations.

Signed-off-by: soer3n <srenhenning@googlemail.com>

* adapt kubevirt cpu struct for configuring vcpus for a virtual machine * modify logic in function for rendering resource requests and limits * modify validation accordingly to be a bit more specific regarding resources Signed-off-by: soer3n <srenhenning@googlemail.com>

Signed-off-by: soer3n <srenhenning@googlemail.com>

soer3n · Mar 13, 2025

@ahmedwaleedmalik I changed the kind to feature and modified the release notes accordingly to the changes i added after discussing the implementation with @moadqassem.

pkg/cloudprovider/provider/kubevirt/provider.go

ahmedwaleedmalik · Mar 13, 2025

pkg/cloudprovider/provider/kubevirt/provider.go

+		if (c.VCPUs == nil && c.Resources.Cpu() == nil) || (c.VCPUs != nil && c.Resources.Cpu() != nil) {
+			return fmt.Errorf("invalid/no cpus configured. Either vpus or cpus have to be configured.")


The error message here is quite misleading since in cases where both vCPU and CPU are set, you are still returning the same message. Please split this into two different if blocks or be a bit more coherent with your error message.

Thanks for the suggestion. I separated both conditions and changed the return message to be a bit more clearer.

sdk/cloudprovider/kubevirt/types.go

ahmedwaleedmalik · Mar 13, 2025

sdk/cloudprovider/kubevirt/types.go

+	Sockets int `json:"sockets,omitempty"`
+	Threads int `json:"threads,omitempty"`


Both Sockets and Threads are unused. We should either remove these variables or utilize them at

machine-controller/pkg/cloudprovider/provider/kubevirt/provider.go

Line 893 in 8b52fcc

return &corev1.ResourceList{corev1.ResourceMemory: memoryResource}, &kubevirtcorev1.CPU{Cores: uint32(vpcus.Cores)}, nil

and

machine-controller/pkg/cloudprovider/provider/kubevirt/provider.go

Lines 854 to 856 in 8b52fcc

virtualMachine.Spec.Template.Spec.Domain.CPU = &kubevirtcorev1.CPU{

Cores: c.VCPUs.Cores,

}

I removed the unused values and only left cores for now.

ahmedwaleedmalik · Mar 13, 2025

Looks much better as part of the providerconfig 🚀

Same feedback as the first point from #1906 (comment), in release note please explicitly mention the "new field" that you have introduced i.e. cloudProviderSpec.virtualMachine.template.vcpus

Signed-off-by: soer3n <srenhenning@googlemail.com>

ahmedwaleedmalik

/approve

kubermatic-bot · Mar 14, 2025

LGTM label has been added.

Details

Git tree hash: 997ea22ccf2bb767bf1bf0be0f7c512163f0e8e8

kubermatic-bot · Mar 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmedwaleedmalik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahmedwaleedmalik]

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

soer3n · Apr 4, 2025

/cherrypick release/v1.61

kubermatic-bot · Apr 4, 2025

@soer3n: #1906 failed to apply on top of branch "release/v1.61":

Applying: change kubevirt vm cpu assignment to not overwrite cpu alloc ratio
Using index info to reconstruct a base tree...
M	pkg/cloudprovider/provider/kubevirt/provider.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloudprovider/provider/kubevirt/provider.go
CONFLICT (content): Merge conflict in pkg/cloudprovider/provider/kubevirt/provider.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 change kubevirt vm cpu assignment to not overwrite cpu alloc ratio

Details

In response to this:

/cherrypick release/v1.61

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.

…ubermatic#1906) * change kubevirt vm cpu assignment to not overwrite cpu alloc ratio Signed-off-by: soer3n <srenhenning@googlemail.com> * update kubevirt testdata Signed-off-by: soer3n <srenhenning@googlemail.com> * revert testdata change and add condition for vm cpu assignment Signed-off-by: soer3n <srenhenning@googlemail.com> * switch to providerSpec for enabling vcpu assignment Signed-off-by: soer3n <srenhenning@googlemail.com> * adapt kubevirt cpu struct * adapt kubevirt cpu struct for configuring vcpus for a virtual machine * modify logic in function for rendering resource requests and limits * modify validation accordingly to be a bit more specific regarding resources Signed-off-by: soer3n <srenhenning@googlemail.com> * revert unnessecarry changes to mocked kubevirt vm Signed-off-by: soer3n <srenhenning@googlemail.com> * changes after review Signed-off-by: soer3n <srenhenning@googlemail.com> --------- Signed-off-by: soer3n <srenhenning@googlemail.com>

* Change KubeVirt VM CPU assignment to not overwrite cpu alloc ratio (#1906) * change kubevirt vm cpu assignment to not overwrite cpu alloc ratio Signed-off-by: soer3n <srenhenning@googlemail.com> * update kubevirt testdata Signed-off-by: soer3n <srenhenning@googlemail.com> * revert testdata change and add condition for vm cpu assignment Signed-off-by: soer3n <srenhenning@googlemail.com> * switch to providerSpec for enabling vcpu assignment Signed-off-by: soer3n <srenhenning@googlemail.com> * adapt kubevirt cpu struct * adapt kubevirt cpu struct for configuring vcpus for a virtual machine * modify logic in function for rendering resource requests and limits * modify validation accordingly to be a bit more specific regarding resources Signed-off-by: soer3n <srenhenning@googlemail.com> * revert unnessecarry changes to mocked kubevirt vm Signed-off-by: soer3n <srenhenning@googlemail.com> * changes after review Signed-off-by: soer3n <srenhenning@googlemail.com> --------- Signed-off-by: soer3n <srenhenning@googlemail.com> * fix kubevirt cpu check + update sdk version in go.mod Signed-off-by: soer3n <srenhenning@googlemail.com> revert change regarding import of sdk submodule Signed-off-by: soer3n <srenhenning@googlemail.com> kubevirt resources and vcpus should only be parsed wihtout specified instance type Signed-off-by: soer3n <srenhenning@googlemail.com> --------- Signed-off-by: soer3n <srenhenning@googlemail.com>

change kubevirt vm cpu assignment to not overwrite cpu alloc ratio

4d4b946

Signed-off-by: soer3n <srenhenning@googlemail.com>

soer3n force-pushed the change-kubevirt-vm-resource-assignment branch from e072ff1 to 4d4b946 Compare March 10, 2025 12:21

update kubevirt testdata

8273435

Signed-off-by: soer3n <srenhenning@googlemail.com>

kubermatic-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 10, 2025

soer3n force-pushed the change-kubevirt-vm-resource-assignment branch from 344510c to 33ead46 Compare March 12, 2025 09:38

revert testdata change and add condition for vm cpu assignment

0a2e73c

Signed-off-by: soer3n <srenhenning@googlemail.com>

soer3n force-pushed the change-kubevirt-vm-resource-assignment branch from 33ead46 to 0a2e73c Compare March 12, 2025 10:33

ahmedwaleedmalik changed the title ~~change kubevirt vm cpu assignment to not overwrite cpu alloc ratio~~ Dhange KubeVirt VM CPU assignment to not overwrite cpu alloc ratio Mar 12, 2025

ahmedwaleedmalik changed the title ~~Dhange KubeVirt VM CPU assignment to not overwrite cpu alloc ratio~~ Change KubeVirt VM CPU assignment to not overwrite cpu alloc ratio Mar 12, 2025

ahmedwaleedmalik requested changes Mar 12, 2025

View reviewed changes

kubermatic-bot assigned ahmedwaleedmalik Mar 12, 2025

moadqassem reviewed Mar 12, 2025

View reviewed changes

soer3n added 3 commits March 12, 2025 15:50

switch to providerSpec for enabling vcpu assignment

591d66d

Signed-off-by: soer3n <srenhenning@googlemail.com>

revert unnessecarry changes to mocked kubevirt vm

8b52fcc

Signed-off-by: soer3n <srenhenning@googlemail.com>

soer3n force-pushed the change-kubevirt-vm-resource-assignment branch from 2162ddc to 8b52fcc Compare March 13, 2025 14:53

soer3n requested a review from ahmedwaleedmalik March 13, 2025 15:43

ahmedwaleedmalik requested changes Mar 13, 2025

View reviewed changes

soer3n force-pushed the change-kubevirt-vm-resource-assignment branch from cd442fb to 2d40bf8 Compare March 14, 2025 06:03

changes after review

966a390

Signed-off-by: soer3n <srenhenning@googlemail.com>

soer3n force-pushed the change-kubevirt-vm-resource-assignment branch from 2d40bf8 to 966a390 Compare March 14, 2025 06:09

soer3n requested a review from ahmedwaleedmalik March 14, 2025 06:12

ahmedwaleedmalik approved these changes Mar 14, 2025

View reviewed changes

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2025

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2025

kubermatic-bot merged commit 99a4aa5 into kubermatic:main Mar 14, 2025
12 checks passed

soer3n mentioned this pull request Mar 18, 2025

configure kubevirt virtual machine vcpus at seed level kubermatic/kubermatic#14245

Closed

soer3n mentioned this pull request Apr 4, 2025

[release/v1.61] Cherry pick 1906 to release/v1.61 #1916

Merged

	UseDedicatedKubevirtVCPUAnnotationKey = "kubermatic.io/use-dedicated-kubevirt-cpu"
	UseDedicatedKubeVirtVCPUAnnotationKey = "kubermatic.io/use-dedicated-kubevirt-cpu"


		if isDedicatedVCPURequested(machine.Spec.Annotations) {
		cpusAsUint64, err := strconv.ParseUint(c.CPUs, 0, 64)

		if (c.VCPUs == nil && c.Resources.Cpu() == nil) \|\| (c.VCPUs != nil && c.Resources.Cpu() != nil) {
		return fmt.Errorf("invalid/no cpus configured. Either vpus or cpus have to be configured.")

		Sockets int `json:"sockets,omitempty"`
		Threads int `json:"threads,omitempty"`

	virtualMachine.Spec.Template.Spec.Domain.CPU = &kubevirtcorev1.CPU{
	Cores: c.VCPUs.Cores,
	}

Search code, repositories, users, issues, pull requests...

Conversation

soer3n commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmedwaleedmalik commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soer3n commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmedwaleedmalik commented Mar 13, 2025

Uh oh!

ahmedwaleedmalik left a comment

Choose a reason for hiding this comment

Uh oh!

kubermatic-bot commented Mar 14, 2025

Uh oh!

kubermatic-bot commented Mar 14, 2025

Uh oh!

Uh oh!

soer3n commented Apr 4, 2025

Uh oh!

kubermatic-bot commented Apr 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

soer3n commented Mar 10, 2025 •

edited

Loading

ahmedwaleedmalik commented Mar 12, 2025 •

edited

Loading