Merge "Support live migration with qos ports"

6 files changed, 273 insertions, 29 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 69f202bd4b..52b4d60c54 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -8503,7 +8503,6 @@ class ComputeManager(manager.Manager):
         migration = objects.Migration(source_compute=instance.host,
                                       dest_compute=self.host,
                                       migration_type='live-migration')
-        # TODO(gibi): calculate and pass resource_provider_mapping
         self.network_api.migrate_instance_finish(
             context, instance, migration, provider_mappings=None)
 
diff --git a/nova/conductor/tasks/live_migrate.py b/nova/conductor/tasks/live_migrate.py
index df357a31ce..6d430b2718 100644
--- a/nova/conductor/tasks/live_migrate.py
+++ b/nova/conductor/tasks/live_migrate.py
@@ -257,7 +257,12 @@ class LiveMigrationTask(base.TaskBase):
         self._check_destination_has_enough_memory()
         source_node, dest_node = self._check_compatible_with_source_hypervisor(
             self.destination)
-        self._call_livem_checks_on_host(self.destination)
+        # NOTE(gibi): This code path is used when the live migration is forced
+        # to a target host and skipping the scheduler. Such operation is
+        # rejected for servers with nested resource allocations since
+        # I7cbd5d9fb875ebf72995362e0b6693492ce32051. So here we can safely
+        # assume that the provider mapping is empty.
+        self._call_livem_checks_on_host(self.destination, {})
         # Make sure the forced destination host is in the same cell that the
         # instance currently lives in.
         # NOTE(mriedem): This can go away if/when the forced destination host
@@ -316,7 +321,7 @@ class LiveMigrationTask(base.TaskBase):
             raise exception.DestinationHypervisorTooOld()
         return source_info, destination_info
 
-    def _call_livem_checks_on_host(self, destination):
+    def _call_livem_checks_on_host(self, destination, provider_mapping):
         self._check_can_migrate_pci(self.source, destination)
         try:
             self.migrate_data = self.compute_rpcapi.\
@@ -338,11 +343,25 @@ class LiveMigrationTask(base.TaskBase):
                 self.migrate_data.vifs = migrate_data_obj.VIFMigrateData.\
                     create_skeleton_migrate_vifs(
                     self.instance.get_network_info())
-            bindings = self._bind_ports_on_destination(destination)
+            bindings = self._bind_ports_on_destination(
+                destination, provider_mapping)
             self._update_migrate_vifs_from_bindings(self.migrate_data.vifs,
                                                     bindings)
 
-    def _bind_ports_on_destination(self, destination):
+    @staticmethod
+    def _get_port_profile_from_provider_mapping(port_id, provider_mappings):
+        if port_id in provider_mappings:
+            # NOTE(gibi): In the resource provider mapping there can be
+            # more than one RP fulfilling a request group. But resource
+            # requests of a Neutron port is always mapped to a
+            # numbered request group that is always fulfilled by one
+            # resource provider. So we only pass that single RP UUID
+            # here.
+            return {'allocation': provider_mappings[port_id][0]}
+        else:
+            return {}
+
+    def _bind_ports_on_destination(self, destination, provider_mappings):
         LOG.debug('Start binding ports on destination host: %s', destination,
                   instance=self.instance)
         # Bind ports on the destination host; returns a dict, keyed by
@@ -355,15 +374,18 @@ class LiveMigrationTask(base.TaskBase):
             # if that is the case, it may have updated the required port
             # profile for the destination node (e.g new PCI address if SR-IOV)
             # perform port binding against the requested profile
