Nir Soffer has posted comments on this change. Change subject: hsm: Support checkStatus param in getDeviceList ......................................................................
Patch Set 2: Code-Review-1 (11 comments) https://gerrit.ovirt.org/#/c/45093/2/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 1477 Line 1478 Line 1479 Line 1480 Line 1481 In another patch, fix this version to 4.17.0. Line 1416: 'data': {'connection': 'str', 'port': 'str', 'iqn': 'str', 'portal': 'str', Line 1417: 'initiatorname': 'str', '*username': 'str', '*password': 'str'}} Line 1418: Line 1419: ## Line 1420: # @BlockDeviceStatus: Thanks nice addition! But the text does not explain anything. We must be very specific what is the meaning of "used". I think that "used" means devices recognized by lvm as pvs (containing a pv header), but maybe I'm wrong and we only pvs which are part of a vg are considered used - please check the underlying implementation to understand the behavior of the current code. Additionally, I think that devides with a partition table are considered as used. Line 1421: # Line 1422: # Enumeration of possible status for a block device. Line 1423: # Line 1424: # @free: The device is free Line 1424: # @free: The device is free Line 1425: # Line 1426: # @used: The device is in use Line 1427: # Line 1428: # @unknown: The device is in use The device status is unknown (New in version 4.17.?) Check with Dan which version should appear here. Line 1429: # Line 1430: # Since: 4.10.0 Line 1431: ## Line 1432: {'enum': 'BlockDeviceStatus', 'data': ['free', 'used', 'unknown']} Line 1493: # Get information about all block devices. Line 1494: # Line 1495: # @storageType: #optional Only return devices of this type Line 1496: # Line 1497: # @checkStatus: #optional Indicates if device status should be checked Align with previous line Line 1498: # (new in version 4.17) Line 1499: # Line 1500: # @guids: #optional Only return info on specific list of block device Line 1501: # GUIDs (new in version 4.17) Line 1494: # Line 1495: # @storageType: #optional Only return devices of this type Line 1496: # Line 1497: # @checkStatus: #optional Indicates if device status should be checked Line 1498: # (new in version 4.17) Version should be more specific, check with Dan what will be the next version where this patch should be available. Line 1499: # Line 1500: # @guids: #optional Only return info on specific list of block device Line 1501: # GUIDs (new in version 4.17) Line 1502: # https://gerrit.ovirt.org/#/c/45093/2/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 2026 Line 2027 Line 2028 Line 2029 Line 2030 Add 'status': 'unknown' here Line 1957: masterVersion, leaseParams) Line 1958: Line 1959: @public Line 1960: def getDeviceList(self, storageType=None, checkStatus=True, Line 1961: guids=(), options={}): Are you sure guids=() does not fit in the previous line? Line 1962: """ Line 1963: List all Block Devices. Line 1964: Line 1965: :param storageType: Filter by storage type. Line 1963: List all Block Devices. Line 1964: Line 1965: :param storageType: Filter by storage type. Line 1966: :type storageType: Some enum? Line 1967: :param testPvCreate: Test PV creation. Please update the name here and elsewhere. Line 1968: :type bool: if true PV creation will be tested for the given devices Line 1969: :param guids: List of device GUIDs to retrieve info. Line 1970: :type guids: list Line 1971: :param options: ? Line 1964: Line 1965: :param storageType: Filter by storage type. Line 1966: :type storageType: Some enum? Line 1967: :param testPvCreate: Test PV creation. Line 1968: :type bool: if true PV creation will be tested for the given devices if True, device status will be checked. Please do not promise how we check this in the documentation, so we don't have to change it if we change the implementation. Also explain that checkStatus is an expensive operation and it should be used only with specific devices using the guids argument. Also explain why the default is True (we just explained why it should be actually False). Line 1969: :param guids: List of device GUIDs to retrieve info. Line 1970: :type guids: list Line 1971: :param options: ? Line 1972: Line 2037: if checkStatus: Line 2038: # Look for devices that will probably fail if pvcreated. Line 2039: devNamesToPVTest = tuple(dev["GUID"] for dev in devices) Line 2040: unusedDevs, usedDevs = lvm.testPVCreate( Line 2041: devNamesToPVTest, metadataSize=blockSD.VG_METADATASIZE) It seems that this is only a partial fix, since it we seem to call pvCreate on devices which are known pvs (has pv uuid) and maybe even part of a vg (have vg uuid). If we change the logic to check only devices which are not pvs (lvm does not recognize them as pvs), we can get nice performance improvement for older engine without changing the engine side. Lets tackle this problem in another patch. Line 2042: # Assuming that unusables v unusables = None Line 2043: free = tuple(os.path.basename(d) for d in unusedDevs) Line 2044: used = tuple(os.path.basename(d) for d in usedDevs) Line 2045: for dev in devices: Line 2052: raise KeyError("pvcreate response foresight is " Line 2053: "can not be determined for %s", dev) Line 2054: else: Line 2055: for dev in devices: Line 2056: dev['status'] = "unknown" This works, but ugly - better initialize status to 'unknown' when the dev dict is created. Line 2057: Line 2058: return devices Line 2059: Line 2060: @public -- To view, visit https://gerrit.ovirt.org/45093 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic28954708f2fd7c7b721aa7f9a0fb6e1a6019597 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <[email protected]> Gerrit-Reviewer: Fred Rolland <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> 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
