Merge lp:~chiradeep/nova/msft-hyper-v-support into lp:~hudson-openstack/nova/trunk

[Chiradeep Vittal]

Introduces basic support for spawning, rebooting and destroying vms when using Microsoft Hyper-V as the hypervisor.
Images need to be in VHD format. Note that although Hyper-V doesn't accept kernel and ramdisk
separate from the image, the nova objectstore api still expects an image to have an associated aki and ari. You can use dummy aki and ari images -- the hyper-v driver won't use them or try to download them.
Requires Python's WMI module.

[Jay Pipes (jaypipes)]

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 :)

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)

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.

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

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 +}

The indexes 3 and 32768 are doubled up.

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

4) This looks hacky :)

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

I recommend just:

vcpus = long(instance['vcpus'])

5) Please remove extraneous commented-out debugging stuff:

213 + #vcpus = 1

6) Python != C ;)

Please remove outer extraneous parentheses:

286 + if (ret_va...

review: Needs Fixing

[Jay Pipes (jaypipes)]

Also, I forgot to mention that we'd really need some Hyper-V specific test cases when Monty and others get around to getting our testing hardware and platform up-to-speed with Windows stuff...

[Chiradeep Vittal (chiradeep)]

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 ...

[Jay Pipes (jaypipes)]

> 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.

Well, just do your best to provide a general overview of how the WMI calls are made, what's expected back from them, etc. :)

> 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.

I will hold off judgment about the WMI and PowerShell APIs...

If you could just put the above note in your general documentation, and even provide a couple of those links for MSDN/PowerShell docs in there, that would be great! :)

Cheers!
jay

[Soren Hansen (soren)]

On 13-10-2010 09:15, Chiradeep Vittal wrote:
> Chiradeep Vittal has proposed merging lp:~chiradeep/nova/msft-hyper-v-support into lp:nova.
>
> Requested reviews:
> Jay Pipes (jaypipes)
>
>
> Introduces basic support for spawning, rebooting and destroying vms when using Microsoft Hyper-V as the hypervisor.
> Images need to be in VHD format. Note that although Hyper-V doesn't accept kernel and ramdisk
> separate from the image, the nova objectstore api still expects an image to have an associated aki and ari. You can use dummy aki and ari images -- the hyper-v driver won't use them or try to download them.
> Requires Python's WMI module.
>

> === modified file 'nova/compute/manager.py'
> --- nova/compute/manager.py 2010-10-12 20:18:29 +0000
> +++ nova/compute/manager.py 2010-10-13 07:15:24 +0000
> @@ -39,6 +39,8 @@
> 'where instances are stored on disk')
> flags.DEFINE_string('compute_driver', 'nova.virt.connection.get_connection',
> 'Driver to use for volume creation')
> +flags.DEFINE_string('images_path', utils.abspath('../images'),
> + 'path to decrypted local images if not using s3')

Each flag must only be defined once. images_path is also defined in
nova.objectstore.images. Either import that or move the flag to a common
place and import that.

> === modified file 'nova/virt/connection.py'
> --- nova/virt/connection.py 2010-08-30 13:19:14 +0000
> +++ nova/virt/connection.py 2010-10-13 07:15:24 +0000
> @@ -26,6 +26,7 @@
> from nova.virt import fake
> from nova.virt import libvirt_conn
> from nova.virt import xenapi
> +from nova.virt import hyperv
>
>
> FLAGS = flags.FLAGS
> @@ -49,6 +50,8 @@
> conn = libvirt_conn.get_connection(read_only)
> elif t == 'xenapi':
> conn = xenapi.get_connection(read_only)
> + elif t == 'hyperv':
> + conn = hyperv.get_connection(read_only)
> else:
> raise Exception('Unknown connection type "%s"' % t)
>
>

This is troublesome. We have no python-wmi on linux, and unconditionally
importing the hyperv driver means unconditionally importing wmi which
fails spectacularly.

nova.virt.connection.get_connection should probably be refactored to use
nova.utils.import_object or similar, based on the defined
connection_type. That would solve this problem.

> === added file 'nova/virt/hyperv.py'
> --- nova/virt/hyperv.py 1970-01-01 00:00:00 +0000
> +++ nova/virt/hyperv.py 2010-10-13 07:15:24 +0000
> @@ -0,0 +1,453 @@
> +# vim: tabstop=4 shiftwidth=4 softtabstop=4
> +
> +# Copyright (c) 2010 Cloud.com, Inc
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License"); you may
> +# not use this file except in compliance with the License. You may obtain
> +# a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> +# License for the specific language governing permissions and limitations
> +# under the L...

review: Needs Fixing

[Jay Pipes (jaypipes)]

Hi again, Chiradeep :) Thanks much for addressing the documentation concerns and the style issues on the first review! I second Soren's suggestions/fixups.

[Chiradeep Vittal (chiradeep)]

For the "import wmi" issue, I propose adopting the solution used in the xenapi
driver (use __import__).
I can raise a bug to get this fixed in the manner suggested by Soren.

Regarding long-running blocking operations (fetch image / start vm), I am not yet
familiar with the Twisted framework. I propose that we leave these as is and raise
bugs to fix later.

New exceptions: did you mean add one into nova/exception.py or add a new one
inside hyperv.py. I've added a couple in the latter.

Unit tests: I've added a unit test (hyperv_unittest.py). The unit test framework, I think
assumes that the controller and the driver are on the same host. I haven't managed
to port the entire controller to Windows (only the compute manager). There is a
dependency on where redis runs), so I haven't integrated it into the test case framework.
I am also not sure on how to make it conditional (on Linux) within the test case framework.

________________________________________
From: <email address hidden> [<email address hidden>] On Behalf Of Soren Hansen [<email address hidden>]
Sent: Wednesday, October 13, 2010 3:52 AM
To: <email address hidden>
Subject: Re: [Merge] lp:~chiradeep/nova/msft-hyper-v-support into lp:nova

