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):