From 1ba150fa5233ebeacdc02e89b2e83a1a6861bc19 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Tue, 31 Jul 2018 21:22:52 +0100 Subject: [PATCH] libvirt: fix disk_bus handling for root disk - The hw_disk_bus image metadata key allows users to specify the bus to which guest hard disk images are attached for libvirt instances. - In commit 7be531fe9462f2b07d4a1abf6687f649d1dfbb89 the libvirt disk mappings were refactored. As part of that change a lossy conversion was introduced which resulted in multiple disk buses being mapped to the same block device prefix. - As a result of this lossy mapping hw_disk_bus=sata and hw_disk_bus=usb were mapped to the same prefix as scsi "sd". this resulted in the request to use sata or usb root disk being ignored. - This change fixes handling of the root disk device bus selection in libvirt blockinfo.py by using the disk bus provided instead of inferring it from the block device name. - This change adds a new unit test to assert the correct handeling of the hw_disk_bus image metadata key in the libvirt driver. Change-Id: I63836f461507f4924dc8f3491ef518a4ce4d32f9 Closes-Bug: 1759420 --- .../tests/unit/virt/libvirt/test_blockinfo.py | 28 ++++++------------- nova/tests/unit/virt/libvirt/test_driver.py | 14 ++++++++++ nova/virt/libvirt/blockinfo.py | 5 +--- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_blockinfo.py b/nova/tests/unit/virt/libvirt/test_blockinfo.py index 33441bc0344..022a85abb9a 100644 --- a/nova/tests/unit/virt/libvirt/test_blockinfo.py +++ b/nova/tests/unit/virt/libvirt/test_blockinfo.py @@ -210,14 +210,13 @@ def test_get_disk_mapping_simple_rootdev(self): "virtio", "ide", image_meta, block_device_info) - expect = { - 'disk': {'bus': 'scsi', 'dev': 'sda', + 'disk': {'bus': 'virtio', 'dev': 'sda', 'type': 'disk', 'boot_index': '1'}, 'disk.local': {'bus': 'virtio', 'dev': 'vda', 'type': 'disk'}, - 'root': {'bus': 'scsi', 'dev': 'sda', + 'root': {'bus': 'virtio', 'dev': 'sda', 'type': 'disk', 'boot_index': '1'} - } + } self.assertEqual(expect, mapping) def test_get_disk_mapping_rescue(self): @@ -934,38 +933,29 @@ def test_get_device_name(self): @mock.patch('nova.virt.libvirt.blockinfo.find_disk_dev_for_disk_bus', return_value='vda') - @mock.patch('nova.virt.libvirt.blockinfo.get_disk_bus_for_disk_dev', - return_value='virtio') - def test_get_root_info_no_bdm(self, mock_get_bus, mock_find_dev): + def test_get_root_info_no_bdm(self, mock_find_dev): instance = objects.Instance(**self.test_instance) image_meta = objects.ImageMeta.from_dict(self.test_image_meta) - blockinfo.get_root_info(instance, 'kvm', image_meta, None, + info = blockinfo.get_root_info(instance, 'kvm', image_meta, None, 'virtio', 'ide') mock_find_dev.assert_called_once_with({}, 'virtio') - blockinfo.get_root_info(instance, 'kvm', image_meta, None, - 'virtio', 'ide', root_device_name='/dev/vda') - mock_get_bus.assert_called_once_with('kvm', '/dev/vda') + self.assertEqual('virtio', info['bus']) @mock.patch('nova.virt.libvirt.blockinfo.find_disk_dev_for_disk_bus', return_value='vda') - @mock.patch('nova.virt.libvirt.blockinfo.get_disk_bus_for_disk_dev', - return_value='virtio') - def test_get_root_info_no_bdm_empty_image_meta(self, mock_get_bus, - mock_find_dev): + def test_get_root_info_no_bdm_empty_image_meta(self, mock_find_dev): # The evacuate operation passes image_ref=None to the compute node for # rebuild which then defaults image_meta to {}, so we don't have any # attributes in the ImageMeta object passed to get_root_info and we # need to make sure we don't try lazy-loading anything. instance = objects.Instance(**self.test_instance) image_meta = objects.ImageMeta.from_dict({}) - blockinfo.get_root_info(instance, 'kvm', image_meta, None, + info = blockinfo.get_root_info(instance, 'kvm', image_meta, None, 'virtio', 'ide') mock_find_dev.assert_called_once_with({}, 'virtio') - blockinfo.get_root_info(instance, 'kvm', image_meta, None, - 'virtio', 'ide', root_device_name='/dev/vda') - mock_get_bus.assert_called_once_with('kvm', '/dev/vda') + self.assertEqual('virtio', info['bus']) @mock.patch('nova.virt.libvirt.blockinfo.get_info_from_bdm') def test_get_root_info_bdm(self, mock_get_info): diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index eac86e44266..5454de6cefb 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -7107,6 +7107,20 @@ def test_xml_disk_bus_ide(self): None, (expec_val,)) + def test_xml_disk_bus_sata(self): + # NOTE(sean-k-mooney): here we assert that when + # root_device_name is set in the block_device_info + # and hw_disk_bus is set in the image properties, + # we use the property value + expected = ("disk", "sata", "vda") + + image_meta = objects.ImageMeta.from_dict({"properties": { + "hw_disk_bus": "sata"}}) + block_device_info = {'root_device_name': "vda"} + self._check_xml_and_disk_bus(image_meta, + block_device_info, + (expected,)) + def test_xml_disk_bus_ide_and_virtio(self): # It's necessary to check if the architecture is power, because # power doesn't have support to ide, and so libvirt translate diff --git a/nova/virt/libvirt/blockinfo.py b/nova/virt/libvirt/blockinfo.py index 0dfea1c19c6..429dc87f85b 100644 --- a/nova/virt/libvirt/blockinfo.py +++ b/nova/virt/libvirt/blockinfo.py @@ -438,10 +438,7 @@ def get_root_info(instance, virt_type, image_meta, root_bdm, else: root_device_bus = disk_bus root_device_type = 'disk' - if root_device_name: - root_device_bus = get_disk_bus_for_disk_dev(virt_type, - root_device_name) - else: + if not root_device_name: root_device_name = find_disk_dev_for_disk_bus({}, root_device_bus) return {'bus': root_device_bus,