Joey Boggs wrote:
>> These patches provide the virt-unpack command which converts vmware
>> format machines into virt-image xml format hvm/paravirt runnable
>> instances. Next revision will contain libvirt format as well. As also
>> with the virt-pack command all required vmware/xen/hvm kernel modules
>> must be in place prior to conversion to boot machine.
>> ------------------------------------------------------------------------
>>
Comments inline:
> diff -Naur virtinst--devel.orig/virt-unpack virtinst--devel/virt-unpack
> --- virtinst--devel.orig/virt-unpack 1969-12-31 19:00:00.000000000 -0500
> +++ virtinst--devel/virt-unpack 2008-06-26 10:30:45.000000000 -0400
> @@ -0,0 +1,231 @@
> +#!/usr/bin/python
> +#
> +# Convert a VMware(tm) virtual machine into an XML image description
> +#
> +# Copyright 2008 Red Hat, Inc.
> +# Joey Boggs <jboggs@xxxxxxxxxx>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA 02110-1301 USA.
> +
> +import sys
> +from string import ascii_letters
> +import virtinst.cli as cli
> +import os
> +import random
> +import pdb
> +import shutil
> +import logging
> +from optparse import OptionParser, OptionValueError
> +
> +def parse_args():
> + parser = OptionParser()
> + parser.set_usage("%prog [options] image.vmx")
> + parser.add_option("-a", "--arch", type="string", dest="arch",
> + help=("Machine Architecture Type (i686/x86_64/ppc)"))
> + parser.add_option("-t", "--type", type="string", dest="type",
> + help=("Output virtualization type (hvm, paravirt"))
For type I would use the convention that virt-install uses: add seperate
options for --paravirt and --hvm (optionally -p or -v). Also I'd recommend
just defaulting to --hvm since that is the most common case at this point.
> + parser.add_option("-d", "--debug", action="store_true", dest="debug",
> + help=("Print debugging information"))
> + parser.add_option("-o", "--outputdir", type="string", dest="outputdir",
> + help=("Output directory in which files will be created"))
> + (options,args) = parser.parse_args()
> +
> + if len(args) < 1:
> + parser.error(("You need to provide an image XML descriptor"))
> + options.file = args[0]
> +
> + if (options.arch is None) or (options.type is None):
> + parser.error(("You need to provide all input options"
> + "\n\nArchitecture: " + str(options.arch) +
> + "\nOutput Virt Type: " + str(options.type)))
> + sys.exit(1)
Won't need sys.exit here since parser.error will handle this for you.
> + return options
> +
> +# Begin creation of xml template from parsed vmx config file
> +def create_xml_template(disks_list,record,options,dirname):
Kind of a nit, but create_xml_template isn't really descriptive.
convert_to_image? vmx_to_image?
> + pv_disk_list = []
> + fv_disk_list = []
> + storage_disk_list = []
> +
> + name = record["displayName"]
> + memory = record["memsize"]
> + memory = int(memory) * 1024
> +
> + # can't guarantee annotation or vcpus are set in vmware config, requires evaluation
> + if record.has_key("annotation"):
> + annotation = record["annotation"]
> + else:
> + annotation = ""
> +
> + if record.has_key("numvcpus"):
> + vcpus = record["numvcpus"]
> + else:
> + vcpus = "1"
> +
> +# create disk filename lists for xml template
> + for (number, file) in enumerate(disks_list):
> + file = str(file.replace(".vmdk","")).strip()
> + pv_disk_list.append("""<drive disk="%s.img" target="xvd%s"/>""" % \
> + (file, ascii_letters[number % 26]))
> + fv_disk_list.append("""<drive disk="%s.img" target="hd%s"/>""" % \
> + (file, ascii_letters[number % 26]))
> + storage_disk_list.append("""<disk file="%s.img" use="system" format="raw"/>""" % (file))
> +# determine virtualization type for image.boot section
> + if options.type == "paravirt":
> + virt_boot_template = """<boot type="xen">
> + <guest>
> + <arch>%(vm_arch)s</arch>
> + <features>
> + <pae/>
> + </features>
> + </guest>
> + <os>
> + <loader>pygrub</loader>
> + </os>
> + %(vm_pv_disks)s
> + </boot>"""
> + elif options.type == "fullvirt":
> +
> + virt_boot_template = """<boot type="hvm">
> + <guest>
> + <arch>%(vm_arch)s</arch>
> + </guest>
> + <os>
> + <loader dev="hd"/>
> + </os>
> + %(vm_fv_disks)s
> + </boot>"""
> + else:
> + print "Invalid virtualization type specified"
> + sys.exit(1)
If you use the above --paravirt/--hvm option change, you won't need
this check.
> +
> +
> +# xml replacements dictionaries
> + virt_boot_xml_dict = {
> + "vm_pv_disks" : "".join(pv_disk_list),
> + "vm_fv_disks" : "".join(fv_disk_list),
> + "vm_arch" : options.arch,
> + }
If you do the above --paravirt/--hvm change, you can then just pass
options.arch directly to the function rather than the whole options
list.
> + virt_boot_template = virt_boot_template % virt_boot_xml_dict
> +
> + virt_image_xml_dict = {
> + "virt_boot_template" : virt_boot_template,
> + "vm_displayName": name.replace(" ","_"),
> + "vm_annotation" : annotation,
> + "vm_vcpus" : vcpus,
> + "vm_mem" : memory,
> + "vm_storage" : "".join(storage_disk_list),
> + }
> +
> + virt_image_xml_template = """<image>
> + <name>%(vm_displayName)s</name>
> + <label>%(vm_displayName)s</label>
> + <description>
> + %(vm_annotation)s
> + </description>
> + <domain>
> + %(virt_boot_template)s
> + <devices>
> + <vcpu>%(vm_vcpus)s</vcpu>
> + <memory>%(vm_mem)s</memory>
> + <interface/>
> + <graphics/>
> + </devices>
> + </domain>
> + <storage>
> + %(vm_storage)s
> + </storage>
> +</image>"""
> +
> + virt_image_xml_template = virt_image_xml_template % virt_image_xml_dict
> + virt_image_xml_config_file = open(dirname + "/" + name + ".virtimage.xml","w")
> + virt_image_xml_config_file.writelines(virt_image_xml_template)
> + virt_image_xml_config_file.close()
I'd recommend moving this end piece to the main function. If keeps the
conversion function doing only the conversion. If we ever wanted to reuse
this code in another app which won't be writing an output file, this would
be a necessary change.
> + return
> +
> +# parse input vmware configuration
> +def parse_vmware_config(options):
> + if not os.access(options.file,os.R_OK):
> + raise ValueError, "Could not read file: %s" % options.file
> + input = open(options.file,"r")
> + contents = input.readlines()
> + input.close()
> + record = {}
> + vm_config = []
> + disks_list = []
> +
> +# strip out comment and blank lines for easy splitting of values
> + for line in contents:
> + if not line.strip() or line.startswith("#"):
> + continue
> + else:
> + vm_config.append(line)
> +
> + for line in vm_config:
> + beforeEq, afterEq = line.split("=", 1)
> + key = beforeEq.replace(" ","")
> + value = afterEq.replace('"',"")
> + record[key] = value.strip()
> + logging.debug("Key: %s Value: %s" % (key,value))
> + if value.endswith("vmdk\n"): # separate disks from config
> + disks_list.append(value)
> + return vm_config,record,disks_list,options
Shouldn't need to return vm_config or options here: options hasn't changed,
vm_config isn't used anywhere else.
> +
> +
> +def convert_disks(disks_list,name,dirname):
Name isn't used here anymore.
> + for disk in disks_list:
> + file = disk.replace(".vmdk","").strip()
> + convert_cmd="qemu-img convert %s -O raw %s/%s.img" % (disk.strip(),dirname,file)
> + logging.debug("Converting %s" % disk.strip())
> + print "\nConverting %s to %s/%s.img" % (disk.strip(),dirname,file)
> + os.system(convert_cmd)
> +
> +
> +
> +def main():
> + options = parse_args()
> + cli.setupLogging("virt-unpack", options.debug)
> + vm_config = parse_vmware_config(options)
> + config, record, disks_list, options = vm_config
> + name = record["displayName"].replace(" ","-")
Just thinking, there are a lot of checks here directly into the record. If
we are passed a vmx file that is missing one of these options, either
through a bad config or some change in the format, we are going to throw
an undescriptive error like "Key error: displayName". Maybe create a small
wrapper function like record_lookup that you pass the record and the key
to, and will throw a more clear error to the user like "key 'blah' was
not parsed from the vmx config"
> + dirname = options.outputdir
> + if not dirname:
> + dirname = name
> + try:
> + logging.debug ("Creating directory %s" % dirname)
> + os.mkdir(dirname)
> + except OSError,e:
> + if e.errno == 17:
> + logging.error("Could not create directory %s or directory already exists", dirname)
> + sys.exit(1)
You'll want to exit here regardless of the errno. I'd recommend just losing
the errno check, and use something general like:
logging.error("Could not create directory '%s': %s" % (dirname, str(e))
sys.exit(1)
Also, if you move the file creation aspect out of the create_xml_template,
you can move the dir creation after the create_xml_template call. That
way we move the dir creation as late as possible so that if there is
an exception earlier in the process we don't need to clean up/delete
the unpopulated directory.
> +
> + create_xml_template(disks_list,record,options,dirname)
> + convert_disks(disks_list,name,dirname)
> +
> + print "\n\nConversion completed and placed in: %s" % dirname
> +
> +
> +if __name__ == "__main__":
> + try:
> + main()
> + except SystemExit, e:
> + sys.exit(e.code)
> + except KeyboardInterrupt, e:
> + print >> sys.stderr, _("Aborted at user request")
> + except Exception, e:
> + logging.exception(e)
> + sys.exit(1)
> +
Thanks,
Cole
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools
