Nir Soffer has posted comments on this change. Change subject: StorageDomain.getInfo - report vg metadata device for block sd ......................................................................
Patch Set 7: (5 comments) https://gerrit.ovirt.org/#/c/64433/7/lib/vdsm/storage/exception.py File lib/vdsm/storage/exception.py: Line 1558: Line 1559: Line 1560: class UnexpectedVolumeGroupMetadata(StorageException): Line 1561: def __init__(self, err): Line 1562: self.value = "err=%s" % err reason=? Line 1563: code = 616 Line 1564: message = "Volume Group metadata isn't as expected" Line 1565: Line 1566: https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py: Line 451: """ Line 452: dev, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA) Line 453: return os.path.basename(dev) Line 454: Line 455: def getVgMetadataDevice(self): Add docstring similar to getMetadataLVDevice (see my comment in the previous patch). Line 456: return os.path.basename(lvm.getVgMetadataPv(self.sdUUID)) Line 457: Line 458: @classmethod Line 459: def getMetaDataMapping(cls, vgName, oldMapping={}): https://gerrit.ovirt.org/#/c/64433/7/vdsm/storage/lvm.py File vdsm/storage/lvm.py: Line 588: return vgs.values() Line 589: Line 590: def getVgPvs(self, vgName): Line 591: """ Line 592: Returns the pvs of the given vg ... reloading pvs only once if some of the pvs are missing or stale. Hopefully we can use this elsewhere instead of calling multiple getPv calls. I wonder if this should be be renamed to getPvs and moved next to getPv and getAllPvs - The common behavior is method returning pv objects. Line 593: """ Line 594: stalepvs = [] Line 595: vgpvs = [] Line 596: vg = self.getVg(vgName) Line 594: stalepvs = [] Line 595: vgpvs = [] Line 596: vg = self.getVg(vgName) Line 597: for pvName in vg.pv_name: Line 598: pv = self._pvs.get(pvName) One issue here, you are accessing a dict which may be modified by other theads on both spm/hsm. The methods in the cache accessing this dict typically take the lock (see reload pvs). Not all code is thread safe here, but when adding new code we better be thread safe. Can be easily fixed by locking the part getting stale pvs: with self._lock: # find vgpvs and stalepvs here # reload outside of the lock (otherwise it will deadlock) Line 599: if pv is None or isinstance(pv, Stub): Line 600: stalepvs.append(pvName) Line 601: else: Line 602: vgpvs.append(pv) Line 1341: pvs = _lvminfo.getVgPvs(vgName) Line 1342: mdpvs = [pv for pv in pvs if not isinstance(pv, Stub) and _isMetadataPv(pv)] Line 1343: if len(mdpvs) != 1: Line 1344: se.UnexpectedVolumeGroupMetadata("Expected one metadata pv in vg: %s, " Line 1345: "vg pvs: %s" % (vgName, pvs)) Errors looks fine, raising it would be even better :-) Line 1346: return mdpvs[0] Line 1347: Line 1348: def _isMetadataPv(pv): Line 1349: """ -- To view, visit https://gerrit.ovirt.org/64433 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a7763d2ab7d796be633ecd69f661cba96e29dde Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
