Merge lp:~tr3buchet/nova/xs_suspend into lp:~hudson-openstack/nova/trunk

[Trey Morris]

I've added suspend along with a few changes to power state as well. I can't imagine suspend will be controversial but I've added a new power state for "suspended" to nova.compute.power_states which libvirt doesn't use and updated the xenapi power mapping to use it for suspended state. I also updated the mappings in nova.api.openstack.servers to map PAUSED to "error" and SUSPENDED to "suspended". Thoughts there are that we don't currently (openstack API v1.0) use pause, so if somehow an instance were to be paused an error occurred somewhere, or someone did something in error. Either way asking the xenserver host for the status would show "paused". Support for more power states needs to be added to the next version of the openstack API.

merge notes:
merge with trunk went well locally
unittests passed locally
testing of suspend succeeded locally

Notes for testing:
Suspending an instance requires PV drivers.
If using the cloudservers API, suspend and resume are not implemented. I tested by modifying in place nova.api.openstack.servers after pulling my branch. I made pause() and unpause() immediately return self.suspend() and self.resume(). Afterwards cloudserver pause (id) and cloudserver unpause (id) will perform the suspend/resume, and you can watch the compute worker log for info on what is happening.

SAMPLE LOG OUTPUT FOR PAUSE/UNPAUSE +++++++++++++++++++++
DEBUG:root:instance 1178831109: pausing
INFO:root:Task OpaqueRef:ba3b1595-0d70-574a-aaca-27442d04be87 status: success.
INFO:root:(VM_UTILS) xenserver vm state -> |Paused|
INFO:root:(VM_UTILS) xenapi power_state -> |3|
DEBUG:root:instance 1178831109: unpausing
INFO:root:Task OpaqueRef:2d9cb792-0ce5-be23-6c50-3dc52cf1535c status: success.
INFO:root:(VM_UTILS) xenserver vm state -> |Running|
INFO:root:(VM_UTILS) xenapi power_state -> |1|

SAMPLE LOG OUTPUT FOR SUSPEND/RESUME +++++++++++++++++++++
DEBUG:root:instance 1178831109: suspending
INFO:root:Task OpaqueRef:bc5d51da-922f-c9fb-f07c-aaa027fde601 status: success.
INFO:root:(VM_UTILS) xenserver vm state -> |Suspended|
INFO:root:(VM_UTILS) xenapi power_state -> |7|
DEBUG:root:instance 1178831109: resuming
INFO:root:Task OpaqueRef:4e5a3af8-baed-c144-30ea-63637390f6c8 status: success.
INFO:root:(VM_UTILS) xenserver vm state -> |Running|
INFO:root:(VM_UTILS) xenapi power_state -> |1|

[Armando Migliaccio (armando-migliaccio)]

It is true that xenapi-unittest is a throw's stone away from being in trunk, but I would encourage start writing unit tests for xenapi along with this branch. During my last merges a number of changes broke the ones written in lp:~armando-migliaccio/nova/xenapi-unittests and that helped me a lot to figure out how to fix things.

Plus, it would be useful very useful for us getting an early feedback on how the fake xenapi works in practice for everyone.
Thanks,
Armando

[Jay Pipes (jaypipes)]

Hi!

Good stuff, Trey! And thanks for the answers to my questions on IRC :)

One tiny thing to address:

Please mark all strings that go to log files with _() so they may be picked up by gettext and the message building facilities.

So, things like:

95 + logging.debug('instance %s: suspending', instance_ref['internal_id'])

should be:

95 + logging.debug(_('instance %s: suspending'), instance_ref['internal_id'])

the little _()'s mark the string 'instance %s: suspending' to be translated.

Cheers!
jay

review: Needs Fixing

[Trey Morris (tr3buchet)]

will do!

[Matt Dietz (cerberus)]

