Change in vdsm[master]: domainMonitor: Fix unsafe status handling

nsoffer at redhat.com nsoffer at redhat.com
Sat Jun 21 00:11:26 UTC 2014


Nir Soffer has uploaded a new change for review.

Change subject: domainMonitor: Fix unsafe status handling
......................................................................

domainMonitor: Fix unsafe status handling

DomainMonitor status was updated in a unsafe way, assuming that another
thread will never ask the status in the middle of an update. The code
was written as if creating a new status object is expensive operation
that should be avoided.

Instead of having long living status object in the monitor, I create new
forozen copy each monitor internal (10 seconds). The frozen copy ensure
that careless developers trying to modify a status will get what they
deserve.

Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
M tests/Makefile.am
A tests/domainMonitorTests.py
M vdsm/storage/domainMonitor.py
3 files changed, 59 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/14/29014/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6507165..e35ae1c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -30,6 +30,7 @@
 	capsTests.py \
 	clientifTests.py \
 	configNetworkTests.py \
+	domainMonitorTests.py \
 	fileVolumeTests.py \
 	fileUtilTests.py \
 	fuserTests.py \
diff --git a/tests/domainMonitorTests.py b/tests/domainMonitorTests.py
new file mode 100644
index 0000000..b068ec8
--- /dev/null
+++ b/tests/domainMonitorTests.py
@@ -0,0 +1,43 @@
+#
+# Copyright 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
+#
+# Refer to the README and COPYING files for full details of the license
+#
+
+from storage import domainMonitor
+from testrunner import VdsmTestCase
+
+
+class FrozenStatusTests(VdsmTestCase):
+
+    def setUp(self):
+        self.status = domainMonitor.DomainMonitorStatus()
+        self.frozen = domainMonitor.FrozenStatus(self.status)
+
+    def test_copy_attributes(self):
+        for name in self.status.__slots__:
+            value = getattr(self.status, name)
+            expected = getattr(self.frozen, name)
+            self.assertEquals(value, expected)
+
+    def test_setting_attribute_raises(self):
+        for name in self.status.__slots__:
+            self.assertRaises(AssertionError, setattr, self.frozen, name, 1)
+
+    def test_deleting_attribute_raises(self):
+        for name in self.status.__slots__:
+            self.assertRaises(AssertionError, delattr, self.frozen, name)
diff --git a/vdsm/storage/domainMonitor.py b/vdsm/storage/domainMonitor.py
index 8a50b4f..baa5588 100644
--- a/vdsm/storage/domainMonitor.py
+++ b/vdsm/storage/domainMonitor.py
@@ -38,9 +38,6 @@
     )
 
     def __init__(self):
-        self.clear()
-
-    def clear(self):
         self.error = None
         self.checkTime = time()
         self.valid = True
@@ -62,14 +59,18 @@
         self.isoPrefix = None
         self.version = -1
 
-    def update(self, st):
-        for attr in self.__slots__:
-            setattr(self, attr, getattr(st, attr))
 
-    def copy(self):
-        res = DomainMonitorStatus()
-        res.update(self)
-        return res
+class FrozenStatus(DomainMonitorStatus):
+
+    def __init__(self, other):
+        for name in other.__slots__:
+            value = getattr(other, name)
+            super(FrozenStatus, self).__setattr__(name, value)
+
+    def __setattr__(self, *args):
+        raise AssertionError('%s is readonly' % self)
+
+    __delattr__ = __setattr__
 
 
 class DomainMonitor(object):
@@ -141,8 +142,8 @@
         self.hostId = hostId
         self.interval = interval
         self.firstChange = True
-        self.status = DomainMonitorStatus()
         self.nextStatus = DomainMonitorStatus()
+        self.status = FrozenStatus(self.nextStatus)
         self.isIsoDomain = None
         self.isoPrefix = None
         self.lastRefresh = time()
@@ -158,7 +159,7 @@
             self.thread.join()
 
     def getStatus(self):
-        return self.status.copy()
+        return self.status
 
     @utils.traceback(on=log.name)
     def _monitorLoop(self):
@@ -184,7 +185,7 @@
                                "%s", self.hostId, self.sdUUID, exc_info=True)
 
     def _monitorDomain(self):
-        self.nextStatus.clear()
+        self.nextStatus = DomainMonitorStatus()
 
         if time() - self.lastRefresh > self.refreshTime:
             # Refreshing the domain object in order to pick up changes as,
@@ -265,7 +266,7 @@
                                "request for domain %s", self.hostId,
                                self.sdUUID, exc_info=True)
 
-        self.status.update(self.nextStatus)
+        self.status = FrozenStatus(self.nextStatus)
 
     def _statusDidChange(self):
         return self.firstChange or self.status.valid != self.nextStatus.valid


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>


More information about the vdsm-patches mailing list