From c8756d5a4a7c06d25705af6243ea8ce8fd148411 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 17 May 2017 22:48:29 -0400 Subject: [PATCH] Handle network-changed event for a specific port It turns out that when Neutron sends the "network-changed" event it has a port in scope and can provide the port ID. Older versions of Neutron wouldn't send this, but if it's provided, we can try to optimize the network info cache refresh and scope it to just that single port, rather than build the entire cache all over again when the other ports in the cache may not have changed at all since the last time the cache was refreshed. This can be especially beneficial for instances with a relatively large number of ports which are being migrated. Depends-On: Ifdaef05208d09ddd9587fed6214cf388e5265ba4 Change-Id: I023b5b1ccb248e68189f62ba0ff75d41093c1f60 Partial-Bug: #1691602 (cherry picked from commit 537f480927293755bd6365068c1abd0c80951bad) --- nova/compute/manager.py | 5 +- nova/network/neutronv2/api.py | 70 +++++++++-- nova/tests/unit/compute/test_compute_mgr.py | 6 +- nova/tests/unit/network/test_neutronv2.py | 128 ++++++++++++++++++++ 4 files changed, 199 insertions(+), 10 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 6c8fee92a84d..0f7b4acefa94 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -7615,7 +7615,10 @@ class ComputeManager(manager.Manager): instance=instance) if event.name == 'network-changed': try: - self.network_api.get_instance_nw_info(context, instance) + LOG.debug('Refreshing instance network info cache due to ' + 'event %s.', event.key, instance=instance) + self.network_api.get_instance_nw_info( + context, instance, refresh_vif_id=event.tag) except exception.NotFound as e: LOG.info('Failed to process external instance event ' '%(event)s due to: %(error)s', diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index bb7ea3cb8d60..48322c303e86 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -1376,7 +1376,8 @@ class API(base_api.NetworkAPI): def _get_instance_nw_info(self, context, instance, networks=None, port_ids=None, admin_client=None, - preexisting_port_ids=None, **kwargs): + preexisting_port_ids=None, + refresh_vif_id=None, **kwargs): # NOTE(danms): This is an inner method intended to be called # by other code that updates instance nwinfo. It *must* be # called with the refresh_cache-%(instance_uuid) lock held! @@ -1387,7 +1388,8 @@ class API(base_api.NetworkAPI): compute_utils.refresh_info_cache_for_instance(context, instance) nw_info = self._build_network_info_model(context, instance, networks, port_ids, admin_client, - preexisting_port_ids) + preexisting_port_ids, + refresh_vif_id) return network_model.NetworkInfo.hydrate(nw_info) def _gather_port_ids_and_networks(self, context, instance, networks=None, @@ -2355,7 +2357,8 @@ class API(base_api.NetworkAPI): def _build_network_info_model(self, context, instance, networks=None, port_ids=None, admin_client=None, - preexisting_port_ids=None): + preexisting_port_ids=None, + refresh_vif_id=None): """Return list of ordered VIFs attached to instance. :param context: Request context. @@ -2373,6 +2376,10 @@ class API(base_api.NetworkAPI): an instance is de-allocated. Supplied list will be added to the cached list of preexisting port IDs for this instance. + :param refresh_vif_id: Optional port ID to refresh within the existing + cache rather than the entire cache. This can be + triggered via a "network-changed" server external event + from Neutron. """ search_opts = {'tenant_id': instance.project_id, @@ -2385,10 +2392,6 @@ class API(base_api.NetworkAPI): data = client.list_ports(**search_opts) current_neutron_ports = data.get('ports', []) - nw_info_refresh = networks is None and port_ids is None - networks, port_ids = self._gather_port_ids_and_networks( - context, instance, networks, port_ids, client) - nw_info = network_model.NetworkInfo() if preexisting_port_ids is None: preexisting_port_ids = [] @@ -2400,6 +2403,59 @@ class API(base_api.NetworkAPI): current_neutron_port_map[current_neutron_port['id']] = ( current_neutron_port) + # Figure out what kind of operation we're processing. If we're given + # a single port to refresh then we try to optimize and update just the + # information for that VIF in the existing cache rather than try to + # rebuild the entire thing. + if refresh_vif_id is not None: + # TODO(mriedem): Consider pulling this out into it's own method. + nw_info = instance.get_network_info() + if nw_info: + current_neutron_port = current_neutron_port_map.get( + refresh_vif_id) + if current_neutron_port: + # Get the network for the port. + networks = self._get_available_networks( + context, instance.project_id, + [current_neutron_port['network_id']], client) + # Build the VIF model given the latest port information. + refreshed_vif = self._build_vif_model( + context, client, current_neutron_port, networks, + preexisting_port_ids) + for index, vif in enumerate(nw_info): + if vif['id'] == refresh_vif_id: + # Update the existing entry. + nw_info[index] = refreshed_vif + LOG.debug('Updated VIF entry in instance network ' + 'info cache for port %s.', + refresh_vif_id, instance=instance) + break + else: + # If it wasn't in the existing cache, add it. + nw_info.append(refreshed_vif) + LOG.debug('Added VIF to instance network info cache ' + 'for port %s.', refresh_vif_id, + instance=instance) + else: + # This port is no longer associated with the instance, so + # simply remove it from the nw_info cache. + for index, vif in enumerate(nw_info): + if vif['id'] == refresh_vif_id: + LOG.info('Port %s from network info_cache is no ' + 'longer associated with instance in ' + 'Neutron. Removing from network ' + 'info_cache.', refresh_vif_id, + instance=instance) + del nw_info[index] + break + return nw_info + # else there is no existing cache and we need to build it + + nw_info_refresh = networks is None and port_ids is None + networks, port_ids = self._gather_port_ids_and_networks( + context, instance, networks, port_ids, client) + nw_info = network_model.NetworkInfo() + for port_id in port_ids: current_neutron_port = current_neutron_port_map.get(port_id) if current_neutron_port: diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index a558aab1919f..9afeeae9778b 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -2836,7 +2836,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.compute.external_instance_event(self.context, instances, events) get_instance_nw_info.assert_called_once_with(self.context, - instances[0]) + instances[0], + refresh_vif_id='tag1') _process_instance_event.assert_called_once_with(instances[1], events[1]) _process_instance_vif_deleted_event.assert_called_once_with( @@ -2904,7 +2905,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.compute.external_instance_event(self.context, instances, events) get_instance_nw_info.assert_called_once_with(self.context, - instances[0]) + instances[0], + refresh_vif_id='tag1') update_instance_cache_with_nw_info.assert_called_once_with( self.compute.network_api, self.context, diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index e9aeebdef846..b1023bd7611b 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -6318,3 +6318,131 @@ class TestNeutronv2AutoAllocateNetwork(test.NoDBTestCase): ntrn, instance, uuids.port_id, port_req_body) do_test(self) + + +class TestGetInstanceNetworkInfo(test.NoDBTestCase): + """Tests rebuilding the network_info cache.""" + + def setUp(self): + super(TestGetInstanceNetworkInfo, self).setUp() + self.api = neutronapi.API() + self.context = context.RequestContext(uuids.user_id, uuids.project_id) + self.instance = fake_instance.fake_instance_obj(self.context) + client_mock = mock.patch('nova.network.neutronv2.api.get_client') + self.client = client_mock.start().return_value + self.addCleanup(client_mock.stop) + # This is a no-db set of tests and we don't care about refreshing the + # info_cache from the database so just mock it out. + refresh_info_cache_for_instance = mock.patch( + 'nova.compute.utils.refresh_info_cache_for_instance') + refresh_info_cache_for_instance.start() + self.addCleanup(refresh_info_cache_for_instance.stop) + + @staticmethod + def _get_vif_in_cache(info_cache, vif_id): + for vif in info_cache: + if vif['id'] == vif_id: + return vif + + @staticmethod + def _get_fake_info_cache(vif_ids, **kwargs): + """Returns InstanceInfoCache based on the list of provided VIF IDs""" + nwinfo = model.NetworkInfo( + [model.VIF(vif_id, **kwargs) for vif_id in vif_ids]) + return objects.InstanceInfoCache(network_info=nwinfo) + + @staticmethod + def _get_fake_port(port_id, **kwargs): + network_id = kwargs.get('network_id', uuids.network_id) + return {'id': port_id, 'network_id': network_id} + + def test_get_nw_info_refresh_vif_id_add_vif(self): + """Tests that a network-changed event occurred on a single port + which is not already in the cache so it's added. + """ + # The cache has one existing port. + self.instance.info_cache = self._get_fake_info_cache([uuids.old_port]) + # The instance has two ports, one old, one new. + self.client.list_ports.return_value = { + 'ports': [self._get_fake_port(uuids.old_port), + self._get_fake_port(uuids.new_port)]} + with test.nested( + mock.patch.object(self.api, '_get_available_networks', + return_value=[{'id': uuids.network_id}]), + mock.patch.object(self.api, '_build_vif_model', + return_value=model.VIF(uuids.new_port)), + # We should not get as far as calling _gather_port_ids_and_networks + mock.patch.object(self.api, '_gather_port_ids_and_networks', + new_callable=mock.NonCallableMock) + ) as ( + get_nets, build_vif, gather_ports + ): + nwinfo = self.api._get_instance_nw_info( + self.context, self.instance, refresh_vif_id=uuids.new_port) + get_nets.assert_called_once_with( + self.context, self.instance.project_id, + [uuids.network_id], self.client) + # Assert that the old and new ports are in the cache. + for port_id in (uuids.old_port, uuids.new_port): + self.assertIsNotNone(self._get_vif_in_cache(nwinfo, port_id)) + + def test_get_nw_info_refresh_vif_id_update_vif(self): + """Tests that a network-changed event occurred on a single port + which is already in the cache so it's updated. + """ + # The cache has two existing active VIFs. + self.instance.info_cache = self._get_fake_info_cache( + [uuids.old_port, uuids.new_port], active=True) + # The instance has two ports, one old, one new. + self.client.list_ports.return_value = { + 'ports': [self._get_fake_port(uuids.old_port), + self._get_fake_port(uuids.new_port)]} + with test.nested( + mock.patch.object(self.api, '_get_available_networks', + return_value=[{'id': uuids.network_id}]), + # Fake that the port is no longer active. + mock.patch.object(self.api, '_build_vif_model', + return_value=model.VIF( + uuids.new_port, active=False)), + # We should not get as far as calling _gather_port_ids_and_networks + mock.patch.object(self.api, '_gather_port_ids_and_networks', + new_callable=mock.NonCallableMock) + ) as ( + get_nets, build_vif, gather_ports + ): + nwinfo = self.api._get_instance_nw_info( + self.context, self.instance, refresh_vif_id=uuids.new_port) + get_nets.assert_called_once_with( + self.context, self.instance.project_id, + [uuids.network_id], self.client) + # Assert that the old and new ports are in the cache and that the + # old port is still active and the new port is not active. + old_vif = self._get_vif_in_cache(nwinfo, uuids.old_port) + self.assertIsNotNone(old_vif) + self.assertTrue(old_vif['active']) + new_vif = self._get_vif_in_cache(nwinfo, uuids.new_port) + self.assertIsNotNone(new_vif) + self.assertFalse(new_vif['active']) + + def test_get_nw_info_refresh_vif_id_remove_vif(self): + """Tests that a network-changed event occurred on a single port + which is already in the cache but not in the current list of ports + for the instance, so it's removed from the cache. + """ + # The cache has two existing VIFs. + self.instance.info_cache = self._get_fake_info_cache( + [uuids.old_port, uuids.removed_port]) + # The instance has one remaining port. + self.client.list_ports.return_value = { + 'ports': [self._get_fake_port(uuids.old_port)]} + # We should not get as far as calling _gather_port_ids_and_networks + with mock.patch.object( + self.api, '_gather_port_ids_and_networks', + new_callable=mock.NonCallableMock): + nwinfo = self.api._get_instance_nw_info( + self.context, self.instance, refresh_vif_id=uuids.removed_port) + # Assert that only the old port is still in the cache. + old_vif = self._get_vif_in_cache(nwinfo, uuids.old_port) + self.assertIsNotNone(old_vif) + removed_vif = self._get_vif_in_cache(nwinfo, uuids.removed_port) + self.assertIsNone(removed_vif)