I disagree with the assessment that we have implementation specific unit tests for generalized functionality. Something like suspend should, at least in theory, operate in the same abstracted way across all supporting hypervisors. I would argue that his tests in this regard are sufficient. For XenServer specific functionality, such as xenstore interaction, the maybe we want targeted tests.

Otherwise, it looks good to me, minus Jay's internationalization fixes.

review: Approve

[Trey Morris (tr3buchet)]

alrighty, fixed. Sorry for the delay, I bit off a huge chunk of messages to correct with _(). That coupled with some large changes to trunk left me a giant merge, so I backed off some.

[Jay Pipes (jaypipes)]

Hi again!

Good stuff! Missed 2 strings, though :)

323 + raise Exception("suspend: instance not present %s" % instance_name)
and
332 + raise Exception("resume: instance not present %s" % instance_name)

fix those up and approve from me! :)

[Armando Migliaccio (armando-migliaccio)]

Matthew,

I am not suggesting to have implementation specific unit tests per se. I am suggesting to add tests where it makes sense. I saw features being added to xenapi, so I sounded sensible to me suggesting a number of tests to verify that part of the code.

> I disagree with the assessment that we have implementation specific unit tests
> for generalized functionality. Something like suspend should, at least in
> theory, operate in the same abstracted way across all supporting hypervisors.
> I would argue that his tests in this regard are sufficient. For XenServer
> specific functionality, such as xenstore interaction, the maybe we want
> targeted tests.
>
> Otherwise, it looks good to me, minus Jay's internationalization fixes.

[Trey Morris (tr3buchet)]

Jay Pipes: fixed!

[Jay Pipes (jaypipes)]

w00tness.

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~tr3buchet/nova/xs_suspend into lp:nova failed. Below is the output from the failed tests.

nova/compute/manager.py:305:80: E501 line too long (80 characters)
        logging.debug(_('instance %s: suspending'), instance_ref['internal_id'])
                                                                               ^
    Limit all lines to a maximum of 79 characters.

    There are still many devices around that are limited to 80 character
    lines; plus, limiting windows to 80 characters makes it possible to have
    several windows side-by-side. The default wrapping on such devices looks
    ugly. Therefore, please limit all lines to a maximum of 79 characters.
    For flowing long blocks of text (docstrings or comments), limiting the
    length to 72 characters is recommended.

[lp:~tr3buchet/nova/xs_suspend]

479. By Trey Morris on 2010-12-28

fixed a line length

[Trey Morris (tr3buchet)]

fixed... Can't believe I missed that.

[Jay Pipes (jaypipes)]

:)

review: Approve

=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2010-12-23 18:06:00 +0000
+++ nova/api/openstack/__init__.py	2010-12-28 05:16:45 +0000
@@ -93,6 +93,8 @@
             logging.debug("Including admin operations in API.")
             server_members['pause'] = 'POST'
             server_members['unpause'] = 'POST'
+            server_members['suspend'] = 'POST'
+            server_members['resume'] = 'POST'
 
         mapper.resource("server", "servers", controller=servers.Controller(),
                         collection={'detail': 'GET'},

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2010-12-23 18:06:00 +0000
+++ nova/api/openstack/servers.py	2010-12-28 05:16:45 +0000
@@ -46,7 +46,8 @@
         power_state.NOSTATE: 'build',
         power_state.RUNNING: 'active',
         power_state.BLOCKED: 'active',
-        power_state.PAUSED: 'suspended',
+        power_state.SUSPENDED: 'suspended',
+        power_state.PAUSED: 'error',
         power_state.SHUTDOWN: 'active',
         power_state.SHUTOFF: 'active',
         power_state.CRASHED: 'error'}
@@ -182,7 +183,7 @@
             self.compute_api.pause(ctxt, id)
         except:
             readable = traceback.format_exc()
-            logging.error("Compute.api::pause %s", readable)
+            logging.error(_("Compute.api::pause %s"), readable)
             return faults.Fault(exc.HTTPUnprocessableEntity())
         return exc.HTTPAccepted()
 
