From be255e4203b573a0967768be5b19e03c0100b649 Mon Sep 17 00:00:00 2001 From: dahn Date: Thu, 29 Apr 2021 17:10:29 +0200 Subject: [PATCH] server: protect against stray snapshot-details without snapshot (#4924) This PR makes sure no orphaned snapshot details are considered in the cleanup at startup job. a real solution would be to implement some kind of cascading delete, but as the parent record is "only" marked as removed this would be a bit com Co-authored-by: Daan Hoogland --- .../engine/orchestration/DataMigrationUtility.java | 4 +++- .../storage/snapshot/SnapshotDataFactoryImpl.java | 3 +++ .../cloudstack/storage/snapshot/SnapshotObject.java | 5 ++++- .../framework/jobs/impl/AsyncJobManagerImpl.java | 4 ++++ .../com/cloud/storage/snapshot/SnapshotManager.java | 3 --- .../cloud/storage/snapshot/SnapshotManagerImpl.java | 10 ---------- 6 files changed, 14 insertions(+), 15 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java index dd451f59465..7fad871462d 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java @@ -200,7 +200,9 @@ public class DataMigrationUtility { snapshotVO != null && snapshotVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator && snapshot.getParentSnapshotId() == 0 ) { SnapshotInfo snap = snapshotFactory.getSnapshot(snapshotVO.getSnapshotId(), DataStoreRole.Image); - files.add(snap); + if (snap != null) { + files.add(snap); + } } } diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java index f8be1adee56..42578850d54 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java @@ -87,6 +87,9 @@ public class SnapshotDataFactoryImpl implements SnapshotDataFactory { @Override public SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role) { SnapshotVO snapshot = snapshotDao.findById(snapshotId); + if (snapshot == null) { + return null; + } SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findBySnapshot(snapshotId, role); if (snapshotStore == null) { snapshotStore = snapshotStoreDao.findByVolume(snapshot.getVolumeId(), role); diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotObject.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotObject.java index f107343f0de..1d343ab22d2 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotObject.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotObject.java @@ -142,7 +142,10 @@ public class SnapshotObject implements SnapshotInfo { List children = new ArrayList<>(); if (vos != null) { for (SnapshotDataStoreVO vo : vos) { - children.add(snapshotFactory.getSnapshot(vo.getSnapshotId(), DataStoreRole.Image)); + SnapshotInfo info = snapshotFactory.getSnapshot(vo.getSnapshotId(), DataStoreRole.Image); + if (info != null) { + children.add(info); + } } } return children; diff --git a/framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java b/framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java index 54ce065f9cd..34f58933cd4 100644 --- a/framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java +++ b/framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java @@ -1097,6 +1097,10 @@ public class AsyncJobManagerImpl extends ManagerBase implements AsyncJobManager, final List snapshotList = _snapshotDetailsDao.findDetails(AsyncJob.Constants.MS_ID, Long.toString(msid), false); for (final SnapshotDetailsVO snapshotDetailsVO : snapshotList) { SnapshotInfo snapshot = snapshotFactory.getSnapshot(snapshotDetailsVO.getResourceId(), DataStoreRole.Primary); + if (snapshot == null) { + _snapshotDetailsDao.remove(snapshotDetailsVO.getId()); + continue; + } snapshotSrv.processEventOnSnapshotObject(snapshot, Snapshot.Event.OperationFailed); _snapshotDetailsDao.removeDetail(snapshotDetailsVO.getResourceId(), AsyncJob.Constants.MS_ID); } diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java index c900b2d14ba..012c2ab8641 100644 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java @@ -24,7 +24,6 @@ import org.apache.cloudstack.framework.config.Configurable; import com.cloud.agent.api.Answer; import com.cloud.agent.api.Command; import com.cloud.exception.ResourceAllocationException; -import com.cloud.storage.Snapshot; import com.cloud.storage.SnapshotVO; import com.cloud.storage.Volume; @@ -83,7 +82,5 @@ public interface SnapshotManager extends Configurable { SnapshotVO getParentSnapshot(VolumeInfo volume); - Snapshot backupSnapshot(Long snapshotId); - SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationException; } diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 4b95eff7b7c..4eaebd871e2 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -420,16 +420,6 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement return snapshotOnSecondary; } - @Override - public Snapshot backupSnapshot(Long snapshotId) { - SnapshotInfo snapshot = snapshotFactory.getSnapshot(snapshotId, DataStoreRole.Image); - if (snapshot != null) { - throw new CloudRuntimeException("Already in the backup snapshot:" + snapshotId); - } - - return snapshotSrv.backupSnapshot(snapshot); - } - @Override public Snapshot backupSnapshotFromVmSnapshot(Long snapshotId, Long vmId, Long volumeId, Long vmSnapshotId) { VMInstanceVO vm = _vmDao.findById(vmId);