From a2984b647a4ed9b593df0cd5ac5077e2c86e1c85 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Tue, 30 Apr 2019 00:06:56 +0200 Subject: [PATCH] update allocation in binding profile during migrate If the server has port with resource allocation and the server is migrated then when the port is bound to the destination host the allocation key needs to be updated in the binding:profile to point to the resource provider that provides resources for this port on the destination host. This patch extends the migrate_instance_finish() network api method to pass the updated resource providers of the ports during migration. Change-Id: I220fa02ee916728e241503084b14984bab4b0c3b blueprint: support-move-ops-with-qos-ports --- nova/compute/manager.py | 37 +++++++------ nova/network/api.py | 9 ++-- nova/network/base_api.py | 12 ++++- nova/network/neutronv2/api.py | 40 +++++++++++---- nova/tests/unit/compute/test_compute.py | 2 +- nova/tests/unit/compute/test_compute_mgr.py | 13 +++-- nova/tests/unit/network/test_api.py | 7 +-- nova/tests/unit/network/test_neutronv2.py | 57 ++++++++++++++++++++- 8 files changed, 135 insertions(+), 42 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index a706c7fae9e..dbd63348a98 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4203,8 +4203,8 @@ def revert_resize(self, context, instance, migration, request_spec=None): self.compute_rpcapi.finish_revert_resize(context, instance, migration, migration.source_compute, request_spec) - def _finish_revert_resize_network_migrate_finish(self, context, instance, - migration): + def _finish_revert_resize_network_migrate_finish( + self, context, instance, migration, provider_mappings): """Causes port binding to be updated. In some Neutron or port configurations - see NetworkModel.get_bind_time_events() - we expect the vif-plugged event from Neutron immediately and wait for it. @@ -4214,6 +4214,8 @@ def _finish_revert_resize_network_migrate_finish(self, context, instance, :param context: The request context. :param instance: The instance undergoing the revert resize. :param migration: The Migration object of the resize being reverted. + :param provider_mappings: a dict of list of resource provider uuids + keyed by port uuid :raises: eventlet.timeout.Timeout or exception.VirtualInterfacePlugException. """ @@ -4238,9 +4240,8 @@ def _finish_revert_resize_network_migrate_finish(self, context, instance, # the migration.dest_compute to source host at here. with utils.temporary_mutation( migration, dest_compute=migration.source_compute): - self.network_api.migrate_instance_finish(context, - instance, - migration) + self.network_api.migrate_instance_finish( + context, instance, migration, provider_mappings) except eventlet.timeout.Timeout: with excutils.save_and_reraise_exception(): LOG.error('Timeout waiting for Neutron events: %s', events, @@ -4292,10 +4293,12 @@ def finish_revert_resize( 'migration_uuid': migration.uuid}) raise + provider_mappings = self._get_request_group_mapping(request_spec) + self.network_api.setup_networks_on_host(context, instance, migration.source_compute) self._finish_revert_resize_network_migrate_finish( - context, instance, migration) + context, instance, migration, provider_mappings) network_info = self.network_api.get_instance_nw_info(context, instance) @@ -4746,7 +4749,7 @@ def _complete_volume_attachments(self, context, bdms): context, bdm.attachment_id) def _finish_resize(self, context, instance, migration, disk_info, - image_meta, bdms): + image_meta, bdms, request_spec): resize_instance = False # indicates disks have been resized old_instance_type_id = migration['old_instance_type_id'] new_instance_type_id = migration['new_instance_type_id'] @@ -4772,11 +4775,12 @@ def _finish_resize(self, context, instance, migration, disk_info, # NOTE(tr3buchet): setup networks on destination host self.network_api.setup_networks_on_host(context, instance, migration.dest_compute) + provider_mappings = self._get_request_group_mapping(request_spec) + # For neutron, migrate_instance_finish updates port bindings for this # host including any PCI devices claimed for SR-IOV ports. - self.network_api.migrate_instance_finish(context, - instance, - migration) + self.network_api.migrate_instance_finish( + context, instance, migration, provider_mappings) network_info = self.network_api.get_instance_nw_info(context, instance) @@ -4850,7 +4854,7 @@ def finish_resize(self, context, disk_info, image, instance, """ try: self._finish_resize_helper(context, disk_info, image, instance, - migration) + migration, request_spec) except Exception: with excutils.save_and_reraise_exception(): # At this point, resize_instance (which runs on the source) has @@ -4871,7 +4875,7 @@ def finish_resize(self, context, disk_info, image, instance, context, instance, migration) def _finish_resize_helper(self, context, disk_info, image, instance, - migration): + migration, request_spec): """Completes the migration process. The caller must revert the instance's allocations if the migration @@ -4883,7 +4887,8 @@ def _finish_resize_helper(self, context, disk_info, image, instance, with self._error_out_instance_on_exception(context, instance): image_meta = objects.ImageMeta.from_dict(image) network_info = self._finish_resize(context, instance, migration, - disk_info, image_meta, bdms) + disk_info, image_meta, bdms, + request_spec) # TODO(melwitt): We should clean up instance console tokens here. The # instance is on a new host and will need to establish a new console @@ -7242,9 +7247,9 @@ def post_live_migration_at_destination(self, context, instance, migration = {'source_compute': instance.host, 'dest_compute': self.host, 'migration_type': 'live-migration'} - self.network_api.migrate_instance_finish(context, - instance, - migration) + # TODO(gibi): calculate and pass resource_provider_mapping + self.network_api.migrate_instance_finish( + context, instance, migration, provider_mappings=None) network_info = self.network_api.get_instance_nw_info(context, instance) self._notify_about_instance_usage( diff --git a/nova/network/api.py b/nova/network/api.py index c8f46e6ce9f..b379ea06009 100644 --- a/nova/network/api.py +++ b/nova/network/api.py @@ -502,7 +502,8 @@ def migrate_instance_start(self, context, instance, migration): self.network_rpcapi.migrate_instance_start(context, **args) - def migrate_instance_finish(self, context, instance, migration): + def migrate_instance_finish( + self, context, instance, migration, provider_mappings): """Finish migrating the network of an instance.""" flavor = instance.get_flavor() args = dict( @@ -524,9 +525,9 @@ def migrate_instance_finish(self, context, instance, migration): def setup_instance_network_on_host(self, context, instance, host, migration=None): """Setup network for specified instance on host.""" - self.migrate_instance_finish(context, instance, - {'source_compute': None, - 'dest_compute': host}) + self.migrate_instance_finish( + context, instance, {'source_compute': None, 'dest_compute': host}, + None) def cleanup_instance_network_on_host(self, context, instance, host): """Cleanup network for specified instance on host.""" diff --git a/nova/network/base_api.py b/nova/network/base_api.py index 2163e17d79f..289e51f809c 100644 --- a/nova/network/base_api.py +++ b/nova/network/base_api.py @@ -343,8 +343,16 @@ def migrate_instance_start(self, context, instance, migration): """Start to migrate the network of an instance.""" raise NotImplementedError() - def migrate_instance_finish(self, context, instance, migration): - """Finish migrating the network of an instance.""" + def migrate_instance_finish( + self, context, instance, migration, provider_mappings): + """Finish migrating the network of an instance. + + :param context: The request context. + :param instance: nova.objects.instance.Instance object. + :param migration: nova.objects.migration.Migration object. + :param provider_mappings: a dict of list of resource provider uuids + keyed by port uuid + """ raise NotImplementedError() def setup_instance_network_on_host(self, context, instance, host, diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 4a263342edf..9b0aab0d189 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -2749,11 +2749,12 @@ def migrate_instance_start(self, context, instance, migration): 'Error: %s', vif['id'], dest_host, resp.status_code, resp.text) - def migrate_instance_finish(self, context, instance, migration): + def migrate_instance_finish( + self, context, instance, migration, provider_mappings): """Finish migrating the network of an instance.""" - self._update_port_binding_for_instance(context, instance, - migration['dest_compute'], - migration=migration) + self._update_port_binding_for_instance( + context, instance, migration['dest_compute'], migration=migration, + provider_mappings=provider_mappings) def add_network_to_project(self, context, project_id, network_uuid=None): """Force add a network to the project.""" @@ -3224,8 +3225,10 @@ def _get_pci_mapping_for_migration(self, instance, migration): migration.get('status') == 'reverted') return instance.migration_context.get_pci_mapping_for_migration(revert) - def _update_port_binding_for_instance(self, context, instance, host, - migration=None): + def _update_port_binding_for_instance( + self, context, instance, host, migration=None, + provider_mappings=None): + neutron = get_client(context, admin=True) search_opts = {'device_id': instance.uuid, 'tenant_id': instance.project_id} @@ -3244,9 +3247,6 @@ def _update_port_binding_for_instance(self, context, instance, host, vif_type = p.get('binding:vif_type') if (p.get(constants.BINDING_HOST_ID) != host or vif_type in FAILED_VIF_TYPES): - # TODO(gibi): To support ports with resource request during - # server move operations we need to take care of 'allocation' - # key in the binding profile per binding. updates[constants.BINDING_HOST_ID] = host # If the host changed, the AZ could have also changed so we @@ -3286,6 +3286,28 @@ def _update_port_binding_for_instance(self, context, instance, host, reason=_("Unable to correlate PCI slot %s") % pci_slot) + if p.get('resource_request'): + if not provider_mappings: + # NOTE(gibi): This should not happen as the API level + # minimum compute service version check ensures that the + # compute services already send the RequestSpec during + # the move operations between the source and the + # destination and the dest compute calculates the + # mapping based on that. + raise exception.PortUpdateFailed( + port_id=p['id'], + reason=_("Provider mappings wasn't provided for the " + "port with resource request")) + + # 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. + binding_profile[constants.ALLOCATION] = \ + provider_mappings[p['id']][0] + updates[constants.BINDING_PROFILE] = binding_profile + port_updates.append((p['id'], updates)) # Avoid rolling back updates if we catch an error above. diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 95b36938e73..123c0ba1e6e 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -4866,7 +4866,7 @@ def _instance_save1(expected_task_state=None): mock_setup.assert_called_once_with(self.context, instance, 'fake-mini') mock_net_mig.assert_called_once_with(self.context, - test.MatchType(objects.Instance), migration) + test.MatchType(objects.Instance), migration, None) mock_get_nw.assert_called_once_with(self.context, instance) mock_notify.assert_has_calls([ mock.call(self.context, instance, 'finish_resize.start', diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 389cd4a961c..1fa4eab6042 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -5250,7 +5250,8 @@ def _test_finish_revert_resize_network_migrate_finish( source_compute='fake-source', dest_compute='fake-dest') - def fake_migrate_instance_finish(context, instance, migration): + def fake_migrate_instance_finish( + context, instance, migration, mapping): # NOTE(artom) This looks weird, but it's checking that the # temporaty_mutation() context manager did its job. self.assertEqual(migration.dest_compute, migration.source_compute) @@ -5263,12 +5264,12 @@ def fake_migrate_instance_finish(context, instance, migration): side_effect=fake_migrate_instance_finish) ) as (mock_wait, mock_migrate_instance_finish): self.compute._finish_revert_resize_network_migrate_finish( - self.context, instance, migration) + self.context, instance, migration, mock.sentinel.mapping) mock_wait.assert_called_once_with( instance, events, deadline=CONF.vif_plugging_timeout, error_callback=self.compute._neutron_failed_migration_callback) mock_migrate_instance_finish.assert_called_once_with( - self.context, instance, migration) + self.context, instance, migration, mock.sentinel.mapping) def test_finish_revert_resize_network_migrate_finish_wait(self): """Test that we wait for bind-time events if we have a hybrid-plugged @@ -7397,7 +7398,8 @@ def test_revert_resize_instance_destroy_disks_non_shared_storage(self): def test_finish_revert_resize_network_calls_order(self): self.nw_info = None - def _migrate_instance_finish(context, instance, migration): + def _migrate_instance_finish( + context, instance, migration, provider_mappings): # The migration.dest_compute is temporarily set to source_compute. self.assertEqual(migration.source_compute, migration.dest_compute) self.nw_info = 'nw_info' @@ -8446,7 +8448,8 @@ def _do_test(mock_notify, post_live_migration_at_destination, self.context, self.instance, {'source_compute': cn_old, 'dest_compute': self.compute.host, - 'migration_type': 'live-migration'}) + 'migration_type': 'live-migration'}, + provider_mappings=None) _get_instance_block_device_info.assert_called_once_with( self.context, self.instance ) diff --git a/nova/tests/unit/network/test_api.py b/nova/tests/unit/network/test_api.py index ba73d4c89a2..6280f352b1f 100644 --- a/nova/tests/unit/network/test_api.py +++ b/nova/tests/unit/network/test_api.py @@ -279,14 +279,14 @@ def test_migrate_instance_finish_with_multihost(self): arg1, arg2, expected = self._stub_migrate_instance_calls( 'migrate_instance_finish', True, info) expected['host'] = 'fake_compute_dest' - self.network_api.migrate_instance_finish(self.context, arg1, arg2) + self.network_api.migrate_instance_finish(self.context, arg1, arg2, {}) self.assertEqual(info['kwargs'], expected) def test_migrate_instance_finish_without_multihost(self): info = {'kwargs': {}} arg1, arg2, expected = self._stub_migrate_instance_calls( 'migrate_instance_finish', False, info) - self.network_api.migrate_instance_finish(self.context, arg1, arg2) + self.network_api.migrate_instance_finish(self.context, arg1, arg2, {}) self.assertEqual(info['kwargs'], expected) def test_is_multi_host_instance_has_no_fixed_ip(self): @@ -516,7 +516,8 @@ def test_setup_instance_network_on_host(self, fake_migrate_finish): self.context, instance, 'fake_compute_source') fake_migrate_finish.assert_called_once_with( self.context, instance, - {'source_compute': None, 'dest_compute': 'fake_compute_source'}) + {'source_compute': None, 'dest_compute': 'fake_compute_source'}, + None) @mock.patch('oslo_concurrency.lockutils.lock') @mock.patch.object(api.API, '_get_instance_nw_info') diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index faeb7436ef8..38a73ef69a7 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -4531,6 +4531,57 @@ def test_update_port_bindings_for_instance_with_live_migration( constants.BINDING_HOST_ID], 'new-host') + @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) + def test_update_port_bindings_for_instance_with_resource_req( + 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.source_compute_rp}, + 'resource_request': mock.sentinel.resource_request}]} + migration = {'status': 'confirmed', + 'migration_type': "migration"} + list_ports_mock = mock.Mock(return_value=fake_ports) + get_client_mock.return_value.list_ports = list_ports_mock + + self.api._update_port_binding_for_instance( + self.context, instance, 'new-host', migration, + {'fake-port-1': [uuids.dest_compute_rp]}) + get_client_mock.return_value.update_port.assert_called_once_with( + 'fake-port-1', + {'port': {'device_owner': 'compute:None', + 'binding:profile': {'allocation': uuids.dest_compute_rp}, + 'binding:host_id': 'new-host'}}) + + @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) + def test_update_port_bindings_for_instance_with_resource_req_no_mapping( + 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.source_compute_rp}, + 'resource_request': mock.sentinel.resource_request}]} + migration = {'status': 'confirmed', + 'migration_type': "migration"} + list_ports_mock = mock.Mock(return_value=fake_ports) + get_client_mock.return_value.list_ports = list_ports_mock + + ex = self.assertRaises( + exception.PortUpdateFailed, + self.api._update_port_binding_for_instance, self.context, + instance, 'new-host', migration, provider_mappings=None) + self.assertIn( + "Provider mappings wasn't provided for the port with resource " + "request", six.text_type(ex)) + def test_get_pci_mapping_for_migration(self): instance = fake_instance.fake_instance_obj(self.context) instance.migration_context = objects.MigrationContext() @@ -6211,7 +6262,8 @@ def test_migrate_instance_finish_binding_true(self): 'migrate_instance_finish', self.context, instance, - migration) + migration, + {}) def test_migrate_instance_finish_binding_true_exception(self): migration = {'source_compute': self.instance.get('host'), @@ -6221,7 +6273,8 @@ def test_migrate_instance_finish_binding_true_exception(self): 'migrate_instance_finish', self.context, instance, - migration) + migration, + {}) def test_setup_instance_network_on_host_true(self): instance = self._fake_instance_object(self.instance)