Skip to content

Cleanup snapshot files in datastores for Error-ed snapshots, and some code improvements #12347

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 3 commits into
base: 4.20
Choose a base branch
from
+122 −87
Open

Cleanup snapshot files in datastores for Error-ed snapshots, and some code improvements #12347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78c44ef
Cleanup snapshots in datastores for Error-ed snapshots, and some code…
sureshanaparti Dec 29, 2025
ecffe53
review comments
sureshanaparti Dec 30, 2025
2305550
event fix
sureshanaparti Jan 2, 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
2 changes: 2 additions & 0 deletions ...src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotDataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public interface SnapshotDataFactory {

SnapshotInfo getSnapshot(long snapshotId, long storeId, DataStoreRole role);

SnapshotInfo getSnapshotIncludingRemoved(long snapshotId, long storeId, DataStoreRole role);

SnapshotInfo getSnapshotWithRoleAndZone(long snapshotId, DataStoreRole role, long zoneId);

SnapshotInfo getSnapshotOnPrimaryStore(long snapshotId);
Expand Down
2 changes: 2 additions & 0 deletions engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap

List<SnapshotVO> listAllByStatus(Snapshot.State... status);

List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status);

void updateVolumeIds(long oldVolId, long newVolId);

List<SnapshotVO> listByStatusNotIn(long volumeId, Snapshot.State... status);
Expand Down
7 changes: 7 additions & 0 deletions engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@ public List<SnapshotVO> listAllByStatus(Snapshot.State... status) {
return listBy(sc, null);
}

@Override
public List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status) {
SearchCriteria<SnapshotVO> sc = StatusSearch.create();
sc.setParameters("status", (Object[])status);
return listIncludingRemovedBy(sc, null);
}

