You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/08/21 16:03:13 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
DaanHoogland commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r693370175
##########
File path: plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
##########
@@ -287,20 +288,45 @@ public void copyAsync(DataObject srcdata, DataObject destData, AsyncCompletionCa
}
CopyCommandResult result = new CopyCommandResult("", answer);
callback.complete(result);
+ } else if (srcdata.getType() == DataObjectType.SNAPSHOT && destData.getType() == DataObjectType.VOLUME) {
+ SnapshotObjectTO srcTO = (SnapshotObjectTO) srcdata.getTO();
+ CopyCommand cmd = new CopyCommand(srcTO, destData.getTO(), StorageManager.PRIMARY_STORAGE_DOWNLOAD_WAIT.value(), true);
+ EndPoint ep = epSelector.select(srcdata, destData);
+ CopyCmdAnswer answer = null;
+ if (ep == null) {
+ String errMsg = "No remote endpoint to send command, check if host or ssvm is down?";
+ s_logger.error(errMsg);
+ answer = new CopyCmdAnswer(errMsg);
+ } else {
+ answer = (CopyCmdAnswer) ep.sendMessage(cmd);
+ }
+ CopyCommandResult result = new CopyCommandResult("", answer);
+ callback.complete(result);
}
}
}
@Override
public boolean canCopy(DataObject srcData, DataObject destData) {
//BUG fix for CLOUDSTACK-4618
- DataStore store = destData.getDataStore();
- if (store.getRole() == DataStoreRole.Primary && srcData.getType() == DataObjectType.TEMPLATE
+ DataStore destStore = destData.getDataStore();
+ if (destStore.getRole() == DataStoreRole.Primary && srcData.getType() == DataObjectType.TEMPLATE
&& (destData.getType() == DataObjectType.TEMPLATE || destData.getType() == DataObjectType.VOLUME)) {
- StoragePoolVO storagePoolVO = primaryStoreDao.findById(store.getId());
+ StoragePoolVO storagePoolVO = primaryStoreDao.findById(destStore.getId());
if (storagePoolVO != null && storagePoolVO.getPoolType() == Storage.StoragePoolType.CLVM) {
return true;
}
+ } else if (srcData.getType() == DataObjectType.SNAPSHOT && destData.getType() == DataObjectType.VOLUME) {
+ DataStore srcStore = srcData.getDataStore();
+ if (srcStore.getRole() == DataStoreRole.Primary && destStore.getRole() == DataStoreRole.Primary) {
+ StoragePoolVO srcStoragePoolVO = primaryStoreDao.findById(srcStore.getId());
+ StoragePoolVO dstStoragePoolVO = primaryStoreDao.findById(destStore.getId());
+ if (srcStoragePoolVO != null && srcStoragePoolVO.getPoolType() == StoragePoolType.RBD
Review comment:
```suggestion
if (StoragePoolType.RBD.equals(srcStoragePoolVO)
```
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1594,39 +1595,72 @@ public Answer createVolumeFromSnapshot(final CopyCommand cmd) {
final DataStoreTO imageStore = srcData.getDataStore();
final VolumeObjectTO volume = snapshot.getVolume();
- if (!(imageStore instanceof NfsTO)) {
+ if (!(imageStore instanceof NfsTO) && !(imageStore instanceof PrimaryDataStoreTO)) {
return new CopyCmdAnswer("unsupported protocol");
}
- final NfsTO nfsImageStore = (NfsTO)imageStore;
-
final String snapshotFullPath = snapshot.getPath();
final int index = snapshotFullPath.lastIndexOf("/");
final String snapshotPath = snapshotFullPath.substring(0, index);
final String snapshotName = snapshotFullPath.substring(index + 1);
- final KVMStoragePool secondaryPool = storagePoolMgr.getStoragePoolByURI(nfsImageStore.getUrl() + File.separator + snapshotPath);
- final KVMPhysicalDisk snapshotDisk = secondaryPool.getPhysicalDisk(snapshotName);
+ KVMPhysicalDisk snapshotDisk = null;
+ KVMPhysicalDisk disk = null;
+ if (imageStore instanceof NfsTO) {
Review comment:
Can you ecxtract the if and the else clause into separate methods, please?
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1594,39 +1595,72 @@ public Answer createVolumeFromSnapshot(final CopyCommand cmd) {
final DataStoreTO imageStore = srcData.getDataStore();
final VolumeObjectTO volume = snapshot.getVolume();
- if (!(imageStore instanceof NfsTO)) {
+ if (!(imageStore instanceof NfsTO) && !(imageStore instanceof PrimaryDataStoreTO)) {
Review comment:
```suggestion
if ( ! (imageStore instanceof NfsTO || imageStore instanceof PrimaryDataStoreTO)) {
```
##########
File path: plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
##########
@@ -287,20 +288,45 @@ public void copyAsync(DataObject srcdata, DataObject destData, AsyncCompletionCa
}
CopyCommandResult result = new CopyCommandResult("", answer);
callback.complete(result);
+ } else if (srcdata.getType() == DataObjectType.SNAPSHOT && destData.getType() == DataObjectType.VOLUME) {
+ SnapshotObjectTO srcTO = (SnapshotObjectTO) srcdata.getTO();
+ CopyCommand cmd = new CopyCommand(srcTO, destData.getTO(), StorageManager.PRIMARY_STORAGE_DOWNLOAD_WAIT.value(), true);
+ EndPoint ep = epSelector.select(srcdata, destData);
+ CopyCmdAnswer answer = null;
+ if (ep == null) {
+ String errMsg = "No remote endpoint to send command, check if host or ssvm is down?";
+ s_logger.error(errMsg);
+ answer = new CopyCmdAnswer(errMsg);
+ } else {
+ answer = (CopyCmdAnswer) ep.sendMessage(cmd);
+ }
+ CopyCommandResult result = new CopyCommandResult("", answer);
+ callback.complete(result);
}
}
}
@Override
public boolean canCopy(DataObject srcData, DataObject destData) {
//BUG fix for CLOUDSTACK-4618
- DataStore store = destData.getDataStore();
- if (store.getRole() == DataStoreRole.Primary && srcData.getType() == DataObjectType.TEMPLATE
+ DataStore destStore = destData.getDataStore();
+ if (destStore.getRole() == DataStoreRole.Primary && srcData.getType() == DataObjectType.TEMPLATE
&& (destData.getType() == DataObjectType.TEMPLATE || destData.getType() == DataObjectType.VOLUME)) {
- StoragePoolVO storagePoolVO = primaryStoreDao.findById(store.getId());
+ StoragePoolVO storagePoolVO = primaryStoreDao.findById(destStore.getId());
if (storagePoolVO != null && storagePoolVO.getPoolType() == Storage.StoragePoolType.CLVM) {
return true;
}
+ } else if (srcData.getType() == DataObjectType.SNAPSHOT && destData.getType() == DataObjectType.VOLUME) {
+ DataStore srcStore = srcData.getDataStore();
+ if (srcStore.getRole() == DataStoreRole.Primary && destStore.getRole() == DataStoreRole.Primary) {
+ StoragePoolVO srcStoragePoolVO = primaryStoreDao.findById(srcStore.getId());
+ StoragePoolVO dstStoragePoolVO = primaryStoreDao.findById(destStore.getId());
+ if (srcStoragePoolVO != null && srcStoragePoolVO.getPoolType() == StoragePoolType.RBD
+ && dstStoragePoolVO != null && (dstStoragePoolVO.getPoolType() == StoragePoolType.RBD
Review comment:
```suggestion
&& StoragePoolType.RBD.equals(dstStoragePoolVO)
```
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -454,18 +456,25 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
if (snapInfo == null) {
throw new CloudRuntimeException("Cannot find snapshot " + snapshot.getId());
}
- // We need to copy the snapshot onto secondary.
- SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.BACKUP);
- snapshotStrategy.backupSnapshot(snapInfo);
- // Attempt to grab it again.
- snapInfo = snapshotFactory.getSnapshot(snapshot.getId(), dataStoreRole);
- if (snapInfo == null) {
- throw new CloudRuntimeException("Cannot find snapshot " + snapshot.getId() + " on secondary and could not create backup");
+ boolean backupSnapToSecondary = SnapshotManager.BackupSnapshotAfterTakingSnapshot.value() == null || SnapshotManager.BackupSnapshotAfterTakingSnapshot.value();
+
+ StoragePoolVO srcPool = _storagePoolDao.findById(snapInfo.getBaseVolume().getPoolId());
+ // We need to copy the snapshot onto secondary.
+ //Skipping the backup to secondary storage with NFS/FS could be supported when CLOUDSTACK-5297 is accepted with small enhancement in:
+ //KVMStorageProcessor::createVolumeFromSnapshot and CloudStackPrimaryDataStoreDriverImpl::copyAsync/createAsync
+ if ((!backupSnapToSecondary && (srcPool.getPoolType() == StoragePoolType.NetworkFilesystem || srcPool.getPoolType() == StoragePoolType.Filesystem))) {
+ SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.BACKUP);
+ snapshotStrategy.backupSnapshot(snapInfo);
+ // Attempt to grab it again.
+ snapInfo = snapshotFactory.getSnapshot(snapshot.getId(), dataStoreRole);
+ if (snapInfo == null) {
+ throw new CloudRuntimeException("Cannot find snapshot " + snapshot.getId() + " on secondary and could not create backup");
+ }
Review comment:
can you extract this to a separate method, please?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org