-            migrate_vifs_with_profile = [mig_vif for mig_vif in
-                                         self.migrate_data.vifs
-                                         if 'profile_json' in mig_vif]
-
-            ports_profile = None
-            if migrate_vifs_with_profile:
-                # Update to the port profile is required
-                ports_profile = {mig_vif.port_id: mig_vif.profile
-                                 for mig_vif in migrate_vifs_with_profile}
+            ports_profile = {}
+            for mig_vif in self.migrate_data.vifs:
+                profile = mig_vif.profile if 'profile_json' in mig_vif else {}
+                # NOTE(gibi): provider_mappings also contribute to the
+                # binding profile of the ports if the port has resource
+                # request. So we need to merge the profile information from
+                # both sources.
+                profile.update(
+                    self._get_port_profile_from_provider_mapping(
+                        mig_vif.port_id, provider_mappings))
+                if profile:
+                    ports_profile[mig_vif.port_id] = profile
 
             bindings = self.network_api.bind_ports_to_host(
                 context=self.context, instance=self.instance, host=destination,
@@ -425,8 +447,15 @@ class LiveMigrationTask(base.TaskBase):
         # is not forced to be the original host
         request_spec.reset_forced_destinations()
 
-        # TODO(gibi): We need to make sure that the requested_resources field
-        # is re calculated based on neutron ports.
+        port_res_req = (
+            self.network_api.get_requested_resource_for_instance(
+                self.context, self.instance.uuid))
+        # NOTE(gibi): When cyborg or other module wants to handle
+        # similar non-nova resources then here we have to collect
+        # all the external resource requests in a single list and
+        # add them to the RequestSpec.
+        request_spec.requested_resources = port_res_req
+
         scheduler_utils.setup_instance_group(self.context, request_spec)
 
         # We currently only support live migrating to hosts in the same
@@ -476,9 +505,19 @@ class LiveMigrationTask(base.TaskBase):
                 # ex.exc_type.
                 raise exception.MigrationSchedulerRPCError(
                     reason=six.text_type(ex))
+
+            scheduler_utils.fill_provider_mapping(request_spec, selection)
+
+            provider_mapping = request_spec.get_request_group_mapping()
+
+            if provider_mapping:
+                compute_utils.\
+                    update_pci_request_spec_with_allocated_interface_name(
+                        self.context, self.report_client, self.instance,
+                        provider_mapping)
             try:
                 self._check_compatible_with_source_hypervisor(host)
-                self._call_livem_checks_on_host(host)
+                self._call_livem_checks_on_host(host, provider_mapping)
             except (exception.Invalid, exception.MigrationPreCheckError) as e:
                 LOG.debug("Skipping host: %(host)s because: %(e)s",
                     {"host": host, "e": e})
diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py
index 8eab0e90a9..4829ae7adb 100644
--- a/nova/network/neutronv2/api.py
+++ b/nova/network/neutronv2/api.py
@@ -1260,10 +1260,6 @@ class API(base_api.NetworkAPI):
 
         client = _get_ksa_client(context, admin=True)
 
-        # TODO(gibi): To support ports with resource request during server
-        # live migrate operation we need to take care of 'allocation' key in
-        # the binding profile per binding.
-
         bindings_by_port_id = {}
         for vif in network_info:
             # Now bind each port to the destination host and keep track of each
@@ -3399,7 +3395,10 @@ class API(base_api.NetworkAPI):
                         reason=_("Unable to correlate PCI slot %s") %
                                  pci_slot)
 
-            if p.get('resource_request'):
+            # NOTE(gibi): during live migration the conductor already sets the
+            # allocation key in the port binding
+            if (p.get('resource_request') and
+                    migration['migration_type'] != constants.LIVE_MIGRATION):
                 if not provider_mappings:
                     # TODO(gibi): Remove this check when compute RPC API is
                     # bumped to 6.0
diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py
index f46477864a..118cb141c4 100644
--- a/nova/tests/functional/test_servers.py
+++ b/nova/tests/functional/test_servers.py
@@ -7237,6 +7237,63 @@ class ServerMoveWithPortResourceRequestTest(
             server, self.compute1_rp_uuid, non_qos_port, qos_port,
             qos_sriov_port, self.flavor_with_group_policy)
 
