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

fromani at redhat.com fromani at redhat.com
Mon Aug 3 07:19:58 UTC 2015


Francesco Romani has posted comments on this change.

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


Patch Set 4: Code-Review-1

(10 comments)

mostly questions, but quite some of them. -1 for visibility.

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 we need to support them for a long time.

Here I don't really like the 'getOvaInfo' name.

We should pick a more intention-revealing name, and it should also remind us that this verb is related to getExternalVMs.

Maybe

  getExternalVMsFromOva

?

Just a suggestion to kickstart more thought process
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 175:         root = ET.fromstring(_read_ovf_from_ova(ova_path))
Line 176:     except ET.ParseError as e:
Line 177:         raise V2VError('Error reading ovf from ova, position: %r' % e.position)
Line 178: 
Line 179:     vm = {}
Should this contain exactly one VM?
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 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:

  return response.success(vmList=[vm])
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?

https://docs.python.org/2/library/tarfile.html

If not, can you please add a quick comment reminding the future us why we did like that?
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, can you please add them? (same for other _add*). It's OK to add them in a followup patch.
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 be nice
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?
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 code in place, so it is really matters which group (general, disks, networks) failed, granted there is enough context in the generic exception?
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
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


-- 
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: 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