Merge pull request #56 from stackhpc/bugfix/yoga-live-migrate-rocky-9

libvirt: remove default cputune shares value

Author: jovial

doc/source/admin/resource-limits.rst

@@ -38,7 +38,8 @@ CPU limits
 Libvirt enforces CPU limits in terms of *shares* and *quotas*, configured
 via :nova:extra-spec:`quota:cpu_shares` and :nova:extra-spec:`quota:cpu_period`
 / :nova:extra-spec:`quota:cpu_quota`, respectively. Both are implemented using
-the `cgroups v1 cpu controller`__.
+the `cgroups cpu controller`__. Note that allowed values for *shares* are
+platform dependant.
 
 CPU shares are a proportional weighted share of total CPU resources relative to
 other instances. It does not limit CPU usage if CPUs are not busy. There is no

nova/tests/unit/virt/libvirt/test_driver.py

@@ -2989,7 +2989,7 @@ def test_get_guest_config_numa_host_instance_fits(self):
             cfg = drvr._get_guest_config(instance_ref, [],
                                          image_meta, disk_info)
             self.assertIsNone(cfg.cpuset)
-            self.assertEqual(0, len(cfg.cputune.vcpupin))
+            self.assertIsNone(cfg.cputune)
             self.assertIsNone(cfg.cpu.numa)
 
     @mock.patch('nova.privsep.utils.supports_direct_io',
@@ -3024,7 +3024,7 @@ def test_get_guest_config_numa_host_instance_no_fit(self):
                                          image_meta, disk_info)
             self.assertFalse(choice_mock.called)
             self.assertIsNone(cfg.cpuset)
