Change in vdsm[master]: resize-lun: Rescan fiber channel devices

nsoffer at redhat.com nsoffer at redhat.com
Thu Jul 9 16:46:47 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: resize-lun: Rescan fiber channel devices
......................................................................


Patch Set 1:

(6 comments)

Looks nice, but I think we can simplify this and should break run() to smaller methods.

https://gerrit.ovirt.org/#/c/43388/1/vdsm/storage/fc-scan
File vdsm/storage/fc-scan:

Line 69
Line 70
Line 71
Line 72
Line 73
See comment bellow.


Line 73
Line 74
Line 75
Line 76
Line 77
Please refactor the first part to _rescan_host() and the new part to _rescan_devices(). In the docstring of each method explain why we need each (e.g. detecting new devices, detecting new device size).


Line 77:             log.exception("Scanning %s failed", path)
Line 78: 
Line 79:         devicePath = "/sys/devices/*/*/*/%s/" \
Line 80:                      "rport-*/target*/*/scsi_device/*/device/rescan"\
Line 81:                      % self.host
Please use only lower_case names.

For breaking lines, never use \ - it is fragile. Use instead:

    devices_rescan = ("long text that does fit in one line"
                      "more text")
Line 82:         for device in glob.glob(devicePath):
Line 83:             device = os.path.basename(device[:-14])
Line 84:             deviceRescanPath = "/sys/class/scsi_device/%s/device/rescan"\
Line 85:                                % device


Line 79:         devicePath = "/sys/devices/*/*/*/%s/" \
Line 80:                      "rport-*/target*/*/scsi_device/*/device/rescan"\
Line 81:                      % self.host
Line 82:         for device in glob.glob(devicePath):
Line 83:             device = os.path.basename(device[:-14])
This is very fragile, but we don't needed (see bellow)
Line 84:             deviceRescanPath = "/sys/class/scsi_device/%s/device/rescan"\
Line 85:                                % device
Line 86:             try:
Line 87:                 log.debug("Rescanning device %s", device)


Line 81:                      % self.host
Line 82:         for device in glob.glob(devicePath):
Line 83:             device = os.path.basename(device[:-14])
Line 84:             deviceRescanPath = "/sys/class/scsi_device/%s/device/rescan"\
Line 85:                                % device
What you found *is* the device rescan path, you don't need to create a new path, and this is the actual path in sysfs, which is the recommended way to work with sysfs.
See https://www.kernel.org/doc/Documentation/sysfs-rules.txt
Line 86:             try:
Line 87:                 log.debug("Rescanning device %s", device)
Line 88:                 with utils.stopwatch("Rescanned device %s" % device, log=log):
Line 89:                     fd = os.open(deviceRescanPath, os.O_WRONLY)


Line 90:                     try:
Line 91:                         os.write(fd, "1")
Line 92:                     finally:
Line 93:                         os.close(fd)
Line 94:                 self.succeeded = True
This does not work now - in the old code, we had one operation, and we set it to True if the operation succeeded. Now we have many operations, so it must be true only if we had no failure.

We can replace succeeded with a readonly property, and count errors in self._errors:

    @property
    def succeeded(self):
        return self._errors == 0
Line 95:             except OSError as e:
Line 96:                 log.error("Rescanning %s failed: %s", deviceRescanPath, e)
Line 97:             except Exception:
Line 98:                 log.exception("Rescanning %s failed", deviceRescanPath)


-- 
To view, visit https://gerrit.ovirt.org/43388
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I201312c44fec30d11ad62823558aa10ad7f7991b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Fred Rolland <frolland at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list