Fabian Deutsch has posted comments on this change. Change subject: Move multipath device checking to ovirt.node.utils.storage ......................................................................
Patch Set 15: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/19951/15/src/ovirt/node/utils/storage.py File src/ovirt/node/utils/storage.py: Line 120: # FIXME explain ... Line 121: name = "/dev/%s" % name.rstrip('0123456789') Line 122: self._cached_live_disk_name = name Line 123: return name Line 124: If this function get's moved here - and I am in favor of this - then we should also update the logic a bit to make it modern and improve it's readability. Line 125: def translate_device_name(self, _dev): Line 126: """Discover whether a device can be mapped to a devicemapper name so Line 127: we can match arbitrary /dev/sd? names, such as flash drives Line 128: """ Line 121: name = "/dev/%s" % name.rstrip('0123456789') Line 122: self._cached_live_disk_name = name Line 123: return name Line 124: Line 125: def translate_device_name(self, _dev): Let's add a doctext. I#d also consider to rename it to canonical_path() Also, we do not need _dev if it's a member function of class Device Line 126: """Discover whether a device can be mapped to a devicemapper name so Line 127: we can match arbitrary /dev/sd? names, such as flash drives Line 128: """ Line 129: self.logger.debug("Translating %s") Line 126: """Discover whether a device can be mapped to a devicemapper name so Line 127: we can match arbitrary /dev/sd? names, such as flash drives Line 128: """ Line 129: self.logger.debug("Translating %s") Line 130: if _dev is None: In case of problems we should raise errors. Line 131: return False Line 132: if "/dev/cciss" in _dev: Line 133: _dev = "/dev/mapper/%s" % process.check_output(["cciss_id", _dev]) Line 134: if not "/dev/mapper" in _dev: Line 129: self.logger.debug("Translating %s") Line 130: if _dev is None: Line 131: return False Line 132: if "/dev/cciss" in _dev: Line 133: _dev = "/dev/mapper/%s" % process.check_output(["cciss_id", _dev]) I'd not like to work with cciss_id here, if possible I'd rather like to query udev or sysfs. Line 134: if not "/dev/mapper" in _dev: Line 135: if not _dev.startswith("/dev/"): Line 136: _dev = "/dev/%s" % _dev Line 137: try: Line 130: if _dev is None: Line 131: return False Line 132: if "/dev/cciss" in _dev: Line 133: _dev = "/dev/mapper/%s" % process.check_output(["cciss_id", _dev]) Line 134: if not "/dev/mapper" in _dev: The next three lines should be simpliefied, possibly in general: if "/dev/cciss" in devpath: … elif "/dev/mapper" in devpath: … else: … Line 135: if not _dev.startswith("/dev/"): Line 136: _dev = "/dev/%s" % _dev Line 137: try: Line 138: output = [x for x in process.check_output(["multipath", "-ll", Line 134: if not "/dev/mapper" in _dev: Line 135: if not _dev.startswith("/dev/"): Line 136: _dev = "/dev/%s" % _dev Line 137: try: Line 138: output = [x for x in process.check_output(["multipath", "-ll", This function is complex enough to get it's own python function, to make it more clear what's happening here. Line 139: _dev]).split('\n') if re.match(r'.*?dm-[0-9]+', Line 140: x)][0].strip() Line 141: _dev = "/dev/mapper/%s" % output.split()[0] Line 142: except process.CalledProcessError as e: Line 147: # multipath returned fine, but we didn't match a known dm name Line 148: self.logger.exception("Translated %s, but didn't find the dm" Line 149: "path" % _dev) Line 150: Line 151: if not hasattr(self, 'disk_dict'): We do not have self.disk_dict in this class Line 152: # Haven't populated yet, do that first Line 153: self.disk_dict = self._storage.get_udev_devices() Line 154: info = self.disk_dict[_dev].split(",", 5) Line 155: device = Device(_dev, *info) -- To view, visit https://gerrit.ovirt.org/19951 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I203d16f75fd9ed3b983cc240d8c9c34fd8189499 Gerrit-PatchSet: 15 Gerrit-Project: ovirt-node Gerrit-Branch: master Gerrit-Owner: Ryan Barry <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ node-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/node-patches
