Nir Soffer has posted comments on this change. Change subject: hsm: Support testPvCreate param in getDeviceList ......................................................................
Patch Set 1: (8 comments) https://gerrit.ovirt.org/#/c/45093/1//COMMIT_MSG Commit Message: Line 6: Line 7: hsm: Support testPvCreate param in getDeviceList Line 8: Line 9: Add optional 'testPvCreate' boolean parameter to getDeviceList. Line 10: By default it will be True to keep same behavior as before. Please explain why this is needed, and provide some measurements how this effects the user experience on a system with lot of devices. Line 11: Line 12: If specified as False, the PV create test will be skipped and the Line 13: 'status' field will not be populated. Line 14: https://gerrit.ovirt.org/#/c/45093/1/client/vdsClient.py File client/vdsClient.py: Line 735: elif len(args) == 1: Line 736: res = self.s.getDeviceList(args[0]) Line 737: else: Line 738: res = self.s.getDeviceList(args[0], Line 739: utils.tobool(args[1]), args[2:]) Check if utils.tobool(args[1]) can be on the first line, this indentation looks strange. Line 740: Line 741: if res['status']['code']: Line 742: return res['status']['code'], res['status']['message'] Line 743: pp.pprint(res['devList']) Line 2282: ('[storageType]', Line 2283: '[testPvCreate]', Line 2284: '[<devlist>]', Line 2285: 'List of all block devices (optionally - matching ' Line 2286: 'storageType, optionally - perform PV create test ' update name Line 2287: 'optionally - of each device listed).', Line 2288: ' getDeviceList', Line 2289: ' return all devices', Line 2290: ' getDeviceList FCP', https://gerrit.ovirt.org/#/c/45093/1/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 1487 Line 1488 Line 1489 Line 1490 Line 1491 This type should be updated now, as status is optional, or it may return third value when status was not checked. Line 1477: # Line 1478: # @storageType: #optional Only return devices of this type Line 1479: # Line 1480: # @testPvCreate: #optional Indicates if PV creation test should be performed Line 1481: # (new in version 4.17) Update to describe what this does instead of how. Line 1482: # Line 1483: # @guids: #optional Only return info on specific list of block device Line 1484: # GUIDs (new in version 4.17) Line 1485: # https://gerrit.ovirt.org/#/c/45093/1/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 1956: return pool.reconstructMaster(hostId, poolName, masterDom, domDict, Line 1957: masterVersion, leaseParams) Line 1958: Line 1959: @public Line 1960: def getDeviceList(self, storageType=None, testPvCreate=True, This is too specific, what we want is to check the device status, so lets call this parameter checkStatus (or another name that explain what it does, and not how it does it). Line 1961: guids=(), options={}): Line 1962: """ Line 1963: List all Block Devices. Line 1964: Line 1978: devices = self._getDeviceList(storageType=storageType, Line 1979: testPvCreate=testPvCreate, guids=guids) Line 1980: return dict(devList=devices) Line 1981: Line 1982: def _getDeviceList(self, storageType=None, testPvCreate=True, guids=()): Update to new parameter name. Line 1983: sdCache.refreshStorage() Line 1984: typeFilter = lambda dev: True Line 1985: if storageType: Line 1986: if sd.storageType(storageType) == sd.type2name(sd.ISCSI_DOMAIN): Line 2046: guid = dev['GUID'] Line 2047: if guid in free: Line 2048: dev['status'] = "free" Line 2049: elif guid in used: Line 2050: dev['status'] = "used" This will not report device status now when new engine call with checkStatus=False, so the schema should be update to explain this behavior. Or, you can report dev["status"] = "unknown", which simplifies the schema, and probably makes the client code simpler. Line 2051: else: Line 2052: raise KeyError("pvcreate response foresight is " Line 2053: "can not be determined for %s", dev) Line 2054: -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
