Nir Soffer has posted comments on this change. Change subject: virt: Add getReplicaXML method ......................................................................
Patch Set 9: (5 comments) https://gerrit.ovirt.org/#/c/40068/9/tests/vmStorageTests.py File tests/vmStorageTests.py: Line 140: <source dev="/path/to/replica"/> Line 141: <driver cache="none" error_policy="stop" Line 142: io="native" name="qemu" type="qcow2"/> Line 143: </disk> Line 144: """ > Since what you're checking here is hidden in xml can you add a comment on t Good idea, I will improve the documentation in the next version. Line 145: self.check({}, conf, xml, is_block_device=True) Line 146: Line 147: def test_block_to_file(self): Line 148: conf = drive_config( https://gerrit.ovirt.org/#/c/40068/9/vdsm/virt/vmdevices/storage.py File vdsm/virt/vmdevices/storage.py: Line 398: Line 399: Line 400: def _getSourceXML(drive): Line 401: source = vmxml.Element('source') Line 402: if drive["diskType"] == DISK_TYPE.BLOCK: > Is it necessary to translate self.diskType => drive["diskType"] Where do you see self.diskType? I can find only one instance in line 345. Line 403: source.setAttrs(dev=drive["path"]) Line 404: elif drive["diskType"] == DISK_TYPE.NETWORK: Line 405: info = drive["volumeInfo"] Line 406: source.setAttrs(protocol=info['protocol'], Line 401: source = vmxml.Element('source') Line 402: if drive["diskType"] == DISK_TYPE.BLOCK: Line 403: source.setAttrs(dev=drive["path"]) Line 404: elif drive["diskType"] == DISK_TYPE.NETWORK: Line 405: info = drive["volumeInfo"] > You added info, but I think the least we change the better it is. You can r I agree that it could be better to avoid this change for easier review, but I don't want to introduce another trivial cleanup patch, specially when this code will die soon (see https://gerrit.ovirt.org/38766) Line 406: source.setAttrs(protocol=info['protocol'], Line 407: name=info['path']) Line 408: hostAttrs = {'name': info['volfileServer'], Line 409: 'port': info['volPort'], Line 421: def _getDriverXML(drive): Line 422: driver = vmxml.Element('driver') Line 423: driverAttrs = {'name': 'qemu'} Line 424: Line 425: if drive['diskType'] == DISK_TYPE.BLOCK: > Are you sure that this change is ok? (aren't you introducing diskType later This change *must* be here, because the function was using self.blockDev, but now it must work with replica dict that does not have a "blockDev" key. Since both the replica dict and the drive object have "diskType", I'm using it here. self.blockDev depends on diskType != "network" and self._blockDev. I'm working on simplifying this (see https://gerrit.ovirt.org/40472). The goal is to eliminate both networkDev and blockDev, and keep only _blockDev for supporting legacy engine that do not send diskType: def diskType(self): # New code must send a diskType if hasattr(self, "_diskType"): return self._diskType # Backward compatibility for clients not sending diskType, supporting # only file or block types. if self._blockDev is None: self._blockDev = ... return DISK_TYPE.BLOCK if self._blockDev else DISK_TYPE.FILE When migrating from one type to another, the drive.diskType will be updated by the migration code. Line 426: driverAttrs['io'] = 'native' Line 427: else: Line 428: driverAttrs['io'] = 'threads' Line 429: Line 431: driverAttrs['type'] = 'qcow2' Line 432: elif drive['format']: Line 433: driverAttrs['type'] = 'raw' Line 434: Line 435: try: > This block changed significantly given the attribute => key switch. It seem This is like the self.volumeInfo change - code became too ugly when you need to check and get multiple levels of dictionaries, and using direct dictionary lookup simplify things. I can keep the old code if you like and add a trivial patch applying this change and add the missing tests if you prefer. Line 436: driverAttrs['iothread'] = str(drive['specParams']['pinToIoThread']) Line 437: except KeyError: Line 438: pass Line 439: -- To view, visit https://gerrit.ovirt.org/40068 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iff17e0bfc86fbff9c817a2f126fb71706396833e Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Fred Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