Review: Needs Fixing
On 13-10-2010 09:15, Chiradeep Vittal wrote:
> Chiradeep Vittal has proposed merging lp:~chiradeep/nova/msft-hyper-v-support into lp:nova.
>
> Requested reviews:
> Jay Pipes (jaypipes)
>
>
> Introduces basic support for spawning, rebooting and destroying vms when using Microsoft Hyper-V as the hypervisor.
> Images need to be in VHD format. Note that although Hyper-V doesn't accept kernel and ramdisk
> separate from the image, the nova objectstore api still expects an image to have an associated aki and ari. You can use dummy aki and ari images -- the hyper-v driver won't use them or try to download them.
> Requires Python's WMI module.
>

> === modified file 'nova/compute/manager.py'
> --- nova/compute/manager.py 2010-10-12 20:18:29 +0000
> +++ nova/compute/manager.py 2010-10-13 07:15:24 +0000
> @@ -39,6 +39,8 @@
> 'where instances are stored on disk')
> flags.DEFINE_string('compute_driver', 'nova.virt.connection.get_connection',
> 'Driver to use for volume creation')
> +flags.DEFINE_string('images_path', utils.abspath('../images'),
> + 'path to decrypted local images if not using s3')

Each flag must only be defined once. images_path is also defined in
nova.objectstore.images. Either import that or move the flag to a common
place and import that.

>>>> fixed by reverting.

> === modified file 'nova/virt/connection.py'
> --- nova/virt/connection.py 2010-08-30 13:19:14 +0000
> +++ nova/virt/connection.py 2010-10-13 07:15:24 +0000
> @@ -26,6 +26,7 @@
> from nova.virt import fake
> from nova.virt import libvirt_conn
> from nova.virt import xenapi
> +from nova.virt import hyperv
>
>
> FLAGS = flags.FLAGS
> @@ -49,6 +50,8 @@
> conn = libvirt_conn.get_connection(read_only)
> elif t == 'xenapi':
> conn = xenapi.get_connection(read_only)
> + elif t == 'hyperv':
> + conn = hyperv.get_connection(read_only)
> else:
> ...

[Soren Hansen (soren)]

On 14-10-2010 08:26, Chiradeep Vittal wrote:
> === added file 'nova/tests/hyperv_unittest.py'
> --- nova/tests/hyperv_unittest.py 1970-01-01 00:00:00 +0000
> +++ nova/tests/hyperv_unittest.py 2010-10-14 06:26:41 +0000
> @@ -0,0 +1,67 @@
> +# vim: tabstop=4 shiftwidth=4 softtabstop=4
> +#
> +# Copyright 2010 Cloud.com, Inc
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
you may
> +# not use this file except in compliance with the License. You may
obtain
> +# a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS"
BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the
> +# License for the specific language governing permissions and
limitations
> +# under the License.
> +"""
> +Tests For Hyper-V driver
> +"""
> +
> +import random
> +
> +from nova import db
> +from nova import flags
> +from nova import test
> +
> +from nova.virt import hyperv
> +
> +FLAGS = flags.FLAGS
> +FLAGS.connection_type = 'hyperv'
> +# Redis is probably not running on Hyper-V host.
> +# Change this to the actual Redis host
> +FLAGS.redis_host = '127.0.0.1'
> +
> +
> +class HyperVTestCase(test.TrialTestCase):
> + """Test cases for the Hyper-V driver"""
> + def setUp(self): # pylint: disable-msg=C0103
> + pass

Why define it at all?

> +
> + def test_create_destroy(self):
> + """Create a VM and destroy it"""
> + instance = {'internal_id' : random.randint(1, 1000000),
> + 'memory_mb' : '1024',
> + 'mac_address' : '02:12:34:46:56:67',
> + 'vcpus' : 2,
> + 'project_id' : 'fake',
> + 'instance_type' : 'm1.small'}
> +
> + instance_ref = db.instance_create(None, instance)
> +
> + conn = hyperv.get_connection(False)
> + conn._create_vm(instance_ref) # pylint: disable-msg=W0212
> + found = [n for n in conn.list_instances()
> + if n == instance_ref['name']]
> + self.assertTrue(len(found) == 1)
> + info = conn.get_info(instance_ref['name'])
> + #Unfortunately since the vm is not running at this point,
> + #we cannot obtain memory information from get_info
> + self.assertEquals(info['num_cpu'], instance_ref['vcpus'])
> +
> + conn.destroy(instance_ref)
> + found = [n for n in conn.list_instances()
> + if n == instance_ref['name']]
> + self.assertTrue(len(found) == 0)

Wouldn't this actually run the VM? The purpose of unit tests is to be
able to test things in units, so to verify that each piece of the puzzle
acts the way it should. Ideally, you should include a mock wmi module
that stubs out all the relevant methods so that we take the actual
hypervisor out of the equation.

> +
> + def tearDown(self): # pylint: disable-msg=C0103
> + pass

Again, why define it at all?

> === modified file 'nova/virt/connection.py'
> --- nova/virt/conne...

[Soren Hansen (soren)]

Apologies for messing up the formatting. I've given my mail client a thorough beating. It promises never to do that again.

[Jay Pipes (jaypipes)]

Thanks for correcting and responding to all the comments from Soren and myself, Chiradeep. Good work :)

review: Approve

[Chiradeep Vittal (chiradeep)]

> On 14-10-2010 08:26, Chiradeep Vittal wrote:
> > === added file 'nova/tests/hyperv_unittest.py'
> > --- nova/tests/hyperv_unittest.py 1970-01-01 00:00:00 +0000
> > +++ nova/tests/hyperv_unittest.py 2010-10-14 06:26:41 +0000
> > @@ -0,0 +1,67 @@
> > +# vim: tabstop=4 shiftwidth=4 softtabstop=4
> > +#
> > +# Copyright 2010 Cloud.com, Inc
> > +#
> > +# Licensed under the Apache License, Version 2.0 (the "License");
> you may
> > +# not use this file except in compliance with the License. You may
> obtain
> > +# a copy of the License at
> > +#
> > +# http://www.apache.org/licenses/LICENSE-2.0
> > +#
> > +# Unless required by applicable law or agreed to in writing, software
> > +# distributed under the License is distributed on an "AS IS"
> BASIS, WITHOUT
> > +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> See the
> > +# License for the specific language governing permissions and
> limitations
> > +# under the License.
> > +"""
> > +Tests For Hyper-V driver
> > +"""
> > +
> > +import random
> > +
> > +from nova import db
> > +from nova import flags
> > +from nova import test
> > +
> > +from nova.virt import hyperv
> > +
> > +FLAGS = flags.FLAGS
> > +FLAGS.connection_type = 'hyperv'
> > +# Redis is probably not running on Hyper-V host.
> > +# Change this to the actual Redis host
> > +FLAGS.redis_host = '127.0.0.1'
> > +
> > +
> > +class HyperVTestCase(test.TrialTestCase):
> > + """Test cases for the Hyper-V driver"""
> > + def setUp(self): # pylint: disable-msg=C0103
> > + pass
>
> Why define it at all?
>
> > +
> > + def test_create_destroy(self):
> > + """Create a VM and destroy it"""
> > + instance = {'internal_id' : random.randint(1, 1000000),
> > + 'memory_mb' : '1024',
> > + 'mac_address' : '02:12:34:46:56:67',
> > + 'vcpus' : 2,
> > + 'project_id' : 'fake',
> > + 'instance_type' : 'm1.small'}
> > +
> > + instance_ref = db.instance_create(None, instance)
> > +
> > + conn = hyperv.get_connection(False)
> > + conn._create_vm(instance_ref) # pylint: disable-msg=W0212
> > + found = [n for n in conn.list_instances()
> > + if n == instance_ref['name']]
> > + self.assertTrue(len(found) == 1)
> > + info = conn.get_info(instance_ref['name'])
> > + #Unfortunately since the vm is not running at this point,
> > + #we cannot obtain memory information from get_info
> > + self.assertEquals(info['num_cpu'], instance_ref['vcpus'])
> > +
> > + conn.destroy(instance_ref)
> > + found = [n for n in conn.list_instances()
> > + if n == instance_ref['name']]
> > + self.assertTrue(len(found) == 0)
>
> Wouldn't this actually run the VM? The purpose of unit tests is to be
> able to test things in units, so to verify that each piece of the puzzle
> acts the way it should. Ideally, you should include a mock wmi module
> that stubs out all the relevant methods so that we take the actual
> hypervisor out of the equation.
> ...

[Soren Hansen (soren)]

I just realised I forgot to vote in my last review..

Anyways, even with these last changes, I'm really not happy with the tests. They're no good if we can't run them.

Ideally, the mock thing would actually pretend to be Hyper-V, but failing that, it's not that hard to replace e.g. your _conn object with one that logs all calls and return values so that you can record them once and then write a simple shim that gives back the same responses when called the same way. I would say that's a minimal requirement. It doesn't cover what happens in case of errors or anything like that, but it's a start, and /especially/ when none of the rest of us have access to a Hyper-V box, it's essential that we have a way to check if things will continue to work.

Also, you're referencing something called VmResourceAllocationException. There's no such thing.

review: Disapprove

[Soren Hansen (soren)]

I should note, by the way, that I really appreciate the work you're
doing. Hyper-V support is a major step towards total world domination
:) I guess that's why I really want to make sure us Linux geeks have a
way to make sure we don't break it :)

[Chiradeep Vittal (chiradeep)]

It isn't just the _conn object that needs to be mocked. We retrieve lots of different types of WMI objects (Msvm_*) and call methods on them. This might be best served by a mocking framework. I need more time to investigate. Do you have any recommendations?
I don't think this is a couple of hours work. Can we get the merge in and raise a bug to keep track
of the unit test issue?

review: Needs Resubmitting

[Soren Hansen (soren)]

> It isn't just the _conn object that needs to be mocked. We retrieve lots of
> different types of WMI objects (Msvm_*) and call methods on them. This might
> be best served by a mocking framework. I need more time to investigate. Do you
> have any recommendations?

I'm afraid not. I don't recall having needed anything advanced like this myself.

> I don't think this is a couple of hours work. Can we get the merge in and
> raise a bug to keep track of the unit test issue?

We're well past feature freeze (and even final freeze), and we don't have a
branch open for our next release yet, so we can't actually technically merge
it yet anyway :-/

[Jay Pipes (jaypipes)]

On Thu, Oct 14, 2010 at 5:26 PM, Chiradeep Vittal <email address hidden> wrote:
> Review: Resubmit
> It isn't just the _conn object that needs to be mocked. We retrieve lots of different types of WMI objects (Msvm_*) and call methods on them. This might be best served by a mocking framework. I need more time to investigate. Do you have any recommendations?

We use a mocking framework called pymox already. Check out the code
in /nova/tests/api/openstack/fakes.py and the test files in that same
directory for examples of how it's used. I'm happy to help you if you
get a bit lost. Hop into IRC and we can chat about it :)

> I don't think this is a couple of hours work. Can we get the merge in and raise a bug to keep track
> of the unit test issue?

As Soren said, we couldn't do the merge right now into trunk because
we're in code freeze (only bug fixes can go in now). Once we get the
next release series branch going, we'll merge into there.

-jay

[Soren Hansen (soren)]

Sorry about not getting back to this sooner.

Ok, let's merge it, and once I finish my libvirt tests refactoring, you can perhaps look at that to see an example of what I'm talking about.

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/virt/images.py

[Soren Hansen (soren)]

Chiradeep, the merge was approved, but failed.

You need to merge with trunk, commit, and push again.

[Chiradeep Vittal (chiradeep)]

Thanks. I'll fix it

--
Chiradeep

On Nov 29, 2010, at 1:12 PM, Soren Hansen <email address hidden> wrote:

> Chiradeep, the merge was approved, but failed.
>
> You need to merge with trunk, commit, and push again.
> --
> https://code.launchpad.net/~chiradeep/nova/msft-hyper-v-support/+merge/38387
> You are the owner of lp:~chiradeep/nova/msft-hyper-v-support.

[Soren Hansen (soren)]

Great. Let me know if you need help.

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/

[Soren Hansen (soren)]

Ping?

[Chiradeep Vittal (chiradeep)]

Sorry about that. Was busy with a move. Just got Internet/phone etc going again. Will commit this week.

________________________________________
From: <email address hidden> [<email address hidden>] On Behalf Of Soren Hansen [<email address hidden>]
Sent: Monday, December 13, 2010 4:02 AM
To: <email address hidden>
Subject: Re: [Merge] lp:~chiradeep/nova/msft-hyper-v-support into lp:nova

Ping?
--
https://code.launchpad.net/~chiradeep/nova/msft-hyper-v-support/+merge/38387
You are the owner of lp:~chiradeep/nova/msft-hyper-v-support.

[Soren Hansen (soren)]

Great, thanks!

review: Approve

[Jay Pipes (jaypipes)]

Looks great, Chiradeep!

Warning: if the eventlet_merge branch gets merged into trunk before this, you will need to resolve a couple conflicts (process.simple_execute no longer exists IIRC..)

Just a heads up...not sure which will hit trunk first...

review: Approve

[Chiradeep Vittal (chiradeep)]

review: Approve

[Thierry Carrez (ttx)]

@Chiradeep: the long-awaited eventlet branch got merged now, please check your branch for conflicts.

[Devin Carlen (devcamcar)]

Awesome, will be great when this lands. Looks like we're just waiting for a fresh merge before this gets approved.

review: Approve

[Soren Hansen (soren)]

Chiradeep, this branch needs to be updated to the new, Twistedless world order. You need to get rid of the yields, you probably don't need the changes to nova/twistd.py anymore. Also, a round of testing to check that it still actually works would probably be a good idea :)

I'm also not sure you still need the redis_host stuff?

review: Needs Fixing

[Chiradeep Vittal (chiradeep)]

Yes, I was in the process of testing it out. Will update you later.

On 01/03/2011 01:27 AM, Soren Hansen wrote:
> Review: Needs Fixing
> Chiradeep, this branch needs to be updated to the new, Twistedless world order. You need to get rid of the yields, you probably don't need the changes to nova/twistd.py anymore. Also, a round of testing to check that it still actually works would probably be a good idea :)
>
> I'm also not sure you still need the redis_host stuff?
>

[Chiradeep Vittal (chiradeep)]

Fixed.
-- Removed dependencies on twisted
-- fixed logging and exceptions strings to enable i18n
-- removed redis dependency in unittest

On 01/03/2011 01:27 AM, Soren Hansen wrote:
> Review: Needs Fixing
> Chiradeep, this branch needs to be updated to the new, Twistedless world order. You need to get rid of the yields, you probably don't need the changes to nova/twistd.py anymore. Also, a round of testing to check that it still actually works would probably be a good idea :)
>
> I'm also not sure you still need the redis_host stuff?
>

[Soren Hansen (soren)]

You need to add yourself to the Authors file.

You probably want to do so by adding this to Authors:
Chiradeep Vittal <email address hidden>

and this to .mailmap:
<email address hidden> <chiradeep@chiradeep-lt2>

To tell bzr about your real e-mail, do this:
bzr whoami "Chiradeep Vittal <email address hidden>"

[Armando Migliaccio (armando-migliaccio)]

I think the test case should no longer inherit from TrialTestCase (line 36 nova/tests/hyperv_unittest.py) since we've moved away from Twisted Trial

review: Needs Fixing

[Armando Migliaccio (armando-migliaccio)]

A couple of questions about the unittest provided with this branch:

- does the test require an actual connection to hyperv? I couldn't find where this is stubbed out
- any particular reasons of calling the private method _create_vm, rather the public spawn method?
- does this test actually run/pass at all?

The call to

  instance_ref = db.instance_create(None, instance)

should raise an exception about the deprecation of a null context

[Chiradeep Vittal (chiradeep)]

Yes the test case is indeed broken. I am inclined to delete it since the
reviewers want a test case that can run without hyper-v. This would
involve a large amount of mocking of the Hyper-V WMI provider.

On 01/05/2011 05:02 AM, Armando Migliaccio wrote:
> A couple of questions about the unittest provided with this branch:
>
> - does the test require an actual connection to hyperv? I couldn't find where this is stubbed out
> - any particular reasons of calling the private method _create_vm, rather the public spawn method?
> - does this test actually run/pass at all?
>
> The call to
>
> instance_ref = db.instance_create(None, instance)
>
> should raise an exception about the deprecation of a null context
>

[lp:~chiradeep/nova/msft-hyper-v-support]

359. By Chiradeep Vittal on 2011-01-05

Make test case work again

360. By Chiradeep Vittal on 2011-01-05

Add to Authors and mailmap

[Vish Ishaya (vishvananda)]

looks like this can finally be merged. Will wait for a final approval from soren.

review: Approve

[Armando Migliaccio (armando-migliaccio)]

Hi Chiradeep,

thanks for the reply!

One more thing: the naming convention for unittest files has changed from <testname>_unittest.py to test_<testname>.py. I noticed that without renaming your test, run_test.sh does not process the file.

If you do rename the file, then the test will be run and (in my case) it fails because of a missing wmi module.

My take is that if the test isn't ready, it's better to remove it from the diff, as this might delay the merging process.

[Armando Migliaccio (armando-migliaccio)]

review: Approve

[Soren Hansen (soren)]

rock'n'roll.

review: Approve

=== modified file '.mailmap'
--- .mailmap	2010-12-30 17:26:21 +0000
+++ .mailmap	2011-01-05 23:07:23 +0000
@@ -30,3 +30,4 @@
 <[e-mail]> <[e-mail]> 
 <[e-mail]> <[e-mail]>
 <[e-mail]> <[e-mail]>
+<[e-mail]> <chiradeep@chiradeep-lt2>

=== modified file 'Authors'
--- Authors	2010-12-30 17:26:21 +0000
+++ Authors	2011-01-05 23:07:23 +0000
@@ -3,6 +3,7 @@
 Anthony Young <[e-mail]>
 Antony Messerli <[e-mail]>
 Armando Migliaccio <[e-mail]>
+Chiradeep Vittal <[e-mail]>
 Chris Behrens <[e-mail]>
 Chmouel Boudjnah <[e-mail]>
 Cory Wright <[e-mail]>

=== added file 'nova/tests/hyperv_unittest.py'
--- nova/tests/hyperv_unittest.py	1970-01-01 00:00:00 +0000
+++ nova/tests/hyperv_unittest.py	2011-01-05 23:07:23 +0000
@@ -0,0 +1,71 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+#
+#    Copyright 2010 Cloud.com, Inc
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+"""
+Tests For Hyper-V driver
+"""
+
+import random
+
+from nova import context
+from nova import db
+from nova import flags
+from nova import test
+from nova.auth import manager
+from nova.virt import hyperv
+
+FLAGS = flags.FLAGS
+FLAGS.connection_type = 'hyperv'
+
+
+class HyperVTestCase(test.TestCase):
+    """Test cases for the Hyper-V driver"""
+    def setUp(self):
+        super(HyperVTestCase, self).setUp()
+        self.manager = manager.AuthManager()
+        self.user = self.manager.create_user('fake', 'fake', 'fake',
+                                             admin=True)
+        self.project = self.manager.create_project('fake', 'fake', 'fake')
+        self.context = context.RequestContext(self.user, self.project)
+
+    def test_create_destroy(self):
+        """Create a VM and destroy it"""
+        instance = {'internal_id': random.randint(1, 1000000),
+                     'memory_mb': '1024',
+                     'mac_address': '02:12:34:46:56:67',
+                     'vcpus': 2,
+                     'project_id': 'fake',
+                     'instance_type': 'm1.small'}
+        instance_ref = db.instance_create(self.context, instance)
+
+        conn = hyperv.get_connection(False)
+        conn._create_vm(instance_ref)  # pylint: disable-msg=W0212
+        found = [n  for n in conn.list_instances()
+                      if n == instance_ref['name']]
+        self.assertTrue(len(found) == 1)
+        info = conn.get_info(instance_ref['name'])
+        #Unfortunately since the vm is not running at this point,
+        #we cannot obtain memory information from get_info
+        self.assertEquals(info['num_cpu'], instance_ref['vcpus'])
+
+        conn.destroy(instance_ref)
+        found = [n  for n in conn.list_instances()
+                      if n == instance_ref['name']]
+        self.assertTrue(len(found) == 0)
+
+    def tearDown(self):
+        super(HyperVTestCase, self).tearDown()
+        self.manager.delete_project(self.project)
+        self.manager.delete_user(self.user)

=== modified file 'nova/virt/connection.py'
--- nova/virt/connection.py	2010-12-22 20:59:53 +0000
+++ nova/virt/connection.py	2011-01-05 23:07:23 +0000
@@ -26,6 +26,7 @@
 from nova.virt import fake
 from nova.virt import libvirt_conn
 from nova.virt import xenapi_conn
+from nova.virt import hyperv
 
 
 FLAGS = flags.FLAGS
@@ -62,6 +63,8 @@
         conn = libvirt_conn.get_connection(read_only)
     elif t == 'xenapi':
         conn = xenapi_conn.get_connection(read_only)
+    elif t == 'hyperv':
+        conn = hyperv.get_connection(read_only)
     else:
         raise Exception('Unknown connection type "%s"' % t)
 

