From 00e3f8349d353b1912a994b26926de4e0ed88258 Mon Sep 17 00:00:00 2001 From: Adam Harwell Date: Mon, 22 Mar 2021 17:14:53 -0700 Subject: [PATCH] Fix empty Batch Member Update to unlock objects Move the decision up one layer to the API controller. The amphora driver should now just do as it is told. Change-Id: Idb3ad20b8539bfdb788981a8634317257d83b238 Story: 2008731 Task: 42083 --- octavia/api/drivers/amphora_driver/v1/driver.py | 11 ++++------- octavia/api/drivers/amphora_driver/v2/driver.py | 11 ++++------- octavia/api/v2/controllers/member.py | 6 ++++++ octavia/tests/functional/api/v2/test_member.py | 4 +--- .../unit/api/drivers/amphora_driver/v1/test_driver.py | 11 ++++++++++- .../unit/api/drivers/amphora_driver/v2/test_driver.py | 11 ++++++++++- ...op-batch-member-update-issue-09b76787553e7752.yaml | 5 +++++ 7 files changed, 40 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/Fix-noop-batch-member-update-issue-09b76787553e7752.yaml diff --git a/octavia/api/drivers/amphora_driver/v1/driver.py b/octavia/api/drivers/amphora_driver/v1/driver.py index ab47d87e9e..8485cc4cae 100644 --- a/octavia/api/drivers/amphora_driver/v1/driver.py +++ b/octavia/api/drivers/amphora_driver/v1/driver.py @@ -288,13 +288,10 @@ class AmphoraProviderDriver(driver_base.ProviderDriver): if m.id not in new_member_ids: deleted_members.append(m) - if deleted_members or new_members or updated_members: - payload = {'old_member_ids': [m.id for m in deleted_members], - 'new_member_ids': [m.member_id for m in new_members], - 'updated_members': updated_members} - self.client.cast({}, 'batch_update_members', **payload) - else: - LOG.info("Member batch update is a noop, returning early.") + payload = {'old_member_ids': [m.id for m in deleted_members], + 'new_member_ids': [m.member_id for m in new_members], + 'updated_members': updated_members} + self.client.cast({}, 'batch_update_members', **payload) def _validate_members(self, db_pool, members): if db_pool.protocol in consts.LVS_PROTOCOLS: diff --git a/octavia/api/drivers/amphora_driver/v2/driver.py b/octavia/api/drivers/amphora_driver/v2/driver.py index 34422220d9..666a05ba18 100644 --- a/octavia/api/drivers/amphora_driver/v2/driver.py +++ b/octavia/api/drivers/amphora_driver/v2/driver.py @@ -312,13 +312,10 @@ class AmphoraProviderDriver(driver_base.ProviderDriver): if m.id not in new_member_ids: deleted_members.append(m) - if deleted_members or new_members or updated_members: - payload = {'old_members': [m.to_dict() for m in deleted_members], - 'new_members': [m.to_dict() for m in new_members], - 'updated_members': updated_members} - self.client.cast({}, 'batch_update_members', **payload) - else: - LOG.info("Member batch update is a noop, returning early.") + payload = {'old_members': [m.to_dict() for m in deleted_members], + 'new_members': [m.to_dict() for m in new_members], + 'updated_members': updated_members} + self.client.cast({}, 'batch_update_members', **payload) def _validate_members(self, db_pool, members): if db_pool.protocol in consts.LVS_PROTOCOLS: diff --git a/octavia/api/v2/controllers/member.py b/octavia/api/v2/controllers/member.py index 1bc5bb8d24..1960771c8c 100644 --- a/octavia/api/v2/controllers/member.py +++ b/octavia/api/v2/controllers/member.py @@ -378,6 +378,12 @@ class MembersController(MemberController): if (m.ip_address, m.protocol_port) not in new_member_uniques: deleted_members.append(m) + if not (deleted_members or new_members or updated_members): + LOG.info("Member batch update is a noop, rolling back and " + "returning early.") + lock_session.rollback() + return + if additive_only: member_count_diff = len(new_members) else: diff --git a/octavia/tests/functional/api/v2/test_member.py b/octavia/tests/functional/api/v2/test_member.py index a58074a887..700bfea281 100644 --- a/octavia/tests/functional/api/v2/test_member.py +++ b/octavia/tests/functional/api/v2/test_member.py @@ -942,9 +942,7 @@ class TestMember(base.BaseAPITest): self.assertEqual([], returned_members) - mock_provider.assert_called_once_with(u'noop_driver', - mock_driver.member_batch_update, - self.pool_id, []) + mock_provider.assert_not_called() def test_create_with_attached_listener(self): api_member = self.create_member( diff --git a/octavia/tests/unit/api/drivers/amphora_driver/v1/test_driver.py b/octavia/tests/unit/api/drivers/amphora_driver/v1/test_driver.py index a3ffa62040..26b609569a 100644 --- a/octavia/tests/unit/api/drivers/amphora_driver/v1/test_driver.py +++ b/octavia/tests/unit/api/drivers/amphora_driver/v1/test_driver.py @@ -481,13 +481,22 @@ class TestAmphoraDriver(base.TestRpc): @mock.patch('oslo_messaging.RPCClient.cast') def test_member_batch_update_clear_already_empty( self, mock_cast, mock_pool_get, mock_session): + """Expect that we will pass an empty payload if directed. + + Logic for whether or not to attempt this will be done above the driver + layer, so our driver is responsible to forward the request even if it + is a perceived no-op. + """ mock_pool = mock.MagicMock() mock_pool_get.return_value = mock_pool self.amp_driver.member_batch_update( self.sample_data.pool1_id, []) - mock_cast.assert_not_called() + payload = {'old_member_ids': [], + 'new_member_ids': [], + 'updated_members': []} + mock_cast.assert_called_with({}, 'batch_update_members', **payload) # Health Monitor @mock.patch('oslo_messaging.RPCClient.cast') diff --git a/octavia/tests/unit/api/drivers/amphora_driver/v2/test_driver.py b/octavia/tests/unit/api/drivers/amphora_driver/v2/test_driver.py index 88afb0a0ea..47e41fa213 100644 --- a/octavia/tests/unit/api/drivers/amphora_driver/v2/test_driver.py +++ b/octavia/tests/unit/api/drivers/amphora_driver/v2/test_driver.py @@ -535,13 +535,22 @@ class TestAmphoraDriver(base.TestRpc): @mock.patch('oslo_messaging.RPCClient.cast') def test_member_batch_update_clear_already_empty( self, mock_cast, mock_pool_get, mock_session): + """Expect that we will pass an empty payload if directed. + + Logic for whether or not to attempt this will be done above the driver + layer, so our driver is responsible to forward the request even if it + is a perceived no-op. + """ mock_pool = mock.MagicMock() mock_pool_get.return_value = mock_pool self.amp_driver.member_batch_update( self.sample_data.pool1_id, []) - mock_cast.assert_not_called() + payload = {'old_members': [], + 'new_members': [], + 'updated_members': []} + mock_cast.assert_called_with({}, 'batch_update_members', **payload) # Health Monitor @mock.patch('oslo_messaging.RPCClient.cast') diff --git a/releasenotes/notes/Fix-noop-batch-member-update-issue-09b76787553e7752.yaml b/releasenotes/notes/Fix-noop-batch-member-update-issue-09b76787553e7752.yaml new file mode 100644 index 0000000000..39e45ecbe2 --- /dev/null +++ b/releasenotes/notes/Fix-noop-batch-member-update-issue-09b76787553e7752.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixed an issue with batch member updates, that don't have any changes, + not properly rolling back the update.