From 942b9cd7f2ab77b7e790831cc985a9f6d2730318 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Mon, 30 Jun 2014 15:52:25 +0100 Subject: [PATCH] VMware: Remove references to ebs_root from spawn() spawn() contained an important boolean called ebs_root. This was True for any spawn which contained a block device mapping. This no longer has any meaning, and was confusing. This change removes references to ebs_root, instead using block_device_info directly. N.B. The block device mapping behaviour of the VMware driver is still not correct, but fixing it is not the purpose of this patch. This patch simply makes the existing behaviour easier to see. N.B. test_spawn_mask_block_device_info_password in test_vmops.py assumes internal details of spawn in order to cause a failure after emitting a log message. In moving the log message we need to update the dance this test does to ensure that spawn fails after the log message, but not before. This change has been split out of https://review.openstack.org/#/c/87002/, which was written by Shawn Hartsock. partial blueprint vmware-spawn-refactor Co-authored-by: Shawn Hartsock Change-Id: I11b355a6b53194bc9f85f542b54c9fbb9c061e04 --- nova/tests/virt/vmwareapi/test_vmops.py | 39 ++++++++++++------ nova/virt/vmwareapi/vmops.py | 53 +++++++++++++++---------- 2 files changed, 60 insertions(+), 32 deletions(-) diff --git a/nova/tests/virt/vmwareapi/test_vmops.py b/nova/tests/virt/vmwareapi/test_vmops.py index 4f92ccacfa23..034bef43a849 100644 --- a/nova/tests/virt/vmwareapi/test_vmops.py +++ b/nova/tests/virt/vmwareapi/test_vmops.py @@ -520,30 +520,45 @@ class VMwareVMOpsTestCase(test.NoDBTestCase): def test_finish_revert_migration_power_off(self): self._test_finish_revert_migration(power_on=False) - def test_spawn_mask_block_device_info_password(self): + @mock.patch.object(vmops.LOG, 'debug') + @mock.patch('nova.virt.vmwareapi.volumeops.VMwareVolumeOps' + '.attach_root_volume') + def test_spawn_mask_block_device_info_password(self, + mock_attach_root_volume, + mock_debug): # Very simple test that just ensures block_device_info auth_password # is masked when logged; the rest of the test just fails out early. data = {'auth_password': 'scrubme'} bdm = [{'connection_info': {'data': data}}] bdi = {'block_device_mapping': bdm} + self.password_logged = False + # Tests that the parameters to the to_xml method are sanitized for # passwords when logged. def fake_debug(*args, **kwargs): if 'auth_password' in args[0]: + self.password_logged = True self.assertNotIn('scrubme', args[0]) - with mock.patch.object(vmops.LOG, 'debug', - side_effect=fake_debug) as debug_mock: - # the invalid disk format will cause an exception - image_meta = {'disk_format': 'fake'} - self.assertRaises(exception.InvalidDiskFormat, self._vmops.spawn, - self._context, self._instance, image_meta, - injected_files=None, admin_password=None, - network_info=[], block_device_info=bdi) - # we don't care what the log message is, we just want to make sure - # our stub method is called which asserts the password is scrubbed - self.assertTrue(debug_mock.called) + mock_debug.side_effect = fake_debug + self.flags(flat_injected=False, vnc_enabled=False) + mock_attach_root_volume.side_effect = Exception + + # Call spawn(). We don't care what it does as long as it generates + # the log message, which we check below. + try: + self._vmops.spawn( + self._context, self._instance, {}, + injected_files=None, admin_password=None, + network_info=[], block_device_info=bdi + ) + except Exception: + pass + + # Check that the relevant log message was generated, and therefore + # that we checked it was scrubbed + self.assertTrue(self.password_logged) def test_get_ds_browser(self): cache = self._vmops._datastore_browser_mapping diff --git a/nova/virt/vmwareapi/vmops.py b/nova/virt/vmwareapi/vmops.py index 1c1c4681097d..642d035945dd 100644 --- a/nova/virt/vmwareapi/vmops.py +++ b/nova/virt/vmwareapi/vmops.py @@ -186,16 +186,11 @@ class VMwareVMOps(object): #. Power on the VM. """ - ebs_root = False - if block_device_info: - msg = "Block device information present: %s" % block_device_info - # NOTE(mriedem): block_device_info can contain an auth_password - # so we have to scrub the message before logging it. - LOG.debug(logging.mask_password(msg), instance=instance) - block_device_mapping = driver.block_device_info_get_mapping( - block_device_info) - if block_device_mapping: - ebs_root = True + + # NOTE(hartsocks): some of the logic below relies on instance_name + # even when it is not set by the caller. + if instance_name is None: + instance_name = instance.uuid client_factory = self._session._get_vim().client.factory datastore = ds_util.get_datastore( @@ -247,9 +242,34 @@ class VMwareVMOps(object): vnc_port = vm_util.get_vnc_port(self._session) self._set_vnc_config(client_factory, instance, vnc_port) - if not ebs_root: - # this logic allows for instances or images to decide - # for themselves which strategy is best for them. + block_device_mapping = [] + if block_device_info is not None: + block_device_mapping = driver.block_device_info_get_mapping( + block_device_info) + + # NOTE(mdbooth): the logic here is that we ignore the image if there + # are block device mappings. This behaviour is incorrect, and a bug in + # the driver. We should be able to accept an image and block device + # mappings. + if len(block_device_mapping) > 0: + msg = "Block device information present: %s" % block_device_info + # NOTE(mriedem): block_device_info can contain an auth_password + # so we have to scrub the message before logging it. + LOG.debug(logging.mask_password(msg), instance=instance) + + for root_disk in block_device_mapping: + connection_info = root_disk['connection_info'] + # TODO(hartsocks): instance is unnecessary, remove it + # we still use instance in many locations for no other purpose + # than logging, can we simplify this? + self._volumeops.attach_root_volume(connection_info, instance, + self._default_root_device, + datastore.ref) + else: + # TODO(hartsocks): Refactor this section image handling section. + # The next section handles manipulating various image types + # as well as preparing those image's virtual disks for mounting + # in our virtual machine. upload_name = instance.image_ref upload_folder = '%s/%s' % (self._base_folder, upload_name) @@ -514,13 +534,6 @@ class VMwareVMOps(object): datastore.ref, str(uploaded_iso_path)) - else: - # Attach the root disk to the VM. - for root_disk in block_device_mapping: - connection_info = root_disk['connection_info'] - self._volumeops.attach_root_volume(connection_info, instance, - self._default_root_device, - datastore.ref) if power_on: vm_util.power_on_instance(self._session, instance, vm_ref=vm_ref)