Change in vdsm[master]: guarded: Raise if attempt to lock will deadlock

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................

guarded: Raise if attempt to lock will deadlock

Previously we did not validate that the same lock was submitted in
different modes, since we assumed that resourceManager will fail. Turns
out that resourceManager will simply deadlock.

This patch validates the locks and raise a guarded.Deadlock in this
case, without taking any locks.

Here is an example exception (wrapped):

    Deadlock: Attempt to lock will deadlock: [
    <FakeGuardedLock ns=02_img.dom, name=img, mode=exclusive at 0x7fab62093110>,
    <FakeGuardedLock ns=02_img.dom, name=img, mode=shared at 0x7fab620931d0>]

Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M lib/vdsm/storage/guarded.py
M tests/storage_guarded_test.py
2 files changed, 46 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/77/64977/1

diff --git a/lib/vdsm/storage/guarded.py b/lib/vdsm/storage/guarded.py
index 99b7ab9..98358a1 100644
--- a/lib/vdsm/storage/guarded.py
+++ b/lib/vdsm/storage/guarded.py
@@ -19,7 +19,9 @@
 #
 
 from __future__ import absolute_import
+import itertools
 import logging
+import operator
 import sys
 
 import six
@@ -29,6 +31,16 @@
 
 class ReleaseError(Exception):
     pass
+
+
+class Deadlock(Exception):
+    msg = "Attempt to lock will deadlock: {self.locks}"
+
+    def __init__(self, locks):
+        self.locks = locks
+
+    def __str__(self):
+        return self.msg.format(self=self)
 
 
 class context(object):
@@ -48,6 +60,9 @@
     acquired in sorted order.  When exiting the context the locks are released
     in reverse order.  Errors are handled as gracefully as possible with any
     acquired locks being released in the proper order.
+
+    Attemping to lock the same lock twice with differnt mode is not supported
+    and will raise a Deadlock exception with the conflicting locks.
     """
 
     def __init__(self, locks):
@@ -55,9 +70,23 @@
         Receives a variable number of locks which must descend from
         AbstractLock.  The locks are deduplicated and sorted.
         """
-        self._locks = sorted(set(locks))
+        self._locks = self._validate(locks)
         self._held_locks = []
 
+    def _validate(self, locks):
+        """
+        Remove duplicate locks and sort the locks.
+
+        Raises Deadlock if trying to take the same lock with different modes.
+        """
+        locks = sorted(set(locks))
+        by_ns_name = operator.attrgetter("ns", "name")
+        for _, group in itertools.groupby(locks, by_ns_name):
+            group = list(group)
+            if len(group) > 1:
+                raise Deadlock(group)
+        return locks
+
     def __enter__(self):
         for lock in self._locks:
             try:
diff --git a/tests/storage_guarded_test.py b/tests/storage_guarded_test.py
index 4ec1de1..ec90ed9 100644
--- a/tests/storage_guarded_test.py
+++ b/tests/storage_guarded_test.py
@@ -188,3 +188,19 @@
             with guarded.context(locks):
                 raise RuntimeError()
         self.assertEqual(expected, log)
+
+    def test_deadlock(self):
+        log = []
+        locks = [
+            FakeGuardedLock('00_storage', 'dom', 'shared', log),
+            # Attemting to lock next locks will deadlock in resourceManager.
+            FakeGuardedLock('02_img.dom', 'img', 'exclusive',
log),
+            FakeGuardedLock('02_img.dom', 'img', 'shared', log),
+            FakeGuardedLock('03_vol.dom', 'vol', 'exclusive',
log),
+        ]
+        # Creating a context should raise
+        with self.assertRaises(guarded.Deadlock):
+            with guarded.context(locks):
+                pass
+        # Without locking any of the locks
+        self.assertEqual([], log)


-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................


Patch Set 1: Verified+1

Verified by the tests.

-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Adam Litke has posted comments on this change.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

https://gerrit.ovirt.org/#/c/64977/1/lib/vdsm/storage/guarded.py
File lib/vdsm/storage/guarded.py:

PS1, Line 83: by_ns_name = operator.attrgetter("ns", "name")
            :         for _, group in itertools.groupby(locks, by_ns_name):
Interesting.  I've never run into operator.attrgetter and itertools.groupby before. 
Neat.


-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................


Patch Set 1: Continuous-Integration+1

Again, network

-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has submitted this change and it was merged.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................


guarded: Raise if attempt to lock will deadlock

Previously we did not validate that the same lock was submitted in
different modes, since we assumed that resourceManager will fail. Turns
out that resourceManager will simply deadlock.

This patch validates the locks and raise a guarded.Deadlock in this
case, without taking any locks.

Here is an example exception (wrapped):

    Deadlock: Attempt to lock will deadlock: [
    <FakeGuardedLock ns=02_img.dom, name=img, mode=exclusive at 0x7fab62093110>,
    <FakeGuardedLock ns=02_img.dom, name=img, mode=shared at 0x7fab620931d0>]

Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/64977
Reviewed-by: Adam Litke <alitke(a)redhat.com&gt;
---
M lib/vdsm/storage/guarded.py
M tests/storage_guarded_test.py
2 files changed, 46 insertions(+), 1 deletion(-)

Approvals:
  Adam Litke: Looks good to me, approved
  Nir Soffer: Verified; Passed CI tests

Objections:
  Jenkins CI: Failed CI tests



-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;

gerrit-hooks has posted comments on this change.

Change subject: guarded: Raise if attempt to lock will deadlock
......................................................................


Patch Set 2:

* #64977::Update tracker: OK
* Set MODIFIED::IGNORE, no Bug-Url found.

-- 
To view, visit https://gerrit.ovirt.org/64977
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cd539548eda04ac7b9faf0ba1be49f29bfa2ed0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No