Adam Litke has posted comments on this change. Change subject: schema: Updates based on the current code base ......................................................................
Patch Set 3: Code-Review-1 (25 comments) http://gerrit.ovirt.org/#/c/23125/3//COMMIT_MSG Commit Message: Line 6: Line 7: schema: Updates based on the current code base Line 8: Line 9: This patch was requested by ybronhei to push vdsm and gluster schema Line 10: updates which were discovered during jsonrpc testing. > it doesn't mean anything. what updates? Agree. It should be possible to list the affected calls / types. Line 11: Line 12: This patch contains as well fix for snapMemVolHandle which should be Line 13: optional. Line 14: http://gerrit.ovirt.org/#/c/23125/3/vdsm/gluster/vdsmapi-gluster-schema.json File vdsm/gluster/vdsmapi-gluster-schema.json: Line 495: # Volume profile information. Line 496: # Line 497: # @volumeName: volume name Line 498: # Line 499: # @bricks: list of bricks Forgive my ignorance regarding Gluster. What is the format of a 'brick'? Is it a UUID, a unique name? If this is used a lot in the Gluster API it may make sense to create a alias type like we have for UUID in the vdsm api. Not required for this patch but something to consider. Line 500: # Line 501: # Since: 4.10.3 Line 502: ## Line 503: {'type': 'VolumeProfileInfo', Line 507: # @VolumeOptionInfo: Line 508: # Line 509: # Volume option information. Line 510: # Line 511: # @name: volume name Is this supposed to be 'option name'? Line 512: # Line 513: # @defaultValue: default value Line 514: # Line 515: # @description: description Line 568: # Line 569: # @brick: #optional Brick name Line 570: # Line 571: # @statusOption: #optional Provide one of possible values: Line 572: # DETAIL, CLIENTS, MEM Since you have enumerated the possible values for statusOption, it should be defined as an enum instead of a str. Line 573: # Line 574: # Returns: Line 575: # Gluster volume status Line 576: # Line 572: # DETAIL, CLIENTS, MEM Line 573: # Line 574: # Returns: Line 575: # Gluster volume status Line 576: # Are you returning a single VolumeStatus or a list of them? The doc string says a single one but the definition below says a list. Line 577: # Since: 4.10.3 Line 578: ## Line 579: {'command': {'class': 'GlusterVolume', 'name': 'status'}, Line 580: 'data': {'volumeName': 'str', '*brick': 'str', '*statusOption': 'str'}, Line 587: # Line 588: # @volumeName: Gluster volume name Line 589: # Line 590: # @nfs: #optional Line 591: # It's optional, but what is it? Missing documentation string. Line 592: # Returns: Line 593: # Gluster volume status Line 594: # Line 595: # Since: 4.10.3 Line 595: # Since: 4.10.3 Line 596: ## Line 597: {'command': {'class': 'GlusterVolume', 'name': 'profileInfo'}, Line 598: 'data': {'volumeName': 'str', '*nfs': 'str'}, Line 599: 'returns': ['VolumeProfileInfo']} Again, did you really mean a list here? Line 600: Line 601: ## Line 602: # @GlusterVolume.list: Line 603: # Line 706: 'data': {'volumeName': 'str', 'option': 'str', 'value': 'str'}, Line 707: 'returns': 'bool'} Line 708: Line 709: ## Line 710: # @GlusterVolume.setOptionsList: This is a horrible name for this API. setOptionsList implies that I would use this to set a list of options. Can you change this to getOptionsList or something that makes it clearer you are retrieving something? Line 711: # Line 712: # List Gluster volume options Line 713: # Line 714: # Returns: Line 891: # Line 892: # @brickList: List of bricks Line 893: # Line 894: # @replicaCount: #optional Line 895: # Need to provide the simple description of this parameter. Line 896: # Returns: Line 897: # Status information of remove brick operation Line 898: # Line 899: # Since: 4.10.3 Line 989: # Line 990: # @volumeName: Gluster volume name Line 991: # Line 992: # @force: #optional Force flag Line 993: # It would be better if you could say what sort of check the force flag bypasses. Line 994: # Returns: Line 995: # Gluster volume rebalance status Line 996: # Line 997: # Since: 4.10.3 Line 1011: # Line 1012: # @newBrick: New brick Line 1013: # Line 1014: # Returns: Line 1015: # Success or failure For commands that return true or false, the result of the command can be determined by ['status']['code'] so you shouldn't need to return anything at all. Line 1016: # Line 1017: # Since: 4.10.3 Line 1018: ## Line 1019: {'command': {'class': 'GlusterVolume', 'name': 'replaceBrickStart'}, http://gerrit.ovirt.org/#/c/23125/3/vdsm_api/vdsmapi-schema.json File vdsm_api/vdsmapi-schema.json: Line 1687 Line 1688 Line 1689 Line 1690 Line 1691 Ditto. Line 1831 Line 1832 Line 1833 Line 1834 Line 1835 You need to update Since because you are presumably breaking compatibility. Line 1836 Line 1837 Line 1838 Line 1839 Line 1840 Can you explain how this is used in practice? It's a new API concept which I think deserves some documentation somewhere. Line 1021: # Line 1022: # Returns: Line 1023: # Host hardware information Line 1024: # Line 1025: # Since: 4.10.0 This was added by 5fdbaf5e0703308abe96601cd4102d67f6696426 and according to git describe, the released version should be 4.11.0 Line 1026: ## Line 1027: {'command': {'class': 'Host', 'name': 'getHardwareInfo'}, Line 1028: 'returns': 'HardwareInformation'} Line 1029: Line 1051: # Line 1052: # @MIXED: The device consists of a mix of iSCSI and FibreChannel paths Line 1053: # Line 1054: # Since: 4.10.0 Line 1055: # Values allowed : 0, 1, 2, 3, 4, 6, 7, 8, -1 What is this? How is it handled by process-schema.py and/or the jsonrpc binding? If numbers are allowed to be passed into the API then they should be added to the enum. Line 1056: ## Line 1057: {'enum': 'BlockDeviceType', 'data': ['iSCSI', 'FCP', 'MIXED']} Line 1058: Line 1059: ## Line 1675: # @Iso: The Storage Domain is used for storing ISO images Line 1676: # @Backup: The Storage Domain is used for import and export of disk images Line 1677: # Line 1678: # Since: 4.10.0 Line 1679: # As in the engine Master(1), Data(2), ISO(3), ImportExport(4), Image(5), Unknown(6) This looks similar to the 'Values allowed' thing above. Why is it important to the API what numbers vdsm may internally map these strings to? It seems like we are exposing an implementation detail here. If we are missing some types (master, importexport, image) then add them to the enum itself. Line 1680: ## Line 1681: {'enum': 'StorageDomainImageClass', Line 1682: 'data': ['Unknown', 'Data', 'Iso', 'Backup']} Line 1683: Line 2968: 'data': {'*vmList': ['UUID']}, Line 2969: 'returns': ['UUID']} Line 2970: Line 2971: ## Line 2972: # @VMFullList: This isn't a list :-/ How about VmFullInfo? Isn't this the same thing as VmDefinition? Since it is such a large object, it would be nice if we could reuse it. Line 2973: # Line 2974: # Full information about VM. Line 2975: # Line 2976: # @acpiEnable: Indicates if ACPI is enabled inside the VM Line 3043: # @status: State of the VM Line 3044: # Line 3045: # @clientIp: The IP address of the client connected to the display Line 3046: # Line 3047: # Since: 4.10.0 Wrong version: use git-blame and git-describe to find the correct one. Line 3048: ## Line 3049: {'type': 'VMFullList', Line 3050: 'data': {'acpiEnable': 'bool', 'custom': 'StringMap', 'devices': ['VmDevice'], Line 3051: 'display': 'VmDisplayType', 'kvmEnable': 'bool', 'memSize': 'uint', Line 3061: 'status': 'str', 'clientIp': 'str'}} Line 3062: Line 3063: ## Line 3064: # @Host.getVMFullList: Line 3065: # When talking about a list, Full implies return all items not return detailed items. How about getVmDetailsList? Line 3066: # Get full information about the current virtual machines. Line 3067: # Line 3068: # @vmList: #optional Filter the results by a list of UUIDs Line 3069: # Line 3067: # Line 3068: # @vmList: #optional Filter the results by a list of UUIDs Line 3069: # Line 3070: # Returns: Line 3071: # A list of VM UUIDs :) You added this because you wanted the full details returned, not just the UUIDs Line 3072: # Line 3073: # Since: 4.10.0 Line 3074: ## Line 3075: {'command': {'class': 'Host', 'name': 'getVMFullList'}, Line 3070: # Returns: Line 3071: # A list of VM UUIDs Line 3072: # Line 3073: # Since: 4.10.0 Line 3074: ## wrong version. Line 3075: {'command': {'class': 'Host', 'name': 'getVMFullList'}, Line 3076: 'data': {'*vmList': ['UUID']}, Line 3077: 'returns': ['VMFullList']} Line 3078: Line 3084: # Returns: Line 3085: # A list of stats for all VMs Line 3086: # Line 3087: # Since: 4.10.0 Line 3088: ## wrong version Line 3089: {'command': {'class': 'Host', 'name': 'getAllVmStats'}, Line 3090: 'returns': ['VmStats']} Line 3091: Line 3092: ## http://gerrit.ovirt.org/#/c/23125/3/vdsm_api/vdsmapi.py File vdsm_api/vdsmapi.py: Line 230 Line 231 Line 232 Line 233 Line 234 Why is this removed? Line 118: Find the API schema file whether we are running from within the source dir Line 119: or from an installed location Line 120: """ Line 121: # Don't depend on module VDSM if not looking for schema Line 122: from vdsm import constants Thank you! Line 123: Line 124: localpath = os.path.dirname(__file__) Line 125: installedpath = constants.P_VDSM Line 126: for directory in localpath, installedpath: -- To view, visit http://gerrit.ovirt.org/23125 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id3d0d894cc31395510733a33f4f6e15276615a3a Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Timothy Asir <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