+    def _turn_off_api_check(self):
+        # The API actively rejecting the move operations with resource
+        # request so we have to turn off that check.
+        # TODO(gibi): Remove this when the move operations are supported and
+        # the API check is removed.
+        patcher = mock.patch(
+            'nova.api.openstack.common.'
+            'supports_port_resource_request_during_move',
+            return_value=True)
+        self.addCleanup(patcher.stop)
+        patcher.start()
+
+    def test_live_migrate_with_qos_port(self):
+        # TODO(gibi): remove this when live migration is fully supported and
+        # therefore the check is removed from the api
+        self._turn_off_api_check()
+
+        non_qos_normal_port = self.neutron.port_1
+        qos_normal_port = self.neutron.port_with_resource_request
+        qos_sriov_port = self.neutron.port_with_sriov_resource_request
+
+        server = self._create_server_with_ports(
+            non_qos_normal_port, qos_normal_port, qos_sriov_port)
+
+        # check that the server allocates from the current host properly
+        self._check_allocation(
+            server, self.compute1_rp_uuid, non_qos_normal_port,
+            qos_normal_port, qos_sriov_port, self.flavor_with_group_policy)
+
+        self.api.post_server_action(
+            server['id'],
+            {
+                'os-migrateLive': {
+                    'host': None,
+                    'block_migration': 'auto'
+                }
+            }
+        )
+
+        self._wait_for_server_parameter(
+            server,
+            {'OS-EXT-SRV-ATTR:host': 'host2',
+             'status': 'ACTIVE'})
+
+        self._check_allocation(
+            server, self.compute2_rp_uuid, non_qos_normal_port,
+            qos_normal_port, qos_sriov_port, self.flavor_with_group_policy)
+
+        self._delete_server_and_check_allocations(
+            server, qos_normal_port, qos_sriov_port)
+
+    # TODO(gibi): add tests for live migration cases:
+    # * with target host
+    # * re-schedule success
+    # * re-schedule fail -> pci request cleanup?
+    # * abort / cancel -> dest / pci request cleanup?
+
 
 class PortResourceRequestReSchedulingTest(
         PortResourceRequestBasedSchedulingTestBase):
diff --git a/nova/tests/unit/conductor/tasks/test_live_migrate.py b/nova/tests/unit/conductor/tasks/test_live_migrate.py
index b1cf177bea..45678c12a1 100644
--- a/nova/tests/unit/conductor/tasks/test_live_migrate.py
+++ b/nova/tests/unit/conductor/tasks/test_live_migrate.py
@@ -12,6 +12,7 @@
 
 import mock
 import oslo_messaging as messaging
+from oslo_serialization import jsonutils
 from oslo_utils.fixture import uuidsentinel as uuids
 import six
 
@@ -74,6 +75,13 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
         self.ensure_network_metadata_mock = _p.start()
         self.addCleanup(_p.stop)
 
+        _p = mock.patch(
+            'nova.network.neutronv2.api.API.'
+            'get_requested_resource_for_instance',
+            return_value=[])
+        self.mock_get_res_req = _p.start()
+        self.addCleanup(_p.stop)
+
     def _generate_task(self):
         self.task = live_migrate.LiveMigrationTask(self.context,
             self.instance, self.destination, self.block_migration,
@@ -438,7 +446,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
         mock_select.assert_called_once_with(self.context, self.fake_spec,
             [self.instance.uuid], return_objects=True, return_alternates=False)
         mock_check.assert_called_once_with('host1')
-        mock_call.assert_called_once_with('host1')
+        mock_call.assert_called_once_with('host1', {})
 
     @mock.patch.object(live_migrate.LiveMigrationTask,
                        '_call_livem_checks_on_host')
@@ -459,7 +467,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
             self.context, self.fake_spec, [self.instance.uuid],
             return_objects=True, return_alternates=False)
         mock_check.assert_called_once_with('host1')
-        mock_call.assert_called_once_with('host1')
+        mock_call.assert_called_once_with('host1', {})
 
     @mock.patch.object(live_migrate.LiveMigrationTask,
                        '_remove_host_allocations')
@@ -486,7 +494,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
             mock.call(self.context, self.fake_spec, [self.instance.uuid],
                       return_objects=True, return_alternates=False)])
         mock_check.assert_has_calls([mock.call('host1'), mock.call('host2')])
-        mock_call.assert_called_once_with('host2')
+        mock_call.assert_called_once_with('host2', {})
 
     def test_find_destination_retry_with_old_hypervisor(self):
         self._test_find_destination_retry_hypervisor_raises(
@@ -521,7 +529,8 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
             mock.call(self.context, self.fake_spec, [self.instance.uuid],
                       return_objects=True, return_alternates=False)])
         mock_check.assert_has_calls([mock.call('host1'), mock.call('host2')])
-        mock_call.assert_has_calls([mock.call('host1'), mock.call('host2')])
+        mock_call.assert_has_calls(
+            [mock.call('host1', {}), mock.call('host2', {})])
 
     @mock.patch.object(live_migrate.LiveMigrationTask,
                        '_remove_host_allocations')
