diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 815ac4f70fe8..615192119836 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -54,7 +54,6 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.ParserConfigurationException; -import com.cloud.network.NetworkService; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; @@ -134,8 +133,8 @@ import org.apache.cloudstack.userdata.UserDataManager; import org.apache.cloudstack.utils.bytescale.ByteScaleUtils; import org.apache.cloudstack.utils.security.ParserUtils; -import org.apache.cloudstack.vm.schedule.VMScheduleManager; import org.apache.cloudstack.vm.UnmanagedVMsManager; +import org.apache.cloudstack.vm.schedule.VMScheduleManager; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; import org.apache.commons.lang.math.NumberUtils; @@ -254,6 +253,7 @@ import com.cloud.network.Network.Provider; import com.cloud.network.Network.Service; import com.cloud.network.NetworkModel; +import com.cloud.network.NetworkService; import com.cloud.network.Networks.TrafficType; import com.cloud.network.PhysicalNetwork; import com.cloud.network.as.AutoScaleManager; @@ -1283,46 +1283,45 @@ private void validateOfferingMaxResource(ServiceOfferingVO offering) { @Override public void validateCustomParameters(ServiceOfferingVO serviceOffering, Map customParameters) { - //TODO need to validate custom cpu, and memory against min/max CPU/Memory ranges from service_offering_details table - if (customParameters.size() != 0) { - Map offeringDetails = serviceOfferingDetailsDao.listDetailsKeyPairs(serviceOffering.getId()); - if (serviceOffering.getCpu() == null) { - int minCPU = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MIN_CPU_NUMBER), 1); - int maxCPU = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MAX_CPU_NUMBER), Integer.MAX_VALUE); - int cpuNumber = NumbersUtil.parseInt(customParameters.get(UsageEventVO.DynamicParameters.cpuNumber.name()), -1); - Integer maxCPUCores = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES.value() == 0 ? Integer.MAX_VALUE: ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES.value(); - if (cpuNumber < minCPU || cpuNumber > maxCPU || cpuNumber > maxCPUCores) { - throw new InvalidParameterValueException(String.format("Invalid CPU cores value, specify a value between %d and %d", minCPU, Math.min(maxCPUCores, maxCPU))); - } - } else if (customParameters.containsKey(UsageEventVO.DynamicParameters.cpuNumber.name())) { - throw new InvalidParameterValueException("The CPU cores of this offering id:" + serviceOffering.getUuid() - + " is not customizable. This is predefined in the Template."); - } - - if (serviceOffering.getSpeed() == null) { - String cpuSpeed = customParameters.get(UsageEventVO.DynamicParameters.cpuSpeed.name()); - if ((cpuSpeed == null) || (NumbersUtil.parseInt(cpuSpeed, -1) <= 0)) { - throw new InvalidParameterValueException("Invalid CPU speed value, specify a value between 1 and " + Integer.MAX_VALUE); - } - } else if (!serviceOffering.isCustomCpuSpeedSupported() && customParameters.containsKey(UsageEventVO.DynamicParameters.cpuSpeed.name())) { - throw new InvalidParameterValueException("The CPU speed of this offering id:" + serviceOffering.getUuid() - + " is not customizable. This is predefined in the Template."); - } - - if (serviceOffering.getRamSize() == null) { - int minMemory = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MIN_MEMORY), 32); - int maxMemory = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MAX_MEMORY), Integer.MAX_VALUE); - int memory = NumbersUtil.parseInt(customParameters.get(UsageEventVO.DynamicParameters.memory.name()), -1); - Integer maxRAMSize = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_RAM_SIZE.value() == 0 ? Integer.MAX_VALUE: ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_RAM_SIZE.value(); - if (memory < minMemory || memory > maxMemory || memory > maxRAMSize) { - throw new InvalidParameterValueException(String.format("Invalid memory value, specify a value between %d and %d", minMemory, Math.min(maxRAMSize, maxMemory))); - } - } else if (customParameters.containsKey(UsageEventVO.DynamicParameters.memory.name())) { - throw new InvalidParameterValueException("The memory of this offering id:" + serviceOffering.getUuid() + " is not customizable. This is predefined in the Template."); - } - } else { + if (MapUtils.isEmpty(customParameters) && serviceOffering.isDynamic()) { throw new InvalidParameterValueException("Need to specify custom parameter values cpu, cpu speed and memory when using custom offering"); } + Map offeringDetails = serviceOfferingDetailsDao.listDetailsKeyPairs(serviceOffering.getId()); + if (serviceOffering.getCpu() == null) { + int minCPU = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MIN_CPU_NUMBER), 1); + int maxCPU = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MAX_CPU_NUMBER), Integer.MAX_VALUE); + int cpuNumber = NumbersUtil.parseInt(customParameters.get(UsageEventVO.DynamicParameters.cpuNumber.name()), -1); + int maxCPUCores = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES.value() == 0 ? Integer.MAX_VALUE: ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES.value(); + if (cpuNumber < minCPU || cpuNumber > maxCPU || cpuNumber > maxCPUCores) { + throw new InvalidParameterValueException(String.format("Invalid CPU cores value, specify a value between %d and %d", minCPU, Math.min(maxCPUCores, maxCPU))); + } + } else if (customParameters.containsKey(UsageEventVO.DynamicParameters.cpuNumber.name())) { + throw new InvalidParameterValueException("The CPU cores of this offering id:" + serviceOffering.getUuid() + + " is not customizable. This is predefined in the Template."); + } + + if (serviceOffering.getSpeed() == null) { + String cpuSpeed = customParameters.get(UsageEventVO.DynamicParameters.cpuSpeed.name()); + if ((cpuSpeed == null) || (NumbersUtil.parseInt(cpuSpeed, -1) <= 0)) { + throw new InvalidParameterValueException("Invalid CPU speed value, specify a value between 1 and " + Integer.MAX_VALUE); + } + } else if (!serviceOffering.isCustomCpuSpeedSupported() && customParameters.containsKey(UsageEventVO.DynamicParameters.cpuSpeed.name())) { + throw new InvalidParameterValueException(String.format("The CPU speed of this offering id:%s" + + " is not customizable. This is predefined as %d MHz.", + serviceOffering.getUuid(), serviceOffering.getSpeed())); + } + + if (serviceOffering.getRamSize() == null) { + int minMemory = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MIN_MEMORY), 32); + int maxMemory = NumbersUtil.parseInt(offeringDetails.get(ApiConstants.MAX_MEMORY), Integer.MAX_VALUE); + int memory = NumbersUtil.parseInt(customParameters.get(UsageEventVO.DynamicParameters.memory.name()), -1); + int maxRAMSize = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_RAM_SIZE.value() == 0 ? Integer.MAX_VALUE: ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_RAM_SIZE.value(); + if (memory < minMemory || memory > maxMemory || memory > maxRAMSize) { + throw new InvalidParameterValueException(String.format("Invalid memory value, specify a value between %d and %d", minMemory, Math.min(maxRAMSize, maxMemory))); + } + } else if (customParameters.containsKey(UsageEventVO.DynamicParameters.memory.name())) { + throw new InvalidParameterValueException("The memory of this offering id:" + serviceOffering.getUuid() + " is not customizable. This is predefined in the Template."); + } } private UserVm upgradeStoppedVirtualMachine(Long vmId, Long svcOffId, Map customParameters) throws ResourceAllocationException { @@ -2765,10 +2764,16 @@ protected void verifyVmLimits(UserVmVO vmInstance, Map details) Map customParameters = new HashMap<>(); customParameters.put(VmDetailConstants.CPU_NUMBER, String.valueOf(newCpu)); customParameters.put(VmDetailConstants.MEMORY, String.valueOf(newMemory)); - if (svcOffering.isCustomCpuSpeedSupported()) { + if (details.containsKey(VmDetailConstants.CPU_SPEED)) { customParameters.put(VmDetailConstants.CPU_SPEED, details.get(VmDetailConstants.CPU_SPEED)); } validateCustomParameters(svcOffering, customParameters); + } else { + if (details.containsKey(VmDetailConstants.CPU_NUMBER) || details.containsKey(VmDetailConstants.MEMORY) || + details.containsKey(VmDetailConstants.CPU_SPEED)) { + throw new InvalidParameterValueException("CPU number, Memory and CPU speed cannot be updated for a " + + "non-dynamic offering"); + } } if (VirtualMachineManager.ResourceCountRunningVMsonly.value()) { return; diff --git a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java index 07e19d99f394..c0f33d8343b2 100644 --- a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java +++ b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java @@ -16,6 +16,25 @@ // under the License. package com.cloud.hypervisor; +import java.io.UnsupportedEncodingException; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.utils.bytescale.ByteScaleUtils; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + import com.cloud.agent.api.to.NicTO; import com.cloud.agent.api.to.VirtualMachineTO; import com.cloud.configuration.ConfigurationManagerImpl; @@ -34,23 +53,6 @@ import com.cloud.utils.Pair; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; -import org.apache.cloudstack.api.ApiConstants; -import org.apache.cloudstack.framework.config.ConfigKey; -import org.apache.cloudstack.utils.bytescale.ByteScaleUtils; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.Spy; -import org.mockito.junit.MockitoJUnitRunner; - -import java.io.UnsupportedEncodingException; -import java.util.Arrays; -import java.util.HashMap; -import java.util.Map; @RunWith(MockitoJUnitRunner.class) public class KVMGuruTest { @@ -111,8 +113,15 @@ public class KVMGuruTest { private static final String detail2Key = "detail2"; private static final String detail2Value = "value2"; + private ConfigKey originalVmServiceOfferingMaxCpuCores; + private ConfigKey originalVmServiceOfferingMaxRAMSize; + @Before public void setup() throws UnsupportedEncodingException { + // Preserve the original value for restoration in tearDown + originalVmServiceOfferingMaxCpuCores = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES; + originalVmServiceOfferingMaxRAMSize = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_RAM_SIZE; + Mockito.when(vmTO.getLimitCpuUse()).thenReturn(true); Mockito.when(vmProfile.getVirtualMachine()).thenReturn(vm); Mockito.when(vm.getHostId()).thenReturn(hostId); @@ -134,6 +143,13 @@ public void setup() throws UnsupportedEncodingException { Arrays.asList(detail1, detail2)); } + @After + public void tearDown() { + // Restore the original value + ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES = originalVmServiceOfferingMaxCpuCores; + ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_RAM_SIZE = originalVmServiceOfferingMaxRAMSize; + } + @Test public void testSetVmQuotaPercentage() { guru.setVmQuotaPercentage(vmTO, vmProfile); diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 06fb65921c3a..21b16bd73afa 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -38,12 +38,16 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.LinkedList; import java.util.List; import java.util.Map; -import com.cloud.network.NetworkService; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.api.command.admin.vm.AssignVMCmd; import org.apache.cloudstack.api.command.user.vm.DeployVMCmd; @@ -65,6 +69,7 @@ import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; @@ -80,6 +85,9 @@ import com.cloud.deploy.DeployDestination; import com.cloud.deploy.DeploymentPlanner; import com.cloud.deploy.DeploymentPlanningManager; +import com.cloud.domain.DomainVO; +import com.cloud.domain.dao.DomainDao; +import com.cloud.event.UsageEventUtils; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InsufficientServerCapacityException; @@ -91,15 +99,33 @@ import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; +import com.cloud.network.Network; import com.cloud.network.NetworkModel; +import com.cloud.network.NetworkService; +import com.cloud.network.dao.FirewallRulesDao; +import com.cloud.network.dao.IPAddressDao; +import com.cloud.network.dao.IPAddressVO; +import com.cloud.network.dao.LoadBalancerVMMapDao; +import com.cloud.network.dao.LoadBalancerVMMapVO; import com.cloud.network.dao.NetworkDao; import com.cloud.network.dao.NetworkVO; +import com.cloud.network.dao.PhysicalNetworkDao; +import com.cloud.network.dao.PhysicalNetworkVO; +import com.cloud.network.guru.NetworkGuru; +import com.cloud.network.rules.FirewallRuleVO; +import com.cloud.network.rules.PortForwardingRule; +import com.cloud.network.rules.dao.PortForwardingRulesDao; +import com.cloud.network.security.SecurityGroupManager; import com.cloud.network.security.SecurityGroupVO; import com.cloud.offering.DiskOffering; +import com.cloud.offering.NetworkOffering; import com.cloud.offering.ServiceOffering; +import com.cloud.offerings.NetworkOfferingVO; +import com.cloud.offerings.dao.NetworkOfferingDao; import com.cloud.server.ManagementService; import com.cloud.service.ServiceOfferingVO; import com.cloud.service.dao.ServiceOfferingDao; +import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.DiskOfferingVO; import com.cloud.storage.GuestOSVO; import com.cloud.storage.ScopeType; @@ -136,31 +162,6 @@ import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.snapshot.VMSnapshotVO; import com.cloud.vm.snapshot.dao.VMSnapshotDao; -import org.mockito.MockedStatic; - -import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.LinkedHashSet; -import java.util.LinkedList; -import com.cloud.domain.DomainVO; -import com.cloud.domain.dao.DomainDao; -import com.cloud.event.UsageEventUtils; -import com.cloud.network.Network; -import com.cloud.network.dao.FirewallRulesDao; -import com.cloud.network.dao.IPAddressDao; -import com.cloud.network.dao.IPAddressVO; -import com.cloud.network.dao.LoadBalancerVMMapDao; -import com.cloud.network.dao.LoadBalancerVMMapVO; -import com.cloud.network.dao.PhysicalNetworkDao; -import com.cloud.network.dao.PhysicalNetworkVO; -import com.cloud.network.guru.NetworkGuru; -import com.cloud.network.rules.FirewallRuleVO; -import com.cloud.network.rules.PortForwardingRule; -import com.cloud.network.rules.dao.PortForwardingRulesDao; -import com.cloud.network.security.SecurityGroupManager; -import com.cloud.offering.NetworkOffering; -import com.cloud.offerings.NetworkOfferingVO; -import com.cloud.offerings.dao.NetworkOfferingDao; @RunWith(MockitoJUnitRunner.class) public class UserVmManagerImplTest { @@ -370,6 +371,9 @@ public class UserVmManagerImplTest { @Mock NetworkService networkServiceMock; + @Mock + ServiceOfferingDetailsDao serviceOfferingDetailsDao; + private static final long vmId = 1l; private static final long zoneId = 2L; private static final long accountId = 3L; @@ -3121,4 +3125,96 @@ public void executeStepsToChangeOwnershipOfVmTestResourceCountRunningVmsOnlyEnab Mockito.verify(userVmManagerImpl, Mockito.never()).resourceCountIncrement(Mockito.anyLong(), Mockito.any(), Mockito.any(), Mockito.any()); } } + + private ServiceOfferingVO getMockedServiceOffering(boolean custom, boolean customSpeed) { + ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class); + when(serviceOffering.getUuid()).thenReturn("offering-uuid"); + when(serviceOffering.isDynamic()).thenReturn(custom); + when(serviceOffering.isCustomCpuSpeedSupported()).thenReturn(customSpeed); + if (custom) { + when(serviceOffering.getCpu()).thenReturn(null); + when(serviceOffering.getRamSize()).thenReturn(null); + } + if (customSpeed) { + when(serviceOffering.getSpeed()).thenReturn(null); + } else { + when(serviceOffering.isCustomCpuSpeedSupported()).thenReturn(false); + when(serviceOffering.getSpeed()).thenReturn(1000); + } + return serviceOffering; + } + + @Test + public void customOfferingNeedsCustomizationThrowsException() { + ServiceOfferingVO serviceOffering = getMockedServiceOffering(true, true); + InvalidParameterValueException ex = Assert.assertThrows(InvalidParameterValueException.class, () -> + userVmManagerImpl.validateCustomParameters(serviceOffering, Collections.emptyMap())); + assertEquals("Need to specify custom parameter values cpu, cpu speed and memory when using custom offering", ex.getMessage()); + } + + @Test + public void cpuSpeedCustomizationNotAllowedThrowsException() { + ServiceOfferingVO serviceOffering = getMockedServiceOffering(true, false); + + Map customParameters = new HashMap<>(); + customParameters.put(VmDetailConstants.CPU_NUMBER, "1"); + customParameters.put(VmDetailConstants.CPU_SPEED, "2500"); + + InvalidParameterValueException ex = Assert.assertThrows(InvalidParameterValueException.class, () -> + userVmManagerImpl.validateCustomParameters(serviceOffering, customParameters)); + Assert.assertTrue(ex.getMessage().startsWith("The CPU speed of this offering")); + } + + @Test + public void cpuSpeedCustomizationAllowedDoesNotThrowException() { + ServiceOfferingVO serviceOffering = getMockedServiceOffering(true, true); + + when(serviceOfferingDetailsDao.listDetailsKeyPairs(anyLong())).thenReturn( + Map.of(ApiConstants.MIN_CPU_NUMBER, "1", + ApiConstants.MAX_CPU_NUMBER, "4", + ApiConstants.MIN_MEMORY, "256", + ApiConstants.MAX_MEMORY, "8192")); + + Map customParameters = new HashMap<>(); + customParameters.put(VmDetailConstants.CPU_NUMBER, "1"); + customParameters.put(VmDetailConstants.CPU_SPEED, "2500"); + customParameters.put(VmDetailConstants.MEMORY, "256"); + + userVmManagerImpl.validateCustomParameters(serviceOffering, customParameters); + } + + @Test + public void verifyVmLimits_fixedOffering_throwsException() { + when(userVmVoMock.getId()).thenReturn(1L); + when(userVmVoMock.getServiceOfferingId()).thenReturn(1L); + when(accountDao.findById(anyLong())).thenReturn(callerAccount); + ServiceOfferingVO serviceOffering = getMockedServiceOffering(false, false); + when(_serviceOfferingDao.findById(anyLong())).thenReturn(serviceOffering); + when(_serviceOfferingDao.findByIdIncludingRemoved(anyLong(), anyLong())).thenReturn(serviceOffering); + + Map customParameters = new HashMap<>(); + customParameters.put(VmDetailConstants.CPU_SPEED, "2500"); + + InvalidParameterValueException ex = Assert.assertThrows(InvalidParameterValueException.class, () -> + userVmManagerImpl.verifyVmLimits(userVmVoMock, customParameters)); + assertEquals("CPU number, Memory and CPU speed cannot be updated for a non-dynamic offering", ex.getMessage()); + } + + @Test + public void verifyVmLimits_constrainedOffering_throwsException() { + when(userVmVoMock.getId()).thenReturn(1L); + when(userVmVoMock.getServiceOfferingId()).thenReturn(1L); + when(accountDao.findById(anyLong())).thenReturn(callerAccount); + ServiceOfferingVO serviceOffering = getMockedServiceOffering(true, false); + when(_serviceOfferingDao.findById(anyLong())).thenReturn(serviceOffering); + when(_serviceOfferingDao.findByIdIncludingRemoved(anyLong(), anyLong())).thenReturn(serviceOffering); + + Map customParameters = new HashMap<>(); + customParameters.put(VmDetailConstants.CPU_NUMBER, "1"); + customParameters.put(VmDetailConstants.CPU_SPEED, "2500"); + + InvalidParameterValueException ex = Assert.assertThrows(InvalidParameterValueException.class, () -> + userVmManagerImpl.verifyVmLimits(userVmVoMock, customParameters)); + Assert.assertTrue(ex.getMessage().startsWith("The CPU speed of this offering")); + } }