@@ -193,6 +194,28 @@
             self.compute_api.unpause(ctxt, id)
         except:
             readable = traceback.format_exc()
-            logging.error("Compute.api::unpause %s", readable)
+            logging.error(_("Compute.api::unpause %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
+    def suspend(self, req, id):
+        """permit admins to suspend the server"""
+        context = req.environ['nova.context']
+        try:
+            self.compute_api.suspend(context, id)
+        except:
+            readable = traceback.format_exc()
+            logging.error(_("compute.api::suspend %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
+    def resume(self, req, id):
+        """permit admins to resume the server from suspend"""
+        context = req.environ['nova.context']
+        try:
+            self.compute_api.resume(context, id)
+        except:
+            readable = traceback.format_exc()
+            logging.error(_("compute.api::resume %s"), readable)
             return faults.Fault(exc.HTTPUnprocessableEntity())
         return exc.HTTPAccepted()

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2010-12-23 21:41:54 +0000
+++ nova/compute/api.py	2010-12-28 05:16:45 +0000
@@ -284,6 +284,24 @@
                  {"method": "unpause_instance",
                   "args": {"instance_id": instance['id']}})
 
+    def suspend(self, context, instance_id):
+        """suspend the instance with instance_id"""
+        instance = self.db.instance_get_by_internal_id(context, instance_id)
+        host = instance['host']
+        rpc.cast(context,
+                 self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                 {"method": "suspend_instance",
+                  "args": {"instance_id": instance['id']}})
+
+    def resume(self, context, instance_id):
+        """resume the instance with instance_id"""
+        instance = self.db.instance_get_by_internal_id(context, instance_id)
+        host = instance['host']
+        rpc.cast(context,
+                 self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                 {"method": "resume_instance",
+                  "args": {"instance_id": instance['id']}})
+
     def rescue(self, context, instance_id):
         """Rescue the given instance."""
         instance = self.db.instance_get_by_internal_id(context, instance_id)

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2010-12-22 22:54:13 +0000
+++ nova/compute/manager.py	2010-12-28 05:16:45 +0000
@@ -297,6 +297,39 @@
                                                        result))
 
     @exception.wrap_exception
+    def suspend_instance(self, context, instance_id):
+        """suspend the instance with instance_id"""
+        context = context.elevated()
+        instance_ref = self.db.instance_get(context, instance_id)
+
+        logging.debug(_('instance %s: suspending'),
+                      instance_ref['internal_id'])
+        self.db.instance_set_state(context, instance_id,
+                                            power_state.NOSTATE,
+                                            'suspending')
+        self.driver.suspend(instance_ref,
+            lambda result: self._update_state_callback(self,
+                                                       context,
+                                                       instance_id,
+                                                       result))
+
+    @exception.wrap_exception
+    def resume_instance(self, context, instance_id):
+        """resume the suspended instance with instance_id"""
+        context = context.elevated()
+        instance_ref = self.db.instance_get(context, instance_id)
+
+        logging.debug(_('instance %s: resuming'), instance_ref['internal_id'])
+        self.db.instance_set_state(context, instance_id,
+                                            power_state.NOSTATE,
+                                            'resuming')
+        self.driver.resume(instance_ref,
+            lambda result: self._update_state_callback(self,
+                                                       context,
+                                                       instance_id,
+                                                       result))
+
+    @exception.wrap_exception
     def get_console_output(self, context, instance_id):
         """Send the console output for an instance."""
         context = context.elevated()

=== modified file 'nova/compute/power_state.py'
--- nova/compute/power_state.py	2010-10-22 00:15:21 +0000
+++ nova/compute/power_state.py	2010-12-28 05:16:45 +0000
@@ -26,6 +26,7 @@
 SHUTDOWN = 0x04
 SHUTOFF = 0x05
 CRASHED = 0x06
+SUSPENDED = 0x07
 
 
 def name(code):
@@ -36,5 +37,6 @@
         PAUSED: 'paused',
         SHUTDOWN: 'shutdown',
         SHUTOFF: 'shutdown',
-        CRASHED: 'crashed'}
+        CRASHED: 'crashed',
+        SUSPENDED: 'suspended'}
     return d[code]

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2010-12-23 18:06:00 +0000
+++ nova/tests/api/openstack/test_servers.py	2010-12-28 05:16:45 +0000
@@ -88,9 +88,13 @@
         self.stubs.Set(nova.db.api, 'instance_get_floating_address',
                        instance_address)
         self.stubs.Set(nova.compute.api.ComputeAPI, 'pause',
-            fake_compute_api)
+                       fake_compute_api)
         self.stubs.Set(nova.compute.api.ComputeAPI, 'unpause',
-            fake_compute_api)
+                       fake_compute_api)
+        self.stubs.Set(nova.compute.api.ComputeAPI, 'suspend',
+                       fake_compute_api)
+        self.stubs.Set(nova.compute.api.ComputeAPI, 'resume',
+                       fake_compute_api)
         self.allow_admin = FLAGS.allow_admin_api
 
     def tearDown(self):
