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