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

shavivi at redhat.com shavivi at redhat.com
Mon Aug 3 09:11:26 UTC 2015


Shahar Havivi has posted comments on this change.

Change subject: v2v: get VM information from OVA file
......................................................................


Patch Set 4:

(9 comments)

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

Line 3953: 
Line 3954: ##
Line 3955: # @Host.getOvaInfo:
Line 3956: #
Line 3957: # Get VM information (disks, interfaces, memory etc) from OVA file
> We should spend a bit of time in choosing good names for public API, since 
getExternalVMsFromOva sounds good
Line 3958: #
Line 3959: # @ova_path: path to the ova file
Line 3960: #
Line 3961: # Returns:


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

Line 180:     _add_general_ovf_info(vm, root, ns)
Line 181:     _add_disks_ovf_info(vm, root, ns)
Line 182:     _add_networks_ovf_info(vm, root, ns)
Line 183: 
Line 184:     return {'status': doneCode, 'vmList': [vm]}
> please consider using the response module:
sure, you right!
Line 185: 
Line 186: 
Line 187: def get_converted_vm(job_id):
Line 188:     try:


Line 680:         params['networks'].append(i)
Line 681: 
Line 682: 
Line 683: def _read_ovf_from_ova(ova_path):
Line 684:     cmd = ['/usr/bin/tar', 'xf', ova_path, '*.ovf', '--to-stdout']
> question: can we make use of the tarfile package?
There is no way to use the --to-stdout in tarfile package,
I will add a comment in the code
Line 685:     rc, output, error = execCmd(cmd)
Line 686:     if rc:
Line 687:         raise V2VError(error)
Line 688: 


Line 688: 
Line 689:     return ''.join(output)
Line 690: 
Line 691: 
Line 692: def _add_general_ovf_info(vm, node, ns):
> this function seems to be easily unit-testable. Do we have tests? if not, c
Done
Line 693:     try:
Line 694:         vm['status'] = 'Down'
Line 695:         vm['vmName'] = node.find('./ovf:VirtualSystem/ovf:Name', ns).text
Line 696:         vm['memSize'] = int(node.find('.//ovf:Item[rasd:ResourceType="4"]/'


Line 692: def _add_general_ovf_info(vm, node, ns):
Line 693:     try:
Line 694:         vm['status'] = 'Down'
Line 695:         vm['vmName'] = node.find('./ovf:VirtualSystem/ovf:Name', ns).text
Line 696:         vm['memSize'] = int(node.find('.//ovf:Item[rasd:ResourceType="4"]/'
> a link to the doc - here or up when you define the _RASD_NS constant would 
Done
Line 697:                             'rasd:VirtualQuantity', ns).text)
Line 698:         vm['smp'] = int(node.find('.//ovf:Item[rasd:ResourceType="3"]/'
Line 699:                         'rasd:VirtualQuantity', ns).text)
Line 700:     except Exception as e:


Line 696:         vm['memSize'] = int(node.find('.//ovf:Item[rasd:ResourceType="4"]/'
Line 697:                             'rasd:VirtualQuantity', ns).text)
Line 698:         vm['smp'] = int(node.find('.//ovf:Item[rasd:ResourceType="3"]/'
Line 699:                         'rasd:VirtualQuantity', ns).text)
Line 700:     except Exception as e:
> can we catch more specific exception?
yes, I will fix that
Line 701:         raise V2VError('Error parsing ovf information: %s' % e.message)
Line 702: 
Line 703: 
Line 704: def _add_disks_ovf_info(vm, node, ns):


Line 697:                             'rasd:VirtualQuantity', ns).text)
Line 698:         vm['smp'] = int(node.find('.//ovf:Item[rasd:ResourceType="3"]/'
Line 699:                         'rasd:VirtualQuantity', ns).text)
Line 700:     except Exception as e:
Line 701:         raise V2VError('Error parsing ovf information: %s' % e.message)
> what's the benefit of having this Exception translation? I see no recovery 
The reason is to know where the exception cam from (ie V2VError from v2v)
what do you suggest, not handling the error (ie it will be raised to the log?
Line 702: 
Line 703: 
Line 704: def _add_disks_ovf_info(vm, node, ns):
Line 705:     try:


Line 713:             alias = node.find('.//ovf:References/ovf:File[@ovf:id="%s"]' %
Line 714:                               fileref, ns)
Line 715:             disk['alias'] = alias.attrib.get('{%s}href' % _OVF_NS)
Line 716:             vm['disks'].append(disk)
Line 717:     except Exception as e:
> same as line 700
Done
Line 718:         raise V2VError('Error parsing ovf disk information: %s' % e.message)
Line 719: 
Line 720: 
Line 721: def _add_networks_ovf_info(vm, node, ns):


Line 731:                 net['type'] = 'bridge'
Line 732:             else:
Line 733:                 net['type'] = 'interface'
Line 734:             vm['networks'].append(net)
Line 735:     except Exception as e:
> same as line 700
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c55c846f837bb5bf363717e05daabf5ee6631ca
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