=== added file 'nova/virt/hyperv.py'
--- nova/virt/hyperv.py	1970-01-01 00:00:00 +0000
+++ nova/virt/hyperv.py	2011-01-05 23:07:23 +0000
@@ -0,0 +1,459 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2010 Cloud.com, Inc
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+A connection to Hyper-V .
+Uses Windows Management Instrumentation (WMI) calls to interact with Hyper-V
+Hyper-V WMI usage:
+    http://msdn.microsoft.com/en-us/library/cc723875%28v=VS.85%29.aspx
+The Hyper-V object model briefly:
+    The physical computer and its hosted virtual machines are each represented
+    by the Msvm_ComputerSystem class.
+
+    Each virtual machine is associated with a
+    Msvm_VirtualSystemGlobalSettingData (vs_gs_data) instance and one or more
+    Msvm_VirtualSystemSettingData (vmsetting) instances. For each vmsetting
+    there is a series of Msvm_ResourceAllocationSettingData (rasd) objects.
+    The rasd objects describe the settings for each device in a VM.
+    Together, the vs_gs_data, vmsettings and rasds describe the configuration
+    of the virtual machine.
+
+    Creating new resources such as disks and nics involves cloning a default
+    rasd object and appropriately modifying the clone and calling the
+    AddVirtualSystemResources WMI method
+    Changing resources such as memory uses the ModifyVirtualSystemResources
+    WMI method
+
+Using the Python WMI library:
+    Tutorial:
+        http://timgolden.me.uk/python/wmi/tutorial.html
+    Hyper-V WMI objects can be retrieved simply by using the class name
+    of the WMI object and optionally specifying a column to filter the
+    result set. More complex filters can be formed using WQL (sql-like)
+    queries.
+    The parameters and return tuples of WMI method calls can gleaned by
+    examining the doc string. For example:
+    >>> vs_man_svc.ModifyVirtualSystemResources.__doc__
+    ModifyVirtualSystemResources (ComputerSystem, ResourceSettingData[])
+                 => (Job, ReturnValue)'
+    When passing setting data (ResourceSettingData) to the WMI method,
+    an XML representation of the data is passed in using GetText_(1).
+    Available methods on a service can be determined using method.keys():
+    >>> vs_man_svc.methods.keys()
+    vmsettings and rasds for a vm can be retrieved using the 'associators'
+    method with the appropriate return class.
+    Long running WMI commands generally return a Job (an instance of
+    Msvm_ConcreteJob) whose state can be polled to determine when it finishes
+
+"""
+
+import os
+import logging
+import time
+
+from nova import exception
+from nova import flags
+from nova.auth import manager
+from nova.compute import power_state
+from nova.virt import images
+
+wmi = None
+
+
+FLAGS = flags.FLAGS
+
+
+HYPERV_POWER_STATE = {
+    3: power_state.SHUTDOWN,
+    2: power_state.RUNNING,
+    32768: power_state.PAUSED,
+}
+
+
+REQ_POWER_STATE = {
+    'Enabled': 2,
+    'Disabled': 3,
+    'Reboot': 10,
+    'Reset': 11,
+    'Paused': 32768,
+    'Suspended': 32769
+}
+
+
+WMI_JOB_STATUS_STARTED = 4096
+WMI_JOB_STATE_RUNNING = 4
+WMI_JOB_STATE_COMPLETED = 7
+
+
+def get_connection(_):
+    global wmi
+    if wmi is None:
+        wmi = __import__('wmi')
+    return HyperVConnection()
+
+
+class HyperVConnection(object):
+    def __init__(self):
+        self._conn = wmi.WMI(moniker='//./root/virtualization')
+        self._cim_conn = wmi.WMI(moniker='//./root/cimv2')
+
+    def init_host(self):
+        #FIXME(chiradeep): implement this
+        logging.debug(_('In init host'))
+        pass
+
+    def list_instances(self):
+        """ Return the names of all the instances known to Hyper-V. """
+        vms = [v.ElementName \
+                for v in self._conn.Msvm_ComputerSystem(['ElementName'])]
+        return vms
+
+    def spawn(self, instance):
+        """ Create a new VM and start it."""
+        vm = self._lookup(instance.name)
+        if vm is not None:
+            raise exception.Duplicate(_('Attempt to create duplicate vm %s') %
+                            instance.name)
+
+        user = manager.AuthManager().get_user(instance['user_id'])
+        project = manager.AuthManager().get_project(instance['project_id'])
+        #Fetch the file, assume it is a VHD file.
+        base_vhd_filename = os.path.join(FLAGS.instances_path,
+                                         instance.name)
+        vhdfile = "%s.vhd" % (base_vhd_filename)
+        images.fetch(instance['image_id'], vhdfile, user, project)
+
+        try:
+            self._create_vm(instance)
+
+            self._create_disk(instance['name'], vhdfile)
+            self._create_nic(instance['name'], instance['mac_address'])
+
+            logging.debug(_('Starting VM %s '), instance.name)
+            self._set_vm_state(instance['name'], 'Enabled')
+            logging.info(_('Started VM %s '), instance.name)
+        except Exception as exn:
+            logging.error(_('spawn vm failed: %s'), exn)
+            self.destroy(instance)
+
+    def _create_vm(self, instance):
+        """Create a VM but don't start it.  """
+        vs_man_svc = self._conn.Msvm_VirtualSystemManagementService()[0]
+
+        vs_gs_data = self._conn.Msvm_VirtualSystemGlobalSettingData.new()
+        vs_gs_data.ElementName = instance['name']
+        (job, ret_val) = vs_man_svc.DefineVirtualSystem(
+                                        [], None, vs_gs_data.GetText_(1))[1:]
+        if ret_val == WMI_JOB_STATUS_STARTED:
+            success = self._check_job_status(job)
+        else:
+            success = (ret_val == 0)
+
+        if not success:
+            raise Exception(_('Failed to create VM %s'), instance.name)
+
+        logging.debug(_('Created VM %s...'), instance.name)
+        vm = self._conn.Msvm_ComputerSystem(ElementName=instance.name)[0]
+
+        vmsettings = vm.associators(
+                          wmi_result_class='Msvm_VirtualSystemSettingData')
+        vmsetting = [s for s in vmsettings
+                        if s.SettingType == 3][0]  # avoid snapshots
+        memsetting = vmsetting.associators(
+                           wmi_result_class='Msvm_MemorySettingData')[0]
+        #No Dynamic Memory, so reservation, limit and quantity are identical.
+        mem = long(str(instance['memory_mb']))
+        memsetting.VirtualQuantity = mem
+        memsetting.Reservation = mem
+        memsetting.Limit = mem
+
+        (job, ret_val) = vs_man_svc.ModifyVirtualSystemResources(
+                                        vm.path_(), [memsetting.GetText_(1)])
+        logging.debug(_('Set memory for vm %s...'), instance.name)
+        procsetting = vmsetting.associators(
+                          wmi_result_class='Msvm_ProcessorSettingData')[0]
+        vcpus = long(instance['vcpus'])
+        procsetting.VirtualQuantity = vcpus
+        procsetting.Reservation = vcpus
+        procsetting.Limit = vcpus
+
+        (job, ret_val) = vs_man_svc.ModifyVirtualSystemResources(
+                                        vm.path_(), [procsetting.GetText_(1)])
+        logging.debug(_('Set vcpus for vm %s...'), instance.name)
+
+    def _create_disk(self, vm_name, vhdfile):
+        """Create a disk and attach it to the vm"""
+        logging.debug(_('Creating disk for %s by attaching disk file %s'),
+                        vm_name, vhdfile)
+        #Find the IDE controller for the vm.
+        vms = self._conn.MSVM_ComputerSystem(ElementName=vm_name)
+        vm = vms[0]
+        vmsettings = vm.associators(
+                        wmi_result_class='Msvm_VirtualSystemSettingData')
+        rasds = vmsettings[0].associators(
+                        wmi_result_class='MSVM_ResourceAllocationSettingData')
+        ctrller = [r for r in rasds
+                   if r.ResourceSubType == 'Microsoft Emulated IDE Controller'\
+                                         and r.Address == "0"]
+        #Find the default disk drive object for the vm and clone it.
+        diskdflt = self._conn.query(
+                  "SELECT * FROM Msvm_ResourceAllocationSettingData \
+                   WHERE ResourceSubType LIKE 'Microsoft Synthetic Disk Drive'\
+                   AND InstanceID LIKE '%Default%'")[0]
+        diskdrive = self._clone_wmi_obj(
+                    'Msvm_ResourceAllocationSettingData', diskdflt)
+        #Set the IDE ctrller as parent.
+        diskdrive.Parent = ctrller[0].path_()
+        diskdrive.Address = 0
+        #Add the cloned disk drive object to the vm.
+        new_resources = self._add_virt_resource(diskdrive, vm)
+        if new_resources is None:
+            raise Exception(_('Failed to add diskdrive to VM %s'),
+                                             vm_name)
+        diskdrive_path = new_resources[0]
+        logging.debug(_('New disk drive path is %s'), diskdrive_path)
+        #Find the default VHD disk object.
+        vhddefault = self._conn.query(
+                "SELECT * FROM Msvm_ResourceAllocationSettingData \
+                 WHERE ResourceSubType LIKE 'Microsoft Virtual Hard Disk' AND \
+                 InstanceID LIKE '%Default%' ")[0]
+
+        #Clone the default and point it to the image file.
+        vhddisk = self._clone_wmi_obj(
+                'Msvm_ResourceAllocationSettingData', vhddefault)
+        #Set the new drive as the parent.
+        vhddisk.Parent = diskdrive_path
+        vhddisk.Connection = [vhdfile]
+
+        #Add the new vhd object as a virtual hard disk to the vm.
+        new_resources = self._add_virt_resource(vhddisk, vm)
+        if new_resources is None:
+            raise Exception(_('Failed to add vhd file to VM %s'),
+                                             vm_name)
+        logging.info(_('Created disk for %s'), vm_name)
+
+    def _create_nic(self, vm_name, mac):
+        """Create a (emulated) nic and attach it to the vm"""
+        logging.debug(_('Creating nic for %s '), vm_name)
+        #Find the vswitch that is connected to the physical nic.
+        vms = self._conn.Msvm_ComputerSystem(ElementName=vm_name)
+        extswitch = self._find_external_network()
+        vm = vms[0]
+        switch_svc = self._conn.Msvm_VirtualSwitchManagementService()[0]
+        #Find the default nic and clone it to create a new nic for the vm.
+        #Use Msvm_SyntheticEthernetPortSettingData for Windows or Linux with
+        #Linux Integration Components installed.
+        emulatednics_data = self._conn.Msvm_EmulatedEthernetPortSettingData()
+        default_nic_data = [n for n in emulatednics_data
+                            if n.InstanceID.rfind('Default') > 0]
+        new_nic_data = self._clone_wmi_obj(
+                                      'Msvm_EmulatedEthernetPortSettingData',
+                                      default_nic_data[0])
+        #Create a port on the vswitch.
+        (new_port, ret_val) = switch_svc.CreateSwitchPort(vm_name, vm_name,
+                                            "", extswitch.path_())
+        if ret_val != 0:
+            logging.error(_('Failed creating a port on the external vswitch'))
+            raise Exception(_('Failed creating port for %s'),
+                                             vm_name)
+        logging.debug(_("Created switch port %s on switch %s"),
+                        vm_name, extswitch.path_())
+        #Connect the new nic to the new port.
+        new_nic_data.Connection = [new_port]
+        new_nic_data.ElementName = vm_name + ' nic'
+        new_nic_data.Address = ''.join(mac.split(':'))
+        new_nic_data.StaticMacAddress = 'TRUE'
+        #Add the new nic to the vm.
+        new_resources = self._add_virt_resource(new_nic_data, vm)
+        if new_resources is None:
+            raise Exception(_('Failed to add nic to VM %s'),
+                                             vm_name)
+        logging.info(_("Created nic for %s "), vm_name)
+
+    def _add_virt_resource(self, res_setting_data, target_vm):
+        """Add a new resource (disk/nic) to the VM"""
+        vs_man_svc = self._conn.Msvm_VirtualSystemManagementService()[0]
+        (job, new_resources, ret_val) = vs_man_svc.\
+                    AddVirtualSystemResources([res_setting_data.GetText_(1)],
+                                                target_vm.path_())
+        success = True
+        if ret_val == WMI_JOB_STATUS_STARTED:
+            success = self._check_job_status(job)
+        else:
+            success = (ret_val == 0)
+        if success:
+            return new_resources
+        else:
+            return None
+
+    #TODO: use the reactor to poll instead of sleep
+    def _check_job_status(self, jobpath):
+        """Poll WMI job state for completion"""
+        #Jobs have a path of the form:
+        #\\WIN-P5IG7367DAG\root\virtualization:Msvm_ConcreteJob.InstanceID=
+        #"8A496B9C-AF4D-4E98-BD3C-1128CD85320D"
+        inst_id = jobpath.split('=')[1].strip('"')
+        jobs = self._conn.Msvm_ConcreteJob(InstanceID=inst_id)
+        if len(jobs) == 0:
+            return False
+        job = jobs[0]
+        while job.JobState == WMI_JOB_STATE_RUNNING:
+            time.sleep(0.1)
+            job = self._conn.Msvm_ConcreteJob(InstanceID=inst_id)[0]
+        if job.JobState != WMI_JOB_STATE_COMPLETED:
+            logging.debug(_("WMI job failed: %s"), job.ErrorSummaryDescription)
+            return False
+        logging.debug(_("WMI job succeeded: %s, Elapsed=%s "), job.Description,
+                      job.ElapsedTime)
+        return True
+
+    def _find_external_network(self):
+        """Find the vswitch that is connected to the physical nic.
+           Assumes only one physical nic on the host
+        """
+        #If there are no physical nics connected to networks, return.
+        bound = self._conn.Msvm_ExternalEthernetPort(IsBound='TRUE')
+        if len(bound) == 0:
+            return None
+        return self._conn.Msvm_ExternalEthernetPort(IsBound='TRUE')[0]\
+            .associators(wmi_result_class='Msvm_SwitchLANEndpoint')[0]\
+            .associators(wmi_result_class='Msvm_SwitchPort')[0]\
+            .associators(wmi_result_class='Msvm_VirtualSwitch')[0]
+
+    def _clone_wmi_obj(self, wmi_class, wmi_obj):
+        """Clone a WMI object"""
+        cl = self._conn.__getattr__(wmi_class)  # get the class
+        newinst = cl.new()
+        #Copy the properties from the original.
+        for prop in wmi_obj._properties:
+            newinst.Properties_.Item(prop).Value =\
+                    wmi_obj.Properties_.Item(prop).Value
+        return newinst
+
+    def reboot(self, instance):
+        """Reboot the specified instance."""
+        vm = self._lookup(instance.name)
+        if vm is None:
+            raise exception.NotFound('instance not present %s' % instance.name)
+        self._set_vm_state(instance.name, 'Reboot')
+
+    def destroy(self, instance):
+        """Destroy the VM. Also destroy the associated VHD disk files"""
+        logging.debug(_("Got request to destroy vm %s"), instance.name)
+        vm = self._lookup(instance.name)
+        if vm is None:
+            return
+        vm = self._conn.Msvm_ComputerSystem(ElementName=instance.name)[0]
+        vs_man_svc = self._conn.Msvm_VirtualSystemManagementService()[0]
+        #Stop the VM first.
+        self._set_vm_state(instance.name, 'Disabled')
+        vmsettings = vm.associators(
+                         wmi_result_class='Msvm_VirtualSystemSettingData')
+        rasds = vmsettings[0].associators(
+                         wmi_result_class='MSVM_ResourceAllocationSettingData')
+        disks = [r for r in rasds \
+                    if r.ResourceSubType == 'Microsoft Virtual Hard Disk']
+        diskfiles = []
+        #Collect disk file information before destroying the VM.
+        for disk in disks:
+            diskfiles.extend([c for c in disk.Connection])
+        #Nuke the VM. Does not destroy disks.
+        (job, ret_val) = vs_man_svc.DestroyVirtualSystem(vm.path_())
+        if ret_val == WMI_JOB_STATUS_STARTED:
+            success = self._check_job_status(job)
+        elif ret_val == 0:
+            success = True
+        if not success:
+            raise Exception(_('Failed to destroy vm %s') % instance.name)
+        #Delete associated vhd disk files.
+        for disk in diskfiles:
+            vhdfile = self._cim_conn.CIM_DataFile(Name=disk)
+            for vf in vhdfile:
+                vf.Delete()
+                logging.debug(_("Del: disk %s vm %s"), vhdfile, instance.name)
+
+    def get_info(self, instance_id):
+        """Get information about the VM"""
+        vm = self._lookup(instance_id)
+        if vm is None:
+            raise exception.NotFound('instance not present %s' % instance_id)
+        vm = self._conn.Msvm_ComputerSystem(ElementName=instance_id)[0]
+        vs_man_svc = self._conn.Msvm_VirtualSystemManagementService()[0]
+        vmsettings = vm.associators(
+                       wmi_result_class='Msvm_VirtualSystemSettingData')
+        settings_paths = [v.path_() for v in vmsettings]
+        #See http://msdn.microsoft.com/en-us/library/cc160706%28VS.85%29.aspx
+        summary_info = vs_man_svc.GetSummaryInformation(
+                                       [4, 100, 103, 105], settings_paths)[1]
+        info = summary_info[0]
+        logging.debug(_("Got Info for vm %s: state=%s, mem=%s, num_cpu=%s, \
+                    cpu_time=%s"), instance_id,
+                    str(HYPERV_POWER_STATE[info.EnabledState]),
+                    str(info.MemoryUsage),
+                    str(info.NumberOfProcessors),
+                    str(info.UpTime))
+
+        return {'state': HYPERV_POWER_STATE[info.EnabledState],
+                'max_mem': info.MemoryUsage,
+                'mem': info.MemoryUsage,
+                'num_cpu': info.NumberOfProcessors,
+                'cpu_time': info.UpTime}
+
+    def _lookup(self, i):
+        vms = self._conn.Msvm_ComputerSystem(ElementName=i)
+        n = len(vms)
+        if n == 0:
+            return None
+        elif n > 1:
+            raise Exception(_('duplicate name found: %s') % i)
+        else:
+            return vms[0].ElementName
+
+    def _set_vm_state(self, vm_name, req_state):
+        """Set the desired state of the VM"""
+        vms = self._conn.Msvm_ComputerSystem(ElementName=vm_name)
+        if len(vms) == 0:
+            return False
+        (job, ret_val) = vms[0].RequestStateChange(REQ_POWER_STATE[req_state])
+        success = False
+        if ret_val == WMI_JOB_STATUS_STARTED:
+            success = self._check_job_status(job)
+        elif ret_val == 0:
+            success = True
+        elif ret_val == 32775:
+            #Invalid state for current operation. Typically means it is
+            #already in the state requested
+            success = True
+        if success:
+            logging.info(_("Successfully changed vm state of %s to %s"),
+                                vm_name, req_state)
+        else:
+            logging.error(_("Failed to change vm state of %s to %s"),
+                                vm_name, req_state)
+            raise Exception(_("Failed to change vm state of %s to %s"),
+                                        vm_name, req_state)
+
+    def attach_volume(self, instance_name, device_path, mountpoint):
+        vm = self._lookup(instance_name)
+        if vm is None:
+            raise exception.NotFound('Cannot attach volume to missing %s vm' %
+                            instance_name)
+
+    def detach_volume(self, instance_name, mountpoint):
+        vm = self._lookup(instance_name)
+        if vm is None:
+            raise exception.NotFound('Cannot detach volume from missing %s ' %
+                            instance_name)

