CLOUDSTACK-9538: FIX failure in Deleting Snapshot From Primary Storage RBD Storage if vm has been removed #1710
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e RBD Storage if vm has been removed
|
Hi, @ustcweizhou do we need to take any more action or inform some people from ACS to take this PR to upcoming releases? |
Contributor
Author
|
@jburwell Could you please review this PR ? Thanks, @ozhanrk wants to make it in 4.9.1. |
Contributor
|
@ustcweizhou @ozhanrk Reading the ticket and the patch, this behavior seems like a common request across storage implementations. Some users want snapshots to be automatically removed and others do not. Given the variability of use cases, I think choosing one approach over another would be inappropriate. It feels like we should step back and evaluate a global-setting with a VM-level override that specifies whether or not snapshots should be removed when a VM is destroyed. Does this reasoning make sense or am I misunderstanding the goal? Also, this feels like a minor enhancement rather than a defect fix. Therefore, I think we should move the target of the work to |
|
Hi @jburwell , this is a missing 3 line of code fix which was occured because of the fixes done in CLOUDSTACK-8302, CLOUDSTACK-9297. Those fixed bug's are already in 4.9.0 release so the codes which @ustcweizhou fixed are the required, remaining parts for the complete functional fix of CLOUDSTACK-9297 ticket. Also this fix only effects RBD users, other storage users does not effect. So if possible please lets include this fix for upcoming 4.9 lts release and if we like to add a new enhancement lets plan it for 4.10 or later upcoming. We are trying to fix this problem nearly from 4.8 release like 6-9 months or more. Currently we using this pr on our 4.9 production environment by compiling this code over 4.9.0 code, so we really need this fix on upcoming lts code. |
Contributor
|
@ozhanrk I recommend combining the fixes into a single PR for review cohesion and to ensure that the merge maintains the stability of the release branches. You could either amend this PR with the necessary commits or create a new PR and close this one -- whichever approach is easier for you. Ultimately, the PR should have one commit per JIRA ticket fixed. |
|
Hi @jburwell , i am so sorry but i do not understand, what you mean by combining fixes under a single pr because this is a only single fix with a 4 lines of code change which only effects rbd storage users for primary storage. The previous tickets which mentioned above was both fixed and currently in 4.9.0.0 code. So this is the only remaining part of that fixes for rbd users. @ustcweizhou i am not a developer, if i am wrong could you explain our current status in more technical way, i am very new to ACS community, so maybe i could not express pr's current status :) |
Contributor
Author
|
@jburwell John, this is a small bug fix for issue: failed to delete snapshot on RBD if the original volume is removed. |
|
Hi @rhtyd @jburwell, |
Contributor
|
@ustcweizhou @ozhanrk this feels like a relatively narrow case (RBD on KVM), and we are extremely late for 4.9.1.0. We have numerous "small" fixes that could be included that would further delay significant fixes that affect nearly all users. I am sorry, but this fix will have to wait until 4.9.2.0/4.10.1.0/4.11.0.0. |
Contributor
|
@ozhanrk @ustcweizhou I asked a functionality question regarding this functionality to which I cannot find an answer. Namely, that this patch is deleting snapshots in a circumstance where not all users want them deleted. |
|
hi @jburwell this pr does not delete any kind of snapshots, this patch only prevents, solves a kind of timeout issue on code thats all, nothing more. This is a 4 lines of small fix where delete snapshot code could not find already deleted snapshots ids so it waits till timeout reached, with this patch it understands they are already deleted so it did not wait and continue the process. |
Member
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-191 |
Member
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-377)
|
Member
|
Do we have LGTMs on this? |
|
Hi @kiwiflyer @nathanejohnson i saw that you are merged this fix and using it on your ENA CloudStack branch, when you have time could you check the code for possible LGTM's please. |
Member
|
LGTM! We're running it in our labs. |
Member
|
This code LGTM. |
|
Hi @rhtyd we have got the required LGTM's now. |
Member
|
Thanks @ozhanrk I'll kick a final test round with xenserver and merge this. |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests |
Member
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-419)
|
|
Trillian test result (tid-418)
|
yadvr
reviewed
Nov 24, 2016
| SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); | ||
| if (((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() == StoragePoolType.RBD) { | ||
| long volumeId = snapshotOnPrimary.getVolumeId(); | ||
| VolumeVO volumeVO = volumeDao.findById(volumeId); |
Member
There was a problem hiding this comment.
This will throw NPE, if snapshot has no primary volume id. Can there be such a case?
Contributor
Author
There was a problem hiding this comment.
@rhtyd Hi Rohit, each snapshot should have a volume_id (at least on all our platforms).
Actually in engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java, the getVolumeId() also return long , not Long.
Member
|
@ustcweizhou @ozhanrk can you check snapshot and volume related errors in the last test runs above. |
Member
|
There were some intermittent errors seen, I'll re-kick tests. |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-252 |
Member
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-438)
|
Member
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✖debian. JID-262 |
Member
|
Failure wrt debian pkg was due to an env issue. |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-462)
|
Member
|
@blueorangutan test centos7 xenserver-65sp1 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests |
|
Trillian test result (tid-468)
|
Member
|
Tests LGTM. |
asfgit
pushed a commit
that referenced
this pull request
Nov 28, 2016
CLOUDSTACK-9538: FIX failure in Deleting Snapshot From Primary Storage RBD Storage if vm has been removed * pr/1710: CLOUDSTACK-9538: FIX failure in Deleting Snapshot From Primary Storage RBD Storage if vm has been removed Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.