Liron Aravot has posted comments on this change.

Change subject: implementing StorageDomain.movePV
......................................................................


Patch Set 2:

(19 comments)

https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

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 su
I don't see a reason to not support old domain unless its too complicated, Done.
Line 564:             self.updateMapping()
Line 565: 
Line 566:     def reduceDevice(self, guid):
Line 567:         self._validateNotMetadataPV(guid)


Line 562:         with self._extendlock:
Line 563:             lvm.movePV(self.sdUUID, guid)
Line 564:             self.updateMapping()
Line 565: 
Line 566:     def reduceDevice(self, guid):
> I think should call this reduce(), similar to extend().
Done and moved to the correct patch (it was accidentally added to this one)
Line 567:         self._validateNotMetadataPV(guid)
Line 568:         with self._extendlock:
Line 569:             lvm.reduceVG(self.sdUUID, guid)
Line 570:             self.updateMapping()


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 i
Well, on that case the engine will fail the flow, if the user will try the 
operation again we should fail with an error indicating that the pv isn't part 
of the VG - when getting that error the engine should update the db accordingly.
That should be similar to deleting an image that has been already deleted.
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?
Done
Line 574:         if lvm.isSamePV(lvm_metadata_pv, guid):
Line 575:             raise 
se.CouldNotPerformOperationOnStorageDomainMetadataPV(guid)
Line 576: 
Line 577:     def getVolumeClass(self):


Line 570:             self.updateMapping()
Line 571: 
Line 572:     def _validateNotMetadataPV(self, guid):
Line 573:         lvm_metadata_pv, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA)
Line 574:         if lvm.isSamePV(lvm_metadata_pv, guid):
> We don't need this helper. 
Done
Line 575:             raise 
se.CouldNotPerformOperationOnStorageDomainMetadataPV(guid)
Line 576: 
Line 577:     def getVolumeClass(self):
Line 578:         """


Line 571: 
Line 572:     def _validateNotMetadataPV(self, guid):
Line 573:         lvm_metadata_pv, _ = lvm.getFirstExt(self.sdUUID, sd.METADATA)
Line 574:         if lvm.isSamePV(lvm_metadata_pv, guid):
Line 575:             raise 
se.CouldNotPerformOperationOnStorageDomainMetadataPV(guid)
> Error name is too long - how about CannotChangeMetadataPV?
I've shortened it by changing to CouldNotPerformOperationOnMetadataPV
Line 576: 
Line 577:     def getVolumeClass(self):
Line 578:         """
Line 579:         Return a type specific volume generator object


https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 780:         :type sdUUID: UUID
Line 781:         :param guid: A block device GUID
Line 782:         :type guid: str
Line 783:         """
Line 784:         job = sdm.api.move_sd_device_data.Job(jobUUID, 1, sdUUID, 
guid)
> Need to document why host_id=1 is correct in this case. If we have more cod
Done
Line 785:         self.sdm_schedule(job)
Line 786: 
Line 787:     def _deatchStorageDomainFromOldPools(self, sdUUID):
Line 788:         # We are called with blank pool uuid, to avoid changing 
exiting


https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 692:     return pv
Line 693: 
Line 694: 
Line 695: def isSamePV(pv_a, pv_b):
Line 696:     return _fqpvname(pv_a) == _fqpvname(pv_b)
> I don't like this normalization orgy. The module should accept only guid or
Done
Line 697: 
Line 698: 
Line 699: def _createpv(devices, metadataSize, options=tuple()):
Line 700:     """


Line 889: 
Line 890: def _hasDataToMove(pvName):
Line 891:     pv = getPV(pvName)
Line 892:     if pv.pe_alloc_count == "0":
Line 893:         return False
> This helper is not very useful in this module, so better inline it in the f
well, it shortens the caller function and removes from it this not very 
interesting code.
Line 894: 
Line 895:     return True
Line 896: 
Line 897: def movePV(vgName, guid):


Line 901:     Raises se.CouldNotMovePhysicalVolumeData if pvmove fails
Line 902:     """
Line 903:     pvName = _fqpvname(guid)
Line 904:     if not _hasDataToMove(pvName):
Line 905:         return
> Inlining _hasDataToMove is more clear:
please reconsider, i believe that having it  _hasDataToMove explains this check 
and makes the code here clearer.
Line 906: 
Line 907:     cmd = ["pvmove", pvName]
Line 908:     rc, out, err = _lvminfo.cmd(cmd)
Line 909:     if rc != 0:


Line 904:     if not _hasDataToMove(pvName):
Line 905:         return
Line 906: 
Line 907:     cmd = ["pvmove", pvName]
Line 908:     rc, out, err = _lvminfo.cmd(cmd)
> We must use filter including the devices belonging to this vg. Otherwise lv
Done
Line 909:     if rc != 0:
Line 910:         raise se.CouldNotMovePhysicalVolumeData(pvName, err)
Line 911: 
Line 912:     _lvminfo._invalidatepvs(pvName)


Line 906: 
Line 907:     cmd = ["pvmove", pvName]
Line 908:     rc, out, err = _lvminfo.cmd(cmd)
Line 909:     if rc != 0:
Line 910:         raise se.CouldNotMovePhysicalVolumeData(pvName, err)
> Exception name is too long, can we shorten it?
Done
Line 911: 
Line 912:     _lvminfo._invalidatepvs(pvName)
Line 913:     _lvminfo._invalidatevgs(vgName)
Line 914: 


Line 909:     if rc != 0:
Line 910:         raise se.CouldNotMovePhysicalVolumeData(pvName, err)
Line 911: 
Line 912:     _lvminfo._invalidatepvs(pvName)
Line 913:     _lvminfo._invalidatevgs(vgName)
> We need to invalidate all lvs in to this vg, since some of them has moved t
Done
Line 914: 
Line 915: 
Line 916: def getVG(vgName):
Line 917:     vg = _lvminfo.getVg(vgName)  # returns single VG namedtuple


https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 367:     def resizePV(self, guid):
Line 368:         pass
Line 369: 
Line 370:     def movePV(self, guid):
Line 371:         pass
> This should raise UnsupportedOperation, not pass. I know old code is doing 
Done
Line 372: 
Line 373:     def getFormat(self):
Line 374:         return str(self.getVersion())
Line 375: 


https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/sdm/api/Makefile.am
File vdsm/storage/sdm/api/Makefile.am:

Line 26:        __init__.py \
Line 27:        base.py \
Line 28:        copy_data.py \
Line 29:        create_volume.py \
Line 30:        move_sd_device_data.py \
> This is too long for a module name. Module names should be one word, and in
changed to move_dev_data, move_data is too general imo - let me know if you to 
change it.


https://gerrit.ovirt.org/#/c/63270/2/vdsm/storage/sdm/api/move_sd_device_data.py
File vdsm/storage/sdm/api/move_sd_device_data.py:

Line 34:     """
Line 35:     log = logging.getLogger('storage.move_sd_device_data')
Line 36: 
Line 37:     def __init__(self, job_id, host_id, sd_id, device):
Line 38:         super(Job, self).__init__(job_id, 'move_sd_device_data', 
host_id)
> We need shorter name.
Done
Line 39:         self.device = device
Line 40:         self.sd_id = sd_id
Line 41: 
Line 42:     def _run(self):


Line 36: 
Line 37:     def __init__(self, job_id, host_id, sd_id, device):
Line 38:         super(Job, self).__init__(job_id, 'move_sd_device_data', 
host_id)
Line 39:         self.device = device
Line 40:         self.sd_id = sd_id
> There is no validation here. Please use params dict and object with peopert
Done
Line 41: 
Line 42:     def _run(self):
Line 43:         if self._status == jobs.STATUS.ABORTED:
Line 44:             return


Line 40:         self.sd_id = sd_id
Line 41: 
Line 42:     def _run(self):
Line 43:         if self._status == jobs.STATUS.ABORTED:
Line 44:             return
> This is not needed, since you do not implement _abort(). See latest jobs.py
thanks, I'll get rid from it on the next rebase.
Line 45: 
Line 46:         sd_manifest = sdCache.produce_manifest(self.sd_id)
Line 47:         with sd_manifest.domain_lock(self.host_id):
Line 48:             rmanager = rm.ResourceManager.getInstance()


Line 43:         if self._status == jobs.STATUS.ABORTED:
Line 44:             return
Line 45: 
Line 46:         sd_manifest = sdCache.produce_manifest(self.sd_id)
Line 47:         with sd_manifest.domain_lock(self.host_id):
> This assumes that you already acquired a host id, but if this domain is not
Done
Line 48:             rmanager = rm.ResourceManager.getInstance()
Line 49:             with rmanager.acquireResource(STORAGE, self.sd_id, 
rm.LockType.exclusive):


-- 
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 <lara...@redhat.com>
Gerrit-Reviewer: Adam Litke <ali...@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

Reply via email to