Skip to content

Skip stale destroyed volumes in local storage checks #13125

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.

Sign up for GitHub

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

Draft
andrijapanicsb wants to merge 3 commits into
base: main
Choose a base branch
from
+55 −3
Draft

Skip stale destroyed volumes in local storage checks #13125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d0725c
Skip stale destroyed volumes in local storage checks
andrijapanicsb May 7, 2026
9b3370c
generic solution
May 11, 2026
e60ca2e
remove tests
May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S

List<VolumeVO> findByInstance(long id);

List<VolumeVO> findByInstanceAndNotDestroyed(long id);

List<VolumeVO> findByInstanceAndType(long id, Volume.Type vType);

List<VolumeVO> findByInstanceAndNotStates(long id, Volume.State...states);
Expand Down
8 changes: 8 additions & 0 deletions engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ public List<VolumeVO> findByInstance(long id) {
return listBy(sc);
}

@Override
public List<VolumeVO> findByInstanceAndNotDestroyed(long id) {
SearchCriteria<VolumeVO> sc = AllFieldsSearch.create();
sc.setParameters("instanceId", id);
sc.setParameters("notDestroyed", Volume.State.Destroy, Volume.State.Expunged);
return listBy(sc);
}

@Override
public List<VolumeVO> findByInstanceAndDeviceId(long instanceId, long deviceId) {
SearchCriteria<VolumeVO> sc = AllFieldsSearch.create();
Expand Down
16 changes: 13 additions & 3 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7510,7 +7510,7 @@ private void checkDestinationHypervisorType(StoragePool destPool, VMInstanceVO v
}

public boolean isVMUsingLocalStorage(VMInstanceVO vm) {
List<VolumeVO> volumes = _volsDao.findByInstance(vm.getId());
List<VolumeVO> volumes = _volsDao.findByInstanceAndNotDestroyed(vm.getId());
return isAnyVmVolumeUsingLocalStorage(volumes);
}

Expand Down Expand Up @@ -7946,10 +7946,20 @@ private boolean isImplicitPlannerUsedByOffering(long offeringId) {
protected boolean isAnyVmVolumeUsingLocalStorage(final List<VolumeVO> volumes) {
for (VolumeVO vol : volumes) {
DiskOfferingVO diskOffering = _diskOfferingDao.findById(vol.getDiskOfferingId());
if (diskOffering.isUseLocalStorage()) {
if (diskOffering != null && diskOffering.isUseLocalStorage()) {
return true;
}
StoragePoolVO storagePool = _storagePoolDao.findById(vol.getPoolId());
Long poolId = vol.getPoolId();
Comment on lines 7948 to +7952
if (poolId == null) {
logger.debug("Skipping volume without storage pool while checking local storage usage: {}", vol);
continue;
}
StoragePoolVO storagePool = _storagePoolDao.findById(poolId);
if (storagePool == null || storagePool.getRemoved() != null) {
throw new CloudRuntimeException(String.format(
"Cannot determine local storage usage for active volume %s because storage pool ID %s is missing or removed",
vol, poolId));
}
Comment on lines +7957 to +7962

@DaanHoogland DaanHoogland Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about when it is not attached to a running VM? might it be valid to still have a already removed pool?

@Pearl1594 Pearl1594 Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method only ever gets volumes attached to the VM in question (that's being migrated)

if (storagePool.isLocal()) {
return true;
}
Expand Down
32 changes: 32 additions & 0 deletions server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,38 @@
}
}

@Test
public void testIsAnyVmVolumeUsingLocalStorageFailsForActiveVolumeWithMissingPool() {
VolumeVO volume = Mockito.mock(VolumeVO.class);

Check warning on line 1330 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhZ&open=AaFpnUk_iKhD6UlogDhZ&pullRequest=13125
Mockito.when(volume.getDiskOfferingId()).thenReturn(1L);

Check warning on line 1331 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDha&open=AaFpnUk_iKhD6UlogDha&pullRequest=13125
Mockito.when(volume.getPoolId()).thenReturn(2L);

Check warning on line 1332 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhb&open=AaFpnUk_iKhD6UlogDhb&pullRequest=13125
DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class);

Check warning on line 1333 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhc&open=AaFpnUk_iKhD6UlogDhc&pullRequest=13125
Mockito.when(diskOfferingDao.findById(1L)).thenReturn(diskOffering);

Check warning on line 1334 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhd&open=AaFpnUk_iKhD6UlogDhd&pullRequest=13125
Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false);

Check warning on line 1335 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhe&open=AaFpnUk_iKhD6UlogDhe&pullRequest=13125
Mockito.when(primaryDataStoreDao.findById(2L)).thenReturn(null);

Check warning on line 1336 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhf&open=AaFpnUk_iKhD6UlogDhf&pullRequest=13125

CloudRuntimeException exception = assertThrows(CloudRuntimeException.class, () ->
userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(Collections.singletonList(volume)));
Assert.assertTrue(exception.getMessage().contains("storage pool ID 2 is missing or removed"));
}

@Test
public void testIsAnyVmVolumeUsingLocalStorageFailsForActiveVolumeWithRemovedPool() {
VolumeVO volume = Mockito.mock(VolumeVO.class);

Check warning on line 1345 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhg&open=AaFpnUk_iKhD6UlogDhg&pullRequest=13125
Mockito.when(volume.getDiskOfferingId()).thenReturn(1L);

Check warning on line 1346 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhh&open=AaFpnUk_iKhD6UlogDhh&pullRequest=13125
Mockito.when(volume.getPoolId()).thenReturn(2L);

Check warning on line 1347 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhi&open=AaFpnUk_iKhD6UlogDhi&pullRequest=13125
DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class);

Check warning on line 1348 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhj&open=AaFpnUk_iKhD6UlogDhj&pullRequest=13125
Mockito.when(diskOfferingDao.findById(1L)).thenReturn(diskOffering);

Check warning on line 1349 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhk&open=AaFpnUk_iKhD6UlogDhk&pullRequest=13125
Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false);

Check warning on line 1350 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhl&open=AaFpnUk_iKhD6UlogDhl&pullRequest=13125
StoragePoolVO storagePool = Mockito.mock(StoragePoolVO.class);
Mockito.when(storagePool.getRemoved()).thenReturn(new Date());

Check warning on line 1352 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhm&open=AaFpnUk_iKhD6UlogDhm&pullRequest=13125
Mockito.when(primaryDataStoreDao.findById(2L)).thenReturn(storagePool);

Check warning on line 1353 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "when". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDhn&open=AaFpnUk_iKhD6UlogDhn&pullRequest=13125

CloudRuntimeException exception = assertThrows(CloudRuntimeException.class, () ->
userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(Collections.singletonList(volume)));
Assert.assertTrue(exception.getMessage().contains("storage pool ID 2 is missing or removed"));
}

private List<VolumeVO> mockVolumesForIsAllVmVolumesOnZoneWideStore(int nullPoolIdVolumes, int nullPoolVolumes, int zoneVolumes, int nonZoneVolumes) {
List<VolumeVO> volumes = new ArrayList<>();
for (int i=0; i< nullPoolIdVolumes + nullPoolVolumes + zoneVolumes + nonZoneVolumes; ++i) {
Expand All @@ -1340,7 +1372,7 @@
Mockito.when(primaryDataStoreDao.findById(index)).thenReturn(null);
continue;
}
StoragePoolVO storagePool = Mockito.mock(StoragePoolVO.class);

Check warning on line 1375 in server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AaFpnUk_iKhD6UlogDho&open=AaFpnUk_iKhD6UlogDho&pullRequest=13125
Mockito.when(primaryDataStoreDao.findById(index)).thenReturn(storagePool);
if (i < zoneVolumes) {
Mockito.when(storagePool.getScope()).thenReturn(ScopeType.ZONE);
Expand Down
Loading