From 4b907b062775bc4f6949247241accd89ef5373c0 Mon Sep 17 00:00:00 2001 From: Adam Harwell Date: Tue, 25 Jun 2019 16:59:52 -0700 Subject: [PATCH] Add `additive_only` parameter to Batch Member call If `additive_only` is set, don't do a complete delta -- skip delete and only update and create members (making the call additive rather than a full replacement). This will allow for adding members in batches without wiping out existing members. Change-Id: I5e47d64243667cfaa10430e12229099b508de40e --- api-ref/source/parameters.yaml | 6 ++ api-ref/source/v2/member.inc | 8 ++ octavia/api/root_controller.py | 5 +- octavia/api/v2/controllers/member.py | 42 +++++++--- .../functional/api/test_root_controller.py | 3 +- .../tests/functional/api/v2/test_member.py | 79 +++++++++++++++++-- ...member-call-additive-4785163e625fed1a.yaml | 6 ++ 7 files changed, 127 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/make-batch-member-call-additive-4785163e625fed1a.yaml diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index c818db6fa9..a20a284cdc 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -76,6 +76,12 @@ path-provider: ############################################################################### # Query fields ############################################################################### +additive-only: + description: | + If ``true`` no members will be deleted during the batch operation. + in: query + required: false + type: boolean cascade-delete: description: | If ``true`` will delete all child objects of the load balancer. diff --git a/api-ref/source/v2/member.inc b/api-ref/source/v2/member.inc index c00e042a59..95f74f7961 100644 --- a/api-ref/source/v2/member.inc +++ b/api-ref/source/v2/member.inc @@ -367,11 +367,18 @@ For example, assume a pool currently has two members. These members have the following address/port combinations: '192.0.2.15:80' and '192.0.2.16:80'. Now assume a PUT request is made that includes members with address/port combinations: '192.0.2.16:80' and '192.0.2.17:80'. + The member '192.0.2.15:80' will be deleted, because it was not in the request. + The member '192.0.2.16:80' will be updated to match the request data for that member, because it was matched. + The member '192.0.2.17:80' will be created, because no such member existed. +The optional parameter ``additive_only`` when defined as ``true`` will skip +deletions for members missing from the provided list. If this were set in the +above example, the member '192.0.2.15:80' would have remained in the pool. + If the request is valid, the service returns the ``Accepted (202)`` response code. To confirm the updates, check that the member provisioning statuses are ``ACTIVE`` for new or updated members, and that any unspecified @@ -397,6 +404,7 @@ Request .. rest_parameters:: ../parameters.yaml + - additive_only: additive-only - admin_state_up: admin_state_up-default-optional - address: address - backup: backup-optional diff --git a/octavia/api/root_controller.py b/octavia/api/root_controller.py index ebab03da78..85a51f4b60 100644 --- a/octavia/api/root_controller.py +++ b/octavia/api/root_controller.py @@ -79,6 +79,9 @@ class RootController(rest.RestController): self._add_a_version(versions, 'v2.9', 'v2', 'SUPPORTED', '2019-03-04T00:00:00Z', host_url) # Healthmonitor host header - self._add_a_version(versions, 'v2.10', 'v2', 'CURRENT', + self._add_a_version(versions, 'v2.10', 'v2', 'SUPPORTED', '2019-03-05T00:00:00Z', host_url) + # Additive batch member update + self._add_a_version(versions, 'v2.11', 'v2', 'CURRENT', + '2019-06-24T00:00:00Z', host_url) return {'versions': versions} diff --git a/octavia/api/v2/controllers/member.py b/octavia/api/v2/controllers/member.py index bd73fc7f8f..6f28b25d1d 100644 --- a/octavia/api/v2/controllers/member.py +++ b/octavia/api/v2/controllers/member.py @@ -16,6 +16,7 @@ from oslo_db import exception as odb_exceptions from oslo_log import log as logging from oslo_utils import excutils +from oslo_utils import strutils import pecan from wsme import types as wtypes from wsmeext import pecan as wsme_pecan @@ -322,9 +323,10 @@ class MembersController(MemberController): @wsme_pecan.wsexpose(None, wtypes.text, body=member_types.MembersRootPUT, status_code=202) - def put(self, members_): + def put(self, additive_only=False, members_=None): """Updates all members.""" members = members_.members + additive_only = strutils.bool_from_string(additive_only) context = pecan.request.context.get('octavia_context') db_pool = self._get_db_pool(context.session, self.pool_id) @@ -336,7 +338,9 @@ class MembersController(MemberController): # Check POST+PUT+DELETE since this operation is all of 'CUD' self._auth_validate_action(context, project_id, constants.RBAC_POST) self._auth_validate_action(context, project_id, constants.RBAC_PUT) - self._auth_validate_action(context, project_id, constants.RBAC_DELETE) + if not additive_only: + self._auth_validate_action(context, project_id, + constants.RBAC_DELETE) # Validate member subnets for member in members: @@ -351,13 +355,6 @@ class MembersController(MemberController): with db_api.get_lock_session() as lock_session: self._test_lb_and_listener_and_pool_statuses(lock_session) - member_count_diff = len(members) - len(old_members) - if member_count_diff > 0 and self.repositories.check_quota_met( - context.session, lock_session, data_models.Member, - db_pool.project_id, count=member_count_diff): - raise exceptions.QuotaException( - resource=data_models.Member._name()) - old_member_uniques = { (m.ip_address, m.protocol_port): m.id for m in old_members} new_member_uniques = [ @@ -380,6 +377,16 @@ class MembersController(MemberController): if (m.ip_address, m.protocol_port) not in new_member_uniques: deleted_members.append(m) + if additive_only: + member_count_diff = len(new_members) + else: + member_count_diff = len(new_members) - len(deleted_members) + if member_count_diff > 0 and self.repositories.check_quota_met( + context.session, lock_session, data_models.Member, + db_pool.project_id, count=member_count_diff): + raise exceptions.QuotaException( + resource=data_models.Member._name()) + provider_members = [] # Create new members for m in new_members: @@ -392,6 +399,7 @@ class MembersController(MemberController): # Update old members for m in updated_members: m.provisioning_status = constants.PENDING_UPDATE + m.project_id = db_pool.project_id db_member_dict = m.to_dict(render_unsets=False) db_member_dict.pop('id') self.repositories.member.update( @@ -402,9 +410,19 @@ class MembersController(MemberController): driver_utils.db_member_to_provider_member(m)) # Delete old members for m in deleted_members: - self.repositories.member.update( - lock_session, m.id, - provisioning_status=constants.PENDING_DELETE) + if additive_only: + # Members are appended to the dict and their status remains + # unchanged, because they are logically "untouched". + db_member_dict = m.to_dict(render_unsets=False) + db_member_dict.pop('id') + m.pool_id = self.pool_id + provider_members.append( + driver_utils.db_member_to_provider_member(m)) + else: + # Members are changed to PENDING_DELETE and not passed. + self.repositories.member.update( + lock_session, m.id, + provisioning_status=constants.PENDING_DELETE) # Dispatch to the driver LOG.info("Sending Pool %s batch member update to provider %s", diff --git a/octavia/tests/functional/api/test_root_controller.py b/octavia/tests/functional/api/test_root_controller.py index e2cea56469..bc58e2f6fa 100644 --- a/octavia/tests/functional/api/test_root_controller.py +++ b/octavia/tests/functional/api/test_root_controller.py @@ -43,7 +43,7 @@ class TestRootController(base_db_test.OctaviaDBTestBase): def test_api_versions(self): versions = self._get_versions_with_config() version_ids = tuple(v.get('id') for v in versions) - self.assertEqual(11, len(version_ids)) + self.assertEqual(12, len(version_ids)) self.assertIn('v2.0', version_ids) self.assertIn('v2.1', version_ids) self.assertIn('v2.2', version_ids) @@ -55,6 +55,7 @@ class TestRootController(base_db_test.OctaviaDBTestBase): self.assertIn('v2.8', version_ids) self.assertIn('v2.9', version_ids) self.assertIn('v2.10', version_ids) + self.assertIn('v2.11', version_ids) # Each version should have a 'self' 'href' to the API version URL # [{u'rel': u'self', u'href': u'http://localhost/v2'}] diff --git a/octavia/tests/functional/api/v2/test_member.py b/octavia/tests/functional/api/v2/test_member.py index cc17f69c58..03ad382c6f 100644 --- a/octavia/tests/functional/api/v2/test_member.py +++ b/octavia/tests/functional/api/v2/test_member.py @@ -620,7 +620,6 @@ class TestMember(base.BaseAPITest): ('192.0.2.6', 80, 'PENDING_CREATE'), ] - member_ids = {} provider_creates = [] provider_updates = [] for rm in returned_members: @@ -628,7 +627,6 @@ class TestMember(base.BaseAPITest): (rm['address'], rm['protocol_port'], rm['provisioning_status']), expected_members) - member_ids[(rm['address'], rm['protocol_port'])] = rm['id'] provider_dict = driver_utils.member_dict_to_provider_dict(rm) # Adjust for API response @@ -672,7 +670,6 @@ class TestMember(base.BaseAPITest): ('192.0.2.6', 80, 'PENDING_CREATE', ['test_tag2']), ] - member_ids = {} provider_members = [] for rm in returned_members: self.assertIn( @@ -680,7 +677,6 @@ class TestMember(base.BaseAPITest): rm['protocol_port'], rm['provisioning_status'], rm['tags']), expected_members) - member_ids[(rm['address'], rm['protocol_port'])] = rm['id'] provider_dict = driver_utils.member_dict_to_provider_dict(rm) # Adjust for API response @@ -723,6 +719,77 @@ class TestMember(base.BaseAPITest): err_msg = ("169.254.169.254 is not a valid option for member address") self.assertEqual(err_msg, response.get('faultstring')) + @mock.patch('octavia.api.drivers.driver_factory.get_driver') + @mock.patch('octavia.api.drivers.utils.call_provider') + def test_additive_only_batch_members(self, mock_provider, mock_get_driver): + mock_driver = mock.MagicMock() + mock_driver.name = 'noop_driver' + mock_get_driver.return_value = mock_driver + + member1 = {'address': '192.0.2.1', 'protocol_port': 80} + member2 = {'address': '192.0.2.2', 'protocol_port': 80} + member3 = {'address': '192.0.2.3', 'protocol_port': 80} + member4 = {'address': '192.0.2.4', 'protocol_port': 80} + member5 = {'address': '192.0.2.5', 'protocol_port': 80} + member6 = {'address': '192.0.2.6', 'protocol_port': 80} + members = [member1, member2, member3, member4] + for m in members: + self.create_member(pool_id=self.pool_id, **m) + self.set_lb_status(self.lb_id) + + # We are only concerned about the batch update, so clear out the + # create members calls above. + mock_provider.reset_mock() + + req_dict = [member1, member2, member5, member6] + body = {self.root_tag_list: req_dict} + path = self.MEMBERS_PATH.format(pool_id=self.pool_id) + path = "{}?additive_only=True".format(path) + self.put(path, body, status=202) + returned_members = self.get( + self.MEMBERS_PATH.format(pool_id=self.pool_id) + ).json.get(self.root_tag_list) + + # Members 1+2 should be updated, 3+4 left alone, and 5+6 created + expected_members = [ + ('192.0.2.1', 80, 'PENDING_UPDATE'), + ('192.0.2.2', 80, 'PENDING_UPDATE'), + ('192.0.2.3', 80, 'ACTIVE'), + ('192.0.2.4', 80, 'ACTIVE'), + ('192.0.2.5', 80, 'PENDING_CREATE'), + ('192.0.2.6', 80, 'PENDING_CREATE'), + ] + + provider_creates = [] + provider_updates = [] + provider_ignored = [] + for rm in returned_members: + self.assertIn( + (rm['address'], + rm['protocol_port'], + rm['provisioning_status']), expected_members) + + provider_dict = driver_utils.member_dict_to_provider_dict(rm) + # Adjust for API response + provider_dict['pool_id'] = self.pool_id + if rm['provisioning_status'] == 'PENDING_UPDATE': + del provider_dict['name'] + del provider_dict['subnet_id'] + provider_updates.append(driver_dm.Member(**provider_dict)) + elif rm['provisioning_status'] == 'PENDING_CREATE': + provider_dict['name'] = None + provider_creates.append(driver_dm.Member(**provider_dict)) + elif rm['provisioning_status'] == 'ACTIVE': + provider_dict['name'] = None + provider_ignored.append(driver_dm.Member(**provider_dict)) + # Order matters here + provider_creates += provider_updates + provider_creates += provider_ignored + + mock_provider.assert_called_once_with(u'noop_driver', + mock_driver.member_batch_update, + provider_creates) + @mock.patch('octavia.api.drivers.driver_factory.get_driver') @mock.patch('octavia.api.drivers.utils.call_provider') def test_update_batch_members(self, mock_provider, mock_get_driver): @@ -756,14 +823,12 @@ class TestMember(base.BaseAPITest): ('192.0.2.2', 80, 'PENDING_UPDATE'), ] - member_ids = {} provider_members = [] for rm in returned_members: self.assertIn( (rm['address'], rm['protocol_port'], rm['provisioning_status']), expected_members) - member_ids[(rm['address'], rm['protocol_port'])] = rm['id'] provider_dict = driver_utils.member_dict_to_provider_dict(rm) # Adjust for API response @@ -807,14 +872,12 @@ class TestMember(base.BaseAPITest): ('192.0.2.4', 80, 'PENDING_DELETE'), ] - member_ids = {} provider_members = [] for rm in returned_members: self.assertIn( (rm['address'], rm['protocol_port'], rm['provisioning_status']), expected_members) - member_ids[(rm['address'], rm['protocol_port'])] = rm['id'] mock_provider.assert_called_once_with(u'noop_driver', mock_driver.member_batch_update, diff --git a/releasenotes/notes/make-batch-member-call-additive-4785163e625fed1a.yaml b/releasenotes/notes/make-batch-member-call-additive-4785163e625fed1a.yaml new file mode 100644 index 0000000000..280be06c54 --- /dev/null +++ b/releasenotes/notes/make-batch-member-call-additive-4785163e625fed1a.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The batch member update resource can now be used additively by passing the + query parameter ``additive_only=True``. Existing members can be updated and + new members will be created, but missing members will not be deleted.