From 732dbdaf5e1ccf5a592131447927e66b9202e534 Mon Sep 17 00:00:00 2001 From: Bence Romsics Date: Tue, 5 Mar 2019 15:43:19 +0100 Subject: [PATCH] Fail placement sync if _get_rp_by_name() fails The Placement sync process involves some input from Placement first. That is the UUID of the compute host RP. This is a remote call just like the Placement updates we send later and it also may fail in all the usual ways of remote calls. We need to fail the sync procedure if this remote call fails. Previously I had the mistaken belief that if I set the parent_uuid to None that will be an invalid call rejected by Placement. But no, that's a valid call and creates a resource provider without a parent. That is the neutron managed resource providers will be in their own resource provider tree instead of the compute host's resource provider tree. In this change we make sure to handle the failure of getting the compute host RP properly. We must not continue with the updates. And we must set the agent's resources_synced to False. Change-Id: Ie6ad33e2170c53a16c39a31a8d7f6496170a90ce Closes-Bug: #1818683 --- neutron/services/placement_report/plugin.py | 48 ++++++++++------- .../services/placement_report/test_plugin.py | 53 +++++++++++++++++-- 2 files changed, 78 insertions(+), 23 deletions(-) diff --git a/neutron/services/placement_report/plugin.py b/neutron/services/placement_report/plugin.py index 8a6e3f737e4..b1187f73931 100644 --- a/neutron/services/placement_report/plugin.py +++ b/neutron/services/placement_report/plugin.py @@ -67,14 +67,6 @@ class PlacementReportPlugin(service_base.ServicePluginBase): rps = self._placement_client.list_resource_providers( name=name)['resource_providers'] # RP names are unique, therefore we can get 0 or 1. But not many. - if len(rps) != 1: - # NOTE(bence romsics): While we could raise() here and by detect - # an error a bit earlier, we want the error to surface in the - # sync batch below so it is going to be properly caught and is - # going to influence the agent's resources_synced attribute. - LOG.warning( - 'placement client: no such resource provider: %s', name) - return {'uuid': None} return rps[0] def _sync_placement_state(self, agent, agent_db): @@ -85,12 +77,26 @@ class PlacementReportPlugin(service_base.ServicePluginBase): supported_vnic_types = mech_driver.supported_vnic_types device_mappings = mech_driver.get_standard_device_mappings(agent) + log_msg = ( + 'Synchronization of resources ' + 'of agent type %(type)s ' + 'at host %(host)s ' + 'to placement %(result)s.') + try: agent_host_rp_uuid = self._get_rp_by_name( name=agent['host'])['uuid'] - except ks_exc.HttpError: - # Delay the error for the same reason as in _get_rp_by_name(). - agent_host_rp_uuid = None + except (IndexError, ks_exc.HttpError, ks_exc.ClientException): + agent_db.resources_synced = False + agent_db.update() + + LOG.warning( + log_msg, + {'type': agent['agent_type'], + 'host': agent['host'], + 'result': 'failed'}) + + return state = placement_report.PlacementState( rp_bandwidths=configurations[ @@ -139,14 +145,18 @@ class PlacementReportPlugin(service_base.ServicePluginBase): agent_db.resources_synced = resources_synced agent_db.update() - LOG.debug( - 'Synchronization of resources' - ' of agent type %(type)s' - ' at host %(host)s' - ' to placement %(result)s.', - {'type': agent['agent_type'], - 'host': agent['host'], - 'result': 'succeeded' if resources_synced else 'failed'}) + if resources_synced: + LOG.debug( + log_msg, + {'type': agent['agent_type'], + 'host': agent['host'], + 'result': 'succeeded'}) + else: + LOG.warning( + log_msg, + {'type': agent['agent_type'], + 'host': agent['host'], + 'result': 'failed'}) self._batch_notifier.queue_event(batch) diff --git a/neutron/tests/unit/services/placement_report/test_plugin.py b/neutron/tests/unit/services/placement_report/test_plugin.py index 2eae9445d8a..0ed3d2843ef 100644 --- a/neutron/tests/unit/services/placement_report/test_plugin.py +++ b/neutron/tests/unit/services/placement_report/test_plugin.py @@ -14,6 +14,7 @@ import mock +from keystoneauth1 import exceptions as ks_exc from neutron_lib.agent import constants as agent_const from oslo_log import log as logging @@ -39,14 +40,58 @@ class PlacementReportPluginTestCases(test_plugin.Ml2PluginV2TestCase): self.assertEqual('fake_rp', rp) def test__get_rp_by_name_not_found(self): + with mock.patch.object( + self.service_plugin._placement_client, + 'list_resource_providers', + return_value={'resource_providers': []}): + self.assertRaises( + IndexError, self.service_plugin._get_rp_by_name, 'no_such_rp') + + def test_no_sync_for_rp_name_not_found(self): + # looking all good + agent = { + 'agent_type': 'test_mechanism_driver_agent', + 'configurations': {'resource_provider_bandwidths': {}}, + 'host': 'fake host', + } + agent_db = mock.Mock() + with mock.patch.object( self.service_plugin._placement_client, 'list_resource_providers', return_value={'resource_providers': []}), \ - mock.patch.object(plugin.LOG, 'warning') as log_mock: - rp = self.service_plugin._get_rp_by_name('whatever') - self.assertEqual(1, log_mock.call_count) - self.assertEqual({'uuid': None}, rp) + mock.patch.object( + self.service_plugin._batch_notifier, + 'queue_event') as mock_queue_event: + + self.service_plugin._sync_placement_state(agent, agent_db) + + self.assertFalse(agent_db.resources_synced) + agent_db.update.assert_called_with() + mock_queue_event.assert_not_called() + + def test_no_sync_for_placement_gone(self): + # looking all good + agent = { + 'agent_type': 'test_mechanism_driver_agent', + 'configurations': {'resource_provider_bandwidths': {}}, + 'host': 'fake host', + } + agent_db = mock.Mock() + + with mock.patch.object( + self.service_plugin._placement_client, + 'list_resource_providers', + side_effect=ks_exc.HttpError), \ + mock.patch.object( + self.service_plugin._batch_notifier, + 'queue_event') as mock_queue_event: + + self.service_plugin._sync_placement_state(agent, agent_db) + + self.assertFalse(agent_db.resources_synced) + agent_db.update.assert_called_with() + mock_queue_event.assert_not_called() def test_no_sync_for_unsupported_agent_type(self): payload = mock.Mock(