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