Code review comment for lp:~nttdata/nova/kvm-pause-suspend

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)
>
> @exception.wrap_exception
> def rescue(self, instance):
>

--
yamahata