Change in vdsm[master]: Group the cluster lock code into a new SP method

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Group the cluster lock code into a new SP method

Both the create and reconstructMaster methods were sharing a very
similar code for temporarly acquiring the cluster lock.

Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
---
M vdsm/storage/sp.py
1 file changed, 29 insertions(+), 24 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/03/603/1
--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

(1 inline comment)

If you don't think my concern should be addressed then it's fine by me. else just fix it and send another patch,

....................................................
File vdsm/storage/sp.py
Line 70: POOL_TEMPORARY_HOSTID = 250
250 is really, it also might need to be lock implementation dependent.

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 70: POOL_TEMPORARY_HOSTID = 250
Yes it should be lock dependent but the purpose this patch is only refactoring the current code. I just realized that it should remain 1000.

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/storage/sp.py
Line 635:         self._acquireTemporaryClusterLock(msdUUID, safeLease)
I think you can pass here the master domain object itself  'msd'. You already produced it. In this case you can avoid unnecessary  domain producing. I know that it's probably will be done from the cache, but first not always and second Edu works very hard to reduce domain produces in the code.

Line 667:             self._releaseTemporaryClusterLock(msdUUID)
same as above

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 635:         self._acquireTemporaryClusterLock(msdUUID, safeLease)
This is consistent with the rest of the APIs where we always pass the (m)sdUUID. I agree that it's "expensive" but the creation and the master migration is done very rarely. I agree on avoiding the "producing" calls but I don't see how that is related to this specific patch. Please let's group this API design change in an other patch that is taking care of changing them all.

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 635:         self._acquireTemporaryClusterLock(msdUUID, safeLease)
It's related to this patch only because this patch add 2 more unnecessary domain produces. But, if you think that change it in different patch is good enough, It's fine for me.

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 635:         self._acquireTemporaryClusterLock(msdUUID, safeLease)
For what I see it adds just 1 extra "produce" (in the reconstructMaster case).

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 70: POOL_TEMPORARY_HOSTID = 250
I prefer keeping non-public stuff __private__. Or even function-local.

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 71: POOL_TEMPORARY_HOSTID = 250
Should be lock provider specific as 250 is completely valid in safelease

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 5: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 6: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 6: Verified

Verified in the pre-integration ticket #554.

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Group the cluster lock code into a new SP method

Both the create and reconstructMaster methods were sharing a very
similar code for temporarly acquiring the cluster lock.

Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
---
M vdsm/storage/safelease.py
M vdsm/storage/sd.py
M vdsm/storage/sp.py
3 files changed, 36 insertions(+), 25 deletions(-)

Approvals:
  Federico Simoncelli: Verified
  Dan Kenigsberg: Looks good to me, approved
  Igor Lvovsky: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Group the cluster lock code into a new SP method
......................................................................

Patch Set 6: Looks good to me, approved

(1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 500:     def _acquireTemporaryClusterLock(self, msdUUID, safeLease):
using a context manager could have been cooler... but never mind ;-)

--
To view, visit http://gerrit.ovirt.org/603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I981d84af7816ccd2db64eb5beb440e9f7637f734
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi