From 9bbb37cda9fcf5c259ccbad03dbea4316c2b0460 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Wed, 23 Oct 2024 17:03:55 +0530 Subject: [PATCH 1/4] Fix NPE issues during host rolling maintenance, due to host tags and custom constrained/unconstrained service offering --- .../RollingMaintenanceManagerImpl.java | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java index 25b2ad53bf2b..bcb16be5aa88 100644 --- a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java +++ b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java @@ -65,12 +65,16 @@ import com.cloud.service.ServiceOfferingVO; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.utils.Pair; +import com.cloud.utils.StringUtils; import com.cloud.utils.Ternary; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmDetailVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.VirtualMachineProfileImpl; +import com.cloud.vm.VmDetailConstants; +import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.dao.VMInstanceDao; public class RollingMaintenanceManagerImpl extends ManagerBase implements RollingMaintenanceManager { @@ -86,6 +90,8 @@ public class RollingMaintenanceManagerImpl extends ManagerBase implements Rollin @Inject private VMInstanceDao vmInstanceDao; @Inject + protected UserVmDetailsDao userVmDetailsDao; + @Inject private ServiceOfferingDao serviceOfferingDao; @Inject private ClusterDetailsDao clusterDetailsDao; @@ -622,9 +628,35 @@ private Pair performCapacityChecksBeforeHostInMaintenance(Host for (VMInstanceVO runningVM : vmsRunning) { boolean canMigrateVm = false; ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(runningVM.getServiceOfferingId()); + Integer cpu = serviceOffering.getCpu(); + Integer speed = serviceOffering.getSpeed(); + Integer ramSize = serviceOffering.getRamSize(); + if (serviceOffering.isDynamic()) { + List vmDetails = userVmDetailsDao.listDetails(runningVM.getId()); + if (CollectionUtils.isNotEmpty(vmDetails)) { + for (UserVmDetailVO vmDetail : vmDetails) { + if (vmDetail.getName() != null &&vmDetail.getValue() != null) { + if (cpu == null && VmDetailConstants.CPU_NUMBER.equals(vmDetail.getName())) { + cpu = Integer.valueOf(vmDetail.getValue()); + } + if (speed == null && VmDetailConstants.CPU_SPEED.equals(vmDetail.getName())) { + speed = Integer.valueOf(vmDetail.getValue()); + } + if (ramSize == null && VmDetailConstants.MEMORY.equals(vmDetail.getName())) { + ramSize = Integer.valueOf(vmDetail.getValue()); + } + } + } + } + } + if (cpu == null || speed == null || ramSize == null) { + s_logger.warn(String.format("Cannot fetch compute resources for the VM %s, skipping it from the capacity check", runningVM)); + continue; + } + for (Host hostInCluster : hostsInCluster) { if (!checkHostTags(hostTags, hostTagsDao.getHostTags(hostInCluster.getId()), serviceOffering.getHostTag())) { - s_logger.debug(String.format("Host tags mismatch between %s and %s Skipping it from the capacity check", host, hostInCluster)); + s_logger.warn(String.format("Host tags mismatch between %s and %s, skipping it from the capacity check", host, hostInCluster)); continue; } DeployDestination deployDestination = new DeployDestination(null, null, null, host); @@ -634,13 +666,13 @@ private Pair performCapacityChecksBeforeHostInMaintenance(Host affinityChecks = affinityChecks && affinityProcessor.check(vmProfile, deployDestination); } if (!affinityChecks) { - s_logger.debug(String.format("Affinity check failed between %s and %s Skipping it from the capacity check", host, hostInCluster)); + s_logger.warn(String.format("Affinity check failed between %s and %s, skipping it from the capacity check", host, hostInCluster)); continue; } boolean maxGuestLimit = capacityManager.checkIfHostReachMaxGuestLimit(host); - boolean hostHasCPUCapacity = capacityManager.checkIfHostHasCpuCapability(hostInCluster.getId(), serviceOffering.getCpu(), serviceOffering.getSpeed()); - int cpuRequested = serviceOffering.getCpu() * serviceOffering.getSpeed(); - long ramRequested = serviceOffering.getRamSize() * 1024L * 1024L; + boolean hostHasCPUCapacity = capacityManager.checkIfHostHasCpuCapability(hostInCluster.getId(), cpu, speed); + int cpuRequested = cpu * speed; + long ramRequested = ramSize * 1024L * 1024L; ClusterDetailsVO clusterDetailsCpuOvercommit = clusterDetailsDao.findDetail(cluster.getId(), "cpuOvercommitRatio"); ClusterDetailsVO clusterDetailsRamOvercommmt = clusterDetailsDao.findDetail(cluster.getId(), "memoryOvercommitRatio"); Float cpuOvercommitRatio = Float.parseFloat(clusterDetailsCpuOvercommit.getValue()); @@ -670,7 +702,7 @@ private Pair performCapacityChecksBeforeHostInMaintenance(Host * Check hosts tags */ private boolean checkHostTags(List hostTags, List hostInClusterTags, String offeringTag) { - if (CollectionUtils.isEmpty(hostTags) && CollectionUtils.isEmpty(hostInClusterTags)) { + if ((CollectionUtils.isEmpty(hostTags) && CollectionUtils.isEmpty(hostInClusterTags)) || StringUtils.isBlank(offeringTag)) { return true; } else if ((CollectionUtils.isNotEmpty(hostTags) && CollectionUtils.isEmpty(hostInClusterTags)) || (CollectionUtils.isEmpty(hostTags) && CollectionUtils.isNotEmpty(hostInClusterTags))) { From d8d10482e6a6e1d6c9ad9415369443fd52739106 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Tue, 5 Nov 2024 21:19:52 +0530 Subject: [PATCH 2/4] review comments & tests --- .../RollingMaintenanceManagerImpl.java | 55 ++++++++++------- .../RollingMaintenanceManagerImplTest.java | 60 +++++++++++++++++++ 2 files changed, 92 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java index bcb16be5aa88..05347a6187d0 100644 --- a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java +++ b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java @@ -37,6 +37,7 @@ import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.log4j.Logger; import com.cloud.agent.AgentManager; @@ -627,33 +628,16 @@ private Pair performCapacityChecksBeforeHostInMaintenance(Host int successfullyCheckedVmMigrations = 0; for (VMInstanceVO runningVM : vmsRunning) { boolean canMigrateVm = false; - ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(runningVM.getServiceOfferingId()); - Integer cpu = serviceOffering.getCpu(); - Integer speed = serviceOffering.getSpeed(); - Integer ramSize = serviceOffering.getRamSize(); - if (serviceOffering.isDynamic()) { - List vmDetails = userVmDetailsDao.listDetails(runningVM.getId()); - if (CollectionUtils.isNotEmpty(vmDetails)) { - for (UserVmDetailVO vmDetail : vmDetails) { - if (vmDetail.getName() != null &&vmDetail.getValue() != null) { - if (cpu == null && VmDetailConstants.CPU_NUMBER.equals(vmDetail.getName())) { - cpu = Integer.valueOf(vmDetail.getValue()); - } - if (speed == null && VmDetailConstants.CPU_SPEED.equals(vmDetail.getName())) { - speed = Integer.valueOf(vmDetail.getValue()); - } - if (ramSize == null && VmDetailConstants.MEMORY.equals(vmDetail.getName())) { - ramSize = Integer.valueOf(vmDetail.getValue()); - } - } - } - } - } - if (cpu == null || speed == null || ramSize == null) { + Ternary cpuSpeedAndRamSize = getComputeResourcesCpuSpeedAndRamSize(runningVM); + Integer cpu = cpuSpeedAndRamSize.first(); + Integer speed = cpuSpeedAndRamSize.second(); + Integer ramSize = cpuSpeedAndRamSize.third(); + if (ObjectUtils.anyNull(cpu, speed,ramSize)) { s_logger.warn(String.format("Cannot fetch compute resources for the VM %s, skipping it from the capacity check", runningVM)); continue; } + ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(runningVM.getServiceOfferingId()); for (Host hostInCluster : hostsInCluster) { if (!checkHostTags(hostTags, hostTagsDao.getHostTags(hostInCluster.getId()), serviceOffering.getHostTag())) { s_logger.warn(String.format("Host tags mismatch between %s and %s, skipping it from the capacity check", host, hostInCluster)); @@ -698,6 +682,31 @@ private Pair performCapacityChecksBeforeHostInMaintenance(Host return new Pair<>(true, "OK"); } + protected Ternary getComputeResourcesCpuSpeedAndRamSize(VMInstanceVO runningVM) { + ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(runningVM.getServiceOfferingId()); + Integer cpu = serviceOffering.getCpu(); + Integer speed = serviceOffering.getSpeed(); + Integer ramSize = serviceOffering.getRamSize(); + if (serviceOffering.isDynamic()) { + List vmDetails = userVmDetailsDao.listDetails(runningVM.getId()); + if (CollectionUtils.isNotEmpty(vmDetails)) { + for (UserVmDetailVO vmDetail : vmDetails) { + if (vmDetail.getName() != null && vmDetail.getValue() != null) { + if (cpu == null && VmDetailConstants.CPU_NUMBER.equals(vmDetail.getName())) { + cpu = Integer.valueOf(vmDetail.getValue()); + } else if (speed == null && VmDetailConstants.CPU_SPEED.equals(vmDetail.getName())) { + speed = Integer.valueOf(vmDetail.getValue()); + } else if (ramSize == null && VmDetailConstants.MEMORY.equals(vmDetail.getName())) { + ramSize = Integer.valueOf(vmDetail.getValue()); + } + } + } + } + } + + return new Ternary<>(cpu, speed, ramSize); + } + /** * Check hosts tags */ diff --git a/server/src/test/java/com/cloud/resource/RollingMaintenanceManagerImplTest.java b/server/src/test/java/com/cloud/resource/RollingMaintenanceManagerImplTest.java index ef0277fd3726..d8363964f05d 100644 --- a/server/src/test/java/com/cloud/resource/RollingMaintenanceManagerImplTest.java +++ b/server/src/test/java/com/cloud/resource/RollingMaintenanceManagerImplTest.java @@ -23,7 +23,15 @@ import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.org.Cluster; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.service.dao.ServiceOfferingDao; +import com.cloud.utils.Ternary; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmDetailVO; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VmDetailConstants; +import com.cloud.vm.dao.UserVmDetailsDao; + import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -53,6 +61,12 @@ public class RollingMaintenanceManagerImplTest { HostVO host4; @Mock Cluster cluster; + @Mock + VMInstanceVO vm; + @Mock + ServiceOfferingDao serviceOfferingDao; + @Mock + UserVmDetailsDao userVmDetailsDao; @Spy @InjectMocks @@ -164,4 +178,50 @@ public void testPerformStateChecksForce() { Assert.assertEquals(1, hosts.size()); } + + @Test + public void testGetComputeResourcesCpuSpeedAndRamSize_ForNormalOffering() { + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(serviceOffering.isDynamic()).thenReturn(false); + Mockito.when(serviceOffering.getCpu()).thenReturn(1); + Mockito.when(serviceOffering.getSpeed()).thenReturn(500); + Mockito.when(serviceOffering.getRamSize()).thenReturn(512); + + Mockito.when(vm.getServiceOfferingId()).thenReturn(1L); + Mockito.when(serviceOfferingDao.findById(1L)).thenReturn(serviceOffering); + + Ternary cpuSpeedAndRamSize = manager.getComputeResourcesCpuSpeedAndRamSize(vm); + + Assert.assertEquals(1, cpuSpeedAndRamSize.first().intValue()); + Assert.assertEquals(500, cpuSpeedAndRamSize.second().intValue()); + Assert.assertEquals(512, cpuSpeedAndRamSize.third().intValue()); + } + + @Test + public void testGetComputeResourcesCpuSpeedAndRamSize_ForCustomOffering() { + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(serviceOffering.isDynamic()).thenReturn(true); + Mockito.when(serviceOffering.getCpu()).thenReturn(null); + Mockito.when(serviceOffering.getSpeed()).thenReturn(null); + Mockito.when(serviceOffering.getRamSize()).thenReturn(null); + + List vmDetails = new ArrayList<>(); + UserVmDetailVO cpuDetail = new UserVmDetailVO(1L, VmDetailConstants.CPU_NUMBER, "2", false); + vmDetails.add(cpuDetail); + UserVmDetailVO speedDetail = new UserVmDetailVO(1L, VmDetailConstants.CPU_SPEED, "1000", false); + vmDetails.add(speedDetail); + UserVmDetailVO ramSizeDetail = new UserVmDetailVO(1L, VmDetailConstants.MEMORY, "1024", false); + vmDetails.add(ramSizeDetail); + + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(vm.getServiceOfferingId()).thenReturn(1L); + Mockito.when(serviceOfferingDao.findById(1L)).thenReturn(serviceOffering); + Mockito.when(userVmDetailsDao.listDetails(1L)).thenReturn(vmDetails); + + Ternary cpuSpeedAndRamSize = manager.getComputeResourcesCpuSpeedAndRamSize(vm); + + Assert.assertEquals(2, cpuSpeedAndRamSize.first().intValue()); + Assert.assertEquals(1000, cpuSpeedAndRamSize.second().intValue()); + Assert.assertEquals(1024, cpuSpeedAndRamSize.third().intValue()); + } } From 0e853e9c6aa8c6a5021777c4dc490e11398ef1a8 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Thu, 9 Jan 2025 13:19:56 +0530 Subject: [PATCH 3/4] addressed comments --- .../RollingMaintenanceManagerImpl.java | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java index 05347a6187d0..2ae3fa15913f 100644 --- a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java +++ b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java @@ -687,20 +687,26 @@ protected Ternary getComputeResourcesCpuSpeedAndRamSi Integer cpu = serviceOffering.getCpu(); Integer speed = serviceOffering.getSpeed(); Integer ramSize = serviceOffering.getRamSize(); - if (serviceOffering.isDynamic()) { - List vmDetails = userVmDetailsDao.listDetails(runningVM.getId()); - if (CollectionUtils.isNotEmpty(vmDetails)) { - for (UserVmDetailVO vmDetail : vmDetails) { - if (vmDetail.getName() != null && vmDetail.getValue() != null) { - if (cpu == null && VmDetailConstants.CPU_NUMBER.equals(vmDetail.getName())) { - cpu = Integer.valueOf(vmDetail.getValue()); - } else if (speed == null && VmDetailConstants.CPU_SPEED.equals(vmDetail.getName())) { - speed = Integer.valueOf(vmDetail.getValue()); - } else if (ramSize == null && VmDetailConstants.MEMORY.equals(vmDetail.getName())) { - ramSize = Integer.valueOf(vmDetail.getValue()); - } - } - } + if (!serviceOffering.isDynamic()) { + return new Ternary<>(cpu, speed, ramSize); + } + + List vmDetails = userVmDetailsDao.listDetails(runningVM.getId()); + if (CollectionUtils.isEmpty(vmDetails)) { + return new Ternary<>(cpu, speed, ramSize); + } + + for (UserVmDetailVO vmDetail : vmDetails) { + if (StringUtils.isBlank(vmDetail.getName()) || StringUtils.isBlank(vmDetail.getValue())) { + continue; + } + + if (cpu == null && VmDetailConstants.CPU_NUMBER.equals(vmDetail.getName())) { + cpu = Integer.valueOf(vmDetail.getValue()); + } else if (speed == null && VmDetailConstants.CPU_SPEED.equals(vmDetail.getName())) { + speed = Integer.valueOf(vmDetail.getValue()); + } else if (ramSize == null && VmDetailConstants.MEMORY.equals(vmDetail.getName())) { + ramSize = Integer.valueOf(vmDetail.getValue()); } } From 7cc7bacf66c4e889aec973af357e5b14921245a9 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Thu, 9 Jan 2025 13:25:14 +0530 Subject: [PATCH 4/4] Update server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java --- .../java/com/cloud/resource/RollingMaintenanceManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java index 2ae3fa15913f..4ada43308ee6 100644 --- a/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java +++ b/server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java @@ -632,7 +632,7 @@ private Pair performCapacityChecksBeforeHostInMaintenance(Host Integer cpu = cpuSpeedAndRamSize.first(); Integer speed = cpuSpeedAndRamSize.second(); Integer ramSize = cpuSpeedAndRamSize.third(); - if (ObjectUtils.anyNull(cpu, speed,ramSize)) { + if (ObjectUtils.anyNull(cpu, speed, ramSize)) { s_logger.warn(String.format("Cannot fetch compute resources for the VM %s, skipping it from the capacity check", runningVM)); continue; }