-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add NULL check during restoreVM operation when host is removed #13779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.22
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9267,17 +9267,24 @@ | |
| Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId(); | ||
|
|
||
| if (hostId != null) { | ||
| // default findById() won't search entries with removed field not null | ||
| Host host = _hostDao.findById(hostId); | ||
| Host host = _hostDao.findByIdIncludingRemoved(hostId); | ||
|
|
||
| // host row may have been hard-deleted from DB, treat like removed | ||
| if (host == null) { | ||
| logger.warn("Host {} not found", hostId); | ||
| s_logger.warn(String.format("Host with id %s not found in DB for VM %s (%s)", | ||
|
Check failure on line 9274 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| hostId, vm.getUuid(), vm.getName())); | ||
| return; | ||
| } | ||
| // host could be in removed state, in which case no operation is performed. | ||
| if (host.getStatus() == Status.Removed) { | ||
| logger.warn("Host {} ({}) for VM {} ({}) removed on {}", | ||
| host.getUuid(), host.getName(), vm.getUuid(), vm.getName(), host.getRemoved()); | ||
| return; | ||
| } | ||
|
|
||
| VolumeInfo volumeInfo = volFactory.getVolume(root.getId()); | ||
|
|
||
| final Command cmd; | ||
|
|
||
| VolumeInfo volumeInfo = volFactory.getVolume(root.getId()); | ||
| if (host.getHypervisorType() == HypervisorType.XenServer) { | ||
| DiskTO disk = new DiskTO(volumeInfo.getTO(), root.getDeviceId(), root.getPath(), root.getVolumeType()); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,7 @@ | |
| import org.apache.cloudstack.acl.SecurityChecker; | ||
| import org.apache.cloudstack.api.ApiCommandResourceType; | ||
| import org.apache.cloudstack.api.ApiConstants; | ||
| import com.cloud.host.Status; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| import org.apache.cloudstack.api.BaseCmd; | ||
| import org.apache.cloudstack.api.BaseCmd.HTTPMethod; | ||
| import org.apache.cloudstack.api.command.admin.vm.AssignVMCmd; | ||
|
|
@@ -90,6 +91,9 @@ | |
| import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; | ||
| import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; | ||
| import org.apache.cloudstack.storage.template.VnfTemplateManager; | ||
| import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService; | ||
| import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; | ||
| import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; | ||
| import org.apache.cloudstack.userdata.UserDataManager; | ||
| import org.apache.cloudstack.vm.UnmanagedVMsManager; | ||
| import org.apache.cloudstack.vm.lease.VMLeaseManager; | ||
|
|
@@ -426,6 +430,15 @@ public class UserVmManagerImplTest { | |
| @Mock | ||
| private VolumeDataFactory volumeDataFactory; | ||
|
|
||
| @Mock | ||
| private VolumeDataFactory volFactory; | ||
|
|
||
| @Mock | ||
| private VolumeOrchestrationService volumeMgr; | ||
|
|
||
| @Mock | ||
| private TemplateDataStoreDao templateDataStoreDao; | ||
|
|
||
| @Mock | ||
| private VolumeInfo volumeInfo; | ||
|
|
||
|
|
@@ -1665,6 +1678,103 @@ public void updateInstanceDetailsMapWithCurrentValuesForAbsentDetailsTestAllCons | |
| Mockito.verify(userVmManagerImpl).addCurrentDetailValueToInstanceDetailsMapIfNewValueWasNotSpecified(Mockito.any(), Mockito.any(), Mockito.eq(VmDetailConstants.CPU_NUMBER), Mockito.any()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testRestoreVirtualMachineWhenHostRemoved() throws ResourceUnavailableException, InsufficientCapacityException, ResourceAllocationException { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also add a test for the host-row-missing case, not just Removed? |
||
| long vmId = 1L; | ||
| Long lastHostId = 42L; | ||
| Long newTemplateId = 2L; | ||
| boolean expunge = false; | ||
| Map<String, String> details = new HashMap<>(); | ||
|
|
||
| UserVmVO vm = mock(UserVmVO.class); | ||
| when(vm.getId()).thenReturn(vmId); | ||
| when(vm.getAccountId()).thenReturn(accountId); | ||
| when(vm.getHostId()).thenReturn(null); | ||
| when(vm.getLastHostId()).thenReturn(lastHostId); | ||
| when(vm.getUuid()).thenReturn("test-uuid"); | ||
| when(vm.getState()).thenReturn(VirtualMachine.State.Stopped); | ||
| when(vm.getTemplateId()).thenReturn(1L); | ||
| when(vm.getDataCenterId()).thenReturn(1L); | ||
| when(vm.isDisplay()).thenReturn(true); | ||
|
|
||
| CallContext mockCallContext = mock(CallContext.class); | ||
| when(mockCallContext.getCallingAccount()).thenReturn(accountMock); | ||
| when(mockCallContext.getCallingUserId()).thenReturn(1L); | ||
|
|
||
| CallContext mockVolumeContext = mock(CallContext.class); | ||
| when(CallContext.register(any(CallContext.class), any(ApiCommandResourceType.class))).thenReturn(mockVolumeContext); | ||
|
|
||
| when(accountDao.findById(accountId)).thenReturn(callerAccount); | ||
| when(accountDao.findByIdIncludingRemoved(accountId)).thenReturn(callerAccount); | ||
| when(callerAccount.getState()).thenReturn(Account.State.ENABLED); | ||
| VMTemplateVO template = mock(VMTemplateVO.class); | ||
| when(templateDao.findById(anyLong())).thenReturn(template); | ||
| when(templateDao.findByIdIncludingRemoved(anyLong())).thenReturn(template); | ||
| when(template.getFormat()).thenReturn(Storage.ImageFormat.QCOW2); | ||
| when(template.getId()).thenReturn(1L); | ||
| when(template.getUuid()).thenReturn("template-uuid"); | ||
| when(template.isDirectDownload()).thenReturn(false); | ||
| when(template.getSize()).thenReturn(10L * 1024 * 1024 * 1024L); // 10GB | ||
|
|
||
| TemplateDataStoreVO templateStore = mock(TemplateDataStoreVO.class); | ||
| when(templateDataStoreDao.findByTemplateZoneReady(1L, 1L)).thenReturn(templateStore); | ||
|
|
||
| ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class); | ||
| when(vm.getServiceOfferingId()).thenReturn(serviceOfferingId); | ||
| when(_serviceOfferingDao.findById(vmId, vm.getServiceOfferingId())).thenReturn(serviceOffering); | ||
|
|
||
| List<VolumeVO> rootVols = new ArrayList<>(); | ||
| VolumeVO rootVol = mock(VolumeVO.class); | ||
| when(rootVol.getId()).thenReturn(10L); | ||
| when(rootVol.getState()).thenReturn(Volume.State.Ready); | ||
| when(rootVol.getPoolId()).thenReturn(5L); | ||
| when(rootVol.getTemplateId()).thenReturn(1L); | ||
| when(rootVol.getSize()).thenReturn(20L * 1024 * 1024 * 1024L); // 20GB | ||
| when(rootVol.getDiskOfferingId()).thenReturn(100L); | ||
| when(rootVol.isDisplay()).thenReturn(true); | ||
| rootVols.add(rootVol); | ||
| DiskOfferingVO diskOffering = mock(DiskOfferingVO.class); | ||
| when(diskOfferingDao.findById(100L)).thenReturn(diskOffering); | ||
|
|
||
| StoragePoolVO storagePool = mock(StoragePoolVO.class); | ||
| when(storagePool.isManaged()).thenReturn(true); | ||
| when(primaryDataStoreDao.findById(5L)).thenReturn(storagePool); | ||
| when(vmSnapshotDaoMock.findByVm(vmId)).thenReturn(new ArrayList<>()); | ||
| when(volumeDaoMock.findByInstanceAndType(vmId, Volume.Type.ROOT)).thenReturn(rootVols); | ||
| when(userVmDao.findById(vmId)).thenReturn(vm); | ||
|
|
||
| HostVO host = mock(HostVO.class); | ||
| when(host.getStatus()).thenReturn(Status.Removed); | ||
| when(hostDao.findByIdIncludingRemoved(lastHostId)).thenReturn(host); | ||
| VolumeInfo volumeInfo = mock(VolumeInfo.class); | ||
| when(volFactory.getVolume(10L)).thenReturn(volumeInfo); | ||
| doNothing().when(resourceLimitMgr).checkVmResourceLimitsForTemplateChange( | ||
| any(Account.class), Mockito.anyBoolean(), any(ServiceOffering.class), | ||
| any(VMTemplateVO.class), any(VMTemplateVO.class), any(List.class)); | ||
| doNothing().when(resourceLimitMgr).checkVolumeResourceLimitForDiskOfferingChange( | ||
| any(Account.class), Mockito.anyBoolean(), anyLong(), anyLong(), | ||
| any(DiskOffering.class), any(DiskOffering.class), any(List.class)); | ||
|
|
||
| VolumeVO newVolume = mock(VolumeVO.class); | ||
| when(volumeMgr.allocateDuplicateVolume(any(VolumeVO.class), any(), anyLong())) | ||
| .thenReturn(newVolume); | ||
| when(newVolume.getId()).thenReturn(11L); | ||
| when(newVolume.getState()).thenReturn(Volume.State.Ready); | ||
| doReturn(20L * 1024 * 1024 * 1024L).when(userVmManagerImpl).getRootVolumeSizeForVmRestore( | ||
| any(Volume.class), any(VMTemplateVO.class), any(UserVmVO.class), | ||
| any(DiskOffering.class), anyMap(), Mockito.anyBoolean()); | ||
|
|
||
| try (MockedStatic<CallContext> ignored = Mockito.mockStatic(CallContext.class)) { | ||
| when(CallContext.current()).thenReturn(mockCallContext); | ||
|
|
||
| UserVm result = userVmManagerImpl.restoreVirtualMachine(accountMock, vmId, newTemplateId, null, expunge, details); | ||
| assertNotNull(result); | ||
| } | ||
|
|
||
| Mockito.verify(userVmDao).findById(vmId); | ||
| Mockito.verify(hostDao).findByIdIncludingRemoved(lastHostId); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCheckVolumesLimits() { | ||
| long diskOffId1 = 1L; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s_loggerdoesn't exist in this class. It only haslogger(see the line right below this one). This won't compile.