@@ -246,6 +250,30 @@
         res = req.get_response(nova.api.API('os'))
         self.assertEqual(res.status_int, 202)
 
+    def test_server_suspend(self):
+        FLAGS.allow_admin_api = True
+        body = dict(server=dict(
+            name='server_test', imageId=2, flavorId=2, metadata={},
+            personality={}))
+        req = webob.Request.blank('/v1.0/servers/1/suspend')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+        res = req.get_response(nova.api.API('os'))
+        self.assertEqual(res.status_int, 202)
+
+    def test_server_resume(self):
+        FLAGS.allow_admin_api = True
+        body = dict(server=dict(
+            name='server_test', imageId=2, flavorId=2, metadata={},
+            personality={}))
+        req = webob.Request.blank('/v1.0/servers/1/resume')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+        res = req.get_response(nova.api.API('os'))
+        self.assertEqual(res.status_int, 202)
+
     def test_server_reboot(self):
         body = dict(server=dict(
             name='server_test', imageId=2, flavorId=2, metadata={},

=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2010-12-23 18:31:46 +0000
+++ nova/tests/test_compute.py	2010-12-28 05:16:45 +0000
@@ -101,13 +101,13 @@
         self.compute.run_instance(self.context, instance_id)
 
         instances = db.instance_get_all(context.get_admin_context())
-        logging.info("Running instances: %s", instances)
+        logging.info(_("Running instances: %s"), instances)
         self.assertEqual(len(instances), 1)
 
         self.compute.terminate_instance(self.context, instance_id)
 
         instances = db.instance_get_all(context.get_admin_context())
-        logging.info("After terminating instances: %s", instances)
+        logging.info(_("After terminating instances: %s"), instances)
         self.assertEqual(len(instances), 0)
 
     def test_run_terminate_timestamps(self):
@@ -136,6 +136,14 @@
         self.compute.unpause_instance(self.context, instance_id)
         self.compute.terminate_instance(self.context, instance_id)
 
+    def test_suspend(self):
+        """ensure instance can be suspended"""
+        instance_id = self._create_instance()
+        self.compute.run_instance(self.context, instance_id)
+        self.compute.suspend_instance(self.context, instance_id)
+        self.compute.resume_instance(self.context, instance_id)
+        self.compute.terminate_instance(self.context, instance_id)
+
     def test_reboot(self):
         """Ensure instance can be rebooted"""
         instance_id = self._create_instance()

=== modified file 'nova/virt/fake.py'
--- nova/virt/fake.py	2010-12-22 20:59:53 +0000
+++ nova/virt/fake.py	2010-12-28 05:16:45 +0000
@@ -148,6 +148,18 @@
         """
         pass
 
+    def suspend(self, instance, callback):
+        """
+        suspend the specified instance
+        """
+        pass
+
+    def resume(self, instance, callback):
+        """
+        resume the specified instance
+        """
+        pass
+
     def destroy(self, instance):
         """
         Destroy (shutdown and delete) the specified instance.

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2010-12-27 10:04:25 +0000
+++ nova/virt/libvirt_conn.py	2010-12-28 05:16:45 +0000
@@ -280,6 +280,14 @@
         raise exception.APIError("unpause not supported for libvirt.")
 
     @exception.wrap_exception
+    def suspend(self, instance, callback):
+        raise exception.APIError("suspend not supported for libvirt")
+
+    @exception.wrap_exception
+    def resume(self, instance, callback):
+        raise exception.APIError("resume not supported for libvirt")
+
+    @exception.wrap_exception
     def rescue(self, instance):
         self.destroy(instance, False)
 

=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2010-12-23 21:41:54 +0000
+++ nova/virt/xenapi/vm_utils.py	2010-12-28 05:16:45 +0000
@@ -39,7 +39,7 @@
     'Halted': power_state.SHUTDOWN,
     'Running': power_state.RUNNING,
     'Paused': power_state.PAUSED,
-    'Suspended': power_state.SHUTDOWN,  # FIXME
+    'Suspended': power_state.SUSPENDED,
     'Crashed': power_state.CRASHED}
 
 
@@ -283,6 +283,10 @@
     @classmethod
     def compile_info(cls, record):
         """Fill record with VM status information"""
+        logging.info(_("(VM_UTILS) xenserver vm state -> |%s|"),
+                     record['power_state'])
+        logging.info(_("(VM_UTILS) xenapi power_state -> |%s|"),
+                     XENAPI_POWER_STATE[record['power_state']])
         return {'state': XENAPI_POWER_STATE[record['power_state']],
                 'max_mem': long(record['memory_static_max']) >> 10,
                 'mem': long(record['memory_dynamic_max']) >> 10,

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2010-12-23 21:41:54 +0000
+++ nova/virt/xenapi/vmops.py	2010-12-28 05:16:45 +0000
@@ -188,6 +188,26 @@
         task = self._session.call_xenapi('Async.VM.unpause', vm)
         self._wait_with_callback(instance.id, task, callback)
 
+    def suspend(self, instance, callback):
+        """suspend the specified instance"""
+        instance_name = instance.name
+        vm = VMHelper.lookup(self._session, instance_name)
+        if vm is None:
+            raise Exception(_("suspend: instance not present %s") %
+                                                     instance_name)
+        task = self._session.call_xenapi('Async.VM.suspend', vm)
+        self._wait_with_callback(task, callback)
+
+    def resume(self, instance, callback):
+        """resume the specified instance"""
+        instance_name = instance.name
+        vm = VMHelper.lookup(self._session, instance_name)
+        if vm is None:
+            raise Exception(_("resume: instance not present %s") %
+                                                    instance_name)
+        task = self._session.call_xenapi('Async.VM.resume', vm, False, True)
+        self._wait_with_callback(task, callback)
+
     def get_info(self, instance_id):
         """Return data about VM instance"""
         vm = VMHelper.lookup(self._session, instance_id)

=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2010-12-23 03:35:41 +0000
+++ nova/virt/xenapi_conn.py	2010-12-28 05:16:45 +0000
@@ -147,6 +147,14 @@
         """Unpause paused VM instance"""
         self._vmops.unpause(instance, callback)
 
+    def suspend(self, instance, callback):
+        """suspend the specified instance"""
+        self._vmops.suspend(instance, callback)
+
+    def resume(self, instance, callback):
+        """resume the specified instance"""
+        self._vmops.resume(instance, callback)
+
     def get_info(self, instance_id):
         """Return data about VM instance"""
         return self._vmops.get_info(instance_id)