Skip to content

Commit f54062d

Browse files
JoaoJandreJoaoJandre
committed
Avoid data loss
1 parent 574ed78 commit f54062d

File tree

6 files changed

+70
-5
lines changed

6 files changed

+70
-5
lines changed

‎engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,12 @@ private StrategyPriority validateVmSnapshot(Snapshot snapshot) {
672672
}
673673
}
674674

675+
if (CollectionUtils.isNotEmpty(vmSnapshotDao.findByVmAndByType(volumeVO.getInstanceId(), VMSnapshot.Type.DiskAndMemory))) {
676+
logger.debug("DefaultSnapshotStrategy cannot handle snapshot [{}] for volume [{}] as the volume is attached to a VM with disk-and-memory VM snapshots." +
677+
"Restoring the volume snapshot will corrupt any newer disk-and-memory VM snapshots.", snapshot);
678+
return StrategyPriority.CANT_HANDLE;
679+
}
680+
675681
return StrategyPriority.DEFAULT;
676682
}
677683

‎engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,19 @@
2626
import javax.naming.ConfigurationException;
2727

2828
import com.cloud.hypervisor.Hypervisor;
29+
import com.cloud.storage.Snapshot;
30+
import com.cloud.storage.dao.SnapshotDao;
2931
import com.cloud.vm.snapshot.VMSnapshotDetailsVO;
3032
import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao;
33+
import org.apache.cloudstack.backup.BackupOfferingVO;
34+
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
3135
import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
3236
import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotOptions;
3337
import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy;
3438
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
3539
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
3640
import org.apache.cloudstack.storage.to.VolumeObjectTO;
41+
import org.apache.commons.collections4.CollectionUtils;
3742
import org.apache.commons.lang3.StringUtils;
3843

3944
import com.cloud.agent.AgentManager;
@@ -106,6 +111,12 @@ public class DefaultVMSnapshotStrategy extends ManagerBase implements VMSnapshot
106111
@Inject
107112
VMSnapshotDetailsDao vmSnapshotDetailsDao;
108113

114+
@Inject
115+
private BackupOfferingDao backupOfferingDao;
116+
117+
@Inject
118+
private SnapshotDao snapshotDao;
119+
109120
protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT = "kvmFileBasedStorageSnapshot";
110121

111122
protected static final String STORAGE_SNAPSHOT = "kvmStorageSnapshot";
@@ -479,24 +490,38 @@ public boolean deleteVMSnapshotFromDB(VMSnapshot vmSnapshot, boolean unmanage) {
479490
@Override
480491
public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMemory) {
481492
UserVmVO vm = userVmDao.findById(vmId);
493+
String cantHandleLog = String.format("Default VM snapshot cannot handle VM snapshot for [%s]", vm);
482494
if (State.Running.equals(vm.getState()) && !snapshotMemory) {
483-
logger.debug("Default VM snapshot strategy cannot handle VM snapshot for [{}] as it is running and its memory will not be affected.", vm);
495+
logger.debug("{} as it is running and its memory will not be affected.", cantHandleLog, vm);
484496
return StrategyPriority.CANT_HANDLE;
485497
}
486498

487499
if (vmHasKvmDiskOnlySnapshot(vm)) {
488-
logger.debug("Default VM snapshot strategy cannot handle VM snapshot for [{}] as it has a disk-only VM snapshot using kvmFileBasedStorageSnapshot strategy." +
489-
"These two strategies are not compatible, as reverting a disk-only VM snapshot will erase newer disk-and-memory VM snapshots.", vm);
500+
logger.debug("{} as it is not compatible with disk-only VM snapshot on KVM. As disk-and-memory snapshots use internal snapshots and disk-only VM snapshots use" +
501+
" external snapshots. When restoring external snapshots, any newer internal snapshots are lost.", cantHandleLog);
490502
return StrategyPriority.CANT_HANDLE;
491503
}
492504

493505
List<VolumeVO> volumes = volumeDao.findByInstance(vmId);
494506
for (VolumeVO volume : volumes) {
495507
if (volume.getFormat() != ImageFormat.QCOW2) {
496-
logger.debug("Default VM snapshot strategy cannot handle VM snapshot for [{}] as it has a volume [{}] that is not in the QCOW2 format.", vm, volume);
508+
logger.debug("{} as it has a volume [{}] that is not in the QCOW2 format.", cantHandleLog, vm, volume);
509+
return StrategyPriority.CANT_HANDLE;
510+
}
511+
512+
if (CollectionUtils.isNotEmpty(snapshotDao.listByVolumeIdAndTypeNotInAndStateNotRemoved(volume.getId(), Snapshot.Type.GROUP))) {
513+
logger.debug("{} as it has a volume [{}] with volume snapshots. As disk-and-memory snapshots use internal snapshots and volume snapshots use external" +
514+
" snapshots. When restoring external snapshots, any newer internal snapshots are lost.", cantHandleLog, volume);
497515
return StrategyPriority.CANT_HANDLE;
498516
}
499517
}
518+
519+
BackupOfferingVO backupOffering = backupOfferingDao.findById(vm.getBackupOfferingId());
520+
if (backupOffering != null && "nas".equals(backupOffering.getProvider())) {
521+
logger.debug("{} as the VM has a backup offering for a provider that is not supported.", cantHandleLog);
522+
return StrategyPriority.CANT_HANDLE;
523+
}
524+
500525
return StrategyPriority.DEFAULT;
501526
}
502527