=== modified file 'nova/virt/images.py'
--- nova/virt/images.py	2010-12-10 00:18:52 +0000
+++ nova/virt/images.py	2011-01-05 23:07:23 +0000
@@ -21,8 +21,12 @@
 Handling of VM disk images.
 """
 
+import logging
 import os.path
+import shutil
+import sys
 import time
+import urllib2
 import urlparse
 
 from nova import flags
@@ -45,6 +49,25 @@
     return f(image, path, user, project)
 
 
+def _fetch_image_no_curl(url, path, headers):
+    request = urllib2.Request(url)
+    for (k, v) in headers.iteritems():
+        request.add_header(k, v)
+
+    def urlretrieve(urlfile, fpath):
+        chunk = 1 * 1024 * 1024
+        f = open(fpath, "wb")
+        while 1:
+            data = urlfile.read(chunk)
+            if not data:
+                break
+            f.write(data)
+
+    urlopened = urllib2.urlopen(request)
+    urlretrieve(urlopened, path)
+    logging.debug(_("Finished retreving %s -- placed in %s"), url, path)
+
+
 def _fetch_s3_image(image, path, user, project):
     url = image_url(image)
 
@@ -61,18 +84,24 @@
                                                                      url_path)
     headers['Authorization'] = 'AWS %s:%s' % (access, signature)
 
-    cmd = ['/usr/bin/curl', '--fail', '--silent', url]
-    for (k, v) in headers.iteritems():
-        cmd += ['-H', '"%s: %s"' % (k, v)]
+    if sys.platform.startswith('win'):
+        return _fetch_image_no_curl(url, path, headers)
+    else:
+        cmd = ['/usr/bin/curl', '--fail', '--silent', url]
+        for (k, v) in headers.iteritems():
+            cmd += ['-H', '%s: %s' % (k, v)]
 
-    cmd += ['-o', path]
-    cmd_out = ' '.join(cmd)
-    return utils.execute(cmd_out)
+        cmd += ['-o', path]
+        cmd_out = ' '.join(cmd)
+        return utils.execute(cmd_out)
 
 
 def _fetch_local_image(image, path, user, project):
-    source = _image_path('%s/image' % image)
-    return utils.execute('cp %s %s' % (source, path))
+    source = _image_path(os.path.join(image, 'image'))
+    if sys.platform.startswith('win'):
+        return shutil.copy(source, path)
+    else:
+        return utils.execute('cp %s %s' % (source, path))
 
 
 def _image_path(path):