Reset instance to ACTIVE when no hosts found

bug 928521

modified nova/scheduler/manager.py to reset vm_state to ACTIVE and set
task_state to None when prep_resize raises a NoHostsFound

refactored run_instance and prep_resize so they don't go through
_schedule and now must be implemented in driver

Changed behavior to set vm_state to error on any other exception in
prep_resize.

Change behavior to change instance vm_state to ERROR on exceptions

Added tests that the vm_state gets updated

Added tests that schedule_prep_resize and schedule_run_instance
have no implementation in the Driver base class

Had to adjust methods and tests for Multi scheduler to reflect the
new Scheduler contract

Change-Id: Ibcac7ef0df3456793a2132beb7a711849510da80

5 files changed, 226 insertions, 51 deletions

diff --git a/nova/scheduler/driver.py b/nova/scheduler/driver.py
index 5ee4ed343..74b5c308b 100644
--- a/nova/scheduler/driver.py
+++ b/nova/scheduler/driver.py
@@ -177,9 +177,19 @@ class Scheduler(object):
         return instance
 
     def schedule(self, context, topic, method, *_args, **_kwargs):
-        """Must override at least this method for scheduler to work."""
+        """Must override schedule method for scheduler to work."""
         raise NotImplementedError(_("Must implement a fallback schedule"))
 
+    def schedule_prep_resize(self, context, request_spec, *_args, **_kwargs):
+        """Must override schedule_prep_resize method for scheduler to work."""
+        msg = _("Driver must implement schedule_prep_resize")
+        raise NotImplementedError(msg)
+
+    def schedule_run_instance(self, context, request_spec, *_args, **_kwargs):
+        """Must override schedule_run_instance method for scheduler to work."""
+        msg = _("Driver must implement schedule_run_instance")
+        raise NotImplementedError(msg)
+
     def schedule_live_migration(self, context, instance_id, dest,
                                 block_migration=False,
                                 disk_over_commit=False):
diff --git a/nova/scheduler/manager.py b/nova/scheduler/manager.py
index 0390f5ced..57bd445ff 100644
--- a/nova/scheduler/manager.py
+++ b/nova/scheduler/manager.py
@@ -75,62 +75,104 @@ class SchedulerManager(manager.Manager):
 
     def _schedule(self, method, context, topic, *args, **kwargs):
         """Tries to call schedule_* method on the driver to retrieve host.
-
         Falls back to schedule(context, topic) if method doesn't exist.
         """
-        driver_method = 'schedule_%s' % method
+        driver_method_name = 'schedule_%s' % method
         try:
-            real_meth = getattr(self.driver, driver_method)
+            driver_method = getattr(self.driver, driver_method_name)
             args = (context,) + args
         except AttributeError, e:
-            LOG.warning(_("Driver Method %(driver_method)s missing: %(e)s."
-                          "Reverting to schedule()") % locals())
-            real_meth = self.driver.schedule
+            LOG.warning(_("Driver Method %(driver_method_name)s missing: "
+                       "%(e)s. Reverting to schedule()") % locals())
+            driver_method = self.driver.schedule
             args = (context, topic, method) + args
 
         # Scheduler methods are responsible for casting.
         try:
-            return real_meth(*args, **kwargs)
+            return driver_method(*args, **kwargs)
+        except Exception as ex:
+            with utils.save_and_reraise_exception():
+                self._set_vm_state_and_notify(method,
+                                             {'vm_state': vm_states.ERROR},
+                                             context, ex, *args, **kwargs)
+
+    def run_instance(self, context, topic, *args, **kwargs):
+        """Tries to call schedule_run_instance on the driver.
+        Sets instance vm_state to ERROR on exceptions
+        """
+        args = (context,) + args
+        try:
+            return self.driver.schedule_run_instance(*args, **kwargs)
+        except exception.NoValidHost as ex:
+            # don't reraise
+            self._set_vm_state_and_notify('run_instance',
+                                         {'vm_state': vm_states.ERROR},
+                                          context, ex, *args, **kwargs)
+        except Exception as ex:
+            with utils.save_and_reraise_exception():
+                self._set_vm_state_and_notify('run_instance',
+                                             {'vm_state': vm_states.ERROR},
+                                             context, ex, *args, **kwargs)
+
+    def prep_resize(self, context, topic, *args, **kwargs):
+        """Tries to call schedule_prep_resize on the driver.
+        Sets instance vm_state to ACTIVE on NoHostFound
+        Sets vm_state to ERROR on other exceptions
+        """
+        args = (context,) + args
+        try:
+            return self.driver.schedule_prep_resize(*args, **kwargs)
         except exception.NoValidHost as ex:
