Nir Soffer has posted comments on this change. Change subject: implementing StorageDomain.movePV ......................................................................
Patch Set 2: (5 comments) https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py: Line 545 Line 546 Line 547 Line 548 Line 549 Same... Line 552 Line 553 Line 554 Line 555 Line 556 Same issue with successful lvm command and failure to update mapping in old code... Line 559: Line 560: def movePV(self, guid): Line 561: self._validateNotMetadataPV(guid) Line 562: with self._extendlock: Line 563: lvm.movePV(self.sdUUID, guid) updateMapping should be needed only for domain version 0 - do we want to support removing devices on such domains? I guess not. If we want to support old domains, we must run update mapping even if movePV failed, since some extents may have moved, changing the pv mapping. Note that if both movePV and updateMapping failed, we should raise the original error from movePV, with the original traceback. We can do this: try: lvm.movePV(self.sdUUID, guid) except Exception: exc = sys.exc_info() else: exc = None try: self.updateMapping() except Exception: if exc is None: raise log.exception("descrption...") if exc: try: six.reraise(*exc) finally: del exc If updateMapping failed, we are left with incorrect mapping, not sure the current code can handle this case. Line 564: self.updateMapping() Line 565: Line 566: def reduceDevice(self, guid): Line 567: self._validateNotMetadataPV(guid) Line 565: Line 566: def reduceDevice(self, guid): Line 567: self._validateNotMetadataPV(guid) Line 568: with self._extendlock: Line 569: lvm.reduceVG(self.sdUUID, guid) No error handling here is fine, assuming that reduceVG remove the pv only if it was successful. However, what engine is going to do if reduceVG succeed, but updateMapping failed? the call will fail, but the pv was removed. Line 570: self.updateMapping() Line 571: Line 572: def _validateNotMetadataPV(self, guid): Line 573: lvm_metadata_pv, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA) Line 569: lvm.reduceVG(self.sdUUID, guid) Line 570: self.updateMapping() Line 571: Line 572: def _validateNotMetadataPV(self, guid): Line 573: lvm_metadata_pv, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA) Why the lvm_ prefix? do we have other kind of metadata_pv? Line 574: if lvm.isSamePV(lvm_metadata_pv, guid): Line 575: raise se.CouldNotPerformOperationOnStorageDomainMetadataPV(guid) Line 576: Line 577: def getVolumeClass(self): -- To view, visit https://gerrit.ovirt.org/63270 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I74183d13061d114a59da23874c86186457046e94 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Liron Aravot <[email protected]> Gerrit-Reviewer: Adam Litke <[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]
