kvm: FIX cpucorespersocket is not working on KVM #4497

ustcweizhou · 2020-11-23T15:11:50Z

Description

This PR fixes the issue that cpu.corespersocket in vm details is not working on kvm.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

rohityadavcloud · 2020-11-25T10:05:01Z

@blueorangutan package

blueorangutan · 2020-11-25T10:06:21Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-11-25T11:08:51Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2435

sureshanaparti · 2020-11-26T12:53:46Z

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

+        if (numCoresPerSocket <= 0) {
+            if (vcpus % 6 == 0) {
+                numCoresPerSocket = 6;
+            } else if (vcpus % 4 == 0) {


when vcpus are 8, it is considered as 4 cores per socket here, instead 8 cores.

@sureshanaparti this is the current behavior.

@sureshanaparti this is the current behavior.

agree, any plan to address here?

@sureshanaparti you mean change the current behavior ? No. I am not going to change it
It worked since cloudstack 4.13 in commit 2f53295
https://issues.apache.org/jira/browse/CLOUDSTACK-5521

@sureshanaparti you mean change the current behavior ? No. I am not going to change it
It worked since cloudstack 4.13 in commit 2f53295
https://issues.apache.org/jira/browse/CLOUDSTACK-5521

ok @weizhouapache so no change in the behavior. thanks.

rohityadavcloud · 2020-11-27T08:53:50Z

@blueorangutan test

blueorangutan · 2020-11-27T08:54:30Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-11-27T17:57:02Z

Trillian test result (tid-3275)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30597 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4497-t3275-kvm-centos7.zip
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti · 2020-12-09T07:18:56Z

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

        return false;
    }
+
+    private void setCpuTopology(CpuModeDef cmd, int vcpus, Map<String, String> details) {


@ustcweizhou possible to pass coresPerSocket here, instead complete VM details here ?

@sureshanaparti
I was thinking about it. at the end I decided to pass vm details based on two reasons
(1) setCpuTopology is a complete function now. if pass coresPerSocket , we need to get coresPerSocket out of the method.
(2) it is better if add more cpu settings , for example the pr #4178

@sureshanaparti
I was thinking about it. at the end I decided to pass vm details based on two reasons
(1) setCpuTopology is a complete function now. if pass coresPerSocket , we need to get coresPerSocket out of the method.
(2) it is better if add more cpu settings , for example the pr #4178

Ok @weizhouapache so going forward, cpu hyperthreading detail mentioned in PR #4178 will be added to this method, right?

@sureshanaparti yes. but I need to discuss with @div8cn about some details. it will be an improvement in 4.16

@sureshanaparti yes. but I need to discuss with @div8cn about some details. it will be an improvement in 4.16

OK, Pls update details here. As this PR is targeted for 4.14.1.0, the proposed changes with cpu hyperthreading detail will be targeted for 4.16, Right?

@sureshanaparti yes, this is a bug which should be fixed in 4.14 and 4.15.
the other pr is an improvement. it will not impact this pr.

DaanHoogland · 2020-12-09T14:07:28Z

i have an offering with 3 cores 256 MHz and get a vm with

...
  <vcpu placement='static'>3</vcpu>
  <cputune>
    <shares>384</shares>
  </cputune>
...
  <cpu>
    <topology sockets='3' cores='1' threads='1'/>
  </cpu>

default was

...
  <vcpu placement='static'>1</vcpu>
  <cputune>
    <shares>250</shares>
  </cputune>
...
  <cpu>
    <topology sockets='1' cores='1' threads='1'/>
  </cpu>
...
...

QED

weizhouapache · 2020-12-09T14:12:57Z

i have an offering with 3 cores 256 MHz and get a vm with

...
  <vcpu placement='static'>3</vcpu>
  <cputune>
    <shares>384</shares>
  </cputune>
...
  <cpu>
    <topology sockets='3' cores='1' threads='1'/>
  </cpu>

default was

...
  <vcpu placement='static'>1</vcpu>
  <cputune>
    <shares>250</shares>
  </cputune>
...
  <cpu>
    <topology sockets='1' cores='1' threads='1'/>
  </cpu>
...
...

QED

@DaanHoogland wait,it might be a bug............

rohityadavcloud · 2020-12-09T14:15:28Z

@weizhouapache can you investigate and send a PR fix asap, or revert?

weizhouapache · 2020-12-09T14:28:17Z

@weizhouapache can you investigate and send a PR fix asap, or revert?

@rhtyd I will create a pr soon.

…4527) This PR fixes a regression issue in #4497 In cloudstack 4.14 or before, the cpu topology is set only when cpucore per socket is set (to 4 or 6). in other conditions, there is no cpu topology in vm xml definition. with #4497, vm will have cpu topology in its xml definition, if cpucore per socket is not set. <topology sockets='<vm cpu cores>' cores='1' threads='1'/> Not sure if it causes any issue. I think it would be better not to add this part in vm xml definition if cpucore per socket is not set.

* master: server: add conditions for custom offerings (apache#4540) vr: Ensuring dnsmasq.leases file is populated (apache#4529) template: Ensuring template is cross zone if type changed to system (apache#4522) storage: Fix hypervisor type cast to string (apache#4516) db upgrade: fix sql exception: Access denied; you need (at least one of) the SUPER privilege(s) for this operation (apache#4533) CLOUDSTACK-10423:Potential sensitive information disclosure (apache#4536) jobs: The patch remove the password from resultObject and make it be humanreadable (apache#4538) listphysicalnetworks: Honouring keyword parameter (apache#4511) Fix NPE when Volume exists on secondary store but doesn't have a download URL (apache#4530) apidoc issue (apache#4532) db: Fix description of volume.stats.interval which is in milliseconds not seconds (apache#4526) kvm: set cpu topology only if cpucore per socket is positive value (apache#4527) xenserver: check and eject patch vbd for systemvms (apache#4525) Fix warning when setup cloudstack-common (apache#4523) kvm: FIX cpucorespersocket is not working on KVM (apache#4497) change debug to warn for unknown exceptions (apache#4521) Fix failure in validating IP address in case of multiple Management Servers (apache#4507) Update log output for FirstFitPlanner (apache#4515) ui: deprecate old UI and move to legacy to be served at /client/legacy (apache#4518)

ustcweizhou added 2 commits November 23, 2020 15:10

kvm: FIX cpucorespersocket is not working on KVM

1c6c474

Add method setCpuTopology

f200980

weizhouapache mentioned this pull request Nov 24, 2020

add kvm vcpu socket and threads config #4178

Closed

5 tasks

rohityadavcloud requested a review from nvazquez November 24, 2020 11:09

rohityadavcloud added this to the 4.14.1.0 milestone Nov 24, 2020

DaanHoogland mentioned this pull request Nov 25, 2020

CPU cores per socket setting not functional #2583

Closed

sureshanaparti reviewed Nov 26, 2020

View reviewed changes

rohityadavcloud requested a review from DaanHoogland December 8, 2020 08:10

DaanHoogland approved these changes Dec 8, 2020

View reviewed changes

sureshanaparti reviewed Dec 9, 2020

View reviewed changes

sureshanaparti approved these changes Dec 9, 2020

View reviewed changes

DaanHoogland merged commit 93f3d35 into apache:4.14 Dec 9, 2020

ustcweizhou mentioned this pull request Dec 9, 2020

kvm: set cpu topology only if cpucore per socket is set #4527

Merged

10 tasks

kvm: FIX cpucorespersocket is not working on KVM #4497

kvm: FIX cpucorespersocket is not working on KVM #4497

Uh oh!

Conversation

ustcweizhou commented Nov 23, 2020

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

rohityadavcloud commented Nov 25, 2020

Uh oh!

blueorangutan commented Nov 25, 2020

Uh oh!

blueorangutan commented Nov 25, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Nov 27, 2020

Uh oh!

blueorangutan commented Nov 27, 2020

Uh oh!

blueorangutan commented Nov 27, 2020

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!

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!

DaanHoogland commented Dec 9, 2020

Uh oh!

weizhouapache commented Dec 9, 2020

Uh oh!

rohityadavcloud commented Dec 9, 2020

Uh oh!

weizhouapache commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants