Code review comment for lp:~citrix-openstack/nova/vmware-vsphere-support

Hi Sateesh!

Phew, this is a big patch! I commend you for being able to go through the VMWare SDK and SOAP API docs...

Some suggestions and style nits :)

47 +Currently supports Nova's flat networking model (Flat Manager).

You note in the description that you are supporting VLAN and FLAT. The above line is from the documentation. May want to correct the docs there.

601 + NetworkHelper.get_vlanid_and_vswicth_for_portgroup(session, bridge)
2139 + def get_vlanid_and_vswicth_for_portgroup(cls, session, pg_name):

Misspelling on the above lines... looks like some Autocomplete fail.

691 + """ Create and spawn the VM """

There are a number of docstrings which incorrectly have a space after and before the opening and closing """ marks. Please correct those. Should be like so:

"""Create and spawn the VM"""

For multi-line docstrings, we use the pep0257 recommendations, so:

"""Create and spawn the VM

This is a more descriptive description...
"""

or:

"""
Create and spawn the VM

This is a more descriptive description...
"""

or even:

"""
Create and spawn the VM

This is a more descriptive description...

"""

All of which will be identical once newline-trimmed, as most all doc-generation does.

See http://www.python.org/dev/peps/pep-0257/ for more details.

Some comments are like this:

03 + #Check if the vsiwtch associated is proper

and some are like this:

713 + # Get Nova record for VM

Please use the latter, with a space after the #.

857 === added file 'nova/tests/vmwareapi/db_fakes.py'

There are already a number of fakes in the test suite(s). Was it necessary to create another one just for the vmwareapi? Can we use one of the existing ones?

1101 +class VimException(Exception):
1102 + """The VIM Exception class"""

The acronym VIM is a bit confusing, and it is used in a number of files in this patch. Most everyone associates VIM with the ViM editor (just Google what is (a) VIM). What does VIM stand for here?

1886 === added file 'nova/virt/vmwareapi/io_util.py'

This file smells a bit like Not Invented Here syndrome... I think it would be best to use an existing library for this type of functionality. Try checking out Eventlet's greenthread-aware Queue class: http://eventlet.net/doc/modules/queue.html.

2279 +class GlanceHTTPWriteFile(ImageServiceFile):
2280 + """Glance file write handler class"""
2281 +
2282 + def __init__(self, host, port, image_id, file_size, os_type, adapter_type,
2283 + version=1, scheme="http"):
2284 + base_url = "%s://%s:%s/images/%s" % (scheme, host, port, image_id)
2285 + (scheme, netloc, path, params, query, fragment) = \
2286 + urlparse.urlparse(base_url)
2287 + if scheme == "http":
2288 + conn = httplib.HTTPConnection(netloc)
2289 + elif scheme == "https":
2290 + conn = httplib.HTTPSConnection(netloc)
2291 + conn.putrequest("PUT", path)
2292 + conn.putheader("User-Agent", USER_AGENT)
2293 + conn.putheader("Content-Length", file_size)
2294 + conn.putheader("Content-Type", "application/octet-stream")
2295 + conn.putheader("x-image-meta-store", "file")
2296 + conn.putheader("x-image-meta-is_public", "True")
2297 + conn.putheader("x-image-meta-type", "raw")
2298 + conn.putheader("x-image-meta-size", file_size)
2299 + conn.putheader("x-image-meta-property-kernel_id", "")
2300 + conn.putheader("x-image-meta-property-ramdisk_id", "")
2301 + conn.putheader("x-image-meta-property-vmware_ostype", os_type)
2302 + conn.putheader("x-image-meta-property-vmware_adaptertype",
2303 + adapter_type)
2304 + conn.putheader("x-image-meta-property-vmware_image_version", version)
2305 + conn.endheaders()
2306 + ImageServiceFile.__init__(self, conn)
2307 +
2308 + def write(self, data):
2309 + """Write data to the file"""
2310 + self.file_handle.send(data)

Please don't do this. Use glance.client instead, which already handles chunked-encoding HTTP transfers for you. The code above (and associated ImageServiceFile in the same read_write_util.py has a number of errors regarding chunked transfer that have already been solved in glance.client. Please let me know if you need assistance using the glance.client.Client class (and more comments about this below...)

2655 +def build_selcetion_spec(client_factory, name):
2676 + visit_folders_select_spec = build_selcetion_spec(client_factory,
2699 + rp_to_rp_select_spec = build_selcetion_spec(client_factory, "rp_to_rp")
2700 + rp_to_vm_select_spec = build_selcetion_spec(client_factory, "rp_to_vm")

Misspelled method name and repeated. Autocomplete fail?

2830 +def get_properites_for_a_collection_of_objects(vim, type,
2090 + "get_properites_for_a_collection_of_objects",

Another method mispelling and repeat.

3372 + LOG.debug(_("Creating Virtual Disk of size "
3373 + "%(vmdk_file_size_in_kb)s KB and adapter type "
3374 + "%(adapter_type)s on the ESX host local store"
3375 + " %(data_store_name)s") %
3376 + {"vmdk_file_size_in_kb": vmdk_file_size_in_kb,
3377 + "adapter_type": adapter_type,
3378 + "data_store_name": data_store_name})

You do this in a few locations. Here's a little Python trick to make things like the above a bit more readable... when you have a format string with %(varname)s replacements, and all of the variables in the format string are in the local scope, you can just do this:

LOG.debug(_("Creating Virtual Disk of size "
            "%(vmdk_file_size_in_kb)s KB and adapter type "
            "%(adapter_type)s on the ESX host local store"
            " %(data_store_name)s") % locals()))

3845 + def get_ajax_console(self, instance):
3846 + """ Return link to instance's ajax console """
3847 + return 'http://fakeajaxconsole/fake_url'

