Freddy Rolland has posted comments on this change.

Change subject: storage: Add resize map support in multipath.py
......................................................................


Patch Set 8:

(6 comments)

https://gerrit.ovirt.org/#/c/40468/8/vdsm/storage/multipath.py
File vdsm/storage/multipath.py:

Line 86: 
Line 87: def resize_if_needed(guid):
Line 88:     """
Line 89:     This function will resize a device if the underlying slaves
Line 90:     have the same size and that size is bigger than the device size.
> This is kind of correct but it is more complicated then needed, since you m
Done
Line 91:     """
Line 92:     name = devicemapper.getDmId(guid)
Line 93:     slaves = [(slave, getDeviceSize(slave))
Line 94:               for slave in devicemapper.getSlaves(name)]


Line 91:     """
Line 92:     name = devicemapper.getDmId(guid)
Line 93:     slaves = [(slave, getDeviceSize(slave))
Line 94:               for slave in devicemapper.getSlaves(name)]
Line 95:     if len(slaves) > 0:
> Check if len(slaves) == 0, and log a warning - we do not expect such device
Done
Line 96:         if len(set(size for slave, size in slaves)) != 1:
Line 97:             raise Error("Map %r slaves size differ %s" % (guid, 
slaves))
Line 98:         map_size = getDeviceSize(name)
Line 99:         slave_size = slaves[0][1]


Line 93:     slaves = [(slave, getDeviceSize(slave))
Line 94:               for slave in devicemapper.getSlaves(name)]
Line 95:     if len(slaves) > 0:
Line 96:         if len(set(size for slave, size in slaves)) != 1:
Line 97:             raise Error("Map %r slaves size differ %s" % (guid, 
slaves))
> Add empty line after you raise, to make the flow more clear.
Done
Line 98:         map_size = getDeviceSize(name)
Line 99:         slave_size = slaves[0][1]
Line 100:         if map_size < slave_size:
Line 101:             log.info("Resizing map %r (map_size=%d, slave_size=%d)",


Line 96:         if len(set(size for slave, size in slaves)) != 1:
Line 97:             raise Error("Map %r slaves size differ %s" % (guid, 
slaves))
Line 98:         map_size = getDeviceSize(name)
Line 99:         slave_size = slaves[0][1]
Line 100:         if map_size < slave_size:
> Lets check if map_size == slave_size and return False:
Done
Line 101:             log.info("Resizing map %r (map_size=%d, slave_size=%d)",
Line 102:                      guid, map_size, slave_size)
Line 103:             supervdsm.getProxy().resizeMap(name)
Line 104:             return True


Line 107: 
Line 108: def _resize_map(name):
Line 109:     """
Line 110:     Invoke multipathd to resize a device
Line 111:     Must run as root
> Describe possible failures.
Done
Line 112:     """
Line 113:     log.debug("Resizing map %r", name)
Line 114:     cmd = [_MULTIPATHD.cmd, "-kresize map %s" % name]
Line 115:     start = utils.monotonic_time()


Line 110:     Invoke multipathd to resize a device
Line 111:     Must run as root
Line 112:     """
Line 113:     log.debug("Resizing map %r", name)
Line 114:     cmd = [_MULTIPATHD.cmd, "-kresize map %s" % name]
> Please check if the format suggested by Ben works:
Done
Line 115:     start = utils.monotonic_time()
Line 116:     rc, out, err = utils.execCmd(cmd, raw=True, execCmdLogger=log)
Line 117:     # multipathd reports some errors using non-zero exit code and 
stderr (need
Line 118:     # to be root), but -k commands always return 0, and the result is 
reported


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Freddy Rolland <froll...@redhat.com>
Gerrit-Reviewer: Fred Rolland <froll...@redhat.com>
Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to