[node-patches] Change in ovirt-node[master]: Move multipath device checking to ovirt.node.utils.storage

[fabiand at redhat.com]

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 <rbarry at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes