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 1 commit into
base: 4.22
Choose a base branch
from
+18 −8
Open

Fix rollback disk snapshots on instance snapshot failure #12949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 14 additions & 6 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 @@ -19,6 +19,7 @@
package org.apache.cloudstack.storage.vmsnapshot;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -111,7 +112,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
FreezeThawVMAnswer freezeAnswer = null;
FreezeThawVMCommand thawCmd = null;
FreezeThawVMAnswer thawAnswer = null;
List<SnapshotInfo> forRollback = new ArrayList<>();
Map<Long, SnapshotInfo> volumeToSnapshotInfoMapForRollback = new HashMap<>();
long startFreeze = 0;
try {
vmSnapshotHelper.vmSnapshotStateTransitTo(vmSnapshotVO, VMSnapshot.Event.CreateRequested);
Expand Down Expand Up @@ -165,7 +166,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, volumeToSnapshotInfoMapForRollback, vol);

if (snapInfo == null) {
thawAnswer = (FreezeThawVMAnswer) agentMgr.send(hostId, thawCmd);
Expand Down Expand Up @@ -222,7 +223,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
}
}
if (!result) {
for (SnapshotInfo snapshotInfo : forRollback) {
for (SnapshotInfo snapshotInfo : volumeToSnapshotInfoMapForRollback.values()) {
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Apr 2, 2026

Choose a reason for hiding this comment

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

this seems the only usage, why not a HashSet?

rollbackDiskSnapshot(snapshotInfo);
}
try {
Expand Down Expand Up @@ -388,10 +389,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 +441,7 @@ protected void revertDiskSnapshot(VMSnapshot vmSnapshot) {
}
}

protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotInfo> forRollback, VolumeInfo vol) {
protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, Map<Long, SnapshotInfo> volumeToSnapshotInfoMapForRollback, 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,6 +455,7 @@ 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());
volumeToSnapshotInfoMapForRollback.put(vol.getId(), snapshotInfo);
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Apr 2, 2026

Choose a reason for hiding this comment

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

so it might be added twice right? so why the else at line 466?

SnapshotStrategy snapshotStrategy = storageStrategyFactory.getSnapshotStrategy(snapshotInfo, SnapshotOperation.TAKE);
if (snapshotStrategy == null) {
throw new CloudRuntimeException("Could not find strategy for snapshot uuid:" + snapshotInfo.getUuid());
Expand All @@ -456,7 +464,7 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotIn
if (snapshotInfo == null) {
throw new CloudRuntimeException("Failed to create snapshot");
} else {
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
} else {
}
if (snapshotInfo != volumeToSnapshotInfoMapForRollback.get(vol.getId())) {

Copilot uses AI. Check for mistakes.
forRollback.add(snapshotInfo);
volumeToSnapshotInfoMapForRollback.put(vol.getId(), snapshotInfo);
}
vmSnapshotDetailsDao.persist(new VMSnapshotDetailsVO(vmSnapshot.getId(), STORAGE_SNAPSHOT, String.valueOf(snapshot.getId()), true));
snapshotInfo.markBackedUp();
Expand Down
6 changes: 4 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 @@ -24,7 +24,9 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;

import javax.inject.Inject;
Expand Down Expand Up @@ -153,7 +155,7 @@ public void setUp() throws Exception {
@Test
public void testCreateDiskSnapshotBasedOnStrategy() throws Exception {
VMSnapshotVO vmSnapshot = Mockito.mock(VMSnapshotVO.class);
List<SnapshotInfo> forRollback = new ArrayList<>();
Map<Long, SnapshotInfo> volumeToSnapshotInfoMapForRollback = new HashMap<>();
VolumeInfo vol = Mockito.mock(VolumeInfo.class);
SnapshotInfo snapshotInfo = Mockito.mock(SnapshotInfo.class);
SnapshotStrategy strategy = Mockito.mock(SnapshotStrategy.class);
Expand All @@ -177,7 +179,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, volumeToSnapshotInfoMapForRollback, vol);
assertNotNull(info);
}

Expand Down
Loading