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 1 commit into
base: main
Choose a base branch
from
+70 −2
Draft

Skip stale destroyed volumes in local storage checks #13125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 18 additions & 2 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 @@ -1797,7 +1797,7 @@
}

@Override
public UserVm updateNicIpForVirtualMachine(UpdateVmNicIpCmd cmd) {

Check warning on line 1800 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 93 to 64, Complexity from 22 to 14, Nesting Level from 5 to 2, Number of Variables from 18 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCU&open=AZ4DWmx1_sdxmytL1HCU&pullRequest=13125
Long nicId = cmd.getNicId();
String ipaddr = cmd.getIpaddress();
Account caller = CallContext.current().getCallingAccount();
Expand Down Expand Up @@ -2072,7 +2072,7 @@
return upgradeRunningVirtualMachine(vmId, newServiceOfferingId, customParameters);
}

private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingId, Map<String, String> customParameters) throws ResourceUnavailableException,

Check warning on line 2075 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 116 to 64, Complexity from 25 to 14, Nesting Level from 5 to 2, Number of Variables from 35 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCS&open=AZ4DWmx1_sdxmytL1HCS&pullRequest=13125
ConcurrentOperationException, ManagementServerException, VirtualMachineMigrationException {

Account caller = CallContext.current().getCallingAccount();
Expand Down Expand Up @@ -2930,7 +2930,7 @@

@Override
@ActionEvent(eventType = EventTypes.EVENT_VM_UPDATE, eventDescription = "updating Vm")
public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableException, InsufficientCapacityException {

Check warning on line 2933 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 130 to 64, Complexity from 50 to 14, Nesting Level from 5 to 2, Number of Variables from 35 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCR&open=AZ4DWmx1_sdxmytL1HCR&pullRequest=13125
validateInputsAndPermissionForUpdateVirtualMachineCommand(cmd);

String displayName = cmd.getDisplayName();
Expand Down Expand Up @@ -4863,7 +4863,7 @@
return _instance + "-" + uuidName;
}

private UserVmVO commitUserVm(final boolean isImport, final DataCenter zone, final Host host, final Host lastHost, final VirtualMachineTemplate template, final String hostName, final String displayName, final Account owner,

Check warning on line 4866 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 115 to 64, Complexity from 30 to 14, Nesting Level from 3 to 2, Number of Variables from 56 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCX&open=AZ4DWmx1_sdxmytL1HCX&pullRequest=13125
final Long diskOfferingId, final Long diskSize, final String userData, Long userDataId, String userDataDetails, final Boolean isDisplayVm, final String keyboard,
final long accountId, final long userId, final ServiceOffering offering, final boolean isIso, final Long guestOsId, final String sshPublicKeys, final LinkedHashMap<String, List<NicProfile>> networkNicMap,
final long id, final String instanceName, final String uuidName, final HypervisorType hypervisorType, final Map<String, String> customParameters,
Expand Down Expand Up @@ -5554,7 +5554,7 @@
}

@Override
public boolean finalizeStart(VirtualMachineProfile profile, long hostId, Commands cmds, ReservationContext context) {

Check warning on line 5557 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 99 to 64, Complexity from 25 to 14, Nesting Level from 5 to 2, Number of Variables from 32 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCT&open=AZ4DWmx1_sdxmytL1HCT&pullRequest=13125
UserVmVO vm = _vmDao.findById(profile.getId());

Answer[] answersToCmds = cmds.getAnswers();
Expand Down Expand Up @@ -7490,7 +7490,7 @@
return null;
}

public void checkHostsDedication(VMInstanceVO vm, long srcHostId, long destHostId) {

Check warning on line 7493 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 84 to 64, Complexity from 28 to 14, Nesting Level from 5 to 2, Number of Variables from 23 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCV&open=AZ4DWmx1_sdxmytL1HCV&pullRequest=13125
HostVO srcHost = _hostDao.findById(srcHostId);
HostVO destHost = _hostDao.findById(destHostId);
boolean srcExplDedicated = checkIfHostIsDedicated(srcHost);
Expand Down Expand Up @@ -7659,12 +7659,28 @@
}

protected boolean isAnyVmVolumeUsingLocalStorage(final List<VolumeVO> volumes) {
for (VolumeVO vol : volumes) {

Check warning on line 7662 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Reduce the total number of break and continue statements in this loop to use at most one. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCQ&open=AZ4DWmx1_sdxmytL1HCQ&pullRequest=13125
if (vol == null || vol.getRemoved() != null ||
Volume.State.Destroy.equals(vol.getState()) ||
Volume.State.Expunged.equals(vol.getState())) {
logger.debug("Skipping non-active volume while checking local storage usage: {}", vol);
continue;
}
Comment on lines +7663 to +7668
Copy link
Copy Markdown
Contributor

@Pearl1594 Pearl1594 May 7, 2026

Choose a reason for hiding this comment

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

I'm wondering if it would rather be better to have a method is the VolumeDao to only return the active / non-removed volumes of a VM. To avoid the filtering here. Just a thought - that way we can reuse the logic in future as well.

Copy link
Copy Markdown
Contributor Author

@andrijapanicsb andrijapanicsb May 7, 2026

Choose a reason for hiding this comment

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

You are the Java guru, not me :)

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 7669 to +7673
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));
}
if (storagePool.isLocal()) {
return true;
}
Expand Down Expand Up @@ -9276,7 +9292,7 @@
}
}

