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/20 11:29:06 UTC
[GitHub] [cloudstack] slavkap opened a new pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
slavkap opened a new pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349
### Description
Fixes #4433
- `snapshot.backup.to.secondary=false`
- Create 2 or more snapshots on Ceph/NFS
- Create volume/template from a snapshot. The snapshot first is copied to secondary storage
- Create volume/template from the second snapshot. The snapshot is not copied to secondary because the snapshot info is retrieved with the wrong SQL query. Operation fails.
The problem appears on Ceph/NFS but it probably affects other storage plugins.
Bypassing secondary storage is supported only for Ceph primary storage, but it didn't cover the functionality to create volumes from snapshots which are kept only on Ceph.
With this fix, if you create snapshots only on NFS and try to create volume/template from a snapshot first, the snapshots are copied to secondary storage. For Ceph, the snapshot will be copied to secondary only when you create a template from a snapshot.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] Bug fix (non-breaking change which fixes an issue)
- [X] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [X] Minor
- [ ] Trivial
### How Has This Been Tested?
Manual tests
hypervisor KVM
- create RBD volumes from RBD snapshots - the snapshots are not copied to secondary storage
- create NFS volumes from RBD snapshots - the snapshots are not copied to secondary storage
- create templates from RBD snapshots - the snapshot is copied to secondary storage
- create templates from NFS snapshots - the snapshot is copied to secondary storage
- create NFS volumes from NFS snapshots - the snapshot is copied to secondary storage
--
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
[GitHub] [cloudstack] slavkap commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-904624288
Thanks, @DaanHoogland, @nvazquez, @GutoVeronezi, for the code reviews! I made the requested changes
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-902629225
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r696650221
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -497,6 +492,25 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
}
+ private SnapshotInfo backupSnapshotIfNeeded(Snapshot snapshot, DataStoreRole dataStoreRole, SnapshotInfo snapInfo) {
+ 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 && (StoragePoolType.NetworkFilesystem.equals(srcPool.getPoolType()) || StoragePoolType.Filesystem.equals(srcPool.getPoolType())))) {
Review comment:
I think that there is an extra parenthesis in the IF.
From:
`if ((!backupSnapToSecondary && (StoragePoolType.NetworkFilesystem.equals(srcPool.getPoolType()) || StoragePoolType.Filesystem.equals(srcPool.getPoolType())))) {`
To:
`if (!backupSnapToSecondary && (StoragePoolType.NetworkFilesystem.equals(srcPool.getPoolType()) || StoragePoolType.Filesystem.equals(srcPool.getPoolType()))) {`
Maybe this conditional could be extracted into a method just have a cleaner code.
--
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
[GitHub] [cloudstack] rhtyd commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-902629025
@blueorangutan package
--
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
[GitHub] [cloudstack] slavkap commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r693795743
##########
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:
@DaanHoogland, thanks for the review! I will make the rest of the comments, but I can't entirely agree here. I mean that for me, it's better to compare enums with `==`. In your case, it's not safe to invoke `equals()` if I remove the check for a null value.
Which is the preferred way in ACS to compare enums?
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-905176464
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] rhtyd merged pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349
--
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
[GitHub] [cloudstack] slavkap commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r694774292
##########
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:
Thanks, @GutoVeronezi, for the code review! Yes, I know what the implementation of `equals` of Enum is. That's why I thought that it's a developer decision which one to use :) I will make the requested changes
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-902649522
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 956
--
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
[GitHub] [cloudstack] nvazquez commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r693890850
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1639,6 +1673,74 @@ public Answer createVolumeFromSnapshot(final CopyCommand cmd) {
}
}
+ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, String snapshotName, String name,
+ PhysicalDiskFormat format, long size, KVMStoragePool destPool, int timeout) {
+
+ KVMStoragePool srcPool = volume.getPool();
+ KVMPhysicalDisk disk = null;
+ String newUuid = name;
+ int rbdFeatures = 61;
+
+ format = PhysicalDiskFormat.RAW;
+ disk = new KVMPhysicalDisk(destPool.getSourceDir() + "/" + newUuid, newUuid, destPool);
+ disk.setFormat(format);
+ if (size > volume.getVirtualSize()) {
Review comment:
What about:
````
disk.setSize(size > volume.getVirtualSize() ? size : volume.getVirtualSize());
disk.setVirtualSize(size > volume.getVirtualSize() ? size : disk.getSize());
````
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1639,6 +1673,74 @@ public Answer createVolumeFromSnapshot(final CopyCommand cmd) {
}
}
+ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, String snapshotName, String name,
+ PhysicalDiskFormat format, long size, KVMStoragePool destPool, int timeout) {
+
+ KVMStoragePool srcPool = volume.getPool();
+ KVMPhysicalDisk disk = null;
+ String newUuid = name;
+ int rbdFeatures = 61;
Review comment:
Minor: if this is the same as `LibvirtStorageAdaptor.rbdFeatures`, can we expose it and use it instead of a new variable on this method?
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1639,6 +1673,74 @@ public Answer createVolumeFromSnapshot(final CopyCommand cmd) {
}
}
+ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, String snapshotName, String name,
+ PhysicalDiskFormat format, long size, KVMStoragePool destPool, int timeout) {
+
+ KVMStoragePool srcPool = volume.getPool();
+ KVMPhysicalDisk disk = null;
+ String newUuid = name;
+ int rbdFeatures = 61;
+
+ format = PhysicalDiskFormat.RAW;
+ disk = new KVMPhysicalDisk(destPool.getSourceDir() + "/" + newUuid, newUuid, destPool);
+ disk.setFormat(format);
+ if (size > volume.getVirtualSize()) {
+ disk.setSize(size);
+ disk.setVirtualSize(size);
+ } else {
+ disk.setSize(volume.getVirtualSize());
+ disk.setVirtualSize(disk.getSize());
+ }
+
+ try {
+
+ Rados r = new Rados(srcPool.getAuthUserName());
+ r.confSet("mon_host", srcPool.getSourceHost() + ":" + srcPool.getSourcePort());
+ r.confSet("key", srcPool.getAuthSecret());
+ r.confSet("client_mount_timeout", "30");
+ r.connect();
+
+ IoCTX io = r.ioCtxCreate(srcPool.getSourceDir());
+ Rbd rbd = new Rbd(io);
+ RbdImage srcImage = rbd.open(volume.getName());
+
+ List<RbdSnapInfo> snaps = srcImage.snapList();
+ boolean snapFound = false;
+ for (RbdSnapInfo snap : snaps) {
+ if (snapshotName.equals(snap.name)) {
+ snapFound = true;
+ break;
+ }
+ }
+
+ if (!snapFound) {
+ return null;
Review comment:
Maybe add a log line as well to help debugging/tracing errors?
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-905231066
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-905191493
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 1003
--
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
[GitHub] [cloudstack] rhtyd merged pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349
--
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
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r694188928
##########
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:
I think that what @DaanHoogland meant is:
```suggestion
if (srcStoragePoolVO != null && StoragePoolType.RBD.equals(srcStoragePoolVO.getPoolType())
```
Technically there are no differences in comparing `enums` with `equals` or `==`, because `Enum#equals` implements `==`:
```java
public final boolean equals(Object other) {
return this==other;
}
```
As far as I know, there is no consensus about it, however I would go with Daan and use `equals` to compare `enums`, as any other object we compare, and let `==` only to primitive types.
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1639,6 +1673,74 @@ public Answer createVolumeFromSnapshot(final CopyCommand cmd) {
}
}
+ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, String snapshotName, String name,
+ PhysicalDiskFormat format, long size, KVMStoragePool destPool, int timeout) {
+
+ KVMStoragePool srcPool = volume.getPool();
+ KVMPhysicalDisk disk = null;
+ String newUuid = name;
+ int rbdFeatures = 61;
+
+ format = PhysicalDiskFormat.RAW;
+ disk = new KVMPhysicalDisk(destPool.getSourceDir() + "/" + newUuid, newUuid, destPool);
+ disk.setFormat(format);
+ if (size > volume.getVirtualSize()) {
+ disk.setSize(size);
+ disk.setVirtualSize(size);
+ } else {
+ disk.setSize(volume.getVirtualSize());
+ disk.setVirtualSize(disk.getSize());
+ }
+
+ try {
+
+ Rados r = new Rados(srcPool.getAuthUserName());
+ r.confSet("mon_host", srcPool.getSourceHost() + ":" + srcPool.getSourcePort());
+ r.confSet("key", srcPool.getAuthSecret());
+ r.confSet("client_mount_timeout", "30");
+ r.connect();
+
+ IoCTX io = r.ioCtxCreate(srcPool.getSourceDir());
+ Rbd rbd = new Rbd(io);
+ RbdImage srcImage = rbd.open(volume.getName());
+
+ List<RbdSnapInfo> snaps = srcImage.snapList();
+ boolean snapFound = false;
+ for (RbdSnapInfo snap : snaps) {
+ if (snapshotName.equals(snap.name)) {
+ snapFound = true;
+ break;
+ }
+ }
+
+ if (!snapFound) {
+ return null;
+ }
+ srcImage.snapProtect(snapshotName);
+
+ rbd.clone(volume.getName(), snapshotName, io, disk.getName(), rbdFeatures, 0);
+ RbdImage diskImage = rbd.open(disk.getName());
+ if (disk.getVirtualSize() > volume.getVirtualSize()) {
+ diskImage.resize(disk.getVirtualSize());
+ }
+
+ diskImage.flatten();
+ rbd.close(diskImage);
+
+ srcImage.snapUnprotect(snapshotName);
+ rbd.close(srcImage);
+ r.ioCtxDestroy(io);
+ } catch (RadosException e) {
+ s_logger.error(String.format("Failed due to %s", e.getMessage()));
+ disk = null;
+ } catch (RbdException e) {
+ s_logger.error(String.format("Failed due to: %s", e.getMessage()));
+ disk = null;
+ }
Review comment:
We could join the catches and add the exception as to the log:
```suggestion
} catch (RbdException | RadosException e) {
s_logger.error(String.format("Failed due to %s", e.getMessage()), e);
disk = null;
}
```
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-902682385
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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
[GitHub] [cloudstack] sureshanaparti commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-905230815
@blueorangutan test
--
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
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#discussion_r697254336
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -497,6 +492,25 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
}
+ private SnapshotInfo backupSnapshotIfNeeded(Snapshot snapshot, DataStoreRole dataStoreRole, SnapshotInfo snapInfo) {
+ 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
Review comment:
potential javadoc?
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-902682195
@blueorangutan test
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-905669341
<b>Trillian test result (tid-1770)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32038 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5349-t1770-kvm-centos7.zip
Smoke tests completed. 87 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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
[GitHub] [cloudstack] nvazquez commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-905176369
@blueorangutan package
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5349: Fix of creating volumes from snapshots without backup to secondary storage
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5349:
URL: https://github.com/apache/cloudstack/pull/5349#issuecomment-903004949
<b>Trillian test result (tid-1738)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34871 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5349-t1738-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Smoke tests completed. 87 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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