From 468b03e0ee4a917ae26106f6e57081bcd9e7a65b Mon Sep 17 00:00:00 2001 From: Kashyap Chamarthy Date: Wed, 11 Jan 2023 17:55:20 +0100 Subject: [PATCH] libvirt: Replace usage of compareCPU() with compareHypervisorCPU() The older libvirt API compareCPU() does not take into account the capabilities of the "host hypervisor" (KVM, QEMU and the details libvirt knows about the host) when comparing CPUs. This is causing unwanted failures during CPU compatibility checks. To fix this and other related problems, libvirt has introduced (in v4.4.0) a newer API, compareHypervisorCPU(), which _does_ take into account the host hypervisor's capabilities before comparing CPUs. This will help fix a range of problems, such as[1][2]. So let's switch to the newer API, which is largely a drop-in replacement. In this patch: - Introduce a wrapper method, compare_hypervisor_cpu() for libvirt's compareHypervisorCPU() API. - Update the _compare_cpu() method to use the wrapper method, compare_hypervisor_cpu(). - Update the unit tests to use the newer API, compareHypervisorCPU(). [1] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1978064 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2138381 Change-Id: Ib523753f52993cfe72e35e0309e429ca879c125c Signed-off-by: Kashyap Chamarthy --- nova/tests/fixtures/libvirt.py | 6 +++ nova/tests/unit/virt/libvirt/test_driver.py | 40 ++++++++++--------- nova/virt/libvirt/driver.py | 2 +- nova/virt/libvirt/host.py | 16 ++++++++ ...compareHypervisorCPU-b75c8f097cc73556.yaml | 12 ++++++ 5 files changed, 57 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/use-compareHypervisorCPU-b75c8f097cc73556.yaml diff --git a/nova/tests/fixtures/libvirt.py b/nova/tests/fixtures/libvirt.py index 5d20e7d54f..4f48463118 100644 --- a/nova/tests/fixtures/libvirt.py +++ b/nova/tests/fixtures/libvirt.py @@ -2044,6 +2044,12 @@ class Connection(object): return VIR_CPU_COMPARE_IDENTICAL + def compareHypervisorCPU( + self, emulator, arch, machine, virttype, + xml, flags + ): + return self.compareCPU(xml, flags) + def getCPUStats(self, cpuNum, flag): if cpuNum < 2: return {'kernel': 5664160000000, diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 9d45ba017c..44d64978a0 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -1320,7 +1320,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) self.assertRaises(exception.Invalid, drvr.init_host, "dummyhost") - @mock.patch('nova.virt.libvirt.host.libvirt.Connection.compareCPU') + @mock.patch( + 'nova.virt.libvirt.host.libvirt.Connection.compareHypervisorCPU') def test__check_cpu_compatibility_advance_model(self, mocked_compare): mocked_compare.side_effect = (2, 0) self.flags(cpu_mode="custom", @@ -1357,7 +1358,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) drvr.init_host("dummyhost") - @mock.patch('nova.virt.libvirt.host.libvirt.Connection.compareCPU') + @mock.patch( + 'nova.virt.libvirt.host.libvirt.Connection.compareHypervisorCPU') def test__check_cpu_compatibility_advance_flag(self, mocked_compare): mocked_compare.side_effect = (-1, 0) self.flags(cpu_mode="custom", @@ -1368,7 +1370,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertRaises(exception.InvalidCPUInfo, drvr.init_host, "dummyhost") - @mock.patch('nova.virt.libvirt.host.libvirt.Connection.compareCPU') + @mock.patch( + 'nova.virt.libvirt.host.libvirt.Connection.compareHypervisorCPU') def test__check_cpu_compatibility_wrong_flag(self, mocked_compare): # here, and in the surrounding similar tests, the non-zero error # code in the compareCPU() side effect indicates error @@ -1381,7 +1384,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertRaises(exception.InvalidCPUInfo, drvr.init_host, "dummyhost") - @mock.patch('nova.virt.libvirt.host.libvirt.Connection.compareCPU') + @mock.patch( + 'nova.virt.libvirt.host.libvirt.Connection.compareHypervisorCPU') def test__check_cpu_compatibility_enabled_and_disabled_flags( self, mocked_compare ): @@ -11162,7 +11166,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, new=mock.Mock(return_value=False)) @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_all_pass_with_block_migration( self, mock_cpu, mock_test_file, ): @@ -11201,7 +11205,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, new=mock.Mock(return_value=False)) @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_all_pass_with_over_commit( self, mock_cpu, mock_test_file, ): @@ -11241,7 +11245,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, new=mock.Mock(return_value=False)) @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_all_pass_no_block_migration( self, mock_cpu, mock_test_file, ): @@ -11279,7 +11283,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file', return_value='fake') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_fills_listen_addrs( self, mock_cpu, mock_test_file, ): @@ -11311,7 +11315,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file', return_value='fake') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU', + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU', return_value=1) def test_check_can_live_migrate_dest_ensure_serial_adds_not_set( self, mock_cpu, mock_test_file, @@ -11419,7 +11423,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, new=mock.Mock(return_value=False)) @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_no_instance_cpu_info( self, mock_cpu, mock_test_file, ): @@ -11460,7 +11464,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, new=mock.Mock(return_value=False)) @mock.patch.object(libvirt_driver.LibvirtDriver, '_create_shared_storage_test_file') - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_file_backed( self, mock_cpu, mock_test_file, ): @@ -11486,7 +11490,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertTrue(return_value.dst_wants_file_backed_memory) - @mock.patch.object(fakelibvirt.Connection, 'compareCPU') + @mock.patch.object(fakelibvirt.Connection, 'compareHypervisorCPU') def test_check_can_live_migrate_dest_incompatible_cpu_raises( self, mock_cpu): instance_ref = objects.Instance(**self.test_instance) @@ -11522,7 +11526,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, for vif in result.vifs: self.assertTrue(vif.supports_os_vif_delegation) - @mock.patch.object(host.Host, 'compare_cpu') + @mock.patch.object(host.Host, 'compare_hypervisor_cpu') @mock.patch.object(nova.virt.libvirt, 'config') def test_compare_cpu_compatible_host_cpu(self, mock_vconfig, mock_compare): instance = objects.Instance(**self.test_instance) @@ -11532,7 +11536,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, instance) self.assertIsNone(ret) - @mock.patch.object(host.Host, 'compare_cpu') + @mock.patch.object(host.Host, 'compare_hypervisor_cpu') @mock.patch.object(nova.virt.libvirt, 'config') def test_compare_cpu_handles_not_supported_error_gracefully(self, mock_vconfig, @@ -11569,7 +11573,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch.object(fakelibvirt.Connection, 'getLibVersion', return_value=versionutils.convert_version_to_int( libvirt_driver.MIN_LIBVIRT_AARCH64_CPU_COMPARE)) - @mock.patch.object(host.Host, 'compare_cpu') + @mock.patch.object(host.Host, 'compare_hypervisor_cpu') def test_compare_cpu_host_aarch64(self, mock_compare, mock_get_libversion, @@ -11592,7 +11596,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_compare.assert_called_once_with(caps.host.cpu.to_xml()) self.assertIsNone(ret) - @mock.patch.object(host.Host, 'compare_cpu') + @mock.patch.object(host.Host, 'compare_hypervisor_cpu') @mock.patch.object(nova.virt.libvirt.LibvirtDriver, '_vcpu_model_to_cpu_config') def test_compare_cpu_compatible_guest_cpu(self, mock_vcpu_to_cpu, @@ -11611,7 +11615,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, ret = conn._compare_cpu(None, None, instance) self.assertIsNone(ret) - @mock.patch.object(host.Host, 'compare_cpu') + @mock.patch.object(host.Host, 'compare_hypervisor_cpu') @mock.patch.object(nova.virt.libvirt, 'config') def test_compare_cpu_invalid_cpuinfo_raises(self, mock_vconfig, mock_compare): @@ -11623,7 +11627,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, jsonutils.dumps(_fake_cpu_info), instance) - @mock.patch.object(host.Host, 'compare_cpu') + @mock.patch.object(host.Host, 'compare_hypervisor_cpu') @mock.patch.object(nova.virt.libvirt, 'config') def test_compare_cpu_incompatible_cpu_raises(self, mock_vconfig, mock_compare): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index ff503de7e6..221d292a74 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -9963,7 +9963,7 @@ class LibvirtDriver(driver.ComputeDriver): try: cpu_xml = cpu.to_xml() LOG.debug("cpu compare xml: %s", cpu_xml, instance=instance) - ret = self._host.compare_cpu(cpu_xml) + ret = self._host.compare_hypervisor_cpu(cpu_xml) except libvirt.libvirtError as e: error_code = e.get_error_code() if error_code == libvirt.VIR_ERR_NO_SUPPORT: diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index 46435a9a7f..9026da7217 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -1605,6 +1605,22 @@ class Host(object): """Compares the given CPU description with the host CPU.""" return self.get_connection().compareCPU(xmlDesc, flags) + def compare_hypervisor_cpu(self, xmlDesc, flags=0): + """Compares the given CPU description with the CPU provided by + the host hypervisor. This is different from the older method, + compare_cpu(), which compares a given CPU definition with the + host CPU without considering the abilities of the host + hypervisor. Except @xmlDesc, rest of all the parameters to + compareHypervisorCPU API are optional (libvirt will choose + sensible defaults). + """ + emulator = None + arch = None + machine = None + virttype = None + return self.get_connection().compareHypervisorCPU( + emulator, arch, machine, virttype, xmlDesc, flags) + def is_cpu_control_policy_capable(self): """Returns whether kernel configuration CGROUP_SCHED is enabled diff --git a/releasenotes/notes/use-compareHypervisorCPU-b75c8f097cc73556.yaml b/releasenotes/notes/use-compareHypervisorCPU-b75c8f097cc73556.yaml new file mode 100644 index 0000000000..924e09a602 --- /dev/null +++ b/releasenotes/notes/use-compareHypervisorCPU-b75c8f097cc73556.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Nova's use of libvirt's compareCPU() API has become error-prone as + it doesn't take into account host hypervisor's capabilities. With + QEMU >=2.9 and libvirt >= 4.4.0, libvirt will do the right thing in + terms of CPU comparison checks via a new replacement API, + compareHypervisorCPU(). Nova satisfies the said minimum version + requirements of QEMU and libvirt by a good margin. + + This change replaces the usage of older API, compareCPU(), with the + new one, compareHypervisorCPU().