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 7be531f
  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

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):

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

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,