Skip to content

Fix rollback disk snapshots on instance snapshot failure #12949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sureshanaparti wants to merge 2 commits into
base: 4.22
Choose a base branch
from
+14 −9
Open

Fix rollback disk snapshots on instance snapshot failure #12949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd1ca29
Fix rollback disk snapshots on instance snapshot failure
sureshanaparti Apr 2, 2026
db0ea04
use list, and add snapshot info prior to take snapshot
sureshanaparti Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions ...hot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/StorageVMSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
FreezeThawVMAnswer freezeAnswer = null;
FreezeThawVMCommand thawCmd = null;
FreezeThawVMAnswer thawAnswer = null;
List<SnapshotInfo> forRollback = new ArrayList<>();
List<SnapshotInfo> snapshotInfoListForRollback = new ArrayList<>();
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapshotInfoListForRollback is very verbose and repeats the generic type (SnapshotInfo) already conveyed by List<SnapshotInfo>. Consider renaming to something shorter and intention-revealing like rollbackSnapshots / snapshotsForRollback to improve readability (optional).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapshotsForRollback seems appropriate, @sureshanaparti ? (or close/ignore)

long startFreeze = 0;
try {
vmSnapshotHelper.vmSnapshotStateTransitTo(vmSnapshotVO, VMSnapshot.Event.CreateRequested);
Expand Down Expand Up @@ -165,7 +165,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
logger.info("The virtual machine is frozen");
for (VolumeInfo vol : vinfos) {
long startSnapshtot = System.nanoTime();
SnapshotInfo snapInfo = createDiskSnapshot(vmSnapshot, forRollback, vol);
SnapshotInfo snapInfo = createDiskSnapshot(vmSnapshot, snapshotInfoListForRollback, vol);

if (snapInfo == null) {
thawAnswer = (FreezeThawVMAnswer) agentMgr.send(hostId, thawCmd);
Expand Down Expand Up @@ -222,7 +222,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
}
}
if (!result) {
for (SnapshotInfo snapshotInfo : forRollback) {
for (SnapshotInfo snapshotInfo : snapshotInfoListForRollback) {
rollbackDiskSnapshot(snapshotInfo);
}
try {
Expand Down Expand Up @@ -388,10 +388,16 @@ private long elapsedTime(long startTime) {

//Rollback if one of disks snapshot fails
protected void rollbackDiskSnapshot(SnapshotInfo snapshotInfo) {
if (snapshotInfo == null) {
return;
}
Long snapshotID = snapshotInfo.getId();
SnapshotVO snapshot = snapshotDao.findById(snapshotID);
if (snapshot == null) {
return;
}
deleteSnapshotByStrategy(snapshot);
logger.debug("Rollback is executed: deleting snapshot with id:" + snapshotID);
logger.debug("Rollback is executed: deleting snapshot with id: {}", snapshotID);
}

protected void deleteSnapshotByStrategy(SnapshotVO snapshot) {
Expand Down Expand Up @@ -434,7 +440,7 @@ protected void revertDiskSnapshot(VMSnapshot vmSnapshot) {
}
}

protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotInfo> forRollback, VolumeInfo vol) {
protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotInfo> snapshotInfoListForRollback, VolumeInfo vol) {
String snapshotName = vmSnapshot.getId() + "_" + vol.getUuid();
SnapshotVO snapshot = new SnapshotVO(vol.getDataCenterId(), vol.getAccountId(), vol.getDomainId(), vol.getId(), vol.getDiskOfferingId(),
snapshotName, (short) Snapshot.Type.GROUP.ordinal(), Snapshot.Type.GROUP.name(), vol.getSize(), vol.getMinIops(), vol.getMaxIops(), Hypervisor.HypervisorType.KVM, null);
Expand All @@ -448,15 +454,14 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotIn
vol.addPayload(setPayload(vol, snapshot, quiescevm));
SnapshotInfo snapshotInfo = snapshotDataFactory.getSnapshot(snapshot.getId(), vol.getDataStore());
snapshotInfo.addPayload(vol.getpayload());
snapshotInfoListForRollback.add(snapshotInfo);
SnapshotStrategy snapshotStrategy = storageStrategyFactory.getSnapshotStrategy(snapshotInfo, SnapshotOperation.TAKE);
Comment on lines 454 to 458
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change alters rollback semantics by adding the snapshot to the rollback list before strategy lookup and takeSnapshot(). Please add/extend a unit test to assert that when snapshot creation fails (e.g., getSnapshotStrategy() returns null or takeSnapshot() throws/returns null), the created snapshot is still included in the rollback list and rollback cleanup is triggered as expected.

Copilot uses AI. Check for mistakes.
if (snapshotStrategy == null) {
throw new CloudRuntimeException("Could not find strategy for snapshot uuid:" + snapshotInfo.getUuid());
}
snapshotInfo = snapshotStrategy.takeSnapshot(snapshotInfo);
if (snapshotInfo == null) {
throw new CloudRuntimeException("Failed to create snapshot");
} else {
forRollback.add(snapshotInfo);
}
vmSnapshotDetailsDao.persist(new VMSnapshotDetailsVO(vmSnapshot.getId(), STORAGE_SNAPSHOT, String.valueOf(snapshot.getId()), true));
snapshotInfo.markBackedUp();
Expand Down
4 changes: 2 additions & 2 deletions ...hot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ public void setUp() throws Exception {
@Test
public void testCreateDiskSnapshotBasedOnStrategy() throws Exception {
VMSnapshotVO vmSnapshot = Mockito.mock(VMSnapshotVO.class);
List<SnapshotInfo> forRollback = new ArrayList<>();
List<SnapshotInfo> snapshotInfoListForRollback = new ArrayList<>();
VolumeInfo vol = Mockito.mock(VolumeInfo.class);
SnapshotInfo snapshotInfo = Mockito.mock(SnapshotInfo.class);
SnapshotStrategy strategy = Mockito.mock(SnapshotStrategy.class);
Expand All @@ -177,7 +177,7 @@ public void testCreateDiskSnapshotBasedOnStrategy() throws Exception {
VMSnapshotDetailsVO vmDetails = new VMSnapshotDetailsVO(vmSnapshot.getId(), volUuid, String.valueOf(snapshot.getId()), false);
when(vmSnapshotDetailsDao.persist(any())).thenReturn(vmDetails);

info = vmStrategy.createDiskSnapshot(vmSnapshot, forRollback, vol);
info = vmStrategy.createDiskSnapshot(vmSnapshot, snapshotInfoListForRollback, vol);
assertNotNull(info);
}

Expand Down
Loading