Change in vdsm[master]: hsm: Add refresh device command

nsoffer at redhat.com nsoffer at redhat.com
Sun Mar 15 20:13:31 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: hsm: Add refresh device command
......................................................................


Patch Set 1:

(28 comments)

Nice!

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:

    if not args:
        usage error - check how other code is doing this here
    guid = args.pop(0)
    if args:
        fail loudly, user must fix his ways
    normal flow

In a perfect world, you could specify which args you accept and the framework would do all the checks automatically, but we don't have such framework.
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 key.

You must check the status key (see other code) and fail loudly if the operation failed.

If status code is 0, you can return 0 and show the size.
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.

This version make it harder to use this tool in a script, because you have to parse the string "guid size=N". The best way would be to return the size, so one can do:

    size=$(vdsClient -s 0 guid)

Check how other verbs returning size are behaving.
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?!

People may use this in a script and they would like to know when the operation failed. They depend on this code returning 0 *only* when the operation succeeded.
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 case.

We will add new configuration only if we must.
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/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.
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
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
> What version will it be ?
3.6 - lets try 4.17.0
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:

    'returns': 'str'}

This verb is like @VM.diskSizeExtend
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 to work with code when function are grouped using some logical method.

This belong to the devices group, not related to volumes or to storage domain.
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 calls multipath.resize(guid), and we don't care how that function is implemented, since we will have to update this documentation if we did.
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.
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?
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:

    dmid = devicemapper.getDmId(guid)
    size = multipath.getDeviceSize(dmid)

Actually multipath.getDeviceSize should have handle getting a size by a guid, but it does not. We will fix the bad api later so we can simply do here:

    size = multipath.getDeviceSize(guid)
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:

    return dict(size=str(size))
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-resize
File vdsm/storage/multipath-resize:

Line 54
Line 55
Line 56
Line 57
Line 58
Please remove this change. I already fixed in my patch. You should rebase your patch on the latest version.


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 bellow, and keep this group sorted.
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 these kind of interfaces together.
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.

- Perform scsi scan of the device paths
- Resize the multipath map
- Which errors are raised?
- Note about scsi scan being blocking operation, so we run this in external tool.
- Note about this blocking up to 30 seconds, and what happen if the operation times out.
- TODO: move to supervdsm when zombiereaper is functional there.
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
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()
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
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.
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
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 dealing here with several options, but with handling possible issues.
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
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).

Instead we want log.debug when operation was finished, see hab.rescan()
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.


-- 
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