bhouse-nexthop opened a new pull request, #14646: URL: https://github.com/apache/cloudstack/pull/14646
### 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 #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) - [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 - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [x] 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`. -- 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]
