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 2022/03/04 11:19:11 UTC
[GitHub] [cloudstack] weizhouapache opened a new pull request #6057: server: mark volume snapshots as Destroyed in some cases when a volume in QCOW2 format
weizhouapache opened a new pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057
when delete a volume in QCOW2 format, if volume snapshot does not exist on primary and secondary storage, mark the snapshot as Destroyed.
This fixes #5433
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059216792
@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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059247327
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2765
--
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 merged pull request #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
nvazquez merged pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057
--
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 edited a comment on pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
nvazquez edited a comment on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059216194
@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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059773368
@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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1062040871
@weizhouapache 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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059773389
@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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059850603
<b>Trillian test result (tid-3504)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31741 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6057-t3504-kvm-centos7.zip
Smoke tests completed. 92 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] blueorangutan commented on pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1061841886
@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] slavkap commented on a change in pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r821571617
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
removeVolume(volume.getId());
}
return volume;
}
+ private void markVolumeSnapshotsAsDestroyed(VolumeVO volume) {
+ if (!Storage.ImageFormat.QCOW2.equals(volume.getFormat())) {
Review comment:
Yes, it's better to remove this check because if we don't have snapshots on primary and secondary storage, the snapshot has to be marked as destroyed. I will test 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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1061878573
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2795
--
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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1063972064
Merging based on approvals and test results
--
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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r823625159
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -468,6 +471,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
_snapshotStoreDao.remove(snapStoreVo.getId());
Review comment:
@slavkap
thanks for 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] slavkap commented on a change in pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r821846706
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
removeVolume(volume.getId());
}
return volume;
}
+ private void markVolumeSnapshotsAsDestroyed(VolumeVO volume) {
+ if (!Storage.ImageFormat.QCOW2.equals(volume.getFormat())) {
Review comment:
Thanks, @weizhouapache. I will test it and let you 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] slavkap commented on a change in pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r821527386
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
Review comment:
@weizhouapache, this is not applied for the ROOT volumes. If the `snapshot.backup.to.secondary` is disabled and you have snapshots on the ROOT volume when you expunge the VM the snapshots will be in a `BackedUp` state in the DB table `snapshots`, but with no records in the `snapshot_store_ref` table, it is removed.
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
removeVolume(volume.getId());
}
return volume;
}
+ private void markVolumeSnapshotsAsDestroyed(VolumeVO volume) {
+ if (!Storage.ImageFormat.QCOW2.equals(volume.getFormat())) {
Review comment:
If you have a VM with Ceph's volumes, for example, and `snapshot.backup.to.secondary` is disabled when you expunge the VM, all snapshots remain active in UI, but there are no records in the `snapshot_store_ref` table.
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -448,9 +448,9 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
volDao.remove(vo.getId());
}
- SnapshotDataStoreVO snapStoreVo = _snapshotStoreDao.findByVolume(vo.getId(), DataStoreRole.Primary);
+ List<SnapshotDataStoreVO> snapStoreVOs = _snapshotStoreDao.listAllByVolumeAndDataStore(vo.getId(), DataStoreRole.Primary);
- if (snapStoreVo != null) {
+ for (SnapshotDataStoreVO snapStoreVo : snapStoreVOs) {
Review comment:
It's only my opinion, but we should handle this in each storage plugin. Each storage has to decide whether it wants to keep the snapshots on the primary, like they handle deleting or keeping the snapshots when a volume is deleted.
--
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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1062478121
<b>Trillian test result (tid-3530)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28307 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6057-t3530-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059216194
@weizhouapache can you please target the PR to the main branch?
@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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1061840108
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r823628333
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -468,6 +471,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
_snapshotStoreDao.remove(snapStoreVo.getId());
Review comment:
@weizhouapache, to keep it as history, nothing more
--
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 #6057: server: mark volume snapshots as Destroyed in some cases when a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059073760
@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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059260804
@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] nvazquez commented on pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059260449
@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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1059757085
<b>Trillian test result (tid-3498)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32258 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6057-t3498-kvm-centos7.zip
Smoke tests completed. 91 look OK, 1 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_03_ssvm_internals | `Failure` | 14.28 | test_ssvm.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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r823624999
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -468,6 +471,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
_snapshotStoreDao.remove(snapStoreVo.getId());
Review comment:
@slavkap
is there any benifit ?
the records will be cleaned up by storage garbage collector.
--
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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r823634858
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -468,6 +471,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
_snapshotStoreDao.remove(snapStoreVo.getId());
Review comment:
@slavkap ah ok, thanks for reply.
the records will be finally cleaned up by storage garbage collector (by default run once every day). They will be kept for only 1 day.
The behaviour is not changed by this PR. It was introduced by 336df84f1787de962a67d0a34551f9027303040e and ccb68e13a74342d4798aae11fe5c57890ff6c0ae
--
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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r823518926
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -468,6 +471,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
_snapshotStoreDao.remove(snapStoreVo.getId());
Review comment:
Hi @weizhouapache, isn't it better to keep the info in the DB for the snapshots and update the State as `Destroyed`?
--
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 #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r821556212
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
removeVolume(volume.getId());
}
return volume;
}
+ private void markVolumeSnapshotsAsDestroyed(VolumeVO volume) {
+ if (!Storage.ImageFormat.QCOW2.equals(volume.getFormat())) {
Review comment:
@slavkap
I was not quite sure if the snapshots still work and removing this check will not lead to other unexpected issues.
If we can make sure it is not a problem to mark volume snapshot as destroyed if snapshot does not exist in primary and secondary storage, we can remove this check.
##########
File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -448,9 +448,9 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
volDao.remove(vo.getId());
}
- SnapshotDataStoreVO snapStoreVo = _snapshotStoreDao.findByVolume(vo.getId(), DataStoreRole.Primary);
+ List<SnapshotDataStoreVO> snapStoreVOs = _snapshotStoreDao.listAllByVolumeAndDataStore(vo.getId(), DataStoreRole.Primary);
- if (snapStoreVo != null) {
+ for (SnapshotDataStoreVO snapStoreVo : snapStoreVOs) {
Review comment:
@slavkap
this is a fix: all snapshots (not only the first snapshot) need to be considered.
the process is same as before, see line 457 to 469
```
if (storagePoolVO.isManaged()) {
DataStore primaryDataStore = dataStoreMgr.getPrimaryDataStore(storagePoolId);
Map<String, String> mapCapabilities = primaryDataStore.getDriver().getCapabilities();
String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString());
Boolean supportsStorageSystemSnapshots = new Boolean(value);
if (!supportsStorageSystemSnapshots) {
_snapshotStoreDao.remove(snapStoreVo.getId());
}
} else {
_snapshotStoreDao.remove(snapStoreVo.getId());
}
```
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
Review comment:
@slavkap
I just tested it. you are right. The snapshots of ROOT volumes are still BackedUp when vm/volumes are removed. I will fix 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] weizhouapache commented on a change in pull request #6057: server: mark volume snapshots as Destroyed in some cases when delete a volume in QCOW2 format
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r821622955
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
removeVolume(volume.getId());
}
return volume;
}
+ private void markVolumeSnapshotsAsDestroyed(VolumeVO volume) {
+ if (!Storage.ImageFormat.QCOW2.equals(volume.getFormat())) {
Review comment:
@slavkap
good.
I have removed this check, and fixed the issue in vm expunge/destroy.
--
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 #6057: server: mark volume snapshots as Destroyed if it does not exist on primary and secondary storage when delete a volume
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#issuecomment-1062038477
@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