From bbc562c572f328cc6ffa9cdbff83ae64672fe5b8 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Fri, 1 May 2020 14:20:04 +0100 Subject: [PATCH] compute: Validate a BDMs disk_bus when provided Previously disk_bus values were never validated and could easily end up being ignored by the underlying virt driver and hypervisor. For example, a common mistake made by users is to request a virtio-scsi disk_bus when using the libvirt virt driver. This however isn't a valid bus and is ignored, defaulting back to the virtio (virtio-blk) bus. This change adds a simple validation in the compute API using the potential disk_bus values provided by the DiskBus field class as used when validating the hw_*_bus image properties. Conflicts: nova/tests/unit/compute/test_compute_api.py NOTE(lyarwood): Conflict as If9c459a9a0aa752c478949e4240286cbdb146494 is not present in stable/train. test_validate_bdm_disk_bus is also updated as Ib31ba2cbff0ebb22503172d8801b6e0c3d2aa68a is not present in stable/train. Closes-Bug: #1876301 Change-Id: I77b28b9cc8f99b159f628f4655d85ff305a71db8 (cherry picked from commit 5913bd889f9d3dfc8d154415e666c821054c229d) (cherry picked from commit fb31ae430a2e4f8869e77e31ea0d6a9478f6aa61) --- api-ref/source/parameters.yaml | 7 ++++--- nova/api/openstack/compute/servers.py | 1 + nova/compute/api.py | 7 +++++++ nova/exception.py | 5 +++++ nova/objects/fields.py | 2 ++ .../api/openstack/compute/test_serversV21.py | 3 ++- nova/tests/unit/compute/test_compute_api.py | 16 ++++++++++++++++ 7 files changed, 37 insertions(+), 4 deletions(-) diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 2a0858f09ab6..8bc699f575e8 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -2449,9 +2449,10 @@ disk_available_least_total: disk_bus: description: | Disk bus type, some hypervisors (currently only libvirt) support - specify this parameter. Some example disk_bus values can be: `ide`, - `usb`, `virtio`, `scsi`. This is not an exhaustive list as it depends - on the virtualization driver, and may change as more support is added. + specify this parameter. Some example disk_bus values can be: ``fdc``, + ``ide``, ``sata``, ``scsi``, ``usb``, ``virtio``, ``xen``, ``lxc`` + and ``uml``. Support for each bus type depends on the virtualization driver + and underlying hypervisor. in: body required: false type: string diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 42308af93123..75e145245dc1 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -762,6 +762,7 @@ class ServersController(wsgi.Controller): exception.InvalidBDMEphemeralSize, exception.InvalidBDMFormat, exception.InvalidBDMSwapSize, + exception.InvalidBDMDiskBus, exception.VolumeTypeNotFound, exception.AutoDiskConfigDisabledByImage, exception.InstanceGroupNotFound, diff --git a/nova/compute/api.py b/nova/compute/api.py index 69837f62d760..803c9df9922f 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1664,6 +1664,13 @@ class API(base.Base): "(source: 'blank', dest: 'volume') need to have non-zero " "size")) + # NOTE(lyarwood): Ensure the disk_bus is at least known to Nova. + # The virt driver may reject this later but for now just ensure + # it's listed as an acceptable value of the DiskBus field class. + disk_bus = bdm.disk_bus if 'disk_bus' in bdm else None + if disk_bus and disk_bus not in fields_obj.DiskBus.ALL: + raise exception.InvalidBDMDiskBus(disk_bus=disk_bus) + ephemeral_size = sum(bdm.volume_size or instance_type['ephemeral_gb'] for bdm in block_device_mappings if block_device.new_format_is_ephemeral(bdm)) diff --git a/nova/exception.py b/nova/exception.py index c98dc0b15dcd..5fcd3f0baf08 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -254,6 +254,11 @@ class TooManyDiskDevices(InvalidBDM): code = 403 +class InvalidBDMDiskBus(InvalidBDM): + msg_fmr = _("Block Device Mapping is invalid: The provided disk bus " + "%(disk_bus)s is not valid.") + + class InvalidAttribute(Invalid): msg_fmt = _("Attribute not supported: %(attr)s") diff --git a/nova/objects/fields.py b/nova/objects/fields.py index a768d0236afc..366a970737ce 100644 --- a/nova/objects/fields.py +++ b/nova/objects/fields.py @@ -339,6 +339,8 @@ class DiskBus(BaseNovaEnum): # NOTE(aspiers): If you change this, don't forget to update the # docs and metadata for hw_*_bus in glance. + # NOTE(lyarwood): Also update the possible values in the api-ref for the + # block_device_mapping_v2.disk_bus parameter. FDC = "fdc" IDE = "ide" SATA = "sata" diff --git a/nova/tests/unit/api/openstack/compute/test_serversV21.py b/nova/tests/unit/api/openstack/compute/test_serversV21.py index 80af96ebb6d0..d5736ceccefd 100644 --- a/nova/tests/unit/api/openstack/compute/test_serversV21.py +++ b/nova/tests/unit/api/openstack/compute/test_serversV21.py @@ -4993,7 +4993,8 @@ class ServersControllerCreateTest(test.TestCase): (exception.InvalidBDMVolume, {'id': 'fake'}), (exception.InvalidBDMImage, {'id': 'fake'}), (exception.InvalidBDMBootSequence, {}), - (exception.InvalidBDMLocalsLimit, {})) + (exception.InvalidBDMLocalsLimit, {}), + (exception.InvalidBDMDiskBus, {'disk_bus': 'foo'})) ex_iter = iter(bdm_exceptions) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index d17a9b54925f..f1b1a193105f 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4469,6 +4469,22 @@ class _ComputeAPIUnitTestMixIn(object): bdms) mock_get_image.assert_called_once_with(self.context, image_id) + @mock.patch('nova.compute.api.API._get_image') + def test_validate_bdm_disk_bus(self, mock_get_image): + """Tests that _validate_bdm fail if an invalid disk_bus is provided + """ + instance = self._create_instance_obj() + bdms = objects.BlockDeviceMappingList(objects=[ + objects.BlockDeviceMapping( + boot_index=0, image_id=instance.image_ref, + source_type='image', destination_type='volume', + volume_type=None, snapshot_id=None, volume_id=None, + volume_size=1, disk_bus='virtio-scsi')]) + self.assertRaises(exception.InvalidBDMDiskBus, + self.compute_api._validate_bdm, + self.context, instance, objects.Flavor(), + bdms) + @mock.patch.object(objects.service, 'get_minimum_version_all_cells', return_value=compute_api.MIN_COMPUTE_VOLUME_TYPE) def test_the_specified_volume_type_id_assignment_to_name(