From 286d7cfc5c45535561b6ed6e7906bb40de5abc93 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Wed, 20 Nov 2019 00:13:03 +0000 Subject: [PATCH 01/54] add [libvirt]/max_queues config option This change adds a max_queues config option to allow operators to set the maximium number of virtio queue pairs that can be allocated to a virtio network interface. Change-Id: I9abe783a9a9443c799e7c74a57cc30835f679a01 Closes-Bug: #1847367 (cherry picked from commit 0e6aac3c2d97c999451da50537df6a0cbddeb4a6) --- nova/conf/libvirt.py | 8 ++++++++ nova/tests/unit/virt/libvirt/test_vif.py | 15 +++++++++++++++ nova/virt/libvirt/vif.py | 7 +++++++ .../notes/virtio-max-queues-27f73e988c7e66ba.yaml | 15 +++++++++++++++ 4 files changed, 45 insertions(+) create mode 100644 releasenotes/notes/virtio-max-queues-27f73e988c7e66ba.yaml diff --git a/nova/conf/libvirt.py b/nova/conf/libvirt.py index 0ee880af477..1364c8ed0b4 100644 --- a/nova/conf/libvirt.py +++ b/nova/conf/libvirt.py @@ -1319,6 +1319,14 @@ This option is only usable for virtio-net device with vhost-user backend. Available only with QEMU/KVM. Requires libvirt v3.7 QEMU v2.10."""), + cfg.IntOpt('max_queues', default=None, min=1, help=""" +The maximum number of virtio queue pairs that can be enabled +when creating a multiqueue guest. The number of virtio queues +allocated will be the lesser of the CPUs requested by the guest +and the max value defined. By default, this value is set to none +meaning the legacy limits based on the reported kernel +major version will be used. +"""), ] diff --git a/nova/tests/unit/virt/libvirt/test_vif.py b/nova/tests/unit/virt/libvirt/test_vif.py index 90a91f49a74..4fc9127180a 100644 --- a/nova/tests/unit/virt/libvirt/test_vif.py +++ b/nova/tests/unit/virt/libvirt/test_vif.py @@ -657,6 +657,21 @@ def test_virtio_multiqueue_in_kernel_3(self, mock_uname): def test_virtio_multiqueue_in_kernel_4(self, mock_uname): self._test_virtio_multiqueue(10, '10') + @mock.patch('os.uname', return_value=('Linux', '', '2.6.32-21-generic')) + def test_virtio_multiqueue_in_kernel_2_max_queues(self, mock_uname): + self.flags(max_queues=2, group='libvirt') + self._test_virtio_multiqueue(10, '2') + + @mock.patch('os.uname', return_value=('Linux', '', '3.19.0-47-generic')) + def test_virtio_multiqueue_in_kernel_3_max_queues(self, mock_uname): + self.flags(max_queues=2, group='libvirt') + self._test_virtio_multiqueue(10, '2') + + @mock.patch('os.uname', return_value=('Linux', '', '4.2.0-35-generic')) + def test_virtio_multiqueue_in_kernel_4_max_queues(self, mock_uname): + self.flags(max_queues=2, group='libvirt') + self._test_virtio_multiqueue(10, '2') + def test_vhostuser_os_vif_multiqueue(self): d = vif.LibvirtGenericVIFDriver() hostimpl = host.Host("qemu:///system") diff --git a/nova/virt/libvirt/vif.py b/nova/virt/libvirt/vif.py index d3a2a26dbab..4bfc9d7240a 100644 --- a/nova/virt/libvirt/vif.py +++ b/nova/virt/libvirt/vif.py @@ -225,6 +225,13 @@ def _get_virtio_mq_settings(self, image_meta, flavor): return (driver, vhost_queues) def _get_max_tap_queues(self): + # Note(sean-k-mooney): some linux distros have backported + # changes for newer kernels which make the kernel version + # number unreliable to determine the max queues supported + # To address this without making the code distro dependent + # we introduce a new config option and prefer it if set. + if CONF.libvirt.max_queues: + return CONF.libvirt.max_queues # NOTE(kengo.sakai): In kernels prior to 3.0, # multiple queues on a tap interface is not supported. # In kernels 3.x, the number of queues on a tap interface diff --git a/releasenotes/notes/virtio-max-queues-27f73e988c7e66ba.yaml b/releasenotes/notes/virtio-max-queues-27f73e988c7e66ba.yaml new file mode 100644 index 00000000000..845f0c258fd --- /dev/null +++ b/releasenotes/notes/virtio-max-queues-27f73e988c7e66ba.yaml @@ -0,0 +1,15 @@ +--- +other: + - | + The nova libvirt virt driver supports creating instances with multi-queue + virtio network interfaces. In previous releases nova has based the maximum + number of virtio queue pairs that can be allocated on the reported kernel + major version. It has been reported in `bug #1847367`_ that some distros have + backported changes from later major versions that make major version + number no longer suitable to determine the maximum virtio queue pair count. + A new config option has been added to the libvirt section of the nova.conf. + When defined nova will now use the ``[libvirt]/max_queues`` option to + define the max queues that can be configured, if undefined it will + fallback to the previous kernel version approach. + + .. _`bug #1847367`: https://bugs.launchpad.net/nova/+bug/1847367 From 44676ddf843ba84e26721cd2e3f65dc45a881f66 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 30 Jul 2020 17:36:24 +0100 Subject: [PATCH 02/54] hardware: Reject requests for no hyperthreads on hosts with HT Attempting to boot an instance with 'hw:cpu_policy=dedicated' will result in a request from nova-scheduler to placement for allocation candidates with $flavor.vcpu 'PCPU' inventory. Similarly, booting an instance with 'hw:cpu_thread_policy=isolate' will result in a request for allocation candidates with 'HW_CPU_HYPERTHREADING=forbidden', i.e. hosts without hyperthreading. This has been the case since the cpu-resources feature was implemented in Train. However, as part of that work and to enable upgrades from hosts that predated Train, we also make a second request for candidates with $flavor.vcpu 'VCPU' inventory. The idea behind this is that old compute nodes would only report 'VCPU' and should be useable, and any new compute nodes that got caught up in this second request could never actually be scheduled to since there wouldn't be enough cores from 'ComputeNode.numa_topology.cells.[*].pcpuset' available to schedule to, resulting in rejection by the 'NUMATopologyFilter'. However, if a host was rejected in the first query because it reported the 'HW_CPU_HYPERTHREADING' trait, it could get picked up by the second query and would happily be scheduled to, resulting in an instance consuming 'VCPU' inventory from a host that properly supported 'PCPU' inventory. The solution is simply, though also a huge hack. If we detect that the host is using new style configuration and should be able to report 'PCPU', check if the instance asked for no hyperthreading and whether the host has it. If all are True, reject the request. Change-Id: Id39aaaac09585ca1a754b669351c86e234b89dd9 Signed-off-by: Stephen Finucane Closes-Bug: #1889633 (cherry picked from commit 9c270332041d6b98951c0b57d7b344fd551a413c) (cherry picked from commit 7ddab327675d36a4ba59d02d22d042d418236336) --- .../functional/libvirt/test_numa_servers.py | 7 +- nova/tests/unit/virt/test_hardware.py | 75 ++++++++++++++++--- nova/virt/hardware.py | 24 ++++++ .../notes/bug-1889633-37e524fb6c20fbdf.yaml | 9 +++ 4 files changed, 99 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/bug-1889633-37e524fb6c20fbdf.yaml diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index 9affbe9c198..6f23c02a8c0 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -343,12 +343,7 @@ def test_create_server_with_isolate_thread_policy_fails(self): } flavor_id = self._create_flavor(vcpu=2, extra_spec=extra_spec) - # FIXME(stephenfin): This should go to error status since there should - # not be a host available - expected_usage = { - 'DISK_GB': 20, 'MEMORY_MB': 2048, 'PCPU': 0, 'VCPU': 2, - } - self._run_build_test(flavor_id, expected_usage=expected_usage) + self._run_build_test(flavor_id, end_status='ERROR') def test_create_server_with_pcpu(self): """Create a server using an explicit 'resources:PCPU' request. diff --git a/nova/tests/unit/virt/test_hardware.py b/nova/tests/unit/virt/test_hardware.py index 9201177fe27..772f05b2188 100644 --- a/nova/tests/unit/virt/test_hardware.py +++ b/nova/tests/unit/virt/test_hardware.py @@ -3020,10 +3020,13 @@ def test_get_pinning_host_siblings_large_instance_odd_fit(self): got_topo = objects.VirtCPUTopology(sockets=1, cores=5, threads=1) self.assertEqualTopology(got_topo, inst_pin.cpu_topology) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_get_pinning_isolate_policy_too_few_fully_free_cores(self): host_pin = objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3]), pcpuset=set([0, 1, 2, 3]), memory=4096, memory_usage=0, @@ -3038,10 +3041,13 @@ def test_get_pinning_isolate_policy_too_few_fully_free_cores(self): inst_pin = hw._numa_fit_instance_cell_with_pinning(host_pin, inst_pin) self.assertIsNone(inst_pin) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_get_pinning_isolate_policy_no_fully_free_cores(self): host_pin = objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3]), pcpuset=set([0, 1, 2, 3]), memory=4096, memory_usage=0, @@ -3056,10 +3062,13 @@ def test_get_pinning_isolate_policy_no_fully_free_cores(self): inst_pin = hw._numa_fit_instance_cell_with_pinning(host_pin, inst_pin) self.assertIsNone(inst_pin) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_get_pinning_isolate_policy_fits(self): host_pin = objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3]), pcpuset=set([0, 1, 2, 3]), memory=4096, memory_usage=0, @@ -3076,10 +3085,13 @@ def test_get_pinning_isolate_policy_fits(self): got_topo = objects.VirtCPUTopology(sockets=1, cores=2, threads=1) self.assertEqualTopology(got_topo, inst_pin.cpu_topology) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_get_pinning_isolate_policy_fits_ht_host(self): host_pin = objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3]), pcpuset=set([0, 1, 2, 3]), memory=4096, memory_usage=0, @@ -3096,10 +3108,13 @@ def test_get_pinning_isolate_policy_fits_ht_host(self): got_topo = objects.VirtCPUTopology(sockets=1, cores=2, threads=1) self.assertEqualTopology(got_topo, inst_pin.cpu_topology) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_get_pinning_isolate_policy_fits_w_usage(self): host_pin = objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3, 4, 5, 6, 7]), pcpuset=set([0, 1, 2, 3, 4, 5, 6, 7]), memory=4096, memory_usage=0, @@ -3116,6 +3131,38 @@ def test_get_pinning_isolate_policy_fits_w_usage(self): got_topo = objects.VirtCPUTopology(sockets=1, cores=2, threads=1) self.assertEqualTopology(got_topo, inst_pin.cpu_topology) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set + @mock.patch.object(hw, 'LOG') + def test_get_pinning_isolate_policy_bug_1889633(self, mock_log): + host_pin = objects.NUMACell( + id=0, + cpuset={0, 1, 4, 5}, + pcpuset={2, 3, 6, 7}, + memory=4096, + memory_usage=0, + pinned_cpus=set(), + siblings=[{0, 4}, {1, 5}, {2, 6}, {3, 7}], + mempages=[], + ) + inst_pin = objects.InstanceNUMACell( + cpuset=set(), + pcpuset={0, 1}, + memory=2048, + cpu_policy=fields.CPUAllocationPolicy.DEDICATED, + cpu_thread_policy=fields.CPUThreadAllocationPolicy.ISOLATE, + ) + limits = objects.NUMATopologyLimits( + cpu_allocation_ratio=2, ram_allocation_ratio=2, + ) + + # host has hyperthreads, which means no NUMA topology should be built + inst_topo = hw._numa_fit_instance_cell(host_pin, inst_pin, limits) + self.assertIsNone(inst_topo) + self.assertIn( + 'Host supports hyperthreads, but instance requested no', + mock_log.warning.call_args[0][0], + ) + class CPUPinningTestCase(test.NoDBTestCase, _CPUPinningTestCaseBase): def test_host_numa_fit_instance_to_host_single_cell(self): @@ -3823,11 +3870,14 @@ def test_isolate_full_usage(self): self.assertEqual(set([0, 1]), host_topo.cells[0].pinned_cpus) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_isolate_w_isolate_thread_alloc(self): host_topo = objects.NUMATopology(cells=[ objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3, 4, 5]), pcpuset=set([0, 1, 2, 3, 4, 5]), memory=2048, cpu_usage=0, @@ -3850,11 +3900,14 @@ def test_isolate_w_isolate_thread_alloc(self): self.assertEqual({0: 0, 1: 2}, inst_topo.cells[0].cpu_pinning) self.assertEqual(set([4]), inst_topo.cells[0].cpuset_reserved) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_isolate_w_isolate_thread_alloc_usage(self): host_topo = objects.NUMATopology(cells=[ objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([0, 1, 2, 3, 4, 5]), pcpuset=set([0, 1, 2, 3, 4, 5]), memory=2048, cpu_usage=0, @@ -3910,11 +3963,14 @@ def test_asymmetric_host(self): self.assertEqual({0: 2, 1: 3}, inst_topo.cells[0].cpu_pinning) self.assertEqual(set([1]), inst_topo.cells[0].cpuset_reserved) + # TODO(stephenfin): Remove when we drop support for vcpu_pin_set def test_asymmetric_host_w_isolate_thread_alloc(self): host_topo = objects.NUMATopology(cells=[ objects.NUMACell( id=0, - cpuset=set(), + # Simulate a legacy host with vcpu_pin_set configuration, + # meaning cpuset == pcpuset + cpuset=set([1, 2, 3, 4, 5]), pcpuset=set([1, 2, 3, 4, 5]), memory=2048, cpu_usage=0, @@ -3924,8 +3980,7 @@ def test_asymmetric_host_w_isolate_thread_alloc(self): mempages=[objects.NUMAPagesTopology( size_kb=4, total=524288, used=0)])]) inst_topo = objects.InstanceNUMATopology( - emulator_threads_policy=( - fields.CPUEmulatorThreadsPolicy.ISOLATE), + emulator_threads_policy=fields.CPUEmulatorThreadsPolicy.ISOLATE, cells=[objects.InstanceNUMACell( id=0, cpuset=set([0, 1]), memory=2048, diff --git a/nova/virt/hardware.py b/nova/virt/hardware.py index 4a275b6beb0..1a22c6b5b17 100644 --- a/nova/virt/hardware.py +++ b/nova/virt/hardware.py @@ -902,6 +902,30 @@ def _get_reserved(sibling_set, vcpus_pinning, num_cpu_reserved=0, 'for the isolate policy without this.') return + # TODO(stephenfin): Drop this when we drop support for 'vcpu_pin_set' + # NOTE(stephenfin): This is total hack. We're relying on the fact that + # the libvirt driver, which is the only one that currently supports + # pinned CPUs, will set cpuset and pcpuset to the same value if using + # legacy configuration, i.e. 'vcpu_pin_set', as part of + # '_get_host_numa_topology'. They can't be equal otherwise since + # 'cpu_dedicated_set' and 'cpu_shared_set' must be disjoint. Therefore, + # if these are equal, the host that this NUMA cell corresponds to is + # using legacy configuration and it's okay to use the old, "pin a core + # and reserve its siblings" implementation of the 'isolate' policy. If + # they're not, the host is using new-style configuration and we've just + # hit bug #1889633 + if threads_per_core != 1 and host_cell.pcpuset != host_cell.cpuset: + LOG.warning( + "Host supports hyperthreads, but instance requested no " + "hyperthreads. This should have been rejected by the " + "scheduler but we likely got here due to the fallback VCPU " + "query. Consider setting '[workarounds] " + "disable_fallback_pcpu_query' to 'True' once hosts are no " + "longer using 'vcpu_pin_set'. Refer to bug #1889633 for more " + "information." + ) + return + pinning = _get_pinning( 1, # we only want to "use" one thread per core sibling_sets[threads_per_core], diff --git a/releasenotes/notes/bug-1889633-37e524fb6c20fbdf.yaml b/releasenotes/notes/bug-1889633-37e524fb6c20fbdf.yaml new file mode 100644 index 00000000000..77c86013e07 --- /dev/null +++ b/releasenotes/notes/bug-1889633-37e524fb6c20fbdf.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + An issue that could result in instances with the ``isolate`` thread policy + (``hw:cpu_thread_policy=isolate``) being scheduled to hosts with SMT + (HyperThreading) and consuming ``VCPU`` instead of ``PCPU`` has been + resolved. See `bug #1889633`__ for more information. + + .. __: https://bugs.launchpad.net/nova/+bug/1889633 From bbc562c572f328cc6ffa9cdbff83ae64672fe5b8 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Fri, 1 May 2020 14:20:04 +0100 Subject: [PATCH 03/54] 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 2a0858f09ab..8bc699f575e 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 42308af9312..75e145245dc 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -762,6 +762,7 @@ def create(self, req, body): 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 69837f62d76..803c9df9922 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1664,6 +1664,13 @@ def _validate_bdm(self, context, instance, instance_type, "(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 c98dc0b15dc..5fcd3f0baf0 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 a768d0236af..366a970737c 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 80af96ebb6d..d5736ceccef 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 @@ def test_create_instance_bdm_api_validation_fails(self): (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 d17a9b54925..f1b1a193105 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4469,6 +4469,22 @@ def test_validate_bdm_missing_volume_size(self, mock_get_image): 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( From ed9eacff92af4897fe560a6e0313a6ca7b278728 Mon Sep 17 00:00:00 2001 From: zhangbailin Date: Thu, 3 Sep 2020 11:03:04 +0800 Subject: [PATCH 04/54] Add note and daxio version to the vPMEM document Make the spec of virtual persistent memory consistent with the contents of the admin manual, update the dependency of virtual persistent memory about daxio, and add NOTE for the tested kernel version. Closes-Bug: #1894022 Change-Id: I30539bb47c98a588b95c066a394949d60af9c520 (cherry picked from commit a8b0c6b456a9afdbdfab69daf8c0d3685f8e3084) (cherry picked from commit eae463ca1541dacdc7507899d25e7d3505194363) --- doc/source/admin/virtual-persistent-memory.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/source/admin/virtual-persistent-memory.rst b/doc/source/admin/virtual-persistent-memory.rst index 73cc0f3b98e..95ad9a942f8 100644 --- a/doc/source/admin/virtual-persistent-memory.rst +++ b/doc/source/admin/virtual-persistent-memory.rst @@ -31,12 +31,23 @@ The following are required to support the vPMEM feature: ``dax_pmem``, ``nd_pmem``, ``device_dax``, ``nd_btt`` +.. note:: + + NVDIMM support is present in the Linux Kernel v4.0 or newer. It is + recommended to use Kernel version 4.2 or later since `NVDIMM support + `_ + is enabled by default. We met some bugs in older versions, and we have + done all verification works with OpenStack on 4.18 version, so 4.18 + version and newer will probably guarantee its functionality. + * QEMU version >= 3.1.0 * Libvirt version >= 5.0.0 * `ndctl`_ version >= 62 +* daxio version >= 1.6 + The vPMEM feature has been verified under the software and hardware listed above. From d92fe4f3e69db51b7bbad52a1fe1740e37bade9a Mon Sep 17 00:00:00 2001 From: Arthur Dayne Date: Tue, 17 Sep 2019 19:08:59 +0800 Subject: [PATCH 05/54] libvirt:driver:Disallow AIO=native when 'O_DIRECT' is not available Because of the libvirt issue[1], there is a bug[2] that if we set cache mode whose write semantic is not O_DIRECT (.i.e unsafe, writeback or writethrough), there will be a problem with the volume drivers (.i.e nova.virt.libvirt.volume.LibvirtISCSIVolumeDriver, nova.virt.libvirt.volume.LibvirtNFSVolumeDriver and so on), which designate native io explicitly. That problem will generate a libvirt xml for the instance, whose content contains ``` ... ... ``` In turn, it will fail to start the instance or attach the disk. > When qemu is configured with a block device that has aio=native set, but > the cache mode doesn't use O_DIRECT (i.e. isn't cache=none/directsync or any > unnamed mode with explicit cache.direct=on), then the raw-posix block driver > for local files and block devices will silently fall back to aio=threads. > The blockdev-add interface rejects such combinations, but qemu can't > change the existing legacy interfaces that libvirt uses today. [1]: https://github.com/libvirt/libvirt/commit/058384003db776c580d0e5a3016a6384e8eb7b92 [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1086704 Closes-Bug: #1841363 Change-Id: If9acc054100a6733f3659a15dd9fc2d462e84d64 (cherry picked from commit af2405e1181d70cdf60bcd0e40b3e80f2db2e3a6) (cherry picked from commit 0bd58921a1fcaffcc4fac25f63434c9cab93b061) --- nova/tests/unit/virt/libvirt/test_driver.py | 45 +++++++++++++++++++ nova/virt/libvirt/driver.py | 16 +++++++ ...-io-is-not-supported-fe56014e9648a518.yaml | 14 ++++++ 3 files changed, 75 insertions(+) create mode 100644 releasenotes/notes/bug-1841363-fallback-to-threaded-io-when-native-io-is-not-supported-fe56014e9648a518.yaml diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 0b1f8961c55..f1ee842ac5a 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -17727,6 +17727,51 @@ def test_set_cache_mode_shareable(self): drvr._set_cache_mode(fake_conf) self.assertEqual('none', fake_conf.driver_cache) + def _make_fake_conf(self, cache=None): + if cache: + self.flags(disk_cachemodes=['block=' + cache], group='libvirt') + else: + self.flags(group='libvirt') + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) + fake_conf = FakeConfigGuestDisk() + fake_conf.source_type = 'block' + fake_conf.driver_io = 'native' + drvr._set_cache_mode(fake_conf) + return fake_conf + + def test_set_cache_mode_driver_io_writeback(self): + """Tests that when conf.driver_io is 'native' and driver_cache is + 'writeback', then conf.driver_io is forced to 'threads' + """ + fake_conf = self._make_fake_conf('writeback') + self.assertEqual('writeback', fake_conf.driver_cache) + self.assertEqual('threads', fake_conf.driver_io) + + def test_set_cache_mode_driver_io_writethrough(self): + """Tests that when conf.driver_io is 'native' and driver_cache is + 'writethrough', then conf.driver_io is forced to 'threads' + """ + fake_conf = self._make_fake_conf('writethrough') + self.assertEqual('writethrough', fake_conf.driver_cache) + self.assertEqual('threads', fake_conf.driver_io) + + def test_set_cache_mode_driver_io_unsafe(self): + """Tests that when conf.driver_io is 'native' and driver_cache is + 'unsafe', then conf.driver_io is forced to 'threads' + """ + fake_conf = self._make_fake_conf('unsafe') + self.assertEqual('unsafe', fake_conf.driver_cache) + self.assertEqual('threads', fake_conf.driver_io) + + def test_without_set_cache_mode_driver_io(self): + """Tests that when conf.driver_io is 'native' and driver_cache is + not set(this is default settings), then conf.driver_io is kept as + 'native' + """ + fake_conf = self._make_fake_conf() + self.assertIsNone(fake_conf.driver_cache) + self.assertEqual('native', fake_conf.driver_io) + def test_set_cache_mode_invalid_mode(self): self.flags(disk_cachemodes=['file=FAKE'], group='libvirt') drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 5043589073e..acea96a24b8 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -597,6 +597,22 @@ def _set_cache_mode(self, conf): driver_cache) conf.driver_cache = cache_mode + # NOTE(acewit): If the [libvirt]disk_cachemodes is set as + # `block=writeback` or `block=writethrough` or `block=unsafe`, + # whose correponding Linux's IO semantic is not O_DIRECT in + # file nova.conf, then it will result in an attachment failure + # because of the libvirt bug + # (https://bugzilla.redhat.com/show_bug.cgi?id=1086704) + if ((getattr(conf, 'driver_io', None) == "native") and + conf.driver_cache not in [None, 'none', 'directsync']): + conf.driver_io = "threads" + LOG.warning("The guest disk driver io mode has fallen back " + "from 'native' to 'threads' because the " + "disk cache mode is set as %(cachemode)s, which does" + "not use O_DIRECT. See the following bug report " + "for more details: https://launchpad.net/bugs/1841363", + {'cachemode': conf.driver_cache}) + def _do_quality_warnings(self): """Warn about potential configuration issues. diff --git a/releasenotes/notes/bug-1841363-fallback-to-threaded-io-when-native-io-is-not-supported-fe56014e9648a518.yaml b/releasenotes/notes/bug-1841363-fallback-to-threaded-io-when-native-io-is-not-supported-fe56014e9648a518.yaml new file mode 100644 index 00000000000..8ea68c97419 --- /dev/null +++ b/releasenotes/notes/bug-1841363-fallback-to-threaded-io-when-native-io-is-not-supported-fe56014e9648a518.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + Since Libvirt v.1.12.0 and the introduction of the `libvirt issue`_ , + there is a fact that if we set cache mode whose write semantic is not + O_DIRECT (i.e. "unsafe", "writeback" or "writethrough"), there will + be a problem with the volume drivers (i.e. LibvirtISCSIVolumeDriver, + LibvirtNFSVolumeDriver and so on), which designate native io explicitly. + + When the driver_cache (default is none) has been configured as neither + "none" nor "directsync", the libvirt driver will ensure the driver_io + to be "threads" to avoid an instance spawning failure. + + .. _`libvirt issue`: https://bugzilla.redhat.com/show_bug.cgi?id=1086704 From 7ace26e4bcd69a0a3d780cfae9f6745e4177b6c2 Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Fri, 21 Aug 2020 13:06:58 -0400 Subject: [PATCH 06/54] post live migration: don't call Neutron needlessly In bug 1879787, the call to network_api.get_instance_nw_info() in _post_live_migration() on the source compute manager eventually calls out to the Neutron REST API. If this fails, the exception is unhandled, and the migrating instance - which is fully running on the destination at this point - will never be updated in the database. This update normally happens later in post_live_migration_at_destination(). The network_info variable obtained from get_instance_nw_info() is used for two things: notifications - which aren't critical - and unplugging the instance's vifs on the source - which is very important! It turns out that at the time of the get_instance_nw_info() call, the network info in the instance info cache is still valid for unplugging the source vifs. The port bindings on the destination are only activated by the network_api.migrate_instance_start() [1] call that happens shortly *after* the problematic get_instance_nw_info() call. In other words, get_instance_nw_info() will always return the source ports. Because of that, we can replace it with a call to instance.get_network_info(). NOTE(artom) The functional test has been excised, as in stable/train the NeutronFixture does not properly support live migration with ports, making the test worthless. The work to support this was done as part of bp/support-move-ops-with-qos-ports-ussuri, and starts at commit b2734b5a9ae8b869fc9e8e229826343da3b47fcb. NOTE(artom) The test_post_live_migration_no_shared_storage_working_correctly and test_post_live_migration_cinder_v3_api unit tests had to be adjusted as part of the backport to pass with the new code. [1] https://opendev.org/openstack/nova/src/commit/d9e04c4ff0b1a9c3383f1848dc846e93030d83cb/nova/network/neutronv2/api.py#L2493-L2522 Change-Id: If0fbae33ce2af198188c91638afef939256c2556 Closes-bug: 1879787 (cherry picked from commit 6488a5dfb293831a448596e2084f484dd0bfa916) (cherry picked from commit 2c949cb3eea9cd9282060da12d32771582953aa2) --- nova/compute/manager.py | 8 +++++++- nova/tests/unit/compute/test_compute.py | 11 ++++++++--- nova/tests/unit/compute/test_compute_mgr.py | 4 +++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 6f47428adfc..eaedc0238f6 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -7496,7 +7496,13 @@ def _post_live_migration(self, ctxt, instance, dest, # Releasing vlan. # (not necessary in current implementation?) - network_info = self.network_api.get_instance_nw_info(ctxt, instance) + # NOTE(artom) At this point in time we have not bound the ports to the + # destination host yet (this happens in migrate_instance_start() + # below). Therefore, the "old" source network info that's still in the + # instance info cache is safe to use here, since it'll be used below + # during driver.post_live_migration_at_source() to unplug the VIFs on + # the source. + network_info = instance.get_network_info() self._notify_about_instance_usage(ctxt, instance, "live_migration._post.start", diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index d84b5be3e5c..77f23c30fe4 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -6482,8 +6482,9 @@ def fakecleanup(*args, **kwargs): mock.patch.object( self.compute.network_api, 'setup_networks_on_host'), mock.patch.object(migration_obj, 'save'), + mock.patch.object(instance, 'get_network_info', return_value=[]), ) as ( - mock_migrate, mock_setup, mock_mig_save + mock_migrate, mock_setup, mock_mig_save, mock_get_nw_info ): self.compute._post_live_migration(c, instance, dest, migrate_data=migrate_data, @@ -6495,6 +6496,7 @@ def fakecleanup(*args, **kwargs): mock_migrate.assert_called_once_with(c, instance, migration) mock_post.assert_called_once_with(c, instance, False, dest) mock_clear.assert_called_once_with(mock.ANY) + mock_get_nw_info.assert_called() @mock.patch('nova.compute.utils.notify_about_instance_action') def test_post_live_migration_working_correctly(self, mock_notify): @@ -6537,12 +6539,15 @@ def test_post_live_migration_working_correctly(self, mock_notify): 'clear_events_for_instance'), mock.patch.object(self.compute, 'update_available_resource'), mock.patch.object(migration_obj, 'save'), + mock.patch.object(instance, 'get_network_info'), ) as ( post_live_migration, unfilter_instance, migrate_instance_start, post_live_migration_at_destination, post_live_migration_at_source, setup_networks_on_host, - clear_events, update_available_resource, mig_save + clear_events, update_available_resource, mig_save, get_nw_info, ): + nw_info = network_model.NetworkInfo.hydrate([]) + get_nw_info.return_value = nw_info self.compute._post_live_migration(c, instance, dest, migrate_data=migrate_data, source_bdms=bdms) @@ -6565,7 +6570,7 @@ def test_post_live_migration_working_correctly(self, mock_notify): post_live_migration_at_destination.assert_has_calls([ mock.call(c, instance, False, dest)]) post_live_migration_at_source.assert_has_calls( - [mock.call(c, instance, [])]) + [mock.call(c, instance, nw_info)]) clear_events.assert_called_once_with(instance) update_available_resource.assert_has_calls([mock.call(c)]) self.assertEqual('completed', migration_obj.status) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 5f01113525d..67dc20f6bd2 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -9287,6 +9287,7 @@ def test_post_live_migration_cinder_v3_api(self): source_node='src') image_bdm.attachment_id = uuids.attachment3 + @mock.patch.object(instance, 'get_network_info') @mock.patch('nova.compute.utils.notify_about_instance_action') @mock.patch('nova.objects.ConsoleAuthToken.' 'clean_console_auths_for_instance') @@ -9305,7 +9306,7 @@ def test_post_live_migration_cinder_v3_api(self): def _test(mock_net_api, mock_notify, mock_driver, mock_rpc, mock_get_bdm_info, mock_attach_delete, mock_update_resource, mock_bdm_save, mock_ga, - mock_clean, mock_notify_action): + mock_clean, mock_notify_action, mock_get_nw_info): self._mock_rt() bdms = objects.BlockDeviceMappingList(objects=[vol_bdm, image_bdm]) @@ -9325,6 +9326,7 @@ def _test(mock_net_api, mock_notify, mock_driver, mock.call(self.context, instance, 'fake-mini', action='live_migration_post', phase='end')]) self.assertEqual(2, mock_notify_action.call_count) + mock_get_nw_info.assert_called() _test() From 06df7cae31246e233dd5cf9b92c6484b519156de Mon Sep 17 00:00:00 2001 From: esubramanian Date: Mon, 8 Jun 2020 23:08:30 -0700 Subject: [PATCH 07/54] Removes the delta file once image is extracted When creating a live snapshot of an instance, nova creates a copy of the instance disk using a QEMU shallow rebase. This copy - the delta file - is then extracted and uploaded. The delta file will eventually be deleted, when the temporary working directory nova is using for the live snapshot is discarded, however, until this happens, we will use 3x the size of the image of host disk space: the original disk, the delta file, and the extracted file. This can be problematic when concurrent snapshots of multiple instances are requested at once. The solution is simple: delete the delta file after it has been extracted and is no longer necessary. Change-Id: I15e9975fa516d81e7d34206e5a4069db5431caa9 Closes-Bug: #1881727 (cherry picked from commit d2af7ca7a5c862f53f18c00ac76fc85336fa79e6) (cherry picked from commit e51555b3f0324b8b72a2b3280a1c30e104b6d8ea) --- nova/tests/unit/virt/libvirt/test_driver.py | 4 +++- nova/virt/libvirt/driver.py | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 0b1f8961c55..ea9b6151fe1 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -19142,10 +19142,11 @@ def test_swap_volume_disconnect_new_volume_on_pivot_error(self, disconnect_volume.assert_called_once_with(self.context, mock.sentinel.new_connection_info, instance) + @mock.patch.object(fileutils, 'delete_if_exists') @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete') @mock.patch('nova.privsep.path.chown') def _test_live_snapshot( - self, mock_chown, mock_is_job_complete, + self, mock_chown, mock_is_job_complete, mock_delete, can_quiesce=False, require_quiesce=False): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) mock_dom = mock.MagicMock() @@ -19203,6 +19204,7 @@ def _test_live_snapshot( mock_chown.assert_called_once_with(dltfile, uid=os.getuid()) mock_snapshot.assert_called_once_with(dltfile, "qcow2", dstfile, "qcow2") + mock_delete.assert_called_once_with(dltfile) mock_define.assert_called_once_with(xmldoc) mock_quiesce.assert_any_call(mock.ANY, self.test_instance, mock.ANY, True) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 5043589073e..6a120629c2f 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -2621,6 +2621,10 @@ def _live_snapshot(self, context, instance, guest, disk_path, out_path, libvirt_utils.extract_snapshot(disk_delta, 'qcow2', out_path, image_format) + # Remove the disk_delta file once the snapshot extracted, so that + # it doesn't hang around till the snapshot gets uploaded + fileutils.delete_if_exists(disk_delta) + def _volume_snapshot_update_status(self, context, snapshot_id, status): """Send a snapshot status update to Cinder. From 4984b3b75b5f7086b7464d9acb940e3fb0334a5e Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Fri, 11 Sep 2020 10:23:30 -0400 Subject: [PATCH 08/54] Correctly disable greendns Previously, we were setting the environment variable to disable greendns in eventlet *after* import eventlet. This has no effect, as eventlet processes environment variables at import time. This patch moves the setting of EVENTLET_NO_GREENDNS before importing eventlet in order to correctly disable greendns. Closes-bug: 1895322 Change-Id: I4deed815c8984df095019a7f61d089f233f1fc66 (cherry picked from commit 7c1d964faab33a02fe2366b5194611252be045fc) (cherry picked from commit 79e6b7fd30a04cdb2374abcaf496b6b5b76084ff) --- nova/monkey_patch.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/nova/monkey_patch.py b/nova/monkey_patch.py index a07ff91dacb..160bb1a6611 100644 --- a/nova/monkey_patch.py +++ b/nova/monkey_patch.py @@ -22,6 +22,19 @@ def _monkey_patch(): + # See https://bugs.launchpad.net/nova/+bug/1164822 + # TODO(mdbooth): This feature was deprecated and removed in eventlet at + # some point but brought back in version 0.21.0, presumably because some + # users still required it to work round issues. However, there have been a + # number of greendns fixes in eventlet since then. Specifically, it looks + # as though the originally reported IPv6 issue may have been fixed in + # version 0.24.0. We should remove this when we can confirm that the + # original issue is fixed. + # NOTE(artom) eventlet processes environment variables at import-time. We + # therefore set this here, before importing eventlet, in order to correctly + # disable greendns. + os.environ['EVENTLET_NO_GREENDNS'] = 'yes' + # NOTE(mdbooth): Anything imported here will not be monkey patched. It is # important to take care not to import anything here which requires monkey # patching. @@ -39,16 +52,6 @@ def _monkey_patch(): problems = (set(['urllib3', 'oslo_context.context']) & set(sys.modules.keys())) - # See https://bugs.launchpad.net/nova/+bug/1164822 - # TODO(mdbooth): This feature was deprecated and removed in eventlet at - # some point but brought back in version 0.21.0, presumably because some - # users still required it to work round issues. However, there have been a - # number of greendns fixes in eventlet since then. Specifically, it looks - # as though the originally reported IPv6 issue may have been fixed in - # version 0.24.0. We should remove this when we can confirm that the - # original issue is fixed. - os.environ['EVENTLET_NO_GREENDNS'] = 'yes' - if debugger.enabled(): # turn off thread patching to enable the remote debugger eventlet.monkey_patch(thread=False) From efc35b1c5293c7c6c85f8cf9fd9d8cd8de71d1d5 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 20 Sep 2019 17:07:35 -0400 Subject: [PATCH 09/54] Sanity check instance mapping during scheduling mnaser reported a weird case where an instance was found in both cell0 (deleted there) and in cell1 (not deleted there but in error state from a failed build). It's unclear how this could happen besides some weird clustered rabbitmq issue where maybe the schedule and build request to conductor happens twice for the same instance and one picks a host and tries to build and the other fails during scheduling and is buried in cell0. To avoid a split brain situation like this, we add a sanity check in _bury_in_cell0 to make sure the instance mapping is not pointing at a cell when we go to update it to cell0. Similarly a check is added in the schedule_and_build_instances flow (the code is moved to a private method to make it easier to test). Worst case is this is unnecessary but doesn't hurt anything, best case is this helps avoid split brain clustered rabbit issues. Closes-Bug: #1775934 Change-Id: I335113f0ec59516cb337d34b6fc9078ea202130f (cherry picked from commit 5b552518e1abdc63fb33c633661e30e4b2fe775e) --- nova/conductor/manager.py | 79 ++++++++++++++++----- nova/tests/unit/conductor/test_conductor.py | 58 +++++++++++++++ 2 files changed, 120 insertions(+), 17 deletions(-) diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index dc2b8444960..1c644cf299d 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -1307,8 +1307,37 @@ def _bury_in_cell0(self, context, request_spec, exc, updates = {'vm_state': vm_states.ERROR, 'task_state': None} for instance in instances_by_uuid.values(): + + inst_mapping = None + try: + # We don't need the cell0-targeted context here because the + # instance mapping is in the API DB. + inst_mapping = \ + objects.InstanceMapping.get_by_instance_uuid( + context, instance.uuid) + except exception.InstanceMappingNotFound: + # The API created the instance mapping record so it should + # definitely be here. Log an error but continue to create the + # instance in the cell0 database. + LOG.error('While burying instance in cell0, no instance ' + 'mapping was found.', instance=instance) + + # Perform a final sanity check that the instance is not mapped + # to some other cell already because of maybe some crazy + # clustered message queue weirdness. + if inst_mapping and inst_mapping.cell_mapping is not None: + LOG.error('When attempting to bury instance in cell0, the ' + 'instance is already mapped to cell %s. Ignoring ' + 'bury in cell0 attempt.', + inst_mapping.cell_mapping.identity, + instance=instance) + continue + with obj_target_cell(instance, cell0) as cctxt: instance.create() + if inst_mapping: + inst_mapping.cell_mapping = cell0 + inst_mapping.save() # Record an instance action with a failed event. self._create_instance_action_for_cell0( @@ -1328,16 +1357,6 @@ def _bury_in_cell0(self, context, request_spec, exc, self._set_vm_state_and_notify( cctxt, instance.uuid, 'build_instances', updates, exc, request_spec) - try: - # We don't need the cell0-targeted context here because the - # instance mapping is in the API DB. - inst_mapping = \ - objects.InstanceMapping.get_by_instance_uuid( - context, instance.uuid) - inst_mapping.cell_mapping = cell0 - inst_mapping.save() - except exception.InstanceMappingNotFound: - pass for build_request in build_requests: try: @@ -1506,13 +1525,8 @@ def schedule_and_build_instances(self, context, build_requests, instance.tags = instance_tags if instance_tags \ else objects.TagList() - # Update mapping for instance. Normally this check is guarded by - # a try/except but if we're here we know that a newer nova-api - # handled the build process and would have created the mapping - inst_mapping = objects.InstanceMapping.get_by_instance_uuid( - context, instance.uuid) - inst_mapping.cell_mapping = cell - inst_mapping.save() + # Update mapping for instance. + self._map_instance_to_cell(context, instance, cell) if not self._delete_build_request( context, build_request, instance, cell, instance_bdms, @@ -1540,6 +1554,37 @@ def schedule_and_build_instances(self, context, build_requests, host=host.service_host, node=host.nodename, limits=host.limits, host_list=host_list) + @staticmethod + def _map_instance_to_cell(context, instance, cell): + """Update the instance mapping to point at the given cell. + + During initial scheduling once a host and cell is selected in which + to build the instance this method is used to update the instance + mapping to point at that cell. + + :param context: nova auth RequestContext + :param instance: Instance object being built + :param cell: CellMapping representing the cell in which the instance + was created and is being built. + :returns: InstanceMapping object that was updated. + """ + inst_mapping = objects.InstanceMapping.get_by_instance_uuid( + context, instance.uuid) + # Perform a final sanity check that the instance is not mapped + # to some other cell already because of maybe some crazy + # clustered message queue weirdness. + if inst_mapping.cell_mapping is not None: + LOG.error('During scheduling instance is already mapped to ' + 'another cell: %s. This should not happen and is an ' + 'indication of bigger problems. If you see this you ' + 'should report it to the nova team. Overwriting ' + 'the mapping to point at cell %s.', + inst_mapping.cell_mapping.identity, cell.identity, + instance=instance) + inst_mapping.cell_mapping = cell + inst_mapping.save() + return inst_mapping + def _cleanup_build_artifacts(self, context, exc, instances, build_requests, request_specs, block_device_mappings, tags, cell_mapping_cache): diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 0f6689eba24..525d673f11e 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -2281,6 +2281,27 @@ def test_schedule_and_build_instances_fill_request_spec_noop( self.params['request_specs'][0].requested_resources = [] self._do_schedule_and_build_instances_test(self.params) + def test_map_instance_to_cell_already_mapped(self): + """Tests a scenario where an instance is already mapped to a cell + during scheduling. + """ + build_request = self.params['build_requests'][0] + instance = build_request.get_new_instance(self.ctxt) + # Simulate MQ split brain craziness by updating the instance mapping + # to point at cell0. + inst_mapping = objects.InstanceMapping.get_by_instance_uuid( + self.ctxt, instance.uuid) + inst_mapping.cell_mapping = self.cell_mappings['cell0'] + inst_mapping.save() + cell1 = self.cell_mappings['cell1'] + inst_mapping = self.conductor._map_instance_to_cell( + self.ctxt, instance, cell1) + # Assert that the instance mapping was updated to point at cell1 but + # also that an error was logged. + self.assertEqual(cell1.uuid, inst_mapping.cell_mapping.uuid) + self.assertIn('During scheduling instance is already mapped to ' + 'another cell', self.stdlog.logger.output) + @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid') def test_cleanup_build_artifacts(self, inst_map_get): """Simple test to ensure the order of operations in the cleanup method @@ -2378,6 +2399,17 @@ def test_bury_in_cell0(self, mock_notify): build_requests = objects.BuildRequestList.get_all(self.ctxt) instances = objects.InstanceList.get_all(self.ctxt) + # Verify instance mappings. + inst_mappings = objects.InstanceMappingList.get_by_cell_id( + self.ctxt, self.cell_mappings['cell0'].id) + # bare_br is the only instance that has a mapping from setUp. + self.assertEqual(1, len(inst_mappings)) + # Since we did not setup instance mappings for the other fake build + # requests used in this test, we should see a message logged about + # there being no instance mappings. + self.assertIn('While burying instance in cell0, no instance mapping ' + 'was found', self.stdlog.logger.output) + self.assertEqual(0, len(build_requests)) self.assertEqual(4, len(instances)) inst_states = {inst.uuid: (inst.deleted, inst.vm_state) @@ -2475,6 +2507,32 @@ def test_bury_in_cell0_with_tags(self, mock_create_tags, mock_get_cell, test.MatchType(context.RequestContext), inst.uuid, self.params['tags']) + @mock.patch('nova.objects.Instance.create') + def test_bury_in_cell0_already_mapped(self, mock_inst_create): + """Tests a scenario where the instance mapping is already mapped to a + cell when we attempt to bury the instance in cell0. + """ + build_request = self.params['build_requests'][0] + # Simulate MQ split brain craziness by updating the instance mapping + # to point at cell1. + inst_mapping = objects.InstanceMapping.get_by_instance_uuid( + self.ctxt, build_request.instance_uuid) + inst_mapping.cell_mapping = self.cell_mappings['cell1'] + inst_mapping.save() + # Now attempt to bury the instance in cell0. + with mock.patch.object(inst_mapping, 'save') as mock_inst_map_save: + self.conductor._bury_in_cell0( + self.ctxt, self.params['request_specs'][0], + exc.NoValidHost(reason='idk'), build_requests=[build_request]) + # We should have exited without creating the instance in cell0 nor + # should the instance mapping have been updated to point at cell0. + mock_inst_create.assert_not_called() + mock_inst_map_save.assert_not_called() + # And we should have logged an error. + self.assertIn('When attempting to bury instance in cell0, the ' + 'instance is already mapped to cell', + self.stdlog.logger.output) + def test_reset(self): with mock.patch('nova.compute.rpcapi.ComputeAPI') as mock_rpc: old_rpcapi = self.conductor_manager.compute_rpcapi From cf6db29168c4ee9a34f3b3ebdd1deae31f95f203 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 16 Sep 2020 10:52:17 +0100 Subject: [PATCH 10/54] tests: Add regression test for bug 1894966 You must specify the 'policies' field. Currently, not doing so will result in a HTTP 500 error code. This should be a 4xx error. Add a test to demonstrate the bug before we provide a fix. Changes: nova/tests/functional/regressions/test_bug_1894966.py NOTE(stephenfin): Need to update 'super' call to Python 2-compatible variant. Change-Id: I72e85855f621d3a51cd58d14247abd302dcd958b Signed-off-by: Stephen Finucane Related-Bug: #1894966 (cherry picked from commit 2c66962c7a40d8ef4fab54324e06edcdec1bd716) (cherry picked from commit 94d24e3e8d04488abdebd4969daf98b780125297) --- .../regressions/test_bug_1894966.py | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 nova/tests/functional/regressions/test_bug_1894966.py diff --git a/nova/tests/functional/regressions/test_bug_1894966.py b/nova/tests/functional/regressions/test_bug_1894966.py new file mode 100644 index 00000000000..72173b82559 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1894966.py @@ -0,0 +1,41 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from nova import test +from nova.tests import fixtures as nova_fixtures +from nova.tests.functional.api import client +from nova.tests.functional import integrated_helpers + + +class TestCreateServerGroupWithEmptyPolicies( + test.TestCase, integrated_helpers.InstanceHelperMixin, +): + """Demonstrate bug #1894966. + + Attempt to create a server group with an invalid 'policies' field. It + should fail cleanly. + """ + def setUp(self): + super(TestCreateServerGroupWithEmptyPolicies, self).setUp() + + api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( + api_version='v2.1')) + self.api = api_fixture.api + self.api.microversion = '2.63' # the last version with the bug + + def test_create_with_empty_policies(self): + exc = self.assertRaises( + client.OpenStackApiException, + self.api.post_server_groups, + {'name': 'test group', 'policies': []}) + # FIXME(stephenfin): This should not be a 500 error + self.assertEqual(500, exc.response.status_code) From 1634d3f59ace0206131992e31ee2d4b64123d7e8 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 16 Sep 2020 10:59:49 +0100 Subject: [PATCH 11/54] api: Set min, maxItems for server_group.policies field As noted inline, the 'policies' field may be a list but it expects one of two items. Change-Id: I34c68df1e6330dab1524aa0abec733610211a407 Signed-off-by: Stephen Finucane Closes-Bug: #1894966 (cherry picked from commit 32c43fc8017ee89d4e6cdf79086d87735a00f0c0) (cherry picked from commit 781210bd598c3e0ee9bd6a7db5d25688b5fc0131) --- .../openstack/compute/schemas/server_groups.py | 18 +++++++++++------- .../functional/regressions/test_bug_1894966.py | 3 +-- .../notes/bug-1894966-d25c12b1320cb910.yaml | 10 ++++++++++ 3 files changed, 22 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/bug-1894966-d25c12b1320cb910.yaml diff --git a/nova/api/openstack/compute/schemas/server_groups.py b/nova/api/openstack/compute/schemas/server_groups.py index c5724bd6873..102c6df869e 100644 --- a/nova/api/openstack/compute/schemas/server_groups.py +++ b/nova/api/openstack/compute/schemas/server_groups.py @@ -11,6 +11,7 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. + import copy from nova.api.validation import parameter_types @@ -27,15 +28,18 @@ 'name': parameter_types.name, 'policies': { # This allows only a single item and it must be one of the - # enumerated values. So this is really just a single string - # value, but for legacy reasons is an array. We could - # probably change the type from array to string with a - # microversion at some point but it's very low priority. + # enumerated values. It's changed to a single string value + # in 2.64. 'type': 'array', - 'items': [{ - 'type': 'string', - 'enum': ['anti-affinity', 'affinity']}], + 'items': [ + { + 'type': 'string', + 'enum': ['anti-affinity', 'affinity'], + }, + ], 'uniqueItems': True, + 'minItems': 1, + 'maxItems': 1, 'additionalItems': False, } }, diff --git a/nova/tests/functional/regressions/test_bug_1894966.py b/nova/tests/functional/regressions/test_bug_1894966.py index 72173b82559..e7e40690848 100644 --- a/nova/tests/functional/regressions/test_bug_1894966.py +++ b/nova/tests/functional/regressions/test_bug_1894966.py @@ -37,5 +37,4 @@ def test_create_with_empty_policies(self): client.OpenStackApiException, self.api.post_server_groups, {'name': 'test group', 'policies': []}) - # FIXME(stephenfin): This should not be a 500 error - self.assertEqual(500, exc.response.status_code) + self.assertEqual(400, exc.response.status_code) diff --git a/releasenotes/notes/bug-1894966-d25c12b1320cb910.yaml b/releasenotes/notes/bug-1894966-d25c12b1320cb910.yaml new file mode 100644 index 00000000000..f87cc7806da --- /dev/null +++ b/releasenotes/notes/bug-1894966-d25c12b1320cb910.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + Resolved an issue whereby providing an empty list for the ``policies`` + field in the request body of the ``POST /os-server-groups`` API would + result in a server error. This only affects the 2.1 to 2.63 microversions, + as the 2.64 microversion replaces the ``policies`` list field with a + ``policy`` string field. See `bug #1894966`__ for more information. + + .. __: https://bugs.launchpad.net/nova/+bug/1894966 From 75c0327a67bf12f4b18d6bb1561fc34bb7081264 Mon Sep 17 00:00:00 2001 From: yingjisun Date: Wed, 26 Feb 2020 21:31:41 +0800 Subject: [PATCH 12/54] Set different VirtualDevice.key In vSphere 7.0, the VirtualDevice.key cannot be the same any more. So set different values to VirtualDevice.key Change-Id: I574ed88729d2f0760ea4065cc0e542eea8d20cc2 Closes-Bug: #1892961 (cherry picked from commit a5d153a4c64f6947531823c0df91be5cbc491977) (cherry picked from commit 0ea5bcca9d7bebf835b173c5e75dc89e666bcb99) --- .../tests/unit/virt/vmwareapi/test_vm_util.py | 90 +++++++++++++++++++ nova/virt/vmwareapi/vm_util.py | 8 +- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/nova/tests/unit/virt/vmwareapi/test_vm_util.py b/nova/tests/unit/virt/vmwareapi/test_vm_util.py index ef19152d005..aa26346b380 100644 --- a/nova/tests/unit/virt/vmwareapi/test_vm_util.py +++ b/nova/tests/unit/virt/vmwareapi/test_vm_util.py @@ -1569,6 +1569,96 @@ def test_vm_create_spec_with_profile_spec(self): profile_spec='fake_profile_spec') self.assertEqual(['fake_profile_spec'], create_spec.vmProfile) + def test_vm_create_spec_with_multi_vifs(self): + datastore = ds_obj.Datastore('fake-ds-ref', 'fake-ds-name') + extra_specs = vm_util.ExtraSpecs() + vif_info = {'network_name': 'br100', + 'mac_address': '00:00:00:ca:fe:01', + 'network_ref': {'type': 'DistributedVirtualPortgroup', + 'dvsw': 'fake-network-id1', + 'dvpg': 'fake-group'}, + 'iface_id': 7, + 'vif_model': 'VirtualE1000'} + vif_info2 = {'network_name': 'br101', + 'mac_address': '00:00:00:ca:fe:02', + 'network_ref': {'type': 'DistributedVirtualPortgroup', + 'dvsw': 'fake-network-id2', + 'dvpg': 'fake-group'}, + 'iface_id': 7, + 'vif_model': 'VirtualE1000'} + create_spec = vm_util.get_vm_create_spec(fake.FakeFactory(), + self._instance, + datastore.name, + [vif_info, vif_info2], + extra_specs) + + port = 'ns0:DistributedVirtualSwitchPortConnection' + backing = 'ns0:VirtualEthernetCardDistributedVirtualPortBackingInfo' + + device_changes = [] + fake_factory = fake.FakeFactory() + device_change = fake_factory.create('ns0:VirtualDeviceConfigSpec') + device_change.operation = 'add' + + device = fake_factory.create('ns0:VirtualE1000') + device.key = -47 + device.macAddress = '00:00:00:ca:fe:01' + device.addressType = 'manual' + device.wakeOnLanEnabled = True + + device.backing = fake_factory.create(backing) + device.backing.port = fake_factory.create(port) + device.backing.port.portgroupKey = 'fake-group' + device.backing.port.switchUuid = 'fake-network-id1' + + device.resourceAllocation = fake_factory.create( + 'ns0:VirtualEthernetCardResourceAllocation') + device.resourceAllocation.share = fake_factory.create( + 'ns0:SharesInfo') + device.resourceAllocation.share.level = None + device.resourceAllocation.share.shares = None + + connectable = fake_factory.create('ns0:VirtualDeviceConnectInfo') + connectable.allowGuestControl = True + connectable.connected = True + connectable.startConnected = True + device.connectable = connectable + device_change.device = device + + device_changes.append(device_change) + + device_change = fake_factory.create('ns0:VirtualDeviceConfigSpec') + device_change.operation = 'add' + + device = fake_factory.create('ns0:VirtualE1000') + device.key = -48 + device.macAddress = '00:00:00:ca:fe:02' + device.addressType = 'manual' + device.wakeOnLanEnabled = True + + device.backing = fake_factory.create(backing) + device.backing.port = fake_factory.create(port) + device.backing.port.portgroupKey = 'fake-group' + device.backing.port.switchUuid = 'fake-network-id2' + + device.resourceAllocation = fake_factory.create( + 'ns0:VirtualEthernetCardResourceAllocation') + device.resourceAllocation.share = fake_factory.create( + 'ns0:SharesInfo') + device.resourceAllocation.share.level = None + device.resourceAllocation.share.shares = None + + connectable = fake_factory.create('ns0:VirtualDeviceConnectInfo') + connectable.allowGuestControl = True + connectable.connected = True + connectable.startConnected = True + device.connectable = connectable + device_change.device = device + + device_changes.append(device_change) + + self.assertEqual(device_changes, create_spec.deviceChange) + @mock.patch.object(pbm, 'get_profile_id_by_name') def test_get_storage_profile_spec(self, mock_retrieve_profile_id): fake_profile_id = fake.DataObject() diff --git a/nova/virt/vmwareapi/vm_util.py b/nova/virt/vmwareapi/vm_util.py index 22820d5d843..ac3c1b9762e 100644 --- a/nova/virt/vmwareapi/vm_util.py +++ b/nova/virt/vmwareapi/vm_util.py @@ -242,9 +242,9 @@ def get_vm_create_spec(client_factory, instance, data_store_name, config_spec.firmware = extra_specs.firmware devices = [] - for vif_info in vif_infos: + for i, vif_info in enumerate(vif_infos): vif_spec = _create_vif_spec(client_factory, vif_info, - extra_specs.vif_limits) + extra_specs.vif_limits, i) devices.append(vif_spec) serial_port_spec = create_serial_port_spec(client_factory) @@ -416,7 +416,7 @@ def convert_vif_model(name): return name -def _create_vif_spec(client_factory, vif_info, vif_limits=None): +def _create_vif_spec(client_factory, vif_info, vif_limits=None, offset=0): """Builds a config spec for the addition of a new network adapter to the VM. """ @@ -478,7 +478,7 @@ def _create_vif_spec(client_factory, vif_info, vif_limits=None): # The Server assigns a Key to the device. Here we pass a -ve temporary key. # -ve because actual keys are +ve numbers and we don't # want a clash with the key that server might associate with the device - net_device.key = -47 + net_device.key = -47 - offset net_device.addressType = "manual" net_device.macAddress = mac_address net_device.wakeOnLanEnabled = True From 06b8f1467dc80203098de983cfc335b898440573 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 21 Sep 2020 16:11:38 +0100 Subject: [PATCH 13/54] libvirt: 'video.vram' property must be an integer The 'vram' property of the 'video' device must be an integer else libvirt will spit the dummy out, e.g. libvirt.libvirtError: XML error: cannot parse video vram '8192.0' The division operator in Python 3 results in a float, not an integer like in Python 2. Use the truncation division operator instead. Change-Id: Iebf678c229da4f455459d068cafeee5f241aea1f Signed-off-by: Stephen Finucane Closes-Bug: #1896496 (cherry picked from commit f2ca089bce842127e7d0644b38a11da9278db8ea) (cherry picked from commit fd7c66f61c0b07cff82d93e0f884777949170857) (cherry picked from commit 121e481a880b48ce800aafdeca0b8b086e2cddf3) --- nova/tests/unit/virt/libvirt/test_driver.py | 16 ++++++++++------ nova/virt/libvirt/driver.py | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index f1ee842ac5a..482fc32f1fd 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -6087,7 +6087,7 @@ def test_get_guest_config_with_video_driver_vram(self): self.assertEqual(cfg.devices[4].type, "spice") self.assertEqual(cfg.devices[5].type, "qxl") - self.assertEqual(cfg.devices[5].vram, 64 * units.Mi / units.Ki) + self.assertEqual(cfg.devices[5].vram, 65536) def _test_add_video_driver(self, model): self.flags(virt_type='kvm', group='libvirt') @@ -6098,15 +6098,19 @@ def _test_add_video_driver(self, model): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) guest = vconfig.LibvirtConfigGuest() - instance_ref = objects.Instance(**self.test_instance) - flavor = instance_ref.get_flavor() + flavor = objects.Flavor( + extra_specs={'hw_video:ram_max_mb': '512'}) image_meta = objects.ImageMeta.from_dict({ - 'properties': {'hw_video_model': model}}) + 'properties': { + 'hw_video_model': model, + 'hw_video_ram': 8, + }, + }) self.assertTrue(drvr._guest_add_video_device(guest)) - video = drvr._add_video_driver(guest, image_meta, - flavor) + video = drvr._add_video_driver(guest, image_meta, flavor) self.assertEqual(model, video.type) + self.assertEqual(8192, video.vram) # should be in bytes def test__add_video_driver(self): self._test_add_video_driver('qxl') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index acea96a24b8..ab7ad2e0966 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -5230,7 +5230,7 @@ def _add_video_driver(self, guest, image_meta, flavor): raise exception.RequestedVRamTooHigh(req_vram=video_ram, max_vram=max_vram) if max_vram and video_ram: - video.vram = video_ram * units.Mi / units.Ki + video.vram = video_ram * units.Mi // units.Ki guest.add_device(video) # NOTE(sean-k-mooney): return the video device we added From 750655c19daf4e8d0dcf35d3b1cab5e6d8ac69ba Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Thu, 27 Aug 2020 17:20:19 -0300 Subject: [PATCH 14/54] Allow tap interface with multiqueue When vif_type="tap" (such as when using calico), attempting to create an instance using an image that has the property hw_vif_multiqueue_enabled=True fails, because the interface is always being created without multiqueue flags. This change checks if the property is defined and passes the multiqueue parameter to create the tap interface accordingly. In case the multiqueue parameter is passed but the vif_model is not virtio (or unspecified), the old behavior is maintained. Change-Id: I0307c43dcd0cace1620d2ac75925651d4ee2e96c Closes-bug: #1893263 (cherry picked from commit 84cfc8e9ab1396ec17abcfc9646c7d40f1d966ae) (cherry picked from commit a69845f3732843ee1451b2e4ebf547d9801e898d) --- nova/tests/unit/virt/libvirt/test_vif.py | 67 ++++++++++++++++++- nova/virt/libvirt/vif.py | 57 +++++++++++----- .../notes/bug-1893263-769acadc4b6141d0.yaml | 5 ++ 3 files changed, 111 insertions(+), 18 deletions(-) create mode 100644 releasenotes/notes/bug-1893263-769acadc4b6141d0.yaml diff --git a/nova/tests/unit/virt/libvirt/test_vif.py b/nova/tests/unit/virt/libvirt/test_vif.py index 648d6dc408a..209211c563b 100644 --- a/nova/tests/unit/virt/libvirt/test_vif.py +++ b/nova/tests/unit/virt/libvirt/test_vif.py @@ -1123,10 +1123,71 @@ def test_tap_ethernet_vif_driver(self): @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) @mock.patch('nova.privsep.linux_net.set_device_mtu') @mock.patch('nova.privsep.linux_net.create_tap_dev') - def test_plug_tap(self, mock_create_tap_dev, mock_set_mtu, + def test_plug_tap_kvm_virtio(self, mock_create_tap_dev, mock_set_mtu, mock_device_exists): - d = vif.LibvirtGenericVIFDriver() - d.plug(self.instance, self.vif_tap) + + d1 = vif.LibvirtGenericVIFDriver() + ins = objects.Instance( + id=1, uuid='f0000000-0000-0000-0000-000000000001', + project_id=723, system_metadata={} + ) + d1.plug(ins, self.vif_tap) + mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', None, + multiqueue=False) + + mock_create_tap_dev.reset_mock() + + d2 = vif.LibvirtGenericVIFDriver() + mq_ins = objects.Instance( + id=1, uuid='f0000000-0000-0000-0000-000000000001', + project_id=723, system_metadata={ + 'image_hw_vif_multiqueue_enabled': 'True' + } + ) + d2.plug(mq_ins, self.vif_tap) + mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', None, + multiqueue=True) + + @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) + @mock.patch('nova.privsep.linux_net.set_device_mtu') + @mock.patch('nova.privsep.linux_net.create_tap_dev') + def test_plug_tap_mq_ignored_virt_type( + self, mock_create_tap_dev, mock_set_mtu, mock_device_exists): + + self.flags(use_virtio_for_bridges=True, + virt_type='xen', + group='libvirt') + + d1 = vif.LibvirtGenericVIFDriver() + ins = objects.Instance( + id=1, uuid='f0000000-0000-0000-0000-000000000001', + project_id=723, system_metadata={ + 'image_hw_vif_multiqueue_enabled': 'True' + } + ) + d1.plug(ins, self.vif_tap) + mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', + None, + multiqueue=False) + + @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) + @mock.patch('nova.privsep.linux_net.set_device_mtu') + @mock.patch('nova.privsep.linux_net.create_tap_dev') + def test_plug_tap_mq_ignored_vif_model( + self, mock_create_tap_dev, mock_set_mtu, mock_device_exists): + + d1 = vif.LibvirtGenericVIFDriver() + ins = objects.Instance( + id=1, uuid='f0000000-0000-0000-0000-000000000001', + project_id=723, system_metadata={ + 'image_hw_vif_multiqueue_enabled': 'True', + 'image_hw_vif_model': 'e1000', + } + ) + d1.plug(ins, self.vif_tap) + mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', + None, + multiqueue=False) def test_unplug_tap(self): d = vif.LibvirtGenericVIFDriver() diff --git a/nova/virt/libvirt/vif.py b/nova/virt/libvirt/vif.py index 89c259a6181..d4801ad887f 100644 --- a/nova/virt/libvirt/vif.py +++ b/nova/virt/libvirt/vif.py @@ -126,6 +126,24 @@ def get_vif_devname(self, vif): return vif['devname'] return ("nic" + vif['id'])[:network_model.NIC_NAME_LEN] + def get_vif_model(self, image_meta=None, vif_model=None): + + model = vif_model + + # If the user has specified a 'vif_model' against the + # image then honour that model + if image_meta: + model = osinfo.HardwareProperties(image_meta).network_model + + # If the virt type is KVM/QEMU/VZ(Parallels), then use virtio according + # to the global config parameter + if (model is None and CONF.libvirt.virt_type in + ('kvm', 'qemu', 'parallels') and + CONF.libvirt.use_virtio_for_bridges): + model = network_model.VIF_MODEL_VIRTIO + + return model + def get_base_config(self, instance, mac, image_meta, inst_type, virt_type, vnic_type, host): # TODO(sahid): We should rewrite it. This method handles too @@ -146,16 +164,9 @@ def get_base_config(self, instance, mac, image_meta, conf, mac, model, driver, vhost_queues, rx_queue_size) return conf - # If the user has specified a 'vif_model' against the - # image then honour that model - if image_meta: - model = osinfo.HardwareProperties(image_meta).network_model - - # If the virt type is KVM/QEMU/VZ(Parallels), then use virtio according - # to the global config parameter - if (model is None and virt_type in ('kvm', 'qemu', 'parallels') and - CONF.libvirt.use_virtio_for_bridges): - model = network_model.VIF_MODEL_VIRTIO + # if model has already been defined, + # image_meta contents will override it + model = self.get_vif_model(image_meta=image_meta, vif_model=model) if not is_vif_model_valid_for_virt(virt_type, model): raise exception.UnsupportedHardware(model=model, virt=virt_type) @@ -212,10 +223,7 @@ def _get_virtio_mq_settings(self, image_meta, flavor): """ driver = None vhost_queues = None - if not isinstance(image_meta, objects.ImageMeta): - image_meta = objects.ImageMeta.from_dict(image_meta) - img_props = image_meta.properties - if img_props.get('hw_vif_multiqueue_enabled'): + if self._requests_multiqueue(image_meta): driver = 'vhost' max_tap_queues = self._get_max_tap_queues() if max_tap_queues: @@ -226,6 +234,19 @@ def _get_virtio_mq_settings(self, image_meta, flavor): return (driver, vhost_queues) + def _requests_multiqueue(self, image_meta): + """Check if multiqueue property is set in the image metadata.""" + + if not isinstance(image_meta, objects.ImageMeta): + image_meta = objects.ImageMeta.from_dict(image_meta) + + img_props = image_meta.properties + + if img_props.get('hw_vif_multiqueue_enabled'): + return True + + return False + def _get_max_tap_queues(self): # NOTE(kengo.sakai): In kernels prior to 3.0, # multiple queues on a tap interface is not supported. @@ -690,7 +711,13 @@ def plug_tap(self, instance, vif): """Plug a VIF_TYPE_TAP virtual interface.""" dev = self.get_vif_devname(vif) mac = vif['details'].get(network_model.VIF_DETAILS_TAP_MAC_ADDRESS) - nova.privsep.linux_net.create_tap_dev(dev, mac) + image_meta = instance.image_meta + vif_model = self.get_vif_model(image_meta=image_meta) + # TODO(ganso): explore whether multiqueue works for other vif models + # that go through this code path. + multiqueue = (self._requests_multiqueue(image_meta) and + vif_model == network_model.VIF_MODEL_VIRTIO) + nova.privsep.linux_net.create_tap_dev(dev, mac, multiqueue=multiqueue) network = vif.get('network') mtu = network.get_meta('mtu') if network else None nova.privsep.linux_net.set_device_mtu(dev, mtu) diff --git a/releasenotes/notes/bug-1893263-769acadc4b6141d0.yaml b/releasenotes/notes/bug-1893263-769acadc4b6141d0.yaml new file mode 100644 index 00000000000..3874489513b --- /dev/null +++ b/releasenotes/notes/bug-1893263-769acadc4b6141d0.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Addressed an issue that prevented instances using multiqueue feature from + being created successfully when their vif_type is TAP. From 794bedf00e6a3dcdf89f07ae3f63deee09138a9a Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Fri, 3 May 2019 13:46:23 -0700 Subject: [PATCH 15/54] Add a workaround config toggle to refuse ceph image upload If a compute node is backed by ceph, and the image is not clone-able in that same ceph, nova will try to download the image from glance and upload it to ceph itself. This is nice in that it "just works", but it also means we store that image in ceph in an extremely inefficient way. In a glance multi-store case with multiple ceph clusters, the user is currently required to make sure that the image they are going to use is stored in a backend local to the compute node they land on, and if they do not (or can not), then nova will do this non-COW inefficient copy of the image, which is likely not what the operator expects. Per the discussion at the Denver PTG, this adds a workaround flag which allows the operators to direct nova to *not* do this behavior and instead refuse to boot the instance entirely. Conflicts: nova/conf/workarounds.py NOTE(melwitt): The conflict is because this patch originally landed on ussuri and change If874f018ea996587e178219569c2903c2ee923cf (Reserve DISK_GB resource for the image cache) landed afterward and was backported to stable/train. Related-Bug: #1858877 Change-Id: I069b6b1d28eaf1eee5c7fb8d0fdef9c0c229a1bf (cherry picked from commit 80191e6d828cf823ce3aa7c6176da5e531694900) --- nova/conf/workarounds.py | 24 ++++++++++ nova/tests/unit/virt/libvirt/test_driver.py | 46 +++++++++++++++++++ nova/virt/libvirt/driver.py | 15 ++++++ ...d_muli_ceph_download-4083decf501dba40.yaml | 19 ++++++++ 4 files changed, 104 insertions(+) create mode 100644 releasenotes/notes/avoid_muli_ceph_download-4083decf501dba40.yaml diff --git a/nova/conf/workarounds.py b/nova/conf/workarounds.py index c9ed0f2e4b5..20e4b5c9d61 100644 --- a/nova/conf/workarounds.py +++ b/nova/conf/workarounds.py @@ -265,6 +265,30 @@ * :oslo.config:option:`DEFAULT.instances_path` * :oslo.config:option:`image_cache_subdirectory_name` * :oslo.config:option:`update_resources_interval` +"""), + cfg.BoolOpt( + 'never_download_image_if_on_rbd', + default=False, + help=""" +When booting from an image on a ceph-backed compute node, if the image does not +already reside on the ceph cluster (as would be the case if glance is +also using the same cluster), nova will download the image from glance and +upload it to ceph itself. If using multiple ceph clusters, this may cause nova +to unintentionally duplicate the image in a non-COW-able way in the local +ceph deployment, wasting space. + +For more information, refer to the bug report: + +https://bugs.launchpad.net/nova/+bug/1858877 + +Enabling this option will cause nova to *refuse* to boot an instance if it +would require downloading the image from glance and uploading it to ceph +itself. + +Related options: + +* ``compute_driver`` (libvirt) +* ``[libvirt]/images_type`` (rbd) """), ] diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 3b131f4da71..95904aafca8 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -21799,6 +21799,52 @@ def test_data_not_injects_with_configdrive(self, mock_image, mock_inject, None) self.assertFalse(mock_inject.called) + @mock.patch('nova.virt.libvirt.utils.fetch_image') + @mock.patch('nova.virt.libvirt.storage.rbd_utils.RBDDriver') + @mock.patch.object(imagebackend, 'IMAGE_API') + def test_create_fetch_image_ceph_workaround(self, mock_image, mock_rbd, + mock_fetch): + # Make sure that rbd clone will fail as un-clone-able + mock_rbd.is_cloneable.return_value = False + # Make sure the rbd code thinks the image does not already exist + mock_rbd.return_value.exists.return_value = False + # Make sure the rbd code says the image is small + mock_rbd.return_value.size.return_value = 128 * units.Mi + # Make sure IMAGE_API.get() returns a raw image + mock_image.get.return_value = {'locations': [], 'disk_format': 'raw'} + + instance = self._create_instance() + disk_images = {'image_id': 'foo'} + self.flags(images_type='rbd', group='libvirt') + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + + def do_create(): + # Reset the fetch mock and run our driver method so we can + # check for called-ness after each attempt + mock_fetch.reset_mock() + drvr._create_and_inject_local_root(self.context, + instance, + False, + '', + disk_images, + get_injection_info(), + None) + + # Do an image create with rbd + do_create() + # Make sure it tried fetch, which implies that it tried and + # failed to clone. + mock_fetch.assert_called() + + # Enable the workaround + self.flags(never_download_image_if_on_rbd=True, + group='workarounds') + # Ensure that we raise the original ImageUnacceptable from the + # failed clone... + self.assertRaises(exception.ImageUnacceptable, do_create) + # ...and ensure that we did _not_ try to fetch + mock_fetch.assert_not_called() + @mock.patch('nova.virt.netutils.get_injected_network_template') @mock.patch('nova.virt.disk.api.inject_data') @mock.patch.object(libvirt_driver.LibvirtDriver, "_conn") diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index f4df91e4940..fc5b7db06da 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -4021,9 +4021,24 @@ def _create_and_inject_local_root(self, context, instance, backend.create_snap(libvirt_utils.RESIZE_SNAPSHOT_NAME) if backend.SUPPORTS_CLONE: def clone_fallback_to_fetch(*args, **kwargs): + refuse_fetch = ( + CONF.libvirt.images_type == 'rbd' and + CONF.workarounds.never_download_image_if_on_rbd) try: backend.clone(context, disk_images['image_id']) except exception.ImageUnacceptable: + if refuse_fetch: + # Re-raise the exception from the failed + # ceph clone. The compute manager expects + # ImageUnacceptable as a possible result + # of spawn(), from which this is called. + with excutils.save_and_reraise_exception(): + LOG.warning( + 'Image %s is not on my ceph and ' + '[workarounds]/' + 'never_download_image_if_on_rbd=True;' + ' refusing to fetch and upload.', + disk_images['image_id']) libvirt_utils.fetch_image(*args, **kwargs) fetch_func = clone_fallback_to_fetch else: diff --git a/releasenotes/notes/avoid_muli_ceph_download-4083decf501dba40.yaml b/releasenotes/notes/avoid_muli_ceph_download-4083decf501dba40.yaml new file mode 100644 index 00000000000..f79c2781196 --- /dev/null +++ b/releasenotes/notes/avoid_muli_ceph_download-4083decf501dba40.yaml @@ -0,0 +1,19 @@ +--- +other: + - | + Nova now has a config option called + ``[workarounds]/never_download_image_if_on_rbd`` which helps to + avoid pathological storage behavior with multiple ceph clusters. + Currently, Nova does *not* support multiple ceph clusters + properly, but Glance can be configured with them. If an instance + is booted from an image residing in a ceph cluster other than the + one Nova knows about, it will silently download it from Glance and + re-upload the image to the local ceph privately for that + instance. Unlike the behavior you expect when configuring Nova and + Glance for ceph, Nova will continue to do this over and over for + the same image when subsequent instances are booted, consuming a + large amount of storage unexpectedly. The new workaround option + will cause Nova to refuse to do this download/upload behavior and + instead fail the instance boot. It is simply a stop-gap effort to + allow unsupported deployments with multiple ceph clusters from + silently consuming large amounts of disk space. From 115b43ed3e9514d9e4fb41da5582f0b185ecd10a Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 8 Oct 2020 05:45:24 +0000 Subject: [PATCH 16/54] Follow up for cherry-pick check for merge patch This is a follow up to change I8e4e5afc773d53dee9c1c24951bb07a45ddc2f1a which fixed an issue with validation when the topmost patch after a Zuul rebase is a merge patch. We need to also use the $commit_hash variable for the check for stable-only patches, else it will incorrectly fail because it is checking the merge patch's commit message. Change-Id: Ia725346b65dd5e2f16aa049c74b45d99e22b3524 (cherry picked from commit 1e10461c71cb78226824988b8c903448ba7a8a76) (cherry picked from commit f1e4f6b078baf72e83cd7341c380aa0fc511519e) (cherry picked from commit e676a480544b3fa71fcaa984a658e2131b7538c5) --- tools/check-cherry-picks.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/check-cherry-picks.sh b/tools/check-cherry-picks.sh index 32627e59b60..5a449c520b7 100755 --- a/tools/check-cherry-picks.sh +++ b/tools/check-cherry-picks.sh @@ -32,7 +32,7 @@ if [ $checked -eq 0 ]; then echo "Checked $checked cherry-pick hashes: OK" exit 0 else - if ! git show --format='%B' --quiet | grep -qi 'stable.*only'; then + if ! git show --format='%B' --quiet $commit_hash | grep -qi 'stable.*only'; then echo 'Stable branch requires either cherry-pick -x headers or [stable-only] tag!' exit 1 fi From e3bb6119cf2d0a503768979312aea4d10cf85cda Mon Sep 17 00:00:00 2001 From: melanie witt Date: Mon, 12 Oct 2020 22:27:52 +0000 Subject: [PATCH 17/54] Prevent archiving of pci_devices records because of 'instance_uuid' Currently in the archive_deleted_rows code, we will attempt to clean up "residue" of deleted instance records by assuming any table with a 'instance_uuid' column represents data tied to an instance's lifecycle and delete such records. This behavior poses a problem in the case where an instance has a PCI device allocated and someone deletes the instance. The 'instance_uuid' column in the pci_devices table is used to track the allocation association of a PCI with an instance. There is a small time window during which the instance record has been deleted but the PCI device has not yet been freed from a database record perspective as PCI devices are freed during the _complete_deletion method in the compute manager as part of the resource tracker update call. Records in the pci_devices table are anyway not related to the lifecycle of instances so they should not be considered residue to clean up if an instance is deleted. This adds a condition to avoid archiving pci_devices on the basis of an instance association. Closes-Bug: #1899541 Change-Id: Ie62d3566230aa3e2786d129adbb2e3570b06e4c6 (cherry picked from commit 1c256cf774693e2395ae8fe4a7a2f416a7aeb03a) (cherry picked from commit 09784db62fcd01124a101c4c69cab6e71e1ac781) (cherry picked from commit 79df36fecf8c8be5ae9d59397882ac844852043e) --- nova/db/sqlalchemy/api.py | 6 +++++- nova/tests/functional/db/test_archive.py | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 02311f240d5..b39f90d3b03 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -5578,7 +5578,11 @@ def _archive_deleted_rows_for_table(metadata, tablename, max_rows, before): # NOTE(jake): instance_actions_events doesn't have a instance_uuid column # but still needs to be archived as it is a FK constraint if ((max_rows is None or rows_archived < max_rows) and - ('instance_uuid' in columns or + # NOTE(melwitt): The pci_devices table uses the 'instance_uuid' + # column to track the allocated association of a PCI device and its + # records are not tied to the lifecycles of instance records. + (tablename != 'pci_devices' and + 'instance_uuid' in columns or tablename == 'instance_actions_events')): instances = models.BASE.metadata.tables['instances'] limit = max_rows - rows_archived if max_rows is not None else None diff --git a/nova/tests/functional/db/test_archive.py b/nova/tests/functional/db/test_archive.py index 79edf0b96ad..bc463c7eccc 100644 --- a/nova/tests/functional/db/test_archive.py +++ b/nova/tests/functional/db/test_archive.py @@ -137,6 +137,19 @@ def test_archive_deleted_rows_with_undeleted_residue(self): # Verify we have some system_metadata since we'll check that later. self.assertTrue(len(instance.system_metadata), 'No system_metadata for instance: %s' % server_id) + # Create a pci_devices record to simulate an instance that had a PCI + # device allocated at the time it was deleted. There is a window of + # time between deletion of the instance record and freeing of the PCI + # device in nova-compute's _complete_deletion method during RT update. + db.pci_device_update(admin_context, 1, 'fake-address', + {'compute_node_id': 1, + 'address': 'fake-address', + 'vendor_id': 'fake', + 'product_id': 'fake', + 'dev_type': 'fake', + 'label': 'fake', + 'status': 'allocated', + 'instance_uuid': instance.uuid}) # Now try and archive the soft deleted records. results, deleted_instance_uuids, archived = \ db.archive_deleted_rows(max_rows=100) @@ -151,6 +164,8 @@ def test_archive_deleted_rows_with_undeleted_residue(self): self.assertIn('instance_actions', results) self.assertIn('instance_actions_events', results) self.assertEqual(sum(results.values()), archived) + # Verify that the pci_devices record has not been dropped + self.assertNotIn('pci_devices', results) def _get_table_counts(self): engine = sqlalchemy_api.get_engine() From cd83da5501e3dc83f9a12c5799c388e06cf8c7f2 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Tue, 27 Oct 2020 08:44:59 +0000 Subject: [PATCH 18/54] libvirt: Only ask tpool.Proxy to autowrap vir* classes I668643c836d46a25df46d4c99a973af5e50a39db attempted to fix service wide pauses by providing a more complete list of classes to tpool.Proxy. While this excluded libvirtError it can include internal libvirt-python classes pointed to by private globals that have been introduced with the use of type checking within the module. Any attempt to wrap these internal classes will result in the failure seen in bug #1901383. As a result this change simply ignores any class found during inspection that doesn't start with the `vir` string, used by libvirt to denote public methods and classes. Closes-Bug: #1901383 Co-Authored-By: Daniel Berrange Change-Id: I568b0c4fd6069b9118ff116532f14abb46cc42ab (cherry picked from commit 0d2ca53bb86b8e4a3c44855cb5ef57f223462543) (cherry picked from commit 048a3337a8c98ec8fc138083376430ffb9027f67) (cherry picked from commit 36cb57d97be07a5560bef4c090f8cab47992c042) --- nova/tests/unit/virt/libvirt/fakelibvirt.py | 10 ++++++++++ nova/tests/unit/virt/libvirt/test_host.py | 3 ++- nova/virt/libvirt/host.py | 8 ++++---- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index d10a44c7386..b52b671f52e 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -1656,6 +1656,16 @@ def make_libvirtError(error_class, msg, error_code=None, virNWFilter = NWFilter +# A private libvirt-python class and global only provided here for testing to +# ensure it's not returned by libvirt.host.Host.get_libvirt_proxy_classes. +class FakeHandler(object): + def __init__(self): + pass + + +_EventAddHandleFunc = FakeHandler + + class FakeLibvirtFixture(fixtures.Fixture): """Performs global setup/stubbing for all libvirt tests. """ diff --git a/nova/tests/unit/virt/libvirt/test_host.py b/nova/tests/unit/virt/libvirt/test_host.py index c9411d6eb75..4c4b1d45c8b 100644 --- a/nova/tests/unit/virt/libvirt/test_host.py +++ b/nova/tests/unit/virt/libvirt/test_host.py @@ -1322,8 +1322,9 @@ def test_get_libvirt_proxy_classes(self): self.assertIn(fakelibvirt.virSecret, proxy_classes) self.assertIn(fakelibvirt.virNWFilter, proxy_classes) - # Assert that we filtered out libvirtError + # Assert that we filtered out libvirtError and any private classes self.assertNotIn(fakelibvirt.libvirtError, proxy_classes) + self.assertNotIn(fakelibvirt._EventAddHandleFunc, proxy_classes) def test_tpool_get_connection(self): # Test that Host.get_connection() returns a tpool.Proxy diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index 603ab9925fd..d8078dd3873 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -124,15 +124,15 @@ def __init__(self, uri, read_only=False, @staticmethod def _get_libvirt_proxy_classes(libvirt_module): """Return a tuple for tpool.Proxy's autowrap argument containing all - classes defined by the libvirt module except libvirtError. + public vir* classes defined by the libvirt module. """ # Get a list of (name, class) tuples of libvirt classes classes = inspect.getmembers(libvirt_module, inspect.isclass) - # Return a list of just the classes, filtering out libvirtError because - # we don't need to proxy that - return tuple([cls[1] for cls in classes if cls[0] != 'libvirtError']) + # Return a list of just the vir* classes, filtering out libvirtError + # and any private globals pointing at private internal classes. + return tuple([cls[1] for cls in classes if cls[0].startswith("vir")]) def _wrap_libvirt_proxy(self, obj): """Return an object wrapped in a tpool.Proxy using autowrap appropriate From ca2fd8098a47cf2c38351dc4c8c65c7c5fe0afc3 Mon Sep 17 00:00:00 2001 From: Keigo Noha Date: Fri, 10 Jul 2020 10:32:02 +0900 Subject: [PATCH 19/54] Change default num_retries for glance to 3 Previously, the default value of num_retries for glance is 0. It means that the request to glance is sent only one time. On the other hand, neutron and cinder clients set the default value to 3. To align the default value for retry to other components, we should change the default value to 3. Closes-Bug: #1888168 Change-Id: Ibbd4bd26408328b9e1a1128b3794721405631193 (cherry picked from commit 662af9fab6eacb46bcaee38d076d33c2c0f82b9b) (cherry picked from commit 1f9dd694b937cc55a81a64fdce442829f009afb3) --- nova/conf/glance.py | 2 +- nova/tests/unit/virt/xenapi/image/test_glance.py | 12 ++++++------ nova/tests/unit/virt/xenapi/test_vm_utils.py | 2 +- ...increase_glance_num_retries-ddfcd7053631882b.yaml | 11 +++++++++++ 4 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/increase_glance_num_retries-ddfcd7053631882b.yaml diff --git a/nova/conf/glance.py b/nova/conf/glance.py index b96f437b0f3..694855e219c 100644 --- a/nova/conf/glance.py +++ b/nova/conf/glance.py @@ -44,7 +44,7 @@ (i.e. "http://10.0.1.0:9292" or "https://my.glance.server/image"). """), cfg.IntOpt('num_retries', - default=0, + default=3, min=0, help=""" Enable glance operation retries. diff --git a/nova/tests/unit/virt/xenapi/image/test_glance.py b/nova/tests/unit/virt/xenapi/image/test_glance.py index 3994af17926..be49622e733 100644 --- a/nova/tests/unit/virt/xenapi/image/test_glance.py +++ b/nova/tests/unit/virt/xenapi/image/test_glance.py @@ -148,7 +148,7 @@ def test_upload_image(self, mock_upload, mock_sr_path, mock_extra_header, mock_sr_path.assert_called_once_with(self.session) mock_extra_header.assert_called_once_with(self.context) mock_upload.assert_called_once_with( - self.session, 0, mock.ANY, mock.ANY, 'fake_image_uuid', + self.session, 3, mock.ANY, mock.ANY, 'fake_image_uuid', 'fake_sr_path', 'fake_extra_header', **params) @mock.patch.object(utils, 'get_auto_disk_config_from_instance') @@ -169,7 +169,7 @@ def test_upload_image_None_os_type(self, mock_upload, mock_sr_path, mock_sr_path.assert_called_once_with(self.session) mock_extra_header.assert_called_once_with(self.context) mock_upload.assert_called_once_with( - self.session, 0, mock.ANY, mock.ANY, 'fake_image_uuid', + self.session, 3, mock.ANY, mock.ANY, 'fake_image_uuid', 'fake_sr_path', 'fake_extra_header', **params) mock_disk_config.assert_called_once_with(self.instance) @@ -190,7 +190,7 @@ def test_upload_image_no_os_type(self, mock_upload, mock_sr_path, mock_sr_path.assert_called_once_with(self.session) mock_extra_header.assert_called_once_with(self.context) mock_upload.assert_called_once_with( - self.session, 0, mock.ANY, mock.ANY, 'fake_image_uuid', + self.session, 3, mock.ANY, mock.ANY, 'fake_image_uuid', 'fake_sr_path', 'fake_extra_header', **params) mock_disk_config.assert_called_once_with(self.instance) @@ -211,7 +211,7 @@ def test_upload_image_auto_config_disk_disabled( mock_sr_path.assert_called_once_with(self.session) mock_extra_header.assert_called_once_with(self.context) mock_upload.assert_called_once_with( - self.session, 0, mock.ANY, mock.ANY, 'fake_image_uuid', + self.session, 3, mock.ANY, mock.ANY, 'fake_image_uuid', 'fake_sr_path', 'fake_extra_header', **params) @mock.patch.object(common_glance, 'generate_identity_headers') @@ -231,7 +231,7 @@ def test_upload_image_raises_exception(self, mock_upload, mock_sr_path, mock_sr_path.assert_called_once_with(self.session) mock_extra_header.assert_called_once_with(self.context) mock_upload.assert_called_once_with( - self.session, 0, mock.ANY, mock.ANY, 'fake_image_uuid', + self.session, 3, mock.ANY, mock.ANY, 'fake_image_uuid', 'fake_sr_path', 'fake_extra_header', **params) @mock.patch.object(time, 'sleep') @@ -330,5 +330,5 @@ def test_upload_image_raises_exception_image_not_found(self, mock_sr_path.assert_called_once_with(self.session) mock_extra_header.assert_called_once_with(self.context) mock_upload.assert_called_once_with( - self.session, 0, mock.ANY, mock.ANY, 'fake_image_uuid', + self.session, 3, mock.ANY, mock.ANY, 'fake_image_uuid', 'fake_sr_path', 'fake_extra_header', **params) diff --git a/nova/tests/unit/virt/xenapi/test_vm_utils.py b/nova/tests/unit/virt/xenapi/test_vm_utils.py index ef2d687e8f8..ef29e27e36b 100644 --- a/nova/tests/unit/virt/xenapi/test_vm_utils.py +++ b/nova/tests/unit/virt/xenapi/test_vm_utils.py @@ -258,7 +258,7 @@ def _assert_call_plugin_serialized_with_retry(self): self.mock_call_plugin.assert_called_once_with( 'glance.py', 'download_vhd2', - 0, + 3, mock.ANY, mock.ANY, extra_headers={'X-Auth-Token': 'auth_token', diff --git a/releasenotes/notes/increase_glance_num_retries-ddfcd7053631882b.yaml b/releasenotes/notes/increase_glance_num_retries-ddfcd7053631882b.yaml new file mode 100644 index 00000000000..72b5467bcd2 --- /dev/null +++ b/releasenotes/notes/increase_glance_num_retries-ddfcd7053631882b.yaml @@ -0,0 +1,11 @@ +--- +upgrade: + - | + The default for ``[glance] num_retries`` has changed from ``0`` to ``3``. + The option controls how many times to retry a Glance API call in response + to a HTTP connection failure. When deploying Glance behind HAproxy it is + possible for a response to arrive just after the HAproxy idle time. As a + result, an exception will be raised when the connection is closed resulting + in a failed request. By increasing the default value, Nova can be more + resilient to this scenario were HAproxy is misconfigured by retrying the + request. From 5c3b4b6c238050926cc79acb83f8d225a79c5657 Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Mon, 21 Sep 2020 11:47:08 -0400 Subject: [PATCH 20/54] Test for disabling greendns In commit 7c1d964faa we fixed how we disable greendns. This patch adds a test for this. It also lays down the groundwork for future tests of how we manage eventlet's monkeypatching. How and what eventlet monkeypatches can be controlled by environment variables that are processed by eventlet at import-time (for exmaple, EVENTLET_NO_GREENDNS). Nova manages all of this in nova.monkey_patch. Therefore, nova.monkey_patch must be the first thing to import eventlet. As nova.tests.functional.__init__ imports nova.monkey_patch, our new test can go in the functional tree. Related-bug: 1895322 Change-Id: I5b6c45b7b9a9eca3c13ecfaa5f50942922b69270 (cherry picked from commit 6f35e4fd2afdb8e26682fc5bd73dbe23236d25c7) (cherry picked from commit 9ac794b93b827dde1d9545b88fd7913b7c00dacb) --- nova/tests/functional/test_monkey_patch.py | 45 ++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 nova/tests/functional/test_monkey_patch.py diff --git a/nova/tests/functional/test_monkey_patch.py b/nova/tests/functional/test_monkey_patch.py new file mode 100644 index 00000000000..b471d333cf7 --- /dev/null +++ b/nova/tests/functional/test_monkey_patch.py @@ -0,0 +1,45 @@ +# Copyright 2020 Red Hat, Inc. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +# NOTE(artom) This file exists to test eventlet monkeypatching. How and what +# eventlet monkeypatches can be controlled by environment variables that +# are processed by eventlet at import-time (for exmaple, EVENTLET_NO_GREENDNS). +# Nova manages all of this in nova.monkey_patch. Therefore, nova.monkey_patch +# must be the first thing to import eventlet. As nova.tests.functional.__init__ +# imports nova.monkey_patch, we're OK here. + +import socket +import traceback + +from nova import test + + +class TestMonkeyPatch(test.TestCase): + + def test_greendns_is_disabled(self): + """Try to resolve a fake fqdn. If we see greendns mentioned in the + traceback of the raised exception, it means we've not actually disabled + greendns. See the TODO and NOTE in nova.monkey_patch to understand why + greendns needs to be disabled. + """ + raised = False + try: + socket.gethostbyname('goat.fake') + except Exception: + tb = traceback.format_exc() + # NOTE(artom) If we've correctly disabled greendns, we expect the + # traceback to not contain any reference to it. + self.assertNotIn('greendns.py', tb) + raised = True + self.assertTrue(raised) From eaecd1ceb0243b31cde9b3c909e8fb3fbef7e635 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 16 Nov 2020 17:35:07 +0000 Subject: [PATCH 21/54] Add missing exception Change Idd49b0c70caedfcd42420ffa2ac926a6087d406e added support for discovery of PMEM devices by the libvirt driver. Some error handling code in this was expected to raise a 'GetPMEMNamespacesFailed' exception, however, a typo meant the exception was actually called 'GetPMEMNamespaceFailed' (singular). This exception was later removed in change I6fd027fb51823b8a8a24ed7b864a2191c4e8e8c0 because it had no references. Re-introduce the exception, this time with the correct name, and add some unit tests to prevent us regressing. Conflicts: nova/exception.py NOTE(stephenfin): Conflicts are because change I6fd027fb51823b8a8a24ed7b864a2191c4e8e8c0 doesn't exist on this branch, meaning the misnamed exception still exists and simply needs to be renamed. Change-Id: I3b597a46314a1b29a952fc0f7a9c4537341e37b8 Signed-off-by: Stephen Finucane Closes-Bug: #1904446 (cherry picked from commit 160ed6ff652b80cd0a86d41dc3f66c15cee66290) (cherry picked from commit 82d415d200e39a5c11fe134e9ef62e427988f2ba) (cherry picked from commit 8f65de96a07623c6a8a9cd49db942ad6d4d3ab03) --- nova/exception.py | 2 +- nova/tests/unit/virt/libvirt/test_driver.py | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/nova/exception.py b/nova/exception.py index 7a5c5239333..3971ef9c44d 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -2541,7 +2541,7 @@ class PMEMNamespaceConfigInvalid(NovaException): "please check your conf file. ") -class GetPMEMNamespaceFailed(NovaException): +class GetPMEMNamespacesFailed(NovaException): msg_fmt = _("Get PMEM namespaces on host failed: %(reason)s.") diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 3b131f4da71..adef6c4c6f6 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -25471,6 +25471,15 @@ def test_vpmems_duplicated_config(self, mock_get): self.assertRaises(exception.PMEMNamespaceConfigInvalid, drvr._discover_vpmems, vpmem_conf) + @mock.patch('nova.privsep.libvirt.get_pmem_namespaces') + def test_get_vpmems_on_host__exception(self, mock_get_ns): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) + mock_get_ns.side_effect = Exception('foo') + + self.assertRaises( + exception.GetPMEMNamespacesFailed, + drvr._get_vpmems_on_host) + @mock.patch('nova.virt.hardware.get_vpmems') def test_get_ordered_vpmems(self, mock_labels): # get orgered vpmems based on flavor extra_specs From 74b2af473c1813931a8d9ea0e11aed7f2f6c8756 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 6 Feb 2019 17:47:32 +0000 Subject: [PATCH 22/54] docs: Rework the PCI passthrough guides Rewrite the document, making the following changes: - Remove use of bullet points in favour of more descriptive steps - Cross-reference various configuration options - Emphasise that ``[pci] alias`` must be set on both controller and compute node - Style nits, such as fixing the header style Change-Id: I2ac7df7d235f0af25f5a99bc8f6abddbae2cb3af Signed-off-by: Stephen Finucane Related-Bug: #1852727 (cherry picked from commit d5259abfe163058b13ad943ad16a5c281c2080e7) --- doc/source/admin/pci-passthrough.rst | 163 +++++++++++++++------------ 1 file changed, 88 insertions(+), 75 deletions(-) diff --git a/doc/source/admin/pci-passthrough.rst b/doc/source/admin/pci-passthrough.rst index 52388db53a3..1884a217a5f 100644 --- a/doc/source/admin/pci-passthrough.rst +++ b/doc/source/admin/pci-passthrough.rst @@ -22,83 +22,98 @@ assigned to only one guest and cannot be shared. **Limitations** - * Attaching SR-IOV ports to existing servers is not currently supported, see - `bug 1708433 `_ for details. + * Attaching SR-IOV ports to existing servers is not currently supported. + This is now rejected by the API but previously it fail later in the + process. See `bug 1708433 `_ + for details. * Cold migration (resize) of servers with SR-IOV devices attached was not supported until the 14.0.0 Newton release, see `bug 1512800 `_ for details. -To enable PCI passthrough, follow the steps below: +To enable PCI passthrough, follow the steps below. -#. Configure nova-scheduler (Controller) +.. note:: -#. Configure nova-api (Controller)** + The PCI device with address ``0000:41:00.0``, the vendor ID of ``8086`` and + the product ID of ``154d`` is used as an example. This will differ between + environments. -#. Configure a flavor (Controller) -#. Enable PCI passthrough (Compute) +Configure ``nova-scheduler`` (Controller) +----------------------------------------- -#. Configure PCI devices in nova-compute (Compute) +The :program:`nova-scheduler` service must be configured to enable the +``PciPassthroughFilter``. To do this, add this filter to the list of filters +specified in :oslo.config:option:`filter_scheduler.enabled_filters` and set +:oslo.config:option:`filter_scheduler.available_filters` to the default of +``nova.scheduler.filters.all_filters``. For example: -.. note:: +.. code-block:: ini + + [filter_scheduler] + enabled_filters = ...,PciPassthroughFilter + available_filters = nova.scheduler.filters.all_filters - The PCI device with address ``0000:41:00.0`` is used as an example. This - will differ between environments. +Once done, restart the :program:`nova-scheduler` service. -Configure nova-scheduler (Controller) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -#. Configure ``nova-scheduler`` as specified in :neutron-doc:`Configure - nova-scheduler - `. +.. _pci-passthrough-alias: -#. Restart the ``nova-scheduler`` service. +Configure ``nova-api`` (Controller) +----------------------------------- -Configure nova-api (Controller) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +PCI devices are requested through flavor extra specs, specifically via the +``pci_passthrough:alias`` flavor extra spec. However, the aliases themselves +must be configured. This done via the :oslo.config:option:`pci.alias` +configuration option. For example, to configure a PCI alias ``a1`` to request +a PCI device with a vendor ID of ``0x8086`` and a product ID of ``0x154d``: -#. Specify the PCI alias for the device. +.. code-block:: ini - Configure a PCI alias ``a1`` to request a PCI device with a ``vendor_id`` of - ``0x8086`` and a ``product_id`` of ``0x154d``. The ``vendor_id`` and - ``product_id`` correspond the PCI device with address ``0000:41:00.0``. + [pci] + alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } - Edit ``/etc/nova/nova.conf``: +Refer to :oslo.config:option:`pci.alias` for syntax information. - .. code-block:: ini +Once configured, restart the :program:`nova-api` service. - [pci] - alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } +.. important:: - Refer to :oslo.config:option:`pci.alias` for syntax information. + This option must also be configured on compute nodes. This is discussed later + in this document. -#. Restart the ``nova-api`` service. -Configure a flavor (Controller) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Configure a flavor (API) +------------------------ -Configure a flavor to request two PCI devices, each with ``vendor_id`` of -``0x8086`` and ``product_id`` of ``0x154d``: +Once the alias has been configured, it can be used for an flavor extra spec. +For example, to request two of the PCI devices referenced by alias ``a1``, run: .. code-block:: console - # openstack flavor set m1.large --property "pci_passthrough:alias"="a1:2" + $ openstack flavor set m1.large --property "pci_passthrough:alias"="a1:2" For more information about the syntax for ``pci_passthrough:alias``, refer to :ref:`Flavors `. -Enable PCI passthrough (Compute) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Enable VT-d and IOMMU. For more information, refer to steps one and two in -:neutron-doc:`Create Virtual Functions -`. +Configure host (Compute) +------------------------ -For Hyper-V compute nodes, the requirements are as follows: +To enable PCI passthrough on an x86, Linux-based compute node, the following +are required: -* Windows 10 or Windows / Hyper-V Server 2016 or newer. -* VT-d and SR-IOV enabled on the host. -* Assignable PCI devices. +* VT-d enabled in the BIOS +* IOMMU enabled on the host OS, by adding the ``intel_iommu=on`` or + ``amd_iommu=on`` parameter to the kernel parameters +* Assignable PCIe devices + +To enable PCI passthrough on a Hyper-V compute node, the following are +required: + +* Windows 10 or Windows / Hyper-V Server 2016 or newer +* VT-d enabled on the host +* Assignable PCI devices In order to check the requirements above and if there are any assignable PCI devices, run the following Powershell commands: @@ -115,56 +130,54 @@ passthrough`__. .. __: https://devblogs.microsoft.com/scripting/passing-through-devices-to-hyper-v-vms-by-using-discrete-device-assignment/ -Configure PCI devices (Compute) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -#. Configure ``nova-compute`` to allow the PCI device to pass through to - VMs. Edit ``/etc/nova/nova.conf``: - .. code-block:: ini +Configure ``nova-compute`` (Compute) +------------------------------------ - [pci] - passthrough_whitelist = { "address": "0000:41:00.0" } +Once PCI passthrough has been configured for the host, :program:`nova-compute` +must be configured to allow the PCI device to pass through to VMs. This is done +using the :oslo.config:option:`pci.passthrough_whitelist` option. For example, +to enable passthrough of a specific device using its address: - Alternatively specify multiple PCI devices using whitelisting: +.. code-block:: ini - .. code-block:: ini + [pci] + passthrough_whitelist = { "address": "0000:41:00.0" } - [pci] - passthrough_whitelist = { "vendor_id": "8086", "product_id": "10fb" } +Refer to :oslo.config:option:`pci.passthrough_whitelist` for syntax information. - All PCI devices matching the ``vendor_id`` and ``product_id`` are added to - the pool of PCI devices available for passthrough to VMs. +Alternatively, to enable passthrough of all devices with the same product and +vendor ID: - Refer to :oslo.config:option:`pci.passthrough_whitelist` for syntax - information. +.. code-block:: ini -#. Specify the PCI alias for the device. + [pci] + passthrough_whitelist = { "vendor_id": "8086", "product_id": "154d" } - From the Newton release, to resize guest with PCI device, configure the PCI - alias on the compute node as well. +If using vendor and product IDs, all PCI devices matching the ``vendor_id`` and +``product_id`` are added to the pool of PCI devices available for passthrough +to VMs. - Configure a PCI alias ``a1`` to request a PCI device with a ``vendor_id`` of - ``0x8086`` and a ``product_id`` of ``0x154d``. The ``vendor_id`` and - ``product_id`` correspond the PCI device with address ``0000:41:00.0``. +In addition, it is necessary to configure the :oslo.config:option:`pci.alias` +option on the compute node too. This is required to allow resizes of guests +with PCI devices. This should be identical to the alias configured +:ref:`previously `. For example: - Edit ``/etc/nova/nova.conf``: +.. code-block:: ini - .. code-block:: ini + [pci] + alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } - [pci] - alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } +Refer to :oslo.config:option:`pci.alias` for syntax information. - Refer to :oslo.config:option:`pci.alias` for syntax information. +Once configured, restart the :program:`nova-compute` service. -#. Restart the ``nova-compute`` service. Create instances with PCI passthrough devices -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +--------------------------------------------- -The ``nova-scheduler`` selects a destination host that has PCI devices -available with the specified ``vendor_id`` and ``product_id`` that matches the -``alias`` from the flavor. +The :program:`nova-scheduler` service selects a destination host that has PCI +devices available that match the ``alias`` specified in the flavor. .. code-block:: console From 9223613ad3805357003a2cbf6c8600ab6ed3dbcf Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 15 Nov 2019 11:33:26 +0000 Subject: [PATCH 23/54] docs: Change order of PCI configuration steps It doesn't really make sense to describe the "higher level" configuration steps necessary for PCI passthrough before describing things like BIOS configuration. Simply switch the ordering. Change-Id: I4ea1d9a332d6585ce2c0d5a531fa3c4ad9c89482 Signed-off-by: Stephen Finucane Related-Bug: #1852727 (cherry picked from commit 557728abaf0c822f2b1a5cdd4fb2e11e19d8ead7) --- doc/source/admin/pci-passthrough.rst | 142 +++++++++++++-------------- 1 file changed, 70 insertions(+), 72 deletions(-) diff --git a/doc/source/admin/pci-passthrough.rst b/doc/source/admin/pci-passthrough.rst index 1884a217a5f..c51cd802ff5 100644 --- a/doc/source/admin/pci-passthrough.rst +++ b/doc/source/admin/pci-passthrough.rst @@ -15,9 +15,16 @@ as multiple PCI devices. Virtual PCI devices are assigned to the same or different guests. In the case of PCI passthrough, the full physical device is assigned to only one guest and cannot be shared. +PCI devices are requested through flavor extra specs, specifically via the +``pci_passthrough:alias=`` flavor extra spec. This guide demonstrates +how to enable PCI passthrough for a type of PCI device with a vendor ID of +``8086`` and a product ID of ``154d`` - an Intel X520 Network Adapter - by +mapping them to the alias ``a1``. You should adjust the instructions for other +devices with potentially different capabilities. + .. note:: - For information on creating servers with virtual SR-IOV devices, refer to + For information on creating servers with SR-IOV network interfaces, refer to the :neutron-doc:`Networking Guide `. **Limitations** @@ -30,72 +37,6 @@ assigned to only one guest and cannot be shared. supported until the 14.0.0 Newton release, see `bug 1512800 `_ for details. -To enable PCI passthrough, follow the steps below. - -.. note:: - - The PCI device with address ``0000:41:00.0``, the vendor ID of ``8086`` and - the product ID of ``154d`` is used as an example. This will differ between - environments. - - -Configure ``nova-scheduler`` (Controller) ------------------------------------------ - -The :program:`nova-scheduler` service must be configured to enable the -``PciPassthroughFilter``. To do this, add this filter to the list of filters -specified in :oslo.config:option:`filter_scheduler.enabled_filters` and set -:oslo.config:option:`filter_scheduler.available_filters` to the default of -``nova.scheduler.filters.all_filters``. For example: - -.. code-block:: ini - - [filter_scheduler] - enabled_filters = ...,PciPassthroughFilter - available_filters = nova.scheduler.filters.all_filters - -Once done, restart the :program:`nova-scheduler` service. - - -.. _pci-passthrough-alias: - -Configure ``nova-api`` (Controller) ------------------------------------ - -PCI devices are requested through flavor extra specs, specifically via the -``pci_passthrough:alias`` flavor extra spec. However, the aliases themselves -must be configured. This done via the :oslo.config:option:`pci.alias` -configuration option. For example, to configure a PCI alias ``a1`` to request -a PCI device with a vendor ID of ``0x8086`` and a product ID of ``0x154d``: - -.. code-block:: ini - - [pci] - alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } - -Refer to :oslo.config:option:`pci.alias` for syntax information. - -Once configured, restart the :program:`nova-api` service. - -.. important:: - - This option must also be configured on compute nodes. This is discussed later - in this document. - - -Configure a flavor (API) ------------------------- - -Once the alias has been configured, it can be used for an flavor extra spec. -For example, to request two of the PCI devices referenced by alias ``a1``, run: - -.. code-block:: console - - $ openstack flavor set m1.large --property "pci_passthrough:alias"="a1:2" - -For more information about the syntax for ``pci_passthrough:alias``, refer to -:ref:`Flavors `. - Configure host (Compute) ------------------------ @@ -104,7 +45,7 @@ To enable PCI passthrough on an x86, Linux-based compute node, the following are required: * VT-d enabled in the BIOS -* IOMMU enabled on the host OS, by adding the ``intel_iommu=on`` or +* IOMMU enabled on the host OS, e.g. by adding the ``intel_iommu=on`` or ``amd_iommu=on`` parameter to the kernel parameters * Assignable PCIe devices @@ -137,7 +78,7 @@ Configure ``nova-compute`` (Compute) Once PCI passthrough has been configured for the host, :program:`nova-compute` must be configured to allow the PCI device to pass through to VMs. This is done using the :oslo.config:option:`pci.passthrough_whitelist` option. For example, -to enable passthrough of a specific device using its address: +assuming our sample PCI device has a PCI address of ``41:00.0`` on each host: .. code-block:: ini @@ -159,9 +100,10 @@ If using vendor and product IDs, all PCI devices matching the ``vendor_id`` and to VMs. In addition, it is necessary to configure the :oslo.config:option:`pci.alias` -option on the compute node too. This is required to allow resizes of guests -with PCI devices. This should be identical to the alias configured -:ref:`previously `. For example: +option. As noted previously, PCI devices are requested through flavor extra +specs - specifically via the ``pci_passthrough:alias`` flavor extra spec - so +this config option allows us to map a given type of device to a specific alias. +For example, to map the sample PCI device to the alias ``a1``: .. code-block:: ini @@ -170,9 +112,65 @@ with PCI devices. This should be identical to the alias configured Refer to :oslo.config:option:`pci.alias` for syntax information. +.. important:: + + This option must also be configured on controller nodes. This is discussed later + in this document. + Once configured, restart the :program:`nova-compute` service. +Configure ``nova-scheduler`` (Controller) +----------------------------------------- + +The :program:`nova-scheduler` service must be configured to enable the +``PciPassthroughFilter``. To do this, add this filter to the list of filters +specified in :oslo.config:option:`filter_scheduler.enabled_filters` and set +:oslo.config:option:`filter_scheduler.available_filters` to the default of +``nova.scheduler.filters.all_filters``. For example: + +.. code-block:: ini + + [filter_scheduler] + enabled_filters = ...,PciPassthroughFilter + available_filters = nova.scheduler.filters.all_filters + +Once done, restart the :program:`nova-scheduler` service. + + +.. _pci-passthrough-alias: + +Configure ``nova-api`` (Controller) +----------------------------------- + +It is necessary to also configure the :oslo.config:option:`pci.alias` config +option on the controller. This configuration should match the configuration +found on the compute nodes. For example: + +.. code-block:: ini + + [pci] + alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } + +Refer to :oslo.config:option:`pci.alias` for syntax information. + +Once configured, restart the :program:`nova-api` service. + + +Configure a flavor (API) +------------------------ + +Once the alias has been configured, it can be used for an flavor extra spec. +For example, to request two of the PCI devices referenced by alias ``a1``, run: + +.. code-block:: console + + $ openstack flavor set m1.large --property "pci_passthrough:alias"="a1:2" + +For more information about the syntax for ``pci_passthrough:alias``, refer to +:ref:`Flavors `. + + Create instances with PCI passthrough devices --------------------------------------------- From 0c0c5b12cf05b5dfb2c3f2e8e4799e4acf94a2ea Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 15 Nov 2019 10:16:28 +0000 Subject: [PATCH 24/54] docs: Clarify configuration steps for PF devices Devices that report SR-IOV capabilities cannot be used without special configuration - namely, the addition of "'device_type': 'type-PF'" or "'device_type': 'type-VF'" to the '[pci] alias' configuration option. Spell this out in the docs. Change-Id: I4abbe30505a5e4ccba16027addd6d5f45066e31b Signed-off-by: Stephen Finucane Closes-Bug: #1852727 (cherry picked from commit 810aafc5ec9a7d25b33cf6c137c47b117c91269a) --- doc/source/admin/pci-passthrough.rst | 29 ++++++++++++++++++++++++---- nova/conf/pci.py | 3 ++- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/doc/source/admin/pci-passthrough.rst b/doc/source/admin/pci-passthrough.rst index c51cd802ff5..48abf1b2032 100644 --- a/doc/source/admin/pci-passthrough.rst +++ b/doc/source/admin/pci-passthrough.rst @@ -100,16 +100,37 @@ If using vendor and product IDs, all PCI devices matching the ``vendor_id`` and to VMs. In addition, it is necessary to configure the :oslo.config:option:`pci.alias` -option. As noted previously, PCI devices are requested through flavor extra -specs - specifically via the ``pci_passthrough:alias`` flavor extra spec - so -this config option allows us to map a given type of device to a specific alias. -For example, to map the sample PCI device to the alias ``a1``: +option, which is a JSON-style configuration option that allows you to map a +given device type, identified by the standard PCI ``vendor_id`` and (optional) +``product_id`` fields, to an arbitrary name or *alias*. This alias can then be +used to request a PCI device using the ``pci_passthrough:alias=`` flavor +extra spec, as discussed previously. For our sample device with a vendor ID of +``0x8086`` and a product ID of ``0x154d``, this would be: .. code-block:: ini [pci] alias = { "vendor_id":"8086", "product_id":"154d", "device_type":"type-PF", "name":"a1" } +It's important to note the addition of the ``device_type`` field. This is +necessary because this PCI device supports SR-IOV. The ``nova-compute`` service +categorizes devices into one of three types, depending on the capabilities the +devices report: + +``type-PF`` + The device supports SR-IOV and is the parent or root device. + +``type-VF`` + The device is a child device of a device that supports SR-IOV. + +``type-PCI`` + The device does not support SR-IOV. + +By default, it is only possible to attach ``type-PCI`` devices using PCI +passthrough. If you wish to attach ``type-PF`` or ``type-VF`` devices, you must +specify the ``device_type`` field in the config option. If the device was a +device that did not support SR-IOV, the ``device_type`` field could be omitted. + Refer to :oslo.config:option:`pci.alias` for syntax information. .. important:: diff --git a/nova/conf/pci.py b/nova/conf/pci.py index 28a68362612..b812b39676a 100644 --- a/nova/conf/pci.py +++ b/nova/conf/pci.py @@ -60,7 +60,8 @@ ``device_type`` Type of PCI device. Valid values are: ``type-PCI``, ``type-PF`` and - ``type-VF``. + ``type-VF``. Note that ``"device_type": "type-PF"`` **must** be specified + if you wish to passthrough a device that supports SR-IOV in its entirety. ``numa_policy`` Required NUMA affinity of device. Valid values are: ``legacy``, From 4653245ddcf989ebac4b964a41d881d78cf9ae2c Mon Sep 17 00:00:00 2001 From: Johannes Kulik Date: Thu, 27 Feb 2020 08:08:32 +0100 Subject: [PATCH 25/54] Validate id as integer for os-aggregates According to the api-ref, the id passed to calls in os-aggregates is supposed to be an integer. No function validated this, so any value passed to these functions would directly reach the DB. While this is fine for SQLite, making a query with a string for an integer column on other databases like PostgreSQL results in a DBError exception and thus a HTTP 500 instead of 400 or 404. This commit adds validation for the id parameter the same way it's already done for other endpoints. Conflicts: nova/api/openstack/compute/aggregates.py Changes: nova/tests/unit/api/openstack/compute/test_aggregates.py NOTE(stephenfin): Conflicts are due to absence of change I4ab96095106b38737ed355fcad07e758f8b5a9b0 ("Add image caching API for aggregates") which we don't want to backport. A test related to this feature must also be removed. Change-Id: I83817f7301680801beaee375825f02eda526eda1 Closes-Bug: 1865040 (cherry picked from commit 2e70a1717f25652912886cbefa3f40e6df908c00) --- nova/api/openstack/compute/aggregates.py | 41 +++++++++++- .../api/openstack/compute/test_aggregates.py | 65 ++++++++++++++----- 2 files changed, 85 insertions(+), 21 deletions(-) diff --git a/nova/api/openstack/compute/aggregates.py b/nova/api/openstack/compute/aggregates.py index 4fce9e9ce2b..f522e323cec 100644 --- a/nova/api/openstack/compute/aggregates.py +++ b/nova/api/openstack/compute/aggregates.py @@ -28,6 +28,7 @@ from nova import exception from nova.i18n import _ from nova.policies import aggregates as aggr_policies +from nova import utils def _get_context(req): @@ -85,11 +86,17 @@ def create(self, req, body): return agg - @wsgi.expected_errors(404) + @wsgi.expected_errors((400, 404)) def show(self, req, id): """Shows the details of an aggregate, hosts and metadata included.""" context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'show') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.get_aggregate(context, id) except exception.AggregateNotFound as e: @@ -107,6 +114,11 @@ def update(self, req, id, body): if 'name' in updates: updates['name'] = common.normalize_name(updates['name']) + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.update_aggregate(context, id, updates) except exception.AggregateNameExists as e: @@ -126,6 +138,12 @@ def delete(self, req, id): """Removes an aggregate by id.""" context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'delete') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: self.api.delete_aggregate(context, id) except exception.AggregateNotFound as e: @@ -136,7 +154,7 @@ def delete(self, req, id): # NOTE(gmann): Returns 200 for backwards compatibility but should be 202 # for representing async API as this API just accepts the request and # request hypervisor driver to complete the same in async mode. - @wsgi.expected_errors((404, 409)) + @wsgi.expected_errors((400, 404, 409)) @wsgi.action('add_host') @validation.schema(aggregates.add_host) def _add_host(self, req, id, body): @@ -145,6 +163,12 @@ def _add_host(self, req, id, body): context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'add_host') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.add_host_to_aggregate(context, id, host) except (exception.AggregateNotFound, @@ -159,7 +183,7 @@ def _add_host(self, req, id, body): # NOTE(gmann): Returns 200 for backwards compatibility but should be 202 # for representing async API as this API just accepts the request and # request hypervisor driver to complete the same in async mode. - @wsgi.expected_errors((404, 409)) + @wsgi.expected_errors((400, 404, 409)) @wsgi.action('remove_host') @validation.schema(aggregates.remove_host) def _remove_host(self, req, id, body): @@ -168,6 +192,12 @@ def _remove_host(self, req, id, body): context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'remove_host') + + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + try: aggregate = self.api.remove_host_from_aggregate(context, id, host) except (exception.AggregateNotFound, exception.AggregateHostNotFound, @@ -189,6 +219,11 @@ def _set_metadata(self, req, id, body): context = _get_context(req) context.can(aggr_policies.POLICY_ROOT % 'set_metadata') + try: + utils.validate_integer(id, 'id') + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + metadata = body["set_metadata"]["metadata"] try: aggregate = self.api.update_aggregate_metadata(context, diff --git a/nova/tests/unit/api/openstack/compute/test_aggregates.py b/nova/tests/unit/api/openstack/compute/test_aggregates.py index eb185849a64..c14f562dc72 100644 --- a/nova/tests/unit/api/openstack/compute/test_aggregates.py +++ b/nova/tests/unit/api/openstack/compute/test_aggregates.py @@ -299,7 +299,7 @@ def test_show_no_admin(self): self.controller.show, self.user_req, "1") - def test_show_with_invalid_id(self): + def test_show_with_bad_aggregate(self): side_effect = exception.AggregateNotFound(aggregate_id='2') with mock.patch.object(self.controller.api, 'get_aggregate', side_effect=side_effect) as mock_get: @@ -307,6 +307,10 @@ def test_show_with_invalid_id(self): self.req, "2") mock_get.assert_called_once_with(self.context, '2') + def test_show_with_invalid_id(self): + self.assertRaises(exc.HTTPBadRequest, self.controller.show, + self.req, 'foo') + def test_update(self): body = {"aggregate": {"name": "new_name", "availability_zone": "nova1"}} @@ -390,10 +394,11 @@ def test_update_with_empty_availability_zone(self): @mock.patch('nova.compute.api.AggregateAPI.update_aggregate') def test_update_with_none_availability_zone(self, mock_update_agg): - agg_id = uuidsentinel.aggregate + agg_id = 173 mock_update_agg.return_value = objects.Aggregate(self.context, name='test', - uuid=agg_id, + uuid=uuidsentinel.agg, + id=agg_id, hosts=[], metadata={}) body = {"aggregate": {"name": "test", @@ -414,6 +419,11 @@ def test_update_with_bad_aggregate(self): mock_update.assert_called_once_with(self.context, '2', body["aggregate"]) + def test_update_with_invalid_id(self): + body = {"aggregate": {"name": "test_name"}} + self.assertRaises(exc.HTTPBadRequest, self.controller.update, + self.req, 'foo', body=body) + def test_update_with_duplicated_name(self): body = {"aggregate": {"name": "test_name"}} side_effect = exception.AggregateNameExists(aggregate_name="test_name") @@ -433,7 +443,7 @@ def test_invalid_action(self): def test_update_with_invalid_action(self): with mock.patch.object(self.controller.api, "update_aggregate", side_effect=exception.InvalidAggregateAction( - action='invalid', aggregate_id='agg1', reason= "not empty")): + action='invalid', aggregate_id='1', reason= "not empty")): body = {"aggregate": {"availability_zone": "nova"}} self.assertRaises(exc.HTTPBadRequest, self.controller.update, self.req, "1", body=body) @@ -467,15 +477,20 @@ def test_add_host_with_already_added_host(self): def test_add_host_with_bad_aggregate(self): side_effect = exception.AggregateNotFound( - aggregate_id="bogus_aggregate") + aggregate_id="2") with mock.patch.object(self.controller.api, 'add_host_to_aggregate', side_effect=side_effect) as mock_add: self.assertRaises(exc.HTTPNotFound, eval(self.add_host), - self.req, "bogus_aggregate", + self.req, "2", body={"add_host": {"host": "host1"}}) - mock_add.assert_called_once_with(self.context, "bogus_aggregate", + mock_add.assert_called_once_with(self.context, "2", "host1") + def test_add_host_with_invalid_id(self): + body = {"add_host": {"host": "host1"}} + self.assertRaises(exc.HTTPBadRequest, eval(self.add_host), + self.req, 'foo', body=body) + def test_add_host_with_bad_host(self): side_effect = exception.ComputeHostNotFound(host="bogus_host") with mock.patch.object(self.controller.api, 'add_host_to_aggregate', @@ -534,16 +549,21 @@ def test_remove_host_no_admin(self): def test_remove_host_with_bad_aggregate(self): side_effect = exception.AggregateNotFound( - aggregate_id="bogus_aggregate") + aggregate_id="2") with mock.patch.object(self.controller.api, 'remove_host_from_aggregate', side_effect=side_effect) as mock_rem: self.assertRaises(exc.HTTPNotFound, eval(self.remove_host), - self.req, "bogus_aggregate", + self.req, "2", body={"remove_host": {"host": "host1"}}) - mock_rem.assert_called_once_with(self.context, "bogus_aggregate", + mock_rem.assert_called_once_with(self.context, "2", "host1") + def test_remove_host_with_invalid_id(self): + body = {"remove_host": {"host": "host1"}} + self.assertRaises(exc.HTTPBadRequest, eval(self.remove_host), + self.req, 'foo', body=body) + def test_remove_host_with_host_not_in_aggregate(self): side_effect = exception.AggregateHostNotFound(aggregate_id="1", host="host1") @@ -621,16 +641,21 @@ def test_set_metadata_no_admin(self): def test_set_metadata_with_bad_aggregate(self): body = {"set_metadata": {"metadata": {"foo": "bar"}}} - side_effect = exception.AggregateNotFound(aggregate_id="bad_aggregate") + side_effect = exception.AggregateNotFound(aggregate_id="2") with mock.patch.object(self.controller.api, 'update_aggregate_metadata', side_effect=side_effect) as mock_update: self.assertRaises(exc.HTTPNotFound, eval(self.set_metadata), - self.req, "bad_aggregate", body=body) - mock_update.assert_called_once_with(self.context, "bad_aggregate", + self.req, "2", body=body) + mock_update.assert_called_once_with(self.context, "2", body["set_metadata"]['metadata']) + def test_set_metadata_with_invalid_id(self): + body = {"set_metadata": {"metadata": {"foo": "bar"}}} + self.assertRaises(exc.HTTPBadRequest, eval(self.set_metadata), + self.req, 'foo', body=body) + def test_set_metadata_with_missing_metadata(self): body = {"asdf": {"foo": "bar"}} self.assertRaises(self.bad_request, eval(self.set_metadata), @@ -679,21 +704,25 @@ def test_delete_aggregate_no_admin(self): def test_delete_aggregate_with_bad_aggregate(self): side_effect = exception.AggregateNotFound( - aggregate_id="bogus_aggregate") + aggregate_id="2") with mock.patch.object(self.controller.api, 'delete_aggregate', side_effect=side_effect) as mock_del: self.assertRaises(exc.HTTPNotFound, self.controller.delete, - self.req, "bogus_aggregate") - mock_del.assert_called_once_with(self.context, "bogus_aggregate") + self.req, "2") + mock_del.assert_called_once_with(self.context, "2") + + def test_delete_with_invalid_id(self): + self.assertRaises(exc.HTTPBadRequest, self.controller.delete, + self.req, 'foo') def test_delete_aggregate_with_host(self): with mock.patch.object(self.controller.api, "delete_aggregate", side_effect=exception.InvalidAggregateAction( - action="delete", aggregate_id="agg1", + action="delete", aggregate_id="2", reason="not empty")): self.assertRaises(exc.HTTPBadRequest, self.controller.delete, - self.req, "agg1") + self.req, "2") def test_marshall_aggregate(self): # _marshall_aggregate() just basically turns the aggregate returned From b2037fc4e356b55949339a1358c16431a9ab8930 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 9 Dec 2020 10:08:19 +0000 Subject: [PATCH 26/54] [stable-only] Cap bandit to 1.6.2 and raise hacking, flake8 and stestr The 1.6.3 [1] release has dropped support for py2 [2] so cap to 1.6.2 when using py2. This change also raises hacking to 1.1.0 in lower-constraints.txt after it was bumped by I35c654bd39f343417e0a1124263ff31dcd0b05c9. This also means that flake8 is bumped to 2.6.0. stestr is also bumped to 2.0.0 as required by oslotest 3.8.0. All of these changes are squashed into a single change to pass the gate. [1] https://github.com/PyCQA/bandit/releases/tag/1.6.3 [2] https://github.com/PyCQA/bandit/pull/615 Depends-On: https://review.opendev.org/c/openstack/devstack/+/768256 Depends-On: https://review.opendev.org/c/openstack/swift/+/766214 Closes-Bug: #1907438 Closes-Bug: #1907756 Change-Id: Ie5221bf37c6ed9268a4aa0737ffcdd811e39360a --- lower-constraints.txt | 6 +++--- test-requirements.txt | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index 18a15993fd3..597a4466c17 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -27,14 +27,14 @@ eventlet==0.20.0 extras==1.0.0 fasteners==0.14.1 fixtures==3.0.0 -flake8==2.5.5 +flake8==2.6.0 future==0.16.0 futurist==1.8.0 gabbi==1.35.0 gitdb2==2.0.3 GitPython==2.1.8 greenlet==0.4.10 -hacking==0.12.0 +hacking==1.1.0 idna==2.6 iso8601==0.1.11 Jinja2==2.10 @@ -149,7 +149,7 @@ SQLAlchemy==1.2.19 sqlalchemy-migrate==0.11.0 sqlparse==0.2.4 statsd==3.2.2 -stestr==1.0.0 +stestr==2.0.0 stevedore==1.20.0 suds-jurko==0.6 taskflow==2.16.0 diff --git a/test-requirements.txt b/test-requirements.txt index a3e369f85a6..98a6bba99ab 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -16,12 +16,12 @@ python-barbicanclient>=4.5.2 # Apache-2.0 python-ironicclient!=2.7.1,>=2.7.0 # Apache-2.0 requests-mock>=1.2.0 # Apache-2.0 oslotest>=3.8.0 # Apache-2.0 -stestr>=1.0.0 # Apache-2.0 +stestr>=2.0.0 # Apache-2.0 osprofiler>=1.4.0 # Apache-2.0 testresources>=2.0.0 # Apache-2.0/BSD testscenarios>=0.4 # Apache-2.0/BSD testtools>=2.2.0 # MIT -bandit>=1.1.0 # Apache-2.0 +bandit>=1.1.0,<=1.6.2 # Apache-2.0 gabbi>=1.35.0 # Apache-2.0 wsgi-intercept>=1.7.0 # MIT License From b6cc7e9aacf8d66a6a881ed698f36bb9e4443c0d Mon Sep 17 00:00:00 2001 From: Takashi Natsume Date: Mon, 28 Sep 2020 22:52:51 +0900 Subject: [PATCH 27/54] Fix a hacking test In test_useless_assertion, the useless_assertion method should be checked instead of nonexistent_assertion_methods_and_attributes. Change-Id: Ifd19f636f58ae353d912bde57cba2cd0a29a9baa Signed-off-by: Takashi Natsume (cherry picked from commit 117508129461436e13c148bb068b0775d67e85d3) (cherry picked from commit f4d62e1a0b77f9611a3be8427adafd96caf24bb1) (cherry picked from commit 7562e64dee54a36162c3c181214aa269068f1712) --- nova/tests/unit/test_hacking.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index e942b3a0739..186c8383a0c 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -1007,7 +1007,7 @@ def test_useless_assertion(self): expected_errors=errors, filename="nova/tests/unit/test_context.py") # Check no errors in other than 'nova/tests' directory. self._assert_has_no_errors( - code, checks.nonexistent_assertion_methods_and_attributes, + code, checks.useless_assertion, filename="nova/compute/api.py") code = """ self.assertIsNone(None_test_var, "Fails") From 8378785f995dd4bec2a5a20f7bf5946b3075120d Mon Sep 17 00:00:00 2001 From: Hemanth Nakkina Date: Tue, 1 Sep 2020 09:36:51 +0530 Subject: [PATCH 28/54] Update pci stat pools based on PCI device changes At start up of nova-compute service, the PCI stat pools are populated based on information in pci_devices table in Nova database. The pools are updated only when new device is added or removed but not on any device changes like device type. If an existing device is configured as SRIOV and nova-compute is restarted, the pci_devices table gets updated but the device is still listed under the old pool in pci_tracker.stats.pool (in-memory object). This patch looks for device type updates in existing devices and updates the pools accordingly. Conflicts: nova/tests/functional/libvirt/test_pci_sriov_servers.py nova/tests/unit/virt/libvirt/fakelibvirt.py nova/tests/functional/libvirt/base.py To avoid the conflicts and make the new functional test execute, following changes are performed - Modified the test case to use flavor extra spec pci_passthrough :alias to create a server with sriov port instead of creating a sriov port and passing port information during server creation. - Removed changes in nova/tests/functional/libvirt/base.py as they are required only if neutron sriov port is created in the test case. Change-Id: Id4ebb06e634a612c8be4be6c678d8265e0b99730 Closes-Bug: #1892361 (cherry picked from commit b8695de6da56db42b83b9d9d4c330148766644be) (cherry picked from commit d8b8a8193b6b8228f6e7d6bde68b5ea6bb53dd8b) (cherry picked from commit f58399cf496566e39d11f82a61e0b47900f2eafa) --- nova/pci/manager.py | 1 + nova/pci/stats.py | 26 ++++ .../libvirt/test_pci_sriov_servers.py | 80 +++++++++++++ nova/tests/unit/pci/test_stats.py | 24 ++++ nova/tests/unit/virt/libvirt/fakelibvirt.py | 112 ++++++++++++------ ...i-deivce-type-update-c407a66fd37f6405.yaml | 12 ++ 6 files changed, 222 insertions(+), 33 deletions(-) create mode 100644 releasenotes/notes/bug-1892361-pci-deivce-type-update-c407a66fd37f6405.yaml diff --git a/nova/pci/manager.py b/nova/pci/manager.py index 3084643f5e8..05930b0bebb 100644 --- a/nova/pci/manager.py +++ b/nova/pci/manager.py @@ -225,6 +225,7 @@ def _set_hvdevs(self, devices): self.stale[new_value['address']] = new_value else: existed.update_device(new_value) + self.stats.update_device(existed) # Track newly discovered devices. for dev in [dev for dev in devices if diff --git a/nova/pci/stats.py b/nova/pci/stats.py index dcf26d3f74e..0a80ecedfc2 100644 --- a/nova/pci/stats.py +++ b/nova/pci/stats.py @@ -105,6 +105,32 @@ def _create_pool_keys_from_dev(self, dev): pool['parent_ifname'] = dev.extra_info['parent_ifname'] return pool + def _get_pool_with_device_type_mismatch(self, dev): + """Check for device type mismatch in the pools for a given device. + + Return (pool, device) if device type does not match or a single None + if the device type matches. + """ + for pool in self.pools: + for device in pool['devices']: + if device.address == dev.address: + if dev.dev_type != pool["dev_type"]: + return pool, device + return None + + return None + + def update_device(self, dev): + """Update a device to its matching pool.""" + pool_device_info = self._get_pool_with_device_type_mismatch(dev) + if pool_device_info is None: + return + + pool, device = pool_device_info + pool['devices'].remove(device) + self._decrease_pool_count(self.pools, pool) + self.add_device(dev) + def add_device(self, dev): """Add a device to its matching pool.""" dev_pool = self._create_pool_keys_from_dev(dev) diff --git a/nova/tests/functional/libvirt/test_pci_sriov_servers.py b/nova/tests/functional/libvirt/test_pci_sriov_servers.py index f203afec156..52b93c96192 100644 --- a/nova/tests/functional/libvirt/test_pci_sriov_servers.py +++ b/nova/tests/functional/libvirt/test_pci_sriov_servers.py @@ -13,11 +13,14 @@ # License for the specific language governing permissions and limitations # under the License. +import copy import fixtures import mock from oslo_log import log as logging from oslo_serialization import jsonutils +from nova import context +from nova import objects from nova.objects import fields from nova.tests.functional.libvirt import base from nova.tests.unit.virt.libvirt import fakelibvirt @@ -116,6 +119,20 @@ class SRIOVServersTest(_PCIServersTestBase): }, )] + def _disable_sriov_in_pf(self, pci_info): + # Check for PF and change the capability from virt_functions + # Delete all the VFs + vfs_to_delete = [] + + for device_name, device in pci_info.devices.items(): + if 'virt_functions' in device.pci_device: + device.generate_xml(skip_capability=True) + elif 'phys_function' in device.pci_device: + vfs_to_delete.append(device_name) + + for device in vfs_to_delete: + del pci_info.devices[device] + def test_create_server_with_VF(self): host_info = fakelibvirt.HostInfo(cpu_nodes=2, cpu_sockets=1, @@ -186,6 +203,69 @@ def test_create_server_with_VF_no_PF(self): self._run_build_test(flavor_id_vfs) self._run_build_test(flavor_id_pfs, end_status='ERROR') + def test_create_server_after_change_in_nonsriov_pf_to_sriov_pf(self): + # Starts a compute with PF not configured with SRIOV capabilities + # Updates the PF with SRIOV capability and restart the compute service + # Then starts a VM with the sriov port. The VM should be in active + # state with sriov port attached. + + # To emulate the device type changing, we first create a + # HostPCIDevicesInfo object with PFs and VFs. Then we make a copy + # and remove the VFs and the virt_function capability. This is + # done to ensure the physical function product id is same in both + # the versions. + host_info = fakelibvirt.HostInfo(cpu_nodes=2, cpu_sockets=1, + cpu_cores=2, cpu_threads=2, + kB_mem=15740000) + pci_info = fakelibvirt.HostPCIDevicesInfo(num_pfs=1, num_vfs=1) + pci_info_no_sriov = copy.deepcopy(pci_info) + + # Disable SRIOV capabilties in PF and delete the VFs + self._disable_sriov_in_pf(pci_info_no_sriov) + + fake_connection = self._get_connection(host_info, + pci_info=pci_info_no_sriov, + hostname='test_compute0') + self.mock_conn.return_value = fake_connection + + self.compute = self.start_service('compute', host='test_compute0') + + ctxt = context.get_admin_context() + pci_devices = objects.PciDeviceList.get_by_compute_node( + ctxt, + objects.ComputeNode.get_by_nodename( + ctxt, 'test_compute0', + ).id, + ) + self.assertEqual(1, len(pci_devices)) + self.assertEqual('type-PCI', pci_devices[0].dev_type) + + # Update connection with original pci info with sriov PFs + fake_connection = self._get_connection(host_info, + pci_info=pci_info, + hostname='test_compute0') + self.mock_conn.return_value = fake_connection + + # Restart the compute service + self.restart_compute_service(self.compute) + self.compute_started = True + + # Verify if PCI devices are of type type-PF or type-VF + pci_devices = objects.PciDeviceList.get_by_compute_node( + ctxt, + objects.ComputeNode.get_by_nodename( + ctxt, 'test_compute0', + ).id, + ) + for pci_device in pci_devices: + self.assertIn(pci_device.dev_type, ['type-PF', 'type-VF']) + + # Create a flavor + extra_spec = {"pci_passthrough:alias": "%s:1" % self.VFS_ALIAS_NAME} + flavor_id = self._create_flavor(extra_spec=extra_spec) + + self._run_build_test(flavor_id) + class GetServerDiagnosticsServerWithVfTestV21(_PCIServersTestBase): diff --git a/nova/tests/unit/pci/test_stats.py b/nova/tests/unit/pci/test_stats.py index be867783fb9..0375ccc63f6 100644 --- a/nova/tests/unit/pci/test_stats.py +++ b/nova/tests/unit/pci/test_stats.py @@ -533,6 +533,30 @@ def test_remove_device(self): self.pci_stats.remove_device(dev2) self._assertPools() + def test_update_device(self): + # Update device type of one of the device from type-PCI to + # type-PF. Verify if the existing pool is updated and a new + # pool is created with dev_type type-PF. + self._create_pci_devices() + dev1 = self.pci_tagged_devices.pop() + dev1.dev_type = 'type-PF' + self.pci_stats.update_device(dev1) + self.assertEqual(3, len(self.pci_stats.pools)) + self._assertPoolContent(self.pci_stats.pools[0], '1137', '0072', + len(self.pci_untagged_devices)) + self.assertEqual(self.pci_untagged_devices, + self.pci_stats.pools[0]['devices']) + self._assertPoolContent(self.pci_stats.pools[1], '1137', '0071', + len(self.pci_tagged_devices), + physical_network='physnet1') + self.assertEqual(self.pci_tagged_devices, + self.pci_stats.pools[1]['devices']) + self._assertPoolContent(self.pci_stats.pools[2], '1137', '0071', + 1, + physical_network='physnet1') + self.assertEqual(dev1, + self.pci_stats.pools[2]['devices'][0]) + class PciDeviceVFPFStatsTestCase(test.NoDBTestCase): diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index d10a44c7386..287a00efa25 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -251,60 +251,71 @@ def __init__(self, dev_type, slot, function, iommu_group, numa_node, applicable if ``dev_type`` is one of: ``PF``, ``VF``. """ - if dev_type == 'PCI': - if vf_ratio: + self.dev_type = dev_type + self.slot = slot + self.function = function + self.iommu_group = iommu_group + self.numa_node = numa_node + self.vf_ratio = vf_ratio + self.generate_xml() + + def generate_xml(self, skip_capability=False): + capability = '' + if self.dev_type == 'PCI': + if self.vf_ratio: raise ValueError('vf_ratio does not apply for PCI devices') prod_id = PCI_PROD_ID prod_name = PCI_PROD_NAME driver = PCI_DRIVER_NAME - capability = '' - elif dev_type == 'PF': + elif self.dev_type == 'PF': prod_id = PF_PROD_ID prod_name = PF_PROD_NAME driver = PF_DRIVER_NAME - capability = self.cap_templ % { - 'cap_type': PF_CAP_TYPE, - 'addresses': '\n'.join([ - self.addr_templ % { - # these are the slot, function values of the child VFs - # we can only assign 8 functions to a slot (0-7) so - # bump the slot each time we exceed this - 'slot': slot + (x // 8), - # ...and wrap the function value - 'function': x % 8, - # the offset is because the PF is occupying function 0 - } for x in range(1, vf_ratio + 1)]) - } - elif dev_type == 'VF': + if not skip_capability: + capability = self.cap_templ % { + 'cap_type': PF_CAP_TYPE, + 'addresses': '\n'.join([ + self.addr_templ % { + # these are the slot, function values of the child + # VFs, we can only assign 8 functions to a slot + # (0-7) so bump the slot each time we exceed this + 'slot': self.slot + (x // 8), + # ...and wrap the function value + 'function': x % 8, + # the offset is because the PF is occupying function 0 + } for x in range(1, self.vf_ratio + 1)]) + } + elif self.dev_type == 'VF': prod_id = VF_PROD_ID prod_name = VF_PROD_NAME driver = VF_DRIVER_NAME - capability = self.cap_templ % { - 'cap_type': VF_CAP_TYPE, - 'addresses': self.addr_templ % { - # this is the slot, function value of the parent PF - # if we're e.g. device 8, we'll have a different slot - # to our parent so reverse this - 'slot': slot - ((vf_ratio + 1) // 8), - # the parent PF is always function 0 - 'function': 0, + if not skip_capability: + capability = self.cap_templ % { + 'cap_type': VF_CAP_TYPE, + 'addresses': self.addr_templ % { + # this is the slot, function value of the parent PF + # if we're e.g. device 8, we'll have a different slot + # to our parent so reverse this + 'slot': self.slot - ((self.vf_ratio + 1) // 8), + # the parent PF is always function 0 + 'function': 0, + } } - } else: raise ValueError('Expected one of: PCI, VF, PCI') self.pci_device = self.pci_device_template % { - 'slot': slot, - 'function': function, + 'slot': self.slot, + 'function': self.function, 'vend_id': PCI_VEND_ID, 'vend_name': PCI_VEND_NAME, 'prod_id': prod_id, 'prod_name': prod_name, 'driver': driver, 'capability': capability, - 'iommu_group': iommu_group, - 'numa_node': numa_node, + 'iommu_group': self.iommu_group, + 'numa_node': self.numa_node, } def XMLDesc(self, flags): @@ -847,6 +858,20 @@ def _parse_definition(self, xml): nic_info['source'] = source.get('network') elif nic_info['type'] == 'bridge': nic_info['source'] = source.get('bridge') + elif nic_info['type'] == 'hostdev': + # is for VF when vnic_type + # is direct. Add sriov vf pci information in nic_info + address = source.find('./address') + pci_type = address.get('type') + pci_domain = address.get('domain').replace('0x', '') + pci_bus = address.get('bus').replace('0x', '') + pci_slot = address.get('slot').replace('0x', '') + pci_function = address.get('function').replace( + '0x', '') + pci_device = "%s_%s_%s_%s_%s" % (pci_type, pci_domain, + pci_bus, pci_slot, + pci_function) + nic_info['source'] = pci_device nics_info += [nic_info] @@ -888,11 +913,32 @@ def _parse_definition(self, xml): return definition + def verify_hostdevs_interface_are_vfs(self): + """Verify for interface type hostdev if the pci device is VF or not. + """ + + error_message = ("Interface type hostdev is currently supported on " + "SR-IOV Virtual Functions only") + + nics = self._def['devices'].get('nics', []) + for nic in nics: + if nic['type'] == 'hostdev': + pci_device = nic['source'] + pci_info_from_connection = self._connection.pci_info.devices[ + pci_device] + if 'phys_function' not in pci_info_from_connection.pci_device: + raise make_libvirtError( + libvirtError, + error_message, + error_code=VIR_ERR_CONFIG_UNSUPPORTED, + error_domain=VIR_FROM_DOMAIN) + def create(self): self.createWithFlags(0) def createWithFlags(self, flags): # FIXME: Not handling flags at the moment + self.verify_hostdevs_interface_are_vfs() self._state = VIR_DOMAIN_RUNNING self._connection._mark_running(self) self._has_saved_state = False @@ -1016,7 +1062,7 @@ def XMLDesc(self, flags): nics = '' for nic in self._def['devices']['nics']: - if 'source' in nic: + if 'source' in nic and nic['type'] != 'hostdev': nics += ''' diff --git a/releasenotes/notes/bug-1892361-pci-deivce-type-update-c407a66fd37f6405.yaml b/releasenotes/notes/bug-1892361-pci-deivce-type-update-c407a66fd37f6405.yaml new file mode 100644 index 00000000000..ba35c25b02d --- /dev/null +++ b/releasenotes/notes/bug-1892361-pci-deivce-type-update-c407a66fd37f6405.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Fixes `bug 1892361`_ in which the pci stat pools are not updated when an + existing device is enabled with SRIOV capability. Restart of nova-compute + service updates the pci device type from type-PCI to type-PF but the pools + still maintain the device type as type-PCI. And so the PF is considered for + allocation to instance that requests vnic_type=direct. With this fix, the + pci device type updates are detected and the pci stat pools are updated + properly. + + .. _bug 1892361: https://bugs.launchpad.net/nova/+bug/1892361 From 4839d41fc0c084fe56106c879c468f1553e8e780 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 21 Sep 2020 17:21:18 +0200 Subject: [PATCH 29/54] Use cell targeted context to query BDMs for metadata The metadata service supports a multicell deployment in a configuration where the nova-api service implements the metadata API. In this case the metadata query needs to be cell targeted. This was partly implemented already. The instance itself is queried from the cell DB properly. However the BDM data used the non targeted context resulting in an empty BDM returned by the metadata service. Functional reproduction test is not added as I did not find a way to have a cell setup in the functional test that reproduce the problem. I reproduced the bug and tested the fix in a devstack. Change-Id: I48f57082edaef3ec4722bd31ce29a90b94d32523 Closes-Bug: #1881944 (cherry picked from commit 1390eecf8dec4c3b022a9b1e259a908197566738) (cherry picked from commit 9a5b6249d65c96e387a9eb0ecdefcc2057c21869) (cherry picked from commit c727cfc36c9d352102e04b9c94bebc7e25a1ab97) --- nova/api/metadata/base.py | 12 +++++++----- nova/tests/unit/test_metadata.py | 16 ++++++++++------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/nova/api/metadata/base.py b/nova/api/metadata/base.py index a53b51c3333..3cba366be06 100644 --- a/nova/api/metadata/base.py +++ b/nova/api/metadata/base.py @@ -122,8 +122,13 @@ def __init__(self, instance, address=None, content=None, extra_md=None, if not content: content = [] + # NOTE(gibi): this is not a cell targeted context even if we are called + # in a situation when the instance is in a different cell than the + # metadata service itself. ctxt = context.get_admin_context() + self.mappings = _format_instance_mapping(instance) + # NOTE(danms): Sanitize the instance to limit the amount of stuff # inside that may not pickle well (i.e. context). We also touch # some of the things we'll lazy load later to make sure we keep their @@ -145,8 +150,6 @@ def __init__(self, instance, address=None, content=None, extra_md=None, self.security_groups = secgroup_api.get_instance_security_groups( ctxt, instance) - self.mappings = _format_instance_mapping(ctxt, instance) - if instance.user_data is not None: self.userdata_raw = base64.decode_as_bytes(instance.user_data) else: @@ -683,9 +686,8 @@ def get_metadata_by_instance_id(instance_id, address, ctxt=None): return InstanceMetadata(instance, address) -def _format_instance_mapping(ctxt, instance): - bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( - ctxt, instance.uuid) +def _format_instance_mapping(instance): + bdms = instance.get_bdms() return block_device.instance_block_mapping(instance, bdms) diff --git a/nova/tests/unit/test_metadata.py b/nova/tests/unit/test_metadata.py index 7a6ecf3af24..c14544ba152 100644 --- a/nova/tests/unit/test_metadata.py +++ b/nova/tests/unit/test_metadata.py @@ -329,12 +329,14 @@ def test_format_instance_mapping(self): 'uuid': 'e5fe5518-0288-4fa3-b0c4-c79764101b85', 'root_device_name': None, 'default_ephemeral_device': None, - 'default_swap_device': None}) + 'default_swap_device': None, + 'context': self.context}) instance_ref1 = objects.Instance(**{'id': 0, 'uuid': 'b65cee2f-8c69-4aeb-be2f-f79742548fc2', 'root_device_name': '/dev/sda1', 'default_ephemeral_device': None, - 'default_swap_device': None}) + 'default_swap_device': None, + 'context': self.context}) def fake_bdm_get(ctxt, uuid): return [fake_block_device.FakeDbBlockDeviceDict( @@ -373,10 +375,12 @@ def fake_bdm_get(ctxt, uuid): 'swap': '/dev/sdc', 'ebs0': '/dev/sdh'} - self.assertEqual(base._format_instance_mapping(self.context, - instance_ref0), block_device._DEFAULT_MAPPINGS) - self.assertEqual(base._format_instance_mapping(self.context, - instance_ref1), expected) + self.assertEqual( + base._format_instance_mapping(instance_ref0), + block_device._DEFAULT_MAPPINGS) + self.assertEqual( + base._format_instance_mapping(instance_ref1), + expected) def test_pubkey(self): md = fake_InstanceMetadata(self, self.instance.obj_clone()) From 4350074029ffbc03ab238c442ec86fab6560e365 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Tue, 20 Oct 2020 21:46:13 +0000 Subject: [PATCH 30/54] Use subqueryload() instead of joinedload() for (system_)metadata Currently, when we "get" a single instance from the database and we load metadata and system_metadata, we do so using a joinedload() which does JOINs with the respective tables. Because of the one-to-many relationship between an instance and (system_)metadata records, doing the database query this way can result in a large number of additional rows being returned unnecessarily and cause a large data transfer. This is similar to the problem addressed by change I0610fb16ccce2ee95c318589c8abcc30613a3fe9 which added separate queries for (system_)metadata when we "get" multiple instances. We don't, however, reuse the same code for this change because _instances_fill_metadata converts the instance database object to a dict, and some callers of _instance_get_by_uuid need to be able to access an instance database object attached to the session (example: instance_update_and_get_original). By using subqueryload() [1], we can perform the additional queries for (system_)metadata to solve the problem with a similar approach. Closes-Bug: #1799298 [1] https://docs.sqlalchemy.org/en/13/orm/loading_relationships.html#subquery-eager-loading Change-Id: I5c071f70f669966e9807b38e99077c1cae5b4606 (cherry picked from commit e728fe668a6de886455f2dbaf655c8a151462c8c) (cherry picked from commit 63d2e62c3a223f883ca810f4c66a2a236cf3d483) (cherry picked from commit e7a45e0335e4cf44fec7f7b8d2505f5b95445cf9) --- nova/db/sqlalchemy/api.py | 17 ++++++++++++++++- nova/tests/unit/db/test_db_api.py | 8 ++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 02311f240d5..92fe2ed6175 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -48,6 +48,7 @@ from sqlalchemy.orm import contains_eager from sqlalchemy.orm import joinedload from sqlalchemy.orm import noload +from sqlalchemy.orm import subqueryload from sqlalchemy.orm import undefer from sqlalchemy.schema import Table from sqlalchemy import sql @@ -1930,13 +1931,27 @@ def _build_instance_get(context, columns_to_join=None): continue if 'extra.' in column: query = query.options(undefer(column)) + elif column in ['metadata', 'system_metadata']: + # NOTE(melwitt): We use subqueryload() instead of joinedload() for + # metadata and system_metadata because of the one-to-many + # relationship of the data. Directly joining these columns can + # result in a large number of additional rows being queried if an + # instance has a large number of (system_)metadata items, resulting + # in a large data transfer. Instead, the subqueryload() will + # perform additional queries to obtain metadata and system_metadata + # for the instance. + query = query.options(subqueryload(column)) else: query = query.options(joinedload(column)) # NOTE(alaski) Stop lazy loading of columns not needed. for col in ['metadata', 'system_metadata']: if col not in columns_to_join: query = query.options(noload(col)) - return query + # NOTE(melwitt): We need to use order_by() so that the + # additional queries emitted by subqueryload() include the same ordering as + # used by the parent query. + # https://docs.sqlalchemy.org/en/13/orm/loading_relationships.html#the-importance-of-ordering + return query.order_by(models.Instance.id) def _instances_fill_metadata(context, instances, manual_joins=None): diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index fb42d1af524..76f301e7868 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -2086,6 +2086,14 @@ def test_instance_get_all_with_meta(self): sys_meta = utils.metadata_to_dict(inst['system_metadata']) self.assertEqual(sys_meta, self.sample_data['system_metadata']) + def test_instance_get_with_meta(self): + inst_id = self.create_instance_with_args().id + inst = db.instance_get(self.ctxt, inst_id) + meta = utils.metadata_to_dict(inst['metadata']) + self.assertEqual(meta, self.sample_data['metadata']) + sys_meta = utils.metadata_to_dict(inst['system_metadata']) + self.assertEqual(sys_meta, self.sample_data['system_metadata']) + def test_instance_update(self): instance = self.create_instance_with_args() metadata = {'host': 'bar', 'key2': 'wuff'} From 62bf0ef129550b05fde9c98586cc29e401860586 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Wed, 30 Sep 2020 17:26:21 +0000 Subject: [PATCH 31/54] Disallow CONF.compute.max_disk_devices_to_attach = 0 The CONF.compute.max_disk_devices_to_attach option controls the maximum number of disk devices allowed to attach to an instance. If it is set to 0, it will literally allow no disk device for instances, preventing them from being able to boot. This adds a note to the config option help to call this out and changes nova-compute to raise InvalidConfiguration during init_host if [compute]max_disk_devices_to_attach has been set to 0. The nova-compute service will fail to start if the option is set to 0. Note: there doesn't appear to be any way to disallow particular values in a oslo.config IntOpt other than the min/max values. Here we need the min value to be -1 to represent unlimited. There is a 'choices' kwarg available but that is only for enumerating valid values and we need to allow any integer >= 1 as well. Change-Id: I6e30468bc28f661ddc17937ab1de04a706f05063 Closes-Bug: #1897950 (cherry picked from commit 25a632a4e1daa1941a6297ddb51088972f23ce6d) (cherry picked from commit 8e12b81839e1fb364b3598699fecf57c78f81df3) (cherry picked from commit 4ad7e5e2630542bce4cfa53adafd1da299adb0f6) --- nova/compute/manager.py | 7 +++++++ nova/conf/compute.py | 8 +++++++- nova/tests/unit/compute/test_compute_mgr.py | 5 +++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index eaedc0238f6..f7bb00df7b1 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -1330,6 +1330,13 @@ def init_host(self): eventlet.semaphore.BoundedSemaphore( CONF.compute.max_concurrent_disk_ops) + if CONF.compute.max_disk_devices_to_attach == 0: + msg = _('[compute]max_disk_devices_to_attach has been set to 0, ' + 'which will prevent instances from being able to boot. ' + 'Set -1 for unlimited or set >= 1 to limit the maximum ' + 'number of disk devices.') + raise exception.InvalidConfiguration(msg) + self.driver.init_host(host=self.host) context = nova.context.get_admin_context() instances = objects.InstanceList.get_by_host( diff --git a/nova/conf/compute.py b/nova/conf/compute.py index fccebabb370..6713f61d47b 100644 --- a/nova/conf/compute.py +++ b/nova/conf/compute.py @@ -946,10 +946,16 @@ The configured maximum is not enforced on shelved offloaded servers, as they have no compute host. +.. warning:: If this option is set to 0, the ``nova-compute`` service will fail + to start, as 0 disk devices is an invalid configuration that would + prevent instances from being able to boot. + Possible values: * -1 means unlimited -* Any integer >= 0 represents the maximum allowed +* Any integer >= 1 represents the maximum allowed. A value of 0 will cause the + ``nova-compute`` service to fail to start, as 0 disk devices is an invalid + configuration that would prevent instances from being able to boot. """), ] diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 67dc20f6bd2..250d06dfc40 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -963,6 +963,11 @@ def test_init_host_with_in_progress_evacuations(self, mock_destroy_evac, mock_error_interrupted.assert_called_once_with( self.context, {active_instance.uuid, evacuating_instance.uuid}) + def test_init_host_disk_devices_configuration_failure(self): + self.flags(max_disk_devices_to_attach=0, group='compute') + self.assertRaises(exception.InvalidConfiguration, + self.compute.init_host) + @mock.patch.object(objects.InstanceList, 'get_by_host', new=mock.Mock()) @mock.patch('nova.compute.manager.ComputeManager.' From e9e0998978f931e318690723ec4a4aed823e2943 Mon Sep 17 00:00:00 2001 From: Andrew Bonney Date: Tue, 6 Oct 2020 14:42:38 +0100 Subject: [PATCH 32/54] Handle disabled CPU features to fix live migration failures When performing a live migration between hypervisors running libvirt, where one or more CPU features are disabled, nova does not take account of these. This results in migration failures as none of the available hypervisor targets appear compatible. This patch ensures that the libvirt 'disable' poicy is taken account of, at least in a basic sense, by explicitly ignoring items flagged in this way when enumerating CPU features. Closes-Bug: #1898715 Change-Id: Iaf14ca97cfac99dd280d1114123f2d4bb6292b63 (cherry picked from commit eeeca4ceff576beaa8558360c8a6a165d716f996) (cherry picked from commit 45a4110d20a574d0c43431a4b87497920c9cbe06) (cherry picked from commit b6c473159ec45e0aa715edd45cde28f77484a5f7) --- nova/tests/unit/virt/libvirt/test_config.py | 41 +++++++++++++++++++++ nova/virt/libvirt/config.py | 8 +++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_config.py b/nova/tests/unit/virt/libvirt/test_config.py index bc30776561d..968582dc15b 100644 --- a/nova/tests/unit/virt/libvirt/test_config.py +++ b/nova/tests/unit/virt/libvirt/test_config.py @@ -338,6 +338,26 @@ def test_config_simple(self): """) + def test_config_parse_require(self): + xml = """ + + """ + xmldoc = etree.fromstring(xml) + obj = config.LibvirtConfigCPUFeature() + obj.parse_dom(xmldoc) + + self.assertEqual(obj.policy, "require") + + def test_config_parse_disable(self): + xml = """ + + """ + xmldoc = etree.fromstring(xml) + obj = config.LibvirtConfigCPUFeature() + obj.parse_dom(xmldoc) + + self.assertEqual(obj.policy, "disable") + class LibvirtConfigGuestCPUFeatureTest(LibvirtConfigBaseTest): @@ -436,6 +456,27 @@ def test_config_complex(self): """) + def test_config_disabled_features(self): + obj = config.LibvirtConfigCPU() + obj.model = "Penryn" + obj.vendor = "Intel" + obj.arch = obj_fields.Architecture.X86_64 + + disabled_feature = config.LibvirtConfigCPUFeature("mtrr") + disabled_feature.policy = "disable" + obj.add_feature(disabled_feature) + obj.add_feature(config.LibvirtConfigCPUFeature("apic")) + + xml = obj.to_xml() + self.assertXmlEqual(xml, """ + + x86_64 + Penryn + Intel + + + """) + def test_only_uniq_cpu_featues(self): obj = config.LibvirtConfigCPU() obj.model = "Penryn" diff --git a/nova/virt/libvirt/config.py b/nova/virt/libvirt/config.py index d0f9f215558..39c4da88478 100644 --- a/nova/virt/libvirt/config.py +++ b/nova/virt/libvirt/config.py @@ -596,11 +596,13 @@ def __init__(self, name=None, **kwargs): **kwargs) self.name = name + self.policy = "require" def parse_dom(self, xmldoc): super(LibvirtConfigCPUFeature, self).parse_dom(xmldoc) self.name = xmldoc.get("name") + self.policy = xmldoc.get("policy", "require") def format_dom(self): ft = super(LibvirtConfigCPUFeature, self).format_dom() @@ -652,7 +654,8 @@ def parse_dom(self, xmldoc): elif c.tag == "feature": f = LibvirtConfigCPUFeature() f.parse_dom(c) - self.add_feature(f) + if f.policy != "disable": + self.add_feature(f) def format_dom(self): cpu = super(LibvirtConfigCPU, self).format_dom() @@ -675,7 +678,8 @@ def format_dom(self): # sorting the features to allow more predictable tests for f in sorted(self.features, key=lambda x: x.name): - cpu.append(f.format_dom()) + if f.policy != "disable": + cpu.append(f.format_dom()) return cpu From 3c8841595ac2ff5d3552150078e968f40cf94727 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Mon, 7 Dec 2020 12:52:39 +0000 Subject: [PATCH 33/54] tools: Allow check-cherry-picks.sh to be disabled by an env var The checks performed by this script aren't always useful to downstream consumers of the repo so allow them to disable the script without having to make changes to tox.ini. NOTE(lyarwood): This backport has Ie8a672fd21184c810bfe9c0e3a49582189bf2111 squashed into it to ensure the introduced env var is passed into the pep8 tox env. tox: Add passenv DISABLE_CHERRY_PICK_CHECK to pep8 I4f551dc4b57905cab8aa005c5680223ad1b57639 introduced the environment variable to disable the check-cherry-pick.sh script but forgot to allow it to be passed into the pep8 tox env. Change-Id: I4f551dc4b57905cab8aa005c5680223ad1b57639 (cherry picked from commit 610396f8ad5fe8c3abb7731fcd42c81e5246a938) (cherry picked from commit 3d86df068a7a78f8faa9ac6437ff4fc19f5cebcc) (cherry picked from commit cb96119e59df64d6884431c8f2e8f6ced8318bd7) --- tools/check-cherry-picks.sh | 5 +++++ tox.ini | 2 ++ 2 files changed, 7 insertions(+) diff --git a/tools/check-cherry-picks.sh b/tools/check-cherry-picks.sh index 5a449c520b7..5ca6ded2033 100755 --- a/tools/check-cherry-picks.sh +++ b/tools/check-cherry-picks.sh @@ -4,6 +4,11 @@ # to verify that they're all on either master or stable/ branches # +# Allow this script to be disabled by a simple env var +if [ ${DISABLE_CHERRY_PICK_CHECK:-0} -eq 1 ]; then + exit 0 +fi + commit_hash="" # Check if the patch is a merge patch by counting the number of parents. diff --git a/tox.ini b/tox.ini index 209875c44f7..f0d1971a3ea 100644 --- a/tox.ini +++ b/tox.ini @@ -51,6 +51,8 @@ commands = description = Run style checks. envdir = {toxworkdir}/shared +passenv = + DISABLE_CHERRY_PICK_CHECK commands = bash tools/flake8wrap.sh {posargs} # Check that all JSON files don't have \r\n in line. From 98048ee1393b98573a1d667b5518331d408c62f5 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Tue, 9 Jun 2020 00:27:39 +0000 Subject: [PATCH 34/54] Raise InstanceMappingNotFound if StaleDataError is encountered We have a race where if a user issues a delete request while an instance is in the middle of booting, we could fail to update the 'queued_for_delete' field on the instance mapping with: sqlalchemy.orm.exc.StaleDataError: UPDATE statement on table 'instance_mappings' expected to update 1 row(s); 0 were matched. This happens if we've retrieved the instance mapping record from the database and then it gets deleted by nova-conductor before we attempt to save() it. This handles the situation by adding try-except around the update call to catch StaleDataError and raise InstanceMappingNotFound instead, which the caller does know how to handle. Closes-Bug: #1882608 Change-Id: I2cdcad7226312ed81f4242c8d9ac919715524b48 (cherry picked from commit 16df22dcd57a73fe3be15c64c41b4081b4826ef2) (cherry picked from commit 812ce632d50bfc32de62d544746e0b9a83d90ab7) --- nova/objects/instance_mapping.py | 13 +++++++++++-- nova/tests/unit/objects/test_instance_mapping.py | 9 +++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/nova/objects/instance_mapping.py b/nova/objects/instance_mapping.py index 0392f04770a..8b5f6ba92e7 100644 --- a/nova/objects/instance_mapping.py +++ b/nova/objects/instance_mapping.py @@ -15,6 +15,7 @@ from oslo_log import log as logging from oslo_utils import versionutils import six +from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import joinedload from sqlalchemy.sql import false from sqlalchemy.sql import func @@ -161,8 +162,16 @@ def _save_in_db(context, instance_uuid, updates): def save(self): changes = self.obj_get_changes() changes = self._update_with_cell_id(changes) - db_mapping = self._save_in_db(self._context, self.instance_uuid, - changes) + try: + db_mapping = self._save_in_db(self._context, self.instance_uuid, + changes) + except orm_exc.StaleDataError: + # NOTE(melwitt): If the instance mapping has been deleted out from + # under us by conductor (delete requested while booting), we will + # encounter a StaleDataError after we retrieved the row and try to + # update it after it's been deleted. We can treat this like an + # instance mapping not found and allow the caller to handle it. + raise exception.InstanceMappingNotFound(uuid=self.instance_uuid) self._from_db_object(self._context, self, db_mapping) self.obj_reset_changes() diff --git a/nova/tests/unit/objects/test_instance_mapping.py b/nova/tests/unit/objects/test_instance_mapping.py index ec50517a20c..2c877c0a1f5 100644 --- a/nova/tests/unit/objects/test_instance_mapping.py +++ b/nova/tests/unit/objects/test_instance_mapping.py @@ -12,6 +12,7 @@ import mock from oslo_utils import uuidutils +from sqlalchemy.orm import exc as orm_exc from nova import exception from nova import objects @@ -151,6 +152,14 @@ def test_save(self, save_in_db): comparators={ 'cell_mapping': self._check_cell_map_value}) + @mock.patch.object(instance_mapping.InstanceMapping, '_save_in_db') + def test_save_stale_data_error(self, save_in_db): + save_in_db.side_effect = orm_exc.StaleDataError + mapping_obj = objects.InstanceMapping(self.context) + mapping_obj.instance_uuid = uuidutils.generate_uuid() + + self.assertRaises(exception.InstanceMappingNotFound, mapping_obj.save) + @mock.patch.object(instance_mapping.InstanceMapping, '_destroy_in_db') def test_destroy(self, destroy_in_db): uuid = uuidutils.generate_uuid() From f60253677830483e63f73bcfd554f5e165cfc850 Mon Sep 17 00:00:00 2001 From: ramboman Date: Wed, 22 Apr 2020 21:33:22 +0800 Subject: [PATCH 35/54] replace the "hide_hypervisor_id" to "hw:hide_hypervisor_id" When we use the flavor extra_specs "hide_hypervisor_id" in AggregateInstanceExtraSpecsFilter, then will retrun False. So we need correct the extra_specs. NOTE: The first two files do not exist in stable/train due to extra specs validators (patch Ib64a1348cce1dca995746214616c4f33d9d664bd) was introduced in Ussuri. The last one file has conflict due to the same bp/flavor-extra-sepc-validators feature (patch: I8da84b48e4d630eeb91d92346aa2323e25e28e3b) added in Ussuri. Conflicts: nova/api/validation/extra_specs/hw.py nova/api/validation/extra_specs/null.py nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py Change-Id: I9d8d8c3a30cf6da7e8fb48374347e069ab075df2 Closes-Bug: 1841932 (cherry picked from commit bf488a8630702160021b5848bf6e86fbb8015205) (cherry picked from commit 9d28d7ec808469ec129b66c69b9e63cd9537a63f) --- doc/source/user/flavors.rst | 6 ++++- nova/tests/unit/virt/libvirt/test_driver.py | 27 ++++++++++++++----- nova/virt/libvirt/driver.py | 4 ++- .../notes/bug-1841932-c871ac7b3b05d67e.yaml | 9 +++++++ 4 files changed, 37 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/bug-1841932-c871ac7b3b05d67e.yaml diff --git a/doc/source/user/flavors.rst b/doc/source/user/flavors.rst index 3e24fc0072d..740a4edee02 100644 --- a/doc/source/user/flavors.rst +++ b/doc/source/user/flavors.rst @@ -706,10 +706,14 @@ Hiding hypervisor signature As of the 18.0.0 Rocky release, this is only supported by the libvirt driver. + Prior to the 21.0.0 Ussuri release, this was called + ``hide_hypervisor_id``. An alias is provided to provide backwards + compatibility. + .. code:: console $ openstack flavor set FLAVOR-NAME \ - --property hide_hypervisor_id=VALUE + --property hw:hide_hypervisor_id=VALUE Where: diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 3b131f4da71..4fec9db3862 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -4445,6 +4445,19 @@ def test_get_guest_config_windows_hyperv_feature2(self): self._test_get_guest_config_windows_hyperv() def test_get_guest_config_windows_hyperv_all_hide_flv(self): + # Similar to test_get_guest_config_windows_hyperv_feature2 + # but also test hiding the HyperV signature with the flavor + # extra_spec "hw:hide_hypervisor_id" + flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, + extra_specs={"hw:hide_hypervisor_id": "true"}, + expected_attrs={"extra_specs"}) + # this works for kvm (the default, tested below) and qemu + self.flags(virt_type='qemu', group='libvirt') + + self._test_get_guest_config_windows_hyperv( + flavor=flavor_hide_id, hvid_hidden=True) + + def test_get_guest_config_windows_hyperv_all_hide_flv_old(self): # Similar to test_get_guest_config_windows_hyperv_feature2 # but also test hiding the HyperV signature with the flavor # extra_spec "hide_hypervisor_id" @@ -4471,10 +4484,10 @@ def test_get_guest_config_windows_hyperv_all_hide_img(self): def test_get_guest_config_windows_hyperv_all_hide_flv_img(self): # Similar to test_get_guest_config_windows_hyperv_feature2 # but also test hiding the HyperV signature with both the flavor - # extra_spec "hide_hypervisor_id" and the image property + # extra_spec "hw:hide_hypervisor_id" and the image property # "img_hide_hypervisor_id" flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, - extra_specs={"hide_hypervisor_id": "true"}, + extra_specs={"hw:hide_hypervisor_id": "true"}, expected_attrs={"extra_specs"}) self.flags(virt_type='qemu', group='libvirt') @@ -6518,7 +6531,7 @@ def test_get_guest_config_with_hiding_hypervisor_id_flavor_extra_specs( self): # Input to the test: flavor extra_specs flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, - extra_specs={"hide_hypervisor_id": "true"}, + extra_specs={"hw:hide_hypervisor_id": "true"}, expected_attrs={"extra_specs"}) self.flags(virt_type='kvm', group='libvirt') @@ -6544,7 +6557,7 @@ def test_get_guest_config_with_hiding_hypervisor_id_img_and_flavor( # Input to the test: image metadata (true) and flavor # extra_specs (true) flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, - extra_specs={"hide_hypervisor_id": "true"}, + extra_specs={"hw:hide_hypervisor_id": "true"}, expected_attrs={"extra_specs"}) image_meta = objects.ImageMeta.from_dict({ "disk_format": "raw", @@ -6571,7 +6584,7 @@ def test_get_guest_config_with_hiding_hypervisor_id_img_or_flavor( # Input to the test: image metadata (false) and flavor # extra_specs (true) flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, - extra_specs={"hide_hypervisor_id": "true"}, + extra_specs={"hw:hide_hypervisor_id": "true"}, expected_attrs={"extra_specs"}) image_meta = objects.ImageMeta.from_dict({ "disk_format": "raw", @@ -6596,7 +6609,7 @@ def test_get_guest_config_with_hiding_hypervisor_id_img_or_flavor( # Input to the test: image metadata (true) and flavor # extra_specs (false) flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, - extra_specs={"hide_hypervisor_id": "false"}, + extra_specs={"hw:hide_hypervisor_id": "false"}, expected_attrs={"extra_specs"}) image_meta = objects.ImageMeta.from_dict({ "disk_format": "raw", @@ -6643,7 +6656,7 @@ def test_get_guest_config_without_hiding_hypervisor_id(self): def test_get_guest_config_without_hiding_hypervisor_id_flavor_extra_specs( self): flavor_hide_id = fake_flavor.fake_flavor_obj(self.context, - extra_specs={"hide_hypervisor_id": "false"}, + extra_specs={"hw:hide_hypervisor_id": "false"}, expected_attrs={"extra_specs"}) self.flags(virt_type='qemu', group='libvirt') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index f4df91e4940..aead8470b29 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -5126,7 +5126,9 @@ def _set_features(self, guest, os_type, caps, virt_type, image_meta, flavor): hide_hypervisor_id = (strutils.bool_from_string( flavor.extra_specs.get('hide_hypervisor_id')) or - image_meta.properties.get('img_hide_hypervisor_id')) + strutils.bool_from_string( + flavor.extra_specs.get('hw:hide_hypervisor_id')) or + image_meta.properties.get('img_hide_hypervisor_id')) if virt_type == "xen": # PAE only makes sense in X86 diff --git a/releasenotes/notes/bug-1841932-c871ac7b3b05d67e.yaml b/releasenotes/notes/bug-1841932-c871ac7b3b05d67e.yaml new file mode 100644 index 00000000000..d54be4f03a9 --- /dev/null +++ b/releasenotes/notes/bug-1841932-c871ac7b3b05d67e.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Add support for the ``hw:hide_hypervisor_id`` extra spec. This is an + alias for the ``hide_hypervisor_id`` extra spec, which was not + compatible with the ``AggregateInstanceExtraSpecsFilter`` scheduler + filter. See + `bug 1841932 `_ for more + details. From ff570d1b4e9b9777405ae75cc09eae2ce255bf19 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Thu, 11 Mar 2021 15:08:12 +0000 Subject: [PATCH 36/54] [stable-only] gate: Pin CEPH_RELEASE to nautilus in LM hook I1edd5a50079f325fa143a7e0d51b3aa3bb5ed45d moved the branchless devstack-plugin-ceph project to the Octopus release of Ceph that drops support for py2. As this was still the default on stable/train this breaks the nova-live-migration and nova-grenade jobs. This change works around this by pinning the CEPH_RELEASE to nautilus within the LM hook as was previously used prior to the above landing. Note that the devstack-plugin-ceph-tempest job from the plugin repo continues to pass as it is correctly pinned to the Luminous release that supports py2. If anything the above enforces the need to move away from these hook scripts and instead inherit our base ceph jobs from this repo in the future to avoid the Ceph release jumping around like this. Change-Id: I1d029ebe78b16ed2d4345201b515baf3701533d5 --- gate/live_migration/hooks/ceph.sh | 1 + gate/live_migration/hooks/run_tests.sh | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/gate/live_migration/hooks/ceph.sh b/gate/live_migration/hooks/ceph.sh index 483f92b0e1a..c588f7c9b29 100755 --- a/gate/live_migration/hooks/ceph.sh +++ b/gate/live_migration/hooks/ceph.sh @@ -8,6 +8,7 @@ function prepare_ceph { configure_ceph #install ceph-common package on compute nodes $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m raw -a "executable=/bin/bash + export CEPH_RELEASE=nautilus source $BASE/new/devstack/functions source $BASE/new/devstack/functions-common git clone https://opendev.org/openstack/devstack-plugin-ceph /tmp/devstack-plugin-ceph diff --git a/gate/live_migration/hooks/run_tests.sh b/gate/live_migration/hooks/run_tests.sh index 00ad3416349..7a5027f2c9a 100755 --- a/gate/live_migration/hooks/run_tests.sh +++ b/gate/live_migration/hooks/run_tests.sh @@ -55,6 +55,11 @@ fi echo '4. test with Ceph for root + ephemeral disks' # Discover and set variables for the OS version so the devstack-plugin-ceph # scripts can find the correct repository to install the ceph packages. +# NOTE(lyarwood): Pin the CEPH_RELEASE to nautilus here as was the case +# prior to https://review.opendev.org/c/openstack/devstack-plugin-ceph/+/777232 +# landing in the branchless plugin, we also have to pin in ceph.sh when +# configuring ceph on a remote node via ansible. +export CEPH_RELEASE=nautilus GetOSVersion prepare_ceph GLANCE_API_CONF=${GLANCE_API_CONF:-/etc/glance/glance-api.conf} From fb81b16df09bedadecca9acd234ef137a427e02a Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Mon, 28 Sep 2020 12:18:29 +0100 Subject: [PATCH 37/54] compute: Lock by instance.uuid lock during swap_volume The libvirt driver is currently the only virt driver implementing swap volume within Nova. While libvirt itself does support moving between multiple volumes attached to the same instance at the same time the current logic within the libvirt driver makes a call to virDomainGetXMLDesc that fails if there are active block jobs against any disk attached to the domain. This change simply uses an instance.uuid based lock in the compute layer to serialise requests to swap_volume to avoid this from being possible. Closes-Bug: #1896621 Change-Id: Ic5ce2580e7638a47f1ffddb4edbb503bf490504c (cherry picked from commit 6cf449bdd0d4beb95cf12311e7d2f8669e625fac) (cherry picked from commit eebf94b6540fcd16826067fac544b5a3238842a3) (cherry picked from commit f7ba1aab5f6f76ba88d6cc63cde2ec246ee61ec5) --- nova/compute/manager.py | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index f7bb00df7b1..6e5d72f27a3 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -6462,9 +6462,33 @@ def _swap_volume(self, context, instance, bdm, connector, @wrap_instance_fault def swap_volume(self, context, old_volume_id, new_volume_id, instance, new_attachment_id): - """Swap volume for an instance.""" - context = context.elevated() + """Replace the old volume with the new volume within the active server + :param context: User request context + :param old_volume_id: Original volume id + :param new_volume_id: New volume id being swapped to + :param instance: Instance with original_volume_id attached + :param new_attachment_id: ID of the new attachment for new_volume_id + """ + @utils.synchronized(instance.uuid) + def _do_locked_swap_volume(context, old_volume_id, new_volume_id, + instance, new_attachment_id): + self._do_swap_volume(context, old_volume_id, new_volume_id, + instance, new_attachment_id) + _do_locked_swap_volume(context, old_volume_id, new_volume_id, instance, + new_attachment_id) + + def _do_swap_volume(self, context, old_volume_id, new_volume_id, + instance, new_attachment_id): + """Replace the old volume with the new volume within the active server + + :param context: User request context + :param old_volume_id: Original volume id + :param new_volume_id: New volume id being swapped to + :param instance: Instance with original_volume_id attached + :param new_attachment_id: ID of the new attachment for new_volume_id + """ + context = context.elevated() compute_utils.notify_about_volume_swap( context, instance, self.host, fields.NotificationPhase.START, From 8559ceeec4e52651a272d08ef344d7c03520ed19 Mon Sep 17 00:00:00 2001 From: Josephine Seifert Date: Wed, 17 Mar 2021 08:09:47 +0100 Subject: [PATCH 38/54] Add config parameter 'live_migration_scheme' to live migration with tls guide This patch adds the config option 'live_migration_scheme = tls' to the secure live migration guide. To let the live migration use the qemu native tls, some configuration of the compute nodes is needed. The guide describes this but misses the 'live_migration_scheme' config option. It is necessary to set 'live_migration_scheme' to tls to use the connection uri for encrypted traffic. Without this parameter everything seems to work, but the unencrypted tcp-connection is still used for the live migration. Closes-Bug: #1919357 Change-Id: Ia5130d411706bf7e1c983156158011a3bc6d5cd6 (cherry picked from commit 5d5ff82bab1dfb12e6223446b1cf20db6a348f38) (cherry picked from commit 276b8db5afd945bfc56ccfadd25bcce1b6df9cb7) (cherry picked from commit a968289b1f89b52d7396b66294f768c732583849) --- .../secure-live-migration-with-qemu-native-tls.rst | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/doc/source/admin/secure-live-migration-with-qemu-native-tls.rst b/doc/source/admin/secure-live-migration-with-qemu-native-tls.rst index 012d78e93be..fb76f656af8 100644 --- a/doc/source/admin/secure-live-migration-with-qemu-native-tls.rst +++ b/doc/source/admin/secure-live-migration-with-qemu-native-tls.rst @@ -120,10 +120,13 @@ Performing the migration (1) On all relevant compute nodes, enable the :oslo.config:option:`libvirt.live_migration_with_native_tls` - configuration attribute:: + configuration attribute and set the + :oslo.config:option:`libvirt.live_migration_scheme` + configuration attribute to tls:: [libvirt] live_migration_with_native_tls = true + live_migration_scheme = tls .. note:: Setting both @@ -131,6 +134,12 @@ Performing the migration :oslo.config:option:`libvirt.live_migration_tunnelled` at the same time is invalid (and disallowed). + .. note:: + Not setting + :oslo.config:option:`libvirt.live_migration_scheme` to ``tls`` + will result in libvirt using the unencrypted TCP connection + without displaying any error or a warning in the logs. + And restart the ``nova-compute`` service:: $ systemctl restart openstack-nova-compute From 9e9c022bde3a3ffdf0dd87e21bf9afde0dbc1e74 Mon Sep 17 00:00:00 2001 From: Vu Tran Date: Mon, 29 Aug 2016 16:33:19 -0400 Subject: [PATCH 39/54] Improve error log when snapshot fails If snapshot creation via glance fails due to lack of space or over quota, we want to have a clearer error message. Change-Id: Ic9133f6bc14d4fe766d37a438bf52c33e89da768 Closes-Bug: #1613770 (cherry picked from commit 024bf10d8aec5e58111793a9652b16682eb61b7c) --- nova/exception.py | 5 +++++ nova/image/glance.py | 2 ++ nova/tests/unit/image/test_glance.py | 5 +++++ 3 files changed, 12 insertions(+) diff --git a/nova/exception.py b/nova/exception.py index 7a5c5239333..d1780349a54 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -620,6 +620,11 @@ class ImageBadRequest(Invalid): "%(response)s") +class ImageQuotaExceeded(NovaException): + msg_fmt = _("Quota exceeded or out of space for image %(image_id)s " + "in the image service.") + + class InstanceUnacceptable(Invalid): msg_fmt = _("Instance %(instance_id)s is unacceptable: %(reason)s") diff --git a/nova/image/glance.py b/nova/image/glance.py index b2c5f9ae059..5c25bd4e41d 100644 --- a/nova/image/glance.py +++ b/nova/image/glance.py @@ -938,6 +938,8 @@ def _translate_image_exception(image_id, exc_value): if isinstance(exc_value, glanceclient.exc.BadRequest): return exception.ImageBadRequest(image_id=image_id, response=six.text_type(exc_value)) + if isinstance(exc_value, glanceclient.exc.HTTPOverLimit): + return exception.ImageQuotaExceeded(image_id=image_id) return exc_value diff --git a/nova/tests/unit/image/test_glance.py b/nova/tests/unit/image/test_glance.py index c5e714c1b0a..59f5710d492 100644 --- a/nova/tests/unit/image/test_glance.py +++ b/nova/tests/unit/image/test_glance.py @@ -305,6 +305,11 @@ def test_client_httpnotfound_converts_to_imagenotfound(self): out_exc = glance._translate_image_exception('123', in_exc) self.assertIsInstance(out_exc, exception.ImageNotFound) + def test_client_httpoverlimit_converts_to_imagequotaexceeded(self): + in_exc = glanceclient.exc.HTTPOverLimit('123') + out_exc = glance._translate_image_exception('123', in_exc) + self.assertIsInstance(out_exc, exception.ImageQuotaExceeded) + class TestGlanceSerializer(test.NoDBTestCase): def test_serialize(self): From a4e2a6a41239a682c0da553ec5938737d0b85b52 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Fri, 21 Aug 2020 17:17:50 +0000 Subject: [PATCH 40/54] add functional regression test for bug #1888395 This change adds a funcitonal regression test that assert the broken behavior when trying to live migrate with a neutron backend that does not support multiple port bindings. Conflicts/Changes: nova/tests/functional/regressions/test_bug_1888395.py: - specify api major version to allow block_migration 'auto' - use TempDir fixture for instances path - worked around lack of create_server and start_computes in integrated helpers in train by inlining the behavior in setUp and test_live_migrate - reverted to python2 compatiable super() syntax nova/tests/unit/virt/libvirt/fake_imagebackend.py: - include portion of change Ia3d7351c1805d98bcb799ab0375673c7f1cb8848 which stubs out the is_file_in_instance_path method. That was included in a feature patch set so just pulling the necessary bit. Change-Id: I470a016d35afe69809321bd67359f466c3feb90a Partial-Bug: #1888395 (cherry picked from commit 71bc6fc9b89535679252ffe5a737eddad60e4102) (cherry picked from commit bea55a7d45bdc97679cf08c9faec789cfc90de27) --- nova/network/neutronv2/constants.py | 1 + .../regressions/test_bug_1888395.py | 122 ++++++++++++++++++ .../unit/virt/libvirt/fake_imagebackend.py | 8 +- 3 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 nova/tests/functional/regressions/test_bug_1888395.py diff --git a/nova/network/neutronv2/constants.py b/nova/network/neutronv2/constants.py index 1bb4605c9f9..6a5e6b8e0ea 100644 --- a/nova/network/neutronv2/constants.py +++ b/nova/network/neutronv2/constants.py @@ -19,6 +19,7 @@ DNS_INTEGRATION = 'DNS Integration' MULTI_NET_EXT = 'Multi Provider Network' SUBSTR_PORT_FILTERING = 'IP address substring filtering' +PORT_BINDING = 'Port Binding' PORT_BINDING_EXTENDED = 'Port Bindings Extended' LIVE_MIGRATION = 'live-migration' DEFAULT_SECGROUP = 'default' diff --git a/nova/tests/functional/regressions/test_bug_1888395.py b/nova/tests/functional/regressions/test_bug_1888395.py new file mode 100644 index 00000000000..8376814a3e6 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1888395.py @@ -0,0 +1,122 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import fixtures +import mock + +from nova import context +from nova.network.neutronv2 import api as neutron +from nova.network.neutronv2 import constants as neutron_constants +from nova.tests.functional import integrated_helpers +from nova.tests.functional.libvirt import base as libvirt_base +from nova.tests.unit.virt.libvirt import fake_os_brick_connector +from nova.tests.unit.virt.libvirt import fakelibvirt + + +class TestLiveMigrationWithoutMultiplePortBindings( + integrated_helpers.InstanceHelperMixin, + libvirt_base.ServersTestBase): + """Regression test for bug 1888395. + + This regression test asserts that Live migration works when + neutron does not support the binding-extended api extension + and the legacy single port binding workflow is used. + """ + + ADMIN_API = True + api_major_version = 'v2.1' + microversion = 'latest' + + def list_extensions(self, *args, **kwargs): + return { + 'extensions': [ + { + # Copied from neutron-lib portbindings.py + "updated": "2014-02-03T10:00:00-00:00", + "name": neutron_constants.PORT_BINDING, + "links": [], + "alias": "binding", + "description": "Expose port bindings of a virtual port to " + "external application" + } + ] + } + + def setUp(self): + self.flags(instances_path=self.useFixture(fixtures.TempDir()).path) + super(TestLiveMigrationWithoutMultiplePortBindings, self).setUp() + self.neutron.list_extensions = self.list_extensions + self.neutron_api = neutron.API() + # TODO(sean-k-mooney): remove after + # I275509eb0e0eb9eaf26fe607b7d9a67e1edc71f8 + # has merged. + self.useFixture(fixtures.MonkeyPatch( + 'nova.virt.libvirt.driver.connector', + fake_os_brick_connector)) + + self.computes = {} + for host in ['start_host', 'end_host']: + host_info = fakelibvirt.HostInfo( + cpu_nodes=1, cpu_sockets=1, cpu_cores=4, cpu_threads=2, + kB_mem=10740000) + fake_connection = self._get_connection( + host_info=host_info, hostname=host) + + # This is fun. Firstly we need to do a global'ish mock so we can + # actually start the service. + with mock.patch('nova.virt.libvirt.host.Host.get_connection', + return_value=fake_connection): + compute = self.start_service('compute', host=host) + + # Once that's done, we need to do some tweaks to each individual + # compute "service" to make sure they return unique objects + compute.driver._host.get_connection = lambda: fake_connection + self.computes[host] = compute + + self.ctxt = context.get_admin_context() + + def test_live_migrate(self): + flavors = self.api.get_flavors() + flavor = flavors[0] + server_req = self._build_minimal_create_server_request( + self.api, 'some-server', flavor_id=flavor['id'], + image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', + networks=[{'port': self.neutron.port_1['id']}]) + server_req['availability_zone'] = 'nova:%s' % "start_host" + created_server = self.api.post_server({'server': server_req}) + server = self._wait_for_state_change( + self.api, created_server, 'ACTIVE') + self.assertFalse( + self.neutron_api.supports_port_binding_extension(self.ctxt)) + # TODO(sean-k-mooney): extend _live_migrate to support passing a host + self.api.post_server_action( + server['id'], + { + 'os-migrateLive': { + 'host': 'end_host', + 'block_migration': 'auto' + } + } + ) + + # FIXME(sean-k-mooney): this should succeed but because of bug #188395 + # it will fail. + # self._wait_for_server_parameter( + # server, {'OS-EXT-SRV-ATTR:host': 'end_host', 'status': 'ACTIVE'}) + # because of the bug the migration will fail in pre_live_migrate so + # the vm should still be active on the start_host + self._wait_for_server_parameter( + self.api, server, + {'OS-EXT-SRV-ATTR:host': 'start_host', 'status': 'ACTIVE'}) + + msg = "NotImplementedError: Cannot load 'vif_type' in the base class" + self.assertIn(msg, self.stdlog.logger.output) diff --git a/nova/tests/unit/virt/libvirt/fake_imagebackend.py b/nova/tests/unit/virt/libvirt/fake_imagebackend.py index d73a396ab5e..4a940fa45e0 100644 --- a/nova/tests/unit/virt/libvirt/fake_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/fake_imagebackend.py @@ -184,11 +184,17 @@ def image_init(instance=None, disk_name=None, path=None): # class. image_init.SUPPORTS_CLONE = False - # Ditto for the 'is_shared_block_storage' function + # Ditto for the 'is_shared_block_storage' function and + # 'is_file_in_instance_path' def is_shared_block_storage(): return False + def is_file_in_instance_path(): + return False + setattr(image_init, 'is_shared_block_storage', is_shared_block_storage) + setattr(image_init, 'is_file_in_instance_path', + is_file_in_instance_path) return image_init From 5a6fd88f7aaa18b9cbd7ab594b4e1dac0b7d22ca Mon Sep 17 00:00:00 2001 From: root Date: Sat, 18 Jul 2020 00:32:54 -0400 Subject: [PATCH 41/54] Set migrate_data.vifs only when using multiple port bindings In the rocky cycle nova was enhanced to support the multiple port binding live migration workflow when neutron supports the binding-extended API extension. When the migration_data object was extended to support multiple port bindings, populating the vifs field was used as a sentinel to indicate that the new workflow should be used. In the train release I734cc01dce13f9e75a16639faf890ddb1661b7eb (SR-IOV Live migration indirect port support) broke the semantics of the migrate_data object by unconditionally populating the vifs field This change restores the rocky semantics, which are depended on by several parts of the code base, by only conditionally populating vifs if neutron supports multiple port bindings. Changes to patch: - unit/virt/libvirt/fakelibvirt.py: Include partial pick from change Ia3d7351c1805d98bcb799ab0375673c7f1cb8848 to add the jobStats, complete_job and fail_job to fakelibvirt. The full change was not cherry-picked as it was part of the numa aware live migration feature in Victoria. - renamed import of nova.network.neutron to nova.network.neutronv2.api - mocked nova.virt.libvirt.guest.Guest.get_job_info to return fakelibvirt.VIR_DOMAIN_JOB_COMPLETED - replaced from urllib import parse as urlparse with import six.moves.urllib.parse as urlparse for py2.7 Conflicts: nova/tests/functional/regressions/test_bug_1888395.py nova/tests/unit/compute/test_compute.py nova/tests/unit/compute/test_compute_mgr.py nova/tests/unit/virt/test_virt_drivers.py Co-Authored-By: Sean Mooney Change-Id: Ia00277ac8a68a635db85f9e0ce2c6d8df396e0d8 Closes-Bug: #1888395 (cherry picked from commit b8f3be6b3c5af91d215b4a0cecb9be098e8d8799) (cherry picked from commit afa843c8a7e128489a8245ed7a1b391c022b3305) --- nova/compute/manager.py | 27 +++++---- .../regressions/test_bug_1888395.py | 59 ++++++++++++++++--- nova/tests/unit/compute/test_compute.py | 10 ++++ nova/tests/unit/compute/test_compute_mgr.py | 35 ++++++++++- nova/tests/unit/virt/libvirt/fakelibvirt.py | 13 +++- nova/tests/unit/virt/test_virt_drivers.py | 5 ++ ...ortbinding-semantics-48e9b1fa969cc5e9.yaml | 14 +++++ 7 files changed, 140 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/restore-rocky-portbinding-semantics-48e9b1fa969cc5e9.yaml diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 6e5d72f27a3..e5f6ace00f3 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -6832,15 +6832,18 @@ def check_can_live_migrate_destination(self, ctxt, instance, LOG.info('Destination was ready for NUMA live migration, ' 'but source is either too old, or is set to an ' 'older upgrade level.', instance=instance) - # Create migrate_data vifs - migrate_data.vifs = \ - migrate_data_obj.VIFMigrateData.create_skeleton_migrate_vifs( - instance.get_network_info()) - # Claim PCI devices for VIFs on destination (if needed) - port_id_to_pci = self._claim_pci_for_instance_vifs(ctxt, instance) - # Update migrate VIFs with the newly claimed PCI devices - self._update_migrate_vifs_profile_with_pci(migrate_data.vifs, - port_id_to_pci) + if self.network_api.supports_port_binding_extension(ctxt): + # Create migrate_data vifs + migrate_data.vifs = \ + migrate_data_obj.\ + VIFMigrateData.create_skeleton_migrate_vifs( + instance.get_network_info()) + # Claim PCI devices for VIFs on destination (if needed) + port_id_to_pci = self._claim_pci_for_instance_vifs( + ctxt, instance) + # Update migrate VIFs with the newly claimed PCI devices + self._update_migrate_vifs_profile_with_pci( + migrate_data.vifs, port_id_to_pci) finally: self.driver.cleanup_live_migration_destination_check(ctxt, dest_check_data) @@ -7008,8 +7011,12 @@ def pre_live_migration(self, context, instance, block_migration, disk, # determine if it should wait for a 'network-vif-plugged' event # from neutron before starting the actual guest transfer in the # hypervisor + using_multiple_port_bindings = ( + 'vifs' in migrate_data and migrate_data.vifs) migrate_data.wait_for_vif_plugged = ( - CONF.compute.live_migration_wait_for_vif_plug) + CONF.compute.live_migration_wait_for_vif_plug and + using_multiple_port_bindings + ) # NOTE(tr3buchet): setup networks on destination host self.network_api.setup_networks_on_host(context, instance, diff --git a/nova/tests/functional/regressions/test_bug_1888395.py b/nova/tests/functional/regressions/test_bug_1888395.py index 8376814a3e6..69576fd094a 100644 --- a/nova/tests/functional/regressions/test_bug_1888395.py +++ b/nova/tests/functional/regressions/test_bug_1888395.py @@ -13,6 +13,9 @@ import fixtures import mock +from lxml import etree +import six.moves.urllib.parse as urlparse + from nova import context from nova.network.neutronv2 import api as neutron from nova.network.neutronv2 import constants as neutron_constants @@ -20,6 +23,7 @@ from nova.tests.functional.libvirt import base as libvirt_base from nova.tests.unit.virt.libvirt import fake_os_brick_connector from nova.tests.unit.virt.libvirt import fakelibvirt +from nova.virt.libvirt import guest as libvirt_guest class TestLiveMigrationWithoutMultiplePortBindings( @@ -83,8 +87,51 @@ def setUp(self): self.computes[host] = compute self.ctxt = context.get_admin_context() + # TODO(sean-k-mooney): remove this when it is part of ServersTestBase + self.useFixture(fixtures.MonkeyPatch( + 'nova.tests.unit.virt.libvirt.fakelibvirt.Domain.migrateToURI3', + self._migrate_stub)) + self.useFixture(fixtures.MonkeyPatch( + 'nova.virt.libvirt.migration._update_serial_xml', + self._update_serial_xml_stub)) + + def _update_serial_xml_stub(self, xml_doc, migrate_data): + return xml_doc + + def _migrate_stub(self, domain, destination, params, flags): + """Stub out migrateToURI3.""" + + src_hostname = domain._connection.hostname + dst_hostname = urlparse.urlparse(destination).netloc + + # In a real live migration, libvirt and QEMU on the source and + # destination talk it out, resulting in the instance starting to exist + # on the destination. Fakelibvirt cannot do that, so we have to + # manually create the "incoming" instance on the destination + # fakelibvirt. + dst = self.computes[dst_hostname] + dst.driver._host.get_connection().createXML( + params['destination_xml'], + 'fake-createXML-doesnt-care-about-flags') + + src = self.computes[src_hostname] + conn = src.driver._host.get_connection() + + # because migrateToURI3 is spawned in a background thread, this method + # does not block the upper nova layers. Because we don't want nova to + # think the live migration has finished until this method is done, the + # last thing we do is make fakelibvirt's Domain.jobStats() return + # VIR_DOMAIN_JOB_COMPLETED. + server = etree.fromstring( + params['destination_xml'] + ).find('./uuid').text + dom = conn.lookupByUUIDString(server) + dom.complete_job() - def test_live_migrate(self): + @mock.patch('nova.virt.libvirt.guest.Guest.get_job_info') + def test_live_migrate(self, mock_get_job_info): + mock_get_job_info.return_value = libvirt_guest.JobInfo( + type=fakelibvirt.VIR_DOMAIN_JOB_COMPLETED) flavors = self.api.get_flavors() flavor = flavors[0] server_req = self._build_minimal_create_server_request( @@ -108,15 +155,9 @@ def test_live_migrate(self): } ) - # FIXME(sean-k-mooney): this should succeed but because of bug #188395 - # it will fail. - # self._wait_for_server_parameter( - # server, {'OS-EXT-SRV-ATTR:host': 'end_host', 'status': 'ACTIVE'}) - # because of the bug the migration will fail in pre_live_migrate so - # the vm should still be active on the start_host self._wait_for_server_parameter( self.api, server, - {'OS-EXT-SRV-ATTR:host': 'start_host', 'status': 'ACTIVE'}) + {'OS-EXT-SRV-ATTR:host': 'end_host', 'status': 'ACTIVE'}) msg = "NotImplementedError: Cannot load 'vif_type' in the base class" - self.assertIn(msg, self.stdlog.logger.output) + self.assertNotIn(msg, self.stdlog.logger.output) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 77f23c30fe4..687b7eaadda 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -18,6 +18,7 @@ """Tests for compute service.""" import datetime +import fixtures as std_fixtures from itertools import chain import operator import sys @@ -6142,9 +6143,15 @@ def test_pre_live_migration_works_correctly(self, mock_get_bdms, # Confirm setup_compute_volume is called when volume is mounted. def stupid(*args, **kwargs): return fake_network.fake_get_instance_nw_info(self) + if CONF.use_neutron: self.stub_out( 'nova.network.neutronv2.api.API.get_instance_nw_info', stupid) + self.useFixture( + std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.' + 'API.supports_port_binding_extension', + lambda *args: True)) else: self.stub_out('nova.network.api.API.get_instance_nw_info', stupid) @@ -6155,6 +6162,9 @@ def stupid(*args, **kwargs): fake_notifier.NOTIFICATIONS = [] migrate_data = objects.LibvirtLiveMigrateData( is_shared_instance_path=False) + vifs = migrate_data_obj.VIFMigrateData.create_skeleton_migrate_vifs( + stupid()) + migrate_data.vifs = vifs mock_pre.return_value = migrate_data with mock.patch.object(self.compute.network_api, diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 250d06dfc40..8981b6f7ba4 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -15,6 +15,7 @@ import contextlib import copy import datetime +import fixtures as std_fixtures import time from cinderclient import exceptions as cinder_exception @@ -3195,20 +3196,48 @@ def _test_check_can_live_migrate_destination(self, do_raise=False, mock_event.assert_called_once_with( self.context, 'compute_check_can_live_migrate_destination', CONF.host, instance.uuid) + return result def test_check_can_live_migrate_destination_success(self): + self.useFixture(std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.API.supports_port_binding_extension', + lambda *args: True)) self._test_check_can_live_migrate_destination() def test_check_can_live_migrate_destination_fail(self): + self.useFixture(std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.API.supports_port_binding_extension', + lambda *args: True)) self.assertRaises( - test.TestingException, - self._test_check_can_live_migrate_destination, - do_raise=True) + test.TestingException, + self._test_check_can_live_migrate_destination, + do_raise=True) + + def test_check_can_live_migrate_destination_contins_vifs(self): + self.useFixture(std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.API.supports_port_binding_extension', + lambda *args: True)) + migrate_data = self._test_check_can_live_migrate_destination() + self.assertIn('vifs', migrate_data) + self.assertIsNotNone(migrate_data.vifs) + + def test_check_can_live_migrate_destination_no_binding_extended(self): + self.useFixture(std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.API.supports_port_binding_extension', + lambda *args: False)) + migrate_data = self._test_check_can_live_migrate_destination() + self.assertNotIn('vifs', migrate_data) def test_check_can_live_migrate_destination_src_numa_lm_false(self): + self.useFixture(std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.API.supports_port_binding_extension', + lambda *args: True)) self._test_check_can_live_migrate_destination(src_numa_lm=False) def test_check_can_live_migrate_destination_src_numa_lm_true(self): + self.useFixture(std_fixtures.MonkeyPatch( + 'nova.network.neutronv2.api.API.supports_port_binding_extension', + lambda *args: True)) self._test_check_can_live_migrate_destination(src_numa_lm=True) def test_dest_can_numa_live_migrate(self): diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index 065d66ab4ea..676a3542b1f 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -770,6 +770,7 @@ def __init__(self, connection, xml, running=False, transient=False): self._has_saved_state = False self._snapshots = {} self._id = self._connection._id_counter + self._job_type = VIR_DOMAIN_JOB_UNBOUNDED def _parse_definition(self, xml): try: @@ -1237,7 +1238,17 @@ def jobInfo(self): return [0] * 12 def jobStats(self, flags=0): - return {} + # NOTE(artom) By returning VIR_DOMAIN_JOB_UNBOUNDED, we're pretending a + # job is constantly running. Tests are expected to call the + # complete_job or fail_job methods when they're ready for jobs (read: + # live migrations) to "complete". + return {'type': self._job_type} + + def complete_job(self): + self._job_type = VIR_DOMAIN_JOB_COMPLETED + + def fail_job(self): + self._job_type = VIR_DOMAIN_JOB_FAILED def injectNMI(self, flags=0): return 0 diff --git a/nova/tests/unit/virt/test_virt_drivers.py b/nova/tests/unit/virt/test_virt_drivers.py index c457b7b89bb..ff69b3f9c93 100644 --- a/nova/tests/unit/virt/test_virt_drivers.py +++ b/nova/tests/unit/virt/test_virt_drivers.py @@ -39,6 +39,7 @@ from nova.tests.unit import fake_block_device from nova.tests.unit.image import fake as fake_image from nova.tests.unit import utils as test_utils +from nova.tests.unit.virt.libvirt import fakelibvirt from nova.virt import block_device as driver_block_device from nova.virt import event as virtevent from nova.virt import fake @@ -617,6 +618,10 @@ def test_unfilter_instance(self): network_info = test_utils.get_test_network_info() self.connection.unfilter_instance(instance_ref, network_info) + @mock.patch( + 'nova.tests.unit.virt.libvirt.fakelibvirt.Domain.jobStats', + new=mock.Mock(return_value={ + 'type': fakelibvirt.VIR_DOMAIN_JOB_COMPLETED})) def test_live_migration(self): instance_ref, network_info = self._get_running_instance() fake_context = context.RequestContext('fake', 'fake') diff --git a/releasenotes/notes/restore-rocky-portbinding-semantics-48e9b1fa969cc5e9.yaml b/releasenotes/notes/restore-rocky-portbinding-semantics-48e9b1fa969cc5e9.yaml new file mode 100644 index 00000000000..dc33e3c61db --- /dev/null +++ b/releasenotes/notes/restore-rocky-portbinding-semantics-48e9b1fa969cc5e9.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + In the Rocky (18.0.0) release support was added to nova to use neutron's + multiple port binding feature when the binding-extended API extension + is available. In the Train (20.0.0) release the SR-IOV live migration + feature broke the semantics of the vifs field in the ``migration_data`` + object that signals if the new multiple port binding workflow should + be used by always populating it even when the ``binding-extended`` API + extension is not present. This broke live migration for any deployment + that did not support the optional ``binding-extended`` API extension. + The Rocky behavior has now been restored enabling live migration + using the single port binding workflow when multiple port bindings + are not available. From 618103db9bff9341a90342b372d8a77419e085f4 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Fri, 2 Oct 2020 15:11:25 +0100 Subject: [PATCH 42/54] libvirt: Increase incremental and max sleep time during device detach Bug #1894804 outlines how DEVICE_DELETED events were often missing from QEMU on Focal based OpenStack CI hosts as originally seen in bug #1882521. This has eventually been tracked down to some undefined QEMU behaviour when a new device_del QMP command is received while another is still being processed, causing the original attempt to be aborted. We hit this race in slower OpenStack CI envs as n-cpu rather crudely retries attempts to detach devices using the RetryDecorator from oslo.service. The default incremental sleep time currently being tight enough to ensure QEMU is still processing the first device_del request on these slower CI hosts when n-cpu asks libvirt to retry the detach, sending another device_del to QEMU hitting the above behaviour. Additionally we have also seen the following check being hit when testing with QEMU >= v5.0.0. This check now rejects overlapping device_del requests in QEMU rather than aborting the original: https://github.com/qemu/qemu/commit/cce8944cc9efab47d4bf29cfffb3470371c3541b This change aims to avoid this situation entirely by raising the default incremental sleep time between detach requests from 2 seconds to 10, leaving enough time for the first attempt to complete. The overall maximum sleep time is also increased from 30 to 60 seconds. Future work will aim to entirely remove this retry logic with a libvirt event driven approach, polling for the the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED and VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED events before retrying. Finally, the cleanup of unused arguments in detach_device_with_retry is left for a follow up change in order to keep this initial change small enough to quickly backport. Closes-Bug: #1882521 Related-Bug: #1894804 Change-Id: Ib9ed7069cef5b73033351f7a78a3fb566753970d (cherry picked from commit dd1e6d4b0cee465fd89744e306fcd25228b3f7cc) (cherry picked from commit 4819f694b2e2d5688fdac7e850f1a6c592253d6b) (cherry picked from commit f32286c62e11d43f6b01bc82caacf588447862e5) --- nova/virt/libvirt/guest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index 318294c659a..a7fbc50ef2c 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -367,8 +367,8 @@ def get_all_devices(self, devtype=None): return devs def detach_device_with_retry(self, get_device_conf_func, device, live, - max_retry_count=7, inc_sleep_time=2, - max_sleep_time=30, + max_retry_count=7, inc_sleep_time=10, + max_sleep_time=60, alternative_device_name=None, supports_device_missing_error_code=False): """Detaches a device from the guest. After the initial detach request, From c61ceac5d0d0844db85791da167cf224ce360a5f Mon Sep 17 00:00:00 2001 From: Alexandre Arents Date: Tue, 26 Nov 2019 10:26:32 +0000 Subject: [PATCH 43/54] Make _rebase_with_qemu_img() generic Move volume_delete related logic away from this method, in order to make it generic and usable elsewhere. NOTE(lyarwood): Conflict caused by I52fbbcac9dc386f24ee81b3321dd0d8355e01976 landing in stbale/ussuri. Conflicts: nova/tests/unit/virt/libvirt/test_driver.py Change-Id: I17357d85f845d4160cb7c7784772530a1e92af76 Related-Bug: #1732428 (cherry picked from commit ce2203456660083119cdbb7e73c1ad15e6e0a074) (cherry picked from commit 2e89699c3301bf801784c637b6919752fcd3503f) --- nova/tests/unit/virt/libvirt/test_driver.py | 19 ++++++++++ nova/virt/libvirt/driver.py | 42 +++++++++------------ 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 4fec9db3862..540baf8ef58 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -23854,6 +23854,25 @@ def test_cpu_traits_with_mode_none_on_power(self, mock_baseline, mock_cap): '''], 1) + @mock.patch('nova.virt.images.qemu_img_info', + return_value=mock.Mock(file_format="fake_fmt")) + @mock.patch('oslo_concurrency.processutils.execute') + def test_rebase_with_qemu_img(self, mock_execute, mock_qemu_img_info): + """rebasing disk image to another backing file""" + self.drvr._rebase_with_qemu_img("disk", "backing_file") + mock_qemu_img_info.assert_called_once_with("backing_file") + mock_execute.assert_called_once_with('qemu-img', 'rebase', + '-b', 'backing_file', '-F', + 'fake_fmt', 'disk') + + # Flatten disk image when no backing file is given. + mock_qemu_img_info.reset_mock() + mock_execute.reset_mock() + self.drvr._rebase_with_qemu_img("disk", None) + self.assertEqual(0, mock_qemu_img_info.call_count) + mock_execute.assert_called_once_with('qemu-img', 'rebase', + '-b', '', 'disk') + class LibvirtVolumeUsageTestCase(test.NoDBTestCase): """Test for LibvirtDriver.get_all_volume_usage.""" diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index aead8470b29..786b494140c 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -2861,32 +2861,15 @@ def _wait_for_snapshot(): timer.start(interval=0.5).wait() @staticmethod - def _rebase_with_qemu_img(guest, device, active_disk_object, - rebase_base): - """Rebase a device tied to a guest using qemu-img. - - :param guest:the Guest which owns the device being rebased - :type guest: nova.virt.libvirt.guest.Guest - :param device: the guest block device to rebase - :type device: nova.virt.libvirt.guest.BlockDevice - :param active_disk_object: the guest block device to rebase - :type active_disk_object: nova.virt.libvirt.config.\ - LibvirtConfigGuestDisk + def _rebase_with_qemu_img(source_path, rebase_base): + """Rebase a disk using qemu-img. + + :param source_path: the disk source path to rebase + :type source_path: string :param rebase_base: the new parent in the backing chain :type rebase_base: None or string """ - # It's unsure how well qemu-img handles network disks for - # every protocol. So let's be safe. - active_protocol = active_disk_object.source_protocol - if active_protocol is not None: - msg = _("Something went wrong when deleting a volume snapshot: " - "rebasing a %(protocol)s network disk using qemu-img " - "has not been fully tested") % {'protocol': - active_protocol} - LOG.error(msg) - raise exception.InternalError(msg) - if rebase_base is None: # If backing_file is specified as "" (the empty string), then # the image is rebased onto no backing file (i.e. it will exist @@ -2901,7 +2884,7 @@ def _rebase_with_qemu_img(guest, device, active_disk_object, b_file_fmt = images.qemu_img_info(backing_file).file_format qemu_img_extra_arg = ['-F', b_file_fmt] - qemu_img_extra_arg.append(active_disk_object.source_path) + qemu_img_extra_arg.append(source_path) # execute operation with disk concurrency semaphore with compute_utils.disk_ops_semaphore: processutils.execute("qemu-img", "rebase", "-b", backing_file, @@ -3064,7 +3047,18 @@ def _get_snap_dev(filename, backing_store): else: LOG.debug('Guest is not running so doing a block rebase ' 'using "qemu-img rebase"', instance=instance) - self._rebase_with_qemu_img(guest, dev, active_disk_object, + + # It's unsure how well qemu-img handles network disks for + # every protocol. So let's be safe. + active_protocol = active_disk_object.source_protocol + if active_protocol is not None: + msg = _("Something went wrong when deleting a volume " + "snapshot: rebasing a %(protocol)s network disk " + "using qemu-img has not been fully tested" + ) % {'protocol': active_protocol} + LOG.error(msg) + raise exception.InternalError(msg) + self._rebase_with_qemu_img(active_disk_object.source_path, rebase_base) else: From 3bc1502f009a8d188ddee9c46888c3bac88180fe Mon Sep 17 00:00:00 2001 From: Alexandre Arents Date: Tue, 26 Nov 2019 10:26:32 +0000 Subject: [PATCH 44/54] Rebase qcow2 images when unshelving an instance During unshelve, instance is spawn with image created by shelve and is deleted just after, instance.image_ref still point to the original instance build image. In qcow2 environment, this is an issue because instance backing file don't match anymore instance.image_ref and during live-migration/resize, target host will fetch image corresponding to instance.image_ref involving instance corruption. This change fetches original image and rebase instance disk on it. This avoid image_ref mismatch and bring back storage benefit to keep common image in cache. If original image is no more available in glance, backing file is merged into disk(flatten), ensuring instance integrity during next live-migration/resize operation. NOTE(lyarwood): Test conflicts caused by If56842da51688 not being present in stable/train. Conflicts: nova/tests/unit/virt/libvirt/test_driver.py Change-Id: I1a33fadf0b7439cf06c06cba2bc06df6cef0945b Closes-Bug: #1732428 (cherry picked from commit 8953a689467f8c3e996086392251de67953a45ba) (cherry picked from commit 7003618884b5e033f822685541da6bc7955fe55a) --- nova/tests/unit/virt/libvirt/test_driver.py | 68 +++++++++++++++++++++ nova/virt/libvirt/driver.py | 38 ++++++++++++ 2 files changed, 106 insertions(+) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 540baf8ef58..cf91fa66940 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -21794,6 +21794,74 @@ def test_unshelve_rbd_image_flatten_during_fetch_image_cache(self): mock_rbd_driver.flatten.assert_called_once_with( mock.sentinel.rbd_name, pool=mock.sentinel.rbd_pool) + @mock.patch.object(libvirt_driver.LibvirtDriver, '_try_fetch_image_cache') + @mock.patch.object(libvirt_driver.LibvirtDriver, '_rebase_with_qemu_img') + def _test_unshelve_qcow2_rebase_image_during_create(self, + mock_rebase, mock_fetch, original_image_in_glance=True): + self.flags(images_type='qcow2', group='libvirt') + + # Original image ref from where instance was created, before SHELVE + # occurs, base_root_fname is related backing file name. + base_image_ref = 'base_image_ref' + base_root_fname = imagecache.get_cache_fname(base_image_ref) + # Snapshot image ref created during SHELVE. + shelved_image_ref = 'shelved_image_ref' + shelved_root_fname = imagecache.get_cache_fname(shelved_image_ref) + + # Instance state during unshelve spawn(). + inst_params = { + 'image_ref': shelved_image_ref, + 'vm_state': vm_states.SHELVED_OFFLOADED, + 'system_metadata': {'image_base_image_ref': base_image_ref} + } + + instance = self._create_instance(params=inst_params) + disk_images = {'image_id': instance.image_ref} + instance_dir = libvirt_utils.get_instance_path(instance) + disk_path = os.path.join(instance_dir, 'disk') + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + + if original_image_in_glance: + # We expect final backing file is original image, not shelved one. + expected_backing_file = os.path.join( + imagecache.ImageCacheManager().cache_dir, + base_root_fname) + else: + # None means rebase will merge backing file into disk(flatten). + expected_backing_file = None + mock_fetch.side_effect = [ + None, + exception.ImageNotFound(image_id=base_image_ref) + ] + + drvr._create_and_inject_local_root( + self.context, instance, False, '', disk_images, None, None) + + mock_fetch.assert_has_calls([ + mock.call(test.MatchType(nova.virt.libvirt.imagebackend.Qcow2), + libvirt_utils.fetch_image, + self.context, shelved_root_fname, shelved_image_ref, + instance, instance.root_gb * units.Gi, None), + mock.call(test.MatchType(nova.virt.libvirt.imagebackend.Qcow2), + libvirt_utils.fetch_image, + self.context, base_root_fname, base_image_ref, + instance, None)]) + mock_rebase.assert_called_once_with(disk_path, expected_backing_file) + + def test_unshelve_qcow2_rebase_image_during_create(self): + # Original image is present in Glance. In that case the 2nd + # fetch succeeds and we rebase instance disk to original image backing + # file, instance is back to nominal state: after unshelve, + # instance.image_ref will match current backing file. + self._test_unshelve_qcow2_rebase_image_during_create() + + def test_unshelve_qcow2_rebase_image_during_create_notfound(self): + # Original image is no longer available in Glance, so 2nd fetch + # will failed (HTTP 404). In that case qemu-img rebase will merge + # backing file into disk, removing backing file dependency. + self._test_unshelve_qcow2_rebase_image_during_create( + original_image_in_glance=False) + @mock.patch('nova.virt.libvirt.driver.imagebackend') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._inject_data') @mock.patch('nova.virt.libvirt.driver.imagecache') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 786b494140c..78fd895549c 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -4026,6 +4026,14 @@ def clone_fallback_to_fetch(*args, **kwargs): root_fname, disk_images['image_id'], instance, size, fallback_from_host) + # During unshelve on Qcow2 backend, we spawn() using snapshot image + # created during shelve. Extra work is needed in order to rebase + # disk image to its original image_ref. Disk backing file will + # then represent back image_ref instead of shelved image. + if (instance.vm_state == vm_states.SHELVED_OFFLOADED and + isinstance(backend, imagebackend.Qcow2)): + self._finalize_unshelve_qcow2_image(context, instance, backend) + if need_inject: self._inject_data(backend, instance, injection_info) @@ -4035,6 +4043,36 @@ def clone_fallback_to_fetch(*args, **kwargs): return created_disks + def _finalize_unshelve_qcow2_image(self, context, instance, backend): + # NOTE(aarents): During qcow2 instance unshelve, backing file + # represents shelved image, not original instance.image_ref. + # We rebase here instance disk to original image. + # This second fetch call does nothing except downloading original + # backing file if missing, as image disk have already been + # created/resized by first fetch call. + base_dir = self.image_cache_manager.cache_dir + base_image_ref = instance.system_metadata.get('image_base_image_ref') + root_fname = imagecache.get_cache_fname(base_image_ref) + base_backing_fname = os.path.join(base_dir, root_fname) + + try: + self._try_fetch_image_cache(backend, libvirt_utils.fetch_image, + context, root_fname, base_image_ref, + instance, None) + except exception.ImageNotFound: + # We must flatten here in order to remove dependency with an orphan + # backing file (as shelved image will be dropped once unshelve + # is successfull). + LOG.warning('Current disk image is created on top of shelved ' + 'image and cannot be rebased to original image ' + 'because it is no longer available in the image ' + 'service, disk will be consequently flattened.', + instance=instance) + base_backing_fname = None + + LOG.info('Rebasing disk image.', instance=instance) + self._rebase_with_qemu_img(backend.path, base_backing_fname) + def _create_configdrive(self, context, instance, injection_info, rescue=False): # As this method being called right after the definition of a From 8a01a58a60e7f4f2c189f8ca065d8c21ccbde04f Mon Sep 17 00:00:00 2001 From: Alexandre Arents Date: Tue, 1 Sep 2020 08:26:25 +0000 Subject: [PATCH 45/54] Update image_base_image_ref during rebuild. In different location we assume system_metadata.image_base_image_ref exists, because it is set during instance creation in method _populate_instance_for_create But once instance is rebuild, all system_metadata image property a dropped and replace by new image property and without setting back image_base_image_ref. This change propose to set image_base_image_ref during rebuild. In specific case of shelve/unshelve in Qcow2 backend, image_base_image_ref is used to rebase disk image, so we ensure this property is set as instance may have been rebuild before the fix was apply. Related-Bug: #1732428 Closes-Bug: #1893618 Change-Id: Ia3031ea1f7db8b398f02d2080ca603ded8970200 (cherry picked from commit fe52b6c25bebdd1b459c7a59fbb8d9f6de200c9d) (cherry picked from commit 560414036d9f2ebcfeb8626029d0bd849d6cad44) --- nova/compute/api.py | 10 ++++++++++ nova/tests/unit/api/openstack/compute/test_shelve.py | 1 + nova/tests/unit/compute/test_compute.py | 1 + 3 files changed, 12 insertions(+) diff --git a/nova/compute/api.py b/nova/compute/api.py index 4fa0cc2a5ae..bc23cbeaa70 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3392,6 +3392,8 @@ def _reset_image_metadata(): new_sys_metadata = utils.get_system_metadata_from_image( image, flavor) + new_sys_metadata.update({'image_base_image_ref': image_id}) + instance.system_metadata.update(new_sys_metadata) instance.save() return orig_sys_metadata @@ -3805,6 +3807,14 @@ def shelve(self, context, instance, clean_shutdown=True): hypervisor. """ instance.task_state = task_states.SHELVING + + # NOTE(aarents): Ensure image_base_image_ref is present as it will be + # needed during unshelve and instance rebuild done before Bug/1893618 + # Fix dropped it. + instance.system_metadata.update( + {'image_base_image_ref': instance.image_ref} + ) + instance.save(expected_task_state=[None]) self._record_action_start(context, instance, instance_actions.SHELVE) diff --git a/nova/tests/unit/api/openstack/compute/test_shelve.py b/nova/tests/unit/api/openstack/compute/test_shelve.py index 8d41490d862..e51729935bf 100644 --- a/nova/tests/unit/api/openstack/compute/test_shelve.py +++ b/nova/tests/unit/api/openstack/compute/test_shelve.py @@ -54,6 +54,7 @@ def test_shelve_task_state_race(self, mock_save, get_instance_mock): self.req.environ['nova.context'], vm_state=vm_states.ACTIVE, task_state=None) instance.launched_at = instance.created_at + instance.system_metadata = {} get_instance_mock.return_value = instance mock_save.side_effect = exception.UnexpectedTaskStateError( instance_uuid=instance.uuid, expected=None, diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 77f23c30fe4..cfdf404c29e 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -9133,6 +9133,7 @@ def fake_rpc_rebuild(context, **kwargs): 'image_ramdisk_id': uuids.ramdisk_id, 'image_something_else': 'meow', 'preserved': 'preserve this!', + 'image_base_image_ref': image_ref, 'boot_roles': ''}, sys_meta) From de94f429474713a68d5efc53677bc468f02dc112 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 16 Jun 2021 12:00:19 +0100 Subject: [PATCH 46/54] Move 'check-cherry-picks' test to gate, n-v check This currently runs in the 'check' pipeline, as part of the pep8 job, which causes otherwise perfectly valid backports to report as failing CI. There's no reason a stable core shouldn't be encouraged to review these patches: we simply want to prevent them *merging* before their parent(s). Resolve this conflict by moving the check to separate voting job in the 'gate' pipeline as well as a non-voting job in the 'check' pipeline to catch more obvious issues. Change-Id: Id3e4452883f6a3cf44ff58b39ded82e882e28c23 Signed-off-by: Stephen Finucane (cherry picked from commit 98b01c9a59df4912f5a162c2c52d1f00c84d24c2) (cherry picked from commit fef0305abefbf165fecb883f03bce97f525a790a) (cherry picked from commit b7677ae08ae151858ecb0e67039e54bb3df89700) (cherry picked from commit 91314f7fbba312d4438fa446804f692d316512a8) --- .zuul.yaml | 14 ++++++++++++++ tools/check-cherry-picks.sh | 5 ----- tox.ini | 12 +++++++++--- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/.zuul.yaml b/.zuul.yaml index f80a9eba226..70ecef6e906 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -118,6 +118,17 @@ python_version: 3.7 timeout: 3600 +- job: + name: nova-tox-validate-backport + parent: openstack-tox + description: | + Determine whether a backport is ready to be merged by checking whether it + has already been merged to master or more recent stable branches. + + Uses tox with the ``validate-backport`` environment. + vars: + tox_envlist: validate-backport + - job: name: nova-live-migration parent: nova-dsvm-multinode-base @@ -393,6 +404,8 @@ - nova-next - nova-tox-functional - nova-tox-functional-py36 + - nova-tox-validate-backport: + voting: false - tempest-integrated-compute: irrelevant-files: *dsvm-irrelevant-files - tempest-slow-py3: @@ -409,6 +422,7 @@ - nova-tox-functional-py36 - nova-multi-cell - nova-next + - nova-tox-validate-backport - tempest-integrated-compute: irrelevant-files: *dsvm-irrelevant-files - tempest-slow-py3: diff --git a/tools/check-cherry-picks.sh b/tools/check-cherry-picks.sh index 5ca6ded2033..5a449c520b7 100755 --- a/tools/check-cherry-picks.sh +++ b/tools/check-cherry-picks.sh @@ -4,11 +4,6 @@ # to verify that they're all on either master or stable/ branches # -# Allow this script to be disabled by a simple env var -if [ ${DISABLE_CHERRY_PICK_CHECK:-0} -eq 1 ]; then - exit 0 -fi - commit_hash="" # Check if the patch is a merge patch by counting the number of parents. diff --git a/tox.ini b/tox.ini index f0d1971a3ea..76992f718c5 100644 --- a/tox.ini +++ b/tox.ini @@ -51,15 +51,12 @@ commands = description = Run style checks. envdir = {toxworkdir}/shared -passenv = - DISABLE_CHERRY_PICK_CHECK commands = bash tools/flake8wrap.sh {posargs} # Check that all JSON files don't have \r\n in line. bash -c "! find doc/ -type f -name *.json | xargs grep -U -n $'\r'" # Check that all included JSON files are valid JSON bash -c '! find doc/ -type f -name *.json | xargs -t -n1 python -m json.tool 2>&1 > /dev/null | grep -B1 -v ^python' - bash tools/check-cherry-picks.sh [testenv:fast8] description = @@ -68,6 +65,15 @@ envdir = {toxworkdir}/shared commands = bash tools/flake8wrap.sh -HEAD +[testenv:validate-backport] +description = + Determine whether a backport is ready to be merged by checking whether it has + already been merged to master or more recent stable branches. +deps = +skipsdist = true +commands = + bash tools/check-cherry-picks.sh + [testenv:functional] # TODO(melwitt): This can be removed when functional tests are gating with # python 3.x From e926ec75e29dcdf3b671811533587bba246a8c45 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 2 Jul 2020 12:13:29 +0200 Subject: [PATCH 47/54] Use absolute path during qemu img rebase During an assisted volume snapshot delete request from Cinder nova removes the snapshot from the backing file chain. During that nova checks the existence of such file. However in some cases (see the bug report) the path is relative and therefore os.path.exists fails. This patch makes sure that nova uses the volume absolute path to make the backing file path absolute as well. Closes-Bug #1885528 Change-Id: I58dca95251b607eaff602783fee2fc38e2421944 (cherry picked from commit b9333125790682f9d60bc74fdbb12a098565e7c2) (cherry picked from commit 831abc9f83a2d3f517030f881e7da724417fea93) (cherry picked from commit c2044d4bd0919860aa2d49687ba9c6ef6f7d37e8) --- nova/tests/unit/virt/libvirt/test_driver.py | 28 +++++++++++++++++---- nova/virt/libvirt/driver.py | 11 +++++++- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index d6e241c8e6b..38a24c1ee70 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -24564,8 +24564,23 @@ def test_volume_snapshot_delete_when_dom_not_running(self, mock_execute, not running should trigger a blockRebase using qemu-img not libvirt. In this test, we rebase the image with another image as backing file. """ + dom_xml = """ + + + + + + 0e38683e-f0af-418f-a3f1-6b67ea0f919d + + + + + + + """ % self.inst['uuid'] + mock_domain, guest = self._setup_block_rebase_domain_and_guest_mocks( - self.dom_xml) + dom_xml) instance = objects.Instance(**self.inst) snapshot_id = 'snapshot-1234' @@ -24576,10 +24591,13 @@ def test_volume_snapshot_delete_when_dom_not_running(self, mock_execute, self.delete_info_1) mock_disk_op_sema.__enter__.assert_called_once() - mock_qemu_img_info.assert_called_once_with("snap.img") - mock_execute.assert_called_once_with('qemu-img', 'rebase', - '-b', 'snap.img', '-F', - 'fake_fmt', 'disk1_file') + mock_qemu_img_info.assert_called_once_with( + "/var/lib/nova/instances/%s/snap.img" % instance.uuid) + mock_execute.assert_called_once_with( + 'qemu-img', 'rebase', + '-b', '/var/lib/nova/instances/%s/snap.img' % instance.uuid, + '-F', 'fake_fmt', + '/var/lib/nova/instances/%s/disk1_file' % instance.uuid) @mock.patch.object(compute_utils, 'disk_ops_semaphore') @mock.patch.object(host.Host, "has_min_version", diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index c9e48b3971b..e04d74c806e 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -2880,7 +2880,16 @@ def _rebase_with_qemu_img(source_path, rebase_base): # If the rebased image is going to have a backing file then # explicitly set the backing file format to avoid any security # concerns related to file format auto detection. - backing_file = rebase_base + if os.path.isabs(rebase_base): + backing_file = rebase_base + else: + # this is a probably a volume snapshot case where the + # rebase_base is relative. See bug + # https://bugs.launchpad.net/nova/+bug/1885528 + backing_file_name = os.path.basename(rebase_base) + volume_path = os.path.dirname(source_path) + backing_file = os.path.join(volume_path, backing_file_name) + b_file_fmt = images.qemu_img_info(backing_file).file_format qemu_img_extra_arg = ['-F', b_file_fmt] From a22d1b04de9e6ebc33b5ab9871b86f8e4022e7a9 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Wed, 31 Mar 2021 11:06:49 -0300 Subject: [PATCH 48/54] Error anti-affinity violation on migrations Error-out the migrations (cold and live) whenever the anti-affinity policy is violated. This addresses violations when multiple concurrent migrations are requested. Added detection on: - prep_resize - check_can_live_migration_destination - pre_live_migration The improved method of detection now locks based on group_id and considers other migrations in-progress as well. Closes-bug: #1821755 Change-Id: I32e6214568bb57f7613ddeba2c2c46da0320fabc (cherry picked from commit 33c8af1f8c46c9c37fcc28fb3409fbd3a78ae39f) (cherry picked from commit 8b62a4ec9bf617dfb2da046c25a9f76b33516508) (cherry picked from commit 6ede6df7f41db809de19e124d3d4994180598f19) (cherry picked from commit bf90a1e06181f6b328b967124e538c6e2579b2e5) --- nova/compute/manager.py | 110 ++++++++++++++--- nova/tests/unit/compute/test_compute_mgr.py | 114 ++++++++++++++++-- .../notes/bug-1821755-7bd03319e34b6b10.yaml | 11 ++ 3 files changed, 211 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml diff --git a/nova/compute/manager.py b/nova/compute/manager.py index e5f6ace00f3..dbf83085152 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -1587,7 +1587,11 @@ def _decode(f): return [_decode(f) for f in injected_files] def _validate_instance_group_policy(self, context, instance, - scheduler_hints): + scheduler_hints=None): + + if CONF.workarounds.disable_group_policy_check_upcall: + return + # NOTE(russellb) Instance group policy is enforced by the scheduler. # However, there is a race condition with the enforcement of # the policy. Since more than one instance may be scheduled at the @@ -1596,29 +1600,63 @@ def _validate_instance_group_policy(self, context, instance, # multiple instances with an affinity policy could end up on different # hosts. This is a validation step to make sure that starting the # instance here doesn't violate the policy. - group_hint = scheduler_hints.get('group') - if not group_hint: - return - - # The RequestSpec stores scheduler_hints as key=list pairs so we need - # to check the type on the value and pull the single entry out. The - # API request schema validates that the 'group' hint is a single value. - if isinstance(group_hint, list): - group_hint = group_hint[0] + if scheduler_hints is not None: + # only go through here if scheduler_hints is provided, even if it + # is empty. + group_hint = scheduler_hints.get('group') + if not group_hint: + return + else: + # The RequestSpec stores scheduler_hints as key=list pairs so + # we need to check the type on the value and pull the single + # entry out. The API request schema validates that + # the 'group' hint is a single value. + if isinstance(group_hint, list): + group_hint = group_hint[0] + + group = objects.InstanceGroup.get_by_hint(context, group_hint) + else: + # TODO(ganso): a call to DB can be saved by adding request_spec + # to rpcapi payload of live_migration, pre_live_migration and + # check_can_live_migrate_destination + try: + group = objects.InstanceGroup.get_by_instance_uuid( + context, instance.uuid) + except exception.InstanceGroupNotFound: + return - @utils.synchronized(group_hint) - def _do_validation(context, instance, group_hint): - group = objects.InstanceGroup.get_by_hint(context, group_hint) + @utils.synchronized(group['uuid']) + def _do_validation(context, instance, group): if group.policy and 'anti-affinity' == group.policy: + + # instances on host instances_uuids = objects.InstanceList.get_uuids_by_host( context, self.host) ins_on_host = set(instances_uuids) + + # instance param is just for logging, the nodename obtained is + # not actually related to the instance at all + nodename = self._get_nodename(instance) + + # instances being migrated to host + migrations = ( + objects.MigrationList.get_in_progress_by_host_and_node( + context, self.host, nodename)) + migration_vm_uuids = set([mig['instance_uuid'] + for mig in migrations]) + + total_instances = migration_vm_uuids | ins_on_host + + # refresh group to get updated members within locked block + group = objects.InstanceGroup.get_by_uuid(context, + group['uuid']) members = set(group.members) # Determine the set of instance group members on this host # which are not the instance in question. This is used to # determine how many other members from the same anti-affinity # group can be on this host. - members_on_host = ins_on_host & members - set([instance.uuid]) + members_on_host = (total_instances & members - + set([instance.uuid])) rules = group.rules if rules and 'max_server_per_host' in rules: max_server = rules['max_server_per_host'] @@ -1630,6 +1668,12 @@ def _do_validation(context, instance, group_hint): raise exception.RescheduledException( instance_uuid=instance.uuid, reason=msg) + + # NOTE(ganso): The check for affinity below does not work and it + # can easily be violated because the lock happens in different + # compute hosts. + # The only fix seems to be a DB lock to perform the check whenever + # setting the host field to an instance. elif group.policy and 'affinity' == group.policy: group_hosts = group.get_hosts(exclude=[instance.uuid]) if group_hosts and self.host not in group_hosts: @@ -1638,8 +1682,7 @@ def _do_validation(context, instance, group_hint): instance_uuid=instance.uuid, reason=msg) - if not CONF.workarounds.disable_group_policy_check_upcall: - _do_validation(context, instance, group_hint) + _do_validation(context, instance, group) def _log_original_error(self, exc_info, instance_uuid): LOG.error('Error: %s', exc_info[1], instance_uuid=instance_uuid, @@ -4773,10 +4816,24 @@ def prep_resize(self, context, image, instance, instance_type, with self._error_out_instance_on_exception( context, instance, instance_state=instance_state),\ errors_out_migration_ctxt(migration): + self._send_prep_resize_notifications( context, instance, fields.NotificationPhase.START, instance_type) try: + scheduler_hints = self._get_scheduler_hints(filter_properties, + request_spec) + # Error out if this host cannot accept the new instance due + # to anti-affinity. At this point the migration is already + # in-progress, so this is the definitive moment to abort due to + # the policy violation. Also, exploding here is covered by the + # cleanup methods in except block. + try: + self._validate_instance_group_policy(context, instance, + scheduler_hints) + except exception.RescheduledException as e: + raise exception.InstanceFaultRollback(inner_exception=e) + self._prep_resize(context, image, instance, instance_type, filter_properties, node, migration, request_spec, @@ -6811,6 +6868,20 @@ def check_can_live_migrate_destination(self, ctxt, instance, :param limits: objects.SchedulerLimits object for this live migration. :returns: a LiveMigrateData object (hypervisor-dependent) """ + + # Error out if this host cannot accept the new instance due + # to anti-affinity. This check at this moment is not very accurate, as + # multiple requests may be happening concurrently and miss the lock, + # but when it works it provides a better user experience by failing + # earlier. Also, it should be safe to explode here, error becomes + # NoValidHost and instance status remains ACTIVE. + try: + self._validate_instance_group_policy(ctxt, instance) + except exception.RescheduledException as e: + msg = ("Failed to validate instance group policy " + "due to: {}".format(e)) + raise exception.MigrationPreCheckError(reason=msg) + src_compute_info = obj_base.obj_to_primitive( self._get_compute_info(ctxt, instance.host)) dst_compute_info = obj_base.obj_to_primitive( @@ -6949,6 +7020,13 @@ def pre_live_migration(self, context, instance, block_migration, disk, """ LOG.debug('pre_live_migration data is %s', migrate_data) + # Error out if this host cannot accept the new instance due + # to anti-affinity. At this point the migration is already in-progress, + # so this is the definitive moment to abort due to the policy + # violation. Also, it should be safe to explode here. The instance + # status remains ACTIVE, migration status failed. + self._validate_instance_group_policy(context, instance) + migrate_data.old_vol_attachment_ids = {} bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( context, instance.uuid) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 8981b6f7ba4..93acfee330f 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -3198,12 +3198,16 @@ def _test_check_can_live_migrate_destination(self, do_raise=False, CONF.host, instance.uuid) return result + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_success(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutronv2.api.API.supports_port_binding_extension', lambda *args: True)) self._test_check_can_live_migrate_destination() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_fail(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutronv2.api.API.supports_port_binding_extension', @@ -3213,7 +3217,9 @@ def test_check_can_live_migrate_destination_fail(self): self._test_check_can_live_migrate_destination, do_raise=True) - def test_check_can_live_migrate_destination_contins_vifs(self): + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) + def test_check_can_live_migrate_destination_contains_vifs(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutronv2.api.API.supports_port_binding_extension', lambda *args: True)) @@ -3221,6 +3227,8 @@ def test_check_can_live_migrate_destination_contins_vifs(self): self.assertIn('vifs', migrate_data) self.assertIsNotNone(migrate_data.vifs) + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_no_binding_extended(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutronv2.api.API.supports_port_binding_extension', @@ -3228,18 +3236,40 @@ def test_check_can_live_migrate_destination_no_binding_extended(self): migrate_data = self._test_check_can_live_migrate_destination() self.assertNotIn('vifs', migrate_data) + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_src_numa_lm_false(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutronv2.api.API.supports_port_binding_extension', lambda *args: True)) self._test_check_can_live_migrate_destination(src_numa_lm=False) + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_src_numa_lm_true(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutronv2.api.API.supports_port_binding_extension', lambda *args: True)) self._test_check_can_live_migrate_destination(src_numa_lm=True) + @mock.patch.object(compute_utils, 'add_instance_fault_from_exc') + def test_check_can_live_migrate_destination_fail_group_policy( + self, mock_fail_db): + + instance = fake_instance.fake_instance_obj( + self.context, host=self.compute.host, vm_state=vm_states.ACTIVE, + node='fake-node') + + ex = exception.RescheduledException( + instance_uuid=instance.uuid, reason="policy violated") + + with mock.patch.object(self.compute, '_validate_instance_group_policy', + side_effect=ex): + self.assertRaises( + exception.MigrationPreCheckError, + self.compute.check_can_live_migrate_destination, + self.context, instance, None, None, None, None) + def test_dest_can_numa_live_migrate(self): positive_dest_check_data = objects.LibvirtLiveMigrateData( dst_supports_numa_live_migration=True) @@ -6979,7 +7009,8 @@ def test_prep_block_device_maintain_original_error_message(self, def test_validate_policy_honors_workaround_disabled(self, mock_get): instance = objects.Instance(uuid=uuids.instance) hints = {'group': 'foo'} - mock_get.return_value = objects.InstanceGroup(policy=None) + mock_get.return_value = objects.InstanceGroup(policy=None, + uuid=uuids.group) self.compute._validate_instance_group_policy(self.context, instance, hints) mock_get.assert_called_once_with(self.context, 'foo') @@ -7005,10 +7036,14 @@ def test_validate_instance_group_policy_handles_hint_list(self, mock_get): instance, hints) mock_get.assert_called_once_with(self.context, uuids.group_hint) + @mock.patch('nova.objects.InstanceGroup.get_by_uuid') @mock.patch('nova.objects.InstanceList.get_uuids_by_host') @mock.patch('nova.objects.InstanceGroup.get_by_hint') - def test_validate_instance_group_policy_with_rules(self, mock_get_by_hint, - mock_get_by_host): + @mock.patch.object(fake_driver.FakeDriver, 'get_available_nodes') + @mock.patch('nova.objects.MigrationList.get_in_progress_by_host_and_node') + def test_validate_instance_group_policy_with_rules( + self, migration_list, nodes, mock_get_by_hint, mock_get_by_host, + mock_get_by_uuid): # Create 2 instance in same host, inst2 created before inst1 instance = objects.Instance(uuid=uuids.inst1) hints = {'group': [uuids.group_hint]} @@ -7017,17 +7052,26 @@ def test_validate_instance_group_policy_with_rules(self, mock_get_by_hint, mock_get_by_host.return_value = existing_insts # if group policy rules limit to 1, raise RescheduledException - mock_get_by_hint.return_value = objects.InstanceGroup( + group = objects.InstanceGroup( policy='anti-affinity', rules={'max_server_per_host': '1'}, - hosts=['host1'], members=members_uuids) + hosts=['host1'], members=members_uuids, + uuid=uuids.group) + mock_get_by_hint.return_value = group + mock_get_by_uuid.return_value = group + nodes.return_value = ['nodename'] + migration_list.return_value = [objects.Migration( + uuid=uuids.migration, instance_uuid=uuids.instance)] self.assertRaises(exception.RescheduledException, self.compute._validate_instance_group_policy, self.context, instance, hints) # if group policy rules limit change to 2, validate OK - mock_get_by_hint.return_value = objects.InstanceGroup( + group2 = objects.InstanceGroup( policy='anti-affinity', rules={'max_server_per_host': 2}, - hosts=['host1'], members=members_uuids) + hosts=['host1'], members=members_uuids, + uuid=uuids.group) + mock_get_by_hint.return_value = group2 + mock_get_by_uuid.return_value = group2 self.compute._validate_instance_group_policy(self.context, instance, hints) @@ -8554,6 +8598,8 @@ def test_max_concurrent_live_semaphore_unlimited(self, mock_executor): manager.ComputeManager() mock_executor.assert_called_once_with() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_pre_live_migration_cinder_v3_api(self): # This tests that pre_live_migration with a bdm with an # attachment_id, will create a new attachment and update @@ -8631,6 +8677,8 @@ def _test(mock_attach, mock_get_bdms, mock_ivbi, _test() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_pre_live_migration_exception_cinder_v3_api(self): # The instance in this test has 2 attachments. The second attach_create # will throw an exception. This will test that the first attachment @@ -8700,6 +8748,8 @@ def _test(mock_attach_create, mock_attach_delete, mock_get_bdms, self.assertGreater(len(m.mock_calls), 0) _test() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_pre_live_migration_exceptions_delete_attachments(self): # The instance in this test has 2 attachments. The call to # driver.pre_live_migration will raise an exception. This will test @@ -10036,6 +10086,54 @@ def fake_reschedule_resize_or_reraise(*args, **kwargs): # (_error_out_instance_on_exception will set to ACTIVE by default). self.assertEqual(vm_states.STOPPED, instance.vm_state) + @mock.patch('nova.compute.utils.notify_usage_exists') + @mock.patch('nova.compute.manager.ComputeManager.' + '_notify_about_instance_usage') + @mock.patch('nova.compute.utils.notify_about_resize_prep_instance') + @mock.patch('nova.objects.Instance.save') + @mock.patch('nova.compute.manager.ComputeManager._revert_allocation') + @mock.patch('nova.compute.manager.ComputeManager.' + '_reschedule_resize_or_reraise') + @mock.patch('nova.compute.utils.add_instance_fault_from_exc') + # this is almost copy-paste from test_prep_resize_fails_rollback + def test_prep_resize_fails_group_validation( + self, add_instance_fault_from_exc, _reschedule_resize_or_reraise, + _revert_allocation, mock_instance_save, + notify_about_resize_prep_instance, _notify_about_instance_usage, + notify_usage_exists): + """Tests that if _validate_instance_group_policy raises + InstanceFaultRollback, the instance.vm_state is reset properly in + _error_out_instance_on_exception + """ + instance = fake_instance.fake_instance_obj( + self.context, host=self.compute.host, vm_state=vm_states.STOPPED, + node='fake-node', expected_attrs=['system_metadata', 'flavor']) + migration = mock.MagicMock(spec='nova.objects.Migration') + request_spec = mock.MagicMock(spec='nova.objects.RequestSpec') + ex = exception.RescheduledException( + instance_uuid=instance.uuid, reason="policy violated") + ex2 = exception.InstanceFaultRollback( + inner_exception=ex) + + def fake_reschedule_resize_or_reraise(*args, **kwargs): + raise ex2 + + _reschedule_resize_or_reraise.side_effect = ( + fake_reschedule_resize_or_reraise) + + with mock.patch.object(self.compute, '_validate_instance_group_policy', + side_effect=ex): + self.assertRaises( + # _error_out_instance_on_exception should reraise the + # RescheduledException inside InstanceFaultRollback. + exception.RescheduledException, self.compute.prep_resize, + self.context, instance.image_meta, instance, instance.flavor, + request_spec, filter_properties={}, node=instance.node, + clean_shutdown=True, migration=migration, host_list=[]) + # The instance.vm_state should remain unchanged + # (_error_out_instance_on_exception will set to ACTIVE by default). + self.assertEqual(vm_states.STOPPED, instance.vm_state) + @mock.patch('nova.compute.rpcapi.ComputeAPI.resize_instance') @mock.patch('nova.compute.resource_tracker.ResourceTracker.resize_claim') @mock.patch('nova.objects.Instance.save') diff --git a/releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml b/releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml new file mode 100644 index 00000000000..4c6135311bb --- /dev/null +++ b/releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Improved detection of anti-affinity policy violation when performing live + and cold migrations. Most of the violations caused by race conditions due + to performing concurrent live or cold migrations should now be addressed + by extra checks in the compute service. Upon detection, cold migration + operations are automatically rescheduled, while live migrations have two + checks and will be rescheduled if detected by the first one, otherwise the + live migration will fail cleanly and revert the instance state back to its + previous value. From 1aa571103f90228ddf3dc27386486196ad58ba0e Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Fri, 26 Mar 2021 12:18:37 +0100 Subject: [PATCH 49/54] [neutron] Get only ID and name of the SGs from Neutron During the VM booting process Nova asks Neutron for the security groups of the project. If there are no any fields specified, Neutron will prepare list of security groups with all fields, including rules. In case if project got many SGs, it may take long time as rules needs to be loaded separately for each SG on Neutron's side. During booting of the VM, Nova really needs only "id" and "name" of the security groups so this patch limits request to only those 2 fields. This lazy loading of the SG rules was introduced in Neutron in [1] and [2]. [1] https://review.opendev.org/#/c/630401/ [2] https://review.opendev.org/#/c/637407/ Related-Bug: #1865223 Change-Id: I15c3119857970c38541f4de270bd561166334548 (cherry picked from commit 388498ac5fa15ed8deef06ec061ea47e4a1b7377) (cherry picked from commit 4f49545afaf3cd453796d48ba96b9a82d11c01bf) (cherry picked from commit f7d84db5876b30d6849877799c08ebc65ac077ca) (cherry picked from commit be4a514c8aea073a9188cfc878c9afcc9b03cb28) --- nova/network/neutronv2/api.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 8f85a748f35..8508e63d78c 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -747,9 +747,15 @@ def _process_security_groups(self, instance, neutron, security_groups): # TODO(arosen) Should optimize more to do direct query for security # group if len(security_groups) == 1 if len(security_groups): + # NOTE(slaweq): fields other than name and id aren't really needed + # so asking only about those fields will allow Neutron to not + # prepare list of rules for each found security group. That may + # speed processing of this request a lot in case when tenant has + # got many security groups + sg_fields = ['id', 'name'] search_opts = {'tenant_id': instance.project_id} user_security_groups = neutron.list_security_groups( - **search_opts).get('security_groups') + fields=sg_fields, **search_opts).get('security_groups') for security_group in security_groups: name_match = None From c0a36d917794fed77e75ba9ed853c01a77b540bd Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Wed, 16 Dec 2020 13:12:13 +0000 Subject: [PATCH 50/54] only wait for plugtime events in pre-live-migration This change modifies _get_neutron_events_for_live_migration to filter the event to just the subset that will be sent at plug-time. Currently neuton has a bug where by the dhcp agent send a network-vif-plugged event during live migration after we update the port profile with "migrating-to:" this cause a network-vif-plugged event to be sent for configuration where vif_plugging in nova/os-vif is a noop. when that is corrected the current logic in nova cause the migration to time out as its waiting for an event that will never arrive. This change filters the set of events we wait for to just the plug time events. Conflicts: nova/compute/manager.py nova/tests/unit/compute/test_compute_mgr.py Related-Bug: #1815989 Closes-Bug: #1901707 Change-Id: Id2d8d72d30075200d2b07b847c4e5568599b0d3b (cherry picked from commit 8b33ac064456482158b23c2a2d52f819ebb4c60e) (cherry picked from commit ef348c4eb3379189f290217c9351157b1ebf0adb) (cherry picked from commit d9c833d5a404dfa206e08c97543e80cb613b3f0b) --- nova/compute/manager.py | 4 ++-- nova/network/model.py | 23 +++++++++++++++++---- nova/tests/unit/compute/test_compute_mgr.py | 21 ++++++++++++++++--- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index dbf83085152..ce74fec4c66 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -7154,8 +7154,8 @@ def _get_neutron_events_for_live_migration(instance): # We don't generate events if CONF.vif_plugging_timeout=0 # meaning that the operator disabled using them. if CONF.vif_plugging_timeout and utils.is_neutron(): - return [('network-vif-plugged', vif['id']) - for vif in instance.get_network_info()] + return (instance.get_network_info() + .get_live_migration_plug_time_events()) else: return [] diff --git a/nova/network/model.py b/nova/network/model.py index d8119fae729..7ed9d2d1b86 100644 --- a/nova/network/model.py +++ b/nova/network/model.py @@ -469,6 +469,14 @@ def has_bind_time_event(self, migration): return (self.is_hybrid_plug_enabled() and not migration.is_same_host()) + @property + def has_live_migration_plug_time_event(self): + """Returns whether this VIF's network-vif-plugged external event will + be sent by Neutron at "plugtime" - in other words, as soon as neutron + completes configuring the network backend. + """ + return self.is_hybrid_plug_enabled() + def is_hybrid_plug_enabled(self): return self['details'].get(VIF_DETAILS_OVS_HYBRID_PLUG, False) @@ -530,15 +538,22 @@ def json(self): return jsonutils.dumps(self) def get_bind_time_events(self, migration): - """Returns whether any of our VIFs have "bind-time" events. See - has_bind_time_event() docstring for more details. + """Returns a list of external events for any VIFs that have + "bind-time" events during cold migration. """ return [('network-vif-plugged', vif['id']) for vif in self if vif.has_bind_time_event(migration)] + def get_live_migration_plug_time_events(self): + """Returns a list of external events for any VIFs that have + "plug-time" events during live migration. + """ + return [('network-vif-plugged', vif['id']) + for vif in self if vif.has_live_migration_plug_time_event] + def get_plug_time_events(self, migration): - """Complementary to get_bind_time_events(), any event that does not - fall in that category is a plug-time event. + """Returns a list of external events for any VIFs that have + "plug-time" events during cold migration. """ return [('network-vif-plugged', vif['id']) for vif in self if not vif.has_bind_time_event(migration)] diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 93acfee330f..526302de8ad 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -8831,6 +8831,16 @@ def test_get_neutron_events_for_live_migration_empty(self): self.flags(vif_plugging_timeout=300, use_neutron=True) self.assertEqual( [], self.compute._get_neutron_events_for_live_migration([])) + # 4. no plug time events + with mock.patch.object(self.instance, 'get_network_info') as nw_info: + nw_info.return_value = network_model.NetworkInfo( + [network_model.VIF( + uuids.port1, details={ + network_model.VIF_DETAILS_OVS_HYBRID_PLUG: False})]) + self.assertFalse(nw_info.return_value[0].is_hybrid_plug_enabled()) + self.assertEqual( + [], self.compute._get_neutron_events_for_live_migration( + self.instance)) @mock.patch('nova.compute.rpcapi.ComputeAPI.pre_live_migration') @mock.patch('nova.compute.manager.ComputeManager._post_live_migration') @@ -8845,9 +8855,11 @@ def test_live_migration_wait_vif_plugged( wait_for_vif_plugged=True) mock_get_bdms.return_value = objects.BlockDeviceMappingList(objects=[]) mock_pre_live_mig.return_value = migrate_data + details = {network_model.VIF_DETAILS_OVS_HYBRID_PLUG: True} self.instance.info_cache = objects.InstanceInfoCache( network_info=network_model.NetworkInfo([ - network_model.VIF(uuids.port1), network_model.VIF(uuids.port2) + network_model.VIF(uuids.port1, details=details), + network_model.VIF(uuids.port2, details=details) ])) self.compute._waiting_live_migrations[self.instance.uuid] = ( self.migration, mock.MagicMock() @@ -8877,11 +8889,12 @@ def test_live_migration_wait_vif_plugged_old_dest_host( of not waiting. """ migrate_data = objects.LibvirtLiveMigrateData() + details = {network_model.VIF_DETAILS_OVS_HYBRID_PLUG: True} mock_get_bdms.return_value = objects.BlockDeviceMappingList(objects=[]) mock_pre_live_mig.return_value = migrate_data self.instance.info_cache = objects.InstanceInfoCache( network_info=network_model.NetworkInfo([ - network_model.VIF(uuids.port1)])) + network_model.VIF(uuids.port1, details=details)])) self.compute._waiting_live_migrations[self.instance.uuid] = ( self.migration, mock.MagicMock() ) @@ -9025,9 +9038,11 @@ def test_live_migration_aborted_before_running(self, mock_rpc, mock_get_bdms.return_value = source_bdms migrate_data = objects.LibvirtLiveMigrateData( wait_for_vif_plugged=True) + details = {network_model.VIF_DETAILS_OVS_HYBRID_PLUG: True} self.instance.info_cache = objects.InstanceInfoCache( network_info=network_model.NetworkInfo([ - network_model.VIF(uuids.port1), network_model.VIF(uuids.port2) + network_model.VIF(uuids.port1, details=details), + network_model.VIF(uuids.port2, details=details) ])) self.compute._waiting_live_migrations = {} fake_migration = objects.Migration( From f1be212a86262f128e73f7201b7b3e51334f1ab4 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Wed, 22 Sep 2021 17:54:23 +0200 Subject: [PATCH 51/54] [stable-only] Pin virtualenv and setuptools Setuptools 58.0 (bundled in virtualenv 20.8) breaks the installation of decorator 3.4.0. So this patch pins virtualenv to avoid the break. As the used 'require' feature was introduced in tox in version 3.2 [1], the required minversion has to be bumped, too. [1] https://tox.readthedocs.io/en/latest/config.html#conf-requires Conflicts: tox.ini NOTE(melwitt): The conflict is because change Ie1a0cbd82a617dbcc15729647218ac3e9cd0e5a9 (Stop testing Python 2) is not in Train. Change-Id: I26b2a14e0b91c0ab77299c3e4fbed5f7916fe8cf (cherry picked from commit b27f8e9adfcf2db3c83722c42e055ba5065ad06e) --- tox.ini | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 76992f718c5..47e6d1e88cb 100644 --- a/tox.ini +++ b/tox.ini @@ -1,10 +1,14 @@ [tox] -minversion = 3.1.1 +minversion = 3.2.0 envlist = py27,py37,functional,pep8 # Automatic envs (pyXX) will only use the python version appropriate to that # env and ignore basepython inherited from [testenv] if we set # ignore_basepython_conflict. ignore_basepython_conflict = True +# Pin the virtualenv and therefore the setuptools version used for the env +# creation. This results in a new tox being installed in .tox/.tox virtualenv +# and the tox on the host will delegate all the calls to the tox in that env. +requires = virtualenv<20.8 [testenv] basepython = python3 From 04d48527b62a35d912f93bc75613a6cca606df66 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 13 May 2021 05:43:42 +0000 Subject: [PATCH 52/54] Reject open redirection in the console proxy NOTE(melwitt): This is the combination of two commits, the bug fix and a followup change to the unit test to enable it also run on Python < 3.6. Our console proxies (novnc, serial, spice) run in a websockify server whose request handler inherits from the python standard SimpleHTTPRequestHandler. There is a known issue [1] in the SimpleHTTPRequestHandler which allows open redirects by way of URLs in the following format: http://vncproxy.my.domain.com//example.com/%2F.. which if visited, will redirect a user to example.com. We can intercept a request and reject requests that pass a redirection URL beginning with "//" by implementing the SimpleHTTPRequestHandler.send_head() method containing the vulnerability to reject such requests with a 400 Bad Request. This code is copied from a patch suggested in one of the issue comments [2]. Closes-Bug: #1927677 [1] https://bugs.python.org/issue32084 [2] https://bugs.python.org/issue32084#msg306545 Conflicts: nova/tests/unit/console/test_websocketproxy.py NOTE(melwitt): The conflict is because change I23ac1cc79482d0fabb359486a4b934463854cae5 (Allow TLS ciphers/protocols to be configurable for console proxies) is not in Train. NOTE(melwitt): The difference from the cherry picked change: HTTPStatus.BAD_REQUEST => 400 is due to the fact that HTTPStatus does not exist in Python 2.7. Reduce mocking in test_reject_open_redirect for compat This is a followup for change Ie36401c782f023d1d5f2623732619105dc2cfa24 to reduce mocking in the unit test coverage for it. While backporting the bug fix, it was found to be incompatible with earlier versions of Python < 3.6 due to a difference in internal implementation [1]. This reduces the mocking in the unit test to be more agnostic to the internals of the StreamRequestHandler (ancestor of SimpleHTTPRequestHandler) and work across Python versions >= 2.7. Related-Bug: #1927677 [1] https://github.com/python/cpython/commit/34eeed42901666fce099947f93dfdfc05411f286 Change-Id: I546d376869a992601b443fb95acf1034da2a8f36 (cherry picked from commit 214cabe6848a1fdb4f5941d994c6cc11107fc4af) (cherry picked from commit 9c2f29783734cb5f9cb05a08d328c10e1d16c4f1) (cherry picked from commit 94e265f3ca615aa18de0081a76975019997b8709) (cherry picked from commit d43b88a33407b1253e7bce70f720a44f7688141f) Change-Id: Ie36401c782f023d1d5f2623732619105dc2cfa24 (cherry picked from commit 781612b33282ed298f742c85dab58a075c8b793e) (cherry picked from commit 470925614223c8dd9b1233f54f5a96c02b2d4f70) (cherry picked from commit 6b70350bdcf59a9712f88b6435ba2c6500133e5b) (cherry picked from commit 719e651e6be277950632e0c2cf5cc9a018344e7b) --- nova/console/websocketproxy.py | 22 +++++++++++++ .../tests/unit/console/test_websocketproxy.py | 33 +++++++++++++++++++ ...reject-open-redirect-4ac0a7895acca7eb.yaml | 19 +++++++++++ 3 files changed, 74 insertions(+) create mode 100644 releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml diff --git a/nova/console/websocketproxy.py b/nova/console/websocketproxy.py index e13b3c0fe15..87d755ec66f 100644 --- a/nova/console/websocketproxy.py +++ b/nova/console/websocketproxy.py @@ -19,6 +19,7 @@ ''' import copy +import os import socket import sys @@ -305,6 +306,27 @@ def socket(self, *args, **kwargs): # Fall back to the websockify <= v0.8.0 'socket' method location. return websockify.WebSocketServer.socket(*args, **kwargs) + def send_head(self): + # This code is copied from this example patch: + # https://bugs.python.org/issue32084#msg306545 + path = self.translate_path(self.path) + if os.path.isdir(path): + parts = urlparse.urlsplit(self.path) + if not parts.path.endswith('/'): + # redirect browser - doing basically what apache does + new_parts = (parts[0], parts[1], parts[2] + '/', + parts[3], parts[4]) + new_url = urlparse.urlunsplit(new_parts) + + # Browsers interpret "Location: //uri" as an absolute URI + # like "http://URI" + if new_url.startswith('//'): + self.send_error(400, + "URI must not start with //") + return None + + return super(NovaProxyRequestHandler, self).send_head() + class NovaWebSocketProxy(websockify.WebSocketProxy): def __init__(self, *args, **kwargs): diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 98e162d59cb..45917c2f41c 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -15,6 +15,7 @@ """Tests for nova websocketproxy.""" import copy +import io import socket import mock @@ -626,6 +627,38 @@ def test_tcp_rst_no_compute_rpcapi(self): self.wh.server.top_new_client(conn, address) self.assertIsNone(self.wh._compute_rpcapi) + def test_reject_open_redirect(self): + # This will test the behavior when an attempt is made to cause an open + # redirect. It should be rejected. + mock_req = mock.MagicMock() + mock_req.makefile().readline.side_effect = [ + b'GET //example.com/%2F.. HTTP/1.1\r\n', + b'' + ] + + client_addr = ('8.8.8.8', 54321) + mock_server = mock.MagicMock() + # This specifies that the server will be able to handle requests other + # than only websockets. + mock_server.only_upgrade = False + + # Constructing a handler will process the mock_req request passed in. + handler = websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data to a 'wfile' + # attribute. + output = io.BytesIO() + handler.wfile = output + # Process the mock_req again to do the capture. + handler.do_GET() + output.seek(0) + result = output.readlines() + + # Verify no redirect happens and instead a 400 Bad Request is returned. + self.assertIn('400 URI must not start with //', result[0].decode()) + class NovaWebsocketSecurityProxyTestCase(test.NoDBTestCase): diff --git a/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml b/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml new file mode 100644 index 00000000000..ce05b9a8670 --- /dev/null +++ b/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml @@ -0,0 +1,19 @@ +--- +security: + - | + A vulnerability in the console proxies (novnc, serial, spice) that allowed + open redirection has been `patched`_. The novnc, serial, and spice console + proxies are implemented as websockify servers and the request handler + inherits from the python standard SimpleHTTPRequestHandler. There is a + `known issue`_ in the SimpleHTTPRequestHandler which allows open redirects + by way of URLs in the following format:: + + http://vncproxy.my.domain.com//example.com/%2F.. + + which if visited, will redirect a user to example.com. + + The novnc, serial, and spice console proxies will now reject requests that + pass a redirection URL beginning with "//" with a 400 Bad Request. + + .. _patched: https://bugs.launchpad.net/nova/+bug/1927677 + .. _known issue: https://bugs.python.org/issue32084 From 8906552cfc2525a44251d4cf313ece61e57251eb Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Mon, 23 Aug 2021 15:37:48 +0100 Subject: [PATCH 53/54] address open redirect with 3 forward slashes Ie36401c782f023d1d5f2623732619105dc2cfa24 was intended to address OSSA-2021-002 (CVE-2021-3654) however after its release it was discovered that the fix only worked for urls with 2 leading slashes or more then 4. This change adresses the missing edgecase for 3 leading slashes and also maintian support for rejecting 2+. Conflicts: nova/console/websocketproxy.py nova/tests/unit/console/test_websocketproxy.py NOTE(melwitt): The conflict and difference in websocketproxy.py from the cherry picked change: HTTPStatus.BAD_REQUEST => 400 is due to the fact that HTTPStatus does not exist in Python 2.7. The conflict in test_websocketproxy.py is because change I23ac1cc79482d0fabb359486a4b934463854cae5 (Allow TLS ciphers/protocols to be configurable for console proxies) is not in Train. The difference in test_websocketproxy.py from the cherry picked change is due to a difference in internal implementation [1] in Python < 3.6. See change I546d376869a992601b443fb95acf1034da2a8f36 for reference. [1] https://github.com/python/cpython/commit/34eeed42901666fce099947f93dfdfc05411f286 Change-Id: I95f68be76330ff09e5eabb5ef8dd9a18f5547866 co-authored-by: Matteo Pozza Closes-Bug: #1927677 (cherry picked from commit 6fbd0b758dcac71323f3be179b1a9d1c17a4acc5) (cherry picked from commit 47dad4836a26292e9d34e516e1525ecf00be127c) (cherry picked from commit 9588cdbfd4649ea53d60303f2d10c5d62a070a07) (cherry picked from commit 0997043f459ac616b594363b5b253bd0ae6ed9eb) --- nova/console/websocketproxy.py | 7 +--- .../tests/unit/console/test_websocketproxy.py | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/nova/console/websocketproxy.py b/nova/console/websocketproxy.py index 87d755ec66f..7641a7cc086 100644 --- a/nova/console/websocketproxy.py +++ b/nova/console/websocketproxy.py @@ -313,14 +313,9 @@ def send_head(self): if os.path.isdir(path): parts = urlparse.urlsplit(self.path) if not parts.path.endswith('/'): - # redirect browser - doing basically what apache does - new_parts = (parts[0], parts[1], parts[2] + '/', - parts[3], parts[4]) - new_url = urlparse.urlunsplit(new_parts) - # Browsers interpret "Location: //uri" as an absolute URI # like "http://URI" - if new_url.startswith('//'): + if self.path.startswith('//'): self.send_error(400, "URI must not start with //") return None diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 45917c2f41c..27677bbf78e 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -659,6 +659,38 @@ def test_reject_open_redirect(self): # Verify no redirect happens and instead a 400 Bad Request is returned. self.assertIn('400 URI must not start with //', result[0].decode()) + def test_reject_open_redirect_3_slashes(self): + # This will test the behavior when an attempt is made to cause an open + # redirect. It should be rejected. + mock_req = mock.MagicMock() + mock_req.makefile().readline.side_effect = [ + b'GET ///example.com/%2F.. HTTP/1.1\r\n', + b'' + ] + + client_addr = ('8.8.8.8', 54321) + mock_server = mock.MagicMock() + # This specifies that the server will be able to handle requests other + # than only websockets. + mock_server.only_upgrade = False + + # Constructing a handler will process the mock_req request passed in. + handler = websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data to a 'wfile' + # attribute. + output = io.BytesIO() + handler.wfile = output + # Process the mock_req again to do the capture. + handler.do_GET() + output.seek(0) + result = output.readlines() + + # Verify no redirect happens and instead a 400 Bad Request is returned. + self.assertIn('400 URI must not start with //', result[0].decode()) + class NovaWebsocketSecurityProxyTestCase(test.NoDBTestCase): From a5da31ec1ea1d1c7b4df146857982699ebdc328e Mon Sep 17 00:00:00 2001 From: Dmitriy Rabotyagov Date: Thu, 30 Sep 2021 14:08:38 +0300 Subject: [PATCH 54/54] Ensure MAC addresses characters are in the same case Currently neutron can report ports to have MAC addresses in upper case when they're created like that. In the meanwhile libvirt configuration file always stores MAC in lower case which leads to KeyError while trying to retrieve migrate_vif. Closes-Bug: #1945646 Change-Id: Ie3129ee395427337e9abcef2f938012608f643e1 (cherry picked from commit 6a15169ed9f16672c2cde1d7f27178bb7868c41f) (cherry picked from commit 63a6388f6a0265f84232731aba8aec1bff3c6d18) (cherry picked from commit 6c3d5de659e558e8f6ee353475b54ff3ca7240ee) (cherry picked from commit 28d0059c1f52e51add31bff50f1f6e443c938792) (cherry picked from commit 184a3c976faed38907af148a533bc6e9faa410f5) --- .../tests/unit/virt/libvirt/test_migration.py | 43 ++++++++++++++++++- nova/virt/libvirt/migration.py | 11 ++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_migration.py b/nova/tests/unit/virt/libvirt/test_migration.py index 7adfb0ef65b..07d11f94e55 100644 --- a/nova/tests/unit/virt/libvirt/test_migration.py +++ b/nova/tests/unit/virt/libvirt/test_migration.py @@ -955,7 +955,48 @@ def test_update_vif_xml_no_matching_vif(self): doc = etree.fromstring(original_xml) ex = self.assertRaises(KeyError, migration._update_vif_xml, doc, data, get_vif_config) - self.assertIn("CA:FE:DE:AD:BE:EF", six.text_type(ex)) + self.assertIn("ca:fe:de:ad:be:ef", six.text_type(ex)) + + def test_update_vif_xml_lower_case_mac(self): + """Tests that the vif in the migrate data is not found in the existing + guest interfaces. + """ + conf = vconfig.LibvirtConfigGuestInterface() + conf.net_type = "bridge" + conf.source_dev = "qbra188171c-ea" + conf.target_dev = "tapa188171c-ea" + conf.mac_addr = "DE:AD:BE:EF:CA:FE" + conf.model = "virtio" + original_xml = """ + 3de6550a-8596-4937-8046-9d862036bca5 + + + + + + + + + +
+ + + """ % uuids.ovs + expected_xml = """ + 3de6550a-8596-4937-8046-9d862036bca5 + + + + + + +
+ + + """ + self._test_update_vif_xml(conf, original_xml, expected_xml) class MigrationMonitorTestCase(test.NoDBTestCase): diff --git a/nova/virt/libvirt/migration.py b/nova/virt/libvirt/migration.py index 7b7214712c8..36a9d4bcd9b 100644 --- a/nova/virt/libvirt/migration.py +++ b/nova/virt/libvirt/migration.py @@ -346,14 +346,21 @@ def _update_vif_xml(xml_doc, migrate_data, get_vif_config): instance_uuid = xml_doc.findtext('uuid') parser = etree.XMLParser(remove_blank_text=True) interface_nodes = xml_doc.findall('./devices/interface') - migrate_vif_by_mac = {vif.source_vif['address']: vif + # MAC address stored for port in neutron DB and in domain XML + # might be in different cases, so to harmonize that + # we convert MAC to lower case for dict key. + migrate_vif_by_mac = {vif.source_vif['address'].lower(): vif for vif in migrate_data.vifs} for interface_dev in interface_nodes: mac = interface_dev.find('mac') mac = mac if mac is not None else {} mac_addr = mac.get('address') if mac_addr: - migrate_vif = migrate_vif_by_mac[mac_addr] + # MAC address stored in libvirt should always be normalized + # and stored in lower case. But just to be extra safe here + # we still normalize MAC retrieved from XML to be absolutely + # sure it will be the same with the Neutron provided one. + migrate_vif = migrate_vif_by_mac[mac_addr.lower()] vif = migrate_vif.get_dest_vif() # get_vif_config is a partial function of # nova.virt.libvirt.vif.LibvirtGenericVIFDriver.get_config