Nir Soffer has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 2: (10 comments) https://gerrit.ovirt.org/#/c/61435/2//COMMIT_MSG Commit Message: Line 23: Line 24: The following rules are currently enforced: Line 25: - Locks are collected from all entities and sorted Line 26: - Duplicate locks are removed Line 27: - Exclusive mode takes precedence over shared mode We removed this feature, right? Line 28: Line 29: Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 https://gerrit.ovirt.org/#/c/61435/2/lib/vdsm/storage/guarded.py File lib/vdsm/storage/guarded.py: Line 34: Provides a guarded context associated with the given entities. Each Line 35: entity must provide a locks property which holds an iterable of Line 36: guarded.Lock objects. The locks associated with the entities are Line 37: filtered for duplicates and sorted according to vdsm locking rules. Line 38: """ We need a class docstring to explain about this class, here we describe the entities argument. Line 39: self._locks = self._collect_locks(entities) Line 40: self._held_locks = [] Line 41: Line 42: def __enter__(self): Line 44: for lock in self._locks: Line 45: lock.acquire() Line 46: self._held_locks.append(lock) Line 47: except: Line 48: self._release() Here we always want to log release failures, so: try: self._release() except: logging.exception(...) raise Line 49: raise Line 50: Line 51: def __exit__(self, exc_type, exc_val, exc_tb): Line 52: self._release() Line 47: except: Line 48: self._release() Line 49: raise Line 50: Line 51: def __exit__(self, exc_type, exc_val, exc_tb): If releasing some locks failed, and there was no exception in the context (exc_type == None, exc_val == None, exc_tb == None), we should fail with the first error inside _release. If there was an exception in the context, and we are cleaning up before raising it, we should not release failures, as it will hide the original error. In this case, we like to log release failures. So maybe _release should raise an error with a description of the failures, instead of logging the failures. Then, we can do here: try: self._release() except: if exc_type is None: raise log.exception("Error releasing locks") Line 52: self._release() Line 53: Line 54: def _collect_locks(self, entities): Line 55: locks = set() Line 62: lock = self._held_locks.pop() Line 63: try: Line 64: lock.release() Line 65: except: Line 66: log.exception("Exception while releasing lock %r", lock) One issue, we may like to report release failure if called from __exit__. This is very similar to utils.RollbackContext. I think the best thing to do here is to collect the errors and raise an error describing the failures: errors = [] while self._held_locks: lock.self._held_locks.pop() try: lock.release() except Exception as e: errors.append(e) if errors: raise ReleaseError(errors) Line 67: Line 68: Line 69: class Lock(object): Line 70: def __init__(self, ns, name, mode): Line 65: except: Line 66: log.exception("Exception while releasing lock %r", lock) Line 67: Line 68: Line 69: class Lock(object): Lets document the purpose of this class, adding uniform interface to different kinds of locks (resourceManager locks, clusterlock leases). Line 70: def __init__(self, ns, name, mode): Line 71: self.ns = ns Line 72: self.name = name Line 73: self.mode = mode Line 69: class Lock(object): Line 70: def __init__(self, ns, name, mode): Line 71: self.ns = ns Line 72: self.name = name Line 73: self.mode = mode Having __init__ in the base class will not work well for other lock types. Lets move down init to the concrete classes, so we have: class ResourceManagerLock(Lock): def __init__(self, ns, name, mode): ... class VolumeLease(Lock): def __init__(self, sd, img_id, vol_id): ... @property def name(self): return self.vol_id @property def ns(self): return "00_cluster" I would start with single class - simplest code needed now, and add the base class later when we add the second class, if needed. If you want to work now on the base class, please also the other class that require it. Otherwise we will make the wrong design. Line 74: Line 75: def acquire(self): Line 76: raise NotImplementedError Line 77: Line 87: def __lt__(self, other): Line 88: return (self.ns, self.name) < (other.ns, other.name) Line 89: Line 90: def __hash__(self): Line 91: return hash(self._key()) Hash should include also the type. Line 92: Line 93: def _key(self): Line 94: return self.ns, self.name, self.mode Line 95: Line 90: def __hash__(self): Line 91: return hash(self._key()) Line 92: Line 93: def _key(self): Line 94: return self.ns, self.name, self.mode We can fix __hash__ and simplify __eq__ like this: def _key(self): return (type(self), self.ns, self.name, self.mode) Then __eq__ can be: return self._key() == other._key() And hash: return hash(self._key()) Line 95: Line 96: Line 97: class ResourceManagerLock(Lock): Line 98: def __init__(self, ns, name, mode): Line 95: Line 96: Line 97: class ResourceManagerLock(Lock): Line 98: def __init__(self, ns, name, mode): Line 99: super(ResourceManagerLock, self).__init__(ns, name, mode) We do nothing here, so we don't need __init__. Line 100: Line 101: def acquire(self): Line 102: rmanager.acquireResource(self.ns, self.name, self.mode) Line 103: -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI 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]
