From ae2e5650d14a2c81dd397727d67b60f9b8dd0dd7 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 16 Oct 2018 17:41:17 +0100 Subject: [PATCH] Fail to live migration if instance has a NUMA topology Live migration is currently totally broken if a NUMA topology is present. This affects everything that's been regrettably stuffed in with NUMA topology including CPU pinning, hugepage support and emulator thread support. Side effects can range from simple unexpected performance hits (due to instances running on the same cores) to complete failures (due to instance cores or huge pages being mapped to CPUs/NUMA nodes that don't exist on the destination host). Until such a time as we resolve these issues, we should alert users to the fact that such issues exist. A workaround option is provided for operators that _really_ need the broken behavior, but it's defaulted to False to highlight the brokenness of this feature to unsuspecting operators. Change-Id: I217fba9138132b107e9d62895d699d238392e761 Signed-off-by: Stephen Finucane Related-bug: #1289064 --- doc/source/admin/adv-config.rst | 2 + doc/source/admin/configuring-migrations.rst | 3 +- doc/source/admin/cpu-topologies.rst | 2 + .../common/numa-live-migration-warning.txt | 10 +++++ doc/source/user/feature-classification.rst | 2 + nova/api/openstack/compute/migrate_server.py | 6 ++- nova/conductor/tasks/live_migrate.py | 26 +++++++++++ nova/conf/workarounds.py | 28 ++++++++++++ .../compute/admin_only_action_common.py | 36 ++++++++++++--- .../openstack/compute/test_migrate_server.py | 8 ++-- .../unit/conductor/tasks/test_live_migrate.py | 44 ++++++++++++++++++- ...-migration-with-numa-bc710a1bcde25957.yaml | 25 +++++++++++ 12 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 doc/source/common/numa-live-migration-warning.txt create mode 100644 releasenotes/notes/disable-live-migration-with-numa-bc710a1bcde25957.yaml diff --git a/doc/source/admin/adv-config.rst b/doc/source/admin/adv-config.rst index 82e6be6b32d1..225b13366372 100644 --- a/doc/source/admin/adv-config.rst +++ b/doc/source/admin/adv-config.rst @@ -20,6 +20,8 @@ vital. In these cases, instances can be expected to deliver near-native performance. The Compute service provides features to improve individual instance for these kind of workloads. +.. include:: /common/numa-live-migration-warning.txt + .. toctree:: :maxdepth: 2 diff --git a/doc/source/admin/configuring-migrations.rst b/doc/source/admin/configuring-migrations.rst index dc6a9a9832e5..d36e0f23bad3 100644 --- a/doc/source/admin/configuring-migrations.rst +++ b/doc/source/admin/configuring-migrations.rst @@ -19,7 +19,8 @@ This document covers live migrations using the .. note:: Not all Compute service hypervisor drivers support live-migration, or - support all live-migration features. + support all live-migration features. Similarly not all compute service + features are supported. Consult :doc:`/user/support-matrix` to determine which hypervisors support live-migration. diff --git a/doc/source/admin/cpu-topologies.rst b/doc/source/admin/cpu-topologies.rst index 5c9174e1c33e..8edb0f565fd4 100644 --- a/doc/source/admin/cpu-topologies.rst +++ b/doc/source/admin/cpu-topologies.rst @@ -7,6 +7,8 @@ control over how instances run on hypervisor CPUs and the topology of virtual CPUs available to instances. These features help minimize latency and maximize performance. +.. include:: /common/numa-live-migration-warning.txt + SMP, NUMA, and SMT ~~~~~~~~~~~~~~~~~~ diff --git a/doc/source/common/numa-live-migration-warning.txt b/doc/source/common/numa-live-migration-warning.txt new file mode 100644 index 000000000000..7b695f218830 --- /dev/null +++ b/doc/source/common/numa-live-migration-warning.txt @@ -0,0 +1,10 @@ +.. important:: + + Unless :oslo.config:option:`specifically enabled + `, live migration is not currently + possible for instances with a NUMA topology when using the libvirt driver. + A NUMA topology may be specified explicitly or can be added implicitly due + to the use of CPU pinning or huge pages. Refer to `bug #1289064`__ for more + information. + + __ https://bugs.launchpad.net/nova/+bug/1289064 diff --git a/doc/source/user/feature-classification.rst b/doc/source/user/feature-classification.rst index ceeafe572f5d..db0ce886d973 100644 --- a/doc/source/user/feature-classification.rst +++ b/doc/source/user/feature-classification.rst @@ -65,6 +65,8 @@ create a particular service. It is common for this workloads needing bare metal like performance, i.e. low latency and close to line speed performance. +.. include:: /common/numa-live-migration-warning.txt + .. feature_matrix:: feature-matrix-nfv.ini .. _matrix-hpc: diff --git a/nova/api/openstack/compute/migrate_server.py b/nova/api/openstack/compute/migrate_server.py index a3a82f0b6999..8ee66c67d519 100644 --- a/nova/api/openstack/compute/migrate_server.py +++ b/nova/api/openstack/compute/migrate_server.py @@ -101,7 +101,11 @@ class MigrateServerController(wsgi.Controller): disk_over_commit = strutils.bool_from_string(disk_over_commit, strict=True) - instance = common.get_instance(self.compute_api, context, id) + # NOTE(stephenfin): we need 'numa_topology' because of the + # 'LiveMigrationTask._check_instance_has_no_numa' check in the + # conductor + instance = common.get_instance(self.compute_api, context, id, + expected_attrs=['numa_topology']) try: self.compute_api.live_migrate(context, instance, block_migration, disk_over_commit, host, force, diff --git a/nova/conductor/tasks/live_migrate.py b/nova/conductor/tasks/live_migrate.py index 3ed3f279f1fa..9836769d06e1 100644 --- a/nova/conductor/tasks/live_migrate.py +++ b/nova/conductor/tasks/live_migrate.py @@ -23,6 +23,7 @@ from nova import exception from nova.i18n import _ from nova import network from nova import objects +from nova.objects import fields as obj_fields from nova.scheduler import utils as scheduler_utils from nova import utils @@ -65,6 +66,7 @@ class LiveMigrationTask(base.TaskBase): def _execute(self): self._check_instance_is_active() + self._check_instance_has_no_numa() self._check_host_is_up(self.source) self._source_cn, self._held_allocations = ( @@ -151,6 +153,30 @@ class LiveMigrationTask(base.TaskBase): state=self.instance.power_state, method='live migrate') + def _check_instance_has_no_numa(self): + """Prevent live migrations of instances with NUMA topologies.""" + if not self.instance.numa_topology: + return + + # Only KVM (libvirt) supports NUMA topologies with CPU pinning; + # HyperV's vNUMA feature doesn't allow specific pinning + hypervisor_type = objects.ComputeNode.get_by_host_and_nodename( + self.context, self.source, self.instance.node).hypervisor_type + if hypervisor_type != obj_fields.HVType.KVM: + return + + msg = ('Instance has an associated NUMA topology. ' + 'Instance NUMA topologies, including related attributes ' + 'such as CPU pinning, huge page and emulator thread ' + 'pinning information, are not currently recalculated on ' + 'live migration. See bug #1289064 for more information.' + ) + + if CONF.workarounds.enable_numa_live_migration: + LOG.warning(msg, instance=self.instance) + else: + raise exception.MigrationPreCheckError(reason=msg) + def _check_host_is_up(self, host): service = objects.Service.get_by_compute_host(self.context, host) diff --git a/nova/conf/workarounds.py b/nova/conf/workarounds.py index e48f7d50ad0d..367248508218 100644 --- a/nova/conf/workarounds.py +++ b/nova/conf/workarounds.py @@ -184,6 +184,34 @@ deploying the new code during an upgrade. Related options: * ``[consoleauth]/token_ttl`` +"""), + + cfg.BoolOpt( + 'enable_numa_live_migration', + default=False, + help=""" +Enable live migration of instances with NUMA topologies. + +Live migration of instances with NUMA topologies is disabled by default +when using the libvirt driver. This includes live migration of instances with +CPU pinning or hugepages. CPU pinning and huge page information for such +instances is not currently re-calculated, as noted in `bug #1289064`_. This +means that if instances were already present on the destination host, the +migrated instance could be placed on the same dedicated cores as these +instances or use hugepages allocated for another instance. Alternately, if the +host platforms were not homogeneous, the instance could be assigned to +non-existent cores or be inadvertently split across host NUMA nodes. + +Despite these known issues, there may be cases where live migration is +necessary. By enabling this option, operators that are aware of the issues and +are willing to manually work around them can enable live migration support for +these instances. + +Related options: + +* ``compute_driver``: Only the libvirt driver is affected. + +.. _bug #1289064: https://bugs.launchpad.net/nova/+bug/1289064 """), ] diff --git a/nova/tests/unit/api/openstack/compute/admin_only_action_common.py b/nova/tests/unit/api/openstack/compute/admin_only_action_common.py index 95c23cc64a12..fdda030cfd5e 100644 --- a/nova/tests/unit/api/openstack/compute/admin_only_action_common.py +++ b/nova/tests/unit/api/openstack/compute/admin_only_action_common.py @@ -48,6 +48,10 @@ class CommonMixin(object): # Reset the mock. self.mock_get.reset_mock() + expected_attrs = None + if action == '_migrate_live': + expected_attrs = ['numa_topology'] + uuid = uuidutils.generate_uuid() self.mock_get.side_effect = exception.InstanceNotFound( instance_id=uuid) @@ -57,7 +61,7 @@ class CommonMixin(object): controller_function, self.req, uuid, body=body_map) self.mock_get.assert_called_once_with(self.context, uuid, - expected_attrs=None, + expected_attrs=expected_attrs, cell_down_support=False) def _test_action(self, action, body=None, method=None, @@ -65,6 +69,10 @@ class CommonMixin(object): # Reset the mock. self.mock_get.reset_mock() + expected_attrs = None + if action == '_migrate_live': + expected_attrs = ['numa_topology'] + if method is None: method = action.replace('_', '') compute_api_args_map = compute_api_args_map or {} @@ -85,13 +93,17 @@ class CommonMixin(object): mock_method.assert_called_once_with(self.context, instance, *args, **kwargs) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=expected_attrs, cell_down_support=False) def _test_not_implemented_state(self, action, method=None): # Reset the mock. self.mock_get.reset_mock() + expected_attrs = None + if action == '_migrate_live': + expected_attrs = ['numa_topology'] + if method is None: method = action.replace('_', '') @@ -110,7 +122,7 @@ class CommonMixin(object): mock_method.assert_called_once_with(self.context, instance, *args, **kwargs) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=expected_attrs, cell_down_support=False) def _test_invalid_state(self, action, method=None, body_map=None, @@ -119,6 +131,10 @@ class CommonMixin(object): # Reset the mock. self.mock_get.reset_mock() + expected_attrs = None + if action == '_migrate_live': + expected_attrs = ['numa_topology'] + if method is None: method = action.replace('_', '') if body_map is None: @@ -146,7 +162,7 @@ class CommonMixin(object): mock_method.assert_called_once_with(self.context, instance, *args, **kwargs) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=expected_attrs, cell_down_support=False) def _test_locked_instance(self, action, method=None, body=None, @@ -154,6 +170,10 @@ class CommonMixin(object): # Reset the mock. self.mock_get.reset_mock() + expected_attrs = None + if action == '_migrate_live': + expected_attrs = ['numa_topology'] + if method is None: method = action.replace('_', '') @@ -173,7 +193,7 @@ class CommonMixin(object): mock_method.assert_called_once_with(self.context, instance, *args, **kwargs) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=expected_attrs, cell_down_support=False) def _test_instance_not_found_in_compute_api(self, action, @@ -181,6 +201,10 @@ class CommonMixin(object): # Reset the mock. self.mock_get.reset_mock() + expected_attrs = None + if action == '_migrate_live': + expected_attrs = ['numa_topology'] + if method is None: method = action.replace('_', '') compute_api_args_map = compute_api_args_map or {} @@ -200,7 +224,7 @@ class CommonMixin(object): mock_method.assert_called_once_with(self.context, instance, *args, **kwargs) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=expected_attrs, cell_down_support=False) diff --git a/nova/tests/unit/api/openstack/compute/test_migrate_server.py b/nova/tests/unit/api/openstack/compute/test_migrate_server.py index 69f42f821538..0ea4d092d97a 100644 --- a/nova/tests/unit/api/openstack/compute/test_migrate_server.py +++ b/nova/tests/unit/api/openstack/compute/test_migrate_server.py @@ -150,7 +150,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): 'hostname', self.force, self.async_) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=['numa_topology'], cell_down_support=False) def test_migrate_live_enabled(self): @@ -228,7 +228,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): self.context, instance, False, self.disk_over_commit, 'hostname', self.force, self.async_) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=['numa_topology'], cell_down_support=False) def test_migrate_live_compute_service_unavailable(self): @@ -446,7 +446,7 @@ class MigrateServerTestsV234(MigrateServerTestsV230): self.context, instance, None, self.disk_over_commit, 'hostname', self.force, self.async_) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=['numa_topology'], cell_down_support=False) def test_migrate_live_unexpected_error(self): @@ -465,7 +465,7 @@ class MigrateServerTestsV234(MigrateServerTestsV230): self.context, instance, None, self.disk_over_commit, 'hostname', self.force, self.async_) self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=None, + expected_attrs=['numa_topology'], cell_down_support=False) diff --git a/nova/tests/unit/conductor/tasks/test_live_migrate.py b/nova/tests/unit/conductor/tasks/test_live_migrate.py index b50845696ba7..139d4c858803 100644 --- a/nova/tests/unit/conductor/tasks/test_live_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_live_migrate.py @@ -19,6 +19,7 @@ from nova.compute import power_state from nova.compute import rpcapi as compute_rpcapi from nova.compute import vm_states from nova.conductor.tasks import live_migrate +from nova import context as nova_context from nova import exception from nova.network import model as network_model from nova import objects @@ -39,7 +40,7 @@ fake_selection2 = objects.Selection(service_host="host2", nodename="node2", class LiveMigrationTaskTestCase(test.NoDBTestCase): def setUp(self): super(LiveMigrationTaskTestCase, self).setUp() - self.context = "context" + self.context = nova_context.get_admin_context() self.instance_host = "host" self.instance_uuid = uuids.instance self.instance_image = "image_ref" @@ -53,12 +54,14 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.instance = objects.Instance._from_db_object( self.context, objects.Instance(), db_instance) self.instance.system_metadata = {'image_hw_disk_bus': 'scsi'} + self.instance.numa_topology = None self.destination = "destination" self.block_migration = "bm" self.disk_over_commit = "doc" self.migration = objects.Migration() self.fake_spec = objects.RequestSpec() self._generate_task() + _p = mock.patch('nova.compute.utils.heal_reqspec_is_bfv') self.heal_reqspec_is_bfv_mock = _p.start() self.addCleanup(_p.stop) @@ -170,6 +173,45 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.assertRaises(exception.InstanceInvalidState, self.task._check_instance_is_active) + @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename') + def test_check_instance_has_no_numa_passes_no_numa(self, mock_get): + self.flags(enable_numa_live_migration=False, group='workarounds') + self.task.instance.numa_topology = None + mock_get.return_value = objects.ComputeNode( + uuid=uuids.cn1, hypervisor_type='kvm') + self.task._check_instance_has_no_numa() + + @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename') + def test_check_instance_has_no_numa_passes_non_kvm(self, mock_get): + self.flags(enable_numa_live_migration=False, group='workarounds') + self.task.instance.numa_topology = objects.InstanceNUMATopology( + cells=[objects.InstanceNUMACell(id=0, cpuset=set([0]), + memory=1024)]) + mock_get.return_value = objects.ComputeNode( + uuid=uuids.cn1, hypervisor_type='xen') + self.task._check_instance_has_no_numa() + + @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename') + def test_check_instance_has_no_numa_passes_workaround(self, mock_get): + self.flags(enable_numa_live_migration=True, group='workarounds') + self.task.instance.numa_topology = objects.InstanceNUMATopology( + cells=[objects.InstanceNUMACell(id=0, cpuset=set([0]), + memory=1024)]) + mock_get.return_value = objects.ComputeNode( + uuid=uuids.cn1, hypervisor_type='kvm') + self.task._check_instance_has_no_numa() + + @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename') + def test_check_instance_has_no_numa_fails(self, mock_get): + self.flags(enable_numa_live_migration=False, group='workarounds') + mock_get.return_value = objects.ComputeNode( + uuid=uuids.cn1, hypervisor_type='kvm') + self.task.instance.numa_topology = objects.InstanceNUMATopology( + cells=[objects.InstanceNUMACell(id=0, cpuset=set([0]), + memory=1024)]) + self.assertRaises(exception.MigrationPreCheckError, + self.task._check_instance_has_no_numa) + @mock.patch.object(objects.Service, 'get_by_compute_host') @mock.patch.object(servicegroup.API, 'service_is_up') def test_check_instance_host_is_up(self, mock_is_up, mock_get): diff --git a/releasenotes/notes/disable-live-migration-with-numa-bc710a1bcde25957.yaml b/releasenotes/notes/disable-live-migration-with-numa-bc710a1bcde25957.yaml new file mode 100644 index 000000000000..4297c186f7f3 --- /dev/null +++ b/releasenotes/notes/disable-live-migration-with-numa-bc710a1bcde25957.yaml @@ -0,0 +1,25 @@ +--- +upgrade: + - | + Live migration of instances with NUMA topologies is now disabled by default + when using the libvirt driver. This includes live migration of instances + with CPU pinning or hugepages. CPU pinning and huge page information for + such instances is not currently re-calculated, as noted in `bug #1289064`_. + This means that if instances were already present on the destination host, + the migrated instance could be placed on the same dedicated cores as these + instances or use hugepages allocated for another instance. Alternately, if + the host platforms were not homogeneous, the instance could be assigned to + non-existent cores or be inadvertently split across host NUMA nodes. + + The `long term solution`_ to these issues is to recalculate the XML on the + destination node. When this work is completed, the restriction on live + migration with NUMA topologies will be lifted. + + For operators that are aware of the issues and are able to manually work + around them, the ``[workarounds] enable_numa_live_migration`` option can + be used to allow the broken behavior. + + For more information, refer to `bug #1289064`_. + + .. _bug #1289064: https://bugs.launchpad.net/nova/+bug/1289064 + .. _long term solution: https://blueprints.launchpad.net/nova/+spec/numa-aware-live-migration