[NEW PATCH] BZ#705058 - StoragePool instanciations should be lock protected. (via gerrit-bot)

[Yotam Oron]

New patch submitted by Yotam Oron ([e-mail])

You can review this change at: http://gerrit.usersys.redhat.com/700

commit 6b27cdc4cae27c74959c30609d874a39ba35e7ca
Author: Yotam Oron 
Date:   Wed Jul 13 11:56:06 2011 +0300

BZ#705058 - StoragePool instanciations should be lock protected.

Add a storage shared lock on the call to _restorePool, since creates a
    new pool object

Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index b0d5d21..f763120 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -162,16 +162,14 @@ class HSM:
             self.taskMng.recoverDumpedTasks()

_poolsTmpDir = config.get('irs', 'pools_data_dir')
-            dirList = os.listdir(_poolsTmpDir)
-            for spUUID in dirList:
-                poolPath = os.path.join(self.storage_repository, spUUID)
-                try:
-                    if os.path.exists(poolPath):
-                        self._restorePool(spUUID)
-                        #TODO Once we support simultaneous connection to multiple pools, remove following line (break)
-                        break
-                except Exception:
-                    self.log.error("Unexpected error", exc_info=True)
+            spUUID = os.listdir(_poolsTmpDir)[0]
+            vars.task.getSharedLock(STORAGE, spUUID)
+            poolPath = os.path.join(self.storage_repository, spUUID)
+            try:
+                if os.path.exists(poolPath):
+                    self._restorePool(spUUID)
+            except Exception:
+                self.log.error("Unexpected error", exc_info=True)

threading.Thread(target=storageRefresh).start()

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 165:             spUUID = os.listdir(_poolsTmpDir)[0]
why did you drop the 'for' loop? now we have an exception problem if _poolsTmpDir is empty. this has no functional effect, but it's not nice.

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 170:                     self._restorePool(spUUID)
do we really need this condition?
What happens if somebody removed spUUID folder under /rhev/data-center ?
Why we can't  reconnect in this situation?

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Yotam Oron]

Yotam Oron has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 165:             spUUID = os.listdir(_poolsTmpDir)[0]
Done

Line 170:                     self._restorePool(spUUID)
This might be so but should be examined in a different BZ (I haven't changed anything from the old code in this respect).

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

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

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Yotam Oron]

Yotam Oron has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Yotam Oron]

Yotam Oron has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

Patch Set 2: Verified

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

Patch Set 2: Looks good to me, approved

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

Patch Set 3: Verified; Looks good to me, approved

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

BZ#705058 - StoragePool instanciations should be lock protected.

Add a storage shared lock on the call to _restorePool, since creates a
new pool object

Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
---
M vdsm/storage/hsm.py
1 file changed, 1 insertion(+), 0 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron

Change in vdsm[master]: BZ#705058 - StoragePool instanciations should be lock protec...

[Haim Ateya]

Haim Ateya has posted comments on this change.

Change subject: BZ#705058 - StoragePool instanciations should be lock protected.
......................................................................

Patch Set 3:

tested this change as a part of change 717 and this change seem to break vdsm initialization on cleanup stage;

Thread-11::ERROR::2011-07-19 12:02:04,240::hsm::175::Storage.HSM::(storageRefresh) Unexpected error
Traceback (most recent call last):
  File "/usr/share/vdsm/storage/hsm.py", line 169, in storageRefresh
    vars.task.getSharedLock(STORAGE, spUUID)
AttributeError: 'thread._local' object has no attribute 'task'

--
To view, visit http://gerrit.usersys.redhat.com/700
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ad3cdb07299d63203db98ae3223559b6fdc4eab
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yotam Oron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yotam Oron