From 227dc5e86aaec2fff5f6329c46ee0624d626665e Mon Sep 17 00:00:00 2001 From: Marcus Sorensen Date: Wed, 10 Apr 2024 08:58:03 -0700 Subject: [PATCH] Add ability to set cpu.threadspercore similar to existing cpu.corespersocket (#411) * Add ability to set cpu.threadspercore similar to existing cpu.corespersocket * Add license to new test file * Add tests to handle some edge cases * Add some edge test cases to CPU topology * Rework logic on KVM CPU topology, handle more cases * Add more test cases * Add more test cases * Update plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java Co-authored-by: Suresh Kumar Anaparti * Added cpu.threadspercore detail in listDetailOptions response (for KVM hypervisor) --------- Co-authored-by: Marcus Sorensen Co-authored-by: Suresh Kumar Anaparti --- .../java/com/cloud/vm/VmDetailConstants.java | 1 + .../resource/LibvirtComputingResource.java | 41 +++++--- .../hypervisor/kvm/resource/LibvirtVMDef.java | 10 +- .../kvm/resource/LibvirtCpuTopologyTest.java | 94 +++++++++++++++++++ .../kvm/resource/LibvirtVMDefTest.java | 13 +++ .../com/cloud/api/query/QueryManagerImpl.java | 1 + 6 files changed, 144 insertions(+), 16 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtCpuTopologyTest.java diff --git a/api/src/main/java/com/cloud/vm/VmDetailConstants.java b/api/src/main/java/com/cloud/vm/VmDetailConstants.java index dd0a5d754bb..754145ffa78 100644 --- a/api/src/main/java/com/cloud/vm/VmDetailConstants.java +++ b/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"; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index a6ed9f6e8fd..4ce11287029 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -5024,31 +5024,48 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv return false; } - private void setCpuTopology(CpuModeDef cmd, int vCpusInDef, Map details) { + protected void setCpuTopology(CpuModeDef cmd, int vCpusInDef, Map details) { if (!enableManuallySettingCpuTopologyOnKvmVm) { s_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 <= 0) { + + if ((numCoresPerSocket * numThreadsPerCore) > vCpusInDef) { + s_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 (vCpusInDef % (numCoresPerSocket * numThreadsPerCore) != 0) { + s_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) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java index 26ef52eae2f..991f7e19708 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java @@ -1716,6 +1716,7 @@ public class LibvirtVMDef { private String _model; private List _features; private int _coresPerSocket = -1; + private int _threadsPerCore = -1; private int _sockets = -1; public void setMode(String mode) { @@ -1732,8 +1733,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; } @@ -1762,9 +1764,9 @@ public class LibvirtVMDef { } } - // add topology - if (_sockets > 0 && _coresPerSocket > 0) { - modeBuilder.append(""); + // add topology. Note we require sockets, cores, and threads defined + if (_sockets > 0 && _coresPerSocket > 0 && _threadsPerCore > 0) { + modeBuilder.append(""); } // close cpu def diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtCpuTopologyTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtCpuTopologyTest.java new file mode 100644 index 00000000000..f5234fc5ae3 --- /dev/null +++ b/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 data() { + return Arrays.asList(new Object[][] { + createTestData("8 cores, 2 per socket",2, null, 8, ""), + createTestData("8 cores, 4 per socket",4, null, 8, ""), + createTestData("8 cores, nothing specified",null, null, 8, ""), + createTestData("12 cores, nothing specified",null, null, 12, ""), + createTestData("8 cores, 2C per socket, 2TPC",2, 2, 8, ""), + createTestData("8 cores, 1C per socket, 2TPC",1, 2, 8, ""), + createTestData("8 cores, default CPS, 2TPC",null, 2, 8, ""), + createTestData("6 cores, default CPS, 2TPC",null, 2, 6, ""), + createTestData("12 cores, 2CPS, 2TPC",2, 2, 12, ""), + createTestData("6 cores, misconfigured cores, CPS, TPC, use default topology",2, 2, 6, ""), + createTestData("odd cores, nothing specified use default topology",null, null, 3, ""), + createTestData("odd cores, uneven CPS use default topology",2, null, 3, ""), + createTestData("8 cores, 2 CPS, odd threads use default topology", 2, 3, 8, ""), + createTestData("1 core, 2 CPS, odd threads use default topology", 2, 1, 1, "") + }); + } + + 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 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()); + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java index 482a3e1acbe..c515db984b2 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java @@ -506,4 +506,17 @@ public class LibvirtVMDefTest extends TestCase { "\n"; assertEquals(expected, str); } + + @Test + public void testTopology() { + LibvirtVMDef.CpuModeDef cpuModeDef = new LibvirtVMDef.CpuModeDef(); + cpuModeDef.setTopology(2, 1, 4); + assertEquals("", cpuModeDef.toString()); + } + + public void testTopologyNoInfo() { + LibvirtVMDef.CpuModeDef cpuModeDef = new LibvirtVMDef.CpuModeDef(); + cpuModeDef.setTopology(-1, -1, 4); + assertEquals("", cpuModeDef.toString()); + } } diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index c1b21104e1c..5dcf95fdeb1 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -4641,6 +4641,7 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q options.put(VmDetailConstants.VIDEO_RAM, Collections.emptyList()); options.put(VmDetailConstants.IO_POLICY, Arrays.asList("threads", "native", "io_uring", "storage_specific")); options.put(VmDetailConstants.IOTHREADS, Arrays.asList("enabled")); + options.put(VmDetailConstants.CPU_THREAD_PER_CORE, Collections.emptyList()); } if (HypervisorType.VMware.equals(hypervisorType)) {