Shahar Havivi has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 20: (9 comments) https://gerrit.ovirt.org/#/c/37509/20/client/vdsClient.py File client/vdsClient.py: Line 1917: Line 1918: return status['status']['code'], status['status']['message'] Line 1919: Line 1920: def convertExternalVm(self, args): Line 1921: validateArgTypes(args, [str, str, str, str, {}, str]) > dict is closer, but will not work. Done Line 1922: if len(args) != 5: Line 1923: raise ValueError('Wrong number of arguments') Line 1924: uri, username, password, vmProperties, jobId = args Line 1925: password = getAuthFromArgs(args, password) Line 1919: Line 1920: def convertExternalVm(self, args): Line 1921: validateArgTypes(args, [str, str, str, str, {}, str]) Line 1922: if len(args) != 5: Line 1923: raise ValueError('Wrong number of arguments') > This should be handled inside validateArgsTypes, using requiredArgsNumber=5 Done Line 1924: uri, username, password, vmProperties, jobId = args Line 1925: password = getAuthFromArgs(args, password) Line 1926: response = self.s.convertExternalVm(uri, username, password, Line 1927: vmProperties, jobId) https://gerrit.ovirt.org/#/c/37509/20/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3806: # Line 3807: # @vmProperties: information of the VM such as name, id etc Line 3808: # Line 3809: # @jobId: Assign a UUID to this operation which can be used Line 3810: # to identify it in @VmStats > You mean HostStats, right? yes.. Line 3811: # Line 3812: # Since: 4.17.0 Line 3813: ## Line 3814: {'command': {'class': 'Host', 'name': 'convertExternalVm'}, https://gerrit.ovirt.org/#/c/37509/20/vdsm/v2v.py File vdsm/v2v.py: Line 247: try: Line 248: self._import() Line 249: except Exception as ex: Line 250: logging.exception('Job %r, error importing vm: %r', Line 251: self._id, ex) > This good idea since we have more context here, and can give better log tha Sure, I added this log when I did encounter error in importing... Line 252: if not self._aborted: Line 253: self._status = STATUS.FAILED Line 254: self._description = ex.message Line 255: try: Line 256: self._abort() Line 257: except Exception as e: Line 258: logging.exception('Job %r, error trying to abort: %r', Line 259: self._id, e) Line 260: raise > Remove this raise, since we already handled the failure. Done Line 261: finally: Line 262: self._teardown_volumes() Line 263: Line 264: def _import(self): Line 335: self._aborted = True Line 336: if self._proc.returncode is None: Line 337: try: Line 338: self._proc.kill() Line 339: except Exception as e: > You want to handle only OSError. Done Line 340: if e.errno == errno.ESRCH: Line 341: return Line 342: logging.error("Job %r exception while killing virt-v2v" Line 343: "error: %r (pid: %d)", Line 337: try: Line 338: self._proc.kill() Line 339: except Exception as e: Line 340: if e.errno == errno.ESRCH: Line 341: return > If the process is not running we want to wait for it, and log its exit code Done Line 342: logging.error("Job %r exception while killing virt-v2v" Line 343: "error: %r (pid: %d)", Line 344: self._id, e, self._proc.pid) Line 345: Line 342: logging.error("Job %r exception while killing virt-v2v" Line 343: "error: %r (pid: %d)", Line 344: self._id, e, self._proc.pid) Line 345: Line 346: if not self._proc.wait(timeout=self.PROC_WAIT_TIMEOUT): > If the process failed to exit, we want to log a warning with the process pi Done Line 347: zombiereaper.autoReapPID(self._proc.pid) Line 348: Line 349: def _generate_disk_parameters(self): Line 350: parameters = [] Line 343: "error: %r (pid: %d)", Line 344: self._id, e, self._proc.pid) Line 345: Line 346: if not self._proc.wait(timeout=self.PROC_WAIT_TIMEOUT): Line 347: zombiereaper.autoReapPID(self._proc.pid) > I think we can simplify a little bit - if we killed the process, we don't h Done Line 348: Line 349: def _generate_disk_parameters(self): Line 350: parameters = [] Line 351: for disk in self._vm_properties['disks']: -- 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: 20 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: Sagi Shnaidman <sshna...@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
