Copilot commented on code in PR #12347:
URL: https://github.com/apache/cloudstack/pull/12347#discussion_r2651106015
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -2055,6 +2008,63 @@ public void cleanupStorage(boolean recurring) {
}
}
+ private void cleanupSnapshotsFromStoreRefInDestroyingState() {
+ List<SnapshotDataStoreVO> storeRefSnapshotsInDestroyingState =
_snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
+ for (SnapshotDataStoreVO snapshotDataStoreVO :
storeRefSnapshotsInDestroyingState) {
+ SnapshotVO snapshot =
_snapshotDao.findById(snapshotDataStoreVO.getSnapshotId());
+ if (snapshot == null) {
+ logger.warn("Did not find snapshot [ID: {}] for which store
reference is in destroying state; therefore, it cannot be destroyed.",
snapshotDataStoreVO.getSnapshotId());
+ continue;
+ }
+ deleteSnapshot(snapshot, snapshotDataStoreVO);
+ }
+ }
Review Comment:
The new method `cleanupSnapshotsFromStoreRefInDestroyingState()` lacks test
coverage. Consider adding unit tests to verify that:
1. It correctly retrieves snapshots in Destroying state
2. It handles cases where snapshot VO is null
3. It properly calls deleteSnapshot for valid snapshots
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java:
##########
@@ -101,6 +101,9 @@ public ObjectInDataStoreManagerImpl() {
stateMachines.addTransition(State.Destroying, Event.DestroyRequested,
State.Destroying);
stateMachines.addTransition(State.Destroying,
Event.OperationSuccessed, State.Destroyed);
stateMachines.addTransition(State.Destroying, Event.OperationFailed,
State.Destroying);
+ stateMachines.addTransition(State.Destroyed, Event.DestroyRequested,
State.Destroyed);
+ stateMachines.addTransition(State.Destroyed, Event.OperationSuccessed,
State.Destroyed);
+ stateMachines.addTransition(State.Destroyed, Event.OperationFailed,
State.Destroyed);
Review Comment:
The new state transitions for Destroyed state lack test coverage. Consider
adding tests to verify that:
1. DestroyRequested event on Destroyed state keeps state as Destroyed
2. OperationSuccessed event on Destroyed state keeps state as Destroyed
3. OperationFailed event on Destroyed state keeps state as Destroyed
These transitions are important for the cleanup mechanism to work correctly
with already-destroyed snapshots.
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java:
##########
@@ -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);
+ }
Review Comment:
The new method `getSnapshotIncludingRemoved()` lacks test coverage. Consider
adding tests to verify that:
1. It correctly retrieves snapshots that have been marked as removed
2. It returns null when snapshot is not found
3. It properly delegates to getSnapshotOnStore helper method
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -2055,6 +2008,63 @@ public void cleanupStorage(boolean recurring) {
}
}
+ private void cleanupSnapshotsFromStoreRefInDestroyingState() {
+ List<SnapshotDataStoreVO> storeRefSnapshotsInDestroyingState =
_snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
+ for (SnapshotDataStoreVO snapshotDataStoreVO :
storeRefSnapshotsInDestroyingState) {
+ SnapshotVO snapshot =
_snapshotDao.findById(snapshotDataStoreVO.getSnapshotId());
+ if (snapshot == null) {
+ logger.warn("Did not find snapshot [ID: {}] for which store
reference is in destroying state; therefore, it cannot be destroyed.",
snapshotDataStoreVO.getSnapshotId());
+ continue;
+ }
+ deleteSnapshot(snapshot, snapshotDataStoreVO);
+ }
+ }
+
+ private void deleteSnapshot(SnapshotVO snapshot, SnapshotDataStoreVO
snapshotDataStoreVO) {
+ if (snapshot == null || snapshotDataStoreVO == null) {
+ return;
+ }
+
+ try {
+ final String snapshotUuid = snapshot.getUuid();
+ final String storeRole =
snapshotDataStoreVO.getRole().toString().toLowerCase();
+ if (logger.isDebugEnabled()) {
+ logger.debug("Snapshot [{}] is in {} state on {} data store
ID: {}.", snapshotUuid, snapshotDataStoreVO.getState(), storeRole,
snapshotDataStoreVO.getDataStoreId());
+ }
+ SnapshotInfo snapshotInfo =
snapshotFactory.getSnapshotIncludingRemoved(snapshotDataStoreVO.getSnapshotId(),
snapshotDataStoreVO.getDataStoreId(), snapshotDataStoreVO.getRole());
+ if (snapshotInfo != null) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Snapshot [{}] in {} state found on {} data
store [{}], it will be deleted.", snapshotUuid, snapshotDataStoreVO.getState(),
storeRole, snapshotInfo.getDataStore().getUuid());
+ }
+ _snapshotService.deleteSnapshot(snapshotInfo);
+ } else if (logger.isDebugEnabled()) {
+ logger.debug("Did not find snapshot [{}] in {} state on {}
data store ID: {}.", snapshotUuid, snapshotDataStoreVO.getState(), storeRole,
snapshotDataStoreVO.getDataStoreId());
+ }
+ } catch (Exception e) {
+ logger.error("Failed to delete snapshot [{}] from storage due to:
[{}].", snapshot, e.getMessage());
+ if (logger.isDebugEnabled()) {
+ logger.debug("Failed to delete snapshot [{}] from storage.",
snapshot, e);
+ }
+ }
+ }
+
+ private void removeSnapshotsInErrorStatus() {
+ List<SnapshotVO> snapshotsInErrorStatus =
_snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error);
+ for (SnapshotVO snapshotVO : snapshotsInErrorStatus) {
+ try {
+ List<SnapshotDataStoreVO> storeRefSnapshotsInErrorStatus =
_snapshotStoreDao.findBySnapshotId(snapshotVO.getId());
+ for (SnapshotDataStoreVO snapshotDataStoreVO :
storeRefSnapshotsInErrorStatus) {
+ deleteSnapshot(snapshotVO, snapshotDataStoreVO);
+ _snapshotStoreDao.expunge(snapshotDataStoreVO.getId());
+ }
+ _snapshotDao.expunge(snapshotVO.getId());
+ } catch (Exception e) {
+ logger.error("Unable to destroy snapshot [{}] due to: [{}].",
snapshotVO, e.getMessage());
+ logger.debug("Unable to destroy snapshot [{}].", snapshotVO,
e);
+ }
+ }
+ }
Review Comment:
The new method `removeSnapshotsInErrorStatus()` lacks test coverage.
Consider adding unit tests to verify that:
1. It correctly retrieves snapshots in Error state (including removed ones)
2. It properly iterates through store references and calls deleteSnapshot
3. It handles expunge operations correctly
4. Exception handling works as expected
##########
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java:
##########
@@ -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);
+ }
Review Comment:
The new method `listAllByStatusIncludingRemoved()` lacks test coverage.
Consider adding tests to verify that:
1. It correctly retrieves snapshots with specified statuses including
removed ones
2. It works with multiple status values
3. It properly uses listIncludingRemovedBy instead of listBy
--
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]
