From 9e4cee97dfd3e220fabd9812c92addfdecd7708f Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Wed, 14 Oct 2026 08:02:44 -0400 Subject: [PATCH 1/2] skip a queued volume detach when the volume is no longer on that vm --- .../cloud/storage/VolumeApiServiceImpl.java | 6 +++++ .../storage/VolumeApiServiceImplTest.java | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 8674014addd7..2a28d8af62c7 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -3559,6 +3559,12 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { Volume volume = _volsDao.findById(volumeId); VMInstanceVO vm = _vmInstanceDao.findById(vmId); + // A queued detach can run after the volume was already detached and attached elsewhere; leave that attachment alone + if (volume.getInstanceId() == null || volume.getInstanceId() != vmId) { + logger.warn("Volume {} is no longer attached to VM {}, skipping detach", 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..01e91494cda6 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -619,6 +619,30 @@ 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()); + } + /** * TESTS FOR ATTACH ROOT VOLUME, COUNT=7 */ From 3ac4425bf2af11888972228d6ea882f157c00362 Mon Sep 17 00:00:00 2001 From: Daman Arora <61474540+Damans227@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:09:50 -0400 Subject: [PATCH 2/2] move the stale detach check into shouldDetachBeSkipped --- .../cloud/storage/VolumeApiServiceImpl.java | 18 +++++++++++++--- .../storage/VolumeApiServiceImplTest.java | 21 +++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 2a28d8af62c7..e19c11e18068 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -3555,13 +3555,25 @@ 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); - // A queued detach can run after the volume was already detached and attached elsewhere; leave that attachment alone - if (volume.getInstanceId() == null || volume.getInstanceId() != vmId) { - logger.warn("Volume {} is no longer attached to VM {}, skipping detach", volume, vm); + if (shouldDetachBeSkipped(volume, vm)) { return volume; } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 01e91494cda6..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; @@ -643,6 +644,26 @@ public void testDetachVolumeSkippedWhenAlreadyDetached() { 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 */