Change in vdsm[master]: v2v: Import VM from OVA file

fromani at redhat.com fromani at redhat.com
Mon Aug 3 07:31:08 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: v2v: Import VM from OVA file
......................................................................


Patch Set 4:

(4 comments)

big question(s) inside, -1 for visibility.

https://gerrit.ovirt.org/#/c/43367/4/vdsm/rpc/vdsmapi-schema.json
File vdsm/rpc/vdsmapi-schema.json:

Line 3967:   'data': {'ova_path': 'str'},
Line 3968:   'returns': ['ExternalVmInfo']}
Line 3969: 
Line 3970: ##
Line 3971: # @Host.convertOva:
same comment as https://gerrit.ovirt.org/#/c/43271/4/vdsm/rpc/vdsmapi-schema.json

Maybe

  convertExternalVmFromOva

?
Line 3972: #
Line 3973: # Convert VM from external file (OVA) to data domain
Line 3974: #
Line 3975: # @ova_path:     actual path to the ova file


https://gerrit.ovirt.org/#/c/43367/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 171: def convert_ova(ova_path, vminfo, job_id, irs):
Line 172:     job = ImportVm.from_ova(ova_path, vminfo, job_id, irs)
Line 173:     job.start()
Line 174:     _add_job(job_id, job)
Line 175:     return {'status': doneCode}
what about using

 response.success()

?
Line 176: 
Line 177: 
Line 178: def get_ova_info(ova_path):
Line 179:     ns = {'ovf': _OVF_NS, 'rasd': _RASD_NS}


Line 293: @contextmanager
Line 294: def password_file(job_id, file_name, password):
Line 295:     if file_name is None:
Line 296:         yield
Line 297:         return
return is unneeded here.

I see why you did this, but it is a little ugly.

I think it is a little nicer if
- we extract what we run in _run() into another helper method:

            try:
                self._import()
            except Exception as ex:
                if self._aborted:
                    logging.debug("Job %r was aborted", self._id)
                else:
                    logging.exception("Job %r failed", self._id)
                    self._status = STATUS.FAILED
                    self._description = ex.message
                    try:
                        self._abort()
                    except Exception as e:
                        logging.exception('Job %r, error trying to abort: %r',
                                          self._id, e)
            finally:
                self._teardown_volumes() 

Let's call this

  _run_import_vm

or whatever name is more fitting.

We can also rename existing _run like

  _run_import_vm_with_passwd

which should now read like

    @traceback(msg="Error importing vm")
    def _run(self):
        with password_file(self._id, self._passwd_file, self._password):
            self._run_import_vm()

or, again, any other better fitting name.
Then, we can rebind _run like you did for _create_command

Indeed this is a little clumsier that I'd like, but seems also clearer and conforming to the other refactoring you did.

What do you think?
Line 298:     fd = os.open(file_name, os.O_WRONLY | os.O_CREAT, 0o600)
Line 299:     try:
Line 300:         os.write(fd, password.value)
Line 301:     finally:


Line 479:                     get_storage_domain_path(self._prepared_volumes[0]['path']),
Line 480:                     self._vminfo['vmName']])
Line 481:         return cmd
Line 482: 
Line 483:     def _from_ova_command(self):
maybe?

  _create_command_from_ova?

anyway, should be similar to whatever you choose for the libvirt counterpart
Line 484:         cmd = [_VIRT_V2V.cmd,
Line 485:                '-i', 'ova', self._ova_path,
Line 486:                '-o', 'vdsm',
Line 487:                '-of', self._get_disk_format(),


-- 
To view, visit https://gerrit.ovirt.org/43367
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0e440748ecc503f4d61e8f4f61bb0c7387589354
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list