Add ability to set cpu.threadspercore similar to existing cpu.corespersocket (#8850)

* Add ability to set cpu.threadspercore similar to existing cpu.corespersocket

* add cpu.threadspercore to VM and template detail options

* Update plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

* add vm detail for KVM

---------

Co-authored-by: Marcus Sorensen <mls@apple.com>
Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

api/src/main/java/com/cloud/vm/VmDetailConstants.java

@@ -19,6 +19,7 @@ package com.cloud.vm;
 public interface VmDetailConstants {
     String KEYBOARD = "keyboard";
     String CPU_CORE_PER_SOCKET = "cpu.corespersocket";
+    String CPU_THREAD_PER_CORE = "cpu.threadspercore";
     String ROOT_DISK_SIZE = "rootdisksize";
     String BOOT_MODE = "boot.mode";
     String NAME_ON_HYPERVISOR= "nameonhypervisor";

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -5149,31 +5149,48 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
         return false;
     }
 
-    private void setCpuTopology(CpuModeDef cmd, int vCpusInDef, Map<String, String> details) {
+    protected void setCpuTopology(CpuModeDef cmd, int vCpusInDef, Map<String, String> details) {
         if (!enableManuallySettingCpuTopologyOnKvmVm) {
             LOGGER.debug(String.format("Skipping manually setting CPU topology on VM's XML due to it is disabled in agent.properties {\"property\": \"%s\", \"value\": %s}.",
               AgentProperties.ENABLE_MANUALLY_SETTING_CPU_TOPOLOGY_ON_KVM_VM.getName(), enableManuallySettingCpuTopologyOnKvmVm));
             return;
         }
-        // multi cores per socket, for larger core configs
-        int numCoresPerSocket = -1;
+
+        int numCoresPerSocket = 1;
+        int numThreadsPerCore = 1;
+
         if (details != null) {
-            final String coresPerSocket = details.get(VmDetailConstants.CPU_CORE_PER_SOCKET);
-            final int intCoresPerSocket = NumbersUtil.parseInt(coresPerSocket, numCoresPerSocket);
-            if (intCoresPerSocket > 0 && vCpusInDef % intCoresPerSocket == 0) {
-                numCoresPerSocket = intCoresPerSocket;
+            numCoresPerSocket = NumbersUtil.parseInt(details.get(VmDetailConstants.CPU_CORE_PER_SOCKET), 1);
+            numThreadsPerCore = NumbersUtil.parseInt(details.get(VmDetailConstants.CPU_THREAD_PER_CORE), 1);
         }
+
+        if ((numCoresPerSocket * numThreadsPerCore) > vCpusInDef) {
+            LOGGER.warn(String.format("cores per socket (%d) * threads per core (%d) exceeds total VM cores. Ignoring extra topology", numCoresPerSocket, numThreadsPerCore));
+            numCoresPerSocket = 1;
+            numThreadsPerCore = 1;
         }
-        if (numCoresPerSocket <= 0) {
+
+        if (vCpusInDef % (numCoresPerSocket * numThreadsPerCore) != 0) {
+            LOGGER.warn(String.format("cores per socket(%d) * threads per core(%d) doesn't divide evenly into total VM cores(%d). Ignoring extra topology", numCoresPerSocket, numThreadsPerCore, vCpusInDef));
+            numCoresPerSocket = 1;
+            numThreadsPerCore = 1;
+        }
+
+        // Set default coupling (makes 4 or 6 core sockets for larger core configs)
+        int numTotalSockets = 1;
+        if (numCoresPerSocket == 1 && numThreadsPerCore == 1) {
             if (vCpusInDef % 6 == 0) {
                 numCoresPerSocket = 6;
             } else if (vCpusInDef % 4 == 0) {
                 numCoresPerSocket = 4;
             }
+            numTotalSockets = vCpusInDef / numCoresPerSocket;
+        } else {
+            int nTotalCores = vCpusInDef / numThreadsPerCore;
+            numTotalSockets = nTotalCores / numCoresPerSocket;
         }
-        if (numCoresPerSocket > 0) {
-            cmd.setTopology(numCoresPerSocket, vCpusInDef / numCoresPerSocket);
-        }
+
+        cmd.setTopology(numCoresPerSocket, numThreadsPerCore, numTotalSockets);
     }
 
     public void setBackingFileFormat(String volPath) {

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java

@@ -545,8 +545,9 @@ public class LibvirtDomainXMLParser {
             }
             final String sockets = getAttrValue("topology", "sockets", cpuModeDefElement);
             final String cores = getAttrValue("topology", "cores", cpuModeDefElement);
-            if (StringUtils.isNotBlank(sockets) && StringUtils.isNotBlank(cores)) {
-                cpuModeDef.setTopology(Integer.parseInt(cores), Integer.parseInt(sockets));
+            final String threads = getAttrValue("topology", "threads", cpuModeDefElement);
+            if (StringUtils.isNotBlank(sockets) && StringUtils.isNotBlank(cores) && StringUtils.isNotBlank(threads)) {
+                cpuModeDef.setTopology(Integer.parseInt(cores), Integer.parseInt(threads), Integer.parseInt(sockets));
             }
         }
     }

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java

@@ -1754,6 +1754,7 @@ public class LibvirtVMDef {
         private String _model;
         private List<String> _features;
         private int _coresPerSocket = -1;
+        private int _threadsPerCore = -1;
         private int _sockets = -1;
 
         public void setMode(String mode) {
@@ -1770,8 +1771,9 @@ public class LibvirtVMDef {
             _model = model;
         }
 
-        public void setTopology(int coresPerSocket, int sockets) {
+        public void setTopology(int coresPerSocket, int threadsPerCore, int sockets) {
             _coresPerSocket = coresPerSocket;
+            _threadsPerCore = threadsPerCore;
             _sockets = sockets;
         }
 
@@ -1800,9 +1802,9 @@ public class LibvirtVMDef {
                 }
             }
 
-            // add topology
-            if (_sockets > 0 && _coresPerSocket > 0) {
-                modeBuilder.append("<topology sockets='" + _sockets + "' cores='" + _coresPerSocket + "' threads='1' />");
+            // add topology. Note we require sockets, cores, and threads defined
+            if (_sockets > 0 && _coresPerSocket > 0 && _threadsPerCore > 0) {
+                modeBuilder.append("<topology sockets='" + _sockets + "' cores='" + _coresPerSocket + "' threads='"  + _threadsPerCore + "' />");
             }
 
             // close cpu def
@@ -1813,6 +1815,8 @@ public class LibvirtVMDef {
         public int getCoresPerSocket() {
             return _coresPerSocket;
         }
+        public int getThreadsPerCore() { return _threadsPerCore; }
+        public int getSockets() { return _sockets; }
     }
 
     public static class SerialDef {

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtCpuTopologyTest.java

@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.cloud.hypervisor.kvm.resource;
+
+import com.cloud.vm.VmDetailConstants;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+@RunWith(value = Parameterized.class)
+public class LibvirtCpuTopologyTest {
+    private final LibvirtComputingResource libvirtComputingResource = Mockito.spy(new LibvirtComputingResource());
+
+    private final String desc;
+    private final Integer coresPerSocket;
+    private final Integer threadsPerCore;
+    private final Integer totalVmCores;
+    private final String expectedXml;
+
+    public LibvirtCpuTopologyTest(String desc, Integer coresPerSocket, Integer threadsPerCore, Integer totalVmCores, String expectedXml) {
+        this.desc = desc;
+        this.coresPerSocket = coresPerSocket;
+        this.threadsPerCore = threadsPerCore;
+        this.totalVmCores = totalVmCores;
+        this.expectedXml = expectedXml;
+    }
+    @Parameterized.Parameters
+    public static Collection<Object[]> data() {
+        return Arrays.asList(new Object[][] {
+                createTestData("8 cores, 2 per socket",2, null, 8, "<cpu><topology sockets='4' cores='2' threads='1' /></cpu>"),
+                createTestData("8 cores, 4 per socket",4, null, 8, "<cpu><topology sockets='2' cores='4' threads='1' /></cpu>"),
+                createTestData("8 cores, nothing specified",null, null, 8, "<cpu><topology sockets='2' cores='4' threads='1' /></cpu>"),
+                createTestData("12 cores, nothing specified",null, null, 12, "<cpu><topology sockets='2' cores='6' threads='1' /></cpu>"),
+                createTestData("8 cores, 2C per socket, 2TPC",2, 2, 8, "<cpu><topology sockets='2' cores='2' threads='2' /></cpu>"),
+                createTestData("8 cores, 1C per socket, 2TPC",1, 2, 8, "<cpu><topology sockets='4' cores='1' threads='2' /></cpu>"),
+                createTestData("8 cores, default CPS, 2TPC",null, 2, 8, "<cpu><topology sockets='4' cores='1' threads='2' /></cpu>"),
+                createTestData("6 cores, default CPS, 2TPC",null, 2, 6, "<cpu><topology sockets='3' cores='1' threads='2' /></cpu>"),
+                createTestData("12 cores, 2CPS, 2TPC",2, 2, 12, "<cpu><topology sockets='3' cores='2' threads='2' /></cpu>"),
+                createTestData("6 cores, misconfigured cores, CPS, TPC, use default topology",2, 2, 6, "<cpu><topology sockets='1' cores='6' threads='1' /></cpu>"),
+                createTestData("odd cores, nothing specified use default topology",null, null, 3, "<cpu><topology sockets='3' cores='1' threads='1' /></cpu>"),
+                createTestData("odd cores, uneven CPS use default topology",2, null, 3, "<cpu><topology sockets='3' cores='1' threads='1' /></cpu>"),
+                createTestData("8 cores, 2 CPS, odd threads use default topology", 2, 3, 8, "<cpu><topology sockets='2' cores='4' threads='1' /></cpu>"),
+                createTestData("1 core, 2 CPS, odd threads use default topology", 2, 1, 1, "<cpu><topology sockets='1' cores='1' threads='1' /></cpu>")
+        });
+    }
+
+    private static Object[] createTestData(String desc, Integer coresPerSocket, Integer threadsPerCore, int totalVmCores, String expected) {
+        return new Object[] {desc, coresPerSocket, threadsPerCore, totalVmCores, expected};
+    }
+
+    @Test
+    public void topologyTest() {
+        LibvirtVMDef.CpuModeDef cpuModeDef = new LibvirtVMDef.CpuModeDef();
+        Map<String, String> details = new HashMap<>();
+
+        if (coresPerSocket != null) {
+            details.put(VmDetailConstants.CPU_CORE_PER_SOCKET, coresPerSocket.toString());
+        }
+
+        if (threadsPerCore != null) {
+            details.put(VmDetailConstants.CPU_THREAD_PER_CORE, threadsPerCore.toString());
+        }
+
+        if (coresPerSocket == null && threadsPerCore == null) {
+            details = null;
+        }
+
+        libvirtComputingResource.setCpuTopology(cpuModeDef, totalVmCores, details);
+        Assert.assertEquals(desc, expectedXml, cpuModeDef.toString());
+    }
+}

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParserTest.java

@@ -31,6 +31,7 @@ import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.WatchDogDef;
 
 import junit.framework.TestCase;
 import org.apache.cloudstack.utils.qemu.QemuObject;
+import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.junit.MockitoJUnitRunner;
@@ -267,7 +268,7 @@ public class LibvirtDomainXMLParserTest extends TestCase {
                 "  <uuid>aafaaabc-8657-4efc-9c52-3422d4e04088</uuid>\n" +
                 "  <memory unit='KiB'>2097152</memory>\n" +
                 "  <currentMemory unit='KiB'>2097152</currentMemory>\n" +
-                "  <vcpu placement='static'>2</vcpu>\n" +
+                "  <vcpu placement='static'>8</vcpu>\n" +
                 "  <os>\n" +
                 "    <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>\n" +
                 "    <boot dev='hd'/>\n" +
@@ -278,6 +279,7 @@ public class LibvirtDomainXMLParserTest extends TestCase {
                 "  </features>\n" +
                 "  <cpu mode='host-model' check='partial'>\n" +
                 "    <model fallback='allow'/>\n" +
+                "    <topology sockets='1' cores='4' threads='2' />" +
                 "  </cpu>\n" +
                 "  <clock offset='utc'>\n" +
                 "    <timer name='rtc' tickpolicy='catchup'/>\n" +
@@ -380,5 +382,8 @@ public class LibvirtDomainXMLParserTest extends TestCase {
             System.out.println("Got exception " + e.getMessage());
             throw e;
         }
+        Assert.assertEquals("CPU socket count is parsed", 1, libvirtDomainXMLParser.getCpuModeDef().getSockets());
+        Assert.assertEquals("CPU cores count is parsed", 4, libvirtDomainXMLParser.getCpuModeDef().getCoresPerSocket());
+        Assert.assertEquals("CPU threads count is parsed", 2, libvirtDomainXMLParser.getCpuModeDef().getThreadsPerCore());
     }
 }

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java

@@ -547,4 +547,17 @@ public class LibvirtVMDefTest extends TestCase {
                 "</controller>\n";
         assertEquals(expected, str);
     }
+
+    @Test
+    public void testTopology() {
+        LibvirtVMDef.CpuModeDef cpuModeDef = new LibvirtVMDef.CpuModeDef();
+        cpuModeDef.setTopology(2, 1, 4);
+        assertEquals("<cpu><topology sockets='4' cores='2' threads='1' /></cpu>", cpuModeDef.toString());
+    }
+
+    public void testTopologyNoInfo() {
+        LibvirtVMDef.CpuModeDef cpuModeDef = new LibvirtVMDef.CpuModeDef();
+        cpuModeDef.setTopology(-1, -1, 4);
+        assertEquals("<cpu></cpu>", cpuModeDef.toString());
+    }
 }

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

@@ -4910,6 +4910,7 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q
         options.put(VmDetailConstants.ROOT_DISK_SIZE, Collections.emptyList());
 
         if (HypervisorType.KVM.equals(hypervisorType)) {
+            options.put(VmDetailConstants.CPU_THREAD_PER_CORE, Collections.emptyList());
             options.put(VmDetailConstants.NIC_ADAPTER, Arrays.asList("e1000", "virtio", "rtl8139", "vmxnet3", "ne2k_pci"));
             options.put(VmDetailConstants.ROOT_DISK_CONTROLLER, Arrays.asList("osdefault", "ide", "scsi", "virtio"));
             options.put(VmDetailConstants.VIDEO_HARDWARE, Arrays.asList("cirrus", "vga", "qxl", "virtio"));