From 0b9d8cee580739e7a4e2a769e4d4e35110a6bc49 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Thu, 12 Oct 2023 12:12:53 -0700 Subject: [PATCH] Do not manage CPU0's state If cpu_power_management_strategy is "cpu_state" and CPU0 is in the dedicated set, we should just ignore it whenever we go to manage the state. Since CPU0 cannot be powered off, but may be otherwise suitable for the dedicated set, we can just skip it whenever we would normally go to power it up or down. Change-Id: I995c0953b361c7016bd77482fa2e2f276d239828 Fixes-Bug: #2038840 --- nova/tests/unit/virt/libvirt/cpu/test_api.py | 75 ++++++++++++++------ nova/tests/unit/virt/test_hardware.py | 5 ++ nova/virt/hardware.py | 5 ++ nova/virt/libvirt/cpu/api.py | 11 ++- 4 files changed, 73 insertions(+), 23 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/cpu/test_api.py b/nova/tests/unit/virt/libvirt/cpu/test_api.py index 79664d7f8e47..5802dec5af9c 100644 --- a/nova/tests/unit/virt/libvirt/cpu/test_api.py +++ b/nova/tests/unit/virt/libvirt/cpu/test_api.py @@ -80,23 +80,23 @@ class TestAPI(test.NoDBTestCase): @mock.patch.object(core, 'set_online') def test_power_up_online(self, mock_online): self.flags(cpu_power_management=True, group='libvirt') - self.flags(cpu_dedicated_set='0-1', group='compute') + self.flags(cpu_dedicated_set='1-2', group='compute') api.power_up(self.fake_inst) - # only core #0 can be set as core #2 is not on the dedicated set + # 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(0) + mock_online.assert_called_once_with(2) @mock.patch.object(core, 'set_governor') def test_power_up_governor(self, mock_set_governor): self.flags(cpu_power_management=True, group='libvirt') self.flags(cpu_power_management_strategy='governor', group='libvirt') - self.flags(cpu_dedicated_set='0-1', group='compute') + self.flags(cpu_dedicated_set='1-2', group='compute') api.power_up(self.fake_inst) - # only core #0 can be set as core #2 is not on the dedicated set + # 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(0, 'performance') + mock_set_governor.assert_called_once_with(2, 'performance') @mock.patch.object(core, 'set_online') def test_power_up_skipped(self, mock_online): @@ -113,23 +113,36 @@ class TestAPI(test.NoDBTestCase): @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='0-1', group='compute') + self.flags(cpu_dedicated_set='1-2', group='compute') api.power_down(self.fake_inst) - # only core #0 can be set as core #2 is not on the dedicated set + # 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(0) + mock_offline.assert_called_once_with(2) @mock.patch.object(core, 'set_governor') - def test_power_down_governor(self, mock_set_governor): + def test_power_down_governor_cpu0_ignored(self, mock_set_governor): self.flags(cpu_power_management=True, group='libvirt') self.flags(cpu_power_management_strategy='governor', group='libvirt') self.flags(cpu_dedicated_set='0-1', group='compute') api.power_down(self.fake_inst) - # only core #0 can be set as core #2 is not on the dedicated set + + # Make sure that core #0 is ignored, since it is special and cannot + # be powered down. + mock_set_governor.assert_not_called() + + @mock.patch.object(core, 'set_governor') + def test_power_down_governor(self, mock_set_governor): + self.flags(cpu_power_management=True, group='libvirt') + self.flags(cpu_power_management_strategy='governor', group='libvirt') + self.flags(cpu_dedicated_set='1-2', group='compute') + + 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) - mock_set_governor.assert_called_once_with(0, 'powersave') + mock_set_governor.assert_called_once_with(2, 'powersave') @mock.patch.object(core, 'set_offline') def test_power_down_skipped(self, mock_offline): @@ -146,22 +159,22 @@ class TestAPI(test.NoDBTestCase): @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-1', group='compute') + self.flags(cpu_dedicated_set='0-2', group='compute') api.power_down_all_dedicated_cpus() - # All dedicated CPUs are turned offline - mock_offline.assert_has_calls([mock.call(0), mock.call(1)]) + # All dedicated CPUs are turned offline, except CPU0 + mock_offline.assert_has_calls([mock.call(1), mock.call(2)]) @mock.patch.object(core, 'set_governor') def test_power_down_all_dedicated_cpus_governor(self, mock_set_governor): self.flags(cpu_power_management=True, group='libvirt') self.flags(cpu_power_management_strategy='governor', group='libvirt') - self.flags(cpu_dedicated_set='0-1', group='compute') + self.flags(cpu_dedicated_set='0-2', group='compute') api.power_down_all_dedicated_cpus() - # All dedicated CPUs are turned offline - mock_set_governor.assert_has_calls([mock.call(0, 'powersave'), - mock.call(1, 'powersave')]) + # 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): @@ -192,9 +205,31 @@ class TestAPI(test.NoDBTestCase): def test_validate_all_dedicated_cpus_for_cpu_state(self, mock_get_online, mock_get_governor): self.flags(cpu_power_management=True, group='libvirt') - self.flags(cpu_dedicated_set='0-1', group='compute') + self.flags(cpu_dedicated_set='1-2', group='compute') self.flags(cpu_power_management_strategy='cpu_state', group='libvirt') mock_get_online.return_value = True mock_get_governor.side_effect = ('powersave', 'performance') self.assertRaises(exception.InvalidConfiguration, api.validate_all_dedicated_cpus) + + @mock.patch.object(core, 'get_governor') + @mock.patch.object(core, 'get_online') + @mock.patch.object(api.LOG, 'warning') + def test_validate_all_dedicated_cpus_for_cpu_state_warning( + self, mock_warning, mock_get_online, mock_get_governor): + self.flags(cpu_power_management=True, group='libvirt') + self.flags(cpu_dedicated_set='0-2', group='compute') + self.flags(cpu_power_management_strategy='cpu_state', group='libvirt') + + mock_get_online.return_value = True + mock_get_governor.return_value = 'performance' + + api.validate_all_dedicated_cpus() + + # Make sure we skipped CPU0 + mock_get_online.assert_has_calls([mock.call(1), mock.call(2)]) + + # Make sure we logged a warning about CPU0 + mock_warning.assert_called_once_with( + 'CPU0 is in cpu_dedicated_set, but it is not eligible for ' + 'state management and will be ignored') diff --git a/nova/tests/unit/virt/test_hardware.py b/nova/tests/unit/virt/test_hardware.py index ab51a3e26ca5..381213f9720d 100644 --- a/nova/tests/unit/virt/test_hardware.py +++ b/nova/tests/unit/virt/test_hardware.py @@ -90,6 +90,11 @@ class CPUSetTestCase(test.NoDBTestCase): cpuset_ids = hw.get_cpu_dedicated_set() self.assertEqual(set([0, 1, 3, 4, 5, 6]), cpuset_ids) + def test_get_cpu_dedicated_set_nozero(self): + self.flags(cpu_dedicated_set='0-5,6,^2', group='compute') + cpuset_ids = hw.get_cpu_dedicated_set_nozero() + self.assertEqual(set([1, 3, 4, 5, 6]), cpuset_ids) + def test_get_cpu_dedicated_set__unset(self): self.flags(cpu_dedicated_set=None, group='compute') cpuset_ids = hw.get_cpu_dedicated_set() diff --git a/nova/virt/hardware.py b/nova/virt/hardware.py index 9693e405d3a7..e3397e23cd08 100644 --- a/nova/virt/hardware.py +++ b/nova/virt/hardware.py @@ -76,6 +76,11 @@ def get_cpu_dedicated_set(): return cpu_ids +def get_cpu_dedicated_set_nozero(): + """Return cpu_dedicated_set without CPU0, if present""" + return get_cpu_dedicated_set() - {0} + + def get_cpu_shared_set(): """Parse ``[compute] cpu_shared_set`` config. diff --git a/nova/virt/libvirt/cpu/api.py b/nova/virt/libvirt/cpu/api.py index 51ab26dce259..a1f5f7a9a018 100644 --- a/nova/virt/libvirt/cpu/api.py +++ b/nova/virt/libvirt/cpu/api.py @@ -81,7 +81,7 @@ def power_up(instance: objects.Instance) -> None: if instance.numa_topology is None: return - cpu_dedicated_set = hardware.get_cpu_dedicated_set() or set() + cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set() pcpus = instance.numa_topology.cpu_pinning powered_up = set() for pcpu in pcpus: @@ -101,7 +101,7 @@ def power_down(instance: objects.Instance) -> None: if instance.numa_topology is None: return - cpu_dedicated_set = hardware.get_cpu_dedicated_set() or set() + cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set() pcpus = instance.numa_topology.cpu_pinning powered_down = set() for pcpu in pcpus: @@ -127,7 +127,7 @@ def power_down_all_dedicated_cpus() -> None: "power management if you only use shared CPUs.") raise exception.InvalidConfiguration(msg) - cpu_dedicated_set = hardware.get_cpu_dedicated_set() or set() + cpu_dedicated_set = hardware.get_cpu_dedicated_set_nozero() or set() for pcpu in cpu_dedicated_set: pcpu = Core(pcpu) if CONF.libvirt.cpu_power_management_strategy == 'cpu_state': @@ -144,6 +144,11 @@ def validate_all_dedicated_cpus() -> None: 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)