[bhouse-nexthop]

Description

listSnapshots failed with Cannot invoke "com.cloud.storage.VolumeVO.getRemoved()" because "volumeVO" is null when any listed snapshot references a volume whose row no longer exists in the volumes table.

Fixes #14599

Why it breaks

• Choosing a snapshot strategy asks every strategy for canHandle() (StorageStrategyFactoryImpl.bestMatch).
• CephSnapshotStrategy, ScaleIOSnapshotStrategy and StorageSystemSnapshotStrategy look up the snapshot's volume with findByIdIncludingRemoved() and use the result without a null check. That still finds expunged volumes, but returns null once the row itself is gone.
• listSnapshots sets each snapshot's revertable flag through SnapshotObject.isRevertable() → getSnapshotStrategy(snapshot, REVERT), so a single such snapshot fails the whole list, whatever storage it is on. Other operations that pick a strategy for that snapshot fail the same way.

How it is fixed

A missing volume row is treated the same as a removed volume.

• Ceph and ScaleIO: baseVolumeExists is false when the row is missing, so they return CANT_HANDLE, as they already did for a removed volume.
• StorageSystem: REVERT returns CANT_HANDLE for a missing row, as it already did for a removed volume.
• StorageSystem's pool id: this strategy also unboxed the volume's pool id (pool_id, else last_pool_id) before its REVERT branch checked whether the volume was removed. A removed volume can have neither, for example a local-storage volume whose host was deleted (VolumeDaoImpl.updateAndRemoveVolume clears pool_id), and that failed listSnapshots the same way.
  • The pool id is now only read where it's used.
  • The REVERT checks skip the volume-pool fallback when there's no pool id, and still use the snapshot's own primary store.
• TAKE and BACKUP in StorageSystem are unchanged. They need the volume's pool, and a missing volume there still fails rather than being routed to another strategy.
• DefaultSnapshotStrategy already handles a null volume for REVERT, so listSnapshots now reports such a snapshot as not revertable.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

How Has This Been Tested?

• New unit tests for a missing volume row: CephSnapshotStrategyTest.canHandleTestVolumeRowMissing, and new ScaleIOSnapshotStrategyTest (missing row, removed volume, and a live PowerFlex volume still getting HIGHEST for REVERT/DELETE) and StorageSystemSnapshotStrategyTest (REVERT with a missing row, a removed volume, a live volume without a pool id on both the backend-snapshot and the managed-pool paths, a backend snapshot on a revert-capable pool still getting HIGHEST when the volume has no pool id, a volume on a managed last_pool_id still getting HIGHEST, and TAKE/BACKUP still using last_pool_id). They fail with the NPE on the current 4.22 code and pass with this change.
• The two existing CephSnapshotStrategyTest.canHandleTestReomved* tests passed null as the removed date, so they never tested a removed volume. They now pass a date and expect CANT_HANDLE.
• All tests in engine/storage/snapshot pass, and checkstyle is clean for the module.
• The error was seen on a 4.22 KVM + Ceph RBD deployment from listSnapshots listall=true. The fixed build has not yet been run against that deployment; the call path above was traced in the code.

How did you try to break this feature and the system with this change?

• Existing volumes, removed or not: behave as before. Only a null lookup result takes the new path.
• Other strategies: DefaultSnapshotStrategy already handles a null volume for REVERT. StorPoolSnapshotStrategy doesn't look up the volume.
• DELETE routing: for a snapshot whose volume row is gone, Ceph and ScaleIO now decline DELETE, exactly as they already do when the volume is only marked removed, so the delete falls through to the next strategy instead of failing with the NPE.
• Not covered here:
  • DefaultSnapshotStrategy (TAKE) still dereferences the volume, but TAKE only runs for a snapshot of a volume that exists.
  • Other code that reads a snapshot's source volume (e.g. createVolume / createTemplate from such a snapshot) can still fail on a missing volume row. Those failed before this change as well; this PR only makes the snapshot listable and lets strategy selection decline it.
  • StorageSystemSnapshotStrategy still dereferences storagePoolDao.findById() without a null check for a snapshot or volume whose storage pool row is gone. That's a different missing row and is left for a separate change.

[codecov]

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.33%. Comparing base (d02adcd) to head (8c452ca).

Files with missing lines	Patch %	Lines
...torage/snapshot/StorageSystemSnapshotStrategy.java	72.72%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14600      +/-   ##
============================================
+ Coverage     18.32%   18.33%   +0.01%     
- Complexity    16644    16664      +20     
============================================
  Files          5939     5939              
  Lines        537768   537773       +5     
  Branches      65929    65930       +1     
============================================
+ Hits          98551    98609      +58     
+ Misses       428008   427945      -63     
- Partials      11209    11219      +10     

Flag	Coverage Δ
uitests	4.08% <ø> (ø)
unittests	19.42% <76.92%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.