Change in vdsm[ovirt-3.6]: hsm: Support checkStatus param in getDeviceList

Sunday, 6 September 2015

Hello Fred Rolland, Allon Mureinik,

I'd like you to do a code review.  Please visit

    https://gerrit.ovirt.org/45759

to review the following change.

Change subject: hsm: Support checkStatus param in getDeviceList
......................................................................

hsm: Support checkStatus param in getDeviceList

In order to populate the 'status' field, the getDeviceList
verb perform a create PV test. This operation is expensive, and in
setups with large number of devices it will cause performance issue.

However, the 'status' field is not always needed. This patch
add an optional 'checkStatus' boolean parameter to getDeviceList.
By default it will be True to keep same behavior as before.

If specified as False, the PV create test will be skipped and the
'status' field will be populated as 'unknown'.

The flow before this patch was:
    - Engine calls getDeviceList, VDSM perform status check for each
      devices, possibly times out
    - User select a few devices (typically one)
    - Engine warn user about used devices

The flow with this patch:
    - Engine calls getDeviceList, skipping status check
    - User select a few devices (typically one)
    - Engine calls getDeviceList, checking status only for selected
    - Engine warn user about used devices

On a setup of 100 ISCSI devices, the command took 3s without
the PV create test against 58s with the test.

real    0m2.987s
user    0m0.263s
sys     0m0.028s

real    0m57.769s
user    0m0.265s
sys     0m0.024s

Examples:
    getDeviceList
        return all devices
    getDeviceList FCP
        return only FCP devices
    getDeviceList FCP True
        return only FCP devices and perform PV create test
    getDeviceList ISCSI False guid1 guid2
       return info for guid1 and guid2, assuming ISCSI type
       without performing PV create test

Change-Id: Ic28954708f2fd7c7b721aa7f9a0fb6e1a6019597
Bug-Url: https://bugzilla.redhat.com/1217401
Signed-off-by: Fred Rolland <frolland(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/45093
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com&gt;
Reviewed-by: Allon Mureinik <amureini(a)redhat.com&gt;
---
M client/vdsClient.py
M vdsm/API.py
M vdsm/rpc/bindingxmlrpc.py
M vdsm/rpc/vdsmapi-schema.json
M vdsm/storage/hsm.py
5 files changed, 77 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/59/45759/1

diff --git a/client/vdsClient.py b/client/vdsClient.py
index ce40974..cda9b81 100755
--- a/client/vdsClient.py
+++ b/client/vdsClient.py
@@ -732,8 +732,12 @@
     def getDeviceList(self, args):
         if len(args) == 0:
             res = self.s.getDeviceList()
+        elif len(args) == 1:
+            res = self.s.getDeviceList(args[0])
         else:
-            res = self.s.getDeviceList(args[0], args[1:])
+            # 'checkStatus' arg should be last
+            res = self.s.getDeviceList(args[0], args[2:],
+                                       utils.tobool(args[1]))
 
         if res['status']['code']:
             return res['status']['code'],
res['status']['message']
@@ -2277,16 +2281,22 @@
                        )),
         'getDeviceList': (serv.getDeviceList,
                           ('[storageType]',
+                           '[checkStatus]',
                            '[<devlist>]',
                            'List of all block devices (optionally - matching '
-                           'storageType, optionally - of each device listed).',
+                           'storageType, optionally - check device status'
+                           'optionally - of each device listed).',
                            '    getDeviceList',
                            '        return all devices',
                            '    getDeviceList FCP',
                            '        return only FCP devices',
-                           '    getDeviceList ISCSI guid1 guid2',
+                           '    getDeviceList FCP True',
+                           '        return only FCP devices and perform ',
+                           '        PV creation test',
+                           '    getDeviceList ISCSI False guid1 guid2',
                            '        return info for guid1 and guid2',
-                           '        , assuming ISCSI type'
+                           '        , assuming ISCSI type and skip PV ',
+                           '        creation test'
                            )),
         'getDevicesVisibility': (serv.getDevicesVisibility,
                                  ('<devlist>',
diff --git a/vdsm/API.py b/vdsm/API.py
index b122424..7582852 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -1679,8 +1679,8 @@
     def getLVMVolumeGroups(self, storageType=None):
         return self._irs.getVGList(storageType)
 
-    def getDeviceList(self, storageType=None, guids=()):
-        return self._irs.getDeviceList(storageType, guids)
+    def getDeviceList(self, storageType=None, guids=(), checkStatus=True):
+        return self._irs.getDeviceList(storageType, guids, checkStatus)
 
     def getDevicesVisibility(self, guidList):
         return self._irs.getDevicesVisibility(guidList)
diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py
index 7f1da4f..4c69b32 100644
--- a/vdsm/rpc/bindingxmlrpc.py
+++ b/vdsm/rpc/bindingxmlrpc.py
@@ -1029,9 +1029,10 @@
         api = API.Global()
         return api.getLVMVolumeGroups(storageType)
 
-    def devicesGetList(self, storageType=None, guids=(), options=None):
+    def devicesGetList(self, storageType=None, guids=(),
+                       checkStatus=True, options=None):
         api = API.Global()
-        res = api.getDeviceList(storageType, guids)
+        res = api.getDeviceList(storageType, guids, checkStatus)
         return unprotect_passwords(res)
 
     def devicesGetVisibility(self, guids, options=None):
diff --git a/vdsm/rpc/vdsmapi-schema.json b/vdsm/rpc/vdsmapi-schema.json
index 034359a..e443bcd 100644
--- a/vdsm/rpc/vdsmapi-schema.json
+++ b/vdsm/rpc/vdsmapi-schema.json
@@ -1417,6 +1417,26 @@
           'initiatorname': 'str', '*username': 'str',
'*password': 'str'}}
 
 ##
