Merge "pwr mgmt: make API into a per-driver object" into stable/2023.1

Author: Zuul

nova/tests/unit/virt/libvirt/cpu/test_api.py

@@ -24,6 +24,7 @@ class TestAPI(test.NoDBTestCase):
     def setUp(self):
         super(TestAPI, self).setUp()
         self.core_1 = api.Core(1)
+        self.api = api.API()
 
         # Create a fake instance with two pinned CPUs but only one is on the
         # dedicated set
@@ -82,7 +83,7 @@ def test_power_up_online(self, mock_online):
         self.flags(cpu_power_management=True, group='libvirt')
         self.flags(cpu_dedicated_set='1-2', group='compute')
 
-        api.power_up(self.fake_inst)
+        self.api.power_up(self.fake_inst)
         # only core #2 can be set as core #0 is not on the dedicated set
         # As a reminder, core(i).online calls set_online(i)
         mock_online.assert_called_once_with(2)
@@ -93,29 +94,29 @@ def test_power_up_governor(self, mock_set_governor):
         self.flags(cpu_power_management_strategy='governor', group='libvirt')
         self.flags(cpu_dedicated_set='1-2', group='compute')
 
-        api.power_up(self.fake_inst)
+        self.api.power_up(self.fake_inst)
         # only core #2 can be set as core #1 is not on the dedicated set
         # As a reminder, core(i).set_high_governor calls set_governor(i)
         mock_set_governor.assert_called_once_with(2, 'performance')
 
     @mock.patch.object(core, 'set_online')
     def test_power_up_skipped(self, mock_online):
         self.flags(cpu_power_management=False, group='libvirt')
-        api.power_up(self.fake_inst)
+        self.api.power_up(self.fake_inst)
         mock_online.assert_not_called()
 
     @mock.patch.object(core, 'set_online')
     def test_power_up_skipped_if_standard_instance(self, mock_online):
         self.flags(cpu_power_management=True, group='libvirt')
-        api.power_up(objects.Instance(numa_topology=None))
+        self.api.power_up(objects.Instance(numa_topology=None))
         mock_online.assert_not_called()
 
     @mock.patch.object(core, 'set_offline')
     def test_power_down_offline(self, mock_offline):
         self.flags(cpu_power_management=True, group='libvirt')
         self.flags(cpu_dedicated_set='1-2', group='compute')
 
-        api.power_down(self.fake_inst)
+        self.api.power_down(self.fake_inst)
         # only core #2 can be set as core #1 is not on the dedicated set
         # As a reminder, core(i).online calls set_online(i)
         mock_offline.assert_called_once_with(2)
@@ -126,7 +127,7 @@ def test_power_down_governor_cpu0_ignored(self, mock_set_governor):
         self.flags(cpu_power_management_strategy='governor', group='libvirt')
         self.flags(cpu_dedicated_set='0-1', group='compute')
 
-        api.power_down(self.fake_inst)
+        self.api.power_down(self.fake_inst)
 
         # Make sure that core #0 is ignored, since it is special and cannot
         # be powered down.
@@ -138,7 +139,7 @@ def test_power_down_governor(self, mock_set_governor):
         self.flags(cpu_power_management_strategy='governor', group='libvirt')
         self.flags(cpu_dedicated_set='1-2', group='compute')
 
-        api.power_down(self.fake_inst)
+        self.api.power_down(self.fake_inst)
 
         # only core #2 can be set as core #0 is not on the dedicated set
         # As a reminder, core(i).set_high_governor calls set_governor(i)
@@ -147,21 +148,21 @@ def test_power_down_governor(self, mock_set_governor):
     @mock.patch.object(core, 'set_offline')
     def test_power_down_skipped(self, mock_offline):
         self.flags(cpu_power_management=False, group='libvirt')
-        api.power_down(self.fake_inst)
+        self.api.power_down(self.fake_inst)
         mock_offline.assert_not_called()
 
     @mock.patch.object(core, 'set_offline')
     def test_power_down_skipped_if_standard_instance(self, mock_offline):
         self.flags(cpu_power_management=True, group='libvirt')
-        api.power_down(objects.Instance(numa_topology=None))
+        self.api.power_down(objects.Instance(numa_topology=None))
         mock_offline.assert_not_called()
 
     @mock.patch.object(core, 'set_offline')
     def test_power_down_all_dedicated_cpus_offline(self, mock_offline):
         self.flags(cpu_power_management=True, group='libvirt')
         self.flags(cpu_dedicated_set='0-2', group='compute')
 
-        api.power_down_all_dedicated_cpus()
+        self.api.power_down_all_dedicated_cpus()
         # All dedicated CPUs are turned offline, except CPU0
         mock_offline.assert_has_calls([mock.call(1), mock.call(2)])
 
@@ -171,15 +172,15 @@ def test_power_down_all_dedicated_cpus_governor(self, mock_set_governor):
         self.flags(cpu_power_management_strategy='governor', group='libvirt')
         self.flags(cpu_dedicated_set='0-2', group='compute')
 
-        api.power_down_all_dedicated_cpus()
+        self.api.power_down_all_dedicated_cpus()
         # All dedicated CPUs are turned offline, except CPU0
         mock_set_governor.assert_has_calls([mock.call(1, 'powersave'),
                                             mock.call(2, 'powersave')])
 
     @mock.patch.object(core, 'set_offline')
     def test_power_down_all_dedicated_cpus_skipped(self, mock_offline):
         self.flags(cpu_power_management=False, group='libvirt')
-        api.power_down_all_dedicated_cpus()
+        self.api.power_down_all_dedicated_cpus()
         mock_offline.assert_not_called()
 
     @mock.patch.object(core, 'set_offline')
@@ -188,7 +189,7 @@ def test_power_down_all_dedicated_cpus_no_dedicated_cpus_configured(
     ):
         self.flags(cpu_power_management=True, group='libvirt')
         self.flags(cpu_dedicated_set=None, group='compute')
-        api.power_down_all_dedicated_cpus()
+        self.api.power_down_all_dedicated_cpus()
         mock_offline.assert_not_called()
 
     @mock.patch.object(core, 'get_governor')
@@ -201,7 +202,7 @@ def test_validate_all_dedicated_cpus_for_governor(self, mock_get_online,
         mock_get_governor.return_value = 'performance'
         mock_get_online.side_effect = (True, False)
         self.assertRaises(exception.InvalidConfiguration,
-                          api.validate_all_dedicated_cpus)
+                          self.api.validate_all_dedicated_cpus)
 
     @mock.patch.object(core, 'get_governor')
     @mock.patch.object(core, 'get_online')
@@ -213,7 +214,7 @@ def test_validate_all_dedicated_cpus_for_cpu_state(self, mock_get_online,
         mock_get_online.return_value = True
         mock_get_governor.side_effect = ('powersave', 'performance')
         self.assertRaises(exception.InvalidConfiguration,
-                          api.validate_all_dedicated_cpus)
+                          self.api.validate_all_dedicated_cpus)
 
     @mock.patch.object(core, 'get_governor')
     @mock.patch.object(core, 'get_online')
@@ -227,7 +228,7 @@ def test_validate_all_dedicated_cpus_for_cpu_state_warning(
         mock_get_online.return_value = True
         mock_get_governor.return_value = 'performance'
 
-        api.validate_all_dedicated_cpus()
+        self.api.validate_all_dedicated_cpus()
 
         # Make sure we skipped CPU0
         mock_get_online.assert_has_calls([mock.call(1), mock.call(2)])
@@ -240,6 +241,6 @@ def test_validate_all_dedicated_cpus_for_cpu_state_warning(
     def test_validate_all_dedicated_cpus_no_cpu(self):
         self.flags(cpu_power_management=True, group='libvirt')
         self.flags(cpu_dedicated_set=None, group='compute')
-        api.validate_all_dedicated_cpus()
+        self.api.validate_all_dedicated_cpus()
         # no assert we want to make sure the validation won't raise if
         # no dedicated cpus are configured

nova/virt/libvirt/cpu/api.py

@@ -75,88 +75,102 @@ def set_low_governor(self) -> None:
         core.set_governor(self.ident, CONF.libvirt.cpu_power_governor_low)
 
 
-def power_up(instance: objects.Instance) -> None:
-    if not CONF.libvirt.cpu_power_management:
-        return
-    if instance.numa_topology is None:
-        return
-
-    cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set()
-    pcpus = instance.numa_topology.cpu_pinning.union(
-        instance.numa_topology.cpuset_reserved)
-    powered_up = set()
-    for pcpu in pcpus:
-        if pcpu in cpu_dedicated_set:
-            pcpu = Core(pcpu)
-            if CONF.libvirt.cpu_power_management_strategy == 'cpu_state':
-                pcpu.online = True
-            else:
-                pcpu.set_high_governor()
-            powered_up.add(str(pcpu))
-    LOG.debug("Cores powered up : %s", powered_up)
-
-
-def power_down(instance: objects.Instance) -> None:
-    if not CONF.libvirt.cpu_power_management:
-        return
-    if instance.numa_topology is None:
-        return
-
-    cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set()
-    pcpus = instance.numa_topology.cpu_pinning.union(
-        instance.numa_topology.cpuset_reserved)
-    powered_down = set()
-    for pcpu in pcpus:
-        if pcpu in cpu_dedicated_set:
-            pcpu = Core(pcpu)
+class API(object):
+
+    def core(self, i):
+        """From a purely functional point of view, there is no need for this
+        method. However, we want to test power management in multinode
+        scenarios (ex: live migration) in our functional tests. If we
+        instantiated the Core class directly in the methods below, the
+        functional tests would not be able to distinguish between cores on the
+        source and destination hosts. In functional tests we can replace this
+        helper method by a stub that returns a fixture, allowing us to maintain
+        distinct core power state for each host.
+
+        See also nova.virt.libvirt.driver.LibvirtDriver.cpu_api.
+        """
+        return Core(i)
+
+    def power_up(self, instance: objects.Instance) -> None:
+        if not CONF.libvirt.cpu_power_management:
+            return
+        if instance.numa_topology is None:
+            return
+
+        cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set()
+        pcpus = instance.numa_topology.cpu_pinning.union(
+            instance.numa_topology.cpuset_reserved)
+        powered_up = set()
+        for pcpu in pcpus:
+            if pcpu in cpu_dedicated_set:
+                pcpu = self.core(pcpu)
+                if CONF.libvirt.cpu_power_management_strategy == 'cpu_state':
+                    pcpu.online = True
+                else:
+                    pcpu.set_high_governor()
+                powered_up.add(str(pcpu))
+        LOG.debug("Cores powered up : %s", powered_up)
+
+    def power_down(self, instance: objects.Instance) -> None:
+        if not CONF.libvirt.cpu_power_management:
+            return
+        if instance.numa_topology is None:
+            return
+
+        cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set()
+        pcpus = instance.numa_topology.cpu_pinning.union(
+            instance.numa_topology.cpuset_reserved)
+        powered_down = set()
+        for pcpu in pcpus:
+            if pcpu in cpu_dedicated_set:
+                pcpu = self.core(pcpu)
+                if CONF.libvirt.cpu_power_management_strategy == 'cpu_state':
+                    pcpu.online = False
+                else:
+                    pcpu.set_low_governor()
+                powered_down.add(str(pcpu))
+        LOG.debug("Cores powered down : %s", powered_down)
+
+    def power_down_all_dedicated_cpus(self) -> None:
+        if not CONF.libvirt.cpu_power_management:
+            return
+
+        cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set()
+        for pcpu in cpu_dedicated_set:
+            pcpu = self.core(pcpu)
             if CONF.libvirt.cpu_power_management_strategy == 'cpu_state':
                 pcpu.online = False
             else:
                 pcpu.set_low_governor()
-            powered_down.add(str(pcpu))
-    LOG.debug("Cores powered down : %s", powered_down)
-
-
-def power_down_all_dedicated_cpus() -> None:
-    if not CONF.libvirt.cpu_power_management:
-        return
-
-    cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set()
-    for pcpu in cpu_dedicated_set:
-        pcpu = Core(pcpu)
+        LOG.debug("Cores powered down : %s", cpu_dedicated_set)
+
+    def validate_all_dedicated_cpus(self) -> None:
+        if not CONF.libvirt.cpu_power_management:
+            return
+        cpu_dedicated_set = hardware.get_cpu_dedicated_set() or set()
+        governors = set()
+        cpu_states = set()
+        for pcpu in cpu_dedicated_set:
+            if (pcpu == 0 and
+                    CONF.libvirt.cpu_power_management_strategy == 'cpu_state'):
+                LOG.warning('CPU0 is in cpu_dedicated_set, '
+                            'but it is not eligible for state management '
+                            'and will be ignored')
+                continue
+            pcpu = self.core(pcpu)
+            # we need to collect the governors strategy and the CPU states
+            governors.add(pcpu.governor)
+            cpu_states.add(pcpu.online)
         if CONF.libvirt.cpu_power_management_strategy == 'cpu_state':
-            pcpu.online = False
-        else:
-            pcpu.set_low_governor()
-    LOG.debug("Cores powered down : %s", cpu_dedicated_set)
-
-
-def validate_all_dedicated_cpus() -> None:
-    if not CONF.libvirt.cpu_power_management:
-        return
-    cpu_dedicated_set = hardware.get_cpu_dedicated_set() or set()
-    governors = set()
-    cpu_states = set()
-    for pcpu in cpu_dedicated_set:
-        if (pcpu == 0 and
-                CONF.libvirt.cpu_power_management_strategy == 'cpu_state'):
-            LOG.warning('CPU0 is in cpu_dedicated_set, but it is not eligible '
-                        'for state management and will be ignored')
-            continue
-        pcpu = Core(pcpu)
-        # we need to collect the governors strategy and the CPU states
-        governors.add(pcpu.governor)
-        cpu_states.add(pcpu.online)
-    if CONF.libvirt.cpu_power_management_strategy == 'cpu_state':
-        # all the cores need to have the same governor strategy
-        if len(governors) > 1:
-            msg = _("All the cores need to have the same governor strategy"
-                    "before modifying the CPU states. You can reboot the "
-                    "compute node if you prefer.")
-            raise exception.InvalidConfiguration(msg)
-    elif CONF.libvirt.cpu_power_management_strategy == 'governor':
-        # all the cores need to be online
-        if False in cpu_states:
-            msg = _("All the cores need to be online before modifying the "
-                    "governor strategy.")
-            raise exception.InvalidConfiguration(msg)
+            # all the cores need to have the same governor strategy
+            if len(governors) > 1:
+                msg = _("All the cores need to have the same governor strategy"
+                        "before modifying the CPU states. You can reboot the "
+                        "compute node if you prefer.")
+                raise exception.InvalidConfiguration(msg)
+        elif CONF.libvirt.cpu_power_management_strategy == 'governor':
+            # all the cores need to be online
+            if False in cpu_states:
+                msg = _("All the cores need to be online before modifying the "
+                        "governor strategy.")
+                raise exception.InvalidConfiguration(msg)

nova/virt/libvirt/driver.py

@@ -545,6 +545,15 @@ def __init__(self, virtapi, read_only=False):
         # events about success or failure.
         self._device_event_handler = AsyncDeviceEventsHandler()
 
+        # NOTE(artom) From a pure functionality point of view, there's no need
+        # for this to be an attribute of self. However, we want to test power
+        # management in multinode scenarios (ex: live migration) in our
+        # functional tests. If the power management code was just a bunch of
+        # module level functions, the functional tests would not be able to
+        # distinguish between cores on the source and destination hosts.
+        # See also nova.virt.libvirt.cpu.api.API.core().
+        self.cpu_api = libvirt_cpu.API()
+
     def _discover_vpmems(self, vpmem_conf=None):
         """Discover vpmems on host and configuration.
 
@@ -822,13 +831,13 @@ def init_host(self, host):
         # modified by Nova before. Note that it can provide an exception if
         # either the governor strategies are different between the cores or if
         # the cores are offline.
-        libvirt_cpu.validate_all_dedicated_cpus()
+        self.cpu_api.validate_all_dedicated_cpus()
         # NOTE(sbauza): We powerdown all dedicated CPUs but if some instances
         # exist that are pinned for some CPUs, then we'll later powerup those
         # CPUs when rebooting the instance in _init_instance()
         # Note that it can provide an exception if the config options are
         # wrongly modified.
-        libvirt_cpu.power_down_all_dedicated_cpus()
+        self.cpu_api.power_down_all_dedicated_cpus()
 
         # TODO(sbauza): Remove this code once mediated devices are persisted
         # across reboots.
@@ -1526,7 +1535,7 @@ def _wait_for_destroy(expected_domid):
             if CONF.libvirt.virt_type == 'lxc':
                 self._teardown_container(instance)
             # We're sure the instance is gone, we can shutdown the core if so
-            libvirt_cpu.power_down(instance)
+            self.cpu_api.power_down(instance)
 
     def destroy(self, context, instance, network_info, block_device_info=None,
                 destroy_disks=True, destroy_secrets=True):
@@ -3180,7 +3189,7 @@ def _resume_guest_after_snapshot(
 
         current_power_state = guest.get_power_state(self._host)
 
-        libvirt_cpu.power_up(instance)
+        self.cpu_api.power_up(instance)
         # TODO(stephenfin): Any reason we couldn't use 'self.resume' here?
         guest.launch(pause=current_power_state == power_state.PAUSED)
 
@@ -7661,7 +7670,7 @@ def _create_guest(
                 post_xml_callback()
 
             if power_on or pause:
-                libvirt_cpu.power_up(instance)
+                self.cpu_api.power_up(instance)
                 guest.launch(pause=pause)
 
             return guest