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 <nsof...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Daniel P. Berrange <berra...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches