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 <sfinucan@redhat.com>
Related-bug: #1289064

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
 

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.

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
 ~~~~~~~~~~~~~~~~~~
 

doc/source/common/numa-live-migration-warning.txt

@@ -0,0 +1,10 @@
+.. important::
+
+   Unless :oslo.config:option:`specifically enabled
+   <workarounds.enable_numa_live_migration>`, 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

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:

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,

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)
 

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
 """),
 ]
 

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)
 
 

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)
 
 

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):

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