Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
automat...@ovirt.org has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: * Update tracker::IGNORE, no Bug-Url found -- To view, visit https://gerrit.ovirt.org/25284 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16abf41ebc8a8a99b292d38c945074752254a34b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has abandoned this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Abandoned Cannot be fix in current code. -- To view, visit https://gerrit.ovirt.org/25284 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I16abf41ebc8a8a99b292d38c945074752254a34b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Saggi, I think the order suggested in this patch *is* the logical order. Even if there were no logging calls inside, I would make the code atomic by first preparing for the change (create objects, etc.), and only if everything is ok, do the actual change (increase active count). Another option is to do the changes in try except block, and revert the locking changes on errors. This way we can keep the original order of the calls. -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Saggi Mizrahi has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: It's not about returning the RequestRef it's about hoping nothing fails between. contextCleanup.defer(request.emit,... and the activeCount bump. Switching those around would be better but in any case, I don't like moving things around and making them a harder to understand. Especially when it's code like that. The first thing someone new who is going to look at it is going to move things back around. But, I do hate silly deadlocks. Add comments about moving things back to their logical locations once we put failsafe on logging. -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Saggi, in registerResource, a new request object is created for each call. This object cannot be shared between different threads until it is returned from this function. So invoking request.grant() in registerResource cannot cause effect any other thread, and can be done before increasing the active count. Can you show the flow of events leading to such issue? -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Saggi Mizrahi has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Another possibility Maybe panic() in case of an unexpected error. -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Saggi Mizrahi has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Code-Review-1 This update is actually wrong. You can't call resource.grant() before incrementing the activeUsers count. grant() sets the _doneEvent() which might prompt another thread to act as if the resource is owned when an error is hit. It's impossible to do anything assuming the standard library can throw errors at any time as it causes to many unpredictable behaviors. The solution, sadly, is to trap the error in a wrapper to the logging or manually wrap every log call in try\except. -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/4/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:
Line 599: resource.activeUsers += 1
Line 600:
Line 601: self._log.debug("Resource '%s' is free. Now locking
as '%s' "
Line 602: "(1 active user)", fullName,
request.lockType)
Line 603: request.grant()
> Does this mean that if something breaks we have a granted request for a res
request.grant(), does *not* grant the resource to the caller, so we don't have
an issue failing after request.grant() was called.
What can be an issue, is the request.emit call, invoking the user callback.
This happens when exiting from this context, and should not happen something
was raised in this context - but I'm not sure this is the case - this code is
way too complex to reason about.
Line 604: ref = RequestRef(request)
Line 605: contextCleanup.defer(request.emit,
Line 606: ResourceRef(namespace, name,
Line 607: resource.realObj,
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Yoav Kleinberger
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Yoav Kleinberger has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Code-Review+1 -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Federico Simoncelli has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Saggi can you review this? Thanks. -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Federico Simoncelli has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/4/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:
Line 599: resource.activeUsers += 1
Line 600:
Line 601: self._log.debug("Resource '%s' is free. Now locking
as '%s' "
Line 602: "(1 active user)", fullName,
request.lockType)
Line 603: request.grant()
Does this mean that if something breaks we have a granted request for a
resource that wasn't recorded in the "resources" dictionary?
I have the feeling that resources[name] should remain where it was. Or maybe
you have to move the RequestRef creation before grant?
Line 604: ref = RequestRef(request)
Line 605: contextCleanup.defer(request.emit,
Line 606: ResourceRef(namespace, name,
Line 607: resource.realObj,
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Yoav Kleinberger
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Itamar Heim has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: ping? -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: ping! -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Waking up reviewers -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Liron Ar has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.ovirt.org/#/c/25284/4/tests/resourceManagerTests.py
File tests/resourceManagerTests.py:
Line 499: res1.release()
Line 500: res2.release()
Line 501:
Line 502: def testAcquireResourceFailureExclusive(self):
Line 503: self.assertAcquireResourceFails("string", "test",
> Good idea for another patch - this strings are used all over this file.
I'd at least add those in this patch and in later patch we could change the
whole file. but whatever you prefer.
Line 504:
resourceManager.LockType.exclusive)
Line 505: self.assertCanAcquireResource("string", "test")
Line 506:
Line 507: def testAcquireResourceFailureExclusiveShared(self):
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/4/tests/resourceManagerTests.py
File tests/resourceManagerTests.py:
Line 499: res1.release()
Line 500: res2.release()
Line 501:
Line 502: def testAcquireResourceFailureExclusive(self):
Line 503: self.assertAcquireResourceFails("string", "test",
> i suggest to export those to constants and to use them in all methods, beca
Good idea for another patch - this strings are used all over this file.
Line 504:
resourceManager.LockType.exclusive)
Line 505: self.assertCanAcquireResource("string", "test")
Line 506:
Line 507: def testAcquireResourceFailureExclusiveShared(self):
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Liron Ar has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/4/tests/resourceManagerTests.py
File tests/resourceManagerTests.py:
Line 499: res1.release()
Line 500: res2.release()
Line 501:
Line 502: def testAcquireResourceFailureExclusive(self):
Line 503: self.assertAcquireResourceFails("string", "test",
i suggest to export those to constants and to use them in all methods, because
wrongly sent string can cause to a false-postive reslt (success to acquire
while actually a lock is stuck).
Line 504:
resourceManager.LockType.exclusive)
Line 505: self.assertCanAcquireResource("string", "test")
Line 506:
Line 507: def testAcquireResourceFailureExclusiveShared(self):
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
oVirt Jenkins CI Server has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7410/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6617/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7519/ : SUCCESS -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 4: Changes: - Make pep8 happy - "timeout=0" is more clear than "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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
oVirt Jenkins CI Server has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 3: Code-Review-1 Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7408/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6615/ : FAILURE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7517/ : FAILURE -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/25284/2/tests/resourceManagerTests.py File tests/resourceManagerTests.py: 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 th The name is little too long for me. If we will have more asserts we can rethink this name. Line 741: class Failure(Exception): Line 742: pass Line 743: Line 744: def FakeRequestRef(*a, **kw): -- 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 Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi 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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 3: Changes: - When checking if resource can be acquired, check both exclusive and shared locks. For checking if resource is not locked, checking exclusive is enough, but checking also shared ensure that we this code path is not broken now. It does not cost anything and make the test code more readable. - Add new tests checking failure while exclusive lock is held -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
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
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-H
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 2: Changes: - Extract helper assertions - Move tests higher to acquireResource, so we don't have to change them if we kill registerResource. - Add test for acquiring a busy resource, hopefully covering all the code paths -- 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 Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
oVirt Jenkins CI Server has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 2: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7404/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6611/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7513/ : SUCCESS -- 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 Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/1/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:
Line 574:
Line 575: self._log.debug("Resource '%s' is currently
locked, "
Line 576: "Entering queue (%d in queue)",
Line 577: fullName, len(resource.queue) + 1)
Line 578: ref = RequestRef(request)
> i referred to line 578, if you moved it from the return statement it seems
It seems better to keep logging before attempting to change the resource.
Line 579: resource.queue.insert(0, request)
Line 580: return ref
Line 581:
Line 582: # TODO : Creating the object inside the namespace
lock causes
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Liron Ar has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/1/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:
Line 574:
Line 575: self._log.debug("Resource '%s' is currently
locked, "
Line 576: "Entering queue (%d in queue)",
Line 577: fullName, len(resource.queue) + 1)
Line 578: ref = RequestRef(request)
> Why? we always log before doing any action. I don't want to change this log
i referred to line 578, if you moved it from the return statement it seems
better to have it defined also before the log.
Line 579: resource.queue.insert(0, request)
Line 580: return ref
Line 581:
Line 582: # TODO : Creating the object inside the namespace
lock causes
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/25284/1/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:
Line 574:
Line 575: self._log.debug("Resource '%s' is currently
locked, "
Line 576: "Entering queue (%d in queue)",
Line 577: fullName, len(resource.queue) + 1)
Line 578: ref = RequestRef(request)
> please move this line to be before the log
Why? we always log before doing any action. I don't want to change this logging
behavior in this patch.
Line 579: resource.queue.insert(0, request)
Line 580: return ref
Line 581:
Line 582: # TODO : Creating the object inside the namespace
lock causes
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 1:
(3 comments)
http://gerrit.ovirt.org/#/c/25284/1/tests/resourceManagerTests.py
File tests/resourceManagerTests.py:
Line 28:
Line 29: import storage.resourceManager as resourceManager
Line 30: from testrunner import VdsmTestCase as TestCaseBase
Line 31: from testValidation import slowtest, stresstest
Line 32: import monkeypatch
> this import should be with the non vdsm imports group
This is a vdsm test utitily module, just like testrunner and testValidation. I
will move it so this group is sorted by imported name.
Line 33:
Line 34:
Line 35: class NullResourceFactory(resourceManager.SimpleResourceFactory):
Line 36: """
Line 221: # And it should not leave a locked resource
Line 222: with self.manager.acquireResource("string", "test",
Line 223:
resourceManager.LockType.exclusive,
Line 224: 0):
Line 225: pass
> the only difference between those methos is the lock type,
Good idea
Line 226:
Line 227: def testRegisterResourceFailureShared(self):
Line 228: # This regeisterion must fail
Line 229: with monkeypatch.MonkeyPatchScope(
Line 741:
Line 742:
Line 743: def FakeRequestRef(*a, **kw):
Line 744: """ Used to simulate failures when registering a resource """
Line 745: raise Failure()
> when you add the test helper method, those can be defined within.
Good idea
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Liron Ar has posted comments on this change.
Change subject: resourceManager: Keep resource state if registerResource fails
..
Patch Set 1:
(4 comments)
http://gerrit.ovirt.org/#/c/25284/1/tests/resourceManagerTests.py
File tests/resourceManagerTests.py:
Line 28:
Line 29: import storage.resourceManager as resourceManager
Line 30: from testrunner import VdsmTestCase as TestCaseBase
Line 31: from testValidation import slowtest, stresstest
Line 32: import monkeypatch
this import should be with the non vdsm imports group
Line 33:
Line 34:
Line 35: class NullResourceFactory(resourceManager.SimpleResourceFactory):
Line 36: """
Line 221: # And it should not leave a locked resource
Line 222: with self.manager.acquireResource("string", "test",
Line 223:
resourceManager.LockType.exclusive,
Line 224: 0):
Line 225: pass
the only difference between those methos is the lock type,
please export the common logic to a helper method that'll accept the lock type
as parameter.
Line 226:
Line 227: def testRegisterResourceFailureShared(self):
Line 228: # This regeisterion must fail
Line 229: with monkeypatch.MonkeyPatchScope(
Line 741:
Line 742:
Line 743: def FakeRequestRef(*a, **kw):
Line 744: """ Used to simulate failures when registering a resource """
Line 745: raise Failure()
when you add the test helper method, those can be defined within.
http://gerrit.ovirt.org/#/c/25284/1/vdsm/storage/resourceManager.py
File vdsm/storage/resourceManager.py:
Line 574:
Line 575: self._log.debug("Resource '%s' is currently
locked, "
Line 576: "Entering queue (%d in queue)",
Line 577: fullName, len(resource.queue) + 1)
Line 578: ref = RequestRef(request)
please move this line to be before the log
Line 579: resource.queue.insert(0, request)
Line 580: return ref
Line 581:
Line 582: # TODO : Creating the object inside the namespace
lock causes
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Daniel P. Berrange
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Liron Ar
Gerrit-Reviewer: Saggi Mizrahi
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
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
oVirt Jenkins CI Server has posted comments on this change. Change subject: resourceManager: Keep resource state if registerResource fails .. Patch Set 1: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7397/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6604/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7506/ : SUCCESS -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Daniel P. Berrange Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Ar Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: resourceManager: Keep resource state if registerResource fails
Nir Soffer has uploaded a new change for review.
Change subject: resourceManager: Keep resource state if registerResource fails
..
resourceManager: Keep resource state if registerResource fails
Previous code was increasing resource activeUsers counter, but
exceptions raised after that caused the method to fail, leaving a locked
resources that nobody can release. Such locked resource may lead to
failure of any pool operation, making the host non-operational, and
requiring a restart of vdsm.
The failure in the field was caused by Python logging bug, raising
OSError when message was logged when log file was rotated. However, such
failure can happen everywhere, and locking code must be written in such
way that failure would never leave a resource locked.
This patch ensure that resource is added and activeUsers counter is
increased only if everything else was fine.
Since simulating logging error is hard, the tests monkeypatch the
RequestRef class to simulate a failure. This is little ugly, depending
on internal implementation detail, but I could not find a cleaner way.
Change-Id: I16abf41ebc8a8a99b292d38c945074752254a34b
Relates-To: https://bugzilla.redhat.com/1065650
Signed-off-by: Nir Soffer
---
M tests/resourceManagerTests.py
M vdsm/storage/resourceManager.py
2 files changed, 50 insertions(+), 9 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/84/25284/1
diff --git a/tests/resourceManagerTests.py b/tests/resourceManagerTests.py
index 01b0669..e2b6461 100644
--- a/tests/resourceManagerTests.py
+++ b/tests/resourceManagerTests.py
@@ -29,6 +29,7 @@
import storage.resourceManager as resourceManager
from testrunner import VdsmTestCase as TestCaseBase
from testValidation import slowtest, stresstest
+import monkeypatch
class NullResourceFactory(resourceManager.SimpleResourceFactory):
@@ -209,6 +210,32 @@
ex.__class__.__name__)
self.fail("Managed to access an attribute not exposed by wrapper")
+
+def testRegisterResourceFailureExclusive(self):
+# This regeisterion must fail
+with monkeypatch.MonkeyPatchScope(
+[(resourceManager, 'RequestRef', FakeRequestRef)]):
+self.assertRaises(Failure, self.manager.registerResource, "string",
+ "test", resourceManager.LockType.exclusive, None)
+
+# And it should not leave a locked resource
+with self.manager.acquireResource("string", "test",
+ resourceManager.LockType.exclusive,
+ 0):
+pass
+
+def testRegisterResourceFailureShared(self):
+# This regeisterion must fail
+with monkeypatch.MonkeyPatchScope(
+[(resourceManager, 'RequestRef', FakeRequestRef)]):
+self.assertRaises(Failure, self.manager.registerResource, "string",
+ "test", resourceManager.LockType.shared, None)
+
+# And it should not leave a locked resource
+with self.manager.acquireResource("string", "test",
+ resourceManager.LockType.exclusive,
+ 0):
+pass
def testAccessAttributeNotExposedByRequestRef(self):
resources = []
@@ -705,3 +732,14 @@
except:
resourceManager.ResourceManager._instance = None
raise
+
+# Helpers
+
+
+class Failure(Exception):
+""" Unique exception for testing """
+
+
+def FakeRequestRef(*a, **kw):
+""" Used to simulate failures when registering a resource """
+raise Failure()
diff --git a/vdsm/storage/resourceManager.py b/vdsm/storage/resourceManager.py
index 1be1450..ce144cf 100644
--- a/vdsm/storage/resourceManager.py
+++ b/vdsm/storage/resourceManager.py
@@ -559,23 +559,25 @@
if len(resource.queue) == 0 and \
resource.currentLock == LockType.shared and \
request.lockType == LockType.shared:
-resource.activeUsers += 1
self._log.debug("Resource '%s' found in shared state "
"and queue is empty, Joining current "
"shared lock (%d active users)",
-fullName, resource.activeUsers)
+fullName, resource.activeUsers + 1)
request.grant()
+ref = RequestRef(request)
contextCleanup.defer(request.emit,
ResourceRef(namespace, name,
resource.realObj,
request.reqID))
-return RequestRef(request)
+
