Nir Soffer has posted comments on this change. Change subject: hsm: Support GUID list param in GetDeviceList ......................................................................
Patch Set 1: (8 comments) https://gerrit.ovirt.org/#/c/40661/1/client/vdsClient.py File client/vdsClient.py: Line 717 Line 718 Line 719 Line 720 Line 721 Old code supported *no* argument, this code require at least 1 argument. Line 30: import string Line 31: import pprint as pp Line 32: Line 33: from vdsm import utils, vdscli Line 34: from storage import sd We cannot import stuff from the storage package, vdsClient does not have access to this package. If you need constants from storage, move them to lib/vdsm/constants.py Line 35: try: Line 36: import vdsClientGluster as ge Line 37: _glusterEnabled = True Line 38: except ImportError: Line 737: storageType = args[0] Line 738: else: Line 739: raise ValueError('Invalid argument "%s". Not a valid ' Line 740: 'storage type' % args[0]) Line 741: devList = args[1].split(',') I don't understand what are you trying to do, and it looks too complicated. See the comment in the help section on how to simplify the command line interface. Line 742: Line 743: devices = self.s.getDeviceList(storageType, devList) Line 744: Line 745: if devices['status']['code']: Line 2231: 'getDeviceList': (serv.getDeviceList, Line 2232: ('[storageType]', Line 2233: '[<devlist>]', Line 2234: 'List of all block devices (optionally - matching ' Line 2235: 'storageType, optionally - of each device listed).' This interface, optional storageType and optional device list does not make sense, too many combinations, and it is not clear what is each argument. You should allow the guids list only if storage type was specified. So: - len(args) == 0: call getDeviceList() - no checking needed - len(args) == 1: call getDeviceList(type) - no checking needed - len(args) == 2: check that type is block storage type, parse device list, call getDeviceList(type, guids) Line 2236: )), Line 2237: 'getDevicesVisibility': (serv.getDevicesVisibility, Line 2238: ('<devlist>', Line 2239: 'Get visibility of each device listed' https://gerrit.ovirt.org/#/c/40661/1/vdsm/API.py File vdsm/API.py: Line 1611: Line 1612: def getLVMVolumeGroups(self, storageType=None): Line 1613: return self._irs.getVGList(storageType) Line 1614: Line 1615: def getDeviceList(self, storageType=None, guidList=None): Use "guids" like we use in other places. Do not include the type in the name. Line 1616: return self._irs.getDeviceList(storageType, guidList) Line 1617: Line 1618: def getDevicesVisibility(self, guidList): Line 1619: return self._irs.getDevicesVisibility(guidList) https://gerrit.ovirt.org/#/c/40661/1/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 1481: # Since: 4.10.0 Line 1482: ## Line 1483: {'command': {'class': 'Host', 'name': 'getDeviceList'}, Line 1484: 'data': {'*storageType': 'BlockDeviceType', Line 1485: '*guidList': ['UUID']}, - The parameter is called "guids" in the function - the schema must match the code. - This is not UUID - use 'str' Line 1486: 'returns': ['BlockDeviceInfo']} Line 1487: Line 1488: ## Line 1489: # @DeviceVisibilityMap: https://gerrit.ovirt.org/#/c/40661/1/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 2028: lambda dev: multipath.devIsFCP(dev.get("devtype")) Line 2029: Line 2030: devices = [] Line 2031: pvs = {} Line 2032: if guids is not None and len(guids) > 0: No need to check is None and len() > 0. Just use: if guids: use provided guids... else: get all pvs... Line 2033: for guid in guids: Line 2034: try: Line 2035: pv = lvm.getPV(guid) Line 2036: except se.StorageException: https://gerrit.ovirt.org/#/c/40661/1/vdsm/storage/multipath.py File vdsm/storage/multipath.py: Line 188: return HBTL(*hbtl[0].split(":")) Line 189: Line 190: Line 191: def pathListIter(filterGuids=None): Line 192: filteringOn = filterGuids is not None and len(filterGuids) > 0 No need for None and len() checks. We can replace the fileringOn with filterGuids. If filterGuids is None or empty list, it is falsy, otherwise it is truthy and we need to filter. Line 193: filterLen = len(filterGuids) if filteringOn else -1 Line 194: devsFound = 0 Line 195: Line 196: knownSessions = {} -- To view, visit https://gerrit.ovirt.org/40661 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic173d94a132e617ae97353d38520a86bede657d7 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <[email protected]> Gerrit-Reviewer: Fred Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
