Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
rohityadavcloud merged 1 commit into from
Dec 10, 2020
Merged

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

rohityadavcloud merged 1 commit into from
Dec 10, 2020

Conversation

@ustcweizhou
Copy link
Contributor

@ustcweizhou ustcweizhou commented Dec 9, 2020

Description

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.

fyi, in https://libvirt.org/formatdomain.html#cpu-model-and-topology, it says

topology
The topology element specifies requested topology of virtual CPU provided to the guest. Four attributes, sockets, dies, cores, and threads, accept non-zero positive integer values. They refer to the number of CPU sockets per NUMA node, number of dies per socket, number of cores per die, and number of threads per core, respectively. The dies attribute is optional and will default to 1 if omitted, while the other attributes are all mandatory. Hypervisors may require that the maximum number of vCPUs specified by the cpus element equals to the number of vcpus resulting from the topology.

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

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@ustcweizhou
Copy link
Contributor Author

ustcweizhou commented Dec 9, 2020

@rhtyd @DaanHoogland @sureshanaparti

@ustcweizhou ustcweizhou changed the title kvm: set cpu topology only if cpucore per socket is positive value kvm: set cpu topology only if cpucore per socket is set Dec 9, 2020
@rohityadavcloud rohityadavcloud added this to the 4.14.1.0 milestone Dec 9, 2020
@rohityadavcloud
Copy link
Member

rohityadavcloud commented Dec 9, 2020

@blueorangutan package

@rohityadavcloud
Copy link
Member

rohityadavcloud commented Dec 9, 2020

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Dec 9, 2020

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

@blueorangutan
Copy link

blueorangutan commented Dec 9, 2020

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

@rohityadavcloud
Copy link
Member

rohityadavcloud commented Dec 9, 2020

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Dec 9, 2020

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

@rohityadavcloud rohityadavcloud modified the milestones: 4.14.1.0, 4.15.0.0 Dec 9, 2020
@blueorangutan
Copy link

blueorangutan commented Dec 10, 2020

Trillian test result (tid-3324)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29670 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4527-t3324-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
sureshanaparti reviewed Dec 10, 2020
View reviewed changes
...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
cmd.setTopology(numCoresPerSocket, vcpus / numCoresPerSocket);
if (numCoresPerSocket > 0) {
cmd.setTopology(numCoresPerSocket, vcpus / numCoresPerSocket);
}
Copy link
Contributor

@sureshanaparti sureshanaparti Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Dec 10, 2020

@weizhouapache @rhtyd I see how this can bug a purist, but this is not really a critical bug is it? if so it is for milestone 4.14.1 . cc @sureshanaparti . Did i interpret my testing of #4497 wrong?

@weizhouapache
Copy link
Member

weizhouapache commented Dec 10, 2020

@weizhouapache @rhtyd I see how this can bug a purist, but this is not really a critical bug is it? if so it is for milestone 4.14.1 . cc @sureshanaparti . Did i interpret my testing of #4497 wrong?

@DaanHoogland
let me explain what are the behaviors

Assume vm has 3 cpu cores,

  1. in older versions,

(1) by default, there is no cpu topology in vm definition
(2) if add vm setting "cpu.corespersocket" = 1, then in vm definition there should be
<topology sockets='3' cores='1' threads='1'/>
but it is missing.

  1. kvm: FIX cpucorespersocket is not working on KVM #4497 fixes the issue in (2), but it also changes the default setting in (1). there are <topology sockets='3' cores='1' threads='1'/> in both (1) and (2) above.

  2. this pr reset the default setting in (1). there will be no cpu topology in vm definition, if cpu.corespersocket is not set.

I have no idea how critical this issue is.

@rohityadavcloud
Copy link
Member

rohityadavcloud commented Dec 10, 2020

Since the previous PR was already merged, let's merge the regression PR.

@rohityadavcloud rohityadavcloud merged commit 1a47719 into apache:4.14 Dec 10, 2020
@blueorangutan
Copy link

blueorangutan commented Dec 10, 2020

Trillian test result (tid-3329)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30918 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4527-t3329-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

qrry added a commit to qrry/cloudstack that referenced this pull request Dec 23, 2020
@qrry
Merge branch 'master' into dev
7b74988 
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohityadavcloud rohityadavcloud rohityadavcloud approved these changes

@sureshanaparti sureshanaparti sureshanaparti approved these changes

@DaanHoogland DaanHoogland Awaiting requested review from DaanHoogland

Assignees

No one assigned

Labels

Severity:Critical Critical bug type:regression

Projects

None yet

Milestone

4.15.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@ustcweizhou @rohityadavcloud @blueorangutan @DaanHoogland @weizhouapache @sureshanaparti