@@ -549,7 +558,8 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
             mock.call(self.context, self.fake_spec, [self.instance.uuid],
                       return_objects=True, return_alternates=False)])
         mock_check.assert_has_calls([mock.call('host1'), mock.call('host2')])
-        mock_call.assert_has_calls([mock.call('host1'), mock.call('host2')])
+        mock_call.assert_has_calls(
+            [mock.call('host1', {}), mock.call('host2', {})])
 
     @mock.patch.object(objects.Migration, 'save')
     @mock.patch.object(live_migrate.LiveMigrationTask,
@@ -612,7 +622,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
                                   side_effect=messaging.MessagingTimeout),
                 mock.patch.object(self.task, '_check_can_migrate_pci')):
             self.assertRaises(exception.MigrationPreCheckError,
-                              self.task._call_livem_checks_on_host, {})
+                              self.task._call_livem_checks_on_host, {}, {})
 
     def test_call_livem_checks_on_host_bind_ports(self):
         data = objects.LibvirtLiveMigrateData()
@@ -639,7 +649,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
                 network_model.VIF(uuids.port2)])
             self.instance.info_cache = objects.InstanceInfoCache(
                 network_info=nwinfo)
-            self.task._call_livem_checks_on_host('dest-host')
+            self.task._call_livem_checks_on_host('dest-host', {})
             # Assert the migrate_data set on the task based on the port
             # bindings created.
             self.assertIn('vifs', data)
@@ -651,6 +661,118 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase):
 
         _test()
 
+    @mock.patch('nova.network.neutronv2.api.API.bind_ports_to_host')
+    def test_bind_ports_on_destination_merges_profiles(self, mock_bind_ports):
+        """Assert that if both the migration_data and the provider mapping
+        contains binding profile related information then such information is
+        merged in the resulting profile.
+        """
+
+        self.task.migrate_data = objects.LibvirtLiveMigrateData(
+            vifs=[
+                objects.VIFMigrateData(
+                    port_id=uuids.port1,
+                    profile_json=jsonutils.dumps(
+                        {'some-key': 'value'}))
+            ])
+        provider_mappings = {uuids.port1: [uuids.dest_bw_rp]}
+
+        self.task._bind_ports_on_destination('dest-host', provider_mappings)
+
+        mock_bind_ports.assert_called_once_with(
+            context=self.context, instance=self.instance, host='dest-host',
+            vnic_types=None,
+            port_profiles={uuids.port1: {'allocation': uuids.dest_bw_rp,
+                                         'some-key': 'value'}})
+
+    @mock.patch('nova.network.neutronv2.api.API.bind_ports_to_host')
+    def test_bind_ports_on_destination_migration_data(self, mock_bind_ports):
+        """Assert that if only the migration_data contains binding profile
+        related information then that is sent to neutron.
+        """
+
+        self.task.migrate_data = objects.LibvirtLiveMigrateData(
+            vifs=[
+                objects.VIFMigrateData(
+                    port_id=uuids.port1,
+                    profile_json=jsonutils.dumps(
+                        {'some-key': 'value'}))
+            ])
+        provider_mappings = {}
+
+        self.task._bind_ports_on_destination('dest-host', provider_mappings)
+
+        mock_bind_ports.assert_called_once_with(
+            context=self.context, instance=self.instance, host='dest-host',
+            vnic_types=None,
+            port_profiles={uuids.port1: {'some-key': 'value'}})
+
+    @mock.patch('nova.network.neutronv2.api.API.bind_ports_to_host')
+    def test_bind_ports_on_destination_provider_mapping(self, mock_bind_ports):
+        """Assert that if only the provider mapping contains binding
+        profile related information then that is sent to neutron.
+        """
+
+        self.task.migrate_data = objects.LibvirtLiveMigrateData(
+            vifs=[
+                objects.VIFMigrateData(
+                    port_id=uuids.port1)
+            ])
+        provider_mappings = {uuids.port1: [uuids.dest_bw_rp]}
+
+        self.task._bind_ports_on_destination('dest-host', provider_mappings)
+
+        mock_bind_ports.assert_called_once_with(
+            context=self.context, instance=self.instance, host='dest-host',
+            vnic_types=None,
+            port_profiles={uuids.port1: {'allocation': uuids.dest_bw_rp}})
+
+    @mock.patch(
+        'nova.compute.utils.'
+        'update_pci_request_spec_with_allocated_interface_name')
+    @mock.patch('nova.scheduler.utils.fill_provider_mapping')
+    @mock.patch.object(live_migrate.LiveMigrationTask,
+                       '_call_livem_checks_on_host')
+    @mock.patch.object(live_migrate.LiveMigrationTask,
+                       '_check_compatible_with_source_hypervisor')
+    @mock.patch.object(query.SchedulerQueryClient, 'select_destinations',
+                       return_value=[[fake_selection1]])
+    @mock.patch.object(objects.RequestSpec, 'reset_forced_destinations')
+    @mock.patch.object(scheduler_utils, 'setup_instance_group')
+    def test_find_destination_with_resource_request(
+            self, mock_setup, mock_reset, mock_select, mock_check, mock_call,
+            mock_fill_provider_mapping, mock_update_pci_req):
+        resource_req = [objects.RequestGroup(requester_id=uuids.port_id)]
+        self.mock_get_res_req.return_value = resource_req
+
+        self.assertEqual(("host1", "node1", fake_limits1),
+                         self.task._find_destination())
+
+        # Make sure the request_spec was updated to include the cell
+        # mapping.
+        self.assertIsNotNone(self.fake_spec.requested_destination.cell)
+        # Make sure the spec was updated to include the project_id.
+        self.assertEqual(self.fake_spec.project_id, self.instance.project_id)
+        # Make sure that requested_resources are added to the request spec
+        self.assertEqual(
+            resource_req, self.task.request_spec.requested_resources)
+
+        mock_setup.assert_called_once_with(self.context, self.fake_spec)
+        mock_reset.assert_called_once_with()
+        self.ensure_network_metadata_mock.assert_called_once_with(
+            self.instance)
+        self.heal_reqspec_is_bfv_mock.assert_called_once_with(
+            self.context, self.fake_spec, self.instance)
+        mock_select.assert_called_once_with(self.context, self.fake_spec,
+            [self.instance.uuid], return_objects=True, return_alternates=False)
+        mock_check.assert_called_once_with('host1')
+        mock_call.assert_called_once_with('host1', {uuids.port_id: []})
+        mock_fill_provider_mapping.assert_called_once_with(
+            self.task.request_spec, fake_selection1)
+        mock_update_pci_req.assert_called_once_with(
+            self.context, self.task.report_client, self.instance,
+            {uuids.port_id: []})
+
     @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid',
                        side_effect=exception.InstanceMappingNotFound(
                            uuid=uuids.instance))
diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py
index 3e2ce97655..c771d8b799 100644
--- a/nova/tests/unit/network/test_neutronv2.py
+++ b/nova/tests/unit/network/test_neutronv2.py
@@ -4658,6 +4658,34 @@ class TestNeutronv2(TestNeutronv2Base):
             "are required for ports with a resource request.",
             six.text_type(ex))
 
+    @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock())
+    def test_update_port_bindings_for_instance_with_resource_req_live_mig(
+            self, get_client_mock):
+
+        instance = fake_instance.fake_instance_obj(self.context)
+        fake_ports = {'ports': [
+            {'id': 'fake-port-1',
+             'binding:vnic_type': 'normal',
+             constants.BINDING_HOST_ID: 'old-host',
+             constants.BINDING_PROFILE:
+                 {'allocation': uuids.dest_compute_rp},
+             'resource_request': mock.sentinel.resource_request}]}
+        migration = objects.Migration(
+            status='confirmed', migration_type='live-migration')
+        list_ports_mock = mock.Mock(return_value=fake_ports)
+        get_client_mock.return_value.list_ports = list_ports_mock
+
+        # No mapping is passed in as during live migration the conductor
+        # already created the binding and added the allocation key
+        self.api._update_port_binding_for_instance(
+            self.context, instance, 'new-host', migration, {})
+
+        # Note that binding:profile is not updated
+        get_client_mock.return_value.update_port.assert_called_once_with(
+            'fake-port-1',
+            {'port': {'device_owner': 'compute:None',
+                      'binding:host_id': 'new-host'}})
+
     def test_get_pci_mapping_for_migration(self):
         instance = fake_instance.fake_instance_obj(self.context)
         instance.migration_context = objects.MigrationContext()