CLOUDSTACK-10202:createSnapshotPolicy API create multiple entries in DB for same volume. by niteshsarda · Pull Request #2373 · apache/cloudstack

niteshsarda · 2017-12-21T04:24:22Z

ISSUE :
createSnapshotPolicy API create multiple entries in DB for same parameters, if multiple threads are executed in parallel.

STEPS TO REPRODUCE :

Created a new machine having root and data disk.
Make sure that no existing snapshot policy is present for the volume.
Execute multiple threads in parallel for createSnapshotPolicy API having all required parameters exactly same.
Verify table snapshot_policy in DB, will get multiple entries for same policy.
Once again execute same multiple threads, by changing any API parameter, will see that existing entries are getting modified in DB and no new entries are added.

marcaurele · 2017-12-21T07:02:31Z

- } else {
- try {
+
+ final GlobalLock createSnapshotPolicyLock = GlobalLock.getInternLock("createSnapshotPolicy");


Would be more effective to append the volume id in the lock name to avoid locking the call for all snapshot policy.

@marcaurele : Implemented the changes as per your suggestion.

marcaurele · 2017-12-21T07:04:00Z

I'm not convinced that such a fix is needed. In what scenario do you see such a problem?

niteshsarda · 2017-12-25T09:02:34Z

@marcaurele : One of our customer have reported this scenario, where they have multiple management servers and when parallel request comes for snapshot policy creation for same volume, then multiple entries are created in DB for same volume.
As per the code, only one snapshot policy entry should be created in database for each volume. Whenever, another request of snapshot policy comes for the same volume, then existing entry should be updated. Same behavior is observed, when volume have existing snapshot policy and parallel threads are executed, then only single entry is getting updated every time. This is because, we have lock available in code in else block. Whereas, in case of first snapshot policy creation for volume, DB lock is not available, hence it creates multiple DB entries.
As multiple management servers might send API requests in parallel for same volume, we should handle this from our code and allow to create only one entry in DB for each volume.

Also, I apologize for confusing title of the PR, I have updated the same, to make it more clear and relevant to the fix.

rafaelweingartner · 2017-12-25T10:27:47Z

I have experienced the same problem some time ago. I did not manage to reproduce it though. One of the volumes had two "daily snapshots" in the database.

yadvr · 2017-12-26T06:35:11Z

@blueorangutan package

blueorangutan · 2017-12-26T06:35:33Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-12-26T07:09:18Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1488

yadvr · 2017-12-26T07:24:27Z

@blueorangutan package

blueorangutan · 2017-12-26T07:24:33Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

marcaurele · 2017-12-26T07:25:39Z

- } else {
- try {
+
+ policy = _snapshotPolicyDao.acquireInLockTable(volumeId);


You can request a lock for the snapshot policy table for existing entries and by giving a snapshot policy ID only, not the ID of a volume: policy = _snapshotPolicyDao.acquireInLockTable(policyId);
In your case you don't have a policy id yet when creating a new one. You'll have to use the global lock.

@marcaurele : Changed the code as per your suggestion. Now added a code for using global lock along with volume id added in the lock name.
Can you please review the new code ?

blueorangutan · 2017-12-26T09:39:15Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1504

marcaurele · 2017-12-26T10:37:01Z

- try {
+
+ final GlobalLock createSnapshotPolicyLock = GlobalLock.getInternLock("createSnapshotPolicy_" + volumeId);
+ createSnapshotPolicyLock.lock(5);


The return value is not checked (boolean) which should handle the case when the lock could not be acquired.

@marcaurele : Do we just need to add the debug logger if lock is not acquired ?
Please confirm.

If the lock is not acquired you have to stop processing the request and return a failure to the user. Look at other part of the code using that lock mechanism.

@marcaurele : Incorporated the changes suggested by you for handling the condition of lock not acquired. In case if lock is not acquired, null value will be returned.
In CreateSnapshotPolicyCmd class, inside execute method from where this method is called, we have already handled the null condition, and in case of null value exception will be thrown.
Please review the latest code.

marcaurele

Last little change on the log level

marcaurele · 2017-12-28T05:51:18Z

+ CallContext.current().putContextParameter(SnapshotPolicy.class, policy.getUuid());
+ return policy;
+ } else {
+ s_logger.debug("Unable to acquire lock for creating snapshot policy of volume : " + volume.getName());


This log should be move to the warn level

@marcaurele : Changed the log level to warn.
Please check latest code.

marcaurele

LGTM based on code review

niteshsarda · 2017-12-28T07:58:30Z

@rhtyd : Can you please re-run the test cases again ?

yadvr · 2017-12-28T09:30:12Z

sure @niteshsarda
@blueorangutan package

blueorangutan · 2017-12-28T09:30:32Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

niteshsarda · 2017-12-28T09:59:25Z

@rhtyd : Thank you.

blueorangutan · 2017-12-28T10:06:09Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1510

yadvr · 2017-12-28T10:07:30Z

Re-kick as commit was pushed after last pkg build run
@blueorangutan package

blueorangutan · 2017-12-28T10:08:32Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

…DB for same volume.

blueorangutan · 2017-12-28T11:50:21Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1516

niteshsarda · 2018-01-01T03:40:30Z

@rhtyd : Can you please re-run the test cases again as results for previous execution is not posted yet?

yadvr · 2018-01-01T09:26:49Z

Tests haven't been kicked yet.
@blueorangutan package

blueorangutan · 2018-01-01T09:27:32Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-01-01T09:52:55Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1532

yadvr · 2018-01-01T09:56:56Z

@blueorangutan test

blueorangutan · 2018-01-01T09:57:32Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-01-01T18:27:23Z

Trillian test result (tid-1961)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30588 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2373-t1961-kvm-centos7.zip
Smoke tests completed. 66 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

yadvr · 2018-01-02T03:23:07Z

Lgtm. Merging this based on 2 lgtms and test results.

Refatoração nos _parsers_ do Usage Closesapache#1056, apache#2373, and apache#2387 See merge request scclouds/scclouds!966

marcaurele reviewed Dec 21, 2017
View reviewed changes

niteshsarda changed the title ~~CLOUDSTACK-10202:createSnapshotPolicy API create multiple entries in DB for same parameters.~~CLOUDSTACK-10202:createSnapshotPolicy API create multiple entries in DB for same volume.Dec 25, 2017

marcaurele reviewed Dec 26, 2017
View reviewed changes

marcaurele requested changes Dec 26, 2017
View reviewed changes

marcaurele requested changes Dec 28, 2017
View reviewed changes

marcaurele approved these changes Dec 28, 2017
View reviewed changes

yadvr added this to the 4.11 milestone Dec 28, 2017

CLOUDSTACK-10202:createSnapshotPolicy API create multiple entries in …
abc5d82
…DB for same volume.

yadvr merged commit 36f43b5 into apache:masterJan 2, 2018

Uh oh!

Conversation

niteshsarda commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaurele commented Dec 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niteshsarda commented Dec 25, 2017

Uh oh!

rafaelweingartner commented Dec 25, 2017

Uh oh!

yadvr commented Dec 26, 2017

Uh oh!

blueorangutan commented Dec 26, 2017

Uh oh!

blueorangutan commented Dec 26, 2017

Uh oh!

yadvr commented Dec 26, 2017

Uh oh!

blueorangutan commented Dec 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaurele left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaurele left a comment

Choose a reason for hiding this comment

Uh oh!

niteshsarda commented Dec 28, 2017

Uh oh!

yadvr commented Dec 28, 2017

Uh oh!

blueorangutan commented Dec 28, 2017

Uh oh!

niteshsarda commented Dec 28, 2017

Uh oh!

blueorangutan commented Dec 28, 2017

Uh oh!

yadvr commented Dec 28, 2017

Uh oh!

blueorangutan commented Dec 28, 2017

Uh oh!

blueorangutan commented Dec 28, 2017

Uh oh!

niteshsarda commented Jan 1, 2018

Uh oh!

yadvr commented Jan 1, 2018

Uh oh!

blueorangutan commented Jan 1, 2018

Uh oh!

blueorangutan commented Jan 1, 2018

Uh oh!

yadvr commented Jan 1, 2018

Uh oh!

blueorangutan commented Jan 1, 2018

Uh oh!

blueorangutan commented Jan 1, 2018

Uh oh!

yadvr commented Jan 2, 2018

marcaurele commented Dec 21, 2017•
edited
Loading