From c66adee2f94b62a74a63a2d84192366e2a92bfa4 Mon Sep 17 00:00:00 2001 From: John Garbutt Date: Fri, 2 Aug 2013 17:56:16 +0100 Subject: [PATCH] xenapi: regroup spawn steps for better progress Currently the operations are not very logically grouped. Now we can see all the individual actions more clearly, we can re-arrange the operations so each step make more sense, and should be better at reporting progress. Rather than checking very late that there is no duplicate VM and enough memory, I have moved these checks before the expensive operation of downloading the disk. After this change there are 10 clear steps during spawn: * initial step so progress is not zero during download disks * download and create disks step * download and create kernel and ramdisk step * create the VM record, now the disks have been downloaded * setup the disks; resizingi up disk, and attaching, adding rescue disk * injecting instance data, currently this means writing to xenstore * setting up the networking, including the initial firewall setup * starting the VM and waiting for it to boot * configuring the VM using the agent (takes a long time on windows) * applying the final firewall filters, now the VM is running With this in place, it will be easy to decide which steps are required and which are not required when reusing the spawn code to implement finish_migration in a later change. Change-Id: I0e33dd7d298390630f632e116c8866a64e769102 --- nova/tests/virt/xenapi/test_vmops.py | 43 ++++++-------- nova/virt/xenapi/vmops.py | 84 ++++++++++++---------------- 2 files changed, 51 insertions(+), 76 deletions(-) diff --git a/nova/tests/virt/xenapi/test_vmops.py b/nova/tests/virt/xenapi/test_vmops.py index 7722662a55..8fd16afd29 100644 --- a/nova/tests/virt/xenapi/test_vmops.py +++ b/nova/tests/virt/xenapi/test_vmops.py @@ -312,21 +312,16 @@ class SpawnTestCase(VMOpsTestBase): injected_files = "fake_files" admin_password = "password" network_info = "net_info" + steps = 10 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) block_device_info['root_device_name'] = \ self.vmops.default_root_dev - steps = 11 - if rescue: - steps = 12 - - self.vmops._update_instance_progress(context, instance, 1, steps) - di_type = "di_type" vm_utils.determine_disk_image_type(image_meta).AndReturn(di_type) - self.vmops._update_instance_progress(context, instance, 2, steps) + self.vmops._update_instance_progress(context, instance, 1, steps) vdis = {"other": {"ref": "fake_ref_2", "osvol": True}} if include_root_vdi: @@ -336,56 +331,50 @@ 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, 3, steps) + self.vmops._update_instance_progress(context, instance, 2, 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, 4, steps) + self.vmops._update_instance_progress(context, instance, 3, 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, 5, steps) + self.vmops._update_instance_progress(context, instance, 4, steps) self.vmops._attach_disks(instance, vm_ref, name_label, vdis, di_type, admin_password, injected_files) - self.vmops._update_instance_progress(context, instance, 6, steps) - if rescue: self.vmops._attach_orig_disk_for_rescue(instance, vm_ref) - self.vmops._update_instance_progress(context, instance, 7, steps) - - 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_hostname(instance, vm_ref, rescue) - self.vmops._update_instance_progress(context, instance, steps - 4, - steps) + self.vmops._update_instance_progress(context, instance, 5, steps) self.vmops._inject_instance_metadata(instance, vm_ref) self.vmops._inject_auto_disk_config(instance, vm_ref) - self.vmops._update_instance_progress(context, instance, steps - 3, - steps) + self.vmops._inject_hostname(instance, vm_ref, rescue) + 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) + 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, steps - 2, - steps) + self.vmops._update_instance_progress(context, instance, 7, 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) + 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, steps - 1, - steps) + self.vmops._update_instance_progress(context, instance, 9, steps) self.vmops.firewall_driver.apply_instance_filter(instance, network_info) diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py index 84bb5e66c3..6e998acb64 100644 --- a/nova/virt/xenapi/vmops.py +++ b/nova/virt/xenapi/vmops.py @@ -333,36 +333,23 @@ class VMOps(object): if bad_volumes_callback and bad_devices: bad_volumes_callback(bad_devices) - def _create_disks(self, context, instance, name_label, disk_image_type, - image_meta, block_device_info=None): - 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) - - root_vdi = vdis.get('root') - if root_vdi: - self._resize_up_root_vdi(instance, root_vdi) - - return vdis - def spawn(self, context, instance, image_meta, injected_files, admin_password, network_info=None, block_device_info=None, name_label=None, rescue=False): - if name_label is None: - name_label = instance['name'] + if block_device_info: + LOG.debug(_("Block device information present: %s") + % block_device_info, instance=instance) + if block_device_info and not block_device_info['root_device_name']: + block_device_info['root_device_name'] = self.default_root_dev step = make_step_decorator(context, instance, self._update_instance_progress) - @step - def bdev_set_default_root(undo_mgr): - if block_device_info: - LOG.debug(_("Block device information present: %s") - % block_device_info, instance=instance) - if block_device_info and not block_device_info['root_device_name']: - block_device_info['root_device_name'] = self.default_root_dev + 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): @@ -370,9 +357,9 @@ class VMOps(object): @step def create_disks_step(undo_mgr, disk_image_type, image_meta): - vdis = self._create_disks(context, instance, name_label, - disk_image_type, image_meta, - block_device_info=block_device_info) + 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) def undo_create_disks(): vdi_refs = [vdi['ref'] for vdi in vdis.values() @@ -397,8 +384,6 @@ class VMOps(object): @step def create_vm_record_step(undo_mgr, vdis, disk_image_type, kernel_file, ramdisk_file): - 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) @@ -424,32 +409,32 @@ class VMOps(object): LOG.warning(_('ipxe_boot is True but no ISO image found'), instance=instance) + root_vdi = vdis.get('root') + if root_vdi: + 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 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): + 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) @step - def setup_network_step(undo_mgr, vm_ref, vdis): - 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_hostname(instance, vm_ref, rescue) - - @step - def inject_instance_data_step(undo_mgr, vm_ref): + 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) + self._file_inject_vm_settings(instance, vm_ref, vdis, network_info) + self.inject_network_info(instance, network_info, vm_ref) @step - def prepare_security_group_filters_step(undo_mgr): + def setup_network_step(undo_mgr, vm_ref): + self._create_vifs(instance, vm_ref, network_info) + try: self.firewall_driver.setup_basic_filtering( instance, network_info) @@ -466,6 +451,9 @@ class VMOps(object): def boot_instance_step(undo_mgr, vm_ref): 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) @@ -481,23 +469,21 @@ class VMOps(object): # over the network and images can be several gigs in size. To # avoid progress remaining at 0% for too long, make sure the # first step is something that completes rather quickly. - bdev_set_default_root(undo_mgr) disk_image_type = determine_disk_image_type_step(undo_mgr) vdis = create_disks_step(undo_mgr, disk_image_type, image_meta) kernel_file, ramdisk_file = create_kernel_ramdisk_step(undo_mgr) + vm_ref = create_vm_record_step(undo_mgr, vdis, disk_image_type, kernel_file, ramdisk_file) attach_disks_step(undo_mgr, vm_ref, vdis, disk_image_type) - setup_network_step(undo_mgr, vm_ref, vdis) - inject_instance_data_step(undo_mgr, vm_ref) - prepare_security_group_filters_step(undo_mgr) - if rescue: - attach_root_disk_step(undo_mgr, vm_ref) + inject_instance_data_step(undo_mgr, vm_ref, vdis) + setup_network_step(undo_mgr, vm_ref) boot_instance_step(undo_mgr, vm_ref) + configure_booted_instance_step(undo_mgr, vm_ref) apply_security_group_filters_step(undo_mgr) except Exception: msg = _("Failed to spawn, rolling back")