Skip stale destroyed volumes in local storage checks#13125
Skip stale destroyed volumes in local storage checks#13125andrijapanicsb wants to merge 1 commit intomainfrom
Conversation
andrijapanicsb
requested review from
DaanHoogland and
harikrishna-patnala
and removed request for
DaanHoogland
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13125 +/- ##
=========================================
Coverage 18.09% 18.09%
- Complexity 16721 16725 +4
=========================================
Files 6037 6037
Lines 542546 542557 +11
Branches 66431 66436 +5
=========================================
+ Hits 98159 98168 +9
+ Misses 433368 433366 -2
- Partials 11019 11023 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
Fixes a NullPointerException in UserVmManagerImpl.isAnyVmVolumeUsingLocalStorage that can occur during VM migration/host maintenance when stale destroyed volume rows reference storage pools that have been removed. The change filters out non-active volume rows and replaces the prior implicit NPE with an explicit CloudRuntimeException for inconsistent active volumes.
Changes:
- Skip null/removed/destroyed/expunged volume rows when checking whether a VM uses local storage.
- For active volumes, throw a clear
CloudRuntimeExceptionwhen the referenced storage pool is missing or removed. - Add unit tests covering the skip behavior and the new failure modes for active volumes with missing/removed pools.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java |
Adds filtering for non-active volumes and explicit error handling when an active volume references a missing/removed pool. |
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java |
Adds unit tests for skipping stale volume rows and for clear failures on active volumes with invalid pool references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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(); |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You are the Java guru, not me :)
Description
During VM migration and host maintenance flows,
UserVmManagerImpl.isAnyVmVolumeUsingLocalStoragechecks VM volumes to decide whether the VM uses local storage. If a stale destroyed volume row still references a storage pool that has since been removed, the method can dereference a nullStoragePoolVOand fail with an NPE.This change skips non-active volume rows while evaluating local-storage usage:
removedsetDestroystateExpungedstateFor active volumes, the method now fails with a clear
CloudRuntimeExceptionif the referenced storage pool is missing or removed instead of throwing a null-pointer exception. This keeps stale destroyed metadata from blocking unrelated VM/host operations while still surfacing real active-volume inconsistency.Fixes #13124
Tests
Added unit coverage in
UserVmManagerImplTestfor:##########################################################
Generated by AI - do you REALLY think I became a Java developer overnight?
So, REVIEW PROPERLY and feel free to decline the PR if it's bunch of junk !
#########################################################