@Override
public List<SnapshotVO> listByIds(Object... ids) {
SearchCriteria<SnapshotVO> sc = snapshotIdsSearch.create();
Expand Down
2 changes: 2 additions & 0 deletions ...schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ public interface SnapshotDataStoreDao extends GenericDao<SnapshotDataStoreVO, Lo

List<SnapshotDataStoreVO> findBySnapshotId(long snapshotId);

List<SnapshotDataStoreVO> findBySnapshotIdWithNonDestroyedState(long snapshotId);

void duplicateCacheRecordsOnRegionStore(long storeId);

// delete the snapshot entry on primary data store to make sure that next snapshot will be full snapshot
Expand Down
7 changes: 7 additions & 0 deletions ...ma/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,13 @@ public List<SnapshotDataStoreVO> findByVolume(long snapshotId, long volumeId, Da

@Override
public List<SnapshotDataStoreVO> findBySnapshotId(long snapshotId) {
SearchCriteria<SnapshotDataStoreVO> sc = searchFilteringStoreIdEqStateEqStoreRoleEqIdEqUpdateCountEqSnapshotIdEqVolumeIdEq.create();
sc.setParameters(SNAPSHOT_ID, snapshotId);
return listBy(sc);
}

@Override
public List<SnapshotDataStoreVO> findBySnapshotIdWithNonDestroyedState(long snapshotId) {
SearchCriteria<SnapshotDataStoreVO> sc = idStateNeqSearch.create();
sc.setParameters(SNAPSHOT_ID, snapshotId);
sc.setParameters(STATE, State.Destroyed);
Expand Down
2 changes: 1 addition & 1 deletion ...orage/integration-test/src/test/java/org/apache/cloudstack/storage/test/SnapshotTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ public void setUp() {
to.setSize(1000L);
CopyCmdAnswer answer = new CopyCmdAnswer(to);
templateOnStore.processEvent(Event.CreateOnlyRequested);
templateOnStore.processEvent(Event.OperationSuccessed, answer);
templateOnStore.processEvent(Event.OperationSucceeded, answer);

}

Expand Down
2 changes: 1 addition & 1 deletion ...storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public void setUp() {
to.setSize(100L);
CopyCmdAnswer answer = new CopyCmdAnswer(to);
templateOnStore.processEvent(Event.CreateOnlyRequested);
templateOnStore.processEvent(Event.OperationSuccessed, answer);
templateOnStore.processEvent(Event.OperationSucceeded, answer);

}

Expand Down
2 changes: 1 addition & 1 deletion ...e/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTestVmware.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public void setUp() {
to.setPath(this.getImageInstallPath());
CopyCmdAnswer answer = new CopyCmdAnswer(to);
templateOnStore.processEvent(Event.CreateOnlyRequested);
templateOnStore.processEvent(Event.OperationSuccessed, answer);
templateOnStore.processEvent(Event.OperationSucceeded, answer);

}

Expand Down
4 changes: 2 additions & 2 deletions ...napshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) {
}

if (Snapshot.State.Error.equals(snapshotVO.getState())) {
List<SnapshotDataStoreVO> storeRefs = snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> storeRefs = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
List<Long> deletedRefs = new ArrayList<>();
for (SnapshotDataStoreVO ref : storeRefs) {
boolean refZoneIdMatch = false;
Expand Down Expand Up @@ -351,7 +351,7 @@ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo, Long zoneId) {
protected Boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, SnapshotVO snapshotVo) {
DataStore dataStore = snapshotInfo.getDataStore();
String storageToString = String.format("%s {uuid: \"%s\", name: \"%s\"}", dataStore.getRole().name(), dataStore.getUuid(), dataStore.getName());
List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotId(snapshotVo.getId());
List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotVo.getId());
boolean isLastSnapshotRef = CollectionUtils.isEmpty(snapshotStoreRefs) || snapshotStoreRefs.size() == 1;
try {
SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);
Expand Down
22 changes: 19 additions & 3 deletions ...napshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public List<SnapshotInfo> getSnapshots(long snapshotId, Long zoneId) {
if (snapshot == null) { //snapshot may have been removed;
return new ArrayList<>();
}
List<SnapshotDataStoreVO> allSnapshotsAndDataStore = snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> allSnapshotsAndDataStore = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
if (CollectionUtils.isEmpty(allSnapshotsAndDataStore)) {
return new ArrayList<>();
}
Expand All @@ -118,7 +118,23 @@ public SnapshotInfo getSnapshot(long snapshotId, long storeId, DataStoreRole rol
if (snapshot == null) {
return null;
}
SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByStoreSnapshot(role, storeId, snapshotId);
return getSnapshotOnStore(snapshot, storeId, role);
}

@Override
public SnapshotInfo getSnapshotIncludingRemoved(long snapshotId, long storeId, DataStoreRole role) {
SnapshotVO snapshot = snapshotDao.findByIdIncludingRemoved(snapshotId);
if (snapshot == null) {
return null;
}
return getSnapshotOnStore(snapshot, storeId, role);
}

private SnapshotInfo getSnapshotOnStore(SnapshotVO snapshot, long storeId, DataStoreRole role) {
if (snapshot == null) {
return null;
}
SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByStoreSnapshot(role, storeId, snapshot.getId());
if (snapshotStore == null) {
return null;
}
Expand Down Expand Up @@ -207,7 +223,7 @@ public List<SnapshotInfo> listSnapshotOnCache(long snapshotId) {

@Override
public void updateOperationFailed(long snapshotId) throws NoTransitionException {
List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
for (SnapshotDataStoreVO snapshotStoreRef : snapshotStoreRefs) {
SnapshotInfo snapshotInfo = getSnapshot(snapshotStoreRef.getSnapshotId(), snapshotStoreRef.getDataStoreId(), snapshotStoreRef.getRole());
if (snapshotInfo != null) {
Expand Down
9 changes: 2 additions & 7 deletions ...ge/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,16 +382,14 @@ public SnapshotInfo backupSnapshot(SnapshotInfo snapshot) {
if (res.isFailed()) {
throw new CloudRuntimeException(res.getResult());
}
SnapshotInfo destSnapshot = res.getSnapshot();
return destSnapshot;
return res.getSnapshot();
} catch (InterruptedException e) {
logger.debug("failed copy snapshot", e);
throw new CloudRuntimeException("Failed to copy snapshot", e);
} catch (ExecutionException e) {
logger.debug("Failed to copy snapshot", e);
throw new CloudRuntimeException("Failed to copy snapshot", e);
}

}

protected Void copySnapshotAsyncCallback(AsyncCallbackDispatcher<SnapshotServiceImpl, CopyCommandResult> callback, CopySnapshotContext<CommandResult> context) {
Expand Down Expand Up @@ -479,7 +477,6 @@ protected Void prepareCopySnapshotZoneAsyncCallback(AsyncCallbackDispatcher<Snap
}

protected Void deleteSnapshotCallback(AsyncCallbackDispatcher<SnapshotServiceImpl, CommandResult> callback, DeleteSnapshotContext<CommandResult> context) {

CommandResult result = callback.getResult();
AsyncCallFuture<SnapshotResult> future = context.future;
SnapshotInfo snapshot = context.snapshot;
Expand Down Expand Up @@ -607,7 +604,7 @@ public void cleanupVolumeDuringSnapshotFailure(Long volumeId, Long snapshotId) {

if (snapshot != null) {
if (snapshot.getState() != Snapshot.State.BackedUp) {
List<SnapshotDataStoreVO> snapshotDataStoreVOs = _snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> snapshotDataStoreVOs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotDataStoreVOs) {
logger.debug("Remove snapshot {}, status {} on snapshot_store_ref table with id: {}", snapshot, snapshotDataStoreVO.getState(), snapshotDataStoreVO.getId());

Expand Down Expand Up @@ -712,7 +709,6 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
SnapshotObject srcSnapshot = (SnapshotObject)snapshot;
srcSnapshot.processEvent(Event.DestroyRequested);
srcSnapshot.processEvent(Event.OperationSucceeded);

srcSnapshot.processEvent(Snapshot.Event.OperationFailed);

_snapshotDetailsDao.removeDetail(srcSnapshot.getId(), AsyncJob.Constants.MS_ID);
Expand All @@ -723,7 +719,6 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
}
}
});

}

@Override
Expand Down
1 change: 0 additions & 1 deletion ...t/src/main/java/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,6 @@ public void postSnapshotCreation(SnapshotInfo snapshot) {
logger.warn("Failed to clean up snapshot '" + snapshot.getId() + "' on primary storage: " + e.getMessage());
}
}

}

private VMSnapshot takeHypervisorSnapshot(VolumeInfo volumeInfo) {
Expand Down
3 changes: 3 additions & 0 deletions ...e/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ public ObjectInDataStoreManagerImpl() {
stateMachines.addTransition(State.Destroying, Event.DestroyRequested, State.Destroying);
stateMachines.addTransition(State.Destroying, Event.OperationSucceeded, State.Destroyed);
stateMachines.addTransition(State.Destroying, Event.OperationFailed, State.Destroying);
stateMachines.addTransition(State.Destroyed, Event.DestroyRequested, State.Destroyed);
stateMachines.addTransition(State.Destroyed, Event.OperationSucceeded, State.Destroyed);
stateMachines.addTransition(State.Destroyed, Event.OperationFailed, State.Destroyed);
stateMachines.addTransition(State.Failed, Event.DestroyRequested, State.Destroying);
// TODO: further investigate why an extra event is sent when it is
// already Ready for DownloadListener
Expand Down
2 changes: 1 addition & 1 deletion .../storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ protected Void managedCopyBaseImageCallback(AsyncCallbackDispatcher<VolumeServic
VolumeApiResult res = new VolumeApiResult(volumeInfo);

if (result.isSuccess()) {
// volumeInfo.processEvent(Event.OperationSuccessed, result.getAnswer());
// volumeInfo.processEvent(Event.OperationSucceeded, result.getAnswer());

VolumeVO volume = volDao.findById(volumeInfo.getId());
CopyCmdAnswer answer = (CopyCmdAnswer)result.getAnswer();
Expand Down
2 changes: 1 addition & 1 deletion ...e/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public static String getSnapshotName(Long snapshotId, String snapshotUuid, Snaps
if (snapshotDetails != null) {
return StorPoolStorageAdaptor.getVolumeNameFromPath(snapshotDetails.getValue(), true);
} else {
List<SnapshotDataStoreVO> snapshots = snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> snapshots = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
if (!CollectionUtils.isEmpty(snapshots)) {
for (SnapshotDataStoreVO snapshotDataStoreVO : snapshots) {
String name = StorPoolStorageAdaptor.getVolumeNameFromPath(snapshotDataStoreVO.getInstallPath(), true);
Expand Down
4 changes: 2 additions & 2 deletions ...orpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ private boolean deleteSnapshotChain(SnapshotInfo snapshot) {
}

protected boolean areLastSnapshotRef(long snapshotId) {
List<SnapshotDataStoreVO> snapshotStoreRefs = _snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> snapshotStoreRefs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
if (CollectionUtils.isEmpty(snapshotStoreRefs) || snapshotStoreRefs.size() == 1) {
return true;
}
Expand Down Expand Up @@ -308,7 +308,7 @@ private boolean deleteSnapshotFromDbIfNeeded(SnapshotVO snapshotVO, Long zoneId)
}

if (Snapshot.State.Error.equals(snapshotVO.getState())) {
List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotId);
List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
List<Long> deletedRefs = new ArrayList<>();
for (SnapshotDataStoreVO ref : storeRefs) {
boolean refZoneIdMatch = false;
Expand Down
Loading
Loading