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/12/03 08:38:33 UTC
[GitHub] [cloudstack] GutoVeronezi opened a new pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
GutoVeronezi opened a new pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297
### Description
This PR solves issue #5124.
The main focus of it is to change how volume snapshots are taken in KVM. However, this proposal of changing `take snapshot` workflow in KVM impacts others snapshot workflows as well. The impacted workflows we mapped are:
- Take volume snapshot with running vm
- Take volume snapshot with stopped vm
- Recurring volume snapshot
- Create template from snapshot
- Create volume from snapshot
- Migrate virtual machine with volumes
- Migrate volumes
- Revert volume snapshot
- Delete volume snapshot
Issue #5124 has more details and flowcharts of all the mapped workflows.
The focus of this PR is to refactor KVM snapshot, others hypervisors workflows should not be affected.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] 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
- [x] Major
- [ ] Minor
### How Has This Been Tested?
I added unit tests to several methods and tested the workflows:
1. Take volume snapshot with running vm
2. Take volume snapshot with stopped vm
3. Recurring volume snapshot
4. Create template from snapshot
5. Create volume from snapshot
6. Migrate virtual machine with volumes
7. Migrate volumes
8. Revert volume snapshot
9. Delete volume snapshot
These workflows were tested twice, once with the global setting `snapshot.backup.to.secondary` as `true` and once as `false`.
To test workflows 1, 2, and 3, I created a VM called `Test1`, then I opened its console to create a file, which I called `testfile`. At first, I wrote `gold` in this file and took a snapshot that I called `gold` (this tests workflow 1). Then I wrote `silver` to the file and configured a recurring snapshot. I waited for the recurring snapshot and, when it was took, I changed the name in database to `silver`, to facilitate the steps (this tests workflow 2). Finally, I wrote `copper` to the file and stopped the VM, then I took a snapshot (this tests workflow 3). After that, I destroyed VM `Test1`.
To test workflow 4, I created a template from snapshot `silver` and called it `template silver`. From it, I created a VM called `Test2`, then I opened its console and retrieved the content of the `testfile` with the command `cat`. It returned `silver`, which is the same value of the file when I took the snapshot on workflow 2. After that, I stopped VM `Test2`.
To test workflow 5, I created a volume from snapshot `gold` and called it `volume gold`, then I changed the root volume of the VM `Test2` to the `volume gold`. I started the VM `Test2` and retrieved the content of the `testfile` with the command `cat`. It returned `gold`, which is the same value of the file when I took the snapshot on workflow 1.
To test workflows 6 and 7, I wrote `platinum` to the file `testfile` in VM `Test2`, then I took a snapshot and called it `platinum`. I migrated the VM and the volume to another host and primary storage with the API `migrateVirtualMachineWithVolume`. When the global setting `snapshot.backup.to.secondary` was `true`, it migrated successfully. When it was `false`, an exception was threw, warning user about snapshots on primary storage (this tests workflow 6). I stopped the VM and migrated only the volume to another primary storage. When the global setting `snapshot.backup.to.secondary` was `true`, it migrated successfully. When it was `false`, an exception was threw, warning users about snapshots on primary storagee (this tests workflow 7).
To test workflow 8, I started VM `Test2` and wrote `diamond` to the file `testfile`. Then I stopped the VM `Test2` and reverted the volume to the snapshot `platinum`. Then I opened its console and retrieved the content of the `testfile` with the command `cat`. It returned `platinum`, which is the same value of the file when I took the snapshot before the workflow 6.
To test workflow 9, I deleted all the snapshots.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r695336940
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -231,9 +229,14 @@
@Inject
VolumeApiService _volumeApiService;
+ @Inject
+ protected SnapshotHelper snapshotHelper;
+
private final StateMachine2<Volume.State, Volume.Event, Volume> _volStateMachine;
protected List<StoragePoolAllocator> _storagePoolAllocators;
+ protected boolean backupSnapshotAfterTakingSnapshot = BackupSnapshotAfterTakingSnapshot.value();
Review comment:
You are right, thanks, have verified it. Discard then :)
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-912420524
@GutoVeronezi, because this feature supports bypassing secondary storage with the option `snapshot.backup.to.secondary=false`, a problem appears when you delete a volume, and there are more than one snapshots created from it only on the primary storage. One snapshot is deleted in the DB, but it remains on the primary storage:
>
> mysql> select store_role, snapshot_id, install_path from snapshot_store_ref where volume_id=307;
> +------------+-------------+------------------------------------------------------------------------------------------+
> | store_role | snapshot_id | install_path |
> +------------+-------------+------------------------------------------------------------------------------------------+
> | Primary | 40 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/d537d452-06c2-484d-a19e-851e47cb2857 |
> | Primary | 41 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/0335db51-0a6d-472c-b587-fbfccfe21ee1 |
> | Primary | 42 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/3c252bf7-f3a2-4d86-906d-9fc4d78ea517 |
> +------------+-------------+------------------------------------------------------------------------------------------+
> 3 rows in set (0.00 sec)
>
> mysql> select store_role, snapshot_id, install_path from snapshot_store_ref where volume_id=307;
> +------------+-------------+------------------------------------------------------------------------------------------+
> | store_role | snapshot_id | install_path |
> +------------+-------------+------------------------------------------------------------------------------------------+
> | Primary | 40 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/d537d452-06c2-484d-a19e-851e47cb2857 |
> | Primary | 42 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/3c252bf7-f3a2-4d86-906d-9fc4d78ea517 |
> +------------+-------------+------------------------------------------------------------------------------------------+
> 2 rows in set (0.00 sec)
> ls -latr snapshots/
> total 131460
> -rw-r--r-- 1 root root 44761088 Sep 3 12:28 d537d452-06c2-484d-a19e-851e47cb2857
> -rw-r--r-- 1 root root 44826624 Sep 3 12:28 0335db51-0a6d-472c-b587-fbfccfe21ee1
> drwxr-xr-x 2 root root 138 Sep 3 12:30 .
> -rw-r--r-- 1 root root 45023232 Sep 3 12:30 3c252bf7-f3a2-4d86-906d-9fc4d78ea517
The deletion is invoked in `VolumeServiceImpl.deleteVolumeCallback()`. I guess the fix of this problem has to be discussed by the community. I mean which solution is the right one:
- to delete all snapshots from primary when deleting the volume
- to remove the code which deletes a record from the DB
- or another option
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-972925446
Packaging result: :heavy_multiplication_x: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1738
--
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 edited a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-899537390
Regarding to @slavkap https://github.com/apache/cloudstack/pull/5297#discussion_r686963157, the feature `blockcommit` was introduced on Libvirt's version `1.2.9`, according to its docs. However some tags were introduced as a future work mark. The tag `delete` was really implemented only in version `6.0.0`, through the commit [qemu: block: enable the snapshot image deletion feature](https://gitlab.com/libvirt/libvirt/-/commit/7d484ede208f33817d3ed033f6bf27522fff5fb5).
Therefore, for cases which operators cannot upgrade Libvirt (like in CentOS 7) to a version that supports this flag, we created a workaround, manually deleting the unused snapshot file. As soon as we have everybody using a version equal or higher than `6.0.0`, we can remove this workaround.
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1020482007
Thanks for the reviews, @slavkap.
Regarding to list all the snapshots directly by the snapshot role: the intention is to list snapshots that only exists in primary storage, therefore, I need to list all the roles and verify if there are no entries with the role `Image` (a.k.a 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] blueorangutan commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985372049
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1824
--
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] weizhouapache commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1057805897
@blueorangutan test ubuntu20 kvm-ubuntu20
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1085073503
Hi @nvazquez, just sent the changes regarding taking snapshot in CentOS 7.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r782154030
##########
File path: engine/storage/src/main/java/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java
##########
@@ -520,4 +531,22 @@ private boolean isSnapshotChainingRequired(long volumeId) {
return false;
}
+ @Override
+ public boolean expungeReferenceBySnapshotIdAndDataStoreRole(long snapshotId, DataStoreRole dataStoreRole) {
+ SnapshotDataStoreVO snapshotDataStoreVo = findOneBy(createSearchCriteriaBySnapshotIdAndStoreRole(snapshotId, dataStoreRole));
+
+ if (snapshotDataStoreVo == null) {
+ return true;
+ }
+
+ return expunge(snapshotDataStoreVo.getId());
+ }
+
+ @Override
+ public List<SnapshotDataStoreVO> listReadyByVolumeId(long volumeId) {
+ SearchCriteria<SnapshotDataStoreVO> sc = volumeIdAndStateReadySearch.create();
+ sc.setParameters("volume_id", volumeId);
+ sc.setParameters("state", State.Ready);
Review comment:
` sc.setParameters("store_role", role);
`
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-919366170
> @GutoVeronezi, because this feature supports bypassing secondary storage with the option `snapshot.backup.to.secondary=false`, a problem appears when you delete a volume, and there are more than one snapshots created from it only on the primary storage. One snapshot is deleted in the DB, but it remains on the primary storage:
>
> > mysql> select store_role, snapshot_id, install_path from snapshot_store_ref where volume_id=307;
> > +------------+-------------+------------------------------------------------------------------------------------------+
> > | store_role | snapshot_id | install_path |
> > +------------+-------------+------------------------------------------------------------------------------------------+
> > | Primary | 40 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/d537d452-06c2-484d-a19e-851e47cb2857 |
> > | Primary | 41 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/0335db51-0a6d-472c-b587-fbfccfe21ee1 |
> > | Primary | 42 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/3c252bf7-f3a2-4d86-906d-9fc4d78ea517 |
> > +------------+-------------+------------------------------------------------------------------------------------------+
> > 3 rows in set (0.00 sec)
> > mysql> select store_role, snapshot_id, install_path from snapshot_store_ref where volume_id=307;
> > +------------+-------------+------------------------------------------------------------------------------------------+
> > | store_role | snapshot_id | install_path |
> > +------------+-------------+------------------------------------------------------------------------------------------+
> > | Primary | 40 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/d537d452-06c2-484d-a19e-851e47cb2857 |
> > | Primary | 42 | /mnt/c9b09777-7340-3752-8a96-f3835f9f5c53/snapshots/3c252bf7-f3a2-4d86-906d-9fc4d78ea517 |
> > +------------+-------------+------------------------------------------------------------------------------------------+
> > 2 rows in set (0.00 sec)
>
> > ls -latr snapshots/
> > total 131460
> > -rw-r--r-- 1 root root 44761088 Sep 3 12:28 d537d452-06c2-484d-a19e-851e47cb2857
> > -rw-r--r-- 1 root root 44826624 Sep 3 12:28 0335db51-0a6d-472c-b587-fbfccfe21ee1
> > drwxr-xr-x 2 root root 138 Sep 3 12:30 .
> > -rw-r--r-- 1 root root 45023232 Sep 3 12:30 3c252bf7-f3a2-4d86-906d-9fc4d78ea517
>
> The deletion is invoked in `VolumeServiceImpl.deleteVolumeCallback()`. I guess the fix of this problem has to be discussed by the community. I mean which solution is the right one:
>
> * to delete all snapshots from primary when deleting the volume
> * to remove the code which deletes a record from the DB
> * or another option
@slavkap as we discussed offline, it is a point that community need to discuss (through issue #5433 and [ML](https://lists.apache.org/thread.html/r9438e1b5e58fee2275b0474255b86cd398666433e10d11b3cd519fac%40%3Cdev.cloudstack.apache.org%3E)) and this PR won't be addressing it, at first.
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-898707759
@RodrigoDLopez thanks for the review. I just updated the tests descriptions. If there is anything to improve, let me know.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r694225751
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
##########
@@ -100,20 +111,23 @@ public Answer execute(final RevertSnapshotCommand command, final LibvirtComputin
rbd.close(image);
rados.ioCtxDestroy(io);
} else {
- NfsTO nfsImageStore = (NfsTO)snapshotImageStore;
- String secondaryStoragePoolUrl = nfsImageStore.getUrl();
- secondaryStoragePool = storagePoolMgr.getStoragePoolByURI(secondaryStoragePoolUrl);
- String ssPmountPath = secondaryStoragePool.getLocalPath();
- snapshotPath = ssPmountPath + File.separator + snapshotRelPath;
-
- Script cmd = new Script(libvirtComputingResource.manageSnapshotPath(), libvirtComputingResource.getCmdsTimeout(), s_logger);
- cmd.add("-v", snapshotPath);
- cmd.add("-n", snapshotDisk.getName());
- cmd.add("-p", snapshotDisk.getPath());
- String result = cmd.execute();
- if (result != null) {
- s_logger.debug("Failed to revert snaptshot: " + result);
- return new Answer(command, false, result);
+ KVMStoragePool secondaryStoragePool = null;
+ if (snapshotImageStore != null && DataStoreRole.Primary != snapshotImageStore.getRole()) {
+ storagePoolMgr.getStoragePoolByURI(snapshotImageStore.getUrl());
+ }
+
+ if (primaryPool.getType() == StoragePoolType.CLVM) {
Review comment:
@nvazquez reverting snapshots that were only in primary storage was not working correctly for KVM. The `RBD` validation was added in PR #3502 to allow `Ceph` to revert snapshots that were only in primary storage, however the rest kept not working. This PR fixed this error to some others storage pool types (which were added to the validation), however `CLVM` was not our focus, therefore we tried to not touch in it.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r722529418
##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -262,142 +274,93 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status");
}
- Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
- boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
-
- if (deletedOnPrimary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId));
- } else {
- s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId));
- }
- if (null != deletedOnSecondary && deletedOnSecondary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId));
- }
- return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary;
+ return destroySnapshotEntriesAndFiles(snapshotVO);
}
- private boolean deleteOnPrimaryIfNeeded(Long snapshotId) {
- SnapshotVO snapshotVO;
- boolean deletedOnPrimary = false;
- snapshotVO = snapshotDao.findById(snapshotId);
- SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) {
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- } else {
- // Here we handle snapshots which are to be deleted but are not marked as destroyed yet.
- // This may occur for instance when they are created only on primary because
- // snapshot.backup.to.secondary was set to false.
- if (snapshotOnPrimaryInfo == null) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot (id: %d) not found on primary storage, skipping snapshot deletion on primary and marking it destroyed", snapshotId));
- }
- snapshotVO.setState(Snapshot.State.Destroyed);
- snapshotDao.update(snapshotId, snapshotVO);
- deletedOnPrimary = true;
- } else {
- SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
- try {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- if (!deletedOnPrimary) {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } else {
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
- deletedOnPrimary = false;
- }
- }
+ /**
+ * Destroys the snapshot entries and files on both primary and secondary storage (if it exists).
+ * @return true if destroy successfully, else false.
+ */
+ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo) {
+ if (!deleteSnapshotInfos(snapshotVo)) {
+ return false;
}
- return deletedOnPrimary;
- }
- private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) {
- Boolean deletedOnSecondary = null;
- SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
- if (snapshotOnImage == null) {
- s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId));
- } else {
- SnapshotObject obj = (SnapshotObject)snapshotOnImage;
- try {
- deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj);
- if (!deletedOnSecondary) {
- s_logger.debug(
- String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.",
- snapshotId));
- } else {
- s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId));
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
-// deletedOnSecondary remain null
- }
- }
- return deletedOnSecondary;
+ updateSnapshotToDestroyed(snapshotVo);
+
+ return true;
}
/**
- * Deletes the snapshot on secondary storage.
- * It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false.
+ * Updates the snapshot to {@link Snapshot.State#Destroyed}.
*/
- private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- List<VolumeDetailVO> volumesFromSnapshot;
- volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);
+ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
+ snapshotVo.setState(Snapshot.State.Destroyed);
+ snapshotDao.update(snapshotVo.getId(), snapshotVo);
+ }
- if (volumesFromSnapshot.size() > 0) {
- try {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " + e1.toString());
+ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo) {
+ Map<String, SnapshotInfo> snapshotInfos = retrieveSnapshotEntries(snapshotVo.getId());
+
+ for (var infoEntry : snapshotInfos.entrySet()) {
+ if (!deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(), snapshotVo)) {
+ return false;
}
- throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
}
- boolean result = deleteSnapshotChain(snapshotOnImage);
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- return result;
+ return true;
}
/**
- * Deletes the snapshot on primary storage. It returns true when the snapshot was not found on primary storage; </br>
- * In case of failure while deleting the snapshot, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException. </br>
+ * Destroys the snapshot entry and file.
+ * @return true if destroy successfully, else false.
*/
- private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (isSnapshotOnPrimaryStorage(snapshotId)) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is found on primary storage for snapshot id: %d, performing snapshot deletion on primary", snapshotId));
- }
- if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
- snapshotOnPrimary.setState(State.Destroyed);
- snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Successfully deleted snapshot id: %d on primary storage", snapshotId));
- }
+ protected boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage, SnapshotVO snapshotVo) {
+ if (snapshotInfo == null) {
+ s_logger.debug(String.format("Could not find %s entry on a %s. Skipping deletion on %s.", snapshotVo, storage, storage));
+ return true;
+ }
+
+ DataStore dataStore = snapshotInfo.getDataStore();
+ storage = String.format("%s {uuid: \"%s\", name: \"%s\"}", storage, dataStore.getUuid(), dataStore.getName());
+
+ try {
+ SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);
+ snapshotObject.processEvent(Snapshot.Event.DestroyRequested);
+
+ if (deleteSnapshotChain(snapshotInfo, storage)) {
+ snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+ s_logger.debug(String.format("%s was deleted on %s.", snapshotVo, storage));
return true;
}
- } else {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is not found on primary storage for snapshot id: %d, skipping snapshot deletion on primary", snapshotId));
- }
- return true;
+
+ snapshotObject.processEvent(Snapshot.Event.OperationFailed);
+ s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storage));
+ } catch (NoTransitionException ex) {
+ s_logger.warn(String.format("Failed to delete %s on %s due to %s.", snapshotVo, storage, ex.getMessage()), ex);
}
+
return false;
}
/**
- * Returns true if the snapshot volume is on primary storage.
+ * Cast SnapshotInfo to SnapshotObject.
+ * @return SnapshotInfo cast to SnapshotObject.
*/
- private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotOnPrimary != null) {
- long volumeId = snapshotOnPrimary.getVolumeId();
- VolumeVO volumeVO = volumeDao.findById(volumeId);
- return volumeVO != null && volumeVO.getRemoved() == null;
- }
- return false;
+ protected SnapshotObject castSnapshotInfoToSnapshotObject(SnapshotInfo snapshotInfo) {
+ return (SnapshotObject) snapshotInfo;
+ }
Review comment:
Sorry for the delay.
I create a method to cast it to facilitate unit 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] weizhouapache commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1057825888
@GutoVeronezi
can you merge latest main branch into your branch ?
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r818671764
##########
File path: scripts/storage/qcow2/managesnapshot.sh
##########
@@ -226,34 +226,10 @@ backup_snapshot() {
return 2
fi
elif [ -f ${disk} ]; then
- # Does the snapshot exist?
- qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk 2>&1)
- ret_code=$?
- if [ $ret_code -gt 0 ] && [[ $qemuimg_ret == *"snapshot: invalid option -- 'U'"* ]]
- then
- forceShareFlag=""
- qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk)
- ret_code=$?
- fi
- if [ $ret_code -gt 0 ] || [[ ! $qemuimg_ret == *"$snapshotname"* ]]
- then
- printf "there is no $snapshotname on disk $disk\n" >&2
- return 1
- fi
- qemuimg_ret=$($qemu_img convert $forceShareFlag -f qcow2 -O qcow2 -l snapshot.name=$snapshotname $disk $destPath/$destName 2>&1 > /dev/null)
+ cp "$disk" "${destPath}/${destName}"
Review comment:
If you check the snapshot creation workflow, it creates a delta of the disk, copies it to another folder and then `blockcommit` the delta with the original file. This way, we only have a single file, which will be copied to the secondary storage later. If you look at the whole snapshot workflow, you will notice that there is no need to convert the image to another format.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-957230976
@GutoVeronezi we are releasing 4.16 now and this is waiting to be merged after that as far as I can tell
--
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 edited a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442
@rhtyd
The errors are related to the CentOS's QEMU binary:
`Operation not supported: live disk snapshot not supported with this QEMU binary`
RH removed/blocked a lot of features in CentOS, like VM migration, memory hotplug and so on. Therefore, default CentOS's QEMU binary does not support them. An alternative is to use `qemu-kvm-ev-2.12.0-44.1.el7_8.1`, which supports more features than the default one.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r694237959
##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -262,142 +271,93 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status");
}
- Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
- boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
-
- if (deletedOnPrimary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId));
- } else {
- s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId));
- }
- if (null != deletedOnSecondary && deletedOnSecondary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId));
- }
- return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary;
+ return destroySnapshotEntriesAndFiles(snapshotVO);
}
- private boolean deleteOnPrimaryIfNeeded(Long snapshotId) {
- SnapshotVO snapshotVO;
- boolean deletedOnPrimary = false;
- snapshotVO = snapshotDao.findById(snapshotId);
- SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) {
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- } else {
- // Here we handle snapshots which are to be deleted but are not marked as destroyed yet.
- // This may occur for instance when they are created only on primary because
- // snapshot.backup.to.secondary was set to false.
- if (snapshotOnPrimaryInfo == null) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot (id: %d) not found on primary storage, skipping snapshot deletion on primary and marking it destroyed", snapshotId));
- }
- snapshotVO.setState(Snapshot.State.Destroyed);
- snapshotDao.update(snapshotId, snapshotVO);
- deletedOnPrimary = true;
- } else {
- SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
- try {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- if (!deletedOnPrimary) {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } else {
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
- deletedOnPrimary = false;
- }
- }
+ /**
+ * Destroys the snapshot entries and files on both primary and secondary storage (if it exists).
+ * @return true if destroy successfully, else false.
+ */
+ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo) {
+ if (!deleteSnapshotInfos(snapshotVo)) {
+ return false;
}
- return deletedOnPrimary;
- }
- private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) {
- Boolean deletedOnSecondary = null;
- SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
- if (snapshotOnImage == null) {
- s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId));
- } else {
- SnapshotObject obj = (SnapshotObject)snapshotOnImage;
- try {
- deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj);
- if (!deletedOnSecondary) {
- s_logger.debug(
- String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.",
- snapshotId));
- } else {
- s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId));
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
-// deletedOnSecondary remain null
- }
- }
- return deletedOnSecondary;
+ updateSnapshotToDestroyed(snapshotVo);
+
+ return true;
}
/**
- * Deletes the snapshot on secondary storage.
- * It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false.
+ * Updates the snapshot to {@link Snapshot.State#Destroyed}.
*/
- private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- List<VolumeDetailVO> volumesFromSnapshot;
- volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);
+ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
+ snapshotVo.setState(Snapshot.State.Destroyed);
+ snapshotDao.update(snapshotVo.getId(), snapshotVo);
+ }
- if (volumesFromSnapshot.size() > 0) {
- try {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " + e1.toString());
+ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo) {
+ Map<String, SnapshotInfo> snapshotInfos = retrieveSnapshotEntries(snapshotVo.getId());
+
+ for (var infoEntry : snapshotInfos.entrySet()) {
+ if (!deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(), snapshotVo)) {
+ return false;
}
- throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
}
- boolean result = deleteSnapshotChain(snapshotOnImage);
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- return result;
+ return true;
}
/**
- * Deletes the snapshot on primary storage. It returns true when the snapshot was not found on primary storage; </br>
- * In case of failure while deleting the snapshot, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException. </br>
+ * Destroys the snapshot entry and file.
+ * @return true if destroy successfully, else false.
*/
- private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (isSnapshotOnPrimaryStorage(snapshotId)) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is found on primary storage for snapshot id: %d, performing snapshot deletion on primary", snapshotId));
- }
- if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
- snapshotOnPrimary.setState(State.Destroyed);
- snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Successfully deleted snapshot id: %d on primary storage", snapshotId));
- }
+ protected boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage, SnapshotVO snapshotVo) {
+ if (snapshotInfo == null) {
+ s_logger.debug(String.format("Could not find %s entry on a %s. Skipping deletion on %s.", snapshotVo, storage, storage));
+ return true;
+ }
+
+ DataStore dataStore = snapshotInfo.getDataStore();
+ storage = String.format("%s {uuid: \"%s\", name: \"%s\"}", storage, dataStore.getUuid(), dataStore.getName());
+
+ try {
+ SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);
+ snapshotObject.processEvent(Snapshot.Event.DestroyRequested);
+
+ if (deleteSnapshotChain(snapshotInfo, storage)) {
+ snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+ s_logger.debug(String.format("%s was deleted on %s.", snapshotVo, storage));
return true;
}
- } else {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is not found on primary storage for snapshot id: %d, skipping snapshot deletion on primary", snapshotId));
- }
- return true;
+
+ snapshotObject.processEvent(Snapshot.Event.OperationFailed);
+ s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storage));
+ } catch (NoTransitionException ex) {
+ s_logger.warn(String.format("Failed to delete %s on %s due to %s.", snapshotVo, storage, ex.getMessage()), ex);
}
+
return false;
}
/**
- * Returns true if the snapshot volume is on primary storage.
+ * Cast SnapshotInfo to SnapshotObject.
+ * @return SnapshotInfo cast to SnapshotObject.
*/
- private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotOnPrimary != null) {
- long volumeId = snapshotOnPrimary.getVolumeId();
- VolumeVO volumeVO = volumeDao.findById(volumeId);
- return volumeVO != null && volumeVO.getRemoved() == null;
- }
- return false;
+ protected SnapshotObject castSnapshotInfoToSnapshotObject(SnapshotInfo snapshotInfo) {
+ return (SnapshotObject) snapshotInfo;
+ }
+
+ /**
+ * Retrieves the snapshot infos on primary and secondary storage.
+ * @param snapshotId The snapshot to retrieve the infos.
+ * @return A map of snapshot infos.
+ */
+ protected Map<String, SnapshotInfo> retrieveSnapshotEntries(long snapshotId) {
+ Map<String, SnapshotInfo> snapshotInfos = new LinkedHashMap<>();
+ snapshotInfos.put("secondary storage", snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image, false));
Review comment:
@nvazquez done, thanks!
--
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 edited a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442
@rhtyd
The errors are related to the CentOS's QEMU binary:
`Operation not supported: live disk snapshot not supported with this QEMU binary`
RH removed/blocked a lot of features in CentOS, like VM migration, memory hotplug and so on. Therefore, default CentOS's QEMU binary does not support them. An alternative is to use `qemu-kvm-ev-2.12.0-44.1.el7_8.1`, which supports more features than the default one.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-957230976
@GutoVeronezi we are releasing 4.16 now and this is waiting to be merged after that as far as I can tell
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-975325508
@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] blueorangutan removed a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-972870292
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r782152003
##########
File path: server/src/main/java/org/apache/cloudstack/snapshot/SnapshotHelper.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.cloudstack.snapshot;
+
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.hypervisor.Hypervisor.HypervisorType;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.Snapshot;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.Storage.StoragePoolType;
+import com.cloud.storage.dao.SnapshotDao;
+
+import static com.cloud.storage.snapshot.SnapshotManager.BackupSnapshotAfterTakingSnapshot;
+import com.cloud.utils.exception.CloudRuntimeException;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import javax.inject.Inject;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreCapabilities;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
+import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory;
+import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.collections.MapUtils;
+import org.apache.commons.lang3.BooleanUtils;
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.commons.lang3.builder.ToStringStyle;
+import org.apache.log4j.Logger;
+
+public class SnapshotHelper {
+ private final Logger logger = Logger.getLogger(this.getClass());
+
+ @Inject
+ protected SnapshotDataStoreDao snapshotDataStoreDao;
+
+ @Inject
+ protected SnapshotDataFactory snapshotFactory;
+
+ @Inject
+ protected SnapshotService snapshotService;
+
+ @Inject
+ protected StorageStrategyFactory storageStrategyFactory;
+
+ @Inject
+ protected DataStoreManager dataStorageManager;
+
+ @Inject
+ protected SnapshotDao snapshotDao;
+
+ @Inject
+ protected PrimaryDataStoreDao primaryDataStoreDao;
+
+ protected boolean backupSnapshotAfterTakingSnapshot = BackupSnapshotAfterTakingSnapshot.value();
+
+ protected final Set<StoragePoolType> storagePoolTypesToValidateWithBackupSnapshotAfterTakingSnapshot = new HashSet<>(Arrays.asList(StoragePoolType.RBD,
+ StoragePoolType.PowerFlex));
+
+ /**
+ * If the snapshot is a backup from a KVM snapshot that should be kept only in primary storage, expunges it from secondary storage.
+ * @param snapInfo the snapshot info to delete.
+ */
+ public void expungeTemporarySnapshot(boolean kvmSnapshotOnlyInPrimaryStorage, SnapshotInfo snapInfo) {
+ if (!kvmSnapshotOnlyInPrimaryStorage) {
+ logger.trace(String.format("Snapshot [%s] is not a temporary backup to create a volume from snapshot. Not expunging it.", snapInfo.getId()));
+ return;
+ }
+
+ if (snapInfo == null) {
+ logger.warn("Unable to expunge snapshot due to its info is null.");
+ return;
+ }
+
+ logger.debug(String.format("Expunging snapshot [%s] due to it is a temporary backup to create a volume from snapshot. It is occurring because the global setting [%s]"
+ + " has the value [%s].", snapInfo.getId(), BackupSnapshotAfterTakingSnapshot.key(), backupSnapshotAfterTakingSnapshot));
+
+ try {
+ snapshotService.deleteSnapshot(snapInfo);
+ } catch (CloudRuntimeException ex) {
+ logger.warn(String.format("Unable to delete the temporary snapshot [%s] on secondary storage due to [%s]. We still will expunge the database reference, consider"
+ + " manually deleting the file [%s].", snapInfo.getId(), ex.getMessage(), snapInfo.getPath()), ex);
+ }
+
+ snapshotDataStoreDao.expungeReferenceBySnapshotIdAndDataStoreRole(snapInfo.getId(), DataStoreRole.Image);
+ }
+
+ /**
+ * Backup the snapshot to secondary storage if it should be backed up and was not yet or it is a temporary backup to create a volume.
+ * @return The parameter snapInfo if the snapshot is not backupable, else backs up the snapshot to secondary storage and returns its info.
+ * @throws CloudRuntimeException
+ */
+ public SnapshotInfo backupSnapshotToSecondaryStorageIfNotExists(SnapshotInfo snapInfo, DataStoreRole dataStoreRole, Snapshot snapshot, boolean kvmSnapshotOnlyInPrimaryStorage) throws CloudRuntimeException {
+ if (!isSnapshotBackupable(snapInfo, dataStoreRole, kvmSnapshotOnlyInPrimaryStorage)) {
+ logger.trace(String.format("Snapshot [%s] is already on secondary storage or is not a KVM snapshot that is only kept in primary storage. Therefore, we do not back it up."
+ + " up.", snapInfo.getId()));
+
+ return snapInfo;
+ }
+
+ snapInfo = getSnapshotInfoByIdAndRole(snapshot.getId(), DataStoreRole.Primary);
+
+ SnapshotStrategy snapshotStrategy = storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotStrategy.SnapshotOperation.BACKUP);
+ snapshotStrategy.backupSnapshot(snapInfo);
+
+ return getSnapshotInfoByIdAndRole(snapshot.getId(), kvmSnapshotOnlyInPrimaryStorage ? DataStoreRole.Image : dataStoreRole);
+ }
+
+ /**
+ * Search for the snapshot info by the snapshot id and {@link DataStoreRole}.
+ * @return The snapshot info if it exists, else throws an exception.
+ * @throws CloudRuntimeException
+ */
+ protected SnapshotInfo getSnapshotInfoByIdAndRole(long snapshotId, DataStoreRole dataStoreRole) throws CloudRuntimeException{
+ SnapshotInfo snapInfo = snapshotFactory.getSnapshot(snapshotId, dataStoreRole);
+
+ if (snapInfo != null) {
+ return snapInfo;
+ }
+
+ throw new CloudRuntimeException(String.format("Could not find snapshot [%s] in %s storage. Therefore, we do not back it up.", snapshotId, dataStoreRole));
+ }
+
+ /**
+ * Verifies if the snapshot is backupable.
+ * @return true if snapInfo is null and dataStoreRole is {@link DataStoreRole#Image} or is a KVM snapshot that is only kept in primary storage, else false.
+ */
+ protected boolean isSnapshotBackupable(SnapshotInfo snapInfo, DataStoreRole dataStoreRole, boolean kvmSnapshotOnlyInPrimaryStorage) {
+ return (snapInfo == null && dataStoreRole == DataStoreRole.Image) || kvmSnapshotOnlyInPrimaryStorage;
+ }
+
+ /**
+ * Verifies if the snapshot was took on KVM and is kept in primary storage.
+ * @return true if hypervisor is {@link HypervisorType#KVM} and data store role is {@link DataStoreRole#Primary} and global setting "snapshot.backup.to.secondary" is false,
+ * else false.
+ */
+ public boolean isKvmSnapshotOnlyInPrimaryStorage(Snapshot snapshot, DataStoreRole dataStoreRole){
+ return snapshot.getHypervisorType() == Hypervisor.HypervisorType.KVM && dataStoreRole == DataStoreRole.Primary && !backupSnapshotAfterTakingSnapshot;
+ }
+
+ public DataStoreRole getDataStoreRole(Snapshot snapshot) {
+ SnapshotDataStoreVO snapshotStore = snapshotDataStoreDao.findBySnapshot(snapshot.getId(), DataStoreRole.Primary);
+
+ if (snapshotStore == null) {
+ return DataStoreRole.Image;
+ }
+
+ long storagePoolId = snapshotStore.getDataStoreId();
+
+ StoragePoolVO storagePoolVO = primaryDataStoreDao.findById(storagePoolId);
+ if ((storagePoolTypesToValidateWithBackupSnapshotAfterTakingSnapshot.contains(storagePoolVO.getPoolType()) || snapshot.getHypervisorType() == HypervisorType.KVM)
+ && !backupSnapshotAfterTakingSnapshot) {
+ return DataStoreRole.Primary;
+ }
+
+ DataStore dataStore = dataStorageManager.getDataStore(storagePoolId, DataStoreRole.Primary);
+
+ if (dataStore == null) {
+ return DataStoreRole.Image;
+ }
+
+ Map<String, String> mapCapabilities = dataStore.getDriver().getCapabilities();
+
+ if (MapUtils.isNotEmpty(mapCapabilities) && BooleanUtils.toBoolean(mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString()))) {
+ return DataStoreRole.Primary;
+ }
+
+ return DataStoreRole.Image;
+ }
+
+ /**
+ * Verifies if it is a KVM volume that has snapshots only in primary storage.
+ * @throws CloudRuntimeException If it is a KVM volume and has at least one snapshot only in primary storage.
+ */
+ public void isKvmVolumeSnapshotsOnlyInPrimaryStorage(VolumeVO volumeVo, HypervisorType hypervisorType) throws CloudRuntimeException {
+ if (HypervisorType.KVM != hypervisorType) {
+ logger.trace(String.format("The %s hypervisor [%s] is not KVM, therefore we will not check if the snapshots are only in primary storage.", volumeVo, hypervisorType));
+ return;
+ }
+
+ Set<Long> snapshotIdsOnlyInPrimaryStorage = getSnapshotIdsOnlyInPrimaryStorage(volumeVo.getId());
+
+ if (CollectionUtils.isEmpty(snapshotIdsOnlyInPrimaryStorage)) {
+ logger.trace(String.format("%s is a KVM volume and all its snapshots exists in the secondary storage, therefore this volume is able for migration.", volumeVo));
+ return;
+ }
+
+ throwCloudRuntimeExceptionOfSnapshotsOnlyInPrimaryStorage(volumeVo, snapshotIdsOnlyInPrimaryStorage);
+ }
+
+ /**
+ * Throws a CloudRuntimeException with the volume and the snapshots only in primary storage.
+ */
+ protected void throwCloudRuntimeExceptionOfSnapshotsOnlyInPrimaryStorage(VolumeVO volumeVo, Set<Long> snapshotIdsOnlyInPrimaryStorage) throws CloudRuntimeException {
+ List<SnapshotVO> snapshots = snapshotDao.listByIds(snapshotIdsOnlyInPrimaryStorage.toArray());
+
+ String message = String.format("%s is a KVM volume and has snapshots only in primary storage. Snapshots [%s].%s", volumeVo,
+ snapshots.stream().map(snapshot -> new ToStringBuilder(snapshot, ToStringStyle.JSON_STYLE).append("uuid", snapshot.getUuid()).append("name", snapshot.getName())
+ .build()).collect(Collectors.joining(", ")), backupSnapshotAfterTakingSnapshot ? "" : " Consider excluding them to migrate the volume to another storage.");
+
+ logger.error(message);
+ throw new CloudRuntimeException(message);
+ }
+
+ /**
+ * Retrieves the ids of the ready snapshots of the volume that only exists in primary storage.
+ * @param volumeId volume id to retrieve the snapshots.
+ * @return The ids of the ready snapshots of the volume that only exists in primary storage
+ */
+ protected Set<Long> getSnapshotIdsOnlyInPrimaryStorage(long volumeId) {
+ List<SnapshotDataStoreVO> snapshotsReferences = snapshotDataStoreDao.listReadyByVolumeId(volumeId);
Review comment:
You can directly list all ready snapshots by volume id and data store role
```suggestion
List<SnapshotDataStoreVO> snapshotsReferences = snapshotDataStoreDao.listReadyByVolumeId(volumeId, DataStoreRole.Primary);
```
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-926595957
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1390
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-926570010
@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] weizhouapache commented on a change in pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r818447192
##########
File path: scripts/storage/qcow2/managesnapshot.sh
##########
@@ -226,34 +226,10 @@ backup_snapshot() {
return 2
fi
elif [ -f ${disk} ]; then
- # Does the snapshot exist?
- qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk 2>&1)
- ret_code=$?
- if [ $ret_code -gt 0 ] && [[ $qemuimg_ret == *"snapshot: invalid option -- 'U'"* ]]
- then
- forceShareFlag=""
- qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk)
- ret_code=$?
- fi
- if [ $ret_code -gt 0 ] || [[ ! $qemuimg_ret == *"$snapshotname"* ]]
- then
- printf "there is no $snapshotname on disk $disk\n" >&2
- return 1
- fi
- qemuimg_ret=$($qemu_img convert $forceShareFlag -f qcow2 -O qcow2 -l snapshot.name=$snapshotname $disk $destPath/$destName 2>&1 > /dev/null)
+ cp "$disk" "${destPath}/${destName}"
Review comment:
@rohityadavcloud
yes
from my understanding, `cp` is OK in some scenarios, but `qemu-img convert` is required to (1) convert image to another format; (2) convert a image with backing file (template image) to a single image; (2) convert a image with snapshots to a single image;
the sucess of operations does not mean that the generated templates are good.
we need to test this. cc @GutoVeronezi
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1051251285
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2719
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-919346404
@rhtyd done, thanks!
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-917994546
@GutoVeronezi can you fix the build/conflicts
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-923743295
@blueorangutan test matrix
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1085851526
@nvazquez 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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1086628882
<b>Trillian test result (tid-3796)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33733 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t3796-kvm-centos7.zip
Smoke tests completed. 90 look OK, 2 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_snapshot_usage | `Error` | 2.13 | test_usage.py
test_01_snapshot_root_disk | `Error` | 3.19 | test_snapshots.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1051189813
@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] blueorangutan commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985398548
@rhtyd a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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] DaanHoogland commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-976647374
So do we say ACS on CentOS7 only works with qemu-kvm-ev in the docs? or this feature?
cc @GutoVeronezi @andrijapanicsb @rhtyd @weizhouapache @GabrielBrascher , and do we merge like this?
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-976675064
@DaanHoogland on CentOS, users depend on `qemu-kvm-ev` for a lot of features, like running VM migrations, memory hotplug and so on. I think the right thing to do is guide users to upgrade to it.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-904418909
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 994
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-904386026
@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] nvazquez commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-976777114
@GutoVeronezi can you explore a way to adapt the code to follow the previous behaviour in case of an unsupported QEMU version? In this case we will avoid regressions on the 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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-972870292
@DaanHoogland 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] blueorangutan removed a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-928087737
--
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 closed pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985397946
@blueorangutan test matrix
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985780976
<b>Trillian test result (tid-2610)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32228 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2610-kvm-centos7.zip
Smoke tests completed. 89 look OK, 2 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_snapshot_usage | `Error` | 1.12 | test_usage.py
test_01_snapshot_root_disk | `Error` | 2.15 | test_snapshots.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r694234874
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -231,9 +229,14 @@
@Inject
VolumeApiService _volumeApiService;
+ @Inject
+ protected SnapshotHelper snapshotHelper;
+
private final StateMachine2<Volume.State, Volume.Event, Volume> _volStateMachine;
protected List<StoragePoolAllocator> _storagePoolAllocators;
+ protected boolean backupSnapshotAfterTakingSnapshot = BackupSnapshotAfterTakingSnapshot.value();
Review comment:
@nvazquez as it is a `boolean` setting, ACS does not allow operators to inform anything different from `true` or `false`. If the setting is null by some reason, the `value()` will return the default value. If we directly change the setting in the database to something different from `true` or `false`, ACS will validate it with `Boolean.valueOf()`, which will use `"true".equalsIgnoreCase(s)` to verify it. With that said, I don't see a case which the setting will be null.
Do I am missing or misunderstanding anything in the process?
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-956174613
Any update about this one?
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1051279464
@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] GutoVeronezi edited a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-898707759
@RodrigoDLopez thanks for the review. I just updated the tests description. If there is anything to improve, let me know.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-924303808
<b>Trillian test result (tid-2143)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37463 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2143-xenserver-71.zip
Smoke tests completed. 86 look OK, 3 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_nic | `Error` | 203.35 | test_nic.py
test_02_list_snapshots_with_removed_data_store | `Error` | 61.31 | test_snapshots.py
ContextSuite context=TestListIdsParams>:teardown | `Error` | 1.11 | test_list_ids_parameter.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r686582721
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1497,8 +1475,43 @@ public Answer createVolume(final CreateObjectCommand cmd) {
}
}
- protected static final MessageFormat SnapshotXML = new MessageFormat(" <domainsnapshot>" + " <name>{0}</name>" + " <domain>"
- + " <uuid>{1}</uuid>" + " </domain>" + " </domainsnapshot>");
+ /**
+ * XML to take disk-only snapshot of the VM.<br><br>
+ * 1st parameter: snapshot's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: absolute path to create the snapshot;<br>
+ * 4th parameter: list of disks to avoid on snapshot {@link #TAG_AVOID_DISK_FROM_SNAPSHOT};
+ */
+ private static final String XML_CREATE_SNAPSHOT = "<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ + "</domainsnapshot>";
+
+ /**
+ * Tag to avoid disk from snapshot.<br><br>
+ * 1st parameter: disk's label (target.dev tag from VM's XML);
+ */
+ private static final String TAG_AVOID_DISK_FROM_SNAPSHOT = "<disk name='%s' snapshot='no' />";
+
+ /**
+ * Virsh command to merge (blockcommit) snapshot into the base file.<br><br>
+ * 1st parameter: VM's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: the absolute path of the base file;
+ */
+ private static final String COMMAND_MERGE_SNAPSHOT = "virsh blockcommit %s %s --base %s --active --wait --delete --pivot";
Review comment:
@GutoVeronezi, one suggestion - the latest Libvirt's Java API bindings support `qemu-monitor-command` and maybe the command `block-commit` could replace this.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r694175284
##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -575,6 +577,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
}
Map<String, String> options = new HashMap<String, String>();
options.put("fullSnapshot", fullSnapshot.toString());
+ options.put(BackupSnapshotAfterTakingSnapshot.key(), String.valueOf(BackupSnapshotAfterTakingSnapshot.value()));
Review comment:
Also here
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
##########
@@ -100,20 +111,23 @@ public Answer execute(final RevertSnapshotCommand command, final LibvirtComputin
rbd.close(image);
rados.ioCtxDestroy(io);
} else {
- NfsTO nfsImageStore = (NfsTO)snapshotImageStore;
- String secondaryStoragePoolUrl = nfsImageStore.getUrl();
- secondaryStoragePool = storagePoolMgr.getStoragePoolByURI(secondaryStoragePoolUrl);
- String ssPmountPath = secondaryStoragePool.getLocalPath();
- snapshotPath = ssPmountPath + File.separator + snapshotRelPath;
-
- Script cmd = new Script(libvirtComputingResource.manageSnapshotPath(), libvirtComputingResource.getCmdsTimeout(), s_logger);
- cmd.add("-v", snapshotPath);
- cmd.add("-n", snapshotDisk.getName());
- cmd.add("-p", snapshotDisk.getPath());
- String result = cmd.execute();
- if (result != null) {
- s_logger.debug("Failed to revert snaptshot: " + result);
- return new Answer(command, false, result);
+ KVMStoragePool secondaryStoragePool = null;
+ if (snapshotImageStore != null && DataStoreRole.Primary != snapshotImageStore.getRole()) {
+ storagePoolMgr.getStoragePoolByURI(snapshotImageStore.getUrl());
+ }
+
+ if (primaryPool.getType() == StoragePoolType.CLVM) {
Review comment:
Does it need to be part of `storagePoolTypesThatSupportRevertSnapshot`?
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -231,9 +229,14 @@
@Inject
VolumeApiService _volumeApiService;
+ @Inject
+ protected SnapshotHelper snapshotHelper;
+
private final StateMachine2<Volume.State, Volume.Event, Volume> _volStateMachine;
protected List<StoragePoolAllocator> _storagePoolAllocators;
+ protected boolean backupSnapshotAfterTakingSnapshot = BackupSnapshotAfterTakingSnapshot.value();
Review comment:
I think a null check would be good here
##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -262,142 +271,93 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status");
}
- Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
- boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
-
- if (deletedOnPrimary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId));
- } else {
- s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId));
- }
- if (null != deletedOnSecondary && deletedOnSecondary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId));
- }
- return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary;
+ return destroySnapshotEntriesAndFiles(snapshotVO);
}
- private boolean deleteOnPrimaryIfNeeded(Long snapshotId) {
- SnapshotVO snapshotVO;
- boolean deletedOnPrimary = false;
- snapshotVO = snapshotDao.findById(snapshotId);
- SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) {
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- } else {
- // Here we handle snapshots which are to be deleted but are not marked as destroyed yet.
- // This may occur for instance when they are created only on primary because
- // snapshot.backup.to.secondary was set to false.
- if (snapshotOnPrimaryInfo == null) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot (id: %d) not found on primary storage, skipping snapshot deletion on primary and marking it destroyed", snapshotId));
- }
- snapshotVO.setState(Snapshot.State.Destroyed);
- snapshotDao.update(snapshotId, snapshotVO);
- deletedOnPrimary = true;
- } else {
- SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
- try {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- if (!deletedOnPrimary) {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } else {
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
- deletedOnPrimary = false;
- }
- }
+ /**
+ * Destroys the snapshot entries and files on both primary and secondary storage (if it exists).
+ * @return true if destroy successfully, else false.
+ */
+ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo) {
+ if (!deleteSnapshotInfos(snapshotVo)) {
+ return false;
}
- return deletedOnPrimary;
- }
- private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) {
- Boolean deletedOnSecondary = null;
- SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
- if (snapshotOnImage == null) {
- s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId));
- } else {
- SnapshotObject obj = (SnapshotObject)snapshotOnImage;
- try {
- deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj);
- if (!deletedOnSecondary) {
- s_logger.debug(
- String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.",
- snapshotId));
- } else {
- s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId));
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
-// deletedOnSecondary remain null
- }
- }
- return deletedOnSecondary;
+ updateSnapshotToDestroyed(snapshotVo);
+
+ return true;
}
/**
- * Deletes the snapshot on secondary storage.
- * It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false.
+ * Updates the snapshot to {@link Snapshot.State#Destroyed}.
*/
- private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- List<VolumeDetailVO> volumesFromSnapshot;
- volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);
+ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
+ snapshotVo.setState(Snapshot.State.Destroyed);
+ snapshotDao.update(snapshotVo.getId(), snapshotVo);
+ }
- if (volumesFromSnapshot.size() > 0) {
- try {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " + e1.toString());
+ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo) {
+ Map<String, SnapshotInfo> snapshotInfos = retrieveSnapshotEntries(snapshotVo.getId());
+
+ for (var infoEntry : snapshotInfos.entrySet()) {
+ if (!deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(), snapshotVo)) {
+ return false;
}
- throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
}
- boolean result = deleteSnapshotChain(snapshotOnImage);
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- return result;
+ return true;
}
/**
- * Deletes the snapshot on primary storage. It returns true when the snapshot was not found on primary storage; </br>
- * In case of failure while deleting the snapshot, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException. </br>
+ * Destroys the snapshot entry and file.
+ * @return true if destroy successfully, else false.
*/
- private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (isSnapshotOnPrimaryStorage(snapshotId)) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is found on primary storage for snapshot id: %d, performing snapshot deletion on primary", snapshotId));
- }
- if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
- snapshotOnPrimary.setState(State.Destroyed);
- snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Successfully deleted snapshot id: %d on primary storage", snapshotId));
- }
+ protected boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage, SnapshotVO snapshotVo) {
+ if (snapshotInfo == null) {
+ s_logger.debug(String.format("Could not find %s entry on a %s. Skipping deletion on %s.", snapshotVo, storage, storage));
+ return true;
+ }
+
+ DataStore dataStore = snapshotInfo.getDataStore();
+ storage = String.format("%s {uuid: \"%s\", name: \"%s\"}", storage, dataStore.getUuid(), dataStore.getName());
+
+ try {
+ SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);
+ snapshotObject.processEvent(Snapshot.Event.DestroyRequested);
+
+ if (deleteSnapshotChain(snapshotInfo, storage)) {
+ snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+ s_logger.debug(String.format("%s was deleted on %s.", snapshotVo, storage));
return true;
}
- } else {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is not found on primary storage for snapshot id: %d, skipping snapshot deletion on primary", snapshotId));
- }
- return true;
+
+ snapshotObject.processEvent(Snapshot.Event.OperationFailed);
+ s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storage));
+ } catch (NoTransitionException ex) {
+ s_logger.warn(String.format("Failed to delete %s on %s due to %s.", snapshotVo, storage, ex.getMessage()), ex);
}
+
return false;
}
/**
- * Returns true if the snapshot volume is on primary storage.
+ * Cast SnapshotInfo to SnapshotObject.
+ * @return SnapshotInfo cast to SnapshotObject.
*/
- private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotOnPrimary != null) {
- long volumeId = snapshotOnPrimary.getVolumeId();
- VolumeVO volumeVO = volumeDao.findById(volumeId);
- return volumeVO != null && volumeVO.getRemoved() == null;
- }
- return false;
+ protected SnapshotObject castSnapshotInfoToSnapshotObject(SnapshotInfo snapshotInfo) {
+ return (SnapshotObject) snapshotInfo;
+ }
+
+ /**
+ * Retrieves the snapshot infos on primary and secondary storage.
+ * @param snapshotId The snapshot to retrieve the infos.
+ * @return A map of snapshot infos.
+ */
+ protected Map<String, SnapshotInfo> retrieveSnapshotEntries(long snapshotId) {
+ Map<String, SnapshotInfo> snapshotInfos = new LinkedHashMap<>();
+ snapshotInfos.put("secondary storage", snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image, false));
Review comment:
Can we make them constant values?
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-904386637
@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] DaanHoogland commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-957230976
@GutoVeronezi we are releasing 4.16 now and this is waiting to be merged after that as far as I can tell
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-927588692
@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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1051279758
@nvazquez 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] DaanHoogland commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1085708789
@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] rohityadavcloud commented on a change in pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r818440432
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1500,8 +1480,44 @@ public Answer createVolume(final CreateObjectCommand cmd) {
}
}
- protected static final MessageFormat SnapshotXML = new MessageFormat(" <domainsnapshot>" + " <name>{0}</name>" + " <domain>"
- + " <uuid>{1}</uuid>" + " </domain>" + " </domainsnapshot>");
+ /**
+ * XML to take disk-only snapshot of the VM.<br><br>
+ * 1st parameter: snapshot's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: absolute path to create the snapshot;<br>
+ * 4th parameter: list of disks to avoid on snapshot {@link #TAG_AVOID_DISK_FROM_SNAPSHOT};
+ */
+ private static final String XML_CREATE_SNAPSHOT = "<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ + "</domainsnapshot>";
+
+ /**
+ * Tag to avoid disk from snapshot.<br><br>
+ * 1st parameter: disk's label (target.dev tag from VM's XML);
+ */
+ private static final String TAG_AVOID_DISK_FROM_SNAPSHOT = "<disk name='%s' snapshot='no' />";
+
+ /**
+ * Virsh command to merge (blockcommit) snapshot into the base file.<br><br>
+ * 1st parameter: VM's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: the absolute path of the base file;
+ * 4th parameter: the flag '--delete', if Libvirt supports it. Libvirt started to support it on version <b>6.0.0</b>;
+ */
+ private static final String COMMAND_MERGE_SNAPSHOT = "virsh blockcommit %s %s --base %s --active --wait %s --pivot";
Review comment:
Instead of this, I'm assuming this shells out as a child process, can we not use libvirt-java? If the bindings are not available add them to upstream libvirt-java cc @wido @GabrielBrascher you may help with that (I see Wido has worked on libvirt-java before)
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-990973725
@slavkap @GutoVeronezi I am a bit worried that the tests aren't passing on KVM. They are without this patch and though I understand that for a lot of functionality qemu-kvm-ev is needed, we shouldn't break things for people using the old qemu distro/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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-974024367
@DaanHoogland 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] GutoVeronezi commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-976867820
@nvazquez it would not be possible without hacking and leaving bad code into the code base. Therefore, I would rather not follow this path.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r782155384
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1585,17 +1607,214 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
s_logger.debug("Failed to manage snapshot: " + result);
return new CreateObjectAnswer("Failed to manage snapshot: " + result);
}
+ } else {
+ snapshotPath = getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);
+ String copyResult = copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
+ validateCopyResult(copyResult, snapshotPath);
}
}
final SnapshotObjectTO newSnapshot = new SnapshotObjectTO();
- // NOTE: sort of hack, we'd better just put snapshtoName
- newSnapshot.setPath(disk.getPath() + File.separator + snapshotName);
+
+ newSnapshot.setPath(snapshotPath);
return new CreateObjectAnswer(newSnapshot);
- } catch (final LibvirtException e) {
- s_logger.debug("Failed to manage snapshot: ", e);
- return new CreateObjectAnswer("Failed to manage snapshot: " + e.toString());
+ } catch (CloudRuntimeException | LibvirtException | IOException ex) {
+ String errorMsg = String.format("Failed take snapshot for volume [%s], in VM [%s], due to [%s].", volume, vmName, ex.getMessage());
+ s_logger.error(errorMsg, ex);
+ return new CreateObjectAnswer(errorMsg);
+ }
+ }
+
+ protected void validateCopyResult(String copyResult, String snapshotPath) throws CloudRuntimeException, IOException {
+ if (copyResult == null) {
+ return;
+ }
+
+ Files.deleteIfExists(Paths.get(snapshotPath));
+ throw new CloudRuntimeException(copyResult);
+ }
+
+ /**
+ * Merges the snapshot into base file to keep volume and VM behavior after stopping - starting.
+ * @param vm Domain of the VM;
+ * @param diskLabel Disk label to manage snapshot and base file;
+ * @param baseFilePath Path of the base file;
+ * @param snapshotName Name of the snapshot;
+ * @throws LibvirtException
+ */
+ protected void mergeSnapshotIntoBaseFile(Domain vm, String diskLabel, String baseFilePath, String snapshotName, VolumeObjectTO volume,
+ Connect conn) throws LibvirtException {
+ boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit = LibvirtUtilitiesHelper.isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit(conn);
+ String vmName = vm.getName();
+ String mergeCommand = String.format(COMMAND_MERGE_SNAPSHOT, vmName, diskLabel, baseFilePath, isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit ? "--delete" : "");
+ String mergeResult = Script.runSimpleBashScript(mergeCommand);
+
+ if (mergeResult == null) {
+ s_logger.debug(String.format("Successfully merged snapshot [%s] into VM [%s] %s base file.", snapshotName, vmName, volume));
+ manuallyDeleteUnusedSnapshotFile(isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, getSnapshotTemporaryPath(baseFilePath, snapshotName));
+ return;
+ }
+
+ String errorMsg = String.format("Failed to merge snapshot [%s] into VM [%s] %s base file. Command [%s] resulted in [%s]. If the VM is stopped and then started, it"
+ + " will start to write in the base file again. All changes made between the snapshot and the VM stop will be in the snapshot. If the VM is stopped, the snapshot must be"
+ + " merged into the base file manually.", snapshotName, vmName, volume, mergeCommand, mergeResult);
+
+ s_logger.warn(String.format("%s VM XML: [%s].", errorMsg, vm.getXMLDesc(0)));
+ throw new CloudRuntimeException(errorMsg);
+ }
+
+ /**
+ * Manually deletes the unused snapshot file.<br/>
+ * This method is necessary due to Libvirt created the tag '--delete' on command 'virsh blockcommit' on version <b>1.2.9</b>, however it was only implemented on version
+ * <b>6.0.0</b>.
+ * @param snapshotPath The unused snapshot file to manually delete.
+ */
+ protected boolean manuallyDeleteUnusedSnapshotFile(boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, String snapshotPath) {
Review comment:
maybe it shouldn't be `boolean`
```suggestion
protected void manuallyDeleteUnusedSnapshotFile(boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, String snapshotPath) {
```
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r782152750
##########
File path: engine/storage/src/main/java/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java
##########
@@ -520,4 +531,22 @@ private boolean isSnapshotChainingRequired(long volumeId) {
return false;
}
+ @Override
+ public boolean expungeReferenceBySnapshotIdAndDataStoreRole(long snapshotId, DataStoreRole dataStoreRole) {
+ SnapshotDataStoreVO snapshotDataStoreVo = findOneBy(createSearchCriteriaBySnapshotIdAndStoreRole(snapshotId, dataStoreRole));
+
+ if (snapshotDataStoreVo == null) {
+ return true;
+ }
+
+ return expunge(snapshotDataStoreVo.getId());
+ }
+
+ @Override
+ public List<SnapshotDataStoreVO> listReadyByVolumeId(long volumeId) {
Review comment:
```suggestion
public List<SnapshotDataStoreVO> listReadyByVolumeId(long volumeId, DataStoreRole role) {
```
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1031509928
Hi, @nvazquez, it has not been discussed on ML yet.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1051705415
<b>Trillian test result (tid-3443)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32683 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t3443-kvm-centos7.zip
Smoke tests completed. 90 look OK, 2 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_snapshot_usage | `Error` | 2.14 | test_usage.py
test_01_snapshot_root_disk | `Error` | 2.13 | test_snapshots.py
--
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] weizhouapache commented on a change in pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r818451112
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1500,8 +1480,44 @@ public Answer createVolume(final CreateObjectCommand cmd) {
}
}
- protected static final MessageFormat SnapshotXML = new MessageFormat(" <domainsnapshot>" + " <name>{0}</name>" + " <domain>"
- + " <uuid>{1}</uuid>" + " </domain>" + " </domainsnapshot>");
+ /**
+ * XML to take disk-only snapshot of the VM.<br><br>
+ * 1st parameter: snapshot's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: absolute path to create the snapshot;<br>
+ * 4th parameter: list of disks to avoid on snapshot {@link #TAG_AVOID_DISK_FROM_SNAPSHOT};
+ */
+ private static final String XML_CREATE_SNAPSHOT = "<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ + "</domainsnapshot>";
+
+ /**
+ * Tag to avoid disk from snapshot.<br><br>
+ * 1st parameter: disk's label (target.dev tag from VM's XML);
+ */
+ private static final String TAG_AVOID_DISK_FROM_SNAPSHOT = "<disk name='%s' snapshot='no' />";
+
+ /**
+ * Virsh command to merge (blockcommit) snapshot into the base file.<br><br>
+ * 1st parameter: VM's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: the absolute path of the base file;
+ * 4th parameter: the flag '--delete', if Libvirt supports it. Libvirt started to support it on version <b>6.0.0</b>;
+ */
+ private static final String COMMAND_MERGE_SNAPSHOT = "virsh blockcommit %s %s --base %s --active --wait %s --pivot";
Review comment:
@rohityadavcloud
yes, Ideally libvirt-java is used here.
But if there is no corresponding method in libvirt-java, this is also acceptable I think, we can improve it afterwards, so this PR will not be blocked by a third party project (libvirt-java)
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1067827808
Hi @GutoVeronezi please advise when the new changes are applied, thanks
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-924273437
<b>Trillian test result (tid-2144)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35859 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2144-kvm-centos7.zip
Smoke tests completed. 87 look OK, 2 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_snapshot_usage | `Error` | 2.13 | test_usage.py
test_01_snapshot_root_disk | `Error` | 2.13 | test_snapshots.py
--
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 edited a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442
@rhtyd
The errors are related to the CentOS's QEMU binary:
`Operation not supported: live disk snapshot not supported with this QEMU binary`
RH removed/blocked a lot of features in CentOS, like VM migration, memory hotplug and so on. Therefore, default CentOS's QEMU binary does not support them. An alternative is to use `qemu-kvm-ev`, which supports more features than the default one.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-975325180
@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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-974079190
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_multiplication_x: debian :heavy_check_mark: suse15. SL-JID 1747
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-990835274
Hi all, I've done some tests on this PR on CentOS7 with qemu version qemu-kvm-ev-2.12.0-44.1.el7_8.1, and all have passed. I'm not using the stock qemu, but as @GutoVeronezi mentioned, many CS functionality does not work with it.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-992142705
@DaanHoogland, I agree with you, but I also agree with @GutoVeronezi about leaving bad code if we want to support both. Maybe opening a discussion will help.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985318079
Close-reopen to rekick travis.
@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] GutoVeronezi commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442
@rhtyd
The errors are related to the CentOS's QEMU binary:
`Operation not supported: live disk snapshot not supported with this QEMU binary`
RH removed/blocked a lot of features in CentOS, like VM migration, memory hotplug and so on. Therefore, default CentOS's QEMU binary does not support them. An alternative is to use `qemu-kvm-ev-2.12.0-44.1.el7_8.1`, which support more features than the default one.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-924370497
<b>Trillian test result (tid-2145)</b>
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43316 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2145-vmware-65u2.zip
Smoke tests completed. 89 look OK, 0 have errors
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] sureshanaparti commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-927586801
@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] GutoVeronezi commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1054190355
@nvazquez CI errors are related to the QEMU binary (see https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442).
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1058376069
<b>Trillian test result (tid-3465)</b>
Environment: kvm-ubuntu20 (x2), Advanced Networking with Mgmt server u20
Total time taken: 34699 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t3465-kvm-ubuntu20.zip
Smoke tests completed. 91 look OK, 1 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 80.36 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 49.93 | test_vm_life_cycle.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1086508348
@nvazquez 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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-926595769
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1389
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-926568854
@GutoVeronezi can you review failures in KVM and XenServer.
@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] GutoVeronezi commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-899537390
The feature `blockcommit` was introduced on Libvirt's version `1.2.9`, according to its docs. However some tags were introduced as a future work mark. The tag `delete` was really implemented only in version `6.0.0`, through the commit [qemu: block: enable the snapshot image deletion feature](https://gitlab.com/libvirt/libvirt/-/commit/7d484ede208f33817d3ed033f6bf27522fff5fb5).
Therefore, for cases which operators cannot upgrade Libvirt (like in CentOS 7) to a version that supports this flag, we created a workaround, manually deleting the unused snapshot file. As soon as we have everybody using a version equal or higher than `6.0.0`, we can remove this workaround.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-896469436
Thanks @GutoVeronezi I'll post my review this week
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-923743477
@rhtyd a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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] rhtyd commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-923687996
@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] DaanHoogland commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-972870007
@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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-975808378
<b>Trillian test result (tid-2570)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31239 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2570-kvm-centos7.zip
Smoke tests completed. 89 look OK, 2 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_snapshot_usage | `Error` | 2.21 | test_usage.py
test_01_snapshot_root_disk | `Error` | 2.13 | test_snapshots.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985318427
@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] GutoVeronezi commented on a change in pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r687005194
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1497,8 +1475,43 @@ public Answer createVolume(final CreateObjectCommand cmd) {
}
}
- protected static final MessageFormat SnapshotXML = new MessageFormat(" <domainsnapshot>" + " <name>{0}</name>" + " <domain>"
- + " <uuid>{1}</uuid>" + " </domain>" + " </domainsnapshot>");
+ /**
+ * XML to take disk-only snapshot of the VM.<br><br>
+ * 1st parameter: snapshot's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: absolute path to create the snapshot;<br>
+ * 4th parameter: list of disks to avoid on snapshot {@link #TAG_AVOID_DISK_FROM_SNAPSHOT};
+ */
+ private static final String XML_CREATE_SNAPSHOT = "<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ + "</domainsnapshot>";
+
+ /**
+ * Tag to avoid disk from snapshot.<br><br>
+ * 1st parameter: disk's label (target.dev tag from VM's XML);
+ */
+ private static final String TAG_AVOID_DISK_FROM_SNAPSHOT = "<disk name='%s' snapshot='no' />";
+
+ /**
+ * Virsh command to merge (blockcommit) snapshot into the base file.<br><br>
+ * 1st parameter: VM's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: the absolute path of the base file;
+ */
+ private static final String COMMAND_MERGE_SNAPSHOT = "virsh blockcommit %s %s --base %s --active --wait --delete --pivot";
Review comment:
Thanks @slavkap, I will make some changes soon.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r697235222
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
##########
@@ -42,37 +49,41 @@
import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager;
import com.cloud.resource.CommandWrapper;
import com.cloud.resource.ResourceWrapper;
+import com.cloud.storage.DataStoreRole;
import com.cloud.storage.Storage.StoragePoolType;
+import com.cloud.utils.Pair;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.utils.script.Script;
@ResourceWrapper(handles = RevertSnapshotCommand.class)
-public final class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper<RevertSnapshotCommand, Answer, LibvirtComputingResource> {
+public class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper<RevertSnapshotCommand, Answer, LibvirtComputingResource> {
private static final Logger s_logger = Logger.getLogger(LibvirtRevertSnapshotCommandWrapper.class);
private static final String MON_HOST = "mon_host";
private static final String KEY = "key";
private static final String CLIENT_MOUNT_TIMEOUT = "client_mount_timeout";
private static final String RADOS_CONNECTION_TIMEOUT = "30";
+ protected Set<StoragePoolType> storagePoolTypesThatSupportRevertSnapshot = new HashSet<>(Arrays.asList(StoragePoolType.RBD, StoragePoolType.Filesystem,
+ StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint));
+
@Override
public Answer execute(final RevertSnapshotCommand command, final LibvirtComputingResource libvirtComputingResource) {
+ SnapshotObjectTO snapshotOnPrimaryStorage = command.getDataOnPrimaryStorage();
SnapshotObjectTO snapshot = command.getData();
VolumeObjectTO volume = snapshot.getVolume();
PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO)volume.getDataStore();
DataStoreTO snapshotImageStore = snapshot.getDataStore();
- if (!(snapshotImageStore instanceof NfsTO) && primaryStore.getPoolType() != StoragePoolType.RBD) {
+ if (!(snapshotImageStore instanceof NfsTO) && !storagePoolTypesThatSupportRevertSnapshot.contains(primaryStore.getPoolType())) {
Review comment:
btw i realise the old code used it as well, but i have the impression it will be used more intensively now.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r686963157
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1497,8 +1475,43 @@ public Answer createVolume(final CreateObjectCommand cmd) {
}
}
- protected static final MessageFormat SnapshotXML = new MessageFormat(" <domainsnapshot>" + " <name>{0}</name>" + " <domain>"
- + " <uuid>{1}</uuid>" + " </domain>" + " </domainsnapshot>");
+ /**
+ * XML to take disk-only snapshot of the VM.<br><br>
+ * 1st parameter: snapshot's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: absolute path to create the snapshot;<br>
+ * 4th parameter: list of disks to avoid on snapshot {@link #TAG_AVOID_DISK_FROM_SNAPSHOT};
+ */
+ private static final String XML_CREATE_SNAPSHOT = "<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ + "</domainsnapshot>";
+
+ /**
+ * Tag to avoid disk from snapshot.<br><br>
+ * 1st parameter: disk's label (target.dev tag from VM's XML);
+ */
+ private static final String TAG_AVOID_DISK_FROM_SNAPSHOT = "<disk name='%s' snapshot='no' />";
+
+ /**
+ * Virsh command to merge (blockcommit) snapshot into the base file.<br><br>
+ * 1st parameter: VM's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: the absolute path of the base file;
+ */
+ private static final String COMMAND_MERGE_SNAPSHOT = "virsh blockcommit %s %s --base %s --active --wait --delete --pivot";
Review comment:
I spoke offline with @GutoVeronezi, and the flag `--delete` does not work with the version of libvirt that I have
```
Using library: libvirt 5.0.0
Using API: QEMU 5.0.0
Running hypervisor: QEMU 2.12.0
```
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-976492325
> Trillian test result (tid-2570) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 31239 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2570-kvm-centos7.zip Smoke tests completed. 89 look OK, 2 have errors Only failed tests results shown below:
>
> Test Result Time (s) Test File
> test_01_snapshot_usage `Error` 2.21 test_usage.py
> test_01_snapshot_root_disk `Error` 2.13 test_snapshots.py
@DaanHoogland tests errors are related to QEMU binary on CentOS, as explained in https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-923721804
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1338
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r697225748
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
##########
@@ -42,37 +49,41 @@
import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager;
import com.cloud.resource.CommandWrapper;
import com.cloud.resource.ResourceWrapper;
+import com.cloud.storage.DataStoreRole;
import com.cloud.storage.Storage.StoragePoolType;
+import com.cloud.utils.Pair;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.utils.script.Script;
@ResourceWrapper(handles = RevertSnapshotCommand.class)
-public final class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper<RevertSnapshotCommand, Answer, LibvirtComputingResource> {
+public class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper<RevertSnapshotCommand, Answer, LibvirtComputingResource> {
private static final Logger s_logger = Logger.getLogger(LibvirtRevertSnapshotCommandWrapper.class);
private static final String MON_HOST = "mon_host";
private static final String KEY = "key";
private static final String CLIENT_MOUNT_TIMEOUT = "client_mount_timeout";
private static final String RADOS_CONNECTION_TIMEOUT = "30";
+ protected Set<StoragePoolType> storagePoolTypesThatSupportRevertSnapshot = new HashSet<>(Arrays.asList(StoragePoolType.RBD, StoragePoolType.Filesystem,
+ StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint));
+
@Override
public Answer execute(final RevertSnapshotCommand command, final LibvirtComputingResource libvirtComputingResource) {
+ SnapshotObjectTO snapshotOnPrimaryStorage = command.getDataOnPrimaryStorage();
SnapshotObjectTO snapshot = command.getData();
VolumeObjectTO volume = snapshot.getVolume();
PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO)volume.getDataStore();
DataStoreTO snapshotImageStore = snapshot.getDataStore();
- if (!(snapshotImageStore instanceof NfsTO) && primaryStore.getPoolType() != StoragePoolType.RBD) {
+ if (!(snapshotImageStore instanceof NfsTO) && !storagePoolTypesThatSupportRevertSnapshot.contains(primaryStore.getPoolType())) {
Review comment:
can primaryStore be `null` here?
##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -262,142 +274,93 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status");
}
- Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
- boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
-
- if (deletedOnPrimary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId));
- } else {
- s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId));
- }
- if (null != deletedOnSecondary && deletedOnSecondary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId));
- }
- return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary;
+ return destroySnapshotEntriesAndFiles(snapshotVO);
}
- private boolean deleteOnPrimaryIfNeeded(Long snapshotId) {
- SnapshotVO snapshotVO;
- boolean deletedOnPrimary = false;
- snapshotVO = snapshotDao.findById(snapshotId);
- SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) {
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- } else {
- // Here we handle snapshots which are to be deleted but are not marked as destroyed yet.
- // This may occur for instance when they are created only on primary because
- // snapshot.backup.to.secondary was set to false.
- if (snapshotOnPrimaryInfo == null) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot (id: %d) not found on primary storage, skipping snapshot deletion on primary and marking it destroyed", snapshotId));
- }
- snapshotVO.setState(Snapshot.State.Destroyed);
- snapshotDao.update(snapshotId, snapshotVO);
- deletedOnPrimary = true;
- } else {
- SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
- try {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
- if (!deletedOnPrimary) {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } else {
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
- deletedOnPrimary = false;
- }
- }
+ /**
+ * Destroys the snapshot entries and files on both primary and secondary storage (if it exists).
+ * @return true if destroy successfully, else false.
+ */
+ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo) {
+ if (!deleteSnapshotInfos(snapshotVo)) {
+ return false;
}
- return deletedOnPrimary;
- }
- private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) {
- Boolean deletedOnSecondary = null;
- SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
- if (snapshotOnImage == null) {
- s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId));
- } else {
- SnapshotObject obj = (SnapshotObject)snapshotOnImage;
- try {
- deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj);
- if (!deletedOnSecondary) {
- s_logger.debug(
- String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.",
- snapshotId));
- } else {
- s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId));
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
-// deletedOnSecondary remain null
- }
- }
- return deletedOnSecondary;
+ updateSnapshotToDestroyed(snapshotVo);
+
+ return true;
}
/**
- * Deletes the snapshot on secondary storage.
- * It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false.
+ * Updates the snapshot to {@link Snapshot.State#Destroyed}.
*/
- private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- List<VolumeDetailVO> volumesFromSnapshot;
- volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);
+ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
+ snapshotVo.setState(Snapshot.State.Destroyed);
+ snapshotDao.update(snapshotVo.getId(), snapshotVo);
+ }
- if (volumesFromSnapshot.size() > 0) {
- try {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " + e1.toString());
+ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo) {
+ Map<String, SnapshotInfo> snapshotInfos = retrieveSnapshotEntries(snapshotVo.getId());
+
+ for (var infoEntry : snapshotInfos.entrySet()) {
+ if (!deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(), snapshotVo)) {
+ return false;
}
- throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
}
- boolean result = deleteSnapshotChain(snapshotOnImage);
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- return result;
+ return true;
}
/**
- * Deletes the snapshot on primary storage. It returns true when the snapshot was not found on primary storage; </br>
- * In case of failure while deleting the snapshot, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException. </br>
+ * Destroys the snapshot entry and file.
+ * @return true if destroy successfully, else false.
*/
- private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (isSnapshotOnPrimaryStorage(snapshotId)) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is found on primary storage for snapshot id: %d, performing snapshot deletion on primary", snapshotId));
- }
- if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
- snapshotOnPrimary.setState(State.Destroyed);
- snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Successfully deleted snapshot id: %d on primary storage", snapshotId));
- }
+ protected boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage, SnapshotVO snapshotVo) {
+ if (snapshotInfo == null) {
+ s_logger.debug(String.format("Could not find %s entry on a %s. Skipping deletion on %s.", snapshotVo, storage, storage));
+ return true;
+ }
+
+ DataStore dataStore = snapshotInfo.getDataStore();
+ storage = String.format("%s {uuid: \"%s\", name: \"%s\"}", storage, dataStore.getUuid(), dataStore.getName());
+
+ try {
+ SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);
+ snapshotObject.processEvent(Snapshot.Event.DestroyRequested);
+
+ if (deleteSnapshotChain(snapshotInfo, storage)) {
+ snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+ s_logger.debug(String.format("%s was deleted on %s.", snapshotVo, storage));
return true;
}
- } else {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is not found on primary storage for snapshot id: %d, skipping snapshot deletion on primary", snapshotId));
- }
- return true;
+
+ snapshotObject.processEvent(Snapshot.Event.OperationFailed);
+ s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storage));
+ } catch (NoTransitionException ex) {
+ s_logger.warn(String.format("Failed to delete %s on %s due to %s.", snapshotVo, storage, ex.getMessage()), ex);
}
+
return false;
}
/**
- * Returns true if the snapshot volume is on primary storage.
+ * Cast SnapshotInfo to SnapshotObject.
+ * @return SnapshotInfo cast to SnapshotObject.
*/
- private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
- SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotOnPrimary != null) {
- long volumeId = snapshotOnPrimary.getVolumeId();
- VolumeVO volumeVO = volumeDao.findById(volumeId);
- return volumeVO != null && volumeVO.getRemoved() == null;
- }
- return false;
+ protected SnapshotObject castSnapshotInfoToSnapshotObject(SnapshotInfo snapshotInfo) {
+ return (SnapshotObject) snapshotInfo;
+ }
Review comment:
Why is this needed? It only casts.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1030662698
Hi @GutoVeronezi I noticed mentions of discussion on the mailing list, has this been discussed? In case it has been discussed, is it ready for review/testing?
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1051189346
@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] nvazquez commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1086507713
@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] GutoVeronezi commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-986808845
> > Trillian test result (tid-2610) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 32228 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2610-kvm-centos7.zip Smoke tests completed. 89 look OK, 2 have errors Only failed tests results shown below:
> > Test Result Time (s) Test File
> > test_01_snapshot_usage `Error` 1.12 test_usage.py
> > test_01_snapshot_root_disk `Error` 2.15 test_snapshots.py
>
> @GutoVeronezi can you check these errors in the failed tests.
@sureshanaparti the tests errors are related to QEMU binary in CentOS, as explained in https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442.
--
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 pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-993625709
@DaanHoogland as I explained in https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442, the errors are related to the CentOS's QEMU binary:
`Operation not supported: live disk snapshot not supported with this QEMU binary`
RedHat removed/blocked a lot of features in CentOS, such as VM migrations, memory hotplug, and so on. Therefore, default CentOS's QEMU binary does not support them. The tests are not passing due to QEMU binary in CentOS 7 used by the CI.
If the QEMU binary in CentOS 7 used by the CI were upgraded to the alternative (`**qemu-kvm-ev**`, which supports more features than the default one) or if it used another distro, like Ubuntu, the tests would pass. Therefore, the problem is not directly linked with the patch itself.
Besides that, in my point of view, such limitations in the default QEMU binary (in CentOS) make it unfeasible to build a cloud with CentOS and the default QEMU binary. Probably, most people using KVM with CentOS are using the `ev` binaries (I might be wrong though, however, that seems to be the case when looking at the users' list). Therefore, I do not see why we would impose such limitation in the ACS feature set, to support an O.S. that is in its final years, and we have a limited supported set of features with it. Also, we just need to guide users to upgrade to **qemu-kvm-ev**, like we already did many other times in the mailing list (e.g. https://lists.apache.org/thread/vg9qz7w52yso5b5xcp4rhtmrodhvdmpf).
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r818600350
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1500,8 +1480,44 @@ public Answer createVolume(final CreateObjectCommand cmd) {
}
}
- protected static final MessageFormat SnapshotXML = new MessageFormat(" <domainsnapshot>" + " <name>{0}</name>" + " <domain>"
- + " <uuid>{1}</uuid>" + " </domain>" + " </domainsnapshot>");
+ /**
+ * XML to take disk-only snapshot of the VM.<br><br>
+ * 1st parameter: snapshot's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: absolute path to create the snapshot;<br>
+ * 4th parameter: list of disks to avoid on snapshot {@link #TAG_AVOID_DISK_FROM_SNAPSHOT};
+ */
+ private static final String XML_CREATE_SNAPSHOT = "<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ + "</domainsnapshot>";
+
+ /**
+ * Tag to avoid disk from snapshot.<br><br>
+ * 1st parameter: disk's label (target.dev tag from VM's XML);
+ */
+ private static final String TAG_AVOID_DISK_FROM_SNAPSHOT = "<disk name='%s' snapshot='no' />";
+
+ /**
+ * Virsh command to merge (blockcommit) snapshot into the base file.<br><br>
+ * 1st parameter: VM's name;<br>
+ * 2nd parameter: disk's label (target.dev tag from VM's XML);<br>
+ * 3rd parameter: the absolute path of the base file;
+ * 4th parameter: the flag '--delete', if Libvirt supports it. Libvirt started to support it on version <b>6.0.0</b>;
+ */
+ private static final String COMMAND_MERGE_SNAPSHOT = "virsh blockcommit %s %s --base %s --active --wait %s --pivot";
Review comment:
@rohityadavcloud @weizhouapache, there is no corresponding method to `blockcommit` in libvirt-java, that's why I had to make it via command line.
--
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] rohityadavcloud commented on a change in pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r818438657
##########
File path: scripts/storage/qcow2/managesnapshot.sh
##########
@@ -226,34 +226,10 @@ backup_snapshot() {
return 2
fi
elif [ -f ${disk} ]; then
- # Does the snapshot exist?
- qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk 2>&1)
- ret_code=$?
- if [ $ret_code -gt 0 ] && [[ $qemuimg_ret == *"snapshot: invalid option -- 'U'"* ]]
- then
- forceShareFlag=""
- qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk)
- ret_code=$?
- fi
- if [ $ret_code -gt 0 ] || [[ ! $qemuimg_ret == *"$snapshotname"* ]]
- then
- printf "there is no $snapshotname on disk $disk\n" >&2
- return 1
- fi
- qemuimg_ret=$($qemu_img convert $forceShareFlag -f qcow2 -O qcow2 -l snapshot.name=$snapshotname $disk $destPath/$destName 2>&1 > /dev/null)
+ cp "$disk" "${destPath}/${destName}"
Review comment:
using qemu-img convert might be necessary, it's possible the source is raw disk etc, furthermore convert might compress the snapshot. cc @nvazquez @DaanHoogland @weizhouapache what do you think?
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-923688468
@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] GutoVeronezi commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1020482007
Thanks for the reviews, @slavkap.
Regarding to list all the snapshots directly by the snapshot role: the intention is to list snapshots that only exists in primary storage, therefore, I need to list all the roles and verify if there are no entries with the role `Image` (a.k.a 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] blueorangutan commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1057806422
@weizhouapache a Trillian-Jenkins test job (ubuntu20 mgmt + kvm-ubuntu20) 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] andrijapanicsb commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-996519146
VM migrations DO work on stock KVMqemu/libvirt - you are probably referring to a storage-live migrations? That, indeed, does not work. But rarely anyone uses this feature.
While I support the idea to improve things (I've actually read the whole design of this feature, great work by all means!) - I also don't like the fact that stock qemy binaries don't work.
I understand that improvements require moving forward, but I also understand that many production customers will not be happy to see things broken.
I would prefer you open a discussion fore this, on the ML, and let's see what the public have to say - I also support moving to qemu-kmv-ev - but many production users are using stock versions.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r782155384
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1585,17 +1607,214 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
s_logger.debug("Failed to manage snapshot: " + result);
return new CreateObjectAnswer("Failed to manage snapshot: " + result);
}
+ } else {
+ snapshotPath = getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);
+ String copyResult = copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
+ validateCopyResult(copyResult, snapshotPath);
}
}
final SnapshotObjectTO newSnapshot = new SnapshotObjectTO();
- // NOTE: sort of hack, we'd better just put snapshtoName
- newSnapshot.setPath(disk.getPath() + File.separator + snapshotName);
+
+ newSnapshot.setPath(snapshotPath);
return new CreateObjectAnswer(newSnapshot);
- } catch (final LibvirtException e) {
- s_logger.debug("Failed to manage snapshot: ", e);
- return new CreateObjectAnswer("Failed to manage snapshot: " + e.toString());
+ } catch (CloudRuntimeException | LibvirtException | IOException ex) {
+ String errorMsg = String.format("Failed take snapshot for volume [%s], in VM [%s], due to [%s].", volume, vmName, ex.getMessage());
+ s_logger.error(errorMsg, ex);
+ return new CreateObjectAnswer(errorMsg);
+ }
+ }
+
+ protected void validateCopyResult(String copyResult, String snapshotPath) throws CloudRuntimeException, IOException {
+ if (copyResult == null) {
+ return;
+ }
+
+ Files.deleteIfExists(Paths.get(snapshotPath));
+ throw new CloudRuntimeException(copyResult);
+ }
+
+ /**
+ * Merges the snapshot into base file to keep volume and VM behavior after stopping - starting.
+ * @param vm Domain of the VM;
+ * @param diskLabel Disk label to manage snapshot and base file;
+ * @param baseFilePath Path of the base file;
+ * @param snapshotName Name of the snapshot;
+ * @throws LibvirtException
+ */
+ protected void mergeSnapshotIntoBaseFile(Domain vm, String diskLabel, String baseFilePath, String snapshotName, VolumeObjectTO volume,
+ Connect conn) throws LibvirtException {
+ boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit = LibvirtUtilitiesHelper.isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit(conn);
+ String vmName = vm.getName();
+ String mergeCommand = String.format(COMMAND_MERGE_SNAPSHOT, vmName, diskLabel, baseFilePath, isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit ? "--delete" : "");
+ String mergeResult = Script.runSimpleBashScript(mergeCommand);
+
+ if (mergeResult == null) {
+ s_logger.debug(String.format("Successfully merged snapshot [%s] into VM [%s] %s base file.", snapshotName, vmName, volume));
+ manuallyDeleteUnusedSnapshotFile(isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, getSnapshotTemporaryPath(baseFilePath, snapshotName));
+ return;
+ }
+
+ String errorMsg = String.format("Failed to merge snapshot [%s] into VM [%s] %s base file. Command [%s] resulted in [%s]. If the VM is stopped and then started, it"
+ + " will start to write in the base file again. All changes made between the snapshot and the VM stop will be in the snapshot. If the VM is stopped, the snapshot must be"
+ + " merged into the base file manually.", snapshotName, vmName, volume, mergeCommand, mergeResult);
+
+ s_logger.warn(String.format("%s VM XML: [%s].", errorMsg, vm.getXMLDesc(0)));
+ throw new CloudRuntimeException(errorMsg);
+ }
+
+ /**
+ * Manually deletes the unused snapshot file.<br/>
+ * This method is necessary due to Libvirt created the tag '--delete' on command 'virsh blockcommit' on version <b>1.2.9</b>, however it was only implemented on version
+ * <b>6.0.0</b>.
+ * @param snapshotPath The unused snapshot file to manually delete.
+ */
+ protected boolean manuallyDeleteUnusedSnapshotFile(boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, String snapshotPath) {
Review comment:
maybe it doesn't have to be `boolean`
```suggestion
protected void manuallyDeleteUnusedSnapshotFile(boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, String snapshotPath) {
```
--
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 removed a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-972870007
@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] DaanHoogland commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-974022353
@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 removed a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-904386637
--
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 edited a comment on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-976492325
> Trillian test result (tid-2570) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 31239 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2570-kvm-centos7.zip Smoke tests completed. 89 look OK, 2 have errors Only failed tests results shown below:
>
> Test Result Time (s) Test File
> test_01_snapshot_usage `Error` 2.21 test_usage.py
> test_01_snapshot_root_disk `Error` 2.13 test_snapshots.py
@DaanHoogland tests errors are related to QEMU binary in CentOS, as explained in https://github.com/apache/cloudstack/pull/5297#issuecomment-940007442.
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1085709068
@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. 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] blueorangutan commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1085751249
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 3050
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1085850327
Thanks @GutoVeronezi ! Starting a new round of tests
@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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1086442846
<b>Trillian test result (tid-3791)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42926 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t3791-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-928087737
<b>Trillian test result (tid-2211)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35587 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2211-kvm-centos7.zip
Smoke tests completed. 87 look OK, 2 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_snapshot_usage | `Error` | 1.12 | test_usage.py
test_01_snapshot_root_disk | `Error` | 2.14 | test_snapshots.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-1054209611
Yes @GutoVeronezi we are working on updating it on Trillian for KVM 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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985781436
<b>Trillian test result (tid-2609)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32393 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2609-xenserver-71.zip
Smoke tests completed. 90 look OK, 1 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestListIdsParams>:teardown | `Error` | 1.11 | test_list_ids_parameter.py
--
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 #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-985816226
<b>Trillian test result (tid-2611)</b>
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36254 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2611-vmware-65u2.zip
Smoke tests completed. 91 look OK, 0 have errors
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] sureshanaparti commented on pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#issuecomment-986495339
> Trillian test result (tid-2610) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 32228 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5297-t2610-kvm-centos7.zip Smoke tests completed. 89 look OK, 2 have errors Only failed tests results shown below:
>
> Test Result Time (s) Test File
> test_01_snapshot_usage `Error` 1.12 test_usage.py
> test_01_snapshot_root_disk `Error` 2.15 test_snapshots.py
@GutoVeronezi can you check these errors in the failed 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