-
Notifications
You must be signed in to change notification settings - Fork 1.2k
add kvm vcpu socket and threads config #4178
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
Conversation
add kvm support sockets and threads config
|
This feature aims to allow user to specify how many CPU sockets the target VM would have by using the 'cores per socket' parameter. Assume user assign 16 vCPUs to a new VM, without manually specify the 'cores per socket', CloudStack would try to divide the 16 vCPUs by 6, 4 and 1 in order. In this case, CloudStack would assign 4 sockets (16vCPUs /4) to this VM and every scoket has 4 vCPUs. In most cases, it shall casue no issue, however if there's sort of CPU limits due to Guest OS or application, e.g. SQL Sever version, it could turn into a big performance issue. In our real case, user deployed an Windows 10 guest OS with 16 vCPUs, and CloudStack assgins 4 virtual CPU sockets by default. Due to Windows 10 only supports 2 CPU sockets (https://answers.microsoft.com/en-us/windows/forum/windows_10-win_upgrade/windows-10-versions-cpu-limits/905c24ad-ad54-4122-b730-b9e7519c823f?auth=1) , it turns out this guest OS is only able to utilize two sockets (8 vCPUs) even though we have assigned 16 vCPUs in toal. If user has CPU intensive application running on this VM, this could introduce bottelneck. |
GabrielBrascher
left a comment
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.
Thanks for the PR, @div8cn. Overal code LGTM (I have not tested it); however, I see some improvements in terms of extracting a few lines to a method(s) and then adding unit test cases.
This is not a -1, it is a tip of what (I think) can be improved. We are trying to cover tests as much as possible on CloudStack codebase and breaking big methods into smaller is a great way of doing it.
| } else if (vcpus % 4 == 0) { | ||
| final int sockets = vcpus / 4; | ||
| cmd.setTopology(4, sockets); | ||
| } |
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.
What do you think of extracting lines 2300 - 2325 into a method, document, and then create JUnit tests for the new method(s)?
|
@GabrielBrascher @div8cn @luhaijiao what is the status of this PR? |
Description
Supplement the vcpu cores Per Socket configuration of KVM VM
Increase the vcpu hyperthreading configuration of KVM VM
Use vm.details for configuration
Types of changes
Screenshots (if appropriate):
How Has This Been Tested?