The above is in the NON-fake vmops.py :)

4030 +def _get_glance_image(image, instance, **kwargs):
4031 + """ Download image from the glance image server. """
4032 + LOG.debug(_("Downloading image %s from glance image server") % image)
4033 + read_file_handle = read_write_util.GlanceHTTPReadFile(FLAGS.glance_host,
4034 + FLAGS.glance_port,
4035 + image)
4036 + file_size = read_file_handle.get_size()
4037 + write_file_handle = read_write_util.VMWareHTTPWriteFile(
4038 + kwargs.get("host"),
4039 + kwargs.get("data_center_name"),
4040 + kwargs.get("datastore_name"),
4041 + kwargs.get("cookies"),
4042 + kwargs.get("file_path"),
4043 + file_size)
4044 + start_transfer(read_file_handle, write_file_handle, file_size)
4045 + LOG.debug(_("Downloaded image %s from glance image server") % image)

If you use glance.client instead of the home-grown GlanceHTTPReadFile, you could do this:

c = glance.client.Client(host, port)
read_file_handle, image_metadata = c.get_image(image_id)

Glance's Client returns a file object that reads the image data in chunks, just like a StringIO.StringIO would. So, IMHO, there's no reason for you to have the GlanceHTTPReadFile object or the ImageServiceFile object. Likewise, the GlanceHTTPWriteFile can be gotten rid of. Just do this:

c = glance.client.Client(host, port)
c.add_image(image_meta, image_data)

where image_meta is something like this:

{'name': <NAME>,
 'is_public': True,
 'disk_format': 'raw',
 'container_format': 'bare',
 'size': <SIZE>,
 'properties': {'kernel_id': "", 'ramdisk_id': "",
                'vmware_adaptertype': <ADAPTER_TYPE>,
                'vmware_os_type', <OS_TYPE>,
                'vmware_image_version': <VERSION>}}

and image_data is a file-like object. Also, do NOT do this:

2295 + conn.putheader("x-image-meta-store", "file")

The reason is because by doing that, you override Glance's default backing store, which is probably not what you wanted to do :) For instance, if Glance is setup to use Swift as its backing store, you just overrode that setting, which should only be done in very specific scenarios, and I don't think this is one of them...

In addition, you had this in GlanceHTTPWriteFile:

2297 + conn.putheader("x-image-meta-type", "raw")

The type is no longer a base attribute in Glance. Use disk-format and/or container-format, like so:

{'disk_format': 'raw', 'container_format': 'bare'}

You can read more about formats here: http://glance.openstack.org/formats.html and the Glance client here: http://glance.openstack.org/client.html

As a final note, I'd like to get Vishy to review the code in the Guest Tool on networking...

Cheers!
jay