Nir Soffer 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 mix the normal flow and errors. Describe what it does: Resize multipath map if the underlying slaves are bigger than the map size. Also explain how slaves can be bigger then the map. Describe possible failures: Raises Error if underlying slaves have different size, or multipathd failed to resize the map. 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 devices. if len(slaves) == 0: log warning... return continue normal flow... 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. Also this function is little too long - lets extract a _validate_slaves function - may be useful in other flows. 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: if map_size == slave_size: return False Continue normal flow... 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. 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: cmd = [_MULTIPATHD.cmd, "resize", "map", name] 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
