CLOUDSTACK-8415 [VMware] SSVM shutdown during snapshot operation results in disks to be left behind #2090
Conversation
|
@DaanHoogland @wilderrodrigues Please review the changes. |
|
@borisstoyanov Can you please kick off VMware trillian tests |
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-705 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
the functionality is good but shouldn't be in here. It is vmware specific and we should move away from closely tighing storage to hypervisors.
| for (SnapshotVO snapshotVO : snapshots) { | ||
| try { | ||
| List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotVO.getId()); | ||
| boolean isVMware = snapshotVO.getHypervisorType().equals(HypervisorType.VMware); |
There was a problem hiding this comment.
please try to move this logic into the vmware plugin. this code should remain generic.
opinions @mike-tutkowski @swill @rhtyd ?
There was a problem hiding this comment.
While I believe your comment makes sense, @DaanHoogland, it appears this file has hypervisor-specific logic sprinkled all around it.
Perhaps it would be better to allow this here and for you to write up a bug noting that it would be ideal for this file to be hypervisor agnostic?
There was a problem hiding this comment.
@DaanHoogland @mike-tutkowski Partial disks are left behind in VMware env when SSVM is shutdown/destroyed. So, DeleteSnapshotsDirCommand for Image store and DeleteVMSnapshotCommand for Primary store (to remove worker VM snapshot) has been triggered in VMware env here to cleanup the partial disks created in secondary storage and VM snapshots in primary respectively. The VMware storage manager would cleanup the worker VM snapshot from DeleteVMSnapshotCommand. No VMware specific code here, except sending these commands for cleanup.
| } | ||
| } | ||
| if (removeSnapshot) { | ||
| _snapshotDao.expunge(snapshotVO.getId()); |
There was a problem hiding this comment.
this method is already to long.
please factor out the new part into a seperate method.
There was a problem hiding this comment.
@DaanHoogland moved to a private method removeSnapshot()
|
Trillian test result (tid-1075)
|
|
@DaanHoogland Can you check the changes. |
|
@DaanHoogland Can you check and confirm the changes. |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0 Failed tests:
Skipped tests: Passed test suits: |
|
ping @DaanHoogland |
…lts in disks to be left behind
|
ping @sureshanaparti can you fix the conflicts? |
|
@sureshanaparti sorry for totally missing this. Yes this looks like what I asked for. please rebase as @rhtyd asked so we can test and merge. |
|
@rhtyd @PaulAngus @andrijapanicsb is this still relevant? |
|
Hi, the PR got auto-closed due to no known source repository or some other Github-specific issue during the recent renaming of the default branch to |
11 participants
Types of changes
In VMware environment, SSVM shutdown during snapshot operation leaves behind partial, unusable disks in secondary storage and VM snapshots.
Fix: Enhance storage garbage collector to delete the partial disks created in secondary storage and VM snapshots while expunging 'ERROR' snapshot.
This closes #540