Skip to content

Commit 9030443

Browse files
abh1sarCopilot
abh1sar
and
Copilot
authored
SharedFS: fix NPE when accessing systemvm templates for unsupported hypervisors (#13930)
* fix NPE when accessing systemvm templates for unsupported hypervisors in deploySharedFSVM() * systemvm -> SystemVM Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Add unit test for skip-hypervisor-without-template fix in deploySharedFSVM() * sharedfs: drop duplicate skip-hypervisor test after 4.22 merge --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 9018034 commit 9030443

5 files changed

Lines changed: 66 additions & 48 deletions

File tree

‎api/src/main/java/com/cloud/resource/ResourceService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ public interface ResourceService {
9191

9292
DataCenter getZone(Long zoneId);
9393

94-
List<HypervisorType> getSupportedHypervisorTypes(long zoneId, boolean forVirtualRouter, Long podId);
94+
List<HypervisorType> getSupportedHypervisorTypes(long zoneId, boolean forSystemVm, Long podId);
9595

9696
boolean releaseHostReservation(Long hostId);
9797

‎plugins/storage/sharedfs/storagevm/src/main/java/org/apache/cloudstack/storage/sharedfs/lifecycle/StorageVmSharedFSLifeCycle.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ private UserVm deploySharedFSVM(Long zoneId, Account owner, List<Long> networkId
160160
ServiceOffering serviceOffering = serviceOfferingDao.findById(serviceOfferingId);
161161
DataCenter zone = dataCenterDao.findById(zoneId);
162162

163-
List<Hypervisor.HypervisorType> hypervisors = resourceMgr.getSupportedHypervisorTypes(zoneId, false, null);
163+
List<Hypervisor.HypervisorType> hypervisors = resourceMgr.getSupportedHypervisorTypes(zoneId, true, null);
164164
if (hypervisors.size() > 0) {
165165
Collections.shuffle(hypervisors);
166166
} else {
@@ -183,8 +183,9 @@ private UserVm deploySharedFSVM(Long zoneId, Account owner, List<Long> networkId
183183
if (template == null) {
184184
if (iter.hasNext()) {
185185
continue;
186+
} else {
187+
throw new CloudRuntimeException(String.format("Unable to find the SystemVM template for any of the available hypervisors in zone %s.", zone.toString()));
186188
}
187-
throw new CloudRuntimeException(String.format("Unable to find the systemvm template for %s or it was not downloaded in %s.", hypervisor.toString(), zone.toString()));
188189
}
189190

190191
LaunchPermissionVO existingPermission = launchPermissionDao.findByTemplateAndAccount(template.getId(), owner.getId());

‎plugins/storage/sharedfs/storagevm/src/test/java/org/apache/cloudstack/storage/sharedfs/lifecycle/StorageVmSharedFSLifeCycleTest.java‎

Lines changed: 59 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,12 @@
8181
import static org.mockito.ArgumentMatchers.anyLong;
8282
import static org.mockito.ArgumentMatchers.anyMap;
8383
import static org.mockito.ArgumentMatchers.anyString;
84+
import static org.mockito.ArgumentMatchers.eq;
8485
import static org.mockito.ArgumentMatchers.isNull;
8586
import static org.mockito.Mockito.mock;
8687
import static org.mockito.Mockito.mockStatic;
88+
import static org.mockito.Mockito.times;
89+
import static org.mockito.Mockito.verify;
8790
import static org.mockito.Mockito.when;
8891

8992
@RunWith(MockitoJUnitRunner.class)
@@ -230,7 +233,7 @@ private SharedFS prepareDeploySharedFS() throws ResourceUnavailableException, In
230233

231234
DataCenterVO zone = mock(DataCenterVO.class);
232235
when(dataCenterDao.findById(s_zoneId)).thenReturn(zone);
233-
when(resourceMgr.getSupportedHypervisorTypes(s_zoneId, false, null)).thenReturn(List.of(Hypervisor.HypervisorType.KVM));
236+
when(resourceMgr.getSupportedHypervisorTypes(s_zoneId, true, null)).thenReturn(List.of(Hypervisor.HypervisorType.KVM));
234237

235238
ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class);
236239
when(serviceOfferingDao.findById(s_serviceOfferingId)).thenReturn(serviceOffering);
@@ -274,46 +277,6 @@ public void testDeploySharedFS() throws ResourceUnavailableException, Insufficie
274277
Assert.assertEquals(Optional.ofNullable(result.second()), Optional.ofNullable(s_vmId));
275278
}
276279

277-
@Test
278-
public void testDeploySharedFSContinuesWhenTemplateIsMissingForNonLastHypervisor() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException, IOException, OperationTimedoutException {
279-
SharedFS sharedFS = prepareDeploySharedFS();
280-
when(resourceMgr.getSupportedHypervisorTypes(s_zoneId, false, null)).thenReturn(new ArrayList<>(List.of(Hypervisor.HypervisorType.External, Hypervisor.HypervisorType.KVM)) {
281-
@Override
282-
public Hypervisor.HypervisorType set(int index, Hypervisor.HypervisorType element) {
283-
// Keep the test order stable while exercising the production shuffle call.
284-
return get(index);
285-
}
286-
});
287-
when(templateDao.findSystemVMReadyTemplate(s_zoneId, Hypervisor.HypervisorType.External, ResourceManager.SystemVmPreferredArchitecture.defaultValue())).thenReturn(null);
288-
289-
Account owner = mock(Account.class);
290-
when(owner.getId()).thenReturn(s_ownerId);
291-
when(accountMgr.getActiveAccountById(s_ownerId)).thenReturn(owner);
292-
293-
UserVm vm = mock(UserVm.class);
294-
when(vm.getId()).thenReturn(s_vmId);
295-
when(userVmService.createAdvancedVirtualMachine(
296-
any(DataCenter.class), any(ServiceOffering.class), any(VirtualMachineTemplate.class), anyList(), any(Account.class), anyString(),
297-
anyString(), anyLong(), anyLong(), any(), isNull(), any(Hypervisor.HypervisorType.class), any(BaseCmd.HTTPMethod.class), anyString(),
298-
isNull(), isNull(), anyList(), isNull(), any(Network.IpAddresses.class), isNull(), isNull(), isNull(),
299-
anyMap(), isNull(), isNull(), isNull(), isNull(),
300-
anyBoolean(), anyString(), isNull(), isNull(), isNull())).thenReturn(vm);
301-
302-
VolumeVO rootVol = mock(VolumeVO.class);
303-
when(rootVol.getVolumeType()).thenReturn(Volume.Type.ROOT);
304-
when(rootVol.getName()).thenReturn("ROOT-1");
305-
VolumeVO dataVol = mock(VolumeVO.class);
306-
when(dataVol.getId()).thenReturn(s_volumeId);
307-
when(dataVol.getName()).thenReturn("DATA-1");
308-
when(dataVol.getVolumeType()).thenReturn(Volume.Type.DATADISK);
309-
when(volumeDao.findByInstance(s_vmId)).thenReturn(List.of(rootVol, dataVol));
310-
311-
Pair<Long, Long> result = lifeCycle.deploySharedFS(sharedFS, s_networkId, s_diskOfferingId, s_size, s_minIops, s_maxIops);
312-
313-
Assert.assertEquals(Optional.ofNullable(result.first()), Optional.ofNullable(s_volumeId));
314-
Assert.assertEquals(Optional.ofNullable(result.second()), Optional.ofNullable(s_vmId));
315-
}
316-
317280
@Test(expected = CloudRuntimeException.class)
318281
public void testDeploySharedFSHypervisorNotFound() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException, IOException, OperationTimedoutException {
319282
SharedFS sharedFS = mock(SharedFS.class);
@@ -341,11 +304,65 @@ public void testDeploySharedFSTemplateNotFound() throws ResourceUnavailableExcep
341304
when(accountMgr.getActiveAccountById(s_ownerId)).thenReturn(null);
342305
DataCenterVO zone = mock(DataCenterVO.class);
343306
when(dataCenterDao.findById(s_zoneId)).thenReturn(zone);
344-
when(resourceMgr.getSupportedHypervisorTypes(s_zoneId, false, null)).thenReturn(List.of(Hypervisor.HypervisorType.KVM));
307+
when(resourceMgr.getSupportedHypervisorTypes(s_zoneId, true, null)).thenReturn(List.of(Hypervisor.HypervisorType.KVM));
345308

346309
lifeCycle.deploySharedFS(sharedFS, s_networkId, s_diskOfferingId, s_size, s_minIops, s_maxIops);
347310
}
348311

312+
@Test
313+
public void testDeploySharedFSSkipsHypervisorWithoutTemplate() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException, IOException, OperationTimedoutException {
314+
// Verifies that when the systemvm template is missing for one of the supported hypervisors,
315+
// deploySharedFSVM() skips it and tries the next one instead of failing with an NPE.
316+
SharedFS sharedFS = mock(SharedFS.class);
317+
when(sharedFS.getDataCenterId()).thenReturn(s_zoneId);
318+
when(sharedFS.getName()).thenReturn(s_name);
319+
when(sharedFS.getServiceOfferingId()).thenReturn(s_serviceOfferingId);
320+
when(sharedFS.getFsType()).thenReturn(SharedFS.FileSystemType.valueOf(s_fsFormat));
321+
when(sharedFS.getAccountId()).thenReturn(s_ownerId);
322+
323+
Account owner = mock(Account.class);
324+
when(owner.getId()).thenReturn(s_ownerId);
325+
when(accountMgr.getActiveAccountById(s_ownerId)).thenReturn(owner);
326+
327+
DataCenterVO zone = mock(DataCenterVO.class);
328+
when(dataCenterDao.findById(s_zoneId)).thenReturn(zone);
329+
330+
List<Hypervisor.HypervisorType> hypervisors = new ArrayList<>(List.of(Hypervisor.HypervisorType.XenServer, Hypervisor.HypervisorType.KVM));
331+
when(resourceMgr.getSupportedHypervisorTypes(s_zoneId, true, null)).thenReturn(hypervisors);
332+
333+
ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class);
334+
when(serviceOfferingDao.findById(s_serviceOfferingId)).thenReturn(serviceOffering);
335+
336+
// The hypervisor list is shuffled before iteration, so instead of pinning which hypervisor is
337+
// tried first, the first lookup (whichever hypervisor that is) returns no template and the
338+
// second lookup returns a valid one, exercising the "skip and try the next hypervisor" fix.
339+
VMTemplateVO template = mock(VMTemplateVO.class);
340+
when(template.getId()).thenReturn(s_templateId);
341+
when(templateDao.findSystemVMReadyTemplate(eq(s_zoneId), any(Hypervisor.HypervisorType.class), eq(ResourceManager.SystemVmPreferredArchitecture.defaultValue())))
342+
.thenReturn(null, template);
343+
344+
UserVm vm = mock(UserVm.class);
345+
when(vm.getId()).thenReturn(s_vmId);
346+
when(userVmService.createAdvancedVirtualMachine(
347+
any(DataCenter.class), any(ServiceOffering.class), any(VirtualMachineTemplate.class), anyList(), any(Account.class), anyString(),
348+
anyString(), anyLong(), anyLong(), any(), isNull(), any(Hypervisor.HypervisorType.class), any(BaseCmd.HTTPMethod.class), anyString(),
349+
isNull(), isNull(), anyList(), isNull(), any(Network.IpAddresses.class), isNull(), isNull(), isNull(),
350+
anyMap(), isNull(), isNull(), isNull(), isNull(),
351+
anyBoolean(), anyString(), isNull(), isNull(), isNull())).thenReturn(vm);
352+
353+
VolumeVO dataVol = mock(VolumeVO.class);
354+
when(dataVol.getId()).thenReturn(s_volumeId);
355+
when(dataVol.getName()).thenReturn("DATA-1");
356+
when(dataVol.getVolumeType()).thenReturn(Volume.Type.DATADISK);
357+
when(volumeDao.findByInstance(s_vmId)).thenReturn(List.of(dataVol));
358+
359+
Pair<Long, Long> result = lifeCycle.deploySharedFS(sharedFS, s_networkId, s_diskOfferingId, s_size, s_minIops, s_maxIops);
360+
Assert.assertEquals(Optional.of(s_volumeId), Optional.ofNullable(result.first()));
361+
Assert.assertEquals(Optional.of(s_vmId), Optional.ofNullable(result.second()));
362+
363+
verify(templateDao, times(2)).findSystemVMReadyTemplate(eq(s_zoneId), any(Hypervisor.HypervisorType.class), eq(ResourceManager.SystemVmPreferredArchitecture.defaultValue()));
364+
}
365+
349366
@Test
350367
public void testDeleteSharedFS() throws ResourceUnavailableException {
351368
SharedFS sharedFS = mock(SharedFS.class);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2962,7 +2962,7 @@ public boolean configure(final String name, final Map<String, Object> params) th
29622962
}
29632963

29642964
@Override
2965-
public List<HypervisorType> getSupportedHypervisorTypes(final long zoneId, final boolean forVirtualRouter, final Long podId) {
2965+
public List<HypervisorType> getSupportedHypervisorTypes(final long zoneId, final boolean forSystemVm, final Long podId) {
29662966
final List<HypervisorType> hypervisorTypes = new ArrayList<>();
29672967

29682968
List<ClusterVO> clustersForZone;
@@ -2974,7 +2974,7 @@ public List<HypervisorType> getSupportedHypervisorTypes(final long zoneId, final
29742974

29752975
for (final ClusterVO cluster : clustersForZone) {
29762976
final HypervisorType hType = cluster.getHypervisorType();
2977-
if (!forVirtualRouter || (hType != HypervisorType.BareMetal && hType != HypervisorType.External && hType != HypervisorType.Ovm)) {
2977+
if (!forSystemVm || (hType != HypervisorType.BareMetal && hType != HypervisorType.External && hType != HypervisorType.Ovm)) {
29782978
hypervisorTypes.add(hType);
29792979
}
29802980
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ public DataCenter getZone(Long zoneId) {
205205
* @see com.cloud.resource.ResourceService#getSupportedHypervisorTypes(long, boolean, java.lang.Long)
206206
*/
207207
@Override
208-
public List<HypervisorType> getSupportedHypervisorTypes(final long zoneId, final boolean forVirtualRouter, final Long podId) {
208+
public List<HypervisorType> getSupportedHypervisorTypes(final long zoneId, final boolean forSystemVm, final Long podId) {
209209
// TODO Auto-generated method stub
210210
return null;
211211
}

0 commit comments

Comments
 (0)