From 01dfb8ffaf6f22e48915e05f70553870c39ea96a Mon Sep 17 00:00:00 2001 From: Adam Harwell Date: Mon, 14 Oct 2019 12:43:32 -0700 Subject: [PATCH] Fix batch member update error on empty change list If the list of changes was empty, the worker would fail to fetch the pool because it was retrieved implicitly from one of the changed members. Pass it explicitly instead, and also short-circuit on NOOPs. Story: 2006719 Task: 37090 Depends-On: https://review.opendev.org/#/c/688546/ Change-Id: I161a522abad4a2aa521ea46cb1065c5b05a2cd2e --- doc/source/contributor/guides/providers.rst | 3 +- .../api/drivers/amphora_driver/v1/driver.py | 15 +++++---- .../api/drivers/amphora_driver/v2/driver.py | 15 +++++---- octavia/api/drivers/noop_driver/driver.py | 11 ++++--- octavia/api/v2/controllers/member.py | 3 +- .../tests/functional/api/v2/test_member.py | 32 ++++++++++++++++--- .../amphora_driver/v1/test_amphora_driver.py | 24 +++++++++++--- .../amphora_driver/v2/test_amphora_driver.py | 24 +++++++++++--- .../api/drivers/test_provider_noop_driver.py | 2 +- 9 files changed, 94 insertions(+), 35 deletions(-) diff --git a/doc/source/contributor/guides/providers.rst b/doc/source/contributor/guides/providers.rst index dab1b0bd3a..74ed171112 100644 --- a/doc/source/contributor/guides/providers.rst +++ b/doc/source/contributor/guides/providers.rst @@ -1111,9 +1111,10 @@ and members in the list that do not already exist should be created. """ raise NotImplementedError() - def member_batch_update(self, members): + def member_batch_update(self, pool_id, members): """Creates, updates, or deletes a set of pool members. + :param pool_id (string): The id of the pool to update. :param members (list): List of member objects. :return: Nothing if the create request was accepted. :raises DriverError: An unexpected error occurred in the driver. diff --git a/octavia/api/drivers/amphora_driver/v1/driver.py b/octavia/api/drivers/amphora_driver/v1/driver.py index 7f5992c6c0..de4d86da6a 100644 --- a/octavia/api/drivers/amphora_driver/v1/driver.py +++ b/octavia/api/drivers/amphora_driver/v1/driver.py @@ -201,9 +201,7 @@ class AmphoraProviderDriver(driver_base.ProviderDriver): consts.MEMBER_UPDATES: member_dict} self.client.cast({}, 'update_member', **payload) - def member_batch_update(self, members): - # Get a list of existing members - pool_id = members[0].pool_id + def member_batch_update(self, pool_id, members): # The DB should not have updated yet, so we can still use the pool db_pool = self.repositories.pool.get(db_apis.get_session(), id=pool_id) @@ -236,10 +234,13 @@ class AmphoraProviderDriver(driver_base.ProviderDriver): if m.id not in new_member_ids: deleted_members.append(m) - 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) + 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.") 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 944bcaa687..ab7db428b6 100644 --- a/octavia/api/drivers/amphora_driver/v2/driver.py +++ b/octavia/api/drivers/amphora_driver/v2/driver.py @@ -200,9 +200,7 @@ class AmphoraProviderDriver(driver_base.ProviderDriver): consts.MEMBER_UPDATES: member_dict} self.client.cast({}, 'update_member', **payload) - def member_batch_update(self, members): - # Get a list of existing members - pool_id = members[0].pool_id + def member_batch_update(self, pool_id, members): # The DB should not have updated yet, so we can still use the pool db_pool = self.repositories.pool.get(db_apis.get_session(), id=pool_id) @@ -235,10 +233,13 @@ class AmphoraProviderDriver(driver_base.ProviderDriver): if m.id not in new_member_ids: deleted_members.append(m) - 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) + 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.") def _validate_members(self, db_pool, members): if db_pool.protocol == consts.PROTOCOL_UDP: diff --git a/octavia/api/drivers/noop_driver/driver.py b/octavia/api/drivers/noop_driver/driver.py index 95a4b3bd1e..64943c4b6f 100644 --- a/octavia/api/drivers/noop_driver/driver.py +++ b/octavia/api/drivers/noop_driver/driver.py @@ -148,10 +148,11 @@ class NoopManager(object): self.driverconfig[new_member.member_id] = ( new_member, 'member_update') - def member_batch_update(self, members): + def member_batch_update(self, pool_id, members): for member in members: - LOG.debug('Provider %s no-op, member_batch_update member %s', - self.__class__.__name__, member.member_id) + LOG.debug('Provider %s no-op, member_batch_update pool_id %s ' + 'member %s', + self.__class__.__name__, pool_id, member.member_id) self.driverconfig[member.member_id] = (member, 'member_batch_update') @@ -294,8 +295,8 @@ class NoopProviderDriver(driver_base.ProviderDriver): def member_update(self, old_member, new_member): self.driver.member_update(old_member, new_member) - def member_batch_update(self, members): - self.driver.member_batch_update(members) + def member_batch_update(self, pool_id, members): + self.driver.member_batch_update(pool_id, members) # Health Monitor def health_monitor_create(self, healthmonitor): diff --git a/octavia/api/v2/controllers/member.py b/octavia/api/v2/controllers/member.py index 6f28b25d1d..210532318d 100644 --- a/octavia/api/v2/controllers/member.py +++ b/octavia/api/v2/controllers/member.py @@ -428,4 +428,5 @@ class MembersController(MemberController): LOG.info("Sending Pool %s batch member update to provider %s", db_pool.id, driver.name) driver_utils.call_provider( - driver.name, driver.member_batch_update, provider_members) + driver.name, driver.member_batch_update, db_pool.id, + provider_members) diff --git a/octavia/tests/functional/api/v2/test_member.py b/octavia/tests/functional/api/v2/test_member.py index 03ad382c6f..ddae3b73d4 100644 --- a/octavia/tests/functional/api/v2/test_member.py +++ b/octavia/tests/functional/api/v2/test_member.py @@ -643,7 +643,7 @@ class TestMember(base.BaseAPITest): mock_provider.assert_called_once_with(u'noop_driver', mock_driver.member_batch_update, - provider_creates) + self.pool_id, provider_creates) @mock.patch('octavia.api.drivers.driver_factory.get_driver') @mock.patch('octavia.api.drivers.utils.call_provider') @@ -686,7 +686,7 @@ class TestMember(base.BaseAPITest): mock_provider.assert_called_once_with(u'noop_driver', mock_driver.member_batch_update, - provider_members) + self.pool_id, provider_members) def test_create_batch_members_with_bad_subnet(self): subnet_id = uuidutils.generate_uuid() @@ -788,7 +788,7 @@ class TestMember(base.BaseAPITest): mock_provider.assert_called_once_with(u'noop_driver', mock_driver.member_batch_update, - provider_creates) + self.pool_id, provider_creates) @mock.patch('octavia.api.drivers.driver_factory.get_driver') @mock.patch('octavia.api.drivers.utils.call_provider') @@ -839,7 +839,7 @@ class TestMember(base.BaseAPITest): mock_provider.assert_called_once_with(u'noop_driver', mock_driver.member_batch_update, - provider_members) + self.pool_id, provider_members) @mock.patch('octavia.api.drivers.driver_factory.get_driver') @mock.patch('octavia.api.drivers.utils.call_provider') @@ -881,7 +881,29 @@ class TestMember(base.BaseAPITest): mock_provider.assert_called_once_with(u'noop_driver', mock_driver.member_batch_update, - provider_members) + self.pool_id, provider_members) + + @mock.patch('octavia.api.drivers.driver_factory.get_driver') + @mock.patch('octavia.api.drivers.utils.call_provider') + def test_delete_batch_members_already_empty(self, mock_provider, + mock_get_driver): + mock_driver = mock.MagicMock() + mock_driver.name = 'noop_driver' + mock_get_driver.return_value = mock_driver + + req_dict = [] + body = {self.root_tag_list: req_dict} + path = self.MEMBERS_PATH.format(pool_id=self.pool_id) + 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) + + self.assertEqual([], returned_members) + + mock_provider.assert_called_once_with(u'noop_driver', + mock_driver.member_batch_update, + self.pool_id, []) 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 cbe17c3fc4..a05f06bbf0 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 @@ -346,7 +346,8 @@ class TestAmphoraDriver(base.TestRpc): 'protocol_port': 80, 'pool_id': self.sample_data.pool1_id} - self.amp_driver.member_batch_update(prov_members) + self.amp_driver.member_batch_update( + self.sample_data.pool1_id, prov_members) payload = {'old_member_ids': [self.sample_data.member1_id], 'new_member_ids': [self.sample_data.member3_id], @@ -380,13 +381,27 @@ class TestAmphoraDriver(base.TestRpc): 'protocol_port': 80, 'pool_id': self.sample_data.pool1_id} - self.amp_driver.member_batch_update(prov_members) + self.amp_driver.member_batch_update( + self.sample_data.pool1_id, prov_members) payload = {'old_member_ids': [self.sample_data.member1_id], 'new_member_ids': [self.sample_data.member3_id], 'updated_members': [update_mem_dict]} mock_cast.assert_called_with({}, 'batch_update_members', **payload) + @mock.patch('octavia.db.api.get_session') + @mock.patch('octavia.db.repositories.PoolRepository.get') + @mock.patch('oslo_messaging.RPCClient.cast') + def test_member_batch_update_clear_already_empty( + self, mock_cast, mock_pool_get, mock_session): + 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() + # Health Monitor @mock.patch('oslo_messaging.RPCClient.cast') def test_health_monitor_create(self, mock_cast): @@ -441,7 +456,8 @@ class TestAmphoraDriver(base.TestRpc): 'protocol_port': 80, 'pool_id': self.sample_data.pool1_id} - self.amp_driver.member_batch_update(prov_members) + self.amp_driver.member_batch_update( + self.sample_data.pool1_id, prov_members) payload = {'old_member_ids': [self.sample_data.member1_id], 'new_member_ids': [self.sample_data.member3_id], @@ -479,7 +495,7 @@ class TestAmphoraDriver(base.TestRpc): self.assertRaises(exceptions.UnsupportedOptionError, self.amp_driver.member_batch_update, - prov_members) + self.sample_data.pool1_id, prov_members) @mock.patch('oslo_messaging.RPCClient.cast') def test_health_monitor_update(self, mock_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 026e090065..b53a2612c0 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 @@ -346,7 +346,8 @@ class TestAmphoraDriver(base.TestRpc): 'protocol_port': 80, 'pool_id': self.sample_data.pool1_id} - self.amp_driver.member_batch_update(prov_members) + self.amp_driver.member_batch_update( + self.sample_data.pool1_id, prov_members) payload = {'old_member_ids': [self.sample_data.member1_id], 'new_member_ids': [self.sample_data.member3_id], @@ -380,13 +381,27 @@ class TestAmphoraDriver(base.TestRpc): 'protocol_port': 80, 'pool_id': self.sample_data.pool1_id} - self.amp_driver.member_batch_update(prov_members) + self.amp_driver.member_batch_update( + self.sample_data.pool1_id, prov_members) payload = {'old_member_ids': [self.sample_data.member1_id], 'new_member_ids': [self.sample_data.member3_id], 'updated_members': [update_mem_dict]} mock_cast.assert_called_with({}, 'batch_update_members', **payload) + @mock.patch('octavia.db.api.get_session') + @mock.patch('octavia.db.repositories.PoolRepository.get') + @mock.patch('oslo_messaging.RPCClient.cast') + def test_member_batch_update_clear_already_empty( + self, mock_cast, mock_pool_get, mock_session): + 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() + # Health Monitor @mock.patch('oslo_messaging.RPCClient.cast') def test_health_monitor_create(self, mock_cast): @@ -441,7 +456,8 @@ class TestAmphoraDriver(base.TestRpc): 'protocol_port': 80, 'pool_id': self.sample_data.pool1_id} - self.amp_driver.member_batch_update(prov_members) + self.amp_driver.member_batch_update( + self.sample_data.pool1_id, prov_members) payload = {'old_member_ids': [self.sample_data.member1_id], 'new_member_ids': [self.sample_data.member3_id], @@ -479,7 +495,7 @@ class TestAmphoraDriver(base.TestRpc): self.assertRaises(exceptions.UnsupportedOptionError, self.amp_driver.member_batch_update, - prov_members) + self.sample_data.pool1_id, prov_members) @mock.patch('oslo_messaging.RPCClient.cast') def test_health_monitor_update(self, mock_cast): diff --git a/octavia/tests/unit/api/drivers/test_provider_noop_driver.py b/octavia/tests/unit/api/drivers/test_provider_noop_driver.py index b372242e15..d3fe818888 100644 --- a/octavia/tests/unit/api/drivers/test_provider_noop_driver.py +++ b/octavia/tests/unit/api/drivers/test_provider_noop_driver.py @@ -230,7 +230,7 @@ class TestNoopProviderDriver(base.TestCase): self.driver.driver.driverconfig[self.member_id]) def test_member_batch_update(self): - self.driver.member_batch_update([self.ref_member]) + self.driver.member_batch_update(self.pool_id, [self.ref_member]) self.assertEqual((self.ref_member, 'member_batch_update'), self.driver.driver.driverconfig[self.member_id])