@@ -507,7 +532,7 @@ private boolean vmHasKvmDiskOnlySnapshot(UserVm vm) {
507532

508533
for (VMSnapshotVO vmSnapshotVO : vmSnapshotDao.findByVmAndByType(vm.getId(), VMSnapshot.Type.Disk)) {
509534
List<VMSnapshotDetailsVO> vmSnapshotDetails = vmSnapshotDetailsDao.listDetails(vmSnapshotVO.getId());
510-
if (vmSnapshotDetails.stream().anyMatch(vmSnapshotDetailsVO -> vmSnapshotDetailsVO.getName().equals(KVM_FILE_BASED_STORAGE_SNAPSHOT))) {
535+
if (vmSnapshotDetails.stream().anyMatch(detailsVO -> KVM_FILE_BASED_STORAGE_SNAPSHOT.equals(detailsVO.getName()) || STORAGE_SNAPSHOT.equals(detailsVO.getName()))) {
511536
return true;
512537
}
513538
}

‎engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
import javax.inject.Inject;
3131

32+
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
3233
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
3334
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
3435
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProviderManager;
@@ -431,5 +432,10 @@ public DataStoreProviderManager manager() {
431432
public VMSnapshotDetailsDao vmSnapshotDetailsDao () {
432433
return Mockito.mock(VMSnapshotDetailsDao.class);
433434
}
435+
436+
@Bean
437+
public BackupOfferingDao backupOfferingDao() {
438+
return Mockito.mock(BackupOfferingDao.class);
439+
}
434440
}
435441
}

‎engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyTest.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525

2626
import javax.inject.Inject;
2727

28+
import com.cloud.storage.dao.SnapshotDao;
2829
import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao;
30+
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
2931
import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy;
3032
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
3133
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
@@ -318,5 +320,20 @@ public HostDao hostDao() {
318320
public PrimaryDataStoreDao primaryDataStoreDao() {
319321
return Mockito.mock(PrimaryDataStoreDao.class);
320322
}
323+
324+
@Bean
325+
public BackupOfferingDao backupOfferingDao() {
326+
return Mockito.mock(BackupOfferingDao.class);
327+
}
328+
329+
@Bean
330+
public VMSnapshotDetailsDao VMSnapshotDetailsDao() {
331+
return Mockito.mock(VMSnapshotDetailsDao.class);
332+
}
333+
334+
@Bean
335+
public SnapshotDao snapshotDao() {
336+
return Mockito.mock(SnapshotDao.class);
337+
}
321338
}
322339
}

‎plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
import org.apache.cloudstack.framework.config.Configurable;
5454
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
5555
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
56+
import org.apache.commons.collections4.CollectionUtils;
5657
import org.apache.logging.log4j.Logger;
5758
import org.apache.logging.log4j.LogManager;
5859

@@ -165,6 +166,12 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
165166
throw new CloudRuntimeException("No valid backup repository found for the VM, please check the attached backup offering");
166167
}
167168

169+
if (CollectionUtils.isNotEmpty(vmSnapshotDao.findByVmAndByType(vm.getId(), VMSnapshot.Type.DiskAndMemory))) {
170+
logger.debug("NAS backup provider cannot take backups of a VM [{}] with disk-and-memory VM snapshots. Restoring the backup will corrupt any newer disk-and-memory " +
171+
"VM snapshots.", vm);
172+
throw new CloudRuntimeException(String.format("Cannot take backup of VM [%s] as it has disk-and-memory VM snapshots.", vm.getUuid()));
173+
}
174+
168175
final Date creationDate = new Date();
169176
final String backupPath = String.format("%s/%s", vm.getInstanceName(),
170177
new SimpleDateFormat("yyyy.MM.dd.HH.mm.ss").format(creationDate));

‎plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.List;
2424
import java.util.Optional;
2525

26+
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
2627
import org.apache.cloudstack.backup.dao.BackupDao;
2728
import org.apache.cloudstack.backup.dao.BackupRepositoryDao;
2829
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
@@ -84,6 +85,9 @@ public class NASBackupProviderTest {
8485
@Mock
8586
private ResourceManager resourceManager;
8687

88+
@Mock
89+
private VMSnapshotDao vmSnapshotDaoMock;
90+
8791
@Test
8892
public void testDeleteBackup() throws OperationTimedoutException, AgentUnavailableException {
8993
Long hostId = 1L;

0 commit comments

Comments
 (0)