Nir Soffer has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 24: (13 comments) https://gerrit.ovirt.org/#/c/37509/24//COMMIT_MSG Commit Message: Line 7: v2v: Convert VM from external source to Data Domain Line 8: Line 9: new verb: convert VM from external source (non-kvm) to Data Domain. Line 10: The convertVmFromExternalSystem() is implemented in v2v module and use Line 11: the virt-v2v external tool to do actual VM conversion. The verb was renamed, please show the current name. Line 12: Line 13: More information can be view at the feature page: Line 14: http://www.ovirt.org/Features/virt-v2v_Integration Line 15: https://gerrit.ovirt.org/#/c/37509/24/client/vdsClient.py File client/vdsClient.py: Line 1920: def _parse_properties(self, properties): Line 1921: d = ast.literal_eval(properties) Line 1922: if type(d) != dict: Line 1923: raise ValueError("Invalid option %r" % properties) Line 1924: return d This works, but has nothing to do with this class. Better make this a module function. With a more generic name (e.g, parse_dict), this can be used by other methods that currently use ast.literal_eval. But please do not change other methods in this patch. Replacing duplicate usage of ast.literal_eval with parse_dict should be done in a separate patch. Line 1925: Line 1926: def convertExternalVm(self, args): Line 1927: validateArgTypes(args, [str, str, str, self._parse_properties, str], Line 1928: requiredArgsNumber=5) Line 2811: 'get VMs from external hypervisor' Line 2812: )), Line 2813: 'convertExternalVm': ( Line 2814: serv.convertExternalVm, ( Line 2815: '<uri> <username> <password> [auth=] <vmProperties> <jobId>', It is not clear what is [auth=]. Better use: <uri> <username> <auth> <vmProperties> <jobId> And explain in the text what is auth, and show examples of possible usage. Line 2816: 'Import and convert VM from external system ' Line 2817: ' vmProperties need to be is a string dictionary, ie:' Line 2818: ' {"vmName": "myVm", "disks": {"poolId": "AABB..."}}' Line 2819: 'If auth argument is provided, password will be ' Line 2816: 'Import and convert VM from external system ' Line 2817: ' vmProperties need to be is a string dictionary, ie:' Line 2818: ' {"vmName": "myVm", "disks": {"poolId": "AABB..."}}' Line 2819: 'If auth argument is provided, password will be ' Line 2820: 'ignored (yet has to be specified, ie -)' The code does not handle auth argument - either this this description is wrong, or the code is. Line 2821: )), Line 2822: } Line 2823: if _glusterEnabled: Line 2824: commands.update(ge.getGlusterCmdDict(serv)) Line 2817: ' vmProperties need to be is a string dictionary, ie:' Line 2818: ' {"vmName": "myVm", "disks": {"poolId": "AABB..."}}' Line 2819: 'If auth argument is provided, password will be ' Line 2820: 'ignored (yet has to be specified, ie -)' Line 2821: )), This help message is not very helpful, like most help messages here. Lets have more useful - see the documentation for downloadImage for good example. Please separate this change (adding convertExternalVm to vdsClient) to a new patch. This patch is too big and this is the reason it takes so much time to get it merged. We like small and easy to review patches. Line 2822: } Line 2823: if _glusterEnabled: Line 2824: commands.update(ge.getGlusterCmdDict(serv)) Line 2825: https://gerrit.ovirt.org/#/c/37509/24/lib/vdsm/constants.py.in File lib/vdsm/constants.py.in: Line 132 Line 133 Line 134 Line 135 Line 136 This is not related to this patch - please do this in another patch. https://gerrit.ovirt.org/#/c/37509/24/vdsm/API.py File vdsm/API.py: Line 1405: """ Line 1406: return v2v.get_external_vms(uri, username, password) Line 1407: Line 1408: def convertExternalVm(self, uri, username, password, Line 1409: vmProperties, jobId): No need to break this line, it fits in one line (78 chars) Line 1410: return v2v.convert_external_vm(uri, username, password, vmProperties, Line 1411: jobId, self._cif.irs) Line 1412: Line 1413: # Networking-related functions https://gerrit.ovirt.org/#/c/37509/24/vdsm/rpc/BindingXMLRPC.py File vdsm/rpc/BindingXMLRPC.py: Line 364: api = API.Global() Line 365: return api.getExternalVMs(uri, username, password) Line 366: Line 367: def convertExternalVm(self, uri, username, password, Line 368: vmProperties, jobId): This fits in one line. Line 369: api = API.Global() Line 370: return api.convertExternalVm(uri, username, password, Line 371: vmProperties, jobId) Line 372: Line 367: def convertExternalVm(self, uri, username, password, Line 368: vmProperties, jobId): Line 369: api = API.Global() Line 370: return api.convertExternalVm(uri, username, password, Line 371: vmProperties, jobId) vmProperties should be on the previous line. Line 372: Line 373: def vmPause(self, vmId): Line 374: vm = API.VM(vmId) Line 375: return vm.pause() https://gerrit.ovirt.org/#/c/37509/24/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3820: # Since: 4.17.0 Line 3821: ## Line 3822: {'command': {'class': 'Host', 'name': 'convertExternalVm'}, Line 3823: 'data': {'uri': 'str', 'username': 'str', 'password': 'str', Line 3824: 'vmProperties': 'ExternalVmInfo', 'jobId': 'UUID'}} Lets rename "vmProperties" to "vmInfo" - it it much shorter, and more correct, as this an instance of ExternalVmInfo. Line 3825: Line 3826: ## Line 3827: # @VMFullInfo: Line 3828: # https://gerrit.ovirt.org/#/c/37509/24/vdsm/v2v.py File vdsm/v2v.py: Line 330: self._status = STATUS.ABORTED Line 331: logging.info('Job %r aborting...', self._id) Line 332: self._abort() Line 333: Line 334: def _abort(self): This looks fine now, but we need some logs. Line 335: self._aborted = True Line 336: if self._proc.returncode is None: Line 337: try: Line 338: self._proc.kill() Line 332: self._abort() Line 333: Line 334: def _abort(self): Line 335: self._aborted = True Line 336: if self._proc.returncode is None: Add debug log - "Job %r killing virt-v2v process" Line 337: try: Line 338: self._proc.kill() Line 339: except OSError as e: Line 340: if e.errno != errno.ESRCH: Line 337: try: Line 338: self._proc.kill() Line 339: except OSError as e: Line 340: if e.errno != errno.ESRCH: Line 341: raise Add debug log - "Job %r virt-v2v process not running" And add debug log for the successful code path (when self._proc.kill() did not raise). else: log debug "Job %r virt-v2v process was killed" Line 342: finally: Line 343: zombiereaper.autoReapPID(self._proc.pid) Line 344: Line 345: def _generate_disk_parameters(self): -- To view, visit https://gerrit.ovirt.org/37509 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea Gerrit-PatchSet: 24 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
