From 767537100c16c0d2465b1af3a475aa9ca1424c22 Mon Sep 17 00:00:00 2001 From: Dave Loszewski Date: Tue, 22 Sep 2026 19:19:35 -0400 Subject: [PATCH] storage: only clear a volume's attachment if it is still on the detaching VM The execution-time skip added in this branch reads volume.getInstanceId() at the top of orchestrateDetachVolumeFromVM, but the attachment row is not cleared until after the agent round-trip returns. On a busy host that gap was 2-3 minutes in the environment reported in #14214, and an attach that lands inside it is still cleared by the straggler, so the race is narrowed rather than closed. Make the final clear conditional as well: - VolumeDao.detachVolume(volumeId, vmId) puts both id and instance_id in the search criteria and reports whether a row actually changed, so a volume that moved while the detach was in flight is left alone. This is the same shape as VolumeDaoImpl.updateState and VMInstanceDaoImpl.updateState, which already guard their updates on the state they expect to find. - The caller logs which VM the volume now belongs to, re-reads it, and skips the rest of the success block, since revoking access or publishing a detach event would assert something that is no longer true. This closes the database-write half of the race. It does not close hypervisor-side divergence: the DettachCommand has already succeeded by this point, so on a losing update the disk may be unplugged while the row says attached. Leaving the newer attachment intact is still strictly better than clearing it, and the warning names the volume and both VMs so the inconsistency can be reconciled. Tests cover both halves: the DAO update asserting that instance_id is in the where clause and that both ids are bound, and the service path on the branch that actually performs the agent round-trip, asserting that a skipped stale detach performs no disk statistics collection and publishes no usage event. Refs #14214, #14401 Signed-off-by: Dave Loszewski --- .../java/com/cloud/storage/dao/VolumeDao.java | 7 + .../com/cloud/storage/dao/VolumeDaoImpl.java | 17 +++ .../cloud/storage/dao/VolumeDaoImplTest.java | 68 +++++++++ .../cloud/storage/VolumeApiServiceImpl.java | 11 +- .../storage/VolumeApiServiceImplTest.java | 133 ++++++++++++++++++ 5 files changed, 234 insertions(+), 2 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java index 4cd9a8e23bfc..8db88a18a41a 100644 --- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java @@ -68,6 +68,13 @@ public interface VolumeDao extends GenericDao, StateDao findCreatedByInstance(long id); diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java index 1d5ff5e93402..bce04c345950 100644 --- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java @@ -347,6 +347,23 @@ public void detachVolume(long volumeId) { update(volumeId, volume); } + @Override + public boolean detachVolume(long volumeId, long vmId) { + VolumeVO existing = findById(volumeId); + VolumeVO volume = createForUpdate(volumeId); + volume.setInstanceId(null); + volume.setDeviceId(null); + volume.setUpdated(new Date()); + volume.setAttached(null); + if (existing != null && existing.getVolumeType() == Type.ROOT) { + volume.setVolumeType(Type.DATADISK); + } + SearchCriteria sc = AllFieldsSearch.create(); + sc.setParameters("id", volumeId); + sc.setParameters("instanceId", vmId); + return update(volume, sc) > 0; + } + @Override @DB public HypervisorType getHypervisorType(long volumeId) { diff --git a/engine/schema/src/test/java/com/cloud/storage/dao/VolumeDaoImplTest.java b/engine/schema/src/test/java/com/cloud/storage/dao/VolumeDaoImplTest.java index 6f153727ab76..0a5af6dc5098 100644 --- a/engine/schema/src/test/java/com/cloud/storage/dao/VolumeDaoImplTest.java +++ b/engine/schema/src/test/java/com/cloud/storage/dao/VolumeDaoImplTest.java @@ -35,6 +35,7 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.Mockito; @@ -43,6 +44,8 @@ import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; +import com.cloud.utils.Pair; +import com.cloud.utils.db.Attribute; import com.cloud.utils.db.Filter; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; @@ -205,4 +208,69 @@ public void testSearchRemovedByVms() { Mockito.any(SearchCriteria.class), Mockito.any(Filter.class), Mockito.eq(null), Mockito.eq(false)); } + + private static final long DETACHED_VOLUME_ID = 1L; + private static final long DETACHED_FROM_VM_ID = 2L; + + private void mockVolumeForConditionalDetach(Volume.Type existingType, int updatedRows) { + VolumeVO existing = Mockito.mock(VolumeVO.class); + Mockito.doReturn(existingType).when(existing).getVolumeType(); + Mockito.doReturn(existing).when(volumeDao).findById(DETACHED_VOLUME_ID); + Mockito.doReturn(updatedRows).when(volumeDao).update(Mockito.any(VolumeVO.class), Mockito.>any()); + } + + @SuppressWarnings("unchecked") + private SearchCriteria captureConditionalDetachCriteria() { + ArgumentCaptor> criteria = ArgumentCaptor.forClass(SearchCriteria.class); + Mockito.verify(volumeDao).update(Mockito.any(VolumeVO.class), criteria.capture()); + return criteria.getValue(); + } + + private List boundValuesOf(SearchCriteria criteria) { + List values = new ArrayList<>(); + for (Pair bound : criteria.getValues()) { + values.add(bound.second()); + } + return values; + } + + @Test + public void detachVolumeTestRestrictsTheUpdateToTheVmTheVolumeIsStillAttachedTo() { + mockVolumeForConditionalDetach(Volume.Type.DATADISK, 1); + + volumeDao.detachVolume(DETACHED_VOLUME_ID, DETACHED_FROM_VM_ID); + + SearchCriteria criteria = captureConditionalDetachCriteria(); + Assert.assertTrue("the update must be restricted by instance_id, otherwise a stale detach clears a newer attachment; where clause was: " + + criteria.getWhereClause(), criteria.getWhereClause().contains("instance_id")); + List bound = boundValuesOf(criteria); + Assert.assertEquals("expected the volume id and the vm id to be bound, got " + bound, 2, bound.size()); + Assert.assertTrue("the vm id must be bound, got " + bound, bound.contains(DETACHED_FROM_VM_ID)); + Assert.assertTrue("the volume id must be bound, got " + bound, bound.contains(DETACHED_VOLUME_ID)); + } + + @Test + public void detachVolumeTestClearsAttachmentWhenStillAttachedToVm() { + mockVolumeForConditionalDetach(Volume.Type.DATADISK, 1); + + Assert.assertTrue(volumeDao.detachVolume(DETACHED_VOLUME_ID, DETACHED_FROM_VM_ID)); + } + + @Test + public void detachVolumeTestKeepsAttachmentWhenAttachedToAnotherVm() { + mockVolumeForConditionalDetach(Volume.Type.DATADISK, 0); + + Assert.assertFalse(volumeDao.detachVolume(DETACHED_VOLUME_ID, DETACHED_FROM_VM_ID)); + } + + @Test + public void detachVolumeTestConvertsADetachedRootVolumeToADataDisk() { + mockVolumeForConditionalDetach(Volume.Type.ROOT, 1); + + volumeDao.detachVolume(DETACHED_VOLUME_ID, DETACHED_FROM_VM_ID); + + ArgumentCaptor updated = ArgumentCaptor.forClass(VolumeVO.class); + Mockito.verify(volumeDao).update(updated.capture(), Mockito.>any()); + Assert.assertEquals(Volume.Type.DATADISK, updated.getValue().getVolumeType()); + } } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index e19c11e18068..69ca4926e585 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -3626,8 +3626,15 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { } if (!sendCommand || (answer != null && answer.getResult())) { - // Mark the volume as detached - _volsDao.detachVolume(volume.getId()); + // Mark the volume as detached, only if it is still attached to this VM. The host has already been told to + // unplug the disk by this point, so the remainder of this block is deliberately skipped when the volume + // moved: revoking access or publishing a detach event would assert something that is no longer true. + if (!_volsDao.detachVolume(volume.getId(), vmId)) { + Volume current = _volsDao.findById(volumeId); + logger.warn("Volume {} is no longer attached to VM {} (its attachment now points at VM id {}); leaving it unchanged", + volume, vm, current == null ? null : current.getInstanceId()); + return current; + } if (answer != null) { String datastoreName = answer.getContextParam("datastoreName"); diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index b40d8232c6a5..e32f6c2141c0 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -37,7 +37,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -73,6 +75,7 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao; import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreVO; +import org.apache.cloudstack.storage.to.VolumeObjectTO; import org.apache.commons.collections.CollectionUtils; import org.junit.After; import org.junit.Assert; @@ -89,8 +92,12 @@ import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.verification.VerificationMode; import org.springframework.test.util.ReflectionTestUtils; +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.Command; import com.cloud.api.query.dao.ServiceOfferingJoinDao; import com.cloud.configuration.ConfigurationManager; import com.cloud.configuration.Resource.ResourceType; @@ -141,6 +148,7 @@ import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.DiskProfile; import com.cloud.vm.UserVmManager; +import com.cloud.vm.UserVmService; import com.cloud.vm.UserVmVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; @@ -203,6 +211,10 @@ public class VolumeApiServiceImplTest { @Mock private UserVmManager userVmManager; @Mock + private UserVmService userVmServiceMock; + @Mock + private AgentManager agentManagerMock; + @Mock private DataCenterDao _dcDao; @Mock private ResourceLimitService resourceLimitServiceMock; @@ -2941,4 +2953,125 @@ public void testResizeVolumeInternal_VMware_VMRunning_ShouldThrowStateGuardError e.getMessage() != null && e.getMessage().contains("VM should be in")); } } + + /** + * Sets up a detach of volume 100 from the stopped VM (id 2) that setUp() returns for any VM lookup. The stubs cover + * the whole detach path so that, without the stale-detach guard, the tests fail on their assertions rather than + * on a missing mock. + */ + private VolumeVO mockVolumeForDetachFromStoppedVm(Long attachedVmId) { + VolumeVO volume = Mockito.mock(VolumeVO.class); + lenient().doReturn(100L).when(volume).getId(); + Mockito.doReturn(attachedVmId).when(volume).getInstanceId(); + lenient().doReturn(null).when(volume).getPoolId(); + Mockito.doReturn(volume).when(volumeDaoMock).findById(100L); + lenient().when(virtualMachineManager.findClusterAndHostIdForVm(anyLong())).thenReturn(new Pair<>(null, null)); + return volume; + } + + private void verifyDetachUsageEvent(MockedStatic usageEventUtilsMocked, VerificationMode mode) { + usageEventUtilsMocked.verify(() -> UsageEventUtils.publishUsageEvent(eq(EventTypes.EVENT_VOLUME_DETACH), anyLong(), anyLong(), anyLong(), + nullable(String.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(String.class), nullable(String.class), + nullable(Long.class), anyBoolean()), mode); + } + + private void verifyStaleDetachSkipped(MockedStatic usageEventUtilsMocked) throws Exception { + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(anyLong()); + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(anyLong(), anyLong()); + Mockito.verify(virtualMachineManager, Mockito.never()).findClusterAndHostIdForVm(anyLong()); + Mockito.verify(userVmManager, Mockito.never()).collectVmDiskStatistics(any()); + Mockito.verify(agentManagerMock, Mockito.never()).send(nullable(Long.class), any(Command.class)); + verifyDetachUsageEvent(usageEventUtilsMocked, Mockito.never()); + } + + @Test + public void detachVolumeViaDestroyVMTestSkipsDetachWhenVolumeAttachedToAnotherVm() throws Exception { + VolumeVO volume = mockVolumeForDetachFromStoppedVm(3L); + + try (MockedStatic usageEventUtilsMocked = Mockito.mockStatic(UsageEventUtils.class)) { + Volume result = volumeApiServiceImpl.detachVolumeViaDestroyVM(2L, 100L); + + Assert.assertSame(volume, result); + verifyStaleDetachSkipped(usageEventUtilsMocked); + } + } + + @Test + public void detachVolumeViaDestroyVMTestSkipsDetachWhenVolumeAlreadyDetached() throws Exception { + VolumeVO volume = mockVolumeForDetachFromStoppedVm(null); + + try (MockedStatic usageEventUtilsMocked = Mockito.mockStatic(UsageEventUtils.class)) { + Volume result = volumeApiServiceImpl.detachVolumeViaDestroyVM(2L, 100L); + + Assert.assertSame(volume, result); + verifyStaleDetachSkipped(usageEventUtilsMocked); + } + } + + @Test + public void detachVolumeViaDestroyVMTestClearsAttachmentWhenStillAttachedToVm() { + mockVolumeForDetachFromStoppedVm(2L); + Mockito.doReturn(true).when(volumeDaoMock).detachVolume(100L, 2L); + + try (MockedStatic usageEventUtilsMocked = Mockito.mockStatic(UsageEventUtils.class)) { + volumeApiServiceImpl.detachVolumeViaDestroyVM(2L, 100L); + + Mockito.verify(volumeDaoMock).detachVolume(100L, 2L); + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(anyLong()); + verifyDetachUsageEvent(usageEventUtilsMocked, Mockito.times(1)); + } + } + + @Test + public void detachVolumeViaDestroyVMTestKeepsAttachmentWhenReattachedDuringDetach() { + mockVolumeForDetachFromStoppedVm(2L); + Mockito.doReturn(false).when(volumeDaoMock).detachVolume(100L, 2L); + + try (MockedStatic usageEventUtilsMocked = Mockito.mockStatic(UsageEventUtils.class)) { + volumeApiServiceImpl.detachVolumeViaDestroyVM(2L, 100L); + + Mockito.verify(volumeDaoMock).detachVolume(100L, 2L); + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(anyLong()); + verifyDetachUsageEvent(usageEventUtilsMocked, Mockito.never()); + } + } + + /** + * The race this guard exists for only happens on the path that talks to the host: the attachment row is not + * cleared until after the agent round-trip returns, and a newer attach can land inside that window. The other + * tests here run against a stopped VM, so they never enter that branch. This one forces it. + */ + @Test + public void detachVolumeViaDestroyVMTestKeepsAttachmentWhenReattachedDuringTheAgentRoundTrip() throws Exception { + VolumeVO volume = mockVolumeForDetachFromStoppedVm(2L); + Mockito.doReturn(1L).when(volume).getPoolId(); + + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findByIdIncludingRemoved(1L); + Mockito.doReturn(pool).when(primaryDataStoreDaoMock).findById(1L); + Mockito.doReturn(true).when(volumeApiServiceImpl).isSendCommandForVmVolumeAttachDetach(nullable(HostVO.class), Mockito.eq(pool)); + + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Mockito.doReturn(Mockito.mock(VolumeObjectTO.class)).when(volumeInfo).getTO(); + Mockito.doReturn(volumeInfo).when(volumeDataFactoryMock).getVolume(100L); + Mockito.doReturn(new Pair, Set>(new ArrayList<>(), new HashSet<>())) + .when(volumeOrchestrationService).getVolumeCheckpointPathsAndImageStoreUrls(Mockito.eq(100L), nullable(HypervisorType.class)); + + Answer answer = Mockito.mock(Answer.class); + Mockito.doReturn(true).when(answer).getResult(); + Mockito.doReturn(answer).when(agentManagerMock).send(nullable(Long.class), any(Command.class)); + + // the volume moved to another VM while the host was being asked to unplug it + Mockito.doReturn(false).when(volumeDaoMock).detachVolume(100L, 2L); + + try (MockedStatic usageEventUtilsMocked = Mockito.mockStatic(UsageEventUtils.class)) { + volumeApiServiceImpl.detachVolumeViaDestroyVM(2L, 100L); + + // the round-trip really happened - otherwise this test proves nothing the others do not + Mockito.verify(agentManagerMock).send(nullable(Long.class), any(Command.class)); + Mockito.verify(volumeDaoMock).detachVolume(100L, 2L); + Mockito.verify(volumeDaoMock, Mockito.never()).detachVolume(anyLong()); + verifyDetachUsageEvent(usageEventUtilsMocked, Mockito.never()); + } + } }