diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 8674014addd7..e19c11e18068 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -3555,10 +3555,28 @@ public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) { return orchestrateDetachVolumeFromVM(vmId, volumeId); } + /** + * A queued detach can run after the volume was already detached and attached elsewhere. + * In that case the detach must be skipped, so the newer attachment is left alone. + * + * @return true if the volume is no longer attached to the given VM + */ + protected boolean shouldDetachBeSkipped(Volume volume, VMInstanceVO vm) { + if (volume.getInstanceId() == null || volume.getInstanceId() != vm.getId()) { + logger.warn("Volume {} is no longer attached to VM {}, skipping detach", volume, vm); + return true; + } + return false; + } + private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { Volume volume = _volsDao.findById(volumeId); VMInstanceVO vm = _vmInstanceDao.findById(vmId); + if (shouldDetachBeSkipped(volume, vm)) { + return volume; + } + String errorMsg = "Failed to detach volume " + volume.getName() + " from VM " + vm.getHostName(); boolean sendCommand = vm.getState() == State.Running; diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 12258e2cc160..b40d8232c6a5 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -144,6 +144,7 @@ import com.cloud.vm.UserVmVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachineManager; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; @@ -619,6 +620,50 @@ public void testDetachVolumeFromStoppedXenVm() throws NoSuchFieldException, Ille volumeApiServiceImpl.detachVolumeFromVM(detachCmd); } + @Test + public void testDetachVolumeSkippedWhenAttachedToAnotherVm() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getInstanceId()).thenReturn(200L); + Mockito.when(volumeDaoMock.findById(100L)).thenReturn(volume); + + Volume result = volumeApiServiceImpl.detachVolumeViaDestroyVM(300L, 100L); + + Assert.assertEquals(volume, result); + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(Mockito.anyLong()); + } + + @Test + public void testDetachVolumeSkippedWhenAlreadyDetached() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getInstanceId()).thenReturn(null); + Mockito.when(volumeDaoMock.findById(100L)).thenReturn(volume); + + Volume result = volumeApiServiceImpl.detachVolumeViaDestroyVM(300L, 100L); + + Assert.assertEquals(volume, result); + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(Mockito.anyLong()); + } + + @Test + public void testShouldDetachBeSkippedFalseWhenAttachedToSameVm() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getInstanceId()).thenReturn(300L); + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(300L); + + Assert.assertFalse(volumeApiServiceImpl.shouldDetachBeSkipped(volume, vm)); + } + + @Test + public void testShouldDetachBeSkippedTrueWhenAttachedToAnotherVm() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getInstanceId()).thenReturn(200L); + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(300L); + + Assert.assertTrue(volumeApiServiceImpl.shouldDetachBeSkipped(volume, vm)); + } + /** * TESTS FOR ATTACH ROOT VOLUME, COUNT=7 */