Merge lp:~nttdata/nova/kvm-pause-suspend into lp:~hudson-openstack/nova/trunk

[Kei Masumoto]

added pause/suspend implementation to nova.virt.libvirt_conn

[Isaku Yamahata (yamahata)]

Hi. Repeating same logic seems ugly. How about something like the following?

def _do_dom(self, action, instance_name)
   if self.readonly:
      tmpconn ...

      action(dom)
      tmpconn.close()
   else:
      dom = ...
      action(dom)

def pause(...)
...
   self._do_dom(virtDoain.suspend)

...

thanks,

On Wed, May 25, 2011 at 08:23:29AM -0000, Kei Masumoto wrote:
> Kei Masumoto has proposed merging lp:~nttdata/nova/kvm-pause-suspend into lp:nova.
>
> Requested reviews:
> Nova Core (nova-core)
>
> For more details, see:
> https://code.launchpad.net/~nttdata/nova/kvm-pause-suspend/+merge/62255
>
> added pause/suspend implementation to nova.virt.libvirt_conn
> --
> https://code.launchpad.net/~nttdata/nova/kvm-pause-suspend/+merge/62255
> You are subscribed to branch lp:nova.

> === modified file 'nova/virt/libvirt_conn.py'
> --- nova/virt/libvirt_conn.py 2011-05-17 16:28:44 +0000
> +++ nova/virt/libvirt_conn.py 2011-05-25 08:17:27 +0000
> @@ -550,19 +550,54 @@
>
> @exception.wrap_exception
> def pause(self, instance, callback):
> - raise exception.ApiError("pause not supported for libvirt.")
> + """Pause VM instance"""
> + if self.read_only:
> + tmpconn = self._connect(self.libvirt_uri, False)
> + dom = tmpconn.lookupByName(instance.name)
> + dom.suspend()
> + tmpconn.close()
> + else:
> + dom = self._conn.lookupByName(instance.name)
> + dom.suspend()
>
> @exception.wrap_exception
> def unpause(self, instance, callback):
> - raise exception.ApiError("unpause not supported for libvirt.")
> + """Unpause paused VM instance"""
> + if self.read_only:
> + tmpconn = self._connect(self.libvirt_uri, False)
> + dom = tmpconn.lookupByName(instance.name)
> + dom.resume()
> + tmpconn.close()
> + else:
> + dom = self._conn.lookupByName(instance.name)
> + dom.resume()
>
> @exception.wrap_exception
> def suspend(self, instance, callback):
> - raise exception.ApiError("suspend not supported for libvirt")
> + """Suspend the specified instance"""
> + if self.read_only:
> + tmpconn = self._connect(self.libvirt_uri, False)
> + dom = tmpconn.lookupByName(instance.name)
> + dom.managedSave(0)
> + tmpconn.close()
> + else:
> + dom = self._conn.lookupByName(instance.name)
> + dom.managedSave(0)
>
> @exception.wrap_exception
> def resume(self, instance, callback):
> - raise exception.ApiError("resume not supported for libvirt")
> + """resume the specified instance"""
> + try:
> + if self.read_only:
> + tmpconn = self._connect(self.libvirt_uri, False)
> + dom = tmpconn.lookupByName(instance.name)
> + tmpconn.close()
> + else:
> + dom = self._conn.lookupByName(instance.name)
> + dom.create()
> + except libvirt.LibvirtError:
> + xml = self.to_xml(instance, None)
> + self._create_new_domain(xml)
>...

[Kei Masumoto (masumotok)]

Hi, thanks for comments.

The reason why I simply wrote same logic is just I was not much confident to make it brief.
I'm gonna fix that soon..

[Brian Lamar (blamar)]

Hey Kei,

Thanks for the contribution!

I have a very big question as to *why* we need to be checking for read_only before every call. I see this pattern being used once or twice in the current code, but I would rather not repeat it unless someone can explain it to me :)

If the connection is read-only, then actions which require write privileges should fail. I don't see the point in creating a temporary connection.

Also, there is a "_lookup_by_name" method which might help with some error cases.

review: Needs Fixing

[Kei Masumoto (masumotok)]

Hi Brian.
Thanks for review!

Agreed to your all comments. Fixed them soon.
nova.virt.libvirt_conn._live_migration also checks read_only. I'm going to raise new bug report later.

[justinsb (justin-fathomdb)]

This looks good, except for the exception handling logic in resume.

I think you should check for the exact error code when you've got the exception, rather than assuming that every exception is the one you expect (e.g. unexpected exceptions are thrown when the libvirt daemon isn't started, or when you don't have the right permissions).

Also, if there is a specific case you're trying to handle, can you please add a comment to the code so that everyone knows what it is. In general, I'm not sure we should be putting extra logic into the resume function - it just feels like it's the wrong layer. But I could be wrong - what is the situation you're handling here?

review: Needs Information

[Brian Lamar (blamar)]

> Also, if there is a specific case you're trying to handle, can you please add
> a comment to the code so that everyone knows what it is. In general, I'm not
> sure we should be putting extra logic into the resume function - it just feels
> like it's the wrong layer. But I could be wrong - what is the situation
> you're handling here?

While I do think this is where the logic should go...I do think we should probably just fail if dom.resume() fails. Something like:

"Tried to resume a VM which was not suspended."

Creating a new VM when calling resume seems like a bit of a unneeded side-effect.

[Kei Masumoto (masumotok)]

Hi, thanks for review!

> Also, if there is a specific case you're trying to handle, can you please
> add a comment to the code so that everyone knows what it is.
Before I wrote some comments in source code, let me explain.

I have encountered situations that /etc/libvirt/qemu/instance-<id>.xml was erased by admin's mistakes after suspending instance( or anytime when instances have problem). Then just calling dom.create() doen't work(raise exception). I know that situation is not normally happens, so that error handling isn't inevitable and not essential. But few line of error handling code don’t disturb anything.

Can I add exception handling and comments, or should I erase all error handling here?

[Kei Masumoto (masumotok)]

Sorry Brian, I didn’t check your reply.

> Creating a new VM when calling resume seems like a bit of a unneeded side-effect.
OK. Error handling here should be erased.

[Jay Pipes (jaypipes)]

lgtm.

review: Approve

[Kei Masumoto (masumotok)]

Thanks for reviewing, Jay!

[Brian Lamar (blamar)]

I'd love to see some tests, but we're really not to the point where tests are a viable option. Also, because testing it so difficult I think there are some error cases that we're missing appropriate handling for... however this is better than not having it implemented in my opinion so thank you for implementing! :)

review: Approve

[Devin Carlen (devcamcar)]

lgtm!

review: Approve

[Vish Ishaya (vishvananda)]

this looks good to me as well

review: Approve

[Dan Prince (dan-prince)]

lgtm.

review: Approve

[Kei Masumoto (masumotok)]

To all reviewers: Thanks for taking time and review!

=== modified file 'nova/virt/libvirt/connection.py'
--- nova/virt/libvirt/connection.py	2011-05-19 20:25:57 +0000
+++ nova/virt/libvirt/connection.py	2011-05-30 05:18:25 +0000
@@ -488,19 +488,27 @@
 
     @exception.wrap_exception
     def pause(self, instance, callback):
-        raise exception.ApiError("pause not supported for libvirt.")
+        """Pause VM instance"""
+        dom = self._lookup_by_name(instance.name)
+        dom.suspend()
 
     @exception.wrap_exception
     def unpause(self, instance, callback):
-        raise exception.ApiError("unpause not supported for libvirt.")
+        """Unpause paused VM instance"""
+        dom = self._lookup_by_name(instance.name)
+        dom.resume()
 
     @exception.wrap_exception
     def suspend(self, instance, callback):
-        raise exception.ApiError("suspend not supported for libvirt")
+        """Suspend the specified instance"""
+        dom = self._lookup_by_name(instance.name)
+        dom.managedSave(0)
 
     @exception.wrap_exception
     def resume(self, instance, callback):
-        raise exception.ApiError("resume not supported for libvirt")
+        """resume the specified instance"""
+        dom = self._lookup_by_name(instance.name)
+        dom.create()
 
     @exception.wrap_exception
     def rescue(self, instance):