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