-            self._set_instance_error(method, context, ex, *args, **kwargs)
+            self._set_vm_state_and_notify('prep_resize',
+                                         {'vm_state': vm_states.ACTIVE,
+                                          'task_state': None},
+                                         context, ex, *args, **kwargs)
         except Exception as ex:
             with utils.save_and_reraise_exception():
-                self._set_instance_error(method, context, ex, *args, **kwargs)
+                self._set_vm_state_and_notify('prep_resize',
+                                             {'vm_state': vm_states.ERROR},
+                                             context, ex, *args, **kwargs)
+
+    def _set_vm_state_and_notify(self, method, updates, context, ex,
+                                *args, **kwargs):
+        """changes VM state and notifies"""
+        # FIXME(comstud): Re-factor this somehow. Not sure this belongs in the
+        # scheduler manager like this. We should make this easier.
+        # run_instance only sends a request_spec, and an instance may or may
+        # not have been created in the API (or scheduler) already. If it was
+        # created, there's a 'uuid' set in the instance_properties of the
+        # request_spec.
+        # (littleidea): I refactored this a bit, and I agree
+        # it should be easier :)
+        # The refactoring could go further but trying to minimize changes
+        # for essex timeframe
 
-    def _set_instance_error(self, method, context, ex, *args, **kwargs):
-        """Sets VM to Error state"""
         LOG.warning(_("Failed to schedule_%(method)s: %(ex)s") % locals())
-        # FIXME(comstud): Re-factor this somehow.  Not sure this belongs
-        # in the scheduler manager like this.  Needs to support more than
-        # run_instance
-        if method != "run_instance":
-            return
-        # FIXME(comstud): We should make this easier.  run_instance
-        # only sends a request_spec, and an instance may or may not
-        # have been created in the API (or scheduler) already.  If it
-        # was created, there's a 'uuid' set in the instance_properties
-        # of the request_spec.
+
+        vm_state = updates['vm_state']
         request_spec = kwargs.get('request_spec', {})
         properties = request_spec.get('instance_properties', {})
         instance_uuid = properties.get('uuid', {})
+
         if instance_uuid:
-            LOG.warning(_("Setting instance %(instance_uuid)s to "
-                    "ERROR state.") % locals())
-            db.instance_update(context, instance_uuid,
-                    {'vm_state': vm_states.ERROR})
+            state = vm_state.upper()
+            msg = _("Setting instance %(instance_uuid)s to %(state)s state.")
+            LOG.warning(msg % locals())
+            db.instance_update(context, instance_uuid, updates)
 
         payload = dict(request_spec=request_spec,
                        instance_properties=properties,
                        instance_id=instance_uuid,
-                       state=vm_states.ERROR,
+                       state=vm_state,
                        method=method,
                        reason=ex)
+
         notifier.notify(notifier.publisher_id("scheduler"),
-                        'scheduler.run_instance', notifier.ERROR, payload)
+                        'scheduler.' + method, notifier.ERROR, payload)
 
     # NOTE (masumotok) : This method should be moved to nova.api.ec2.admin.
-    #                    Based on bexar design summit discussion,
-    #                    just put this here for bexar release.
+    # Based on bexar design summit discussion,
+    # just put this here for bexar release.
     def show_host_resources(self, context, host):
         """Shows the physical/usage resource given by hosts.
 
diff --git a/nova/scheduler/multi.py b/nova/scheduler/multi.py
index 3bec060ab..d29370ffd 100644
--- a/nova/scheduler/multi.py
+++ b/nova/scheduler/multi.py
@@ -41,10 +41,8 @@ FLAGS = flags.FLAGS
 FLAGS.register_opts(multi_scheduler_opts)
 
 # A mapping of methods to topics so we can figure out which driver to use.
-_METHOD_MAP = {'run_instance': 'compute',
-               'prep_resize': 'compute',
-               'live_migration': 'compute',
-               'create_volume': 'volume',
+# There are currently no compute methods proxied through the map
+_METHOD_MAP = {'create_volume': 'volume',
                'create_volumes': 'volume'}
 
 
@@ -75,3 +73,9 @@ class MultiScheduler(driver.Scheduler):
     def schedule(self, context, topic, method, *_args, **_kwargs):
         return self.drivers[topic].schedule(context, topic,
                 method, *_args, **_kwargs)
+
+    def schedule_run_instance(self, *args, **kwargs):
+        return self.drivers['compute'].schedule_run_instance(*args, **kwargs)
+
+    def schedule_prep_resize(self, *args, **kwargs):
+        return self.drivers['compute'].schedule_prep_resize(*args, **kwargs)
diff --git a/nova/tests/scheduler/test_multi_scheduler.py b/nova/tests/scheduler/test_multi_scheduler.py
index 7468a2447..3d7220c2d 100644
--- a/nova/tests/scheduler/test_multi_scheduler.py
+++ b/nova/tests/scheduler/test_multi_scheduler.py
@@ -28,13 +28,7 @@ from nova.tests.scheduler import test_scheduler
 class FakeComputeScheduler(driver.Scheduler):
     is_fake_compute = True
 
-    def schedule_run_instance(self, *args, **kwargs):
-        pass
-
-    def schedule_live_migration(self, *args, **kwargs):
-        pass
-
-    def schedule_prep_resize(self, *args, **kwargs):
+    def schedule_theoretical(self, *args, **kwargs):
         pass
 
     def schedule(self, *args, **kwargs):
@@ -79,7 +73,8 @@ class MultiDriverTestCase(test_scheduler.SchedulerTestCase):
         compute_driver = mgr.drivers['compute']
         volume_driver = mgr.drivers['volume']
 
-        test_methods = {compute_driver: ['run_instance', 'prep_resize'],
+        #no compute methods are proxied at this time
+        test_methods = {compute_driver: [],
                         volume_driver: ['create_volume', 'create_volumes']}
 
         for driver, methods in test_methods.iteritems():
diff --git a/nova/tests/scheduler/test_scheduler.py b/nova/tests/scheduler/test_scheduler.py
index 67f607b07..ccf9d38d9 100644
--- a/nova/tests/scheduler/test_scheduler.py
+++ b/nova/tests/scheduler/test_scheduler.py
@@ -29,6 +29,7 @@ from nova import context
 from nova import db
 from nova import exception
 from nova import flags
+from nova.notifier import api as notifier
 from nova import rpc
 from nova.rpc import common as rpc_common
 from nova.scheduler import driver
@@ -47,6 +48,9 @@ class SchedulerManagerTestCase(test.TestCase):
     driver_cls = driver.Scheduler
     driver_cls_name = 'nova.scheduler.driver.Scheduler'
 
+    class AnException(Exception):
+        pass
+
     def setUp(self):
         super(SchedulerManagerTestCase, self).setUp()
         self.flags(scheduler_driver=self.driver_cls_name)
@@ -183,21 +187,55 @@ class SchedulerManagerTestCase(test.TestCase):
                                  'memory_mb_used': 512}}
         self.assertDictMatch(result, expected)
 
-    def test_run_instance_exception_puts_instance_in_error_state(self):
-        """Test that a NoValidHost exception for run_instance puts
-        the instance in ERROR state and eats the exception.
-        """
-
-        fake_instance_uuid = 'fake-instance-id'
-
+    def _mox_schedule_method_helper(self, method_name):
         # Make sure the method exists that we're going to test call
         def stub_method(*args, **kwargs):
             pass
 
-        setattr(self.manager.driver, 'schedule_run_instance', stub_method)
+        setattr(self.manager.driver, method_name, stub_method)
 
         self.mox.StubOutWithMock(self.manager.driver,
-                'schedule_run_instance')
+                method_name)
+
+    def test_schedule_exeception_changes_state_notifies_and_raises(self):
+        """Test that an exception scheduling calls
+        _set_vm_state_and_notify and reraises
+        """
+        fake_instance_uuid = 'fake-instance-id'
+
+        self._mox_schedule_method_helper('schedule_something')
+
+        self.mox.StubOutWithMock(self.manager, '_set_vm_state_and_notify')
+
+        request_spec = {'instance_properties':
+                {'uuid': fake_instance_uuid}}
+        self.fake_kwargs['request_spec'] = request_spec
+
+        ex = self.AnException('something happened')
+        self.manager.driver.schedule_something(self.context,
+                *self.fake_args, **self.fake_kwargs).AndRaise(ex)
+
+        # Adding the context to the args is kind of gnarly, but thats what
+        # happens. Could be refactored to keep all the context, spec, topic
+        # stuff a bit cleaner.
+        self.manager._set_vm_state_and_notify('something',
+                {'vm_state': vm_states.ERROR}, self.context,
+                ex, *((self.context,) + self.fake_args), **self.fake_kwargs)
+
+        self.mox.ReplayAll()
+
+        self.assertRaises(self.AnException, self.manager.something,
+                         self.context, self.topic,
+                         *self.fake_args, **self.fake_kwargs)
+
+    def test_run_instance_exception_puts_instance_in_error_state(self):
+        """Test that an NoValidHost exception for run_instance puts
+        the instance in ERROR state and eats the exception.
+        """
+
+        fake_instance_uuid = 'fake-instance-id'
+
+        self._mox_schedule_method_helper('schedule_run_instance')
         self.mox.StubOutWithMock(db, 'instance_update')
 
         request_spec = {'instance_properties':
@@ -214,6 +252,57 @@ class SchedulerManagerTestCase(test.TestCase):
         self.manager.run_instance(self.context, self.topic,
                 *self.fake_args, **self.fake_kwargs)
 
+    def test_prep_resize_no_valid_host_back_in_active_state(self):
+        """Test that a NoValidHost exception for prep_resize puts
+        the instance in ACTIVE state
+        """
+        fake_instance_uuid = 'fake-instance-id'
+
+        self._mox_schedule_method_helper('schedule_prep_resize')
+
+        self.mox.StubOutWithMock(db, 'instance_update')
+
+        request_spec = {'instance_properties':
+                {'uuid': fake_instance_uuid}}
+        self.fake_kwargs['request_spec'] = request_spec
+
+        self.manager.driver.schedule_prep_resize(self.context,
+                *self.fake_args, **self.fake_kwargs).AndRaise(
+                        exception.NoValidHost(reason=""))
+        db.instance_update(self.context, fake_instance_uuid,
+                {'vm_state': vm_states.ACTIVE,
+                 'task_state': None})
+
+        self.mox.ReplayAll()
+        self.manager.prep_resize(self.context, self.topic,
+                *self.fake_args, **self.fake_kwargs)
+
+    def test_prep_resize_exception_host_in_error_state_and_raise(self):
+        """Test that a NoValidHost exception for prep_resize puts
+        the instance in ACTIVE state
+        """
+        fake_instance_uuid = 'fake-instance-id'
+
+        self._mox_schedule_method_helper('schedule_prep_resize')
+
+        self.mox.StubOutWithMock(db, 'instance_update')
+
+        request_spec = {'instance_properties':
+                {'uuid': fake_instance_uuid}}
+        self.fake_kwargs['request_spec'] = request_spec
+
+        self.manager.driver.schedule_prep_resize(self.context,
+                *self.fake_args, **self.fake_kwargs).AndRaise(
+                self.AnException('something happened'))
+        db.instance_update(self.context, fake_instance_uuid,
+                {'vm_state': vm_states.ERROR})
+
+        self.mox.ReplayAll()
+
+        self.assertRaises(self.AnException, self.manager.prep_resize,
+                         self.context, self.topic,
+                         *self.fake_args, **self.fake_kwargs)
+
 
 class SchedulerTestCase(test.TestCase):
     """Test case for base scheduler driver class"""
@@ -916,6 +1005,41 @@ class SchedulerTestCase(test.TestCase):
                 block_migration=block_migration)
 
 
+class SchedulerDriverBaseTestCase(SchedulerTestCase):
+    """Test cases for base scheduler driver class methods
+       that can't will fail if the driver is changed"""
+
+    def test_unimplemented_schedule(self):
+        fake_args = (1, 2, 3)
+        fake_kwargs = {'cat': 'meow'}
+
+        self.assertRaises(NotImplementedError, self.driver.schedule,
+                         self.context, self.topic, 'schedule_something',
+                         *fake_args, **fake_kwargs)
+
+    def test_unimplemented_schedule_run_instance(self):
+        fake_args = (1, 2, 3)
+        fake_kwargs = {'cat': 'meow'}
+        fake_request_spec = {'instance_properties':
+                {'uuid': 'uuid'}}
+
+        self.assertRaises(NotImplementedError,
+                         self.driver.schedule_run_instance,
+                         self.context, fake_request_spec,
+                         *fake_args, **fake_kwargs)
+
+    def test_unimplemented_schedule_prep_resize(self):
+        fake_args = (1, 2, 3)
+        fake_kwargs = {'cat': 'meow'}
+        fake_request_spec = {'instance_properties':
+                {'uuid': 'uuid'}}
+
+        self.assertRaises(NotImplementedError,
+                         self.driver.schedule_prep_resize,
+                         self.context, fake_request_spec,
+                         *fake_args, **fake_kwargs)
+
+
 class SchedulerDriverModuleTestCase(test.TestCase):
     """Test case for scheduler driver module methods"""