You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by likitha <gi...@git.apache.org> on 2015/06/30 08:40:26 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
GitHub user likitha opened a pull request:
https://github.com/apache/cloudstack/pull/540
CLOUDSTACK-8415. SSVM shutdown during snapshot operation leaves behin…
…d partial unusable disks and VM snapshots in secondary storage.
Enhance storage garbage collector to delete corresponding partial disks created in secondary storage and VM snapshots while expunging 'ERROR' snapshot.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/likitha/cloudstack CLOUDSTACK-8415
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/540.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #540
----
commit da8f25e33ca41057aac26910fe6b667a41adcaa5
Author: Likitha Shetty <li...@citrix.com>
Date: 2015-04-07T13:24:40Z
CLOUDSTACK-8415. SSVM shutdown during snapshot operation leaves behind partial unusable disks and VM snapshots in secondary storage.
Enhance storage garbage collector to delete corresponding partial disks created in secondary storage and VM snapshots while expunging 'ERROR' snapshot.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/540#issuecomment-216185889
tag:vmware-pickup
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34767521
--- Diff: server/src/com/cloud/storage/StorageManagerImpl.java ---
@@ -1120,14 +1123,60 @@ public void cleanupStorage(boolean recurring) {
}
// remove snapshots in Error state
- List<SnapshotVO> snapshots = _snapshotDao.listAllByStatus(Snapshot.State.Error);
+ List<SnapshotVO> snapshots = _snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error);
for (SnapshotVO snapshotVO : snapshots) {
try {
List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotVO.getId());
+ boolean isVMware = snapshotVO.getHypervisorType().equals(HypervisorType.VMware);
+ boolean removeSnapshot = true;
for (SnapshotDataStoreVO ref : storeRefs) {
- _snapshotStoreDao.expunge(ref.getId());
+ // Cleanup corresponding items (if any) from secondary storage.
+ if (isVMware) {
--- End diff --
Good catch!
We would have to get the PR in and test it, without the IF, against Xen and KVM, at least. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #540: CLOUDSTACK-8415. SSVM shutdown during snapshot operat...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/540
@likitha is this PR still pertinent?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34767845
--- Diff: engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java ---
@@ -172,7 +175,11 @@ public DataObject create(DataObject obj, DataStore dataStore) {
if (snapshotDataStoreVO != null) {
ss.setParentSnapshotId(snapshotDataStoreVO.getSnapshotId());
}
- ss.setInstallPath(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId());
+ String snapshotInstallPath = TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId();
+ if (snapshot.getHypervisorType().equals(HypervisorType.VMware)) {
+ snapshotInstallPath = snapshotInstallPath.concat("/" + UUID.randomUUID().toString());
--- End diff --
I would also call contact twice: 1 for the "/"; and the 2nd time for the UUID.randomUUID().toString().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34767796
--- Diff: engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java ---
@@ -172,7 +175,11 @@ public DataObject create(DataObject obj, DataStore dataStore) {
if (snapshotDataStoreVO != null) {
ss.setParentSnapshotId(snapshotDataStoreVO.getSnapshotId());
}
- ss.setInstallPath(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId());
+ String snapshotInstallPath = TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId();
+ if (snapshot.getHypervisorType().equals(HypervisorType.VMware)) {
--- End diff --
It seems like it should be applied to any hypervisor.
Is that another one we can take in and fix?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34767146
--- Diff: server/src/com/cloud/storage/StorageManagerImpl.java ---
@@ -1120,14 +1123,60 @@ public void cleanupStorage(boolean recurring) {
}
// remove snapshots in Error state
- List<SnapshotVO> snapshots = _snapshotDao.listAllByStatus(Snapshot.State.Error);
+ List<SnapshotVO> snapshots = _snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error);
for (SnapshotVO snapshotVO : snapshots) {
try {
List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotVO.getId());
+ boolean isVMware = snapshotVO.getHypervisorType().equals(HypervisorType.VMware);
+ boolean removeSnapshot = true;
for (SnapshotDataStoreVO ref : storeRefs) {
- _snapshotStoreDao.expunge(ref.getId());
+ // Cleanup corresponding items (if any) from secondary storage.
+ if (isVMware) {
--- End diff --
only for vmware???
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/540#issuecomment-121854348
LGTM :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #540: CLOUDSTACK-8415. SSVM shutdown during snapshot operat...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/540
@jburwell Likitha has moved on, unlikely we'll hear from her or see further development activity
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/540#issuecomment-121883348
This seems to me to be a solution for vmware for a more generic problem. in the generic code conditions for hypervisortype == vmware are set while the problem is generic over hypervisor typos.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34767004
--- Diff: engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java ---
@@ -172,7 +175,11 @@ public DataObject create(DataObject obj, DataStore dataStore) {
if (snapshotDataStoreVO != null) {
ss.setParentSnapshotId(snapshotDataStoreVO.getSnapshotId());
}
- ss.setInstallPath(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId());
+ String snapshotInstallPath = TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId();
+ if (snapshot.getHypervisorType().equals(HypervisorType.VMware)) {
--- End diff --
Is this really only true for vmware? It seems that adding a uuid to the path is a rather universal action in the ssvm, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/540#issuecomment-129041806
@likitha Any updates?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34769880
--- Diff: engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java ---
@@ -172,7 +175,11 @@ public DataObject create(DataObject obj, DataStore dataStore) {
if (snapshotDataStoreVO != null) {
ss.setParentSnapshotId(snapshotDataStoreVO.getSnapshotId());
}
- ss.setInstallPath(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId());
+ String snapshotInstallPath = TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId();
+ if (snapshot.getHypervisorType().equals(HypervisorType.VMware)) {
+ snapshotInstallPath = snapshotInstallPath.concat("/" + UUID.randomUUID().toString());
--- End diff --
Yep... I edited. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/540#discussion_r34769780
--- Diff: engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java ---
@@ -172,7 +175,11 @@ public DataObject create(DataObject obj, DataStore dataStore) {
if (snapshotDataStoreVO != null) {
ss.setParentSnapshotId(snapshotDataStoreVO.getSnapshotId());
}
- ss.setInstallPath(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId());
+ String snapshotInstallPath = TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId();
+ if (snapshot.getHypervisorType().equals(HypervisorType.VMware)) {
+ snapshotInstallPath = snapshotInstallPath.concat("/" + UUID.randomUUID().toString());
--- End diff --
auto spell typo? contact == concat
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8415. SSVM shutdown during sna...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/540#issuecomment-129227064
Who wants to step in and finish this work? It seems the original author is not able to finish it. If no one steps in, we'll have to close the PR without merging it so please help :-).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---