Nir Soffer has uploaded a new change for review.
Change subject: resourceManager: Get rid of getInstance
......................................................................
resourceManager: Get rid of getInstance
The single instance of ResourceManager is created in import time,
eliminating the clumsy getInstance mechanism.
Internal callers in the resourceManager module are using now the module
interface directly.
Change-Id: I136ab52f15332f43f2b0ecd9e9a2ff7b558df87a
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
---
M tests/storage_resourcemanager_test.py
M vdsm/storage/resourceManager.py
2 files changed, 57 insertions(+), 87 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/37/65037/1
diff --git a/tests/storage_resourcemanager_test.py
b/tests/storage_resourcemanager_test.py
index 8ff65c8..5859f61 100644
--- a/tests/storage_resourcemanager_test.py
+++ b/tests/storage_resourcemanager_test.py
@@ -130,19 +130,13 @@
class ResourceManagerTests(TestCaseBase):
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testErrorInFactory(self):
req = rm._registerResource(
"error", "resource", rm.EXCLUSIVE, lambda req, res: 1)
self.assertTrue(req.canceled())
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
- def testSingleton(self):
- a = rm.ResourceManager.getInstance()
- b = rm.ResourceManager.getInstance()
- self.assertEquals(id(a), id(b))
-
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRegisterInvalidNamespace(self):
try:
rm.registerNamespace("I.HEART.DOTS", rm.SimpleResourceFactory())
@@ -151,7 +145,7 @@
self.fail("Managed to register an invalid namespace")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testFailCreateAfterSwitch(self):
resources = []
@@ -166,30 +160,30 @@
self.assertTrue(sharedReq1.canceled())
self.assertEquals(resources[0], None)
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testReregisterNamespace(self):
self.assertRaises((ValueError, KeyError), rm.registerNamespace,
"storage", rm.SimpleResourceFactory())
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceSwitchLockTypeFail(self):
self.testResourceLockSwitch("switchfail")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestInvalidResource(self):
self.assertRaises(ValueError, rm.acquireResource,
"storage", "DOT.DOT", rm.SHARED)
self.assertRaises(ValueError, rm.acquireResource,
"DOT.DOT", "resource", rm.SHARED)
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testReleaseInvalidResource(self):
self.assertRaises(ValueError, rm.releaseResource,
"DONT_EXIST", "resource")
self.assertRaises(ValueError, rm.releaseResource, "storage",
"DOT")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceWrapper(self):
s = StringIO
with rm.acquireResource("string", "test", rm.EXCLUSIVE) as
resource:
@@ -198,7 +192,7 @@
continue
self.assertTrue(hasattr(resource, attr))
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testAccessAttributeNotExposedByWrapper(self):
with rm.acquireResource("string", "test", rm.EXCLUSIVE) as
resource:
try:
@@ -212,7 +206,7 @@
self.fail("Managed to access an attribute not exposed by wrapper")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testAccessAttributeNotExposedByRequestRef(self):
resources = []
@@ -233,7 +227,7 @@
self.fail("Managed to access an attribute not exposed by wrapper")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestRefStr(self):
resources = []
@@ -247,7 +241,7 @@
req.wait()
resources[0].release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestRefCmp(self):
resources = []
requests = []
@@ -278,7 +272,7 @@
self.assertNotEqual(req1, "STUFF")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestRecancel(self):
resources = []
@@ -295,7 +289,7 @@
blocker.release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestRegrant(self):
resources = []
@@ -306,7 +300,7 @@
req.grant()
self.assertRaises(rm.RequestAlreadyProcessedError, req.grant)
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestWithBadCallbackOnCancel(self):
def callback(req, res):
raise Exception("BUY MILK!")
@@ -319,7 +313,7 @@
blocker.release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRequestWithBadCallbackOnGrant(self):
def callback(req, res):
res.release()
@@ -329,13 +323,13 @@
"string", "resource", rm.EXCLUSIVE, callback)
req.wait()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testRereleaseResource(self):
res = rm.acquireResource("string", "resource", rm.EXCLUSIVE)
res.release()
res.release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testCancelExclusiveBetweenShared(self):
resources = []
@@ -379,11 +373,11 @@
while len(resources) > 0:
resources.pop().release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testCrashOnSwitch(self):
self.testResourceLockSwitch("crashy")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceLockSwitch(self, namespace="string"):
resources = []
@@ -424,7 +418,7 @@
hash(exclusive3)
hash(sharedReq3)
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceAcquireTimeout(self):
exclusive1 = rm.acquireResource("string", "resource",
rm.EXCLUSIVE)
self.assertRaises(rm.RequestTimedOutError,
@@ -432,12 +426,12 @@
rm.EXCLUSIVE, 1)
exclusive1.release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceAcquireInvalidTimeout(self):
self.assertRaises(TypeError, rm.acquireResource, "string",
"resource", rm.EXCLUSIVE, "A")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceInvalidation(self):
resource = rm.acquireResource("string", "test",
rm.EXCLUSIVE)
try:
@@ -447,11 +441,11 @@
resource.release()
self.assertRaises(Exception, resource.write, "test")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testForceRegisterNamespace(self):
rm.registerNamespace("storage", rm.SimpleResourceFactory(), True)
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceAutorelease(self):
self.log.info("Acquiring resource", extra={'resource':
"bob"})
res = rm.acquireResource("storage", "resource", rm.SHARED)
@@ -471,7 +465,7 @@
break
time.sleep(1)
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testAcquireResourceShared(self):
res1 = rm.acquireResource("storage", "resource", rm.SHARED)
res2 = rm.acquireResource("storage", "resource", rm.SHARED,
10)
@@ -479,7 +473,7 @@
res1.release()
res2.release()
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testResourceStatuses(self):
self.assertEquals(rm._getResourceStatus("storage",
"resource"),
rm.LockState.free)
@@ -499,7 +493,7 @@
self.fail("Managed to get status on a non existing resource")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testAcquireNonExistingResource(self):
try:
rm.acquireResource("null", "resource", rm.EXCLUSIVE)
@@ -508,7 +502,7 @@
self.fail("Managed to get status on a non existing resource")
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testAcquireResourceExclusive(self):
resources = []
@@ -548,7 +542,7 @@
self.assertTrue(exclusiveReq2.granted())
resources.pop().release() # exclusiveReq 2
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
def testCancelRequest(self):
resources = []
@@ -575,7 +569,7 @@
self.assertTrue(exclusiveReq3.granted())
resources.pop().release() # exclusiveReq 3
- @MonkeyPatch(rm.ResourceManager, "_instance", manager())
+ @MonkeyPatch(rm, "_manager", manager())
@slowtest
@stresstest
def testStressTest(self):
@@ -697,19 +691,18 @@
self.assertFalse(a < b)
self.assertFalse(b < a)
+ @MonkeyPatch(rm, "_manager", FakeResourceManager())
def test_acquire_release(self):
- fake_rm = FakeResourceManager()
-
lock = rm.ResourceManagerLock('ns_A', 'name_A', rm.SHARED)
- lock._rm = fake_rm
expected = []
lock.acquire()
expected.append(('acquireResource',
- (lock.ns, lock.name, lock.mode), {}))
- self.assertEqual(expected, fake_rm.__calls__)
+ (lock.ns, lock.name, lock.mode),
+ {"timeout": None}))
+ self.assertEqual(expected, rm._manager.__calls__)
lock.release()
expected.append(('releaseResource', (lock.ns, lock.name), {}))
- self.assertEqual(expected, fake_rm.__calls__)
+ self.assertEqual(expected, rm._manager.__calls__)
def test_repr(self):
mode = rm.SHARED
diff --git a/vdsm/storage/resourceManager.py b/vdsm/storage/resourceManager.py
index 5bb4550..a3b0c0e 100644
--- a/vdsm/storage/resourceManager.py
+++ b/vdsm/storage/resourceManager.py
@@ -292,13 +292,11 @@
"ignored.")
return
- ResourceManager.getInstance().releaseResource(self.namespace,
- self.name)
+ releaseResource(self.namespace, self.name)
self._isValid = False
def getStatus(self):
- return ResourceManager.getInstance().getResourceStatus(self.namespace,
- self.name)
+ return _getResourceStatus(self.namespace, self.name)
def __enter__(self):
return self
@@ -318,7 +316,7 @@
# might try to acquire the lock in a locked context and reach a
# deadlock. This is why I need to use a timer. It will defer
# the operation and use a different context.
- ResourceManager.getInstance().releaseResource(namespace, name)
+ releaseResource(namespace, name)
t = concurrent.thread(
release,
args=(self._log, self.namespace, self.name),
@@ -336,30 +334,16 @@
"""
Manages all the resources in the application.
- This class is a singleton. use `getInstance()` to get the global instance
+ This class is for internal usage only, clients should use the module
+ interface.
"""
_log = logging.getLogger("storage.ResourceManager")
_namespaceValidator = re.compile(r"^[\w\d_-]+$")
_resourceNameValidator = re.compile(r"^[^\s.]+$")
- _instance = None
- _singletonLock = threading.Lock()
-
def __init__(self):
self._syncRoot = rwlock.RWLock()
self._namespaces = {}
-
- @classmethod
- def getInstance(cls):
- """
- Get the global resource manager
- """
- if cls._instance is None:
- with cls._singletonLock:
- if cls._instance is None:
- cls._instance = ResourceManager()
-
- return cls._instance
def registerNamespace(self, namespace, factory, force=False):
if not self._namespaceValidator.match(namespace):
@@ -782,8 +766,6 @@
if raiseonfailure is None:
raiseonfailure = self.raiseonfailure
- manager = ResourceManager.getInstance()
-
if timeout_ms is not None:
timeout = timeout_ms / 1000.0
@@ -796,8 +778,8 @@
"already acquired" % (self, fullName))
try:
- resource = manager.acquireResource(namespace, name,
- locktype, timeout)
+ resource = acquireResource(namespace, name, locktype,
+ timeout)
self.resources[resource.fullName] = resource
if hasattr(self.ownerobject, "resourceAcquired"):
@@ -844,8 +826,6 @@
raise ValueError("Owner %s: acquire: resource %s is already "
"acquired" % (self, fullName))
- manager = ResourceManager.getInstance()
-
self.lock.acquire()
try:
if fullName in self.requests:
@@ -853,8 +833,8 @@
(fullName, self))
try:
- request = manager.registerResource(namespace, name, locktype,
- self._onRequestFinished)
+ request = _registerResource(namespace, name, locktype,
+ self._onRequestFinished)
except ValueError as ex:
self.log.debug("%s: request for '%s' could not be processed
"
"(%s)", self, fullName, ex)
@@ -1002,7 +982,6 @@
self._ns = ns
self._name = name
self._mode = mode
- self._rm = ResourceManager.getInstance()
@property
def ns(self):
@@ -1017,45 +996,43 @@
return self._mode
def acquire(self):
- res = self._rm.acquireResource(self.ns, self.name, self.mode)
+ res = acquireResource(self.ns, self.name, self.mode)
# Locks are released by default then the last reference is garbage
# collected. Since we don't need the reference we'll just disable
# autoRelease.
res.autoRelease = False
def release(self):
- self._rm.releaseResource(self.ns, self.name)
+ releaseResource(self.ns, self.name)
+
+
+# The single resource manager - this instance is monkeypatched by the tests.
+_manager = ResourceManager()
# Public api - client should use only these to manage resources.
def registerNamespace(namespace, factory, force=False):
- manager = ResourceManager.getInstance()
- manager.registerNamespace(namespace, factory, force=force)
+ _manager.registerNamespace(namespace, factory, force=force)
def unregisterNamespace(namespace):
- manager = ResourceManager.getInstance()
- manager.unregisterNamespace(namespace)
+ _manager.unregisterNamespace(namespace)
def acquireResource(namespace, name, lockType, timeout=None):
- manager = ResourceManager.getInstance()
- return manager.acquireResource(namespace, name, lockType, timeout=timeout)
+ return _manager.acquireResource(namespace, name, lockType, timeout=timeout)
def releaseResource(namespace, name):
- manager = ResourceManager.getInstance()
- manager.releaseResource(namespace, name)
+ _manager.releaseResource(namespace, name)
# Private apis for the tests - clients should never use these!
def _registerResource(namespace, name, lockType, callback):
- manager = ResourceManager.getInstance()
- return manager.registerResource(namespace, name, lockType, callback)
+ return _manager.registerResource(namespace, name, lockType, callback)
def _getResourceStatus(namespace, name):
- manager = ResourceManager.getInstance()
- return manager.getResourceStatus(namespace, name)
+ return _manager.getResourceStatus(namespace, name)
--
To view, visit
https://gerrit.ovirt.org/65037
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I136ab52f15332f43f2b0ecd9e9a2ff7b558df87a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>