From 99c51e34230394cadf0b82e364ea10c38e193979 Mon Sep 17 00:00:00 2001 From: Phil Day Date: Fri, 7 Jun 2013 15:18:03 +0100 Subject: Allow reboot or rebuild from vm_state=Error In general most operations are only valid on an instance that has booted successfully at least once so this change extends the instance state check logic to include evidence that the instance has been successfully started at least once. This enables more operations to be allowed in against instances in an Error state. For example reboot and rebuild are useful recover options for an instance which has reached an error state, but not if the instance failed during its initial build. With this change the only actions allowed on an instance which has never booted successfully are delete and force_delete. Soft delete is not allowed, as the restore is in effect a start unless there is specific virt driver support. In addition the following actions are now allowed against instances in an Error state providing the instance has booted at least once: Reboot, Rebuild, and Rescue. Fixes bug: 1183946 Change-Id: I63fd8d2a182df5cf12754892e8076933b3b1e79f --- nova/api/openstack/common.py | 3 + nova/compute/api.py | 31 ++++-- nova/tests/api/ec2/test_cloud.py | 6 ++ .../compute/contrib/test_admin_actions.py | 4 +- .../api/openstack/compute/test_server_metadata.py | 3 + nova/tests/compute/test_compute.py | 110 ++++++++++++++++++++- 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 bf6508803..b6695c510 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() @@ -5710,6 +5714,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): @@ -5984,7 +6003,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) @@ -6015,6 +6034,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) @@ -6029,6 +6052,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': ''})) @@ -6175,7 +6226,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) @@ -6192,6 +6243,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) @@ -6201,7 +6255,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) @@ -6218,6 +6280,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) @@ -6227,6 +6292,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()) @@ -6263,7 +6332,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) @@ -6287,6 +6356,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') @@ -7689,7 +7785,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') @@ -7996,6 +8093,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'} @@ -8008,6 +8106,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'} @@ -8322,6 +8421,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 -- cgit