diff --git a/etc/nova/nova.conf.sample b/etc/nova/nova.conf.sample index 93ae85e24c..35c4723c63 100644 --- a/etc/nova/nova.conf.sample +++ b/etc/nova/nova.conf.sample @@ -687,6 +687,10 @@ # (integer value) #network_allocate_retries=0 +# The number of times to attempt to reap an instance's files. +# (integer value) +#maximum_instance_delete_attempts=5 + # interval to pull bandwidth usage info (integer value) #bandwidth_poll_interval=600 @@ -723,6 +727,10 @@ # shelved (integer value) #shelved_offload_time=0 +# Interval in seconds for retrying failed instance file +# deletes (integer value) +#instance_delete_interval=300 + # Action to take if a running deleted instance is # detected.Valid options are 'noop', 'log' and 'reap'. Set to # 'noop' to disable. (string value) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 49f007010d..a38a3cba42 100755 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -148,6 +148,10 @@ interval_opts = [ help='Time in seconds before a shelved instance is eligible ' 'for removing from a host. -1 never offload, 0 offload ' 'when shelved'), + cfg.IntOpt('instance_delete_interval', + default=300, + help=('Interval in seconds for retrying failed instance file ' + 'deletes')) ] timeout_opts = [ @@ -187,11 +191,19 @@ running_deleted_opts = [ "instance should be considered eligible for cleanup."), ] +instance_cleaning_opts = [ + cfg.IntOpt('maximum_instance_delete_attempts', + default=5, + help=('The number of times to attempt to reap an instance\'s ' + 'files.')), +] + CONF = cfg.CONF CONF.register_opts(compute_opts) CONF.register_opts(interval_opts) CONF.register_opts(timeout_opts) CONF.register_opts(running_deleted_opts) +CONF.register_opts(instance_cleaning_opts) CONF.import_opt('allow_resize_to_same_host', 'nova.compute.api') CONF.import_opt('console_topic', 'nova.console.rpcapi') CONF.import_opt('host', 'nova.netconf') @@ -4799,3 +4811,35 @@ class ComputeManager(manager.SchedulerDependentManager): context, filters, columns_to_join=[]) self.driver.manage_image_cache(context, filtered_instances) + + @periodic_task.periodic_task(spacing=CONF.instance_delete_interval) + def _run_pending_deletes(self, context): + """Retry any pending instance file deletes.""" + if CONF.instance_delete_interval == 0: + return + + LOG.debug(_('Cleaning up deleted instances')) + filters = {'deleted': True, + 'host': CONF.host, + 'cleaned': False} + attrs = ['info_cache', 'security_groups', 'system_metadata'] + with utils.temporary_mutation(context, read_deleted='yes'): + instances = instance_obj.InstanceList.get_by_filters( + context, filters, expected_attrs=attrs) + LOG.debug(_('There are %d instances to clean'), len(instances)) + + for instance in instances: + attempts = int(instance.system_metadata.get('clean_attempts', '0')) + LOG.debug(_('Instance has had %(attempts)s of %(max)s ' + 'cleanup attempts'), + {'attempts': attempts, + 'max': CONF.maximum_instance_delete_attempts}, + instance=instance) + if attempts < CONF.maximum_instance_delete_attempts: + success = self.driver.delete_instance_files(instance) + + instance.system_metadata['clean_attempts'] = str(attempts + 1) + if success: + instance.cleaned = True + with utils.temporary_mutation(context, read_deleted='yes'): + instance.save(context) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 6e89059c34..e8a9cfcc12 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -1777,6 +1777,12 @@ def instance_get_all_by_filters(context, filters, sort_key, sort_dir, query_prefix = query_prefix.\ filter(models.Instance.vm_state != vm_states.SOFT_DELETED) + if 'cleaned' in filters: + if filters.pop('cleaned'): + query_prefix = query_prefix.filter(models.Instance.cleaned == 1) + else: + query_prefix = query_prefix.filter(models.Instance.cleaned == 0) + if not context.is_admin: # If we're not admin context, add appropriate filter.. if context.project_id: diff --git a/nova/db/sqlalchemy/migrate_repo/versions/206_add_instance_cleaned.py b/nova/db/sqlalchemy/migrate_repo/versions/206_add_instance_cleaned.py new file mode 100644 index 0000000000..6245ef2692 --- /dev/null +++ b/nova/db/sqlalchemy/migrate_repo/versions/206_add_instance_cleaned.py @@ -0,0 +1,62 @@ +# Copyright 2013 Rackspace Australia +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from sqlalchemy import Column, Index, Integer, MetaData, Table + + +def upgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + instances = Table('instances', meta, autoload=True) + shadow_instances = Table('shadow_instances', meta, autoload=True) + + cleaned_column = Column('cleaned', Integer, default=0) + instances.create_column(cleaned_column) + shadow_instances.create_column(cleaned_column.copy()) + + cleaned_index = Index('instances_host_deleted_cleaned_idx', + instances.c.host, instances.c.deleted, + instances.c.cleaned) + cleaned_index.create(migrate_engine) + + migrate_engine.execute(instances.update(). + where(instances.c.deleted > 0). + values(cleaned=1)) + migrate_engine.execute(shadow_instances.update(). + where(shadow_instances.c.deleted > 0). + values(cleaned=1)) + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + instances = Table('instances', meta, autoload=True) + instances.columns.cleaned.drop() + + # NOTE(mikal): some engines remove this index when the column is + # removed (sqlite for example). Instead of hardcoding that + # behaviour we just try to remove the index and ignore the failure + # if there is one. + try: + instances = Table('instances', meta, autoload=True) + cleaned_index = Index('instances_host_deleted_cleaned_idx', + instances.c.host, instances.c.deleted) + cleaned_index.drop(migrate_engine) + except Exception: + pass + + shadow_instances = Table('shadow_instances', meta, autoload=True) + shadow_instances.columns.cleaned.drop() diff --git a/nova/db/sqlalchemy/models.py b/nova/db/sqlalchemy/models.py index ee8c4bfcc5..1000306064 100644 --- a/nova/db/sqlalchemy/models.py +++ b/nova/db/sqlalchemy/models.py @@ -167,7 +167,9 @@ class Instance(BASE, NovaBase): Index('instances_task_state_updated_at_idx', 'task_state', 'updated_at'), Index('instances_host_node_deleted_idx', - 'host', 'node', 'deleted') + 'host', 'node', 'deleted'), + Index('instances_host_deleted_cleaned_idx', + 'host', 'deleted', 'cleaned'), ) injected_files = [] @@ -284,6 +286,9 @@ class Instance(BASE, NovaBase): # the cells tree and it'll be a full cell name such as 'api!hop1!hop2' cell_name = Column(String(255), nullable=True) + # Records whether an instance has been deleted from disk + cleaned = Column(Integer, default=0) + class InstanceInfoCache(BASE, NovaBase): """ diff --git a/nova/objects/instance.py b/nova/objects/instance.py index b4a77aad16..485732e267 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -49,7 +49,8 @@ class Instance(base.NovaObject): # Version 1.3: Added expected_vm_state and admin_state_reset to # save() # Version 1.4: Added locked_by and deprecated locked - VERSION = '1.4' + # Version 1.5: Added cleaned + VERSION = '1.5' fields = { 'id': int, @@ -133,6 +134,8 @@ class Instance(base.NovaObject): 'fault': obj_utils.nested_object_or_none( instance_fault.InstanceFault), + 'cleaned': bool, + } obj_extra_fields = ['name'] @@ -221,6 +224,8 @@ class Instance(base.NovaObject): continue elif field == 'deleted': instance.deleted = db_inst['deleted'] == db_inst['id'] + elif field == 'cleaned': + instance.cleaned = db_inst['cleaned'] == 1 else: instance[field] = db_inst[field] @@ -349,6 +354,13 @@ class Instance(base.NovaObject): _handle_cell_update_from_api() return + # Cleaned needs to be turned back into an int here + if 'cleaned' in updates: + if updates['cleaned']: + updates['cleaned'] = 1 + else: + updates['cleaned'] = 0 + if expected_task_state is not None: updates['expected_task_state'] = expected_task_state if expected_vm_state is not None: diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py index 5602e83deb..b69a233042 100644 --- a/nova/tests/api/openstack/fakes.py +++ b/nova/tests/api/openstack/fakes.py @@ -471,7 +471,7 @@ def stub_instance(id, user_id=None, project_id=None, host=None, limit=None, marker=None, launched_at=timeutils.utcnow(), terminated_at=timeutils.utcnow(), - availability_zone='', locked_by=None): + availability_zone='', locked_by=None, cleaned=False): if user_id is None: user_id = 'fake_user' @@ -565,7 +565,8 @@ def stub_instance(id, user_id=None, project_id=None, host=None, "launched_on": "", "cell_name": "", "architecture": "", - "os_type": ""} + "os_type": "", + "cleaned": cleaned} instance.update(info_cache) instance['info_cache']['instance_uuid'] = instance['uuid'] diff --git a/nova/tests/compute/test_compute_mgr.py b/nova/tests/compute/test_compute_mgr.py index 526fe14d9d..2bc0054ea7 100644 --- a/nova/tests/compute/test_compute_mgr.py +++ b/nova/tests/compute/test_compute_mgr.py @@ -522,3 +522,51 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): for ps in (power_state.NOSTATE, power_state.SHUTDOWN): self._test_sync_to_stop(power_state.RUNNING, vs, ps, stop=False) + + def test_run_pending_deletes(self): + self.flags(instance_delete_interval=10) + + class FakeInstance(object): + def __init__(self, uuid, name, smd): + self.uuid = uuid + self.name = name + self.system_metadata = smd + self.cleaned = False + + def __getitem__(self, name): + return getattr(self, name) + + def save(self, context): + pass + + class FakeInstanceList(object): + def get_by_filters(self, *args, **kwargs): + return [] + + a = FakeInstance('123', 'apple', {'clean_attempts': '100'}) + b = FakeInstance('456', 'orange', {'clean_attempts': '3'}) + c = FakeInstance('789', 'banana', {}) + + self.mox.StubOutWithMock(instance_obj.InstanceList, + 'get_by_filters') + instance_obj.InstanceList.get_by_filters( + {'read_deleted': 'yes'}, + {'deleted': True, 'host': 'fake-mini', 'cleaned': False}, + expected_attrs=['info_cache', 'security_groups', + 'system_metadata']).AndReturn([a, b, c]) + + self.mox.StubOutWithMock(self.compute.driver, 'delete_instance_files') + self.compute.driver.delete_instance_files( + mox.IgnoreArg()).AndReturn(True) + self.compute.driver.delete_instance_files( + mox.IgnoreArg()).AndReturn(False) + + self.mox.ReplayAll() + + self.compute._run_pending_deletes({}) + self.assertFalse(a.cleaned) + self.assertEqual('100', a.system_metadata['clean_attempts']) + self.assertTrue(b.cleaned) + self.assertEqual('4', b.system_metadata['clean_attempts']) + self.assertFalse(c.cleaned) + self.assertEqual('1', c.system_metadata['clean_attempts']) diff --git a/nova/tests/db/test_db_api.py b/nova/tests/db/test_db_api.py index a46f0cf674..9c0124212b 100644 --- a/nova/tests/db/test_db_api.py +++ b/nova/tests/db/test_db_api.py @@ -1535,6 +1535,21 @@ class InstanceTestCase(test.TestCase, ModelsObjectComparatorMixin): 'soft_deleted': True}) self._assertEqualListsOfInstances([inst2, inst3], result) + def test_instance_get_all_by_filters_cleaned(self): + inst1 = self.create_instance_with_args() + inst2 = self.create_instance_with_args(reservation_id='b') + db.instance_update(self.ctxt, inst1['uuid'], {'cleaned': 1}) + result = db.instance_get_all_by_filters(self.ctxt, {}) + self.assertEqual(2, len(result)) + self.assertIn(inst1['uuid'], [result[0]['uuid'], result[1]['uuid']]) + self.assertIn(inst2['uuid'], [result[0]['uuid'], result[1]['uuid']]) + if inst1['uuid'] == result[0]['uuid']: + self.assertTrue(result[0]['cleaned']) + self.assertFalse(result[1]['cleaned']) + else: + self.assertTrue(result[1]['cleaned']) + self.assertFalse(result[0]['cleaned']) + def test_instance_get_all_by_host_and_node_no_join(self): instance = self.create_instance_with_args() result = db.instance_get_all_by_host_and_node(self.ctxt, 'h1', 'n1') diff --git a/nova/tests/db/test_migrations.py b/nova/tests/db/test_migrations.py index 96d2b3c803..f782b1ec97 100644 --- a/nova/tests/db/test_migrations.py +++ b/nova/tests/db/test_migrations.py @@ -477,6 +477,31 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn): globals(), locals(), ['versioning_api'], -1) self.migration_api = temp.versioning_api + def assertColumnExists(self, engine, table, column): + t = db_utils.get_table(engine, table) + self.assertIn(column, t.c) + + def assertColumnNotExists(self, engine, table, column): + t = db_utils.get_table(engine, table) + self.assertNotIn(column, t.c) + + def assertIndexExists(self, engine, table, index): + t = db_utils.get_table(engine, table) + index_names = [idx.name for idx in t.indexes] + self.assertIn(index, index_names) + + def assertIndexMembers(self, engine, table, index, members): + self.assertIndexExists(engine, table, index) + + t = db_utils.get_table(engine, table) + index_columns = None + for idx in t.indexes: + if idx.name == index: + index_columns = idx.columns.keys() + break + + self.assertEqual(sorted(members), sorted(index_columns)) + def _pre_upgrade_134(self, engine): now = timeutils.utcnow() data = [{ @@ -2455,6 +2480,54 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn): rows = table.select().execute().fetchall() self.assertFalse('locked_by' in rows[0]) + def _pre_upgrade_206(self, engine): + instances = db_utils.get_table(engine, 'instances') + shadow_instances = db_utils.get_table(engine, 'shadow_instances') + + data = [ + { + 'id': 650, + 'deleted': 0, + }, + { + 'id': 651, + 'deleted': 2, + }, + ] + for item in data: + instances.insert().values(item).execute() + shadow_instances.insert().values(item).execute() + return data + + def _check_206(self, engine, data): + self.assertColumnExists(engine, 'instances', 'cleaned') + self.assertColumnExists(engine, 'shadow_instances', 'cleaned') + self.assertIndexMembers(engine, 'instances', + 'instances_host_deleted_cleaned_idx', + ['host', 'deleted', 'cleaned']) + + instances = db_utils.get_table(engine, 'instances') + shadow_instances = db_utils.get_table(engine, 'shadow_instances') + + def get_(table, ident): + return table.select().\ + where(table.c.id == ident).\ + execute().\ + first() + + for table in (instances, shadow_instances): + id_1 = get_(instances, 650) + self.assertEqual(0, id_1['deleted']) + self.assertEqual(0, id_1['cleaned']) + + id_2 = get_(instances, 651) + self.assertEqual(2, id_2['deleted']) + self.assertEqual(1, id_2['cleaned']) + + def _post_downgrade_206(self, engine): + self.assertColumnNotExists(engine, 'instances', 'cleaned') + self.assertColumnNotExists(engine, 'shadow_instances', 'cleaned') + class TestBaremetalMigrations(BaseMigrationTestCase, CommonTestsMixIn): """Test sqlalchemy-migrate migrations.""" diff --git a/nova/tests/objects/test_instance.py b/nova/tests/objects/test_instance.py index 85c127c101..b887471288 100644 --- a/nova/tests/objects/test_instance.py +++ b/nova/tests/objects/test_instance.py @@ -292,6 +292,30 @@ class _TestInstanceObject(object): # NOTE(danms): Make sure it's actually a bool self.assertEqual(inst.deleted, True) + def test_get_not_cleaned(self): + ctxt = context.get_admin_context() + fake_inst = dict(self.fake_instance, id=123, cleaned=None) + fake_uuid = fake_inst['uuid'] + self.mox.StubOutWithMock(db, 'instance_get_by_uuid') + db.instance_get_by_uuid(ctxt, fake_uuid, columns_to_join=[] + ).AndReturn(fake_inst) + self.mox.ReplayAll() + inst = instance.Instance.get_by_uuid(ctxt, fake_uuid) + # NOTE(mikal): Make sure it's actually a bool + self.assertEqual(inst.cleaned, False) + + def test_get_cleaned(self): + ctxt = context.get_admin_context() + fake_inst = dict(self.fake_instance, id=123, cleaned=1) + fake_uuid = fake_inst['uuid'] + self.mox.StubOutWithMock(db, 'instance_get_by_uuid') + db.instance_get_by_uuid(ctxt, fake_uuid, columns_to_join=[] + ).AndReturn(fake_inst) + self.mox.ReplayAll() + inst = instance.Instance.get_by_uuid(ctxt, fake_uuid) + # NOTE(mikal): Make sure it's actually a bool + self.assertEqual(inst.cleaned, True) + def test_with_info_cache(self): ctxt = context.get_admin_context() fake_inst = dict(self.fake_instance) @@ -444,6 +468,28 @@ class _TestInstanceListObject(object): self.assertEqual(inst_list.objects[i].uuid, fakes[i]['uuid']) self.assertRemotes() + def test_get_all_by_filters_works_for_cleaned(self): + fakes = [self.fake_instance(1), + self.fake_instance(2, updates={'deleted': 2, + 'cleaned': None})] + ctxt = context.get_admin_context() + ctxt.read_deleted = 'yes' + self.mox.StubOutWithMock(db, 'instance_get_all_by_filters') + db.instance_get_all_by_filters(ctxt, + {'deleted': True, 'cleaned': False}, + 'uuid', 'asc', limit=None, marker=None, + columns_to_join=['metadata']).AndReturn( + [fakes[1]]) + self.mox.ReplayAll() + inst_list = instance.InstanceList.get_by_filters( + ctxt, {'deleted': True, 'cleaned': False}, 'uuid', 'asc', + expected_attrs=['metadata']) + + self.assertEqual(1, len(inst_list)) + self.assertTrue(isinstance(inst_list.objects[0], instance.Instance)) + self.assertEqual(inst_list.objects[0].uuid, fakes[1]['uuid']) + self.assertRemotes() + def test_get_by_host(self): fakes = [self.fake_instance(1), self.fake_instance(2)] diff --git a/nova/tests/virt/libvirt/test_libvirt.py b/nova/tests/virt/libvirt/test_libvirt.py index 102c588887..ae8fce6215 100644 --- a/nova/tests/virt/libvirt/test_libvirt.py +++ b/nova/tests/virt/libvirt/test_libvirt.py @@ -39,6 +39,7 @@ from nova.compute import vm_states from nova import context from nova import db from nova import exception +from nova.objects import instance as instance_obj from nova.openstack.common import fileutils from nova.openstack.common import importutils from nova.openstack.common import jsonutils @@ -3149,21 +3150,31 @@ class LibvirtConnTestCase(test.TestCase): def fake_lookup_by_name(instance_name): raise exception.InstanceNotFound(instance_id=instance_name) + def fake_delete_instance_files(instance): + pass + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.stubs.Set(conn, '_lookup_by_name', fake_lookup_by_name) + self.stubs.Set(conn, '_delete_instance_files', + fake_delete_instance_files) instance = db.instance_create(self.context, self.test_instance) conn.destroy(instance, {}) def test_destroy_removes_disk(self): - instance = {"name": "instancename", "id": "instanceid", - "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64"} + instance = {"name": "instancename", "id": "42", + "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64", + "cleaned": 0, 'info_cache': None, 'security_groups': None} self.mox.StubOutWithMock(libvirt_driver.LibvirtDriver, '_undefine_domain') libvirt_driver.LibvirtDriver._undefine_domain(instance) + self.mox.StubOutWithMock(db, 'instance_get_by_uuid') + db.instance_get_by_uuid(mox.IgnoreArg(), mox.IgnoreArg(), + columns_to_join=[]).AndReturn(instance) self.mox.StubOutWithMock(shutil, "rmtree") - shutil.rmtree(os.path.join(CONF.instances_path, instance['name'])) + shutil.rmtree(os.path.join(CONF.instances_path, + 'instance-%08x' % int(instance['id']))) self.mox.StubOutWithMock(libvirt_driver.LibvirtDriver, '_cleanup_lvm') libvirt_driver.LibvirtDriver._cleanup_lvm(instance) @@ -3182,6 +3193,13 @@ class LibvirtConnTestCase(test.TestCase): def fake_unfilter_instance(instance, network_info): pass + def fake_obj_load_attr(self, attrname): + if not hasattr(self, attrname): + self[attrname] = {} + + def fake_save(self, context): + pass + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.stubs.Set(conn, '_destroy', fake_destroy) @@ -3189,6 +3207,12 @@ class LibvirtConnTestCase(test.TestCase): self.stubs.Set(conn.firewall_driver, 'unfilter_instance', fake_unfilter_instance) self.stubs.Set(os.path, 'exists', fake_os_path_exists) + self.stubs.Set(instance_obj.Instance, 'fields', + {'id': int, 'uuid': str, 'cleaned': int}) + self.stubs.Set(instance_obj.Instance, 'obj_load_attr', + fake_obj_load_attr) + self.stubs.Set(instance_obj.Instance, 'save', fake_save) + conn.destroy(instance, []) def test_destroy_not_removes_disk(self): @@ -3223,6 +3247,41 @@ class LibvirtConnTestCase(test.TestCase): self.stubs.Set(os.path, 'exists', fake_os_path_exists) conn.destroy(instance, [], None, False) + def test_delete_instance_files(self): + instance = {"name": "instancename", "id": "42", + "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64", + "cleaned": 0, 'info_cache': None, 'security_groups': None} + + self.mox.StubOutWithMock(db, 'instance_get_by_uuid') + self.mox.StubOutWithMock(os.path, 'exists') + self.mox.StubOutWithMock(shutil, "rmtree") + + db.instance_get_by_uuid(mox.IgnoreArg(), mox.IgnoreArg(), + columns_to_join=[]).AndReturn(instance) + os.path.exists(mox.IgnoreArg()).AndReturn(False) + os.path.exists(mox.IgnoreArg()).AndReturn(True) + shutil.rmtree(os.path.join(CONF.instances_path, instance['uuid'])) + os.path.exists(mox.IgnoreArg()).AndReturn(True) + os.path.exists(mox.IgnoreArg()).AndReturn(False) + os.path.exists(mox.IgnoreArg()).AndReturn(True) + shutil.rmtree(os.path.join(CONF.instances_path, instance['uuid'])) + os.path.exists(mox.IgnoreArg()).AndReturn(False) + self.mox.ReplayAll() + + def fake_obj_load_attr(self, attrname): + if not hasattr(self, attrname): + self[attrname] = {} + + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + self.stubs.Set(instance_obj.Instance, 'fields', + {'id': int, 'uuid': str, 'cleaned': int}) + self.stubs.Set(instance_obj.Instance, 'obj_load_attr', + fake_obj_load_attr) + + inst_obj = instance_obj.Instance.get_by_uuid(None, instance['uuid']) + self.assertFalse(conn.delete_instance_files(inst_obj)) + self.assertTrue(conn.delete_instance_files(inst_obj)) + def test_reboot_different_ids(self): class FakeLoopingCall: def start(self, *a, **k): @@ -3348,9 +3407,15 @@ class LibvirtConnTestCase(test.TestCase): def fake_get_info(instance_name): return {'state': power_state.SHUTDOWN, 'id': -1} + def fake_delete_instance_files(instance): + return None + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.stubs.Set(conn, '_lookup_by_name', fake_lookup_by_name) self.stubs.Set(conn, 'get_info', fake_get_info) + self.stubs.Set(conn, '_delete_instance_files', + fake_delete_instance_files) + instance = {"name": "instancename", "id": "instanceid", "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64"} conn.destroy(instance, []) @@ -3370,9 +3435,14 @@ class LibvirtConnTestCase(test.TestCase): def fake_get_info(instance_name): return {'state': power_state.SHUTDOWN, 'id': -1} + def fake_delete_instance_files(instance): + return None + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.stubs.Set(conn, '_lookup_by_name', fake_lookup_by_name) self.stubs.Set(conn, 'get_info', fake_get_info) + self.stubs.Set(conn, '_delete_instance_files', + fake_delete_instance_files) instance = {"name": "instancename", "id": "instanceid", "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64"} conn.destroy(instance, []) @@ -3394,9 +3464,14 @@ class LibvirtConnTestCase(test.TestCase): def fake_get_info(instance_name): return {'state': power_state.SHUTDOWN, 'id': -1} + def fake_delete_instance_files(instance): + return None + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.stubs.Set(conn, '_lookup_by_name', fake_lookup_by_name) self.stubs.Set(conn, 'get_info', fake_get_info) + self.stubs.Set(conn, '_delete_instance_files', + fake_delete_instance_files) instance = {"name": "instancename", "id": "instanceid", "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64"} conn.destroy(instance, []) @@ -3417,9 +3492,14 @@ class LibvirtConnTestCase(test.TestCase): def fake_get_info(instance_name): return {'state': power_state.SHUTDOWN, 'id': -1} + def fake_delete_instance_files(instance): + return None + conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) self.stubs.Set(conn, '_lookup_by_name', fake_lookup_by_name) self.stubs.Set(conn, 'get_info', fake_get_info) + self.stubs.Set(conn, '_delete_instance_files', + fake_delete_instance_files) instance = {"name": "instancename", "id": "instanceid", "uuid": "875a8070-d0b9-4949-8b31-104d125c9a64"} conn.destroy(instance, []) diff --git a/nova/tests/virt/test_virt_drivers.py b/nova/tests/virt/test_virt_drivers.py index 5f390e34a5..50bbf43ca7 100644 --- a/nova/tests/virt/test_virt_drivers.py +++ b/nova/tests/virt/test_virt_drivers.py @@ -110,6 +110,9 @@ class _FakeDriverBackendTestCase(object): block_device_info=None): return '[]' + def fake_delete_instance_files(_self, _instance): + pass + self.stubs.Set(nova.virt.libvirt.driver.LibvirtDriver, 'get_instance_disk_info', fake_get_instance_disk_info) @@ -117,6 +120,10 @@ class _FakeDriverBackendTestCase(object): self.stubs.Set(nova.virt.libvirt.driver.disk, 'extend', fake_extend) + self.stubs.Set(nova.virt.libvirt.driver.LibvirtDriver, + '_delete_instance_files', + fake_delete_instance_files) + # Like the existing fakelibvirt.migrateToURI, do nothing, # but don't fail for these tests. self.stubs.Set(nova.virt.libvirt.driver.libvirt.Domain, diff --git a/nova/virt/driver.py b/nova/virt/driver.py index 9ffec0615a..144b7b12e4 100755 --- a/nova/virt/driver.py +++ b/nova/virt/driver.py @@ -974,6 +974,13 @@ class ComputeDriver(object): LOG.error(_("Exception dispatching event %(event)s: %(ex)s"), {'event': event, 'ex': ex}) + def delete_instance_files(self, instance): + """Delete any lingering instance files for an instance. + + :returns: True if the instance was deleted from disk, False otherwise. + """ + return True + def load_compute_driver(virtapi, compute_driver=None): """Load a compute driver module. diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 6eb54c7ecc..55c2aae001 100755 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -71,6 +71,7 @@ from nova.compute import vm_mode from nova import context as nova_context from nova import exception from nova.image import glance +from nova.objects import instance as instance_obj from nova.openstack.common import excutils from nova.openstack.common import fileutils from nova.openstack.common.gettextutils import _ @@ -912,18 +913,7 @@ class LibvirtDriver(driver.ComputeDriver): disk_dev) if destroy_disks: - target = libvirt_utils.get_instance_path(instance) - LOG.info(_('Deleting instance files %s'), target, - instance=instance) - if os.path.exists(target): - # If we fail to get rid of the directory - # tree, this shouldn't block deletion of - # the instance as whole. - try: - shutil.rmtree(target) - except OSError as e: - LOG.error(_('Failed to cleanup directory %(target)s: ' - '%(e)s'), {'target': target, 'e': e}) + self._delete_instance_files(instance) #NOTE(bfilippov): destroy all LVM disks for this instance self._cleanup_lvm(instance) @@ -4065,6 +4055,42 @@ class LibvirtDriver(driver.ComputeDriver): def inject_network_info(self, instance, nw_info): self.firewall_driver.setup_basic_filtering(instance, nw_info) + def _delete_instance_files(self, instance): + # NOTE(mikal): a shim to handle this file not using instance objects + # everywhere. Remove this when that conversion happens. + context = nova_context.get_admin_context() + inst_obj = instance_obj.Instance.get_by_uuid(context, instance['uuid']) + + # NOTE(mikal): this code should be pushed up a layer when this shim is + # removed. + attempts = int(inst_obj.system_metadata.get('clean_attempts', '0')) + success = self.delete_instance_files(inst_obj) + inst_obj.system_metadata['clean_attempts'] = str(attempts + 1) + if success: + inst_obj.cleaned = True + inst_obj.save(context) + + def delete_instance_files(self, instance): + target = libvirt_utils.get_instance_path(instance) + if os.path.exists(target): + LOG.info(_('Deleting instance files %s'), target, + instance=instance) + try: + shutil.rmtree(target) + except OSError as e: + LOG.error(_('Failed to cleanup directory %(target)s: ' + '%(e)s'), {'target': target, 'e': e}, + instance=instance) + + # It is possible that the delete failed, if so don't mark the instance + # as cleaned. + if os.path.exists(target): + LOG.info(_('Deletion of %s failed'), target, instance=instance) + return False + + LOG.info(_('Deletion of %s complete'), target, instance=instance) + return True + class HostState(object): """Manages information about the compute node through libvirt."""