From 0552f37a204cbfa80cf5ec0dd44843eb6e7aaa25 Mon Sep 17 00:00:00 2001 From: Brian Elliott Date: Tue, 17 Sep 2013 10:48:57 +0000 Subject: [PATCH] xenapi: Make rescue safer If rescue setup fails, detach the root disk from the rescue VM so it doesn't get nuked by the undo stack logic in spawn() bug 1227898 Change-Id: I8864b4c524f04f7b387be1018fea9919a461c48b --- nova/tests/virt/xenapi/test_vmops.py | 40 +++++++++++++++++++-------- nova/tests/virt/xenapi/test_xenapi.py | 27 ++++++++++++++++++ nova/virt/xenapi/vmops.py | 26 ++++++++++++----- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/nova/tests/virt/xenapi/test_vmops.py b/nova/tests/virt/xenapi/test_vmops.py index 8502a35b92..510baa9524 100644 --- a/nova/tests/virt/xenapi/test_vmops.py +++ b/nova/tests/virt/xenapi/test_vmops.py @@ -313,6 +313,9 @@ class SpawnTestCase(VMOpsTestBase): admin_password = "password" network_info = "net_info" steps = 10 + if rescue: + steps += 1 + block_device_info = block_device_info_param if block_device_info and not block_device_info['root_device_name']: block_device_info = dict(block_device_info_param) @@ -321,7 +324,8 @@ class SpawnTestCase(VMOpsTestBase): di_type = "di_type" vm_utils.determine_disk_image_type(image_meta).AndReturn(di_type) - self.vmops._update_instance_progress(context, instance, 1, steps) + step = 1 + self.vmops._update_instance_progress(context, instance, step, steps) vdis = {"other": {"ref": "fake_ref_2", "osvol": True}} if include_root_vdi: @@ -331,26 +335,28 @@ class SpawnTestCase(VMOpsTestBase): block_device_info=block_device_info).AndReturn(vdis) if include_root_vdi: self.vmops._resize_up_root_vdi(instance, vdis["root"]) - self.vmops._update_instance_progress(context, instance, 2, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) kernel_file = "kernel" ramdisk_file = "ramdisk" vm_utils.create_kernel_and_ramdisk(context, session, instance, name_label).AndReturn((kernel_file, ramdisk_file)) - self.vmops._update_instance_progress(context, instance, 3, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) vm_ref = "fake_vm_ref" self.vmops._ensure_instance_name_unique(name_label) self.vmops._ensure_enough_free_mem(instance) self.vmops._create_vm_record(context, instance, name_label, vdis, di_type, kernel_file, ramdisk_file).AndReturn(vm_ref) - self.vmops._update_instance_progress(context, instance, 4, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) self.vmops._attach_disks(instance, vm_ref, name_label, vdis, di_type, admin_password, injected_files) - if rescue: - self.vmops._attach_orig_disk_for_rescue(instance, vm_ref) - self.vmops._update_instance_progress(context, instance, 5, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) self.vmops._inject_instance_metadata(instance, vm_ref) self.vmops._inject_auto_disk_config(instance, vm_ref) @@ -358,28 +364,38 @@ class SpawnTestCase(VMOpsTestBase): self.vmops._file_inject_vm_settings(instance, vm_ref, vdis, network_info) self.vmops.inject_network_info(instance, network_info, vm_ref) - self.vmops._update_instance_progress(context, instance, 6, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) self.vmops._create_vifs(instance, vm_ref, network_info) self.vmops.firewall_driver.setup_basic_filtering(instance, network_info).AndRaise(NotImplementedError) self.vmops.firewall_driver.prepare_instance_filter(instance, network_info) - self.vmops._update_instance_progress(context, instance, 7, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) + if rescue: + self.vmops._attach_orig_disk_for_rescue(instance, vm_ref) + step += 1 + self.vmops._update_instance_progress(context, instance, step, + steps) self.vmops._start(instance, vm_ref) self.vmops._wait_for_instance_to_start(instance, vm_ref) - self.vmops._update_instance_progress(context, instance, 8, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) self.vmops._configure_new_instance_with_agent(instance, vm_ref, injected_files, admin_password) self.vmops._remove_hostname(instance, vm_ref) - self.vmops._update_instance_progress(context, instance, 9, steps) + step += 1 + self.vmops._update_instance_progress(context, instance, step, steps) self.vmops.firewall_driver.apply_instance_filter(instance, network_info) + step += 1 last_call = self.vmops._update_instance_progress(context, instance, - steps, steps) + step, steps) if throw_exception: last_call.AndRaise(throw_exception) self.vmops._destroy(instance, vm_ref, network_info=network_info) diff --git a/nova/tests/virt/xenapi/test_xenapi.py b/nova/tests/virt/xenapi/test_xenapi.py index 5482f5636c..81b0b450f0 100644 --- a/nova/tests/virt/xenapi/test_xenapi.py +++ b/nova/tests/virt/xenapi/test_xenapi.py @@ -1241,6 +1241,33 @@ class XenAPIVMTestCase(stubs.XenAPITestBase): vdi_uuids.append(xenapi_fake.get_record('VBD', vbd_uuid)["VDI"]) self.assertTrue("swap" not in vdi_uuids) + def test_rescue_preserve_disk_on_failure(self): + # test that the original disk is preserved if rescue setup fails + # bug #1227898 + instance = self._create_instance() + session = xenapi_conn.XenAPISession('test_url', 'root', 'test_pass', + fake.FakeVirtAPI()) + image_meta = {'id': IMAGE_VHD, + 'disk_format': 'vhd'} + + vm_ref = vm_utils.lookup(session, instance['name']) + vdi_ref, vdi_rec = vm_utils.get_vdi_for_vm_safely(session, vm_ref) + + # raise an error in the spawn setup process and trigger the + # undo manager logic: + def fake_start(*args, **kwargs): + raise test.TestingException('Start Error') + + self.stubs.Set(self.conn._vmops, '_start', fake_start) + + self.assertRaises(test.TestingException, self.conn.rescue, + self.context, instance, [], image_meta, '') + + # confirm original disk still exists: + vdi_ref2, vdi_rec2 = vm_utils.get_vdi_for_vm_safely(session, vm_ref) + self.assertEqual(vdi_ref, vdi_ref2) + self.assertEqual(vdi_rec['uuid'], vdi_rec2['uuid']) + def test_unrescue(self): instance = self._create_instance() conn = xenapi_conn.XenAPIDriver(fake.FakeVirtAPI(), False) diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py index e9af2a01d0..04a2d30ac1 100644 --- a/nova/virt/xenapi/vmops.py +++ b/nova/virt/xenapi/vmops.py @@ -424,11 +424,20 @@ class VMOps(object): self._attach_mapped_block_devices(instance, block_device_info) - if rescue: - # NOTE(johannes): Attach root disk to rescue VM now, before - # booting the VM, since we can't hotplug block devices - # on non-PV guests - self._attach_orig_disk_for_rescue(instance, vm_ref) + if rescue: + # NOTE(johannes): Attach root disk to rescue VM now, before + # booting the VM, since we can't hotplug block devices + # on non-PV guests + @step + def attach_root_disk_step(undo_mgr, vm_ref): + vbd_ref = self._attach_orig_disk_for_rescue(instance, vm_ref) + + def undo_attach_root_disk(): + # destroy the vbd in preparation to re-attach the VDI + # to its original VM. (does not delete VDI) + vm_utils.destroy_vbd(self._session, vbd_ref) + + undo_mgr.undo_with(undo_attach_root_disk) @step def inject_instance_data_step(undo_mgr, vm_ref, vdis): @@ -492,6 +501,9 @@ class VMOps(object): inject_instance_data_step(undo_mgr, vm_ref, vdis) setup_network_step(undo_mgr, vm_ref) + if rescue: + attach_root_disk_step(undo_mgr, vm_ref) + boot_instance_step(undo_mgr, vm_ref) configure_booted_instance_step(undo_mgr, vm_ref) @@ -506,8 +518,8 @@ class VMOps(object): def _attach_orig_disk_for_rescue(self, instance, vm_ref): orig_vm_ref = vm_utils.lookup(self._session, instance['name']) vdi_ref = self._find_root_vdi_ref(orig_vm_ref) - vm_utils.create_vbd(self._session, vm_ref, vdi_ref, DEVICE_RESCUE, - bootable=False) + return vm_utils.create_vbd(self._session, vm_ref, vdi_ref, + DEVICE_RESCUE, bootable=False) def _file_inject_vm_settings(self, instance, vm_ref, vdis, network_info): if CONF.flat_injected: