From fb121e59ae33401913b69df476a85c010deb0040 Mon Sep 17 00:00:00 2001 From: Fernando Royo Date: Mon, 14 Feb 2022 19:56:20 +0100 Subject: [PATCH] Retry logical switch associations to load balancers On load-balancer creation, or other operations related to association of the load balancer to the logical router, all logical switches associated with the logical router in the target network are also associated with the load-balancer. When the topology includes multiple subnets, it may happen that the operation over the load-balancer match in time with the removal of some of the subnets. The creation of the load-balancer is a transactional atomic process. This patch is splitting such transactions and retrying in case of the above error. In case the attempts are exhausted and the error remains, we evaluate command by command, in case LsLbAdd or LsLbDel of an Ls associated to the Lr, we can omit the error and go forward. Depends-On: https://review.opendev.org/c/openstack/ovn-octavia-provider/+/834454 Closes-Bug: #1963921 Change-Id: I2c7e0c677f0687990bbd71b3f8d511051dbe0359 (cherry picked from commit 1a62902b5601c764da659f13242131828b4078ca) --- ovn_octavia_provider/common/constants.py | 1 + ovn_octavia_provider/common/utils.py | 11 +- ovn_octavia_provider/helper.py | 181 ++++++--- ovn_octavia_provider/tests/functional/base.py | 71 ++++ .../tests/functional/test_driver.py | 10 + .../tests/unit/test_helper.py | 344 +++++++++++++++--- 6 files changed, 524 insertions(+), 94 deletions(-) diff --git a/ovn_octavia_provider/common/constants.py b/ovn_octavia_provider/common/constants.py index 9e2f916a..5a4aa234 100644 --- a/ovn_octavia_provider/common/constants.py +++ b/ovn_octavia_provider/common/constants.py @@ -15,6 +15,7 @@ from octavia_lib.common import constants # TODO(mjozefcz): Use those variables from neutron-lib once released. LRP_PREFIX = "lrp-" +OVN_NAME_PREFIX = "neutron-" LB_VIP_PORT_PREFIX = "ovn-lb-vip-" OVN_PORT_NAME_EXT_ID_KEY = 'neutron:port_name' OVN_ROUTER_NAME_EXT_ID_KEY = 'neutron:router_name' diff --git a/ovn_octavia_provider/common/utils.py b/ovn_octavia_provider/common/utils.py index 49bd0217..a4a7f2b7 100644 --- a/ovn_octavia_provider/common/utils.py +++ b/ovn_octavia_provider/common/utils.py @@ -15,13 +15,22 @@ from oslo_utils import netutils from ovn_octavia_provider.common import constants +def ovn_uuid(name): + # Get the UUID of a neutron OVN entry (neutron-) + return name.replace(constants.OVN_NAME_PREFIX, '') + + def ovn_name(id): # The name of the OVN entry will be neutron- # This is due to the fact that the OVN application checks if the name # is a UUID. If so then there will be no matches. # We prefix the UUID to enable us to use the Neutron UUID when # updating, deleting etc. - return 'neutron-%s' % id + # To be sure that just one prefix is used, we will check it before + # return concatenation. + if not id.startswith(constants.OVN_NAME_PREFIX): + return constants.OVN_NAME_PREFIX + '%s' % id + return id def ovn_lrouter_port_name(id): diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py index 1a221691..49b0cf08 100644 --- a/ovn_octavia_provider/helper.py +++ b/ovn_octavia_provider/helper.py @@ -29,6 +29,8 @@ from oslo_log import log as logging from oslo_serialization import jsonutils from ovs.stream import Stream from ovsdbapp.backend.ovs_idl import idlutils +from ovsdbapp.schema.ovn_northbound import commands as cmd + import tenacity from ovn_octavia_provider.common import clients @@ -200,7 +202,6 @@ class OvnProviderHelper(): # Network and Router references as pushed in the patch # https://github.com/openvswitch/ovs/commit # /612f80fa8ebf88dad2e204364c6c02b451dca36c - commands = [] network = info['network'] router = info['router'] @@ -210,14 +211,28 @@ class OvnProviderHelper(): r_lb = set(router.load_balancer) - nw_lb # Delete all LB on N/W from Router for nlb in nw_lb: - commands.extend(self._update_lb_to_lr_association(nlb, router, - delete=True)) + try: + self._update_lb_to_lr_association(nlb, router, delete=True) + except idlutils.RowNotFound: + LOG.warning("The disassociation of loadbalancer %s to the " + "logical router %s failed, trying step by step", + nlb.uuid, router.uuid) + self._update_lb_to_lr_association_by_step( + nlb, router, delete=True) + # Delete all LB on Router from N/W for rlb in r_lb: - commands.append(self.ovn_nbdb_api.ls_lb_del( - network.uuid, rlb.uuid)) - if commands: - self._execute_commands(commands) + try: + self._update_lb_to_ls_association( + rlb, + network_id=utils.ovn_uuid(network.name), + associate=False, + update_ls_ref=False) + except idlutils.RowNotFound: + LOG.warning("The disassociation of loadbalancer %s to the " + "logical switch %s failed, just keep going on", + rlb.uuid, utils.ovn_uuid(network.name)) + pass def lb_create_lrp_assoc_handler(self, row): try: @@ -231,21 +246,31 @@ class OvnProviderHelper(): 'info': request_info}) def lb_create_lrp_assoc(self, info): - commands = [] - router_lb = set(info['router'].load_balancer) network_lb = set(info['network'].load_balancer) # Add only those lb to routers which are unique to the network for lb in (network_lb - router_lb): - commands.extend(self._update_lb_to_lr_association( - lb, info['router'])) + try: + self._update_lb_to_lr_association(lb, info['router']) + except idlutils.RowNotFound: + LOG.warning("The association of loadbalancer %s to the " + "logical router %s failed, trying step by step", + lb.uuid, info['router'].uuid) + self._update_lb_to_lr_association_by_step(lb, info['router']) # Add those lb to the network which are unique to the router for lb in (router_lb - network_lb): - commands.append(self.ovn_nbdb_api.ls_lb_add( - info['network'].uuid, lb.uuid, may_exist=True)) - if commands: - self._execute_commands(commands) + try: + self._update_lb_to_ls_association( + lb, + network_id=utils.ovn_uuid(info['network'].name), + associate=True, + update_ls_ref=False) + except idlutils.RowNotFound: + LOG.warning("The association of loadbalancer %s to the " + "logical switch %s failed, just keep going on", + lb.uuid, utils.ovn_uuid(info['network'].name)) + pass def vip_port_update_handler(self, vip_lp): """Handler for VirtualIP port updates. @@ -479,8 +504,24 @@ class OvnProviderHelper(): for command in commands: txn.add(command) + @tenacity.retry( + retry=tenacity.retry_if_exception_type(idlutils.RowNotFound), + wait=tenacity.wait_exponential(), + stop=tenacity.stop_after_attempt(3), + reraise=True) def _update_lb_to_ls_association(self, ovn_lb, network_id=None, - subnet_id=None, associate=True): + subnet_id=None, associate=True, + update_ls_ref=True): + # Note(froyo): Large topologies can change from the time we + # list the ls association commands and the execution, retry + # if this situation arises. + commands = self._get_lb_to_ls_association_commands( + ovn_lb, network_id, subnet_id, associate, update_ls_ref) + self._execute_commands(commands) + + def _get_lb_to_ls_association_commands(self, ovn_lb, network_id=None, + subnet_id=None, associate=True, + update_ls_ref=True): """Update LB association with Logical Switch This function deals with updating the References of Logical Switch @@ -549,10 +590,12 @@ class OvnProviderHelper(): else: ls_refs[ls_name] = ref_ct - 1 - ls_refs = {ovn_const.LB_EXT_IDS_LS_REFS_KEY: jsonutils.dumps(ls_refs)} - commands.append(self.ovn_nbdb_api.db_set( - 'Load_Balancer', ovn_lb.uuid, - ('external_ids', ls_refs))) + if update_ls_ref: + ls_refs = { + ovn_const.LB_EXT_IDS_LS_REFS_KEY: jsonutils.dumps(ls_refs)} + commands.append(self.ovn_nbdb_api.db_set( + 'Load_Balancer', ovn_lb.uuid, + ('external_ids', ls_refs))) return commands @@ -607,12 +650,44 @@ class OvnProviderHelper(): ('external_ids', lr_rf))) return commands + @tenacity.retry( + retry=tenacity.retry_if_exception_type(idlutils.RowNotFound), + wait=tenacity.wait_exponential(), + stop=tenacity.stop_after_attempt(3), + reraise=True) def _update_lb_to_lr_association(self, ovn_lb, ovn_lr, delete=False): + # Note(froyo): Large topologies can change from the time we + # list the ls associated to lr until we execute the + # association command, retry if this situation arises. + commands = self._get_lb_to_lr_association_commands( + ovn_lb, ovn_lr, delete) + self._execute_commands(commands) + + def _update_lb_to_lr_association_by_step(self, ovn_lb, ovn_lr, + delete=False): + # Note(froyo): just to make association commands step by + # step, in order to keep going on when LsLbAdd or LsLbDel + # happen. + commands = self._get_lb_to_lr_association_commands( + ovn_lb, ovn_lr, delete) + for command in commands: + try: + command.execute(check_error=True) + except idlutils.RowNotFound: + if isinstance(command, (cmd.LsLbAddCommand, + cmd.LsLbDelCommand)): + LOG.warning('action lb to ls fail because ls ' + '%s is not found, keep going on...', + getattr(command, 'switch', '')) + else: + raise + + def _get_lb_to_lr_association_commands( + self, ovn_lb, ovn_lr, delete=False): lr_ref = ovn_lb.external_ids.get(ovn_const.LB_EXT_IDS_LR_REF_KEY) if delete: return self._del_lb_to_lr_association(ovn_lb, ovn_lr, lr_ref) - else: - return self._add_lb_to_lr_association(ovn_lb, ovn_lr, lr_ref) + return self._add_lb_to_lr_association(ovn_lb, ovn_lr, lr_ref) def _find_ls_for_lr(self, router): neutron_client = clients.get_neutron_client() @@ -868,16 +943,29 @@ class OvnProviderHelper(): loadbalancer[constants.ID], protocol=protocol) ovn_lb = ovn_lb if protocol else ovn_lb[0] - commands = self._update_lb_to_ls_association( + # NOTE(froyo): This is the association of the lb to the VIP ls + # so this is executed right away + self._update_lb_to_ls_association( ovn_lb, network_id=port['network_id'], - associate=True) + associate=True, update_ls_ref=True) ls_name = utils.ovn_name(port['network_id']) ovn_ls = self.ovn_nbdb_api.ls_get(ls_name).execute( check_error=True) ovn_lr = self._find_lr_of_ls(ovn_ls, subnet.get('gateway_ip')) if ovn_lr: - commands.extend(self._update_lb_to_lr_association( - ovn_lb, ovn_lr)) + try: + # NOTE(froyo): This is the association of the lb to the + # router associated to VIP ls and all ls connected to that + # router we try atomically, if it fails we will go step by + # step, discarding the associations from lb to a + # non-existent ls, but we will demand the association of + # lb to lr + self._update_lb_to_lr_association(ovn_lb, ovn_lr) + except idlutils.RowNotFound: + LOG.warning("The association of loadbalancer %s to the " + "logical router %s failed, trying step by " + "step", ovn_lb.uuid, ovn_lr.uuid) + self._update_lb_to_lr_association_by_step(ovn_lb, ovn_lr) # NOTE(mjozefcz): In case of LS references where passed - # apply LS to the new LB. That could happend in case we @@ -893,11 +981,10 @@ class OvnProviderHelper(): # to duplicate. if ls == ovn_ls.name: continue - commands.extend(self._update_lb_to_ls_association( - ovn_lb, network_id=ls.replace('neutron-', ''), - associate=True)) + self._update_lb_to_ls_association( + ovn_lb, network_id=utils.ovn_uuid(ls), + associate=True, update_ls_ref=True) - self._execute_commands(commands) operating_status = constants.ONLINE # The issue is that since OVN doesnt support any HMs, # we ideally should never put the status as 'ONLINE' @@ -1024,6 +1111,9 @@ class OvnProviderHelper(): self.ovn_nbdb_api.lr_lb_del(lr.uuid, ovn_lb.uuid, if_exists=True)) commands.append(self.ovn_nbdb_api.lb_del(ovn_lb.uuid)) + + # TODO(froyo): atomic process to execute all commands in a transaction + # if any of them fails the transaction is aborted self._execute_commands(commands) return status @@ -1519,16 +1609,21 @@ class OvnProviderHelper(): external_ids[pool_key] = pool_data[pool_key] commands.extend(self._refresh_lb_vips(ovn_lb.uuid, external_ids)) + # Note (froyo): commands are now splitted to separate atomic process, + # leaving outside the not mandatory ones to allow add_member + # finish correctly + self._execute_commands(commands) + subnet_id = member[constants.SUBNET_ID] - commands.extend( - self._update_lb_to_ls_association( - ovn_lb, subnet_id=subnet_id, associate=True)) + self._update_lb_to_ls_association( + ovn_lb, subnet_id=subnet_id, associate=True, update_ls_ref=True) # Make sure that all logical switches related to logical router # are associated with the load balancer. This is needed to handle # potential race that happens when lrp and lb are created at the # same time. neutron_client = clients.get_neutron_client() + ovn_lr = None try: subnet = neutron_client.show_subnet(subnet_id) ls_name = utils.ovn_name(subnet['subnet']['network_id']) @@ -1536,15 +1631,20 @@ class OvnProviderHelper(): check_error=True) ovn_lr = self._find_lr_of_ls( ovn_ls, subnet['subnet'].get('gateway_ip')) - if ovn_lr: - commands.extend(self._update_lb_to_lr_association( - ovn_lb, ovn_lr)) except n_exc.NotFound: pass except idlutils.RowNotFound: pass - self._execute_commands(commands) + if ovn_lr: + try: + self._update_lb_to_lr_association(ovn_lb, ovn_lr) + except idlutils.RowNotFound: + LOG.warning("The association of loadbalancer %s to the " + "logical router %s failed, trying step by step", + ovn_lb.uuid, ovn_lr.uuid) + self._update_lb_to_lr_association_by_step(ovn_lb, ovn_lr) + return member_info def member_create(self, member): @@ -1614,11 +1714,10 @@ class OvnProviderHelper(): external_ids[pool_key] = ",".join(existing_members) commands.extend( self._refresh_lb_vips(ovn_lb.uuid, external_ids)) - commands.extend( - self._update_lb_to_ls_association( - ovn_lb, subnet_id=member.get(constants.SUBNET_ID), - associate=False)) self._execute_commands(commands) + self._update_lb_to_ls_association( + ovn_lb, subnet_id=member.get(constants.SUBNET_ID), + associate=False, update_ls_ref=True) return pool_status else: msg = f"Member {member[constants.ID]} not found in the pool" diff --git a/ovn_octavia_provider/tests/functional/base.py b/ovn_octavia_provider/tests/functional/base.py index a92c4eab..f4fed357 100644 --- a/ovn_octavia_provider/tests/functional/base.py +++ b/ovn_octavia_provider/tests/functional/base.py @@ -319,6 +319,77 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, expected_lbs = self._make_expected_lbs(lb_data) self._validate_loadbalancers(expected_lbs) + def _create_load_balancer_custom_lr_ls_and_validate( + self, admin_state_up=True, create_router=True, + force_retry_ls_to_lr_assoc=True): + + self._o_driver_lib.update_loadbalancer_status.reset_mock() + r_id = self._create_router('r1') if create_router else None + + net_info = [] + net_info.append(self._create_net("n1", "10.0.1.0/24", r_id)) + net_info.append(self._create_net("n2", "10.0.2.0/24", r_id)) + net_info.append(self._create_net("n3", "10.0.3.0/24", r_id)) + + lb_data = {} + lb_data['model'] = self._create_lb_model( + vip=net_info[0][2], + vip_network_id=net_info[0][0], + vip_subnet_id=net_info[0][1], + vip_port_id=net_info[0][3], + admin_state_up=admin_state_up) + + lb_data[ovn_const.LB_EXT_IDS_LR_REF_KEY] = \ + (ovn_const.LR_REF_KEY_HEADER + r_id) + lb_data['vip_net_info'] = net_info[0] + lb_data[ovn_const.LB_EXT_IDS_LS_REFS_KEY] = {} + lb_data['listeners'] = [] + lb_data['pools'] = [] + self._update_ls_refs(lb_data, net_info[0][0]) + ls = [ovn_const.LR_REF_KEY_HEADER + net[0] for net in net_info] + + if force_retry_ls_to_lr_assoc: + ls_foo = copy.deepcopy(ls) + ls_foo.append('neutron-foo') + self.ovn_driver._ovn_helper._find_ls_for_lr = mock.MagicMock() + self.ovn_driver._ovn_helper._find_ls_for_lr.side_effect = \ + [ls_foo, ls] + + self.ovn_driver.loadbalancer_create(lb_data['model']) + + name = '%s%s' % (ovn_const.LB_VIP_PORT_PREFIX, + lb_data['model'].loadbalancer_id) + self.driver.update_port( + self.context, net_info[0][3], {'port': {'name': name}}) + + if lb_data['model'].admin_state_up: + expected_status = { + 'loadbalancers': [{"id": lb_data['model'].loadbalancer_id, + "provisioning_status": "ACTIVE", + "operating_status": o_constants.ONLINE}] + } + else: + expected_status = { + 'loadbalancers': [{"id": lb_data['model'].loadbalancer_id, + "provisioning_status": "ACTIVE", + "operating_status": o_constants.OFFLINE}] + } + self._wait_for_status_and_validate(lb_data, [expected_status]) + self.assertTrue( + self._is_lb_associated_to_ls( + lb_data['model'].loadbalancer_id, + ovn_const.LR_REF_KEY_HEADER + net_info[0][0])) + + # NOTE(froyo): Just to check all net connected to lr have a + # reference to lb + for net_id in ls: + self.assertTrue( + self._is_lb_associated_to_ls( + lb_data['model'].loadbalancer_id, + net_id)) + + return lb_data + def _create_load_balancer_and_validate(self, lb_info, admin_state_up=True, only_model=False, diff --git a/ovn_octavia_provider/tests/functional/test_driver.py b/ovn_octavia_provider/tests/functional/test_driver.py index 421821ce..b31e451c 100644 --- a/ovn_octavia_provider/tests/functional/test_driver.py +++ b/ovn_octavia_provider/tests/functional/test_driver.py @@ -34,6 +34,16 @@ class TestOvnOctaviaProviderDriver(ovn_base.TestOvnOctaviaBase): 'cidr': '10.0.0.0/24'}, admin_state_up=False) self._delete_load_balancer_and_validate(lb_data) + def test_create_lb_custom_network(self): + self._create_load_balancer_custom_lr_ls_and_validate( + admin_state_up=True, create_router=True, + force_retry_ls_to_lr_assoc=False) + + def test_create_lb_custom_network_retry(self): + self._create_load_balancer_custom_lr_ls_and_validate( + admin_state_up=True, create_router=True, + force_retry_ls_to_lr_assoc=True) + def test_delete_lb_on_nonexisting_lb(self): # LoadBalancer doesnt exist anymore, so just create a model and delete lb_data = self._create_load_balancer_and_validate( diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py index ad1d2403..966bd63a 100644 --- a/ovn_octavia_provider/tests/unit/test_helper.py +++ b/ovn_octavia_provider/tests/unit/test_helper.py @@ -143,11 +143,27 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): '_update_lb_to_ls_association', return_value=[]) self._update_lb_to_ls_association.start() + self._get_lb_to_ls_association_commands = mock.patch.object( + self.helper, + '_get_lb_to_ls_association_commands', + return_value=[]) + self._get_lb_to_ls_association_commands.start() self._update_lb_to_lr_association = mock.patch.object( self.helper, '_update_lb_to_lr_association', return_value=[]) self._update_lb_to_lr_association.start() + self._get_lb_to_lr_association_commands = mock.patch.object( + self.helper, + '_get_lb_to_lr_association_commands', + return_value=[]) + self._get_lb_to_lr_association_commands.start() + self._update_lb_to_lr_association_by_step = \ + mock.patch.object( + self.helper, + '_update_lb_to_lr_association_by_step', + return_value=[]) + self._update_lb_to_lr_association_by_step.start() # NOTE(mjozefcz): Create foo router and network. net_id = uuidutils.generate_uuid() @@ -491,8 +507,10 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): selection_fields=['ip_src', 'ip_dst', 'tp_src', 'tp_dst']) self.helper._update_lb_to_ls_association.assert_has_calls([ mock.call(self.ovn_lb, associate=True, - network_id=self.lb['vip_network_id']), - mock.call(self.ovn_lb, associate=True, network_id='foo')]) + network_id=self.lb['vip_network_id'], + update_ls_ref=True), + mock.call(self.ovn_lb, associate=True, network_id='foo', + update_ls_ref=True)]) def test_lb_create_on_multi_protocol_UDP(self): self._test_lb_create_on_multi_protocol('UDP') @@ -1579,22 +1597,21 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper._update_lb_to_lr_association.assert_not_called() mock_execute.assert_not_called() - @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') - def test_lb_delete_lrp_assoc_no_net_lb_r_lb(self, mock_execute): + def test_lb_delete_lrp_assoc_no_net_lb_r_lb(self): info = { 'network': self.network, 'router': self.router, } self.network.load_balancer = [] self.helper.lb_delete_lrp_assoc(info) - expected = [ - self.helper.ovn_nbdb_api.ls_lb_del( - self.network.uuid, - self.router.load_balancer[0].uuid - ), - ] + self.helper._update_lb_to_lr_association.assert_not_called() - mock_execute.assert_called_once_with(expected) + self.helper._update_lb_to_ls_association.assert_called_once_with( + self.router.load_balancer[0], + network_id=info['network'].uuid, + associate=False, + update_ls_ref=False + ) @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') def test_lb_delete_lrp_assoc_net_lb_no_r_lb(self, mock_execute): @@ -1609,8 +1626,23 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.network.load_balancer[0], self.router, delete=True ) - @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') - def test_lb_delete_lrp_assoc(self, mock_execute): + def test_lb_delete_lrp_assoc_r_lb_exception(self): + info = { + 'network': self.network, + 'router': self.router, + } + self.helper._update_lb_to_ls_association.side_effect = [ + idlutils.RowNotFound] + with self.assertLogs(level='WARN') as cm: + self.helper.lb_delete_lrp_assoc(info) + self.assertEqual( + cm.output, + ['WARNING:ovn_octavia_provider.helper:' + 'The disassociation of loadbalancer ' + '%s to the logical switch %s failed, just keep going on' + % (self.router.load_balancer[0].uuid, self.network.uuid)]) + + def test_lb_delete_lrp_assoc(self): info = { 'network': self.network, 'router': self.router, @@ -1619,13 +1651,28 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper._update_lb_to_lr_association.assert_called_once_with( self.network.load_balancer[0], self.router, delete=True ) - expected = [ - self.helper.ovn_nbdb_api.ls_lb_del( - self.network.uuid, - self.router.load_balancer[0].uuid - ), - ] - mock_execute.assert_called_once_with(expected) + self.helper._update_lb_to_ls_association.assert_called_once_with( + self.router.load_balancer[0], + network_id=self.network.uuid, + associate=False, update_ls_ref=False + ) + + def test_lb_delete_lrp_assoc_ls_by_step(self): + self._update_lb_to_ls_association.stop() + info = { + 'network': self.network, + 'router': self.router, + } + self.helper._update_lb_to_lr_association.side_effect = [ + idlutils.RowNotFound] + self.helper.lb_delete_lrp_assoc(info) + self.helper._update_lb_to_lr_association.assert_called_once_with( + self.network.load_balancer[0], self.router, delete=True + ) + self.helper._update_lb_to_lr_association_by_step \ + .assert_called_once_with( + self.network.load_balancer[0], + self.router, delete=True) def test_lb_create_lrp_assoc_handler(self): lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row() @@ -1648,8 +1695,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper.lb_create_lrp_assoc_handler(lrp) self.mock_add_request.assert_not_called() - @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') - def test_lb_create_lrp_assoc(self, mock_execute): + def test_lb_create_lrp_assoc(self): info = { 'network': self.network, 'router': self.router, @@ -1658,16 +1704,41 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper._update_lb_to_lr_association.assert_called_once_with( self.network.load_balancer[0], self.router ) - expected = [ - self.helper.ovn_nbdb_api.ls_lb_add( - self.network.uuid, - self.router.load_balancer[0].uuid - ), - ] - mock_execute.assert_called_once_with(expected) - @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') - def test_lb_create_lrp_assoc_uniq_lb(self, mock_execute): + def test_lb_create_lrp_assoc_r_lb_exception(self): + info = { + 'network': self.network, + 'router': self.router, + } + self.helper._update_lb_to_ls_association.side_effect = [ + idlutils.RowNotFound] + with self.assertLogs(level='WARN') as cm: + self.helper.lb_create_lrp_assoc(info) + self.assertEqual( + cm.output, + ['WARNING:ovn_octavia_provider.helper:' + 'The association of loadbalancer ' + '%s to the logical switch %s failed, just keep going on' + % (self.router.load_balancer[0].uuid, self.network.uuid)]) + + def test_lb_create_lrp_assoc_ls_by_step(self): + self._update_lb_to_ls_association.stop() + info = { + 'network': self.network, + 'router': self.router, + } + self.helper._update_lb_to_lr_association.side_effect = [ + idlutils.RowNotFound] + self.helper.lb_create_lrp_assoc(info) + self.helper._update_lb_to_lr_association.assert_called_once_with( + self.network.load_balancer[0], self.router + ) + self.helper._update_lb_to_lr_association_by_step \ + .assert_called_once_with( + self.network.load_balancer[0], + self.router) + + def test_lb_create_lrp_assoc_uniq_lb(self): info = { 'network': self.network, 'router': self.router, @@ -1676,7 +1747,6 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.network.load_balancer = self.router.load_balancer self.helper.lb_create_lrp_assoc(info) self.helper._update_lb_to_lr_association.assert_not_called() - mock_execute.assert_not_called() def test__find_lb_in_ls(self): net_lb = self.helper._find_lb_in_ls(self.network) @@ -1744,9 +1814,10 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): ovn_helper.OvnProviderHelper, '_del_lb_to_lr_association') @mock.patch.object( ovn_helper.OvnProviderHelper, '_add_lb_to_lr_association') - def test__update_lb_to_lr_association(self, add, delete): - self._update_lb_to_lr_association.stop() - self.helper._update_lb_to_lr_association(self.ref_lb1, self.router) + def test__get_lb_to_lr_association_commands(self, add, delete): + self._get_lb_to_lr_association_commands.stop() + self.helper._get_lb_to_lr_association_commands( + self.ref_lb1, self.router) lr_ref = self.ref_lb1.external_ids.get( ovn_const.LB_EXT_IDS_LR_REF_KEY) add.assert_called_once_with(self.ref_lb1, self.router, lr_ref) @@ -1756,9 +1827,39 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): ovn_helper.OvnProviderHelper, '_del_lb_to_lr_association') @mock.patch.object( ovn_helper.OvnProviderHelper, '_add_lb_to_lr_association') - def test__update_lb_to_lr_association_delete(self, add, delete): - self._update_lb_to_lr_association.stop() - self.helper._update_lb_to_lr_association( + def test__get_lb_to_lr_association_commands_delete(self, add, delete): + self._get_lb_to_lr_association_commands.stop() + self.helper._get_lb_to_lr_association_commands( + self.ref_lb1, self.router, delete=True) + lr_ref = self.ref_lb1.external_ids.get( + ovn_const.LB_EXT_IDS_LR_REF_KEY) + add.assert_not_called() + delete.assert_called_once_with(self.ref_lb1, self.router, lr_ref) + + @mock.patch.object( + ovn_helper.OvnProviderHelper, '_del_lb_to_lr_association') + @mock.patch.object( + ovn_helper.OvnProviderHelper, '_add_lb_to_lr_association') + def test__get_lb_to_lr_association_commands_by_step( + self, add, delete): + self._update_lb_to_lr_association_by_step.stop() + self._get_lb_to_lr_association_commands.stop() + self.helper._update_lb_to_lr_association_by_step( + self.ref_lb1, self.router) + lr_ref = self.ref_lb1.external_ids.get( + ovn_const.LB_EXT_IDS_LR_REF_KEY) + add.assert_called_once_with(self.ref_lb1, self.router, lr_ref) + delete.assert_not_called() + + @mock.patch.object( + ovn_helper.OvnProviderHelper, '_del_lb_to_lr_association') + @mock.patch.object( + ovn_helper.OvnProviderHelper, '_add_lb_to_lr_association') + def test__get_lb_to_lr_association_commands_by_step_delete( + self, add, delete): + self._update_lb_to_lr_association_by_step.stop() + self._get_lb_to_lr_association_commands.stop() + self.helper._update_lb_to_lr_association_by_step( self.ref_lb1, self.router, delete=True) lr_ref = self.ref_lb1.external_ids.get( ovn_const.LB_EXT_IDS_LR_REF_KEY) @@ -1916,17 +2017,19 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): values.assert_not_called()) self.assertIsNone(returned_lr) - def test__update_lb_to_ls_association_empty_network_and_subnet(self): - self._update_lb_to_ls_association.stop() - returned_commands = self.helper._update_lb_to_ls_association( - self.ref_lb1, associate=True) + def test__get_lb_to_ls_association_command_empty_network_and_subnet(self): + self._get_lb_to_ls_association_commands.stop() + returned_commands = self.helper._get_lb_to_ls_association_commands( + self.ref_lb1, associate=True, update_ls_ref=True) self.assertListEqual(returned_commands, []) def test__update_lb_to_ls_association_network(self): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid, associate=True) + self.ref_lb1, network_id=self.network.uuid, + associate=True, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( self.network.name) @@ -1934,9 +2037,22 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper.ovn_nbdb_api.db_set.assert_called_once_with( 'Load_Balancer', self.ref_lb1.uuid, ('external_ids', ls_refs)) + def test__update_lb_to_ls_association_network_no_update_ls_ref(self): + self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() + + self.helper._update_lb_to_ls_association( + self.ref_lb1, network_id=self.network.uuid, + associate=True, update_ls_ref=False) + + self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( + self.network.name) + self.helper.ovn_nbdb_api.db_set.assert_not_called() + @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') def test__update_lb_to_ls_association_subnet(self, net_cli): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() subnet = fakes.FakeSubnet.create_one_subnet( attrs={'id': 'foo_subnet_id', 'name': 'foo_subnet_name', @@ -1944,18 +2060,21 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): net_cli.return_value.show_subnet.return_value = { 'subnet': subnet} self.helper._update_lb_to_ls_association( - self.ref_lb1, subnet_id=subnet.id, associate=True) + self.ref_lb1, subnet_id=subnet.id, + associate=True, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( 'neutron-foo_network_id') def test__update_lb_to_ls_association_empty_ls_refs(self): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() (self.helper.ovn_nbdb_api.ls_get.return_value.execute. return_value) = self.network self.ref_lb1.external_ids.pop('ls_refs') self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid) + self.ref_lb1, network_id=self.network.uuid, + update_ls_ref=True) self.helper.ovn_nbdb_api.ls_lb_add.assert_called_once_with( self.network.uuid, self.ref_lb1.uuid, may_exist=True) @@ -1965,11 +2084,13 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): def test__update_lb_to_ls_association_no_ls(self): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() (self.helper.ovn_nbdb_api.ls_get.return_value.execute. side_effect) = [idlutils.RowNotFound] - returned_commands = self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid) + returned_commands = self.helper._get_lb_to_ls_association_commands( + self.ref_lb1, network_id=self.network.uuid, + update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( self.network.name) @@ -1977,11 +2098,13 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): def test__update_lb_to_ls_association_network_disassociate(self): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() (self.helper.ovn_nbdb_api.ls_get.return_value.execute. return_value) = self.network self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid, associate=False) + self.ref_lb1, network_id=self.network.uuid, + associate=False, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( self.network.name) @@ -1991,13 +2114,65 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper.ovn_nbdb_api.ls_lb_del.assert_called_once_with( self.network.uuid, self.ref_lb1.uuid, if_exists=True) + def test__update_lb_to_ls_association_net_disassoc_no_update_ls_ref(self): + self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() + (self.helper.ovn_nbdb_api.ls_get.return_value.execute. + return_value) = self.network + + self.helper._update_lb_to_ls_association( + self.ref_lb1, network_id=self.network.uuid, + associate=False, update_ls_ref=False) + + self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( + self.network.name) + self.helper.ovn_nbdb_api.db_set.assert_not_called() + self.helper.ovn_nbdb_api.ls_lb_del.assert_called_once_with( + self.network.uuid, self.ref_lb1.uuid, if_exists=True) + + def test__update_lb_to_ls_association_dissasoc_net_not_assoc(self): + self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() + (self.helper.ovn_nbdb_api.ls_get.return_value.execute. + return_value) = self.network + + self.helper._update_lb_to_ls_association( + self.ref_lb1, network_id='foo', + associate=False, update_ls_ref=False) + + self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( + 'neutron-foo') + self.helper.ovn_nbdb_api.db_set.assert_not_called() + self.helper.ovn_nbdb_api.ls_lb_del.assert_not_called() + + def test__update_lb_to_ls_association_net_ls_ref_wrong_format(self): + self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() + + (self.helper.ovn_nbdb_api.ls_get.return_value.execute. + return_value) = self.network + + self.ref_lb1.external_ids.update({ + ovn_const.LB_EXT_IDS_LS_REFS_KEY: + '{\"neutron-%s\"}'}) + + self.helper._update_lb_to_ls_association( + self.ref_lb1, network_id=self.network.uuid, + associate=False, update_ls_ref=False) + + self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( + self.network.name) + self.helper.ovn_nbdb_api.db_set.assert_not_called() + def test__update_lb_to_ls_association_network_dis_ls_not_found(self): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() (self.helper.ovn_nbdb_api.ls_get.return_value.execute. side_effect) = [idlutils.RowNotFound] self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid, associate=False) + self.ref_lb1, network_id=self.network.uuid, + associate=False, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( self.network.name) @@ -2011,10 +2186,12 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self, net_cli): net_cli.return_value.show_subnet.side_effect = n_exc.NotFound self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() (self.helper.ovn_nbdb_api.ls_get.return_value.execute. return_value) = self.network self.helper._update_lb_to_ls_association( - self.ref_lb1, subnet_id='foo', associate=False) + self.ref_lb1, subnet_id='foo', + associate=False, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_not_called() self.helper.ovn_nbdb_api.db_set.assert_not_called() self.helper.ovn_nbdb_api.ls_lb_del.assert_not_called() @@ -2026,13 +2203,15 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.ref_lb1.external_ids.pop('ls_refs') self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid, associate=False) + self.ref_lb1, network_id=self.network.uuid, + associate=False, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_lb_del.assert_not_called() self.helper.ovn_nbdb_api.db_set.assert_not_called() def test__update_lb_to_ls_association_disassoc_multiple_refs(self): self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() (self.helper.ovn_nbdb_api.ls_get.return_value.execute. return_value) = self.network # multiple refs @@ -2040,7 +2219,8 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.ref_lb1.external_ids.update(ls_refs) self.helper._update_lb_to_ls_association( - self.ref_lb1, network_id=self.network.uuid, associate=False) + self.ref_lb1, network_id=self.network.uuid, + associate=False, update_ls_ref=True) self.helper.ovn_nbdb_api.ls_get.assert_called_once_with( self.network.name) @@ -2048,6 +2228,26 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper.ovn_nbdb_api.db_set.assert_called_once_with( 'Load_Balancer', self.ref_lb1.uuid, ('external_ids', exp_ls_refs)) + @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') + def test__update_lb_to_ls_association_retry(self, execute): + self._update_lb_to_ls_association.stop() + self._get_lb_to_ls_association_commands.stop() + self.helper._update_lb_to_ls_association( + self.ref_lb1, network_id=self.network.uuid) + expected = self.helper._get_lb_to_ls_association_commands( + self.ref_lb1, network_id=self.network.uuid) + execute.assert_called_once_with(expected) + + @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') + def test__update_lb_to_ls_association_retry_failed(self, execute): + execute.side_effect = [idlutils.RowNotFound for _ in range(4)] + self._update_lb_to_ls_association.stop() + self.assertRaises( + idlutils.RowNotFound, + self.helper._update_lb_to_ls_association, + self.ref_lb1, + network_id=self.network.uuid) + def test_logical_switch_port_update_event_vip_port(self): self.switch_port_event = ovn_event.LogicalSwitchPortUpdateEvent( self.helper) @@ -2117,6 +2317,46 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.switch_port_event.run(mock.ANY, row, mock.ANY) self.mock_add_request.assert_not_called() + @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') + def test__update_lb_to_lr_association_retry(self, execute): + self._update_lb_to_lr_association.stop() + self._get_lb_to_lr_association_commands.stop() + self.helper._update_lb_to_lr_association(self.ref_lb1, self.router) + expected = self.helper._get_lb_to_lr_association_commands( + self.ref_lb1, self.router) + execute.assert_called_once_with(expected) + + @mock.patch.object(ovn_helper.OvnProviderHelper, '_execute_commands') + def test__update_lb_to_lr_association_retry_failed(self, execute): + execute.side_effect = [idlutils.RowNotFound for _ in range(4)] + self._update_lb_to_lr_association.stop() + self.assertRaises( + idlutils.RowNotFound, + self.helper._update_lb_to_lr_association, + self.ref_lb1, + self.router) + + def test__update_lb_to_lr_association_by_step(self): + self._get_lb_to_lr_association_commands.stop() + self._update_lb_to_lr_association_by_step.stop() + self.helper._update_lb_to_lr_association_by_step( + self.network.load_balancer[0], + self.router) + self.helper.ovn_nbdb_api.db_set.assert_called() + self.helper.ovn_nbdb_api.lr_lb_add.assert_called() + + def test__update_lb_to_lr_association_by_step_exception_raise( + self): + self._get_lb_to_lr_association_commands.stop() + self._update_lb_to_lr_association_by_step.stop() + (self.helper.ovn_nbdb_api.db_set.return_value.execute. + side_effect) = [idlutils.RowNotFound] + self.assertRaises( + idlutils.RowNotFound, + self.helper._update_lb_to_lr_association_by_step, + self.network.load_balancer[0], + self.router) + @mock.patch('ovn_octavia_provider.helper.OvnProviderHelper.' '_find_ovn_lbs') def test_vip_port_update_handler_multiple_lbs(self, lb):