Repository navigation
Skip stale destroyed volumes in local storage checks - #13125
andrijapanicsb wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13125 +/- ##
============================================
- Coverage 20.45% 20.44% -0.01%
Complexity 21413 21413
============================================
Files 6487 6487
Lines 589337 589349 +12
Branches 72248 72250 +2
============================================
- Hits 120524 120521 -3
- Misses 455677 455692 +15
Partials 13136 13136
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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(); |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17815 |
|
| 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)); | ||
| } |
There was a problem hiding this comment.
how about when it is not attached to a running VM? might it be valid to still have a already removed pool?
There was a problem hiding this comment.
this method only ever gets volumes attached to the VM in question (that's being migrated)
|
@Pearl1594 , any action to get this moving forwards needed? Do we need to re-implement (given your comment #13125 (comment)) or can we amend this PR? |
That comment was addressed @DaanHoogland |
ea1712b to
e60ca2e
Compare
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.49% |
| Branch coverage | 19.80% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
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 !
#########################################################