xenapi: ensure finish_migration cleans on errors

This change makes finish_migration share code with spawn.

This fixes some inconsistencies, such as applying the security
group filters during finish_migration.

Fixes bug 1073303
Fixes bug 1073306

Change-Id: Ib4f2a96618692c141708535f9463c856f3395922

nova/tests/virt/xenapi/stubs.py

@@ -299,6 +299,9 @@ def fake_move_disks(self, instance, disk_info):
         vdi_rec['other_config']['nova_disk_type'] = 'root'
         return {'uuid': vdi_rec['uuid'], 'ref': vdi_ref}
 
+    def fake_wait_for_instance_to_start(self, *args):
+        pass
+
     def fake_get_vdi(session, vm_ref):
         vdi_ref_parent = fake.create_vdi('derp-parent', fakesr)
         vdi_rec_parent = fake.get_record('VDI', vdi_ref_parent)
@@ -320,6 +323,8 @@ def fake_generate_ephemeral(*args):
         pass
 
     stubs.Set(vmops.VMOps, '_destroy', fake_destroy)
+    stubs.Set(vmops.VMOps, '_wait_for_instance_to_start',
+              fake_wait_for_instance_to_start)
     stubs.Set(vm_utils, 'move_disks', fake_move_disks)
     stubs.Set(vm_utils, 'scan_default_sr', fake_sr)
     stubs.Set(vm_utils, 'get_vdi_for_vm_safely', fake_get_vdi)

nova/tests/virt/xenapi/test_vmops.py

@@ -404,7 +404,8 @@ def test_spawn_performs_rollback_and_throws_exception(self):
         self.assertRaises(test.TestingException, self._test_spawn,
                           throw_exception=test.TestingException())
 
-    def test_finish_migration(self):
+    def _test_finish_migration(self, power_on=True, resize_instance=True,
+                               throw_exception=None):
         self._stub_out_common()
         self.mox.StubOutWithMock(vm_utils, "move_disks")
         self.mox.StubOutWithMock(self.vmops, "_attach_mapped_block_devices")
@@ -416,49 +417,78 @@ def test_finish_migration(self):
         disk_info = "disk_info"
         network_info = "net_info"
         image_meta = {"id": "image_id"}
-        resize_instance = True
         block_device_info = "bdi"
         session = self.vmops._session
 
-        root_vdi = "root_vdi"
+        self.vmops._ensure_instance_name_unique(name_label)
+        self.vmops._ensure_enough_free_mem(instance)
+
+        di_type = "di_type"
+        vm_utils.determine_disk_image_type(image_meta).AndReturn(di_type)
+
+        root_vdi = {"ref": "fake_ref"}
         vdis = {"root": root_vdi}
         vm_utils.move_disks(self.vmops._session, instance,
                             disk_info).AndReturn(root_vdi)
 
-        self.vmops._resize_up_root_vdi(instance, root_vdi)
-
         kernel_file = "kernel"
         ramdisk_file = "ramdisk"
         vm_utils.create_kernel_and_ramdisk(context, session,
                 instance, name_label).AndReturn((kernel_file, ramdisk_file))
 
-        di_type = "di_type"
-        vm_utils.determine_disk_image_type(image_meta).AndReturn(di_type)
-        self.vmops._ensure_instance_name_unique(name_label)
-        self.vmops._ensure_enough_free_mem(instance)
         vm_ref = "fake_vm_ref"
         self.vmops._create_vm_record(context, instance, name_label, vdis,
                 di_type, kernel_file, ramdisk_file).AndReturn(vm_ref)
 
-        self.vmops._attach_disks(instance, vm_ref, name_label, vdis, di_type)
+        if resize_instance:
+            self.vmops._resize_up_root_vdi(instance, root_vdi)
+        self.vmops._attach_disks(instance, vm_ref, name_label, vdis, di_type,
+                                 None, None)
+        self.vmops._attach_mapped_block_devices(instance, block_device_info)
 
+        self.vmops._inject_instance_metadata(instance, vm_ref)
+        self.vmops._inject_auto_disk_config(instance, vm_ref)
         self.vmops._file_inject_vm_settings(instance, vm_ref, vdis,
                                             network_info)
-        self.vmops._create_vifs(instance, vm_ref, network_info)
         self.vmops.inject_network_info(instance, network_info, vm_ref)
-        self.vmops._inject_instance_metadata(instance, vm_ref)
 
-        self.vmops._attach_mapped_block_devices(instance, block_device_info)
+        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._start(instance, vm_ref)
+        if power_on:
+            self.vmops._start(instance, vm_ref)
+            self.vmops._wait_for_instance_to_start(instance, vm_ref)
 
-        self.vmops._update_instance_progress(context, instance,
-                                             step=5, total_steps=5)
+        self.vmops.firewall_driver.apply_instance_filter(instance,
+                                                         network_info)
+
+        last_call = self.vmops._update_instance_progress(context, instance,
+                                                        step=5, total_steps=5)
+        if throw_exception:
+            last_call.AndRaise(throw_exception)
+            self.vmops._destroy(instance, vm_ref, network_info=network_info)
+            vm_utils.destroy_kernel_ramdisk(self.vmops._session, instance,
+                                            kernel_file, ramdisk_file)
+            vm_utils.safe_destroy_vdis(self.vmops._session, ["fake_ref"])
 
         self.mox.ReplayAll()
         self.vmops.finish_migration(context, migration, instance, disk_info,
                                     network_info, image_meta, resize_instance,
-                                    block_device_info)
+                                    block_device_info, power_on)
+
+    def test_finish_migration(self):
+        self._test_finish_migration()
+
+    def test_finish_migration_no_power_on(self):
+        self._test_finish_migration(power_on=False, resize_instance=False)
+
+    def test_finish_migrate_performs_rollback_on_error(self):
+        self.assertRaises(test.TestingException, self._test_finish_migration,
+                          power_on=False, resize_instance=False,
+                          throw_exception=test.TestingException())
 
     def test_remove_hostname(self):
         vm, vm_ref = self.create_vm("dummy")

nova/virt/xenapi/vmops.py

@@ -274,39 +274,32 @@ def _restore_orig_vm_and_cleanup_orphan(self, instance,
     def finish_migration(self, context, migration, instance, disk_info,
                          network_info, image_meta, resize_instance,
                          block_device_info=None, power_on=True):
-        root_vdi = vm_utils.move_disks(self._session, instance, disk_info)
-        vdis = {'root': root_vdi}
 
-        if resize_instance:
-            self._resize_up_root_vdi(instance, root_vdi)
+        def null_step_decorator(f):
+            return f
 
-        name_label = instance['name']
+        def create_disks_step(undo_mgr, disk_image_type, image_meta,
+                              name_label):
+            #TODO(johngarbutt) clean up the move_disks if this is not run
+            root_vdi = vm_utils.move_disks(self._session, instance, disk_info)
 
-        kernel_file, ramdisk_file = vm_utils.create_kernel_and_ramdisk(
-                context, self._session, instance, name_label)
-
-        disk_image_type = vm_utils.determine_disk_image_type(image_meta)
-        self._ensure_instance_name_unique(name_label)
-        self._ensure_enough_free_mem(instance)
-        vm_ref = self._create_vm_record(context, instance, name_label,
-                vdis, disk_image_type, kernel_file, ramdisk_file)
-
-        self._attach_disks(instance, vm_ref, name_label, vdis,
-                           disk_image_type)
+            def undo_create_disks():
+                vm_utils.safe_destroy_vdis(self._session, [root_vdi['ref']])
 
-        self._file_inject_vm_settings(instance, vm_ref, vdis, network_info)
-        self._create_vifs(instance, vm_ref, network_info)
-        self.inject_network_info(instance, network_info, vm_ref)
-        self._inject_instance_metadata(instance, vm_ref)
+            undo_mgr.undo_with(undo_create_disks)
+            return {'root': root_vdi}
 
-        self._attach_mapped_block_devices(instance, block_device_info)
+        def completed_callback():
+            self._update_instance_progress(context, instance,
+                                           step=5,
+                                           total_steps=RESIZE_TOTAL_STEPS)
 
-        # 5. Start VM
-        if power_on:
-            self._start(instance, vm_ref)
-        self._update_instance_progress(context, instance,
-                                       step=5,
-                                       total_steps=RESIZE_TOTAL_STEPS)
+        self._spawn(context, instance, image_meta, null_step_decorator,
+                    create_disks_step, first_boot=False, injected_files=None,
+                    admin_password=None, network_info=network_info,
+                    block_device_info=block_device_info, name_label=None,
+                    rescue=False, power_on=power_on, resize=resize_instance,
+                    completed_callback=completed_callback)
 
     def _start(self, instance, vm_ref=None, bad_volumes_callback=None):
         """Power on a VM instance."""
@@ -336,6 +329,7 @@ def _start(self, instance, vm_ref=None, bad_volumes_callback=None):
     def spawn(self, context, instance, image_meta, injected_files,
               admin_password, network_info=None, block_device_info=None,
               name_label=None, rescue=False):
+
         if block_device_info:
             LOG.debug(_("Block device information present: %s")
                       % block_device_info, instance=instance)
@@ -345,18 +339,9 @@ def spawn(self, context, instance, image_meta, injected_files,
         step = make_step_decorator(context, instance,
                                    self._update_instance_progress)
 
-        if name_label is None:
-            name_label = instance['name']
-
-        self._ensure_instance_name_unique(name_label)
-        self._ensure_enough_free_mem(instance)
-
-        @step
-        def determine_disk_image_type_step(undo_mgr):
-            return vm_utils.determine_disk_image_type(image_meta)
-
         @step
-        def create_disks_step(undo_mgr, disk_image_type, image_meta):
+        def create_disks_step(undo_mgr, disk_image_type, image_meta,
+                              name_label):
             vdis = vm_utils.get_vdis_for_instance(context, self._session,
                         instance, name_label, image_meta.get('id'),
                         disk_image_type, block_device_info=block_device_info)
@@ -369,6 +354,25 @@ def undo_create_disks():
             undo_mgr.undo_with(undo_create_disks)
             return vdis
 
+        self._spawn(context, instance, image_meta, step, create_disks_step,
+                    True, injected_files, admin_password,
+                    network_info, block_device_info, name_label, rescue)
+
+    def _spawn(self, context, instance, image_meta, step, create_disks_step,
+               first_boot, injected_files=None, admin_password=None,
+               network_info=None, block_device_info=None,
+               name_label=None, rescue=False, power_on=True, resize=True,
+               completed_callback=None):
+        if name_label is None:
+            name_label = instance['name']
+
+        self._ensure_instance_name_unique(name_label)
+        self._ensure_enough_free_mem(instance)
+
+        @step
+        def determine_disk_image_type_step(undo_mgr):
+            return vm_utils.determine_disk_image_type(image_meta)
+
         @step
         def create_kernel_ramdisk_step(undo_mgr):
             kernel_file, ramdisk_file = vm_utils.create_kernel_and_ramdisk(
@@ -410,12 +414,15 @@ def attach_disks_step(undo_mgr, vm_ref, vdis, disk_image_type):
                                 instance=instance)
 
             root_vdi = vdis.get('root')
-            if root_vdi:
+            if root_vdi and resize:
                 self._resize_up_root_vdi(instance, root_vdi)
 
             self._attach_disks(instance, vm_ref, name_label, vdis,
                                disk_image_type, admin_password,
                                injected_files)
+            if not first_boot:
+                self._attach_mapped_block_devices(instance,
+                                                  block_device_info)
 
             if rescue:
                 # NOTE(johannes): Attach root disk to rescue VM now, before
@@ -427,7 +434,8 @@ def attach_disks_step(undo_mgr, vm_ref, vdis, disk_image_type):
         def inject_instance_data_step(undo_mgr, vm_ref, vdis):
             self._inject_instance_metadata(instance, vm_ref)
             self._inject_auto_disk_config(instance, vm_ref)
-            self._inject_hostname(instance, vm_ref, rescue)
+            if first_boot:
+                self._inject_hostname(instance, vm_ref, rescue)
             self._file_inject_vm_settings(instance, vm_ref, vdis, network_info)
             self.inject_network_info(instance, network_info, vm_ref)
 
@@ -449,14 +457,16 @@ def setup_network_step(undo_mgr, vm_ref):
 
         @step
         def boot_instance_step(undo_mgr, vm_ref):
-            self._start(instance, vm_ref)
-            self._wait_for_instance_to_start(instance, vm_ref)
+            if power_on:
+                self._start(instance, vm_ref)
+                self._wait_for_instance_to_start(instance, vm_ref)
 
         @step
         def configure_booted_instance_step(undo_mgr, vm_ref):
-            self._configure_new_instance_with_agent(instance, vm_ref,
-                    injected_files, admin_password)
-            self._remove_hostname(instance, vm_ref)
+            if first_boot:
+                self._configure_new_instance_with_agent(instance, vm_ref,
+                        injected_files, admin_password)
+                self._remove_hostname(instance, vm_ref)
 
         @step
         def apply_security_group_filters_step(undo_mgr):
@@ -471,7 +481,8 @@ def apply_security_group_filters_step(undo_mgr):
             # first step is something that completes rather quickly.
             disk_image_type = determine_disk_image_type_step(undo_mgr)
 
-            vdis = create_disks_step(undo_mgr, disk_image_type, image_meta)
+            vdis = create_disks_step(undo_mgr, disk_image_type, image_meta,
+                                     name_label)
             kernel_file, ramdisk_file = create_kernel_ramdisk_step(undo_mgr)
 
             vm_ref = create_vm_record_step(undo_mgr, vdis, disk_image_type,
@@ -485,6 +496,9 @@ def apply_security_group_filters_step(undo_mgr):
 
             configure_booted_instance_step(undo_mgr, vm_ref)
             apply_security_group_filters_step(undo_mgr)
+
+            if completed_callback:
+                completed_callback()
         except Exception:
             msg = _("Failed to spawn, rolling back")
             undo_mgr.rollback_and_reraise(msg=msg, instance=instance)