Hi Jay, Thanks a lot for your review comments. Yeah, this patch might have taken a lot of time from you! I have incorporated the changes as follows, * Updated document vmware_readme.rst to mention VLAN networking * Corrected docstrings as per pep0257 recommentations. * Stream-lined the comments. (Now all comments starts with "# " * Updated code with locals() where ever applicable. * About VIM in the source code/files: 'VIM' stands for VMware Virtual Infrastructure Methodology. We have used the terminology from VMware. Now added a question in FAQ inside vmware_readme.rst in doc/source. A reference doc can be seen at https://www.vmware.com/pdf/vim_datasheet.pdf * About new fake db: The vmwareapi fake module uses a different set of fields and hence the structures required are different. Ex: bridge : 'xenbr0' does not hold good for VMware environment and bridge : 'vmnic0' is used instead. Also return values varies, hence went for implementing separate fake db. * Now using eventlet library instead and removed io_utils.py from branch. * Now using glance.client.Client instead of homegrown code to talk to Glance server to handle images. * Corrected all mis-spelled function names and corresponding calls. Yeah, an auto-complete side-effect! Please have a look at the branch and let me know your views. Thanks for your time. Regards, Sateesh > 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': , > 'is_public': True, > 'disk_format': 'raw', > 'container_format': 'bare', > 'size': , > 'properties': {'kernel_id': "", 'ramdisk_id': "", > 'vmware_adaptertype': , > 'vmware_os_type', , > 'vmware_image_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