Change in vdsm[master]: sp: Nest try-finally for temporary secure change
oVirt Jenkins CI Server has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 3: Build Successful http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1448/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Dan Kenigsberg has submitted this change and it was merged. Change subject: sp: Nest try-finally for temporary secure change .. sp: Nest try-finally for temporary secure change There were two instances where the same try-finally block was used for both acquiring a lock and setting the pool as secure for a block of code: acquire lock try: set secure code that needs both lock and secure state finally: set unsecure release lock This type of usage is incorrect. try-finally block should be used for one change to ensure that the change is finally reverted, and to make the intention of the code clear. This patch use nested try-finally blocks, one for the lock, and one for the secure state: acquire lock try: set secure try: code that needs both lock and secure state finally: set unsecure finally: release lock Because of the extra nesting, comments inside the nested code were reformatted and missing punctuation was added. I considered replacing the nested try-finally with a secured context manager, but since the current code uses try-except-finally, using a context manager would create an even deeper nesting or extracting some new private methods. To keep minimal changes, I avoid this direction. It seems that the secure state block can become smaller - some method inside the secured block are @unsecured. I kept the secure block semantics are they are now, and will check minimizing the secured block in a later patch. Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Signed-off-by: Nir Soffer Reviewed-on: http://gerrit.ovirt.org/23955 Reviewed-by: Dan Kenigsberg Reviewed-by: Federico Simoncelli Reviewed-by: Ayal Baron Reviewed-by: Allon Mureinik --- M vdsm/storage/sp.py 1 file changed, 48 insertions(+), 48 deletions(-) Approvals: Ayal Baron: Looks good to me, approved Nir Soffer: Verified Federico Simoncelli: Looks good to me, but someone else must approve Allon Mureinik: Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, but someone else must approve -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Nir Soffer has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 2: Verified+1 Verified on two hosts data center, both running Fedora 19 with ISCSI storage. - Create new storage domain with spm - Create new storage domain with hsm - Reconstract master flow, several times - Detach storage domain - Remove storage domain using hsm -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
oVirt Jenkins CI Server has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 2: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9288/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10072/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10227/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5154/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3312/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_gerrit/1290/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Itamar Heim has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: I actually do this manually, not a bot... -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Dan Kenigsberg has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: bot, do not abandon. it's an obviously-correct fix begging for verification. -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Itamar Heim has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: ping -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Allon Mureinik has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: Code-Review+1 Nir, let's get this verified and merged please? -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Ayal Baron has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Federico Simoncelli has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
oVirt Jenkins CI Server has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6148/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7041/ : ABORTED http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6935/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/93/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_nfs/31/ : FAILURE -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Dan Kenigsberg has posted comments on this change. Change subject: sp: Nest try-finally for temporary secure change .. Patch Set 1: Code-Review+1 We really miss context manager semantics around _acquireTemporaryClusterLock and setSecure. -- To view, visit http://gerrit.ovirt.org/23955 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Eduardo Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Sergey Gotliv Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Nir Soffer has posted comments on this change.
Change subject: sp: Nest try-finally for temporary secure change
..
Patch Set 1:
(4 comments)
See the comments explaining the issues of the previous code.
http://gerrit.ovirt.org/#/c/23955/1/vdsm/storage/sp.py
File vdsm/storage/sp.py:
Line 597
Line 598
Line 599
Line 600
Line 601
If this raise, the temporary lock will not be released, and monitors would not
be stopped.
Line 701
Line 702
Line 703
Line 704
Line 705
Note - if this raise, the lock will not released, and monitor would not be
stopped.
Line 598: self.log.error("Cleanup failed due to an
unexpected error",
Line 599:exc_info=True)
Line 600: raise
Line 601: finally:
Line 602: self._setUnsecure()
If this raise, everything is cleaned up properly.
Line 603: finally:
Line 604: self._releaseTemporaryClusterLock(msdUUID)
Line 605: self.stopMonitoringDomains()
Line 606:
Line 702: # As in the create method we need to temporarily set the
object
Line 703: # secure in order to change the domains map.
Line 704: # TODO: it is clear that reconstructMaster and create
(StoragePool)
Line 705: # are extremely similar and they should be unified.
Line 706: self._setSecure()
If this raise, the lock and monitors are cleaned up properly.
Line 707: try:
Line 708: self.createMaster(poolName, futureMaster,
masterVersion,
Line 709: leaseParams)
Line 710: self.setMasterDomain(msdUUID, masterVersion)
--
To view, visit http://gerrit.ovirt.org/23955
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Eduardo
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Nir Soffer
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sp: Nest try-finally for temporary secure change
Nir Soffer has uploaded a new change for review.
Change subject: sp: Nest try-finally for temporary secure change
..
sp: Nest try-finally for temporary secure change
There were two instances where the same try-finally block was used for
both acquiring a lock and setting the pool as secure for a block of
code:
acquire lock
try:
set secure
code that needs both lock and secure state
finally:
set unsecure
release lock
This type of usage is incorrect. try-finally block should be used for
one change to ensure that the change is finally reverted, and to make
the intention of the code clear.
This patch use nested try-finally blocks, one for the lock, and one for
the secure state:
acquire lock
try:
set secure
try:
code that needs both lock and secure state
finally:
set unsecure
finally:
release lock
Because of the extra nesting, comments inside the nested code were
reformatted and missing punctuation was added.
I considered replacing the nested try-finally with a secured context
manager, but since the current code uses try-except-finally, using a
context manager would create an even deeper nesting or extracting some
new private methods. To keep minimal changes, I avoid this direction.
It seems that the secure state block can become smaller - some method
inside the secured block are @unsecured. I kept the secure block
semantics are they are now, and will check minimizing the secured block
in a later patch.
Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b
Signed-off-by: Nir Soffer
---
M vdsm/storage/sp.py
1 file changed, 48 insertions(+), 48 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/23955/1
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index a020e1e..c26f7a8 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -566,40 +566,41 @@
fileUtils.createdir(self.poolPath)
self._acquireTemporaryClusterLock(msdUUID, leaseParams)
-
try:
self._setSecure()
-# Mark 'master' domain
-# We should do it before actually attaching this domain to the pool
-# During 'master' marking we create pool metadata and each attached
-# domain should register there
-self.createMaster(poolName, msd, masterVersion, leaseParams)
-self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
-# Attach storage domains to the storage pool
-# Since we are creating the pool then attach is done from the hsm
-# and not the spm therefore we must manually take the master domain
-# lock
-# TBD: create will receive only master domain and further attaches
-# should be done under SPM
-
-# Master domain was already attached (in createMaster),
-# no need to reattach
-for sdUUID in domList:
-# No need to attach the master
-if sdUUID != msdUUID:
-self.attachSD(sdUUID)
-except Exception:
-self.log.error("Create pool %s canceled ", poolName, exc_info=True)
try:
-fileUtils.cleanupdir(self.poolPath)
-self.__cleanupDomains(domList, msdUUID, masterVersion)
-except:
-self.log.error("Cleanup failed due to an unexpected error",
- exc_info=True)
-raise
-finally:
-self._setUnsecure()
+# Mark 'master' domain. We should do it before actually
+# attaching this domain to the pool During 'master' marking we
+# create pool metadata and each attached domain should register
+# there.
+self.createMaster(poolName, msd, masterVersion, leaseParams)
+self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
+# Attach storage domains to the storage pool. Since we are
+# creating the pool then attach is done from the hsm and not
+# the spm therefore we must manually take the master domain
+# lock.
+# TBD: create will receive only master domain and further
+# attaches should be done under SPM.
+# Master domain was already attached (in createMaster), no need
+# to reattach.
+for sdUUID in domList:
+# No need to attach the master
+if sdUUID != msdUUID:
+self.attachSD(sdUUID)
+except Exception:
+self.log.error("Create pool %s canceled ", poolName,
+ exc_info=True)
+try:
+fileUtils.cleanupdir(self.poolPath)
+
