Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails

laravot at redhat.com laravot at redhat.com
Tue Mar 4 13:59:38 UTC 2014


Liron Ar has posted comments on this change.

Change subject: resourceManager: Keep resource state if registerResource fails
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.ovirt.org/#/c/25284/2/tests/resourceManagerTests.py
File tests/resourceManagerTests.py:

Line 502:     def testAcquireResourceFailureExclusive(self):
Line 503:         self.assertAcquireResourceFails("string", "test",
Line 504:                                         resourceManager.LockType.exclusive)
Line 505:         self.assertCanAcquireResource("string", "test",
Line 506:                                       resourceManager.LockType.exclusive)
perhaps add here a check that the resource can be acquired also in shared or it can be done in assertCanAcquireResource or a new method (see comment down below)
Line 507: 
Line 508:     def testAcquireResourceFailureExclusiveBusy(self):
Line 509:         with self.manager.acquireResource("string", "test",
Line 510:                                           resourceManager.LockType.shared, 0):


Line 508:     def testAcquireResourceFailureExclusiveBusy(self):
Line 509:         with self.manager.acquireResource("string", "test",
Line 510:                                           resourceManager.LockType.shared, 0):
Line 511:             self.assertAcquireResourceFails("string", "test",
Line 512:                                             resourceManager.LockType.exclusive)
1. this should be separated to a different patch, it tests generally the resource manager.

2. we don't need the monkeypatching in that usecase, acquiring the exclusive lock should always fail when we have the shared lock.
Line 513:         self.assertCanAcquireResource("string", "test",
Line 514:                                       resourceManager.LockType.exclusive)
Line 515: 
Line 516:     def testAcquireResourceFailureShared(self):


Line 516:     def testAcquireResourceFailureShared(self):
Line 517:         self.assertAcquireResourceFails("string", "test",
Line 518:                                         resourceManager.LockType.shared)
Line 519:         self.assertCanAcquireResource("string", "test",
Line 520:                                       resourceManager.LockType.exclusive)
perhaps add here a check that the resource can be acquired also in shared or it can be done in assertCanAcquireResource or a new method (see comment down below)
Line 521: 
Line 522:     def testAcquireResourceFailureSharedBusy(self):
Line 523:         with self.manager.acquireResource("string", "test",
Line 524:                                           resourceManager.LockType.shared, 0):


Line 524:                                           resourceManager.LockType.shared, 0):
Line 525:             self.assertAcquireResourceFails("string", "test",
Line 526:                                             resourceManager.LockType.shared)
Line 527:         self.assertCanAcquireResource("string", "test",
Line 528:                                       resourceManager.LockType.exclusive)
perhaps add here a check that the resource can be acquired also in shared or it can be done in assertCanAcquireResource or a new method (see comment down below)
Line 529: 
Line 530:     def testResourceStatuses(self):
Line 531:         manager = self.manager
Line 532:         self.assertEquals(manager.getResourceStatus("storage", "resource"),


Line 736:             raise
Line 737: 
Line 738:     # Helpers
Line 739: 
Line 740:     def assertAcquireResourceFails(self, namespace, name, locktype):
perhaps rename to assertAcquireResourceFailsInternally or something like that
Line 741:         class Failure(Exception):
Line 742:             pass
Line 743: 
Line 744:         def FakeRequestRef(*a, **kw):


Line 748:                 [(resourceManager, 'RequestRef', FakeRequestRef)]):
Line 749:             self.assertRaises(Failure, self.manager.acquireResource,
Line 750:                               namespace, name, locktype, 0)
Line 751: 
Line 752:     def assertCanAcquireResource(self, namespace, name, locktype):
this method can be refactored to check always both exclusive and shared lock (with the current use) or it can be done in the calls (see  previous comments).
Line 753:         with self.manager.acquireResource(namespace, name, locktype, 0):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I16abf41ebc8a8a99b292d38c945074752254a34b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel P. Berrange <berrange at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list