Nir Soffer has posted comments on this change.

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


Patch Set 3: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/45093/3/vdsm/rpc/vdsmapi-schema.json
File vdsm/rpc/vdsmapi-schema.json:

Line 1426: # @used:     The device is in use.
Line 1427: #            A device will be considered used on the following cases:
Line 1428: #            - a PV exists on this device and is part of a VG
Line 1429: #            - a partition table is found on the device
Line 1430: #            - The device is not accessible
This does not make sense - why do we consider inaccessible device as used?
Line 1431: #
Line 1432: # @unknown:  The device status is unknown
Line 1433: #            (new in version 4.17)
Line 1434: #


Line 1429: #            - a partition table is found on the device
Line 1430: #            - The device is not accessible
Line 1431: #
Line 1432: # @unknown:  The device status is unknown
Line 1433: #            (new in version 4.17)
Did you check if old engine can handle this response?
Line 1434: #
Line 1435: # Since: 4.10.0
Line 1436: ##
Line 1437: {'enum': 'BlockDeviceStatus', 'data': ['free', 'used', 'unknown']}


https://gerrit.ovirt.org/#/c/45093/3/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1967:         :param checkStatus: Check the status of the devices. This 
operation is
Line 1968:                             an expensive operation and should be 
used only with
Line 1969:                             specific devices using the guids 
argument.
Line 1970:                             The default is True for backward 
compatibility.
Line 1971:         :type bool: if true the status will be checked for the given 
devices.
:type checkStatus: bool

:type ...: sections should not include documentation, only type names. The line 
"if true..." should be part of the :param ...: section.
Line 1972:         :param guids: List of device GUIDs to retrieve info.
Line 1973:         :type guids: list
Line 1974:         :param options: ?
Line 1975: 


Line 2023:                 pvsize = ""
Line 2024:                 vguuid = ""
Line 2025: 
Line 2026:             devInfo = {'GUID': dev.get("guid", ""), 'pvUUID': pvuuid,
Line 2027:                        'status': 'unknown',
This may break old engine - please check that old engine can cope with this 
value.

For example, if engine look only for "used" or "free", it will treat "unknown" 
as the other value, which may be correct or not.
Line 2028:                        'pvsize': str(pvsize),
Line 2029:                        'vgUUID': vguuid, 'vendorID': 
dev.get("vendor", ""),
Line 2030:                        'productID': dev.get("product", ""),
Line 2031:                        'fwrev': dev.get("fwrev", ""),


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

Reply via email to