From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: storage: move to lib/vdsm ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/78070/3/lib/vdsm/storage/blockVolume.py File lib/vdsm/storage/blockVolume.py:
Line 304: def newVolumeLease(cls, metaId, sdUUID, volUUID): Line 305: # pylint: disable=no-member Line 306: cls.log.debug("Initializing volume lease volUUID=%s sdUUID=%s, " Line 307: "metaId=%s", volUUID, sdUUID, metaId) Line 308: manifest = vdsm.storage.blockSD.BlockStorageDomainManifest(sdUUID)
please see my comment in the other file.
I think that I did not understand the problem also when you explained it there.
You cannot use "from vdsm.storage import foo" because of which circular dependency? How does your currently suggested code works around that circular mess?
You don't need to solve all of the existing problems in the code. Moving the code with its illnesses under lib/vdsm/storage is a good step on its own right. But it would be great if you document them more clearly. E.g., have a separate patch saying "Due to a reason unknown to me, pylint complains about cls.log after we move the code to lib/vdsm/storage. This patch silences these errors".
If you cannot break it into a separate patch, a second-best is to add a paragraph about it in the commit message.
Similarly, if you cannot avoid module rename, please explain why. Line 309: metaSdUUID, mdSlot = metaId Line 310: Line 311: leasePath = manifest.getLeasesFilePath() Line 312: leaseOffset = ((mdSlot + RESERVED_LEASES) *
vdsm-patches@lists.fedorahosted.org