Adam Litke has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 11: (12 comments) https://gerrit.ovirt.org/#/c/61435/11//COMMIT_MSG Commit Message: Line 29: mode (in that order) Line 30: - Due to namespace sorting, ResourceManager locks are taken before Line 31: volume leases Line 32: - Duplicate locks are removed (only when ns, name, and mode match) Line 33: - Locks are acquired in sorted order and released in reverse order > We accept lock lists now, not entities, should update to describe the imple Done Line 34: Line 35: Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 https://gerrit.ovirt.org/#/c/61435/11/lib/vdsm/storage/guarded.py File lib/vdsm/storage/guarded.py: Line 19: # Line 20: Line 21: from __future__ import absolute_import Line 22: import logging Line 23: import six > six is not stdlib module, should be imported after stdlib group. Done Line 24: import sys Line 25: Line 26: from vdsm.storage import constants as sc Line 27: Line 21: from __future__ import absolute_import Line 22: import logging Line 23: import six Line 24: import sys Line 25: > Import six here, separate from vdsm imports. Done Line 26: from vdsm.storage import constants as sc Line 27: Line 28: from storage import resourceManager as rm Line 29: from storage import sd Line 26: from vdsm.storage import constants as sc Line 27: Line 28: from storage import resourceManager as rm Line 29: from storage import sd Line 30: from storage.sdc import sdCache > We cannot import these from lib/vdsm/storage/. We have to move storage modu Done Line 31: Line 32: log = logging.getLogger('storage.guarded') Line 33: rmanager = rm.ResourceManager.getInstance() Line 34: Line 55: in reverse order. Errors are handled as gracefully as possible with any Line 56: acquired locks being released in the proper order. Line 57: """ Line 58: Line 59: def __init__(self, *lock_lists): > Why not accept iterable of locks? This syntax force the caller to create a Ok. I think this will work well and make this module simpler. Line 60: """ Line 61: Receives a variable number of lock lists. The lists must contain only Line 62: objects which descend from AbstractLock. All of the lists are combined Line 63: into a single list which is deduplicated and sorted. Line 61: Receives a variable number of lock lists. The lists must contain only Line 62: objects which descend from AbstractLock. All of the lists are combined Line 63: into a single list which is deduplicated and sorted. Line 64: """ Line 65: self._locks = sorted(set(reduce(lambda x, y: x + y, lock_lists, []))) > To complicated, please avoid reduce(). Yeah, this will be fine. Changing. Line 66: self._held_locks = [] Line 67: Line 68: def __enter__(self): Line 69: for lock in self._locks: Line 70: try: Line 71: lock.acquire() Line 72: except: Line 73: exc = sys.exc_info() Line 74: log.exception("Error acquiring lock %r", lock) > Since we raise the current exception, we should not log a traceback here. W Done Line 75: try: Line 76: self._release() Line 77: except ReleaseError: Line 78: log.exception("Error releasing locks") Line 75: try: Line 76: self._release() Line 77: except ReleaseError: Line 78: log.exception("Error releasing locks") Line 79: six.reraise(exc[0], exc[1], exc[2]) > Can't we use *exc? Done Line 80: self._held_locks.append(lock) Line 81: return self Line 82: Line 83: def __exit__(self, exc_type, exc_val, exc_tb): Line 86: except ReleaseError: Line 87: if exc_type is not None: Line 88: log.exception("Error releasing locks") Line 89: return False Line 90: raise > Better keep the happy path idiom, first handle errors, then the normal (err Done Line 91: Line 92: def _release(self): Line 93: errors = [] Line 94: while self._held_locks: Line 100: if errors: Line 101: raise ReleaseError(errors) Line 102: Line 103: Line 104: class AbstractLock(object): > We can test this module by creating one or more concrete lock objects, all This is handled by the FakeGuardedLock tests. Line 105: @property Line 106: def ns(self): Line 107: raise NotImplementedError Line 108: Line 135: def _key(self): Line 136: return type(self), self.ns, self.name, self.mode Line 137: Line 138: Line 139: class ResourceManagerLock(AbstractLock): > We can fix the import issues by implementing this in vdsm/storage/resourceM Done Line 140: def __init__(self, ns, name, mode): Line 141: self._ns = ns Line 142: self._name = name Line 143: self._mode = mode Line 160: def release(self): Line 161: rmanager.releaseResource(self.ns, self.name) Line 162: Line 163: Line 164: class VolumeLease(AbstractLock): > We can fix the import issues by implementing this in vdsm/storage/sd.py or Done Line 165: def __init__(self, host_id, sd_id, img_id, vol_id): Line 166: self._host_id = host_id Line 167: self._sd_id = sd_id Line 168: self._img_id = img_id -- To view, visit https://gerrit.ovirt.org/61435 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
