Change in vdsm[master]: hsm: Add refresh device verb
frolland at redhat.com
frolland at redhat.com
Mon Mar 16 12:04:58 UTC 2015
Fred Rolland has posted comments on this change.
Change subject: hsm: Add refresh device verb
......................................................................
Patch Set 1:
(29 comments)
https://gerrit.ovirt.org/#/c/38754/1//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2015-03-15 15:21:15 +0200
Line 4: Commit: Fred Rolland <frolland at redhat.com>
Line 5: CommitDate: 2015-03-15 17:18:44 +0200
Line 6:
Line 7: hsm: Add refresh device command
> We call this "verb" in vdsm.
Done
Line 8:
Line 9: This command calls the multipath-resize and the returns
Line 10: the device size.
Line 11: multipath-resize resizes online multipath device after a LUN
https://gerrit.ovirt.org/#/c/38754/1/client/vdsClient.py
File client/vdsClient.py:
Line 722: pp.pprint(devices['devList'])
Line 723: return 0, ''
Line 724:
Line 725: def refreshDevice(self, args):
Line 726: if len(args) == 1:
> This should be something like this:
Done, used validateArgTypes
Line 727: guid = args[0]
Line 728: size = self.s.refreshDevice(guid)
Line 729: print '%s size= %s' % (guid, size)
Line 730: return 0, ''
Line 724:
Line 725: def refreshDevice(self, args):
Line 726: if len(args) == 1:
Line 727: guid = args[0]
Line 728: size = self.s.refreshDevice(guid)
> You do not get size here - you will get a dict with a status key and size k
Done
Line 729: print '%s size= %s' % (guid, size)
Line 730: return 0, ''
Line 731:
Line 732: def getDevicesVisibility(self, args):
Line 725: def refreshDevice(self, args):
Line 726: if len(args) == 1:
Line 727: guid = args[0]
Line 728: size = self.s.refreshDevice(guid)
Line 729: print '%s size= %s' % (guid, size)
> Since we called with guid, it is not needed in the return value.
I used printDict(res, self.pretty) , like in getVolumeSize
Line 730: return 0, ''
Line 731:
Line 732: def getDevicesVisibility(self, args):
Line 733: devList = args[0].split(',')
Line 726: if len(args) == 1:
Line 727: guid = args[0]
Line 728: size = self.s.refreshDevice(guid)
Line 729: print '%s size= %s' % (guid, size)
Line 730: return 0, ''
> You return 0 when the command may have failed?!
Done
Line 731:
Line 732: def getDevicesVisibility(self, args):
Line 733: devList = args[0].split(',')
Line 734: res = self.s.getDevicesVisibility(devList, {})
https://gerrit.ovirt.org/#/c/38754/1/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:
Line 252: 'The maximal number of seconds to wait for scsi scan to return.'),
Line 253:
Line 254: ('multipath_resize_maximal_timeout', '30',
Line 255: 'The maximal number of seconds to wait '
Line 256: 'for multipath resize to return.'),
> Lets start with current iscsi_rescan_maximal_timeout - this is the same use
Done
Line 257:
Line 258: ('sd_health_check_delay', '10',
Line 259: 'Storage domain health check delay, the amount of seconds to '
Line 260: 'wait between two successive run of the domain health check.'),
https://gerrit.ovirt.org/#/c/38754/1/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:
Line 163: EXT_VDSM_TOOL = os.path.join('@BINDIR@', 'vdsm-tool')
Line 164:
Line 165: EXT_CURL_IMG_WRAP = '@LIBEXECDIR@/curl-img-wrap'
Line 166: EXT_FC_SCAN = '@LIBEXECDIR@/fc-scan'
Line 167: EXT_MP_RESIZE = '@LIBEXECDIR@/multipath-resize'
> This should be the name of the tool, which is multipath-resize, so, this sh
Done
https://gerrit.ovirt.org/#/c/38754/1/vdsm/rpc/vdsmapi-schema.json
File vdsm/rpc/vdsmapi-schema.json:
Line 1475: 'data': {'*storageType': 'BlockDeviceType'},
Line 1476: 'returns': ['BlockDeviceInfo']}
Line 1477:
Line 1478: ##
Line 1479: # @DeviceSize:
> You don't need this.
Done
Line 1480: #
Line 1481: # Device size.
Line 1482: #
Line 1483: # @size: Device size.
Line 1494: #
Line 1495: # @guid: A block device GUID
Line 1496: #
Line 1497: # Returns:
Line 1498: # Device size
> Please document the units of this size
Done
Line 1499: #
Line 1500: # Since: 4.10.0
Line 1501: ##
Line 1502: {'command': {'class': 'Host', 'name': 'refreshDevice'},
Line 1496: #
Line 1497: # Returns:
Line 1498: # Device size
Line 1499: #
Line 1500: # Since: 4.10.0
> 3.6 - lets try 4.17.0
Done
Line 1501: ##
Line 1502: {'command': {'class': 'Host', 'name': 'refreshDevice'},
Line 1503: 'data': {'guid': ['UUID']},
Line 1504: 'returns': 'DeviceSize'}
Line 1500: # Since: 4.10.0
Line 1501: ##
Line 1502: {'command': {'class': 'Host', 'name': 'refreshDevice'},
Line 1503: 'data': {'guid': ['UUID']},
Line 1504: 'returns': 'DeviceSize'}
> Use:
Done
Line 1505:
Line 1506: ##
Line 1507: # @DeviceVisibilityMap:
Line 1508: #
https://gerrit.ovirt.org/#/c/38754/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:
Line 733:
Line 734: return dict(size=str(roundedSizeBytes))
Line 735:
Line 736: @public
Line 737: def refreshDevice(self, guid, options=None):
> I would put this near getDeviceList and getDeviceVisibility. Iit is easier
Done
Line 738: """
Line 739: Call multipath-resize with specified device guid
Line 740:
Line 741: :param guid: Device UUID to refresh size.
Line 735:
Line 736: @public
Line 737: def refreshDevice(self, guid, options=None):
Line 738: """
Line 739: Call multipath-resize with specified device guid
> This is an implementation detail, we don't want to expose it here. This cal
Done
Line 740:
Line 741: :param guid: Device UUID to refresh size.
Line 742: :type guid: UUID
Line 743: :param options: ?
Line 739: Call multipath-resize with specified device guid
Line 740:
Line 741: :param guid: Device UUID to refresh size.
Line 742: :type guid: UUID
Line 743: :param options: ?
> Don't copy this - document that this is unused.
Done
Line 744: :returns: dictionary of size and respective size in bytes
Line 745: :rtype: dict
Line 746: """
Line 747: multipath.resize(guid)
Line 740:
Line 741: :param guid: Device UUID to refresh size.
Line 742: :type guid: UUID
Line 743: :param options: ?
Line 744: :returns: dictionary of size and respective size in bytes
> This returns dictionary with one item: "size" - what is respective size?
Done
Line 745: :rtype: dict
Line 746: """
Line 747: multipath.resize(guid)
Line 748:
Line 747: multipath.resize(guid)
Line 748:
Line 749: st = os.stat("/dev/mapper/%s" % guid)
Line 750: minor = os.minor(st.st_rdev)
Line 751: devicesize = multipath.getDeviceSize("dm-%d" % minor)
> You should use vdsm apis for this:
Done
Line 752: return dict(size=devicesize)
Line 753:
Line 754: @public
Line 755: def extendStorageDomain(self, sdUUID, spUUID, guids,
Line 748:
Line 749: st = os.stat("/dev/mapper/%s" % guid)
Line 750: minor = os.minor(st.st_rdev)
Line 751: devicesize = multipath.getDeviceSize("dm-%d" % minor)
Line 752: return dict(size=devicesize)
> You must convert the size to string - remember xmlrpc int32 limits:
Done
Line 753:
Line 754: @public
Line 755: def extendStorageDomain(self, sdUUID, spUUID, guids,
Line 756: force=False, options=None):
https://gerrit.ovirt.org/#/c/38754/1/vdsm/storage/multipath.py
File vdsm/storage/multipath.py:
Line 28: import logging
Line 29: import re
Line 30: from collections import namedtuple
Line 31: from vdsm.config import config
Line 32: from vdsm.infra import zombiereaper
> vdsm imports should be grouped together - move this to the vdsm import bell
Done
Line 33:
Line 34: from vdsm import constants
Line 35: from vdsm import utils
Line 36: import hba
Line 308: def devIsFCP(type):
Line 309: return type in [DEV_FCP, DEV_MIXED]
Line 310:
Line 311:
Line 312: def resize(uuid):
> Please move this near rescan - this similar operation, and we like to put t
Done
Line 313: """
Line 314: Calls script multipath-resize
Line 315: """
Line 316: timeout = config.getint('irs', 'multipath_resize_maximal_timeout')
Line 310:
Line 311:
Line 312: def resize(uuid):
Line 313: """
Line 314: Calls script multipath-resize
> We need much more info here, to explain why this works in this way.
Done
Line 315: """
Line 316: timeout = config.getint('irs', 'multipath_resize_maximal_timeout')
Line 317:
Line 318: proc = utils.execCmd([constants.EXT_MP_RESIZE], sync=False,
Line 312: def resize(uuid):
Line 313: """
Line 314: Calls script multipath-resize
Line 315: """
Line 316: timeout = config.getint('irs', 'multipath_resize_maximal_timeout')
> Use iscsi_rescan_maximal_timeout
Done
Line 317:
Line 318: proc = utils.execCmd([constants.EXT_MP_RESIZE], sync=False,
Line 319: execCmdLogger=log, sudo=True)
Line 320: try:
Line 313: """
Line 314: Calls script multipath-resize
Line 315: """
Line 316: timeout = config.getint('irs', 'multipath_resize_maximal_timeout')
Line 317:
> We need log.debug when resize operation is starting - see hba.rescan()
Done
Line 318: proc = utils.execCmd([constants.EXT_MP_RESIZE], sync=False,
Line 319: execCmdLogger=log, sudo=True)
Line 320: try:
Line 321: proc.wait(timeout)
Line 314: Calls script multipath-resize
Line 315: """
Line 316: timeout = config.getint('irs', 'multipath_resize_maximal_timeout')
Line 317:
Line 318: proc = utils.execCmd([constants.EXT_MP_RESIZE], sync=False,
> You need to add the guid argument
Done
Line 319: execCmdLogger=log, sudo=True)
Line 320: try:
Line 321: proc.wait(timeout)
Line 322: finally:
Line 315: """
Line 316: timeout = config.getint('irs', 'multipath_resize_maximal_timeout')
Line 317:
Line 318: proc = utils.execCmd([constants.EXT_MP_RESIZE], sync=False,
Line 319: execCmdLogger=log, sudo=True)
> sudo is critical parameter, please put it first, not last.
Done
Line 320: try:
Line 321: proc.wait(timeout)
Line 322: finally:
Line 323: if proc.returncode is None:
Line 321: proc.wait(timeout)
Line 322: finally:
Line 323: if proc.returncode is None:
Line 324: zombiereaper.autoReapPID(proc.pid)
Line 325: raise Error("Timeout scanning (pid=%s)" % proc.pid)
> scanning -> resizing
Done
Line 326: elif proc.returncode != 0:
Line 327: stderr = proc.stderr.read(512)
Line 328: raise Error("Scan failed: %r" % stderr)
Line 329: return ""
Line 322: finally:
Line 323: if proc.returncode is None:
Line 324: zombiereaper.autoReapPID(proc.pid)
Line 325: raise Error("Timeout scanning (pid=%s)" % proc.pid)
Line 326: elif proc.returncode != 0:
> if is better here, since the previous condition is raising. We are not deal
Done
Line 327: stderr = proc.stderr.read(512)
Line 328: raise Error("Scan failed: %r" % stderr)
Line 329: return ""
Line 330:
Line 324: zombiereaper.autoReapPID(proc.pid)
Line 325: raise Error("Timeout scanning (pid=%s)" % proc.pid)
Line 326: elif proc.returncode != 0:
Line 327: stderr = proc.stderr.read(512)
Line 328: raise Error("Scan failed: %r" % stderr)
> Scan -> Resize
Done
Line 329: return ""
Line 330:
Line 331:
Line 332: class Error(Exception):
Line 325: raise Error("Timeout scanning (pid=%s)" % proc.pid)
Line 326: elif proc.returncode != 0:
Line 327: stderr = proc.stderr.read(512)
Line 328: raise Error("Scan failed: %r" % stderr)
Line 329: return ""
> Not needed, this function returns nothing (like void).
Done
Line 330:
Line 331:
Line 332: class Error(Exception):
Line 329: return ""
Line 330:
Line 331:
Line 332: class Error(Exception):
Line 333: """ multipath operation failed """
> Put at the start of the module - see hba.py.
Done
--
To view, visit https://gerrit.ovirt.org/38754
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6fbeec7920a7943a9b1ea9dbd93477fa9b642f5b
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list