Code review comment for lp:~chiradeep/nova/msft-hyper-v-support

Thanks Jay. I'll put together something along the lines of the XenAPI driver.
The WMI calls are well documented on MSDN. Also, the Python WMI library generates the docstring for any WMI function that lists the parameters and return tuple.
Not sure that I want to regurgitate *all* those details in this doc, but I'll see if I can highlight some of the exemplary calls.

Some more comments below, preceded by >>>>
________________________________________
From: <email address hidden> [<email address hidden>] On Behalf Of Jay Pipes [<email address hidden>]
Sent: Monday, October 11, 2010 10:11 AM
To: <email address hidden>
Subject: Re: [Merge] lp:~chiradeep/nova/msft-hyper-v-support into lp:nova

Review: Needs Fixing
Hi Chiradeep!

Thanks for the contribution and a start of Windows support in OpenStack! :)

Here are my review notes for you.

1) Documentation is sparse :)

It would be excellent to get some overall design and module documentation for your work. :) This will help those coders in the future who are maintaining and enhancing the hyper-V work. Please see Ewan Mellor's excellent documentation in the xenapi virtualization layer for an example of the kind of documentation that is helpful for maintainers.

This documentation is, IMHO, pretty critical considering the sorry state of python-wmi documentation regarding virtual environments.

Additional code comments would be very useful in places such as the following:

181 + (job, ret_val) = vs_man_svc.DefineVirtualSystem(
182 + [], None, vs_gs_data.GetText_(1))[1:]

What does vs_gs_data.GetText(1) return? What does sv_man_svc.DefineVirtualSystem() return? Without understanding these types of things, maintaining developers have zero chance of successfully fixing bugs that may occur :)

>>>> This is simply how WMI works. When you want to pass in object references, you generate an XML representation using GetText. I think that whoever maintains this
needs a bookmark to the MSDN libraries and the PowerShell libraries. Equally someone maintaining the XenApi driver or libvirt driver needs to *get* the object model and the conventions.

2) Please use Nova's exceptions, which are tested for in the test suites and programs, instead of generic Exception:

402 + raise Exception('instance not present %s' % instance_id)

Should be replaced with:

from nova import exception
...
raise exception.NotFound('instance not found %s' % instance_id)

same for here:

360 + if vm is None:
361 + raise Exception('instance not present %s' % instance.name)

>>>> sure.

3) Your rewrite of the _fetch_s3_image() function is good, however, it removes the efficiency of using a shared process pool in Linux/BSD environments in favour of cross-platform compatibility. It would be nice to keep the existing code and use your rewrite in an if sys.platform.startswith('win'): block.

>>>> sure

============

And here are some style-related things to fix up...

1) Please make sure imports are ordered according to the instructions in the HACKING file. In particular, please make sure system lib imports are first, in alphabetical order, followed by a newline, then imports of 3rd-party libraries (like wmi) in alphabetical order, followed by a newline and imports of nova modules.

2) Something is amiss here:

118 +HYPERV_POWER_STATE = {
119 + 3 : power_state.SHUTDOWN,
120 + 2 : power_state.RUNNING,
121 + 32768 : power_state.PAUSED,
122 + 32768: power_state.PAUSED, # TODO
123 + 3 : power_state.CRASHED
124 +}

>>> sure. There isn't a CRASHED state in Hyper-V, but I guess the mapping doesn't have to be complete in the openstack->driver direction, just the other way.

3) Define constants for "magic numbers"

183 + if (ret_val == 4096 ): #WMI job started

Feel free to either create a constant WMI_JOB_STARTED and use that instead of the number 4096 and putting a comment at the end of the line. Self-documenting constants++ :)

Same goes for these locations:

323 + while job.JobState == 4: #job started
327 + if (job.JobState != 7): #job success
385 + if (ret_val == 4096 ): #WMI job started
442 + if (return_val == 4096 ): #WMI job started

>>>> no prob
4) This looks hacky :)

212 + vcpus = long(str(instance['vcpus']))

I recommend just:

vcpus = long(instance['vcpus'])

>>>> sure

5) Please remove extraneous commented-out debugging stuff:

213 + #vcpus = 1

6) Python != C ;)

Please remove outer extraneous parentheses:

286 + if (ret_val != 0):
307 + if (return_val == 4096 ): #WMI job started
320 + if (len(jobs) == 0):
327 + if (job.JobState != 7): #job success
340 + if (len(bound) == 0):
385 + if (ret_val == 4096 ): #WMI job started
387 + elif (ret_val == 0):
442 + if (return_val == 4096 ): #WMI job started
444 + elif (return_val == 0):

7) One newline between methods, 2 newlines between module-level definitions. There are 2 or 3 newlines between method names in a lot of the code.

>>>> sure
--
https://code.launchpad.net/~chiradeep/nova/msft-hyper-v-support/+merge/38036
You are the owner of lp:~chiradeep/nova/msft-hyper-v-support.