+# @BlockDeviceStatus:
+#
+# Enumeration of possible status for a block device.
+#
+# @free:     The device is free
+#
+# @used:     The device is in use.
+#            A device will be considered used on the following cases:
+#            - a PV exists on this device and is part of a VG
+#            - a partition table is found on the device
+#            - The device may have a mounted file system
+#
+# @unknown:  The device status is unknown
+#            (new in version 4.17.5)
+#
+# Since: 4.10.0
+##
+{'enum': 'BlockDeviceStatus', 'data': ['free',
'used', 'unknown']}
+
+##
 # @BlockDeviceInfo:
 #
 # Block device information.
@@ -1451,6 +1471,8 @@
 # @pvsize:             The LVM physical volume size (in bytes)
 #                      (new in version 4.17)
 #
+# @status:             The device status (free/used/unknown)
+#
 # Since: 4.10.0
 #
 # Notes:  The value of @serial may be dependent on the current host so this
@@ -1468,7 +1490,7 @@
           'pathstatus': ['BlockDevicePathInfo'],
           'pathlist': ['IscsiSessionInfo'], 'logicalblocksize':
'uint',
           'physicalblocksize': 'uint', 'partitioned':
'bool',
-          'pvsize': 'uint'}}
+          'pvsize': 'uint', 'status':
'BlockDeviceStatus'}}
 
 ##
 # @Host.getDeviceList:
@@ -1480,6 +1502,9 @@
 # @guids:        #optional Only return info on specific list of block device
 #                 GUIDs  (new in version 4.17)
 #
+# @checkStatus:  #optional Indicates if device status should be checked
+#                 (new in version 4.17.5)
+#
 # Returns:
 # An array of @BlockDeviceInfo
 #
@@ -1487,7 +1512,8 @@
 ##
 {'command': {'class': 'Host', 'name':
'getDeviceList'},
  'data': {'*storageType': 'BlockDeviceType',
-          '*guids': ['str']},
+          '*guids': ['str'],
+          '*checkStatus': 'bool'},
  'returns': ['BlockDeviceInfo']}
 
 ##
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 10409dd..191cf61 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1957,7 +1957,8 @@
                                       masterVersion, leaseParams)
 
     @public
-    def getDeviceList(self, storageType=None, guids=(), options={}):
+    def getDeviceList(self, storageType=None, guids=(), checkStatus=True,
+                      options={}):
         """
         List all Block Devices.
 
@@ -1965,6 +1966,12 @@
         :type storageType: Some enum?
         :param guids: List of device GUIDs to retrieve info.
         :type guids: list
+        :param checkStatus: if true the status will be checked for the given
+                            devices. This operation is an expensive operation
+                            and should be used only with specific devices
+                            using the guids argument.The default is True for
+                            backward compatibility.
+        :type checkStatus: bool
         :param options: ?
 
         :returns: Dict containing a list of all the devices of the storage
@@ -1972,10 +1979,11 @@
         :rtype: dict
         """
         vars.task.setDefaultException(se.BlockDeviceActionError())
-        devices = self._getDeviceList(storageType=storageType, guids=guids)
+        devices = self._getDeviceList(storageType=storageType, guids=guids,
+                                      checkStatus=checkStatus)
         return dict(devList=devices)
 
-    def _getDeviceList(self, storageType=None, guids=()):
+    def _getDeviceList(self, storageType=None, guids=(), checkStatus=True):
         sdCache.refreshStorage()
         typeFilter = lambda dev: True
         if storageType:
@@ -2028,24 +2036,27 @@
                        'pathlist': dev.get("connections", []),
                        'logicalblocksize': dev.get("logicalblocksize",
""),
                        'physicalblocksize':
dev.get("physicalblocksize", "")}
+            if not checkStatus:
+                devInfo["status"] = "unknown"
             devices.append(devInfo)
 
-        # Look for devices that will probably fail if pvcreated.
-        devNamesToPVTest = tuple(dev["GUID"] for dev in devices)
-        unusedDevs, usedDevs = lvm.testPVCreate(
-            devNamesToPVTest, metadataSize=blockSD.VG_METADATASIZE)
-        # Assuming that unusables v unusables = None
-        free = tuple(os.path.basename(d) for d in unusedDevs)
-        used = tuple(os.path.basename(d) for d in usedDevs)
-        for dev in devices:
-            guid = dev['GUID']
-            if guid in free:
-                dev['status'] = "free"
-            elif guid in used:
-                dev['status'] = "used"
-            else:
-                raise KeyError("pvcreate response foresight is "
-                               "can not be determined for %s", dev)
+        if checkStatus:
+            # Look for devices that will probably fail if pvcreated.
+            devNamesToPVTest = tuple(dev["GUID"] for dev in devices)
+            unusedDevs, usedDevs = lvm.testPVCreate(
+                devNamesToPVTest, metadataSize=blockSD.VG_METADATASIZE)
+            # Assuming that unusables v unusables = None
+            free = tuple(os.path.basename(d) for d in unusedDevs)
+            used = tuple(os.path.basename(d) for d in usedDevs)
+            for dev in devices:
+                guid = dev['GUID']
+                if guid in free:
+                    dev['status'] = "free"
+                elif guid in used:
+                    dev['status'] = "used"
+                else:
+                    raise KeyError("pvcreate response foresight is "
+                                   "can not be determined for %s", dev)
 
         return devices
 


-- 
To view, visit https://gerrit.ovirt.org/45759
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic28954708f2fd7c7b721aa7f9a0fb6e1a6019597
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Fred Rolland <frolland(a)redhat.com&gt;

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011