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 <frolland(a)redhat.com>
Gerrit-Reviewer: Fred Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes