Skip to content

Commit c5e8f63

Browse files
sureshanapartisureshanaparti
authored
Fix NPE issues during host rolling maintenance, due to host tags and custom constrained/unconstrained service offering (#9844)
1 parent 9967bb3 commit c5e8f63

File tree

2 files changed

+113
-6
lines changed

2 files changed

+113
-6
lines changed

‎server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java‎

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.apache.cloudstack.context.CallContext;
3838
import org.apache.cloudstack.framework.config.ConfigKey;
3939
import org.apache.commons.collections.CollectionUtils;
40+
import org.apache.commons.lang3.ObjectUtils;
4041
import org.apache.log4j.Logger;
4142

4243
import com.cloud.agent.AgentManager;
@@ -65,12 +66,16 @@
6566
import com.cloud.service.ServiceOfferingVO;
6667
import com.cloud.service.dao.ServiceOfferingDao;
6768
import com.cloud.utils.Pair;
69+
import com.cloud.utils.StringUtils;
6870
import com.cloud.utils.Ternary;
6971
import com.cloud.utils.component.ManagerBase;
7072
import com.cloud.utils.exception.CloudRuntimeException;
73+
import com.cloud.vm.UserVmDetailVO;
7174
import com.cloud.vm.VMInstanceVO;
7275
import com.cloud.vm.VirtualMachine.State;
7376
import com.cloud.vm.VirtualMachineProfileImpl;
77+
import com.cloud.vm.VmDetailConstants;
78+
import com.cloud.vm.dao.UserVmDetailsDao;
7479
import com.cloud.vm.dao.VMInstanceDao;
7580

7681
public class RollingMaintenanceManagerImpl extends ManagerBase implements RollingMaintenanceManager {
@@ -86,6 +91,8 @@ public class RollingMaintenanceManagerImpl extends ManagerBase implements Rollin
8691
@Inject
8792
private VMInstanceDao vmInstanceDao;
8893
@Inject
94+
protected UserVmDetailsDao userVmDetailsDao;
95+
@Inject
8996
private ServiceOfferingDao serviceOfferingDao;
9097
@Inject
9198
private ClusterDetailsDao clusterDetailsDao;
@@ -621,10 +628,19 @@ private Pair<Boolean, String> performCapacityChecksBeforeHostInMaintenance(Host
621628
int successfullyCheckedVmMigrations = 0;
622629
for (VMInstanceVO runningVM : vmsRunning) {
623630
boolean canMigrateVm = false;
631+
Ternary<Integer, Integer, Integer> cpuSpeedAndRamSize = getComputeResourcesCpuSpeedAndRamSize(runningVM);
632+
Integer cpu = cpuSpeedAndRamSize.first();
633+
Integer speed = cpuSpeedAndRamSize.second();
634+
Integer ramSize = cpuSpeedAndRamSize.third();
635+
if (ObjectUtils.anyNull(cpu, speed, ramSize)) {
636+
s_logger.warn(String.format("Cannot fetch compute resources for the VM %s, skipping it from the capacity check", runningVM));
637+
continue;
638+
}
639+
624640
ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(runningVM.getServiceOfferingId());
625641
for (Host hostInCluster : hostsInCluster) {
626642
if (!checkHostTags(hostTags, hostTagsDao.getHostTags(hostInCluster.getId()), serviceOffering.getHostTag())) {
627-
s_logger.debug(String.format("Host tags mismatch between %s and %s Skipping it from the capacity check", host, hostInCluster));
643+
s_logger.warn(String.format("Host tags mismatch between %s and %s, skipping it from the capacity check", host, hostInCluster));
628644
continue;
629645
}
630646
DeployDestination deployDestination = new DeployDestination(null, null, null, host);
@@ -634,13 +650,13 @@ private Pair<Boolean, String> performCapacityChecksBeforeHostInMaintenance(Host
634650
affinityChecks = affinityChecks && affinityProcessor.check(vmProfile, deployDestination);
635651
}
636652
if (!affinityChecks) {
637-
s_logger.debug(String.format("Affinity check failed between %s and %s Skipping it from the capacity check", host, hostInCluster));
653+
s_logger.warn(String.format("Affinity check failed between %s and %s, skipping it from the capacity check", host, hostInCluster));
638654
continue;
639655
}
640656
boolean maxGuestLimit = capacityManager.checkIfHostReachMaxGuestLimit(host);
641-
boolean hostHasCPUCapacity = capacityManager.checkIfHostHasCpuCapability(hostInCluster.getId(), serviceOffering.getCpu(), serviceOffering.getSpeed());
642-
int cpuRequested = serviceOffering.getCpu() * serviceOffering.getSpeed();
643-
long ramRequested = serviceOffering.getRamSize() * 1024L * 1024L;
657+
boolean hostHasCPUCapacity = capacityManager.checkIfHostHasCpuCapability(hostInCluster.getId(), cpu, speed);
658+
int cpuRequested = cpu * speed;
659+
long ramRequested = ramSize * 1024L * 1024L;
644660
ClusterDetailsVO clusterDetailsCpuOvercommit = clusterDetailsDao.findDetail(cluster.getId(), "cpuOvercommitRatio");
645661
ClusterDetailsVO clusterDetailsRamOvercommmt = clusterDetailsDao.findDetail(cluster.getId(), "memoryOvercommitRatio");
646662
Float cpuOvercommitRatio = Float.parseFloat(clusterDetailsCpuOvercommit.getValue());
@@ -666,11 +682,42 @@ private Pair<Boolean, String> performCapacityChecksBeforeHostInMaintenance(Host
666682
return new Pair<>(true, "OK");
667683
}
668684

685+
protected Ternary<Integer, Integer, Integer> getComputeResourcesCpuSpeedAndRamSize(VMInstanceVO runningVM) {
686+
ServiceOfferingVO serviceOffering = serviceOfferingDao.findById(runningVM.getServiceOfferingId());
687+
Integer cpu = serviceOffering.getCpu();
688+
Integer speed = serviceOffering.getSpeed();
689+
Integer ramSize = serviceOffering.getRamSize();
690+
if (!serviceOffering.isDynamic()) {
691+
return new Ternary<>(cpu, speed, ramSize);
692+
}
693+
694+
List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(runningVM.getId());
695+
if (CollectionUtils.isEmpty(vmDetails)) {
696+
return new Ternary<>(cpu, speed, ramSize);
697+
}
698+
699+
for (UserVmDetailVO vmDetail : vmDetails) {
700+
if (StringUtils.isBlank(vmDetail.getName()) || StringUtils.isBlank(vmDetail.getValue())) {
701+
continue;
702+
}
703+
704+
if (cpu == null && VmDetailConstants.CPU_NUMBER.equals(vmDetail.getName())) {
705+
cpu = Integer.valueOf(vmDetail.getValue());
706+
} else if (speed == null && VmDetailConstants.CPU_SPEED.equals(vmDetail.getName())) {
707+
speed = Integer.valueOf(vmDetail.getValue());
708+
} else if (ramSize == null && VmDetailConstants.MEMORY.equals(vmDetail.getName())) {
709+
ramSize = Integer.valueOf(vmDetail.getValue());
710+
}
711+
}
712+
713+
return new Ternary<>(cpu, speed, ramSize);
714+
}
715+
669716
/**
670717
* Check hosts tags
671718
*/
672719
private boolean checkHostTags(List<HostTagVO> hostTags, List<HostTagVO> hostInClusterTags, String offeringTag) {
673-
if (CollectionUtils.isEmpty(hostTags) && CollectionUtils.isEmpty(hostInClusterTags)) {
720+
if ((CollectionUtils.isEmpty(hostTags) && CollectionUtils.isEmpty(hostInClusterTags)) || StringUtils.isBlank(offeringTag)) {
674721
return true;
675722
} else if ((CollectionUtils.isNotEmpty(hostTags) && CollectionUtils.isEmpty(hostInClusterTags)) ||
676723
(CollectionUtils.isEmpty(hostTags) && CollectionUtils.isNotEmpty(hostInClusterTags))) {

‎server/src/test/java/com/cloud/resource/RollingMaintenanceManagerImplTest.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,15 @@
2323
import com.cloud.host.dao.HostDao;
2424
import com.cloud.hypervisor.Hypervisor;
2525
import com.cloud.org.Cluster;
26+
import com.cloud.service.ServiceOfferingVO;
27+
import com.cloud.service.dao.ServiceOfferingDao;
28+
import com.cloud.utils.Ternary;
2629
import com.cloud.utils.exception.CloudRuntimeException;
30+
import com.cloud.vm.UserVmDetailVO;
31+
import com.cloud.vm.VMInstanceVO;
32+
import com.cloud.vm.VmDetailConstants;
33+
import com.cloud.vm.dao.UserVmDetailsDao;
34+
2735
import org.junit.Assert;
2836
import org.junit.Before;
2937
import org.junit.Test;
@@ -53,6 +61,12 @@ public class RollingMaintenanceManagerImplTest {
5361
HostVO host4;
5462
@Mock
5563
Cluster cluster;
64+
@Mock
65+
VMInstanceVO vm;
66+
@Mock
67+
ServiceOfferingDao serviceOfferingDao;
68+
@Mock
69+
UserVmDetailsDao userVmDetailsDao;
5670

5771
@Spy
5872
@InjectMocks
@@ -164,4 +178,50 @@ public void testPerformStateChecksForce() {
164178

165179
Assert.assertEquals(1, hosts.size());
166180
}
181+
182+
@Test
183+
public void testGetComputeResourcesCpuSpeedAndRamSize_ForNormalOffering() {
184+
ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class);
185+
Mockito.when(serviceOffering.isDynamic()).thenReturn(false);
186+
Mockito.when(serviceOffering.getCpu()).thenReturn(1);
187+
Mockito.when(serviceOffering.getSpeed()).thenReturn(500);
188+
Mockito.when(serviceOffering.getRamSize()).thenReturn(512);
189+
190+
Mockito.when(vm.getServiceOfferingId()).thenReturn(1L);
191+
Mockito.when(serviceOfferingDao.findById(1L)).thenReturn(serviceOffering);
192+
193+
Ternary<Integer, Integer, Integer> cpuSpeedAndRamSize = manager.getComputeResourcesCpuSpeedAndRamSize(vm);
194+
195+
Assert.assertEquals(1, cpuSpeedAndRamSize.first().intValue());
196+
Assert.assertEquals(500, cpuSpeedAndRamSize.second().intValue());
197+
Assert.assertEquals(512, cpuSpeedAndRamSize.third().intValue());
198+
}
199+
200+
@Test
201+
public void testGetComputeResourcesCpuSpeedAndRamSize_ForCustomOffering() {
202+
ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class);
203+
Mockito.when(serviceOffering.isDynamic()).thenReturn(true);
204+
Mockito.when(serviceOffering.getCpu()).thenReturn(null);
205+
Mockito.when(serviceOffering.getSpeed()).thenReturn(null);
206+
Mockito.when(serviceOffering.getRamSize()).thenReturn(null);
207+
208+
List<UserVmDetailVO> vmDetails = new ArrayList<>();
209+
UserVmDetailVO cpuDetail = new UserVmDetailVO(1L, VmDetailConstants.CPU_NUMBER, "2", false);
210+
vmDetails.add(cpuDetail);
211+
UserVmDetailVO speedDetail = new UserVmDetailVO(1L, VmDetailConstants.CPU_SPEED, "1000", false);
212+
vmDetails.add(speedDetail);
213+
UserVmDetailVO ramSizeDetail = new UserVmDetailVO(1L, VmDetailConstants.MEMORY, "1024", false);
214+
vmDetails.add(ramSizeDetail);
215+
216+
Mockito.when(vm.getId()).thenReturn(1L);
217+
Mockito.when(vm.getServiceOfferingId()).thenReturn(1L);
218+
Mockito.when(serviceOfferingDao.findById(1L)).thenReturn(serviceOffering);
219+
Mockito.when(userVmDetailsDao.listDetails(1L)).thenReturn(vmDetails);
220+
221+
Ternary<Integer, Integer, Integer> cpuSpeedAndRamSize = manager.getComputeResourcesCpuSpeedAndRamSize(vm);
222+
223+
Assert.assertEquals(2, cpuSpeedAndRamSize.first().intValue());
224+
Assert.assertEquals(1000, cpuSpeedAndRamSize.second().intValue());
225+
Assert.assertEquals(1024, cpuSpeedAndRamSize.third().intValue());
226+
}
167227
}

0 commit comments

Comments
 (0)