New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLOUDSTACK-9932 snapshot is getting deleted while volume is in creating state #2149
Conversation
volumeCheck = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null); | ||
|
||
if (volumeCheck.size() > 0) { | ||
throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in using "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change "is in using" to "is in use" ? That would be a better log line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wido changes done as suggested
@@ -530,6 +534,13 @@ public boolean deleteSnapshot(long snapshotId) { | |||
return false; | |||
} | |||
|
|||
List<VolumeDetailVO> volumeCheck; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the variable name to more meaningful. eg: volumesFromSnapshot, volumesFromSnapshotCheck,...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sureshanaparti changes done as suggested
if (volumeCheck.size() > 0) { | ||
throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use "); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other approach to check the volumes creating from a given snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a volume created from snapshot, we are not maintaining volume in relation to snapshot details anywhere in database. It's been recently integrated and information stored in VOLUME_DETAILS table.
Data/Records in this table will be persist till volume creation operation finish(success/failure). Once volume operation done this record will be removed. I have not come across alternatives other than this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavanaravapalli thanks for the confirmation.
Code changes LGTM. |
List<VolumeDetailVO> volumesFromSnapshot; | ||
volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null); | ||
|
||
if (volumesFromSnapshot.size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be race condition here. After volumesFromSnapshot = 0, a new volume can be created from snapshot, before the snapshot is actually deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kishankavala race condition problem handled and, code changes done accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@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-793 |
@blueorangutan test |
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-1194)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM based on code review and test results
@pavanaravapalli please fix the conflicts |
@pavanaravapalli please rebase and fix conflicts |
@rhtyd ok I will do . |
@pavanaravapalli can you fix the conflicts? |
…reation from the snapshot is in progress
@rhtyd rebased and fixed conflicts |
@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-1426 |
@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-1497 |
@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-1513 |
@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-1553 |
@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-1989)
|
Merging this based on code reviews and test results. |
Added validation to check if any volume(s) are in creating state , before performing delete snapshot.