private void handleManagedStorage(UserVmVO vm, VolumeVO root) {

Check warning on line 9295 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

A "Brain Method" was detected. Refactor it to reduce at least one of the following metrics: LOC from 65 to 64, Complexity from 17 to 14, Nesting Level from 6 to 2, Number of Variables from 15 to 6. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWmx1_sdxmytL1HCY&open=AZ4DWmx1_sdxmytL1HCY&pullRequest=13125
if (Volume.State.Allocated.equals(root.getState())) {
return;
}
Expand Down
52 changes: 52 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 @@ -1264,6 +1264,58 @@
}
}

@Test
public void testIsAnyVmVolumeUsingLocalStorageSkipsDestroyedVolumeWithMissingPool() {
VolumeVO volume = Mockito.mock(VolumeVO.class);
Mockito.when(volume.getState()).thenReturn(Volume.State.Destroy);

Assert.assertFalse(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(Collections.singletonList(volume)));
Mockito.verify(primaryDataStoreDao, never()).findById(anyLong());
}

@Test
public void testIsAnyVmVolumeUsingLocalStorageSkipsRemovedVolume() {
VolumeVO volume = Mockito.mock(VolumeVO.class);
Mockito.when(volume.getRemoved()).thenReturn(new Date());

Assert.assertFalse(userVmManagerImpl.isAnyVmVolumeUsingLocalStorage(Collections.singletonList(volume)));
Mockito.verify(primaryDataStoreDao, never()).findById(anyLong());
}

@Test
public void testIsAnyVmVolumeUsingLocalStorageFailsForActiveVolumeWithMissingPool() {
VolumeVO volume = Mockito.mock(VolumeVO.class);
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
Mockito.when(volume.getDiskOfferingId()).thenReturn(1L);
Mockito.when(volume.getPoolId()).thenReturn(2L);
DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class);
Mockito.when(diskOfferingDao.findById(1L)).thenReturn(diskOffering);
Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false);
Mockito.when(primaryDataStoreDao.findById(2L)).thenReturn(null);

CloudRuntimeException exception = assertThrows(CloudRuntimeException.class, () ->

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

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor the code of the lambda to have only one invocation possibly throwing a runtime exception. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWm6H_sdxmytL1HCZ&open=AZ4DWm6H_sdxmytL1HCZ&pullRequest=13125
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);
Mockito.when(volume.getState()).thenReturn(Volume.State.Ready);
Mockito.when(volume.getDiskOfferingId()).thenReturn(1L);
Mockito.when(volume.getPoolId()).thenReturn(2L);
DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class);
Mockito.when(diskOfferingDao.findById(1L)).thenReturn(diskOffering);
Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false);
StoragePoolVO storagePool = Mockito.mock(StoragePoolVO.class);
Mockito.when(storagePool.getRemoved()).thenReturn(new Date());
Mockito.when(primaryDataStoreDao.findById(2L)).thenReturn(storagePool);

CloudRuntimeException exception = assertThrows(CloudRuntimeException.class, () ->

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

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor the code of the lambda to have only one invocation possibly throwing a runtime exception. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ4DWm6H_sdxmytL1HCa&open=AZ4DWm6H_sdxmytL1HCa&pullRequest=13125
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 Down
Loading