Merge "Allow reboot or rebuild from vm_state=Error"

6 files changed, 143 insertions, 14 deletions

diff --git a/nova/api/openstack/common.py b/nova/api/openstack/common.py
index bec919f4b..dd746e23d 100644
--- a/nova/api/openstack/common.py
+++ b/nova/api/openstack/common.py
@@ -358,9 +358,12 @@ def raise_http_conflict_for_instance_invalid_state(exc, action):
     """
     attr = exc.kwargs.get('attr')
     state = exc.kwargs.get('state')
+    not_launched = exc.kwargs.get('not_launched')
     if attr and state:
         msg = _("Cannot '%(action)s' while instance is in %(attr)s "
                 "%(state)s") % {'action': action, 'attr': attr, 'state': state}
+    elif not_launched:
+        msg = _("Cannot '%s' an instance which has never been active") % action
     else:
         # At least give some meaningful message
         msg = _("Instance is in an invalid state for '%s'") % action
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 1e24e8ce5..0a9b0e67b 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -110,10 +110,12 @@ RO_SECURITY_GROUPS = ['default']
 SM_IMAGE_PROP_PREFIX = "image_"
 
 
-def check_instance_state(vm_state=None, task_state=(None,)):
+def check_instance_state(vm_state=None, task_state=(None,),
+                         must_have_launched=True):
     """Decorator to check VM and/or task state before entry to API functions.
 
-    If the instance is in the wrong state, the wrapper will raise an exception.
+    If the instance is in the wrong state, or has not been sucessfully started
+    at least once the wrapper will raise an exception.
     """
 
     if vm_state is not None and not isinstance(vm_state, set):
@@ -137,6 +139,13 @@ def check_instance_state(vm_state=None, task_state=(None,)):
                     instance_uuid=instance['uuid'],
                     state=instance['task_state'],
                     method=f.__name__)
+            if must_have_launched and not instance['launched_at']:
+                raise exception.InstanceInvalidState(
+                    attr=None,
+                    not_launched=True,
+                    instance_uuid=instance['uuid'],
+                    state=instance['vm_state'],
+                    method=f.__name__)
 
             return f(self, context, instance, *args, **kw)
         return inner
@@ -1305,7 +1314,8 @@ class API(base.Base):
     # NOTE(maoy): we allow delete to be called no matter what vm_state says.
     @wrap_check_policy
     @check_instance_lock
-    @check_instance_state(vm_state=None, task_state=None)
+    @check_instance_state(vm_state=None, task_state=None,
+                          must_have_launched=True)
     def soft_delete(self, context, instance):
         """Terminate an instance."""
         LOG.debug(_('Going to try to soft delete instance'),
@@ -1329,7 +1339,8 @@ class API(base.Base):
 
     @wrap_check_policy
     @check_instance_lock
-    @check_instance_state(vm_state=None, task_state=None)
+    @check_instance_state(vm_state=None, task_state=None,
+                          must_have_launched=False)
     def delete(self, context, instance):
         """Terminate an instance."""
         LOG.debug(_("Going to try to terminate instance"), instance=instance)
@@ -1369,7 +1380,8 @@ class API(base.Base):
 
     @wrap_check_policy
     @check_instance_lock
-    @check_instance_state(vm_state=[vm_states.SOFT_DELETED])
+    @check_instance_state(vm_state=[vm_states.SOFT_DELETED],
+                          must_have_launched=False)
     def force_delete(self, context, instance):
         """Force delete a previously deleted (but not reclaimed) instance."""
         self._delete_instance(context, instance)
@@ -1790,7 +1802,8 @@ class API(base.Base):
     @wrap_check_policy
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED,
-                                    vm_states.PAUSED, vm_states.SUSPENDED],
+                                    vm_states.PAUSED, vm_states.SUSPENDED,
+                                    vm_states.ERROR],
                           task_state=[None, task_states.REBOOTING,
                                       task_states.REBOOTING_HARD,
                                       task_states.RESUMING,
@@ -1826,7 +1839,8 @@ class API(base.Base):
 
     @wrap_check_policy
     @check_instance_lock
-    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED],
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED,
+                                    vm_states.ERROR],
                           task_state=[None])
     def rebuild(self, context, instance, image_href, admin_password, **kwargs):
         """Rebuild the given instance with the provided attributes."""
@@ -2224,7 +2238,8 @@ class API(base.Base):
 
     @wrap_check_policy
     @check_instance_lock
-    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED])
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED,
+                                    vm_states.ERROR])
     def rescue(self, context, instance, rescue_password=None):
         """Rescue the given instance."""
 
diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py
index 543bf4a62..22a6947f2 100644
--- a/nova/tests/api/ec2/test_cloud.py
+++ b/nova/tests/api/ec2/test_cloud.py
@@ -46,6 +46,7 @@ from nova.network import api as network_api
 from nova.network import quantumv2
 from nova.openstack.common import log as logging
 from nova.openstack.common import rpc
+from nova.openstack.common import timeutils
 from nova import test
 from nova.tests.api.openstack.compute.contrib import (
     test_quantum_security_groups as test_quantum)
@@ -880,6 +881,7 @@ class CloudTestCase(test.TestCase):
                 'instance_type_id': 1,
                 'host': 'host1',
                 'vm_state': 'active',
+                'launched_at': timeutils.utcnow(),
                 'hostname': 'server-1111',
                 'created_at': datetime.datetime(2012, 5, 1, 1, 1, 1),
                 'system_metadata': sys_meta
@@ -891,6 +893,7 @@ class CloudTestCase(test.TestCase):
                 'instance_type_id': 1,
                 'host': 'host2',
                 'vm_state': 'active',
+                'launched_at': timeutils.utcnow(),
                 'hostname': 'server-1112',
                 'created_at': datetime.datetime(2012, 5, 1, 1, 1, 2),
                 'system_metadata': sys_meta
@@ -2442,6 +2445,7 @@ class CloudTestCase(test.TestCase):
                 'image_ref': image_uuid,
                 'instance_type_id': 1,
                 'vm_state': 'active',
+                'launched_at': timeutils.utcnow(),
                 'hostname': 'server-1111',
                 'created_at': datetime.datetime(2012, 5, 1, 1, 1, 1)
         }
@@ -2492,6 +2496,7 @@ class CloudTestCase(test.TestCase):
                 'image_ref': image_uuid,
                 'instance_type_id': 1,
                 'vm_state': 'active',
+                'launched_at': timeutils.utcnow(),
                 'hostname': 'server-1111',
                 'created_at': datetime.datetime(2012, 5, 1, 1, 1, 1)
         }
@@ -2501,6 +2506,7 @@ class CloudTestCase(test.TestCase):
                 'image_ref': image_uuid,
                 'instance_type_id': 1,
                 'vm_state': 'active',
+                'launched_at': timeutils.utcnow(),
                 'hostname': 'server-1112',
                 'created_at': datetime.datetime(2012, 5, 1, 1, 1, 2)
         }
diff --git a/nova/tests/api/openstack/compute/contrib/test_admin_actions.py b/nova/tests/api/openstack/compute/contrib/test_admin_actions.py
index 5e64bdf94..39eebbcd1 100644
--- a/nova/tests/api/openstack/compute/contrib/test_admin_actions.py
+++ b/nova/tests/api/openstack/compute/contrib/test_admin_actions.py
@@ -26,6 +26,7 @@ from nova.conductor import api as conductor_api
 from nova import context
 from nova import exception
 from nova.openstack.common import jsonutils
+from nova.openstack.common import timeutils
 from nova import test
 from nova.tests.api.openstack import fakes
 
@@ -41,6 +42,7 @@ INSTANCE = {
              "tenant_id": 'fake_tenant_id',
              "created_at": datetime.datetime(2010, 10, 10, 12, 0, 0),
              "updated_at": datetime.datetime(2010, 11, 11, 11, 0, 0),
+             "launched_at": datetime.datetime(2010, 11, 11, 11, 0, 0),
              "security_groups": [{"id": 1, "name": "test"}],
              "progress": 0,
              "image_ref": 'http://foo.com/123',
@@ -61,7 +63,7 @@ def fake_compute_api_raises_invalid_state(*args, **kwargs):
 
 def fake_compute_api_get(self, context, instance_id):
     return {'id': 1, 'uuid': instance_id, 'vm_state': vm_states.ACTIVE,
-            'task_state': None}
+            'task_state': None, 'launched_at': timeutils.utcnow()}
 
 
 class AdminActionsTest(test.TestCase):
diff --git a/nova/tests/api/openstack/compute/test_server_metadata.py b/nova/tests/api/openstack/compute/test_server_metadata.py
index fa25ad4a3..f0548ffa0 100644
--- a/nova/tests/api/openstack/compute/test_server_metadata.py
+++ b/nova/tests/api/openstack/compute/test_server_metadata.py
@@ -26,6 +26,7 @@ from nova.compute import vm_states
 import nova.db
 from nova import exception
 from nova.openstack.common import jsonutils
+from nova.openstack.common import timeutils
 from nova import test
 from nova.tests.api.openstack import fakes
 
@@ -77,6 +78,7 @@ def return_server(context, server_id):
             'uuid': '0cc3346e-9fef-4445-abe6-5d2b2690ec64',
             'name': 'fake',
             'locked': False,
+            'launched_at': timeutils.utcnow(),
             'vm_state': vm_states.ACTIVE}
 
 
@@ -85,6 +87,7 @@ def return_server_by_uuid(context, server_uuid):
             'uuid': '0cc3346e-9fef-4445-abe6-5d2b2690ec64',
             'name': 'fake',
             'locked': False,
+            'launched_at': timeutils.utcnow(),
             'vm_state': vm_states.ACTIVE}
 
 
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 928934f61..c887041f8 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -259,6 +259,9 @@ class BaseTestCase(test.TestCase):
         inst['os_type'] = 'Linux'
         inst['system_metadata'] = make_fake_sys_meta()
         inst['locked'] = False
+        inst['created_at'] = timeutils.utcnow()
+        inst['updated_at'] = timeutils.utcnow()
+        inst['launched_at'] = timeutils.utcnow()
         inst.update(params)
         _create_service_entries(self.context.elevated(),
                 {'fake_zone': [inst['host']]})
@@ -1248,6 +1251,7 @@ class ComputeTestCase(BaseTestCase):
     def test_run_terminate_timestamps(self):
         # Make sure timestamps are set for launched and destroyed.
         instance = jsonutils.to_primitive(self._create_fake_instance())
+        instance['launched_at'] = None
         self.assertEqual(instance['launched_at'], None)
         self.assertEqual(instance['deleted_at'], None)
         launch = timeutils.utcnow()
@@ -5722,6 +5726,21 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, instance['uuid'])
 
+    def test_delete_if_not_launched(self):
+        instance, instance_uuid = self._run_instance(params={
+                'host': CONF.host})
+
+        db.instance_update(self.context, instance['uuid'],
+                           {"vm_state": vm_states.ERROR,
+                            "launched_at": None})
+
+        self.compute_api.delete(self.context, instance)
+
+        instance = db.instance_get_by_uuid(self.context, instance_uuid)
+        self.assertEqual(instance['task_state'], task_states.DELETING)
+
+        db.instance_destroy(self.context, instance['uuid'])
+
     def test_delete_in_resizing(self):
         def fake_quotas_reserve(context, expire=None, project_id=None,
                                                              **deltas):
@@ -5996,7 +6015,7 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, instance['uuid'])
 
-    def test_rebuild(self):
+    def _test_rebuild(self, vm_state):
         instance = jsonutils.to_primitive(self._create_fake_instance())
         instance_uuid = instance['uuid']
         self.compute.run_instance(self.context, instance=instance)
@@ -6027,6 +6046,10 @@ class ComputeAPITestCase(BaseTestCase):
 
         image_ref = instance["image_ref"] + '-new_image_ref'
         password = "new_password"
+
+        db.instance_update(self.context, instance['uuid'],
+                           {"vm_state": vm_state})
+
         self.compute_api.rebuild(self.context, instance, image_ref, password)
         self.assertEqual(info['image_ref'], image_ref)
 
@@ -6041,6 +6064,34 @@ class ComputeAPITestCase(BaseTestCase):
                 'preserved': 'preserve this!'})
         db.instance_destroy(self.context, instance['uuid'])
 
+    def test_rebuild(self):
+        # Test we can rebuild an instance in the Error State
+        self._test_rebuild(vm_state=vm_states.ACTIVE)
+
+    def test_rebuild_in_error_state(self):
+        # Test we can rebuild an instance in the Error State
+        self._test_rebuild(vm_state=vm_states.ERROR)
+
+    def test_rebuild_in_error_not_launched(self):
+        instance = jsonutils.to_primitive(
+            self._create_fake_instance(params={'image_ref': ''}))
+        instance_uuid = instance['uuid']
+        self.stubs.Set(fake_image._FakeImageService, 'show', self.fake_show)
+        self.compute.run_instance(self.context, instance=instance)
+
+        db.instance_update(self.context, instance['uuid'],
+                           {"vm_state": vm_states.ERROR,
+                            "launched_at": None})
+
+        instance = db.instance_get_by_uuid(self.context, instance['uuid'])
+
+        self.assertRaises(exception.InstanceInvalidState,
+                          self.compute_api.rebuild,
+                          self.context,
+                          instance,
+                          instance['image_ref'],
+                          "new password")
+
     def test_rebuild_no_image(self):
         instance = jsonutils.to_primitive(
             self._create_fake_instance(params={'image_ref': ''}))
@@ -6187,7 +6238,7 @@ class ComputeAPITestCase(BaseTestCase):
         self.stubs.Set(nova.virt.fake.FakeDriver, 'legacy_nwinfo',
                        lambda x: False)
 
-    def test_reboot_soft(self):
+    def _test_reboot_soft(self, vm_state):
         # Ensure instance can be soft rebooted.
         instance = jsonutils.to_primitive(self._create_fake_instance())
         self.compute.run_instance(self.context, instance=instance)
@@ -6204,6 +6255,9 @@ class ComputeAPITestCase(BaseTestCase):
         inst_ref = db.instance_get_by_uuid(self.context, instance['uuid'])
         self.assertEqual(inst_ref['task_state'], None)
 
+        db.instance_update(self.context, instance['uuid'],
+                           {"vm_state": vm_state})
+
         reboot_type = "SOFT"
         self._stub_out_reboot(device_name)
         self.compute_api.reboot(self.context, inst_ref, reboot_type)
@@ -6213,7 +6267,15 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, inst_ref['uuid'])
 
-    def test_reboot_hard(self):
+    def test_soft_reboot(self):
+        # Ensure instance can be rebooted while in error state.
+        self._test_reboot_soft(vm_state=vm_states.ACTIVE)
+
+    def test_soft_reboot_of_instance_in_error(self):
+        # Ensure instance can be rebooted while in error state.
+        self._test_reboot_soft(vm_state=vm_states.ERROR)
+
+    def test_reboot_hard(self, vm_state=vm_states.ACTIVE):
         # Ensure instance can be hard rebooted.
         instance = jsonutils.to_primitive(self._create_fake_instance())
         self.compute.run_instance(self.context, instance=instance)
@@ -6230,6 +6292,9 @@ class ComputeAPITestCase(BaseTestCase):
         inst_ref = db.instance_get_by_uuid(self.context, instance['uuid'])
         self.assertEqual(inst_ref['task_state'], None)
 
+        db.instance_update(self.context, instance['uuid'],
+                           {"vm_state": vm_state})
+
         reboot_type = "HARD"
         self._stub_out_reboot(device_name)
         self.compute_api.reboot(self.context, inst_ref, reboot_type)
@@ -6239,6 +6304,10 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, inst_ref['uuid'])
 
+    def test_hard_reboot_of_instance_in_error(self):
+        # Ensure instance can be rebooted while in error state.
+        self.test_reboot_hard(vm_state=vm_states.ERROR)
+
     def test_hard_reboot_of_soft_rebooting_instance(self):
         # Ensure instance can be hard rebooted while soft rebooting.
         instance = jsonutils.to_primitive(self._create_fake_instance())
@@ -6275,7 +6344,7 @@ class ComputeAPITestCase(BaseTestCase):
                           inst_ref,
                           reboot_type)
 
-    def test_soft_reboot_of_rescued_instance(self):
+    def test_reboot_of_rescued_instance(self):
         # Ensure instance can't be rebooted while in rescued state.
         instance = jsonutils.to_primitive(self._create_fake_instance())
         self.compute.run_instance(self.context, instance=instance)
@@ -6299,6 +6368,33 @@ class ComputeAPITestCase(BaseTestCase):
                           inst_ref,
                           'HARD')
 
+    def test_reboot_of_instance_in_error_not_launched(self):
+        # Ensure instance can be not rebooted while in error states
+        # if they have never been booted at least once.
+        instance = jsonutils.to_primitive(self._create_fake_instance())
+        self.compute.run_instance(self.context, instance=instance)
+
+        inst_ref = db.instance_get_by_uuid(self.context, instance['uuid'])
+        self.assertEqual(inst_ref['task_state'], None)
+
+        db.instance_update(self.context, instance['uuid'],
+                           {"vm_state": vm_states.ERROR,
+                            "launched_at": None})
+
+        inst_ref = db.instance_get_by_uuid(self.context, inst_ref['uuid'])
+
+        self.assertRaises(exception.InstanceInvalidState,
+                          self.compute_api.reboot,
+                          self.context,
+                          inst_ref,
+                          'SOFT')
+
+        self.assertRaises(exception.InstanceInvalidState,
+                          self.compute_api.reboot,
+                          self.context,
+                          inst_ref,
+                          'HARD')
+
     def test_hostname_create(self):
         # Ensure instance hostname is set during creation.
         inst_type = flavors.get_flavor_by_name('m1.tiny')
@@ -7738,7 +7834,8 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertRaises(exception.InvalidDevicePath,
                 self.compute_api.attach_volume,
                 self.context,
-                {'locked': False, 'vm_state': vm_states.ACTIVE},
+                {'locked': False, 'vm_state': vm_states.ACTIVE,
+                 'launched_at': timeutils.utcnow()},
                 None,
                 '/invalid')
 
@@ -8045,6 +8142,7 @@ class ComputeAPITestCase(BaseTestCase):
         # Ensure exception is raised while detaching an un-attached volume
         instance = {'uuid': 'uuid1',
                     'locked': False,
+                    'launched_at': timeutils.utcnow(),
                     'vm_state': vm_states.ACTIVE}
         volume = {'id': 1, 'attach_status': 'detached'}
 
@@ -8057,6 +8155,7 @@ class ComputeAPITestCase(BaseTestCase):
         # instance doesn't match.
         instance = {'uuid': 'uuid1',
                     'locked': False,
+                    'launched_at': timeutils.utcnow(),
                     'vm_state': vm_states.ACTIVE}
         volume = {'id': 1, 'attach_status': 'in-use',
                   'instance_uuid': 'uuid2'}
@@ -8371,6 +8470,7 @@ class ComputeAPITestCase(BaseTestCase):
     def test_fail_evacuate_from_non_existing_host(self):
         inst = {}
         inst['vm_state'] = vm_states.ACTIVE
+        inst['launched_at'] = timeutils.utcnow()
         inst['image_ref'] = FAKE_IMAGE_REF
         inst['reservation_id'] = 'r-fakeres'
         inst['user_id'] = self.user_id