Nir Soffer has posted comments on this change. Change subject: v2v: extract specific classes for libvirt and ova ......................................................................
Patch Set 4: (7 comments) https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py File vdsm/v2v.py: Line 159: Line 160: Line 161: def convert_external_vm(uri, username, password, vminfo, job_id, irs): Line 162: command = LibvirtCommand(uri, username, password, vminfo, job_id, irs) Line 163: job = ImportVm(command) job_id belongs to the job, not to the LibvirtCommand. Line 164: job.start() Line 165: _add_job(job_id, job) Line 166: return {'status': doneCode} Line 167: Line 167: Line 168: Line 169: def convert_ova(ova_path, vminfo, job_id, irs): Line 170: command = OvaCommand(ova_path, vminfo, job_id, irs) Line 171: job = ImportVm(command) job_id belongs to the job, not to the OvaCommand. Line 172: job.start() Line 173: _add_job(job_id, job) Line 174: return response.success() Line 175: Line 299: def proc(self): Line 300: return self._proc Line 301: Line 302: @contextmanager Line 303: def execute(self): You sould provide instead a context manager for preparing volumes, like the password_file context manager: def prepared_volumes(self): self._generate_disk_parameters() self._prepare_volumes() try: yield finally: self._teardown_volumes() Subclasses can combine this, for example, LIbvirtCommand.execute: def execute(self): with self._prepare_volumes(), self._password_file(): proc = execCmd(...) yield proc OvaCommand.execute needs only the volumes, so: def execute(self): with self._prepare_volumes(): proc = execCmd(...) yield proc If the execCmd(...) is common, it can also moved up: def execute(self): with self._prepare_volumes(): yield self._start_v2v_process() Line 304: self._generate_disk_parameters() Line 305: self._prepare_volumes() Line 306: try: Line 307: self._proc = execCmd(self._command(), sync=False, Line 309: nice=NICENESS.HIGH, Line 310: ioclass=IOCLASS.IDLE, Line 311: env=self.environments()) Line 312: Line 313: yield Why not: yield proc And you don't need the the proc() method now. Line 314: finally: Line 315: self._teardown_volumes() Line 316: Line 317: def _get_disk_format(self): Line 325: try: Line 326: self._disk_parameters.append('--vdsm-image-uuid') Line 327: self._disk_parameters.append(disk['imageID']) Line 328: self._disk_parameters.append('--vdsm-vol-uuid') Line 329: self._disk_parameters.append(disk['volumeID']) Why keep disk_parameters instead of returning them? This is more fragile, as calling this twice is now wrong, was ok before. Line 330: except KeyError as e: Line 331: raise InvalidInputError('Job %r missing required property: %s' Line 332: % (self._id, e)) Line 333: Line 448: cmd.extend(self._disk_parameters) Line 449: return cmd Line 450: Line 451: @contextmanager Line 452: def execute(self): > this just invoked the parent's execute(), thus feels redundant and could be I agree, but it wold be nicer if subclasses implement their execute(), and the parent only provides the helper methods to do this. Line 453: with super(self.__class__, self).execute(): Line 454: yield Line 455: Line 456: Line 536: logging.info('Job %r finished import successfully', Line 537: self._command.id) Line 538: Line 539: def _wait_for_process(self): Line 540: if self._command.proc.returncode is not None: This works, but I think this that it would be nicer if the Command classes generate a process object, and this class keeps this object, instead of going through the command object. The command object gives no services regarding its proc object, so it should not keep it. Line 541: return Line 542: logging.debug("Job %r waiting for virt-v2v process", self._command.id) Line 543: if not self._command.proc.wait(timeout=self.PROC_WAIT_TIMEOUT): Line 544: raise V2VProcessError("Job %r timeout waiting for process pid=%s", -- To view, visit https://gerrit.ovirt.org/49951 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
