diff --git a/octavia/api/drivers/amphora_driver/v1/driver.py b/octavia/api/drivers/amphora_driver/v1/driver.py index 26c4b683fb..83420f3662 100644 --- a/octavia/api/drivers/amphora_driver/v1/driver.py +++ b/octavia/api/drivers/amphora_driver/v1/driver.py @@ -285,13 +285,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 == consts.PROTOCOL_UDP: diff --git a/octavia/api/drivers/amphora_driver/v2/driver.py b/octavia/api/drivers/amphora_driver/v2/driver.py index a93c92b2af..9cdf7693c5 100644 --- a/octavia/api/drivers/amphora_driver/v2/driver.py +++ b/octavia/api/drivers/amphora_driver/v2/driver.py @@ -309,13 +309,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 == consts.PROTOCOL_UDP: diff --git a/octavia/api/v2/controllers/member.py b/octavia/api/v2/controllers/member.py index 7e21a9ddd2..ab209af587 100644 --- a/octavia/api/v2/controllers/member.py +++ b/octavia/api/v2/controllers/member.py @@ -377,6 +377,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_amphora_driver.py b/octavia/tests/unit/api/drivers/amphora_driver/v1/test_amphora_driver.py index 5eb8bf10b5..c3cee3d8a8 100644 --- a/octavia/tests/unit/api/drivers/amphora_driver/v1/test_amphora_driver.py +++ b/octavia/tests/unit/api/drivers/amphora_driver/v1/test_amphora_driver.py @@ -458,13 +458,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_amphora_driver.py b/octavia/tests/unit/api/drivers/amphora_driver/v2/test_amphora_driver.py index 7b85cf5188..ee701c77ca 100644 --- a/octavia/tests/unit/api/drivers/amphora_driver/v2/test_amphora_driver.py +++ b/octavia/tests/unit/api/drivers/amphora_driver/v2/test_amphora_driver.py @@ -467,13 +467,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.