From 50b1eb170bb794ed221058f9c341585fc68f3eaf Mon Sep 17 00:00:00 2001 From: Mohammed Naser Date: Thu, 25 Aug 2022 19:44:17 -0400 Subject: [PATCH] Cache subnets validation for batch member update At the moment, we are actually going ahead and sending an API request for every single member when doing a batch update, this can be very expensive in large number of members and will cause a big hit against the Neutron API, it potentially also makes those API calls timeout. This patch instead implements a simple cache (per batch call) to simply store the subnets which are considered valid which means that it will make significantly less calls, potentially even one. It also adjusts the behaviour so that it only runs this on newly added members which should avoid running it for deleted and updated members which won't have an effect in this change. Change-Id: I24ec7dc3b18111c126bcfdaa0780bfd7993020fb (cherry picked from commit e0a9ba1560bf38d07d7e03bc0f36195703884db1) (cherry picked from commit 0545e81b376184cfc224f8c57829cdf355b4ddcf) --- octavia/api/v2/controllers/member.py | 26 ++++++++--- .../tests/functional/api/v2/test_member.py | 44 +++++++++++++++++++ .../catch_validation-27ffe48ca187c46f.yaml | 8 ++++ 3 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/catch_validation-27ffe48ca187c46f.yaml diff --git a/octavia/api/v2/controllers/member.py b/octavia/api/v2/controllers/member.py index f630609399..1defed9ec4 100644 --- a/octavia/api/v2/controllers/member.py +++ b/octavia/api/v2/controllers/member.py @@ -336,13 +336,6 @@ class MembersController(MemberController): self._auth_validate_action(context, project_id, constants.RBAC_DELETE) - # Validate member subnets - for member in members: - if member.subnet_id and not validate.subnet_exists( - member.subnet_id, context=context): - raise exceptions.NotFound(resource='Subnet', - id=member.subnet_id) - # Load the driver early as it also provides validation driver = driver_factory.get_driver(provider) @@ -382,8 +375,27 @@ class MembersController(MemberController): resource=data_models.Member._name()) provider_members = [] + valid_subnets = set() # Create new members for m in new_members: + # NOTE(mnaser): In order to avoid hitting the Neutron API hard + # when creating many new members, we cache the + # validation results. We also validate new + # members only since subnet ID is immutable. + # If the member doesn't have a subnet, or the subnet is + # already valid, move on. Run validate and add it to + # cache otherwise. + if m.subnet_id and m.subnet_id not in valid_subnets: + # If the subnet does not exist, + # raise an exception and get out. + if not validate.subnet_exists( + m.subnet_id, context=context): + raise exceptions.NotFound( + resource='Subnet', id=m.subnet_id) + + # Mark the subnet as valid for future runs. + valid_subnets.add(m.subnet_id) + m = m.to_dict(render_unsets=False) m['project_id'] = db_pool.project_id created_member = self._graph_create(lock_session, m) diff --git a/octavia/tests/functional/api/v2/test_member.py b/octavia/tests/functional/api/v2/test_member.py index 12bd4fd7b9..d3c7c5f21c 100644 --- a/octavia/tests/functional/api/v2/test_member.py +++ b/octavia/tests/functional/api/v2/test_member.py @@ -880,6 +880,50 @@ class TestMember(base.BaseAPITest): mock_driver.member_batch_update, provider_members) + @mock.patch('octavia.api.drivers.driver_factory.get_driver') + @mock.patch('octavia.api.drivers.utils.call_provider') + def test_update_members_subnet_duplicate( + self, mock_provider, mock_get_driver): + mock_driver = mock.MagicMock() + mock_driver.name = 'noop_driver' + mock_get_driver.return_value = mock_driver + subnet_id = uuidutils.generate_uuid() + + member1 = {'address': '192.0.2.1', 'protocol_port': 80, + 'project_id': self.project_id, 'subnet_id': subnet_id} + member2 = {'address': '192.0.2.2', 'protocol_port': 80, + 'project_id': self.project_id, 'subnet_id': subnet_id} + + req_dict = [member1, member2] + body = {self.root_tag_list: req_dict} + path = self.MEMBERS_PATH.format(pool_id=self.pool_id) + with mock.patch("octavia.common.validate." + "subnet_exists") as m_subnet_exists: + m_subnet_exists.return_value = True + self.put(path, body, status=202) + m_subnet_exists.assert_called_once_with( + member1['subnet_id'], context=mock.ANY) + + @mock.patch('octavia.api.drivers.driver_factory.get_driver') + @mock.patch('octavia.api.drivers.utils.call_provider') + def test_update_members_subnet_not_found( + self, mock_provider, mock_get_driver): + mock_driver = mock.MagicMock() + mock_driver.name = 'noop_driver' + mock_get_driver.return_value = mock_driver + fake_subnet_id = uuidutils.generate_uuid() + + member1 = {'address': '192.0.2.1', 'protocol_port': 80, + 'project_id': self.project_id, 'subnet_id': fake_subnet_id} + + req_dict = [member1] + body = {self.root_tag_list: req_dict} + path = self.MEMBERS_PATH.format(pool_id=self.pool_id) + with mock.patch("octavia.common.validate." + "subnet_exists") as m_subnet_exists: + m_subnet_exists.return_value = False + self.put(path, body, status=404) + @mock.patch('octavia.api.drivers.driver_factory.get_driver') @mock.patch('octavia.api.drivers.utils.call_provider') def test_delete_batch_members(self, mock_provider, mock_get_driver): diff --git a/releasenotes/notes/catch_validation-27ffe48ca187c46f.yaml b/releasenotes/notes/catch_validation-27ffe48ca187c46f.yaml new file mode 100644 index 0000000000..73a2045ff0 --- /dev/null +++ b/releasenotes/notes/catch_validation-27ffe48ca187c46f.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + In order to avoid hitting the Neutron API hard + when batch update with creating many new members, we cache the + subnet validation results in batch update members API call. + We also change to validate new members only during batch update + members since subnet ID is immutable.