From 23d743a444a426379499193324723a9c7ab33734 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20J=C3=B3zefczyk?= Date: Mon, 11 May 2020 10:16:56 +0000 Subject: [PATCH] Add support for OVN LB selection fields Prior this patch OVN Octavia provider driver used default 5-tuple-hash algorithm which is pretty similar to SOURCE_IP_PORT. Unfornutelly because of the bug described here [1] it was not clear how 5-tuple-hash works and some inconsistencies between kernel and user space implementations were found. OVN recently added support for selective fields in OVN LB, to explicitly define what fields are being hashed to tackle this problem. This commit adds support for that kind of hashing. If installation of OVN on which OVN Octavia provider is running doesn't support selective fields - it will use old behavior. [1] https://mail.openvswitch.org/pipermail/ovs-discuss/2020-April/049896.html [2] https://github.com/ovn-org/ovn/commit/5af304e7478adcf5ac50ed41e96a55bebebff3e8 Change-Id: I7b4ab99d1be2855e18b186557990c85f170ad548 Closes-Bug: #1871239 --- ovn_octavia_provider/common/constants.py | 8 +++ ovn_octavia_provider/driver.py | 1 + ovn_octavia_provider/helper.py | 34 ++++++++-- ovn_octavia_provider/tests/functional/base.py | 37 +++++++---- .../tests/unit/schemas/ovn-nb.ovsschema | 64 +++++++++++++++++-- .../tests/unit/test_driver.py | 2 + .../tests/unit/test_helper.py | 49 +++++++++++++- 7 files changed, 168 insertions(+), 27 deletions(-) diff --git a/ovn_octavia_provider/common/constants.py b/ovn_octavia_provider/common/constants.py index 2ad3076d..5ae33e09 100644 --- a/ovn_octavia_provider/common/constants.py +++ b/ovn_octavia_provider/common/constants.py @@ -81,3 +81,11 @@ EXCEPTION_MSG = "Exception occurred during %s" # Used in functional tests LR_REF_KEY_HEADER = 'neutron-' + +# LB selection fields to represent LB algorithm +LB_SELECTION_FIELDS_MAP = { + constants.LB_ALGORITHM_SOURCE_IP_PORT: ["ip_dst", "ip_src", + "tp_dst", "tp_src"], + constants.LB_ALGORITHM_SOURCE_IP: ["ip_src", "ip_dst"], + None: ["ip_src", "ip_dst", "tp_src", "tp_dst"], +} diff --git a/ovn_octavia_provider/driver.py b/ovn_octavia_provider/driver.py index aceb8fa0..070b7ab6 100644 --- a/ovn_octavia_provider/driver.py +++ b/ovn_octavia_provider/driver.py @@ -103,6 +103,7 @@ class OvnProviderDriver(driver_base.ProviderDriver): request_info = {'id': pool.pool_id, 'loadbalancer_id': pool.loadbalancer_id, 'protocol': pool.protocol, + 'lb_algorithm': pool.lb_algorithm, 'listener_id': pool.listener_id, 'admin_state_up': admin_state_up} request = {'type': ovn_const.REQ_TYPE_POOL_CREATE, diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py index f3177c8c..f033a3e6 100644 --- a/ovn_octavia_provider/helper.py +++ b/ovn_octavia_provider/helper.py @@ -363,12 +363,17 @@ class OvnProviderHelper(object): raise idlutils.RowNotFound(table='Load_Balancer', col='name', match=lb_id) - def _get_or_create_ovn_lb(self, lb_id, protocol, admin_state_up): + def _get_or_create_ovn_lb( + self, lb_id, protocol, admin_state_up, + lb_algorithm=constants.LB_ALGORITHM_SOURCE_IP_PORT): """Find or create ovn lb with given protocol Find the loadbalancer configured with given protocol or create required if not found """ + # TODO(mjozefcz): For now we support only one LB algorithm. + # As we may extend that in the future we would need to + # look here also for lb_algorithm, along with protocol. # Make sure that its lowercase - OVN NBDB stores lowercases # for this field. protocol = protocol.lower() @@ -395,6 +400,7 @@ class OvnProviderHelper(object): lb_info = { 'id': lb_id, 'protocol': protocol, + constants.LB_ALGORITHM: lb_algorithm, 'vip_address': ovn_lbs[0].external_ids.get( ovn_const.LB_EXT_IDS_VIP_KEY), 'vip_port_id': @@ -735,6 +741,14 @@ class OvnProviderHelper(object): return True return False + def _are_selection_fields_supported(self): + return self.ovn_nbdb_api.is_col_present( + 'Load_Balancer', 'selection_fields') + + @staticmethod + def _get_selection_keys(lb_algorithm): + return ovn_const.LB_SELECTION_FIELDS_MAP[lb_algorithm] + def check_lb_protocol(self, lb_id, listener_protocol): ovn_lb = self._find_ovn_lbs(lb_id, protocol=listener_protocol) if not ovn_lb: @@ -778,12 +792,18 @@ class OvnProviderHelper(object): lr_ref = loadbalancer.get(ovn_const.LB_EXT_IDS_LR_REF_KEY) if lr_ref: external_ids[ovn_const.LB_EXT_IDS_LR_REF_KEY] = lr_ref - + # In case we have LB algoritm set + lb_algorithm = loadbalancer.get(constants.LB_ALGORITHM) + kwargs = { + 'name': loadbalancer[constants.ID], + 'protocol': protocol, + 'external_ids': external_ids} + if self._are_selection_fields_supported(): + kwargs['selection_fields'] = self._get_selection_keys(lb_algorithm) try: self.ovn_nbdb_api.db_create( - 'Load_Balancer', name=loadbalancer[constants.ID], - protocol=protocol, - external_ids=external_ids).execute(check_error=True) + 'Load_Balancer', + **kwargs).execute(check_error=True) ovn_lb = self._find_ovn_lbs( loadbalancer[constants.ID], protocol=protocol) @@ -1208,7 +1228,9 @@ class OvnProviderHelper(object): ovn_lb = self._get_or_create_ovn_lb( pool[constants.LOADBALANCER_ID], pool[constants.PROTOCOL], - pool[constants.ADMIN_STATE_UP]) + pool[constants.ADMIN_STATE_UP], + lb_algorithm=pool[constants.LB_ALGORITHM]) + external_ids = copy.deepcopy(ovn_lb.external_ids) pool_key = self._get_pool_key( pool[constants.ID], is_enabled=pool[constants.ADMIN_STATE_UP]) diff --git a/ovn_octavia_provider/tests/functional/base.py b/ovn_octavia_provider/tests/functional/base.py index 63f56dcb..d6d0e353 100644 --- a/ovn_octavia_provider/tests/functional/base.py +++ b/ovn_octavia_provider/tests/functional/base.py @@ -172,9 +172,13 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, external_ids[ ovn_const.LB_EXT_IDS_LS_REFS_KEY] = jsonutils.loads( ls_refs) - lbs.append({'name': lb.name, 'protocol': lb.protocol, - 'vips': lb.vips, 'external_ids': external_ids}) - + lb_dict = {'name': lb.name, 'protocol': lb.protocol, + 'vips': lb.vips, 'external_ids': external_ids} + try: + lb_dict['selection_fields'] = lb.selection_fields + except AttributeError: + pass + lbs.append(lb_dict) return lbs def _get_loadbalancer_id(self, lb_name): @@ -432,6 +436,12 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, ovn_const.LR_REF_KEY_HEADER + vip_net_id)) def _make_expected_lbs(self, lb_data): + def _get_lb_field_by_protocol(protocol, field='external_ids'): + "Get needed external_ids and pass by reference" + lb = [lb for lb in expected_lbs + if lb.get('protocol') == [protocol]] + return lb[0].get(field) + if not lb_data or not lb_data.get('model'): return [] @@ -454,17 +464,16 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, if len(expected_protocols) == 0: expected_protocols.append(None) - expected_lbs = [{'name': lb_data['model'].loadbalancer_id, - 'protocol': [protocol] if protocol else [], - 'vips': {}, - 'external_ids': copy.deepcopy(external_ids)} - for protocol in expected_protocols] - - def _get_lb_field_by_protocol(protocol, field='external_ids'): - "Get needed external_ids and pass by reference" - lb = [lb for lb in expected_lbs - if lb.get('protocol') == [protocol]] - return lb[0].get(field) + expected_lbs = [] + for protocol in expected_protocols: + lb = {'name': lb_data['model'].loadbalancer_id, + 'protocol': [protocol] if protocol else [], + 'vips': {}, + 'external_ids': copy.deepcopy(external_ids)} + if self.ovn_driver._ovn_helper._are_selection_fields_supported(): + lb['selection_fields'] = ovn_const.LB_SELECTION_FIELDS_MAP[ + o_constants.LB_ALGORITHM_SOURCE_IP_PORT] + expected_lbs.append(lb) # For every connected subnet to the LB set the ref # counter. diff --git a/ovn_octavia_provider/tests/unit/schemas/ovn-nb.ovsschema b/ovn_octavia_provider/tests/unit/schemas/ovn-nb.ovsschema index 2c87cbba..a06972aa 100644 --- a/ovn_octavia_provider/tests/unit/schemas/ovn-nb.ovsschema +++ b/ovn_octavia_provider/tests/unit/schemas/ovn-nb.ovsschema @@ -1,10 +1,11 @@ { "name": "OVN_Northbound", - "version": "5.16.0", - "cksum": "923459061 23095", + "version": "5.23.0", + "cksum": "111023208 25806", "tables": { "NB_Global": { "columns": { + "name": {"type": "string"}, "nb_cfg": {"type": {"key": "integer"}}, "sb_cfg": {"type": {"key": "integer"}}, "hv_cfg": {"type": {"key": "integer"}}, @@ -59,7 +60,12 @@ "min": 0, "max": "unlimited"}}, "external_ids": { "type": {"key": "string", "value": "string", - "min": 0, "max": "unlimited"}}}, + "min": 0, "max": "unlimited"}}, + "forwarding_groups": { + "type": {"key": {"type": "uuid", + "refTable": "Forwarding_Group", + "refType": "strong"}, + "min": 0, "max": "unlimited"}}}, "isRoot": true}, "Logical_Switch_Port": { "columns": { @@ -113,6 +119,18 @@ "min": 0, "max": "unlimited"}}}, "indexes": [["name"]], "isRoot": false}, + "Forwarding_Group": { + "columns": { + "name": {"type": "string"}, + "vip": {"type": "string"}, + "vmac": {"type": "string"}, + "liveness": {"type": "boolean"}, + "external_ids": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}, + "child_port": {"type": {"key": "string", + "min": 1, "max": "unlimited"}}}, + "isRoot": false}, "Address_Set": { "columns": { "name": {"type": "string"}, @@ -150,12 +168,39 @@ "min": 0, "max": "unlimited"}}, "protocol": { "type": {"key": {"type": "string", - "enum": ["set", ["tcp", "udp"]]}, + "enum": ["set", ["tcp", "udp", "sctp"]]}, "min": 0, "max": 1}}, + "health_check": {"type": { + "key": {"type": "uuid", + "refTable": "Load_Balancer_Health_Check", + "refType": "strong"}, + "min": 0, + "max": "unlimited"}}, + "ip_port_mappings": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}, + "selection_fields": { + "type": {"key": {"type": "string", + "enum": ["set", + ["eth_src", "eth_dst", "ip_src", "ip_dst", + "tp_src", "tp_dst"]]}, + "min": 0, "max": "unlimited"}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, "isRoot": true}, + "Load_Balancer_Health_Check": { + "columns": { + "vip": {"type": "string"}, + "options": { + "type": {"key": "string", + "value": "string", + "min": 0, + "max": "unlimited"}}, + "external_ids": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}}, + "isRoot": false}, "ACL": { "columns": { "name": {"type": {"key": {"type": "string", @@ -303,6 +348,9 @@ "ipv6_ra_configs": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}, + "ipv6_prefix": {"type": {"key": "string", + "min": 0, + "max": "unlimited"}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, @@ -330,13 +378,17 @@ "action": {"type": { "key": {"type": "string", "enum": ["set", ["allow", "drop", "reroute"]]}}}, - "nexthop": {"type": {"key": "string", "min": 0, "max": 1}}}, + "nexthop": {"type": {"key": "string", "min": 0, "max": 1}}, + "external_ids": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}}, "isRoot": false}, "NAT": { "columns": { "external_ip": {"type": "string"}, "external_mac": {"type": {"key": "string", "min": 0, "max": 1}}, + "external_port_range": {"type": "string"}, "logical_ip": {"type": "string"}, "logical_port": {"type": {"key": "string", "min": 0, "max": 1}}, @@ -345,6 +397,8 @@ "snat", "dnat_and_snat" ]]}}}, + "options": {"type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, diff --git a/ovn_octavia_provider/tests/unit/test_driver.py b/ovn_octavia_provider/tests/unit/test_driver.py index a215ae36..c0dc1c5e 100644 --- a/ovn_octavia_provider/tests/unit/test_driver.py +++ b/ovn_octavia_provider/tests/unit/test_driver.py @@ -482,6 +482,7 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): 'loadbalancer_id': self.ref_pool.loadbalancer_id, 'listener_id': self.ref_pool.listener_id, 'protocol': self.ref_pool.protocol, + 'lb_algorithm': constants.LB_ALGORITHM_SOURCE_IP_PORT, 'admin_state_up': self.ref_pool.admin_state_up} expected_dict = {'type': ovn_const.REQ_TYPE_POOL_CREATE, 'info': info} @@ -493,6 +494,7 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): info = {'id': self.ref_pool.pool_id, 'loadbalancer_id': self.ref_pool.loadbalancer_id, 'protocol': self.ref_pool.protocol, + 'lb_algorithm': constants.LB_ALGORITHM_SOURCE_IP_PORT, 'listener_id': self.ref_pool.listener_id, 'admin_state_up': True} expected_dict = {'type': ovn_const.REQ_TYPE_POOL_CREATE, diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py index 06de9d97..4199ac71 100644 --- a/ovn_octavia_provider/tests/unit/test_helper.py +++ b/ovn_octavia_provider/tests/unit/test_helper.py @@ -53,6 +53,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): 'loadbalancer_id': self.loadbalancer_id, 'listener_id': self.listener_id, 'protocol': 'TCP', + 'lb_algorithm': constants.LB_ALGORITHM_SOURCE_IP_PORT, 'admin_state_up': False} self.member = {'id': self.member_id, 'address': self.member_address, @@ -244,6 +245,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): expected_lb_info = { 'id': self.ovn_lb.name, 'protocol': 'tcp', + 'lb_algorithm': constants.LB_ALGORITHM_SOURCE_IP_PORT, 'vip_address': udp_lb.external_ids.get( ovn_const.LB_EXT_IDS_VIP_KEY), 'vip_port_id': @@ -299,13 +301,34 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): ovn_const.LB_EXT_IDS_VIP_PORT_ID_KEY: mock.ANY, 'enabled': 'False'}, name=mock.ANY, - protocol=None) + protocol=None, + selection_fields=['ip_src', 'ip_dst', 'tp_src', 'tp_dst']) @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') def test_lb_create_enabled(self, net_cli): self.lb['admin_state_up'] = True net_cli.return_value.list_ports.return_value = self.ports status = self.helper.lb_create(self.lb) + self.assertEqual(status['loadbalancers'][0]['provisioning_status'], + constants.ACTIVE) + self.assertEqual(status['loadbalancers'][0]['operating_status'], + constants.ONLINE) + self.helper.ovn_nbdb_api.db_create.assert_called_once_with( + 'Load_Balancer', external_ids={ + ovn_const.LB_EXT_IDS_VIP_KEY: mock.ANY, + ovn_const.LB_EXT_IDS_VIP_PORT_ID_KEY: mock.ANY, + 'enabled': 'True'}, + name=mock.ANY, + protocol=None, + selection_fields=['ip_src', 'ip_dst', 'tp_src', 'tp_dst']) + + @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') + def test_lb_create_selection_fields_not_supported(self, net_cli): + self.lb['admin_state_up'] = True + net_cli.return_value.list_ports.return_value = self.ports + self.helper._are_selection_fields_supported = ( + mock.Mock(return_value=False)) + status = self.helper.lb_create(self.lb) self.assertEqual(status['loadbalancers'][0]['provisioning_status'], constants.ACTIVE) self.assertEqual(status['loadbalancers'][0]['operating_status'], @@ -318,6 +341,27 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): name=mock.ANY, protocol=None) + @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') + def test_lb_create_selection_fields_not_supported_algo(self, net_cli): + self.lb['admin_state_up'] = True + net_cli.return_value.list_ports.return_value = self.ports + self.pool['lb_algoritm'] = 'foo' + status = self.helper.lb_create(self.lb) + self.assertEqual(status['loadbalancers'][0]['provisioning_status'], + constants.ACTIVE) + self.assertEqual(status['loadbalancers'][0]['operating_status'], + constants.ONLINE) + # NOTE(mjozefcz): Make sure that we use the same selection + # fields as for default algorithm - source_ip_port. + self.helper.ovn_nbdb_api.db_create.assert_called_once_with( + 'Load_Balancer', external_ids={ + ovn_const.LB_EXT_IDS_VIP_KEY: mock.ANY, + ovn_const.LB_EXT_IDS_VIP_PORT_ID_KEY: mock.ANY, + 'enabled': 'True'}, + name=mock.ANY, + protocol=None, + selection_fields=['ip_src', 'ip_dst', 'tp_src', 'tp_dst']) + @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') def test_lb_create_on_multi_protocol(self, net_cli): """This test situation when new protocol is added @@ -342,7 +386,8 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): ovn_const.LB_EXT_IDS_LR_REF_KEY: 'foo', 'enabled': 'True'}, name=mock.ANY, - protocol='udp') + protocol='udp', + 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']),