Copilot commented on code in PR #12949:
URL: https://github.com/apache/cloudstack/pull/12949#discussion_r3026719842
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/StorageVMSnapshotStrategy.java:
##########
@@ -456,7 +464,7 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot
vmSnapshot, List<SnapshotIn
if (snapshotInfo == null) {
throw new CloudRuntimeException("Failed to create snapshot");
} else {
Review Comment:
`volumeToSnapshotInfoMapForRollback.put(vol.getId(), snapshotInfo)` is
performed twice (before and after `takeSnapshot`). If the intent is to ensure
rollback coverage when `takeSnapshot` throws, keep the pre-`takeSnapshot` put,
but drop the `else` block and only overwrite the map entry if `takeSnapshot`
returns a different `SnapshotInfo` instance. This removes redundant writes and
makes the intent clearer.
```suggestion
}
if (snapshotInfo !=
volumeToSnapshotInfoMapForRollback.get(vol.getId())) {
```
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/StorageVMSnapshotStrategy.java:
##########
@@ -456,7 +464,7 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot
vmSnapshot, List<SnapshotIn
if (snapshotInfo == null) {
throw new CloudRuntimeException("Failed to create snapshot");
} else {
Review Comment:
The new rollback behavior relies on adding the snapshot to the rollback map
before `snapshotStrategy.takeSnapshot(...)` runs so that failures/exceptions
still get cleaned up. There’s no unit test covering the failure path (e.g.,
`takeSnapshot` throws/returns null) and verifying that rollback deletes the
persisted snapshot record. Please add a test for this scenario to prevent
regressions of #12927.
--
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]
