From c586d635387e9baa3c0857afb56d05137fcddd7c Mon Sep 17 00:00:00 2001 From: Gary Kotton Date: Thu, 12 Sep 2013 02:23:36 -0700 Subject: [PATCH] VMware: fix VM resize bug The following operations would fail due to invalid references, either by name or UUID: - resize - resize-confirm - resize-revert Closes-bug: #1199954 Change-Id: I39b804ed3c0c57455547d9ece0ef779cf73a5926 --- nova/tests/virt/vmwareapi/test_vmwareapi.py | 37 ++++++++++++++++++- .../virt/vmwareapi/test_vmwareapi_vm_util.py | 13 +++++++ nova/virt/vmwareapi/driver.py | 20 ++++++---- nova/virt/vmwareapi/fake.py | 12 ++++++ nova/virt/vmwareapi/vm_util.py | 14 ++++++- nova/virt/vmwareapi/vmops.py | 35 ++++++++++++------ 6 files changed, 107 insertions(+), 24 deletions(-) diff --git a/nova/tests/virt/vmwareapi/test_vmwareapi.py b/nova/tests/virt/vmwareapi/test_vmwareapi.py index d094257ba859..90a458b099fe 100644 --- a/nova/tests/virt/vmwareapi/test_vmwareapi.py +++ b/nova/tests/virt/vmwareapi/test_vmwareapi.py @@ -648,7 +648,7 @@ class VMwareAPIVMTestCase(test.NoDBTestCase): res = self.conn.get_console_output(self.instance) self.assertNotEqual(0, len(res)) - def _test_finish_migration(self, power_on): + def _test_finish_migration(self, power_on, resize_instance=False): """ Tests the finish_migration method on vmops """ @@ -679,7 +679,7 @@ class VMwareAPIVMTestCase(test.NoDBTestCase): disk_info=None, network_info=None, block_device_info=None, - resize_instance=False, + resize_instance=resize_instance, image_meta=None, power_on=power_on) @@ -691,6 +691,12 @@ class VMwareAPIVMTestCase(test.NoDBTestCase): self.assertRaises(NotImplementedError, self._test_finish_migration, power_on=False) + def test_confirm_migration(self): + self._create_vm() + self.assertRaises(NotImplementedError, + self.conn.confirm_migration, self.context, + self.instance, None) + def _test_finish_revert_migration(self, power_on): """ Tests the finish_revert_migration method on vmops @@ -1082,6 +1088,11 @@ class VMwareAPIVCDriverTestCase(VMwareAPIVMTestCase): self._test_finish_migration(power_on=False) self.assertEqual(False, self.power_on_called) + def test_finish_migration_power_on_resize(self): + self._test_finish_migration(power_on=True, + resize_instance=True) + self.assertEquals(True, self.power_on_called) + def test_finish_revert_migration_power_on(self): self._test_finish_revert_migration(power_on=True) self.assertEqual(True, self.power_on_called) @@ -1161,3 +1172,25 @@ class VMwareAPIVCDriverTestCase(VMwareAPIVMTestCase): self.assertRaises(NotImplementedError, self.conn.get_diagnostics, self.instance) + + def test_migrate_disk_and_power_off(self): + def fake_update_instance_progress(context, instance, step, + total_steps): + pass + + def fake_get_host_ref_from_name(dest): + return None + + self._create_vm() + instance_type = {'name': 'fake', 'flavorid': 'fake_id'} + self.stubs.Set(self.conn._vmops, "_update_instance_progress", + fake_update_instance_progress) + self.stubs.Set(self.conn._vmops, "_get_host_ref_from_name", + fake_get_host_ref_from_name) + self.conn.migrate_disk_and_power_off(self.context, self.instance, + 'fake_dest', instance_type, + None) + + def test_confirm_migration(self): + self._create_vm() + self.conn.confirm_migration(self.context, self.instance, None) diff --git a/nova/tests/virt/vmwareapi/test_vmwareapi_vm_util.py b/nova/tests/virt/vmwareapi/test_vmwareapi_vm_util.py index bae477650a25..a2d7f97c75e5 100755 --- a/nova/tests/virt/vmwareapi/test_vmwareapi_vm_util.py +++ b/nova/tests/virt/vmwareapi/test_vmwareapi_vm_util.py @@ -221,6 +221,19 @@ class VMwareVMUtilTestCase(test.NoDBTestCase): vm_util.get_datastore_ref_and_name, fake_session(fake_objects)) + def test_get_resize_spec(self): + fake_instance = {'id': 7, 'name': 'fake!', + 'uuid': 'bda5fb9e-b347-40e8-8256-42397848cb00', + 'vcpus': 2, 'memory_mb': 2048} + result = vm_util.get_vm_resize_spec(fake.FakeFactory(), + fake_instance) + expected = """{'memoryMB': 2048, + 'numCPUs': 2, + 'obj_name': 'ns0:VirtualMachineConfigSpec'}""" + expected = re.sub(r'\s+', '', expected) + result = re.sub(r'\s+', '', repr(result)) + self.assertEqual(expected, result) + def test_get_cdrom_attach_config_spec(self): result = vm_util.get_cdrom_attach_config_spec(fake.FakeFactory(), diff --git a/nova/virt/vmwareapi/driver.py b/nova/virt/vmwareapi/driver.py index f145704df8a9..0ae8afe0a153 100644 --- a/nova/virt/vmwareapi/driver.py +++ b/nova/virt/vmwareapi/driver.py @@ -431,26 +431,30 @@ class VMwareVCDriver(VMwareESXDriver): Transfers the disk of a running instance in multiple phases, turning off the instance before the end. """ - return self._vmops.migrate_disk_and_power_off(context, instance, - dest, instance_type) + _vmops = self._get_vmops_for_compute_node(instance['node']) + return _vmops.migrate_disk_and_power_off(context, instance, + dest, instance_type) def confirm_migration(self, migration, instance, network_info): """Confirms a resize, destroying the source VM.""" - self._vmops.confirm_migration(migration, instance, network_info) + _vmops = self._get_vmops_for_compute_node(instance['node']) + _vmops.confirm_migration(migration, instance, network_info) def finish_revert_migration(self, instance, network_info, block_device_info=None, power_on=True): """Finish reverting a resize, powering back on the instance.""" - self._vmops.finish_revert_migration(instance, network_info, - block_device_info, power_on) + _vmops = self._get_vmops_for_compute_node(instance['node']) + _vmops.finish_revert_migration(instance, network_info, + block_device_info, power_on) def finish_migration(self, context, migration, instance, disk_info, network_info, image_meta, resize_instance=False, block_device_info=None, power_on=True): """Completes a resize, turning on the migrated instance.""" - self._vmops.finish_migration(context, migration, instance, disk_info, - network_info, image_meta, resize_instance, - block_device_info, power_on) + _vmops = self._get_vmops_for_compute_node(instance['node']) + _vmops.finish_migration(context, migration, instance, disk_info, + network_info, image_meta, resize_instance, + block_device_info, power_on) def live_migration(self, context, instance_ref, dest, post_method, recover_method, block_migration=False, diff --git a/nova/virt/vmwareapi/fake.py b/nova/virt/vmwareapi/fake.py index dbd713399eec..e300ad98bb74 100644 --- a/nova/virt/vmwareapi/fake.py +++ b/nova/virt/vmwareapi/fake.py @@ -331,6 +331,9 @@ class VirtualMachine(ManagedObject): setting of the Virtual Machine object. """ try: + if not hasattr(val, 'deviceChange'): + return + if len(val.deviceChange) < 2: return @@ -967,6 +970,10 @@ class FakeVim(object): task_mdo = create_task(method, "success") return task_mdo.obj + def _clone_vm(self, method, *args, **kwargs): + """Fakes a VM clone.""" + return self._just_return_task(method) + def _unregister_vm(self, method, *args, **kwargs): """Unregisters a VM from the Host System.""" vm_ref = args[0] @@ -1107,6 +1114,11 @@ class FakeVim(object): elif attr_name == "UnregisterVM": return lambda *args, **kwargs: self._unregister_vm(attr_name, *args, **kwargs) + elif attr_name == "CloneVM_Task": + return lambda *args, **kwargs: self._clone_vm(attr_name, + *args, **kwargs) + elif attr_name == "Rename_Task": + return lambda *args, **kwargs: self._just_return_task(attr_name) elif attr_name == "SearchDatastore_Task": return lambda *args, **kwargs: self._search_ds(attr_name, *args, **kwargs) diff --git a/nova/virt/vmwareapi/vm_util.py b/nova/virt/vmwareapi/vm_util.py index 953e5a8fd3d4..7f7a06fd0f2d 100644 --- a/nova/virt/vmwareapi/vm_util.py +++ b/nova/virt/vmwareapi/vm_util.py @@ -106,6 +106,14 @@ def get_vm_create_spec(client_factory, instance, data_store_name, return config_spec +def get_vm_resize_spec(client_factory, instance): + """Provides updates for a VM spec.""" + resize_spec = client_factory.create('ns0:VirtualMachineConfigSpec') + resize_spec.numCPUs = int(instance['vcpus']) + resize_spec.memoryMB = int(instance['memory_mb']) + return resize_spec + + def create_controller_spec(client_factory, key, adapter_type="lsiLogic"): """ Builds a Config Spec for the LSI or Bus Logic Controller's addition @@ -480,7 +488,8 @@ def clone_vm_spec(client_factory, location, clone_spec = client_factory.create('ns0:VirtualMachineCloneSpec') clone_spec.location = location clone_spec.powerOn = power_on - clone_spec.snapshot = snapshot + if snapshot: + clone_spec.snapshot = snapshot clone_spec.template = template return clone_spec @@ -491,7 +500,8 @@ def relocate_vm_spec(client_factory, datastore=None, host=None, rel_spec = client_factory.create('ns0:VirtualMachineRelocateSpec') rel_spec.datastore = datastore rel_spec.diskMoveType = disk_move_type - rel_spec.host = host + if host: + rel_spec.host = host return rel_spec diff --git a/nova/virt/vmwareapi/vmops.py b/nova/virt/vmwareapi/vmops.py index a6b86ac432f6..ecb9e90b1505 100644 --- a/nova/virt/vmwareapi/vmops.py +++ b/nova/virt/vmwareapi/vmops.py @@ -1097,7 +1097,7 @@ class VMwareVMOps(object): self._power_on(instance) def _get_orig_vm_name_label(self, instance): - return instance['name'] + '-orig' + return instance['uuid'] + '-orig' def _update_instance_progress(self, context, instance, step, total_steps): """Update instance progress percent to reflect current step number @@ -1130,9 +1130,9 @@ class VMwareVMOps(object): total_steps=RESIZE_TOTAL_STEPS) vm_ref = vm_util.get_vm_ref(self._session, instance) + # Read the host_ref for the destination. If this is None then the + # VC will decide on placement host_ref = self._get_host_ref_from_name(dest) - if host_ref is None: - raise exception.HostNotFound(host=dest) # 1. Power off the instance self.power_off(instance) @@ -1156,7 +1156,8 @@ class VMwareVMOps(object): # Get the clone vm spec ds_ref = vm_util.get_datastore_ref_and_name( - self._session, None, dest)[0] + self._session, self._cluster, host_ref, + datastore_regex=self._datastore_regex)[0] client_factory = self._session._get_vim().client.factory rel_spec = vm_util.relocate_vm_spec(client_factory, ds_ref, host_ref) clone_spec = vm_util.clone_vm_spec(client_factory, rel_spec) @@ -1168,7 +1169,7 @@ class VMwareVMOps(object): self._session._get_vim(), "CloneVM_Task", vm_ref, folder=vm_folder_ref, - name=instance['name'], + name=instance['uuid'], spec=clone_spec) self._session._wait_for_task(instance['uuid'], vm_clone_task) LOG.debug(_("Cloned VM to host %s") % dest, instance=instance) @@ -1179,10 +1180,9 @@ class VMwareVMOps(object): def confirm_migration(self, migration, instance, network_info): """Confirms a resize, destroying the source VM.""" instance_name = self._get_orig_vm_name_label(instance) - # Destroy the original VM. - vm_ref = vm_util.get_vm_ref_from_uuid(self._session, instance['uuid']) - if vm_ref is None: - vm_ref = vm_util.get_vm_ref_from_name(self._session, instance_name) + # Destroy the original VM. The vm_ref is via the instance_name + # and not the UUID + vm_ref = vm_util.get_vm_ref_from_name(self._session, instance_name) if vm_ref is None: LOG.debug(_("instance not present"), instance=instance) return @@ -1192,7 +1192,7 @@ class VMwareVMOps(object): destroy_task = self._session._call_method( self._session._get_vim(), "Destroy_Task", vm_ref) - self._session._wait_for_task(instance['uuid'], destroy_task) + self._session._wait_for_task(instance_name, destroy_task) LOG.debug(_("Destroyed the VM"), instance=instance) except Exception as excep: LOG.warn(_("In vmwareapi:vmops:confirm_migration, got this " @@ -1223,6 +1223,16 @@ class VMwareVMOps(object): network_info, image_meta, resize_instance=False, block_device_info=None, power_on=True): """Completes a resize, turning on the migrated instance.""" + if resize_instance: + client_factory = self._session._get_vim().client.factory + vm_ref = vm_util.get_vm_ref(self._session, instance) + vm_resize_spec = vm_util.get_vm_resize_spec(client_factory, + instance) + reconfig_task = self._session._call_method( + self._session._get_vim(), + "ReconfigVM_Task", vm_ref, + spec=vm_resize_spec) + # 4. Start VM if power_on: self._power_on(instance) @@ -1430,8 +1440,9 @@ class VMwareVMOps(object): "HostSystem", ["name"]) vm_util._cancel_retrieve_if_necessary(self._session, host_objs) for host in host_objs: - if host.propSet[0].val == host_name: - return host.obj + if hasattr(host, 'propSet'): + if host.propSet[0].val == host_name: + return host.obj return None def _get_vmfolder_ref(self):