You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/06/25 13:11:50 UTC
[GitHub] [cloudstack] div8cn opened a new pull request #4178: add kvm vcpu socket and threads config
div8cn opened a new pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178
## Description
<!--- Describe your changes in detail -->
Supplement the vcpu cores Per Socket configuration of KVM VM
Increase the vcpu hyperthreading configuration of KVM VM
Use vm.details for configuration
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] luhaijiao commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
luhaijiao commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-650724813
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-732741576
#4497
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712785827
@GabrielBrascher @div8cn @luhaijiao what is the status of this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712849788
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] div8cn closed pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
div8cn closed pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712849247
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712786100
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712786100
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712786656
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712849247
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#discussion_r447212031
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2297,12 +2297,32 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
cmd.setFeatures(_cpuFeatures);
}
// multi cores per socket, for larger core configs
- if (vcpus % 6 == 0) {
- final int sockets = vcpus / 6;
- cmd.setTopology(6, sockets);
- } else if (vcpus % 4 == 0) {
- final int sockets = vcpus / 4;
- cmd.setTopology(4, sockets);
+ String coresPerSocket = null;
+ Boolean coresthreads = null;
+ if (vmTO.getDetails() != null) {
+ coresPerSocket = vmTO.getDetails().get(VmDetailConstants.CPU_CORE_PER_SOCKET);
+ coresthreads = Boolean.valueOf(vmTO.getDetails().get(VmDetailConstants.CPU_HYPERTHREADING));
+
+ s_logger.debug("user custom set coresPerSocket:" + coresPerSocket + ", coresThreads:" + coresthreads + " , vmTO:" + vmTO.getUuid());
+ }
+ if (coresPerSocket != null) {
+ final int numCoresPerSocket = NumbersUtil.parseInt(coresPerSocket, 1);
+ if (vcpus % numCoresPerSocket == 0) {
+ final int sockets = vcpus / numCoresPerSocket;
+ if(coresthreads && numCoresPerSocket % 2 == 0) {
+ cmd.setTopology(numCoresPerSocket /2, sockets,2);
+ } else {
+ cmd.setTopology(numCoresPerSocket, sockets);
+ }
+ }
+ } else {
+ if (vcpus % 6 == 0) {
+ final int sockets = vcpus / 6;
+ cmd.setTopology(6, sockets);
+ } else if (vcpus % 4 == 0) {
+ final int sockets = vcpus / 4;
+ cmd.setTopology(4, sockets);
+ }
Review comment:
What do you think of extracting lines 2300 - 2325 into a method, document, and then create JUnit tests for the new method(s)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712849788
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712844379
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2223
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#discussion_r447212031
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2297,12 +2297,32 @@ public LibvirtVMDef createVMFromSpec(final VirtualMachineTO vmTO) {
cmd.setFeatures(_cpuFeatures);
}
// multi cores per socket, for larger core configs
- if (vcpus % 6 == 0) {
- final int sockets = vcpus / 6;
- cmd.setTopology(6, sockets);
- } else if (vcpus % 4 == 0) {
- final int sockets = vcpus / 4;
- cmd.setTopology(4, sockets);
+ String coresPerSocket = null;
+ Boolean coresthreads = null;
+ if (vmTO.getDetails() != null) {
+ coresPerSocket = vmTO.getDetails().get(VmDetailConstants.CPU_CORE_PER_SOCKET);
+ coresthreads = Boolean.valueOf(vmTO.getDetails().get(VmDetailConstants.CPU_HYPERTHREADING));
+
+ s_logger.debug("user custom set coresPerSocket:" + coresPerSocket + ", coresThreads:" + coresthreads + " , vmTO:" + vmTO.getUuid());
+ }
+ if (coresPerSocket != null) {
+ final int numCoresPerSocket = NumbersUtil.parseInt(coresPerSocket, 1);
+ if (vcpus % numCoresPerSocket == 0) {
+ final int sockets = vcpus / numCoresPerSocket;
+ if(coresthreads && numCoresPerSocket % 2 == 0) {
+ cmd.setTopology(numCoresPerSocket /2, sockets,2);
+ } else {
+ cmd.setTopology(numCoresPerSocket, sockets);
+ }
+ }
+ } else {
+ if (vcpus % 6 == 0) {
+ final int sockets = vcpus / 6;
+ cmd.setTopology(6, sockets);
+ } else if (vcpus % 4 == 0) {
+ final int sockets = vcpus / 4;
+ cmd.setTopology(4, sockets);
+ }
Review comment:
What do you think of extracting lines 2300 - 2325 into a method? That would allow adding Javadoc and UNIT test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4178: add kvm vcpu socket and threads config
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4178:
URL: https://github.com/apache/cloudstack/pull/4178#issuecomment-712786656
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org