bhouse-nexthop opened a new pull request, #14600:
URL: https://github.com/apache/cloudstack/pull/14600
### 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)
- [x] 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
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
