Liron Aravot has posted comments on this change. Change subject: StorageDomain.getInfo - report vg metadata device for block sd ......................................................................
Patch Set 7: (4 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=? Done 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 previou Done 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. 1. Done 2. moved and renamed to getPvs 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 the I'm not sure that's needed. Currently the methods that take the lock are the one's the modify the dicts and the objects in them - so that look is essentially a write lock only (except GetVGDevs). In this method we don't modify the dict, only _reloadpvs does and it takes the lock when it does. acquiring the lock here means that we'll here on the different operations - do we really need it? I can see that you actually changed the lock to be a write lock in change I86153c34c3ddc4055bb0679ce46aa48757d9cae1 :) I'd start by adding the method as is, keeping the same semantics - if we'll see its needed we can add it (though i believe it doesn't). Line 599: if pv is None or isinstance(pv, Stub): Line 600: stalepvs.append(pvName) Line 601: else: Line 602: vgpvs.append(pv) -- 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
