[bhouse-nexthop]

Description

revertSnapshot failed with Cannot invoke "com.cloud.storage.VolumeVO.getState()" because "volume" is null for any snapshot whose volume has been deleted. A few other snapshot APIs had the same missing null check.

Fixes #14644

Changes

• revertSnapshot (SnapshotManagerImpl): the volume is looked up with findById(), which does not return removed volumes. When it is null, the call now fails with Cannot revert snapshot <uuid>, because its volume no longer exists. Nothing else in the revert changes.
• copySnapshot without sourcezoneid (getCheckedSnapshotForCopy): the source zone came from the volume, found the same way, and NPEd for a deleted volume. It now falls back to the snapshot's own data_center_id, which is the zone the snapshot was taken in, since a volume does not change zones. The volume is now only looked up when no source zone is given. With sourcezoneid passed, the copy already worked for such snapshots; the rest of the copy does not use the volume.
• createVolume from a snapshot (allocVolume) and createTemplate from a snapshot (createPrivateTemplateRecord): these already look the volume up including removed rows, so a deleted volume is fine. They NPEd only when the volume row itself is missing (the case in listSnapshots fails with NPE in snapshot strategy canHandle when a snapshot's volume row no longer exists #14599). They now refuse with a clear error in that case. For createTemplate the check runs while creating the template record, before the job that would otherwise fail in createPrivateTemplate, so no template record is left behind.

This complements #14600, which stops listSnapshots from failing on the same kind of snapshot. Neither PR depends on the other.

Not changed here

Deploying a VM from a snapshot whose volume row is missing still fails, in VolumeOrchestrator.allocateTemplatedVolume, which reads the size from the volume. UserVmManagerImpl accepts that case when a template id is given, so the right fix there is to make the deploy work, not to refuse it. That needs a live test, so I've left it for a separate change.

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?

The revertSnapshot NPE was seen on a KVM + Ceph RBD lab while testing #14600. This fix has not yet been run there; it is covered by unit tests.

New tests:

• SnapshotManagerTest.testRevertSnapshotVolumeGone: the error is raised and the strategy's revert is never called.
• SnapshotManagerImplTest.testGetCheckedSnapshotForCopyVolumeRemoved: no volume, so the source zone comes from the snapshot.
• VolumeApiServiceImplTest.allocVolumeFromSnapshotWhoseVolumeRowIsGoneIsRefused.
• TemplateManagerImplTest.testCreatePrivateTemplateRecordSnapshotVolumeRowGone: refused, and no template is persisted.

Without the fix, the first three fail with the NPEs from the issue. The fourth fails further on, because the record path went past the missing volume.

Updated tests:

• testCreatePrivateTemplateRecordForRegionStore relied on the volume lookup returning null. It now returns a volume, so the test goes through the normal path.
• testGetCheckedSnapshotForCopyDestContainsSource passes a source zone, so it no longer stubs a volume lookup that isn't made.

SnapshotManagerTest, SnapshotManagerImplTest, TemplateManagerImplTest and VolumeApiServiceImplTest all pass (259 tests). Checkstyle is clean for server.

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

• Only a null volume takes the new paths. A present volume, removed or not, behaves as before in all four APIs.
• For copySnapshot I checked that the copy path after the zone lookup does not use the volume. canCopyOnPrimary already skips a missing volume, and with storage replication addStoragePoolsForCopyToPrimary already refuses one with Could not find volume of a snapshot.

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.32%. Comparing base (d02adcd) to head (dbb2c6e).

Files with missing lines	Patch %	Lines
...n/java/com/cloud/storage/VolumeApiServiceImpl.java	50.00%	0 Missing and 1 partial ⚠️
...n/java/com/cloud/template/TemplateManagerImpl.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.22   #14646   +/-   ##
=========================================
  Coverage     18.32%   18.32%           
  Complexity    16644    16644           
=========================================
  Files          5939     5939           
  Lines        537768   537774    +6     
  Branches      65929    65933    +4     
=========================================
+ Hits          98551    98562   +11     
+ Misses       428008   427990   -18     
- Partials      11209    11222   +13     

Flag	Coverage Δ
uitests	4.08% <ø> (ø)
unittests	19.41% <77.77%> (+<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.

[bhouse-nexthop]

Test report: snapshot operations when the snapshot's volume is gone

Summary: for a snapshot whose volume was deleted, revertSnapshot is refused with a clear error, and createVolume and createTemplate from it work. When the volume row itself is missing, as in #14599, all three are refused with clear errors and nothing is left behind. No NPE was logged on any of these paths.

Environment

• A CloudStack 24.0 (main) build with this PR, alongside some of my other open PRs, including Don't fail snapshot strategy selection when a snapshot's volume row is gone #14600. Three KVM hosts, Ceph RBD primary storage, one NFS secondary store, snapshot.backup.to.secondary=true.
• The PR targets 4.22; it wasn't run on a 4.22 build. 4.22's SnapshotManagerImpl.revertSnapshot also calls getState() on the result of _volsDao.findById() without a null check.
• A 1 GB data volume with one snapshot (BackedUp).

Case 1: the volume was deleted (deleteVolume; the row stays, Expunged, with removed set)

• listSnapshots shows the snapshot with revertable=false.

Call	Result
revertSnapshot	refused, 431: Cannot revert snapshot <uuid>, because its volume no longer exists
createVolume snapshotid=<snapshot>	OK: a 1 GiB volume, Ready (then deleted)
createTemplate snapshotid=<snapshot>	OK: template Ready (then deleted)

Before this PR, revertSnapshot here failed with Cannot invoke "com.cloud.storage.VolumeVO.getState()" because "volume" is null. I saw that while testing #14600.

Case 2: the volume row is missing (as in #14599)

• To get there, I pointed a second snapshot's volume_id at an id with no row, and put it back afterwards.

Call	Result
revertSnapshot	refused, 431: Cannot revert snapshot <uuid>, because its volume no longer exists
createVolume snapshotid=<snapshot>	refused, 431: Cannot create a volume from snapshot <uuid>, because the volume it was taken from no longer exists
createTemplate snapshotid=<snapshot>	refused, 431: Cannot create a Template from Snapshot <uuid>, because the volume it was taken from no longer exists

After the refusals there was no template record and no volume left with the requested names. The template refusal happens before a template record is written, as the PR says.

• None of these calls logged a NullPointerException.
• Both snapshots, the created volume and the created template were deleted afterwards.

Result: pass.

Side note, not from this PR: taking a snapshot of a detached volume on the zone-wide RBD pool logged a caught NPE at DEBUG: post process snapshot failed java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because "clusterId" is null at SnapshotManagerImpl.postCreateSnapshot. That is the kvmIncrementalSnapshot.valueIn(clusterId) / endLastChainIfNeeded(clusterId, ...) branch. The snapshot itself was BackedUp. This code is unchanged by the PR.

Not covered

• copySnapshot without sourcezoneid: the lab has one zone and one secondary store.
• Case 2 used a DB change to make the volume row missing. The production case came from a row that was really deleted.