-            self.assertEqual(0, len(cfg.cputune.vcpupin))
+            self.assertIsNone(cfg.cputune)
             self.assertIsNone(cfg.cpu.numa)
 
     def _test_get_guest_memory_backing_config(
@@ -3429,7 +3429,7 @@ def test_get_guest_config_numa_host_instance_pci_no_numa_info(self):
             cfg = conn._get_guest_config(instance_ref, [],
                                          image_meta, disk_info)
             self.assertEqual(set([3]), cfg.cpuset)
-            self.assertEqual(0, len(cfg.cputune.vcpupin))
+            self.assertIsNone(cfg.cputune)
             self.assertIsNone(cfg.cpu.numa)
 
     @mock.patch('nova.privsep.utils.supports_direct_io',
@@ -3481,7 +3481,7 @@ def test_get_guest_config_numa_host_instance_2pci_no_fit(self):
                                          image_meta, disk_info)
             self.assertFalse(choice_mock.called)
             self.assertEqual(set([3]), cfg.cpuset)
-            self.assertEqual(0, len(cfg.cputune.vcpupin))
+            self.assertIsNone(cfg.cputune)
             self.assertIsNone(cfg.cpu.numa)
 
     @mock.patch.object(fakelibvirt.Connection, 'getType')
@@ -3577,7 +3577,7 @@ def test_get_guest_config_numa_host_instance_fit_w_cpu_pinset(self):
             # NOTE(ndipanov): we make sure that pin_set was taken into account
             # when choosing viable cells
             self.assertEqual(set([2, 3]), cfg.cpuset)
-            self.assertEqual(0, len(cfg.cputune.vcpupin))
+            self.assertIsNone(cfg.cputune)
             self.assertIsNone(cfg.cpu.numa)
 
     def test_get_guest_config_non_numa_host_instance_topo(self):
@@ -3617,7 +3617,7 @@ def test_get_guest_config_non_numa_host_instance_topo(self):
             cfg = drvr._get_guest_config(instance_ref, [],
                                          image_meta, disk_info)
             self.assertIsNone(cfg.cpuset)
-            self.assertEqual(0, len(cfg.cputune.vcpupin))
+            self.assertIsNone(cfg.cputune)
             self.assertIsNone(cfg.numatune)
             self.assertIsNotNone(cfg.cpu.numa)
             for instance_cell, numa_cfg_cell in zip(
@@ -7020,25 +7020,9 @@ def test_get_guest_config_with_rng_dev_not_present(self, mock_path):
                           [],
                           image_meta, disk_info)
 
-    def test_guest_cpu_shares_with_multi_vcpu(self):
-        self.flags(virt_type='kvm', group='libvirt')
-
-        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
-
-        instance_ref = objects.Instance(**self.test_instance)
-        instance_ref.flavor.vcpus = 4
-        image_meta = objects.ImageMeta.from_dict(self.test_image_meta)
-
-        disk_info = blockinfo.get_disk_info(CONF.libvirt.virt_type,
-                                            instance_ref,
-                                            image_meta)
-
-        cfg = drvr._get_guest_config(instance_ref, [],
-                                     image_meta, disk_info)
-
-        self.assertEqual(4096, cfg.cputune.shares)
-
-    def test_get_guest_config_with_cpu_quota(self):
+    @mock.patch.object(
+        host.Host, "is_cpu_control_policy_capable", return_value=True)
+    def test_get_guest_config_with_cpu_quota(self, is_able):
         self.flags(virt_type='kvm', group='libvirt')
 
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
@@ -11608,7 +11592,7 @@ def test_live_migration_update_graphics_xml(self, mock_xml,
                                                       mock_migrateToURI3,
                                                       mock_min_version):
         self.compute = manager.ComputeManager()
-        instance_ref = self.test_instance
+        instance_ref = objects.Instance(**self.test_instance)
         target_connection = '127.0.0.2'
 
         xml_tmpl = ("<domain type='kvm'>"
@@ -12288,7 +12272,7 @@ def test_live_migration_update_serial_console_xml(self, mock_xml,
                                                       mock_get,
                                                       mock_min_version):
         self.compute = manager.ComputeManager()
-        instance_ref = self.test_instance
+        instance_ref = objects.Instance(**self.test_instance)
         target_connection = '127.0.0.2'
 
         xml_tmpl = ("<domain type='kvm'>"
@@ -12578,7 +12562,7 @@ def test_live_migration_raises_exception(self, mock_xml,
                                              mock_min_version):
         # Prepare data
         self.compute = manager.ComputeManager()
-        instance_ref = self.test_instance
+        instance_ref = objects.Instance(**self.test_instance)
         target_connection = '127.0.0.2'
 
         disk_paths = ['vda', 'vdb']

nova/tests/unit/virt/libvirt/test_migration.py

@@ -28,6 +28,7 @@
 from nova import test
 from nova.tests import fixtures as nova_fixtures
 from nova.tests.fixtures import libvirt as fakelibvirt
+from nova.tests.unit.virt.libvirt import test_driver
 from nova.virt.libvirt import config as vconfig
 from nova.virt.libvirt import guest as libvirt_guest
 from nova.virt.libvirt import host
@@ -80,16 +81,51 @@ def test_get_updated_guest_xml(
         get_volume_config = mock.MagicMock()
         mock_guest.get_xml_desc.return_value = '<domain></domain>'
 
-        migration.get_updated_guest_xml(
-            mock.sentinel.instance, mock_guest, data, get_volume_config)
+        instance = objects.Instance(**test_driver._create_test_instance())
+        migration.get_updated_guest_xml(instance, mock_guest, data,
+                                        get_volume_config)
         mock_graphics.assert_called_once_with(mock.ANY, data)
         mock_serial.assert_called_once_with(mock.ANY, data)
         mock_volume.assert_called_once_with(
-            mock.ANY, data, mock.sentinel.instance, get_volume_config)
+            mock.ANY, data, instance, get_volume_config)
         mock_perf_events_xml.assert_called_once_with(mock.ANY, data)
         mock_memory_backing.assert_called_once_with(mock.ANY, data)
         self.assertEqual(1, mock_tostring.called)
 
+    def test_update_quota_xml(self):
+        old_xml = """<domain>
+                         <name>fake-instance</name>
+                         <cputune>
+                             <shares>42</shares>
+                             <period>1337</period>
+                         </cputune>
+                     </domain>"""
+        instance = objects.Instance(**test_driver._create_test_instance())
+        new_xml = migration._update_quota_xml(instance,
+                                              etree.fromstring(old_xml))
+        new_xml = etree.tostring(new_xml, encoding='unicode')
+        self.assertXmlEqual(
+            """<domain>
+                   <name>fake-instance</name>
+                   <cputune>
+                       <period>1337</period>
+                   </cputune>
+               </domain>""", new_xml)
+
+    def test_update_quota_xml_empty_cputune(self):
+        old_xml = """<domain>
+                         <name>fake-instance</name>
+                         <cputune>
+                             <shares>42</shares>
+                         </cputune>
+                     </domain>"""
+        instance = objects.Instance(**test_driver._create_test_instance())
+        new_xml = migration._update_quota_xml(instance,
+                                              etree.fromstring(old_xml))
+        new_xml = etree.tostring(new_xml, encoding='unicode')
+        self.assertXmlEqual('<domain><name>fake-instance</name></domain>',
+                            new_xml)
+
     def test_update_device_resources_xml_vpmem(self):
         # original xml for vpmems, /dev/dax0.1 and /dev/dax0.2 here
         # are vpmem device path on source host

nova/virt/libvirt/driver.py

@@ -5682,15 +5682,11 @@ def _update_guest_cputune(self, guest, flavor):
         if not is_able or CONF.libvirt.virt_type not in ('lxc', 'kvm', 'qemu'):
             return
 
-        if guest.cputune is None:
-            guest.cputune = vconfig.LibvirtConfigGuestCPUTune()
-            # Setting the default cpu.shares value to be a value
-            # dependent on the number of vcpus
-        guest.cputune.shares = 1024 * guest.vcpus
-
         for name in cputuning:
             key = "quota:cpu_" + name
             if key in flavor.extra_specs:
+                if guest.cputune is None:
+                    guest.cputune = vconfig.LibvirtConfigGuestCPUTune()
                 setattr(guest.cputune, name,
                         int(flavor.extra_specs[key]))
 

nova/virt/libvirt/migration.py

@@ -62,6 +62,7 @@ def get_updated_guest_xml(instance, guest, migrate_data, get_volume_config,
         xml_doc, migrate_data, instance, get_volume_config)
     xml_doc = _update_perf_events_xml(xml_doc, migrate_data)
     xml_doc = _update_memory_backing_xml(xml_doc, migrate_data)
+    xml_doc = _update_quota_xml(instance, xml_doc)
     if get_vif_config is not None:
         xml_doc = _update_vif_xml(xml_doc, migrate_data, get_vif_config)
     if 'dst_numa_info' in migrate_data:
@@ -71,6 +72,18 @@ def get_updated_guest_xml(instance, guest, migrate_data, get_volume_config,
     return etree.tostring(xml_doc, encoding='unicode')
 
 
+def _update_quota_xml(instance, xml_doc):
+    flavor_shares = instance.flavor.extra_specs.get('quota:cpu_shares')
+    cputune = xml_doc.find('./cputune')
+    shares = xml_doc.find('./cputune/shares')
+    if shares is not None and not flavor_shares:
+        cputune.remove(shares)
+    # Remove the cputune element entirely if it has no children left.
+    if cputune is not None and not list(cputune):
+        xml_doc.remove(cputune)
+    return xml_doc
+
+
 def _update_device_resources_xml(xml_doc, new_resources):
     vpmems = []
     for resource in new_resources:

releasenotes/notes/remove-default-cputune-shares-values-85d5ddf4b8e24eaa.yaml

@@ -0,0 +1,15 @@
+upgrade:
+  - |
+    In the libvirt driver, the default value of the ``<cputune><shares>``
+    element has been removed, and is now left to libvirt to decide. This is
+    because allowed values are platform dependant, and the previous code was
+    not guaranteed to be supported on all platforms. If any of your flavors are
+    using the quota:cpu_shares extra spec, you may need to resize to a
+    supported value before upgrading.
+
+    To facilitate the transition to no Nova default for ``<cputune><shares>``,
+    its value will be removed during live migration unless a value is set in
+    the ``quota:cpu_shares`` extra spec. This can cause temporary CPU
+    starvation for the live migrated instance if other instances on the
+    destination host still have the old default ``<cputune><shares>`` value. To
+    fix this, hard reboot, cold migrate, or live migrate the other instances.