Nir Soffer has posted comments on this change. Change subject: storage: Add resize_if_needed method in multipath.py ......................................................................
Patch Set 5: (17 comments) https://gerrit.ovirt.org/#/c/40468/5//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2015-05-03 11:38:08 +0300 Line 4: Commit: Fred Rolland <froll...@redhat.com> Line 5: CommitDate: 2015-05-03 17:25:20 +0300 Line 6: Line 7: storage: Add resize_if_needed method in multipath.py You added also resize_devices_if_needed. Why are you describing only this function? Line 8: Line 9: This method checks if a multipath device should be resized. Line 10: It will check the slaves sizes and if it is bigger than the multipath Line 11: device it will call "multipathd resize". Line 5: CommitDate: 2015-05-03 17:25:20 +0300 Line 6: Line 7: storage: Add resize_if_needed method in multipath.py Line 8: Line 9: This method checks if a multipath device should be resized. method -> function Line 10: It will check the slaves sizes and if it is bigger than the multipath Line 11: device it will call "multipathd resize". Line 12: Line 13: Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d Line 7: storage: Add resize_if_needed method in multipath.py Line 8: Line 9: This method checks if a multipath device should be resized. Line 10: It will check the slaves sizes and if it is bigger than the multipath Line 11: device it will call "multipathd resize". It does not call "multipathd resize" - lets not show incomplete example, and instead describe that we resize the map using multipathd. Missing context on why we need to resize multipath map, and how slave size can be larger than the multipath map. Line 12: Line 13: Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d Line 14: Bug-Url:https://bugzilla.redhat.com/609689 https://gerrit.ovirt.org/#/c/40468/5/vdsm/storage/multipath.py File vdsm/storage/multipath.py: Line 52: "/usr/lib/udev/scsi_id", # Fedora Line 53: "/lib/udev/scsi_id", # EL6, Ubuntu Line 54: ) Line 55: Line 56: MULTIPATHD = utils.CommandPath("multipathd", Should be private - _MULTIPATHD Line 57: "/usr/sbin/multipathd", Line 58: "/sbin/multipathd") Line 59: Line 60: Line 54: ) Line 55: Line 56: MULTIPATHD = utils.CommandPath("multipathd", Line 57: "/usr/sbin/multipathd", Line 58: "/sbin/multipathd") Is /sbin/multipathd for rhel6? we don't support it any more. Please use only paths used by rhel7 fedora and latest debian. Line 59: Line 60: Line 61: def rescan(): Line 62: """ Line 312: Line 313: yield dmInfoDir.split("/")[3], guid Line 314: Line 315: Line 316: def resize_devices_if_needed(filterGuids=None): I don't think we need or want this. We don't want to make it easy to resize *all* devices. The caller should specify the devices to resize. For using in getDeviceList, you can resize each device as part of the loop in HSM._getDeviceList, and we don't need to handle multiple guids here. For using when connecting to the storage pool, we like to resize only pvs used by the vgs you are connecting to, so again do not need the interation code here, we have list of pvs, and we can write a simple loop calling multipath.resize_if_needed() Line 317: filteringOn = filterGuids is not None Line 318: filterLen = len(filterGuids) if filteringOn else -1 Line 319: devsFound = 0 Line 320: Line 328: devsFound += 1 Line 329: resize_if_needed(guid) Line 330: Line 331: Line 332: def resize_if_needed(guid): Please move up under rescan Also needs detailed documentation Line 333: log.debug("Check if device %r needs resize", guid) Line 334: name = devicemapper.getDmId(guid) Line 335: deviceSize = getDeviceSize(name) Line 336: svdsm = supervdsm.getProxy() Line 329: resize_if_needed(guid) Line 330: Line 331: Line 332: def resize_if_needed(guid): Line 333: log.debug("Check if device %r needs resize", guid) Extra indentation Line 334: name = devicemapper.getDmId(guid) Line 335: deviceSize = getDeviceSize(name) Line 336: svdsm = supervdsm.getProxy() Line 337: for slave in devicemapper.getSlaves(name): Line 335: deviceSize = getDeviceSize(name) Line 336: svdsm = supervdsm.getProxy() Line 337: for slave in devicemapper.getSlaves(name): Line 338: if getDeviceSize(slave) > deviceSize: Line 339: log.debug("Slave size is larger, %r needs resize", guid) Not enough details. Line 340: try: Line 341: svdsm.resize_map(name) Line 342: except Exception as e: Line 343: log.error("Error: %s while trying to resize device: " Line 337: for slave in devicemapper.getSlaves(name): Line 338: if getDeviceSize(slave) > deviceSize: Line 339: log.debug("Slave size is larger, %r needs resize", guid) Line 340: try: Line 341: svdsm.resize_map(name) Why do you call this multiple times? Line 342: except Exception as e: Line 343: log.error("Error: %s while trying to resize device: " Line 344: "%s", str(e.message), guid) Line 345: break Line 338: if getDeviceSize(slave) > deviceSize: Line 339: log.debug("Slave size is larger, %r needs resize", guid) Line 340: try: Line 341: svdsm.resize_map(name) Line 342: except Exception as e: Since we should call multipathd once, we don't need to handle errors. The caller will have to handle this error. Line 343: log.error("Error: %s while trying to resize device: " Line 344: "%s", str(e.message), guid) Line 345: break Line 346: Line 340: try: Line 341: svdsm.resize_map(name) Line 342: except Exception as e: Line 343: log.error("Error: %s while trying to resize device: " Line 344: "%s", str(e.message), guid) You don't need to convert e.message to str, and you don't need to access e.message - you should use: log.error("text %s", e) Formatting object in %s invokes object.__str__ (like toString() in java). Finally it is more clear if you keep your error separate from the underlying error: log.error("Error resizing %r: %s", e) Line 345: break Line 346: Line 347: Line 348: def resize_map(name): Line 342: except Exception as e: Line 343: log.error("Error: %s while trying to resize device: " Line 344: "%s", str(e.message), guid) Line 345: break Line 346: This should be something like this: name = devicemapper.getDmId(guid) slaves = [(slave, getDeviceSize(slave)) for slave in devicemapper.getSlaves(name)] if len(set(size for slave, size in slaves)) != 1: raise multipath.Error("Map %r slaves size differ %s" % (guid, slaves)) map_size = getDeviceSize(name) slave_size = slaves[0][1] if map_size < slave_size: log.info("Resizing map %r (map_size=%d, slave_size=%d)", guid, map_size, slave_size) supervdsm.getProxy().resizeMap(name) Line 347: Line 348: def resize_map(name): Line 349: log.debug("Resizing map %r", name) Line 350: cmd = [MULTIPATHD.cmd, "-kresize map %s" % name] Line 344: "%s", str(e.message), guid) Line 345: break Line 346: Line 347: Line 348: def resize_map(name): This requires root permissions, and can run only from supervdsm, so lets make it private - _resize_map And add a note: "Must run as root" Line 349: log.debug("Resizing map %r", name) Line 350: cmd = [MULTIPATHD.cmd, "-kresize map %s" % name] Line 351: start = utils.monotonic_time() Line 352: rc, out, err = utils.execCmd(cmd, raw=True, execCmdLogger=log) Line 353: # multipathd reports some errors using non-zero exit code and stderr (need Line 354: # to be root), but -k commands always return 0, and the result is reported Line 355: # using stdout. Line 356: if rc != 0 or out != "ok\n": Line 357: raise Exception("Resizing map %r failed: out=%r err=%r" Use multipath.Error Line 358: % (name, out, err)) Line 359: elapsed = utils.monotonic_time() - start Line 360: log.debug("Resized map %r in %.2f seconds", name, elapsed) Line 361: https://gerrit.ovirt.org/#/c/40468/5/vdsm/supervdsmServer File vdsm/supervdsmServer: Line 63: changeNumvfs Line 64: Line 65: from network.tc import setPortMirroring, unsetPortMirroring Line 66: from storage.multipath import getScsiSerial as _getScsiSerial Line 67: from storage.multipath import resize_map as _resize_map You are using same code as the exiting code, but in this case we should not follow bad example. Please follow the hba example - import the module, not a function, and invoke the module.function you need. Line 68: from storage.iscsi import getDevIscsiInfo as _getdeviSCSIinfo Line 69: from storage.iscsi import readSessionInfo as _readSessionInfo Line 70: from supervdsm import _SuperVdsmManager Line 71: from storage import hba Line 147: return _getScsiSerial(*args, **kwargs) Line 148: Line 149: @logDecorator Line 150: def resize_map(selfself, devName): Line 151: return _resize_map(devName) Should be: @logDecorator def resizeMap(self, devName): multipath._resize_map(devName) I prefer resize_map but this module is using mixedCase, lets keep it consistent for now. Line 152: Line 153: @logDecorator Line 154: def removeDeviceMapping(self, devName): Line 155: return _removeMapping(devName) -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Fred 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
