diff --git a/ovn_octavia_provider/common/constants.py b/ovn_octavia_provider/common/constants.py index bcc26488..6af65c7b 100644 --- a/ovn_octavia_provider/common/constants.py +++ b/ovn_octavia_provider/common/constants.py @@ -39,6 +39,8 @@ LB_EXT_IDS_POOL_PREFIX = 'pool_' LB_EXT_IDS_LISTENER_PREFIX = 'listener_' LB_EXT_IDS_MEMBER_PREFIX = 'member_' LB_EXT_IDS_HM_KEY = 'octavia:healthmonitor' +LB_EXT_IDS_HM_POOL_KEY = 'octavia:pool_id' +LB_EXT_IDS_HM_VIP = 'octavia:vip' LB_EXT_IDS_HMS_KEY = 'octavia:healthmonitors' LB_EXT_IDS_VIP_KEY = 'neutron:vip' LB_EXT_IDS_VIP_FIP_KEY = 'neutron:vip_fip' diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py index d184fa91..6e13d475 100644 --- a/ovn_octavia_provider/helper.py +++ b/ovn_octavia_provider/helper.py @@ -1415,10 +1415,14 @@ class OvnProviderHelper(): if not listener.get(constants.ADMIN_STATE_UP, True): operating_status = constants.OFFLINE - if (ovn_lb.health_check and - not self._update_lbhc_vip(ovn_lb, - listener[constants.PROTOCOL_PORT])): - operating_status = constants.ERROR + if pool_key: + for lb_hc in ovn_lb.health_check: + if pool_key[len(ovn_const.LB_EXT_IDS_POOL_PREFIX):] == ( + lb_hc.external_ids.get( + ovn_const.LB_EXT_IDS_HM_POOL_KEY)): + if not self._update_lbhc_vip_port( + lb_hc, listener[constants.PROTOCOL_PORT]): + operating_status = constants.ERROR status = { constants.LISTENERS: [ @@ -1585,8 +1589,6 @@ class OvnProviderHelper(): constants.LOADBALANCERS: [ {constants.ID: listener[constants.LOADBALANCER_ID], constants.PROVISIONING_STATUS: constants.ACTIVE}]} - if ovn_lb.health_check: - self._update_lbhc_vip(ovn_lb, listener[constants.PROTOCOL_PORT]) return status def pool_create(self, pool): @@ -2273,6 +2275,16 @@ class OvnProviderHelper(): "check Neutron logs. Perhaps port " "was already deleted.", port_id) + # NOTE(froyo): This could be removed in some cycles after Bobcat, this + # check is created to ensure that LB HC vip field is correctly format like + # IP:PORT + def _check_lbhc_vip_format(self, vip): + if vip: + ip_port = vip.rsplit(':', 1) + if len(ip_port) == 2 and ip_port[1].isdigit(): + return True + return False + def handle_vip_fip(self, fip_info): ovn_lb = fip_info['ovn_lb'] external_ids = copy.deepcopy(ovn_lb.external_ids) @@ -2286,15 +2298,22 @@ class OvnProviderHelper(): commands.append( self.ovn_nbdb_api.db_set('Load_Balancer', ovn_lb.uuid, ('external_ids', vip_fip_info))) - if ovn_lb.health_check: + for lb_hc in ovn_lb.health_check: + vip = fip_info['vip_fip'] + lb_hc_external_ids = copy.deepcopy(lb_hc.external_ids) + lb_hc_external_ids[ovn_const.LB_EXT_IDS_HM_VIP] = vip + if self._check_lbhc_vip_format(lb_hc.vip): + port = lb_hc.vip.rsplit(':')[-1] + vip += ':' + port + else: + vip = '' kwargs = { - 'vip': fip_info['vip_fip'], - 'options': ovn_lb.health_check[0].options, - 'external_ids': ovn_lb.health_check[0].external_ids} + 'vip': vip, + 'options': lb_hc.options, + 'external_ids': lb_hc_external_ids} with self.ovn_nbdb_api.transaction(check_error=True) as txn: - fip_lbhc = txn.add( - self.ovn_nbdb_api.db_create( - 'Load_Balancer_Health_Check', **kwargs)) + fip_lbhc = txn.add(self.ovn_nbdb_api.db_create( + 'Load_Balancer_Health_Check', **kwargs)) txn.add(self.ovn_nbdb_api.db_add( 'Load_Balancer', ovn_lb.uuid, 'health_check', fip_lbhc)) @@ -2439,6 +2458,19 @@ class OvnProviderHelper(): hm_id, ovn_lb.external_ids) return status vip_port = self._get_pool_listener_port(ovn_lb, pool_key) + + # This is to enable lookups by Octavia DB ID value + external_ids = { + ovn_const.LB_EXT_IDS_HM_KEY: hm_id, + ovn_const.LB_EXT_IDS_HM_VIP: vip, + ovn_const.LB_EXT_IDS_HM_POOL_KEY: pool_key[ + len(ovn_const.LB_EXT_IDS_POOL_PREFIX):], + } + + if fip: + external_ids_fip = copy.deepcopy(external_ids) + external_ids_fip[ovn_const.LB_EXT_IDS_HM_VIP] = fip + if not vip_port: # This is not fatal as we can add it when a listener is created vip = [] @@ -2460,9 +2492,6 @@ class OvnProviderHelper(): 'success_count': str(info['success_count']), 'failure_count': str(info['failure_count'])} - # This is to enable lookups by Octavia DB ID value - external_ids = {ovn_const.LB_EXT_IDS_HM_KEY: hm_id} - # Just seems like this needs ovsdbapp support, see: # ovsdbapp/schema/ovn_northbound/impl_idl.py - lb_add() # ovsdbapp/schema/ovn_northbound/commands.py - LbAddCommand() @@ -2475,7 +2504,7 @@ class OvnProviderHelper(): fip_kwargs = { 'vip': fip, 'options': options, - 'external_ids': external_ids} + 'external_ids': external_ids_fip} operating_status = constants.ONLINE if not info['admin_state_up']: @@ -2514,60 +2543,20 @@ class OvnProviderHelper(): LOG.exception(ovn_const.EXCEPTION_MSG, "set of health check") return status - def _update_lbhc_vip(self, ovn_lb, vip_port): - vip = ovn_lb.external_ids.get(ovn_const.LB_EXT_IDS_VIP_KEY) - if not vip: - LOG.error("Could not find VIP for LB external_ids: %s", - ovn_lb.external_ids) - return False - - vip_version = netaddr.IPAddress(vip).version - if vip_version == 6: - vip_lbhc = [lbhc for lbhc in ovn_lb.health_check - if lbhc.vip == [] or lbhc.vip[1:].split("]")[0] == vip] + def _update_lbhc_vip_port(self, lbhc, vip_port): + if lbhc.vip: + vip = lbhc.vip.rsplit(":")[0] + ':' + str(vip_port) else: - vip_lbhc = [lbhc for lbhc in ovn_lb.health_check - if lbhc.vip == [] or lbhc.vip.split(":")[0] == vip] - if not vip_lbhc: - LOG.error("Could not find HC associated to VIP: %s", vip) - return False - - vip = vip + ':' + str(vip_port) - fip = ovn_lb.external_ids.get(ovn_const.LB_EXT_IDS_VIP_FIP_KEY) - create_fip_lbhc = False - if fip: - fip = fip + ':' + str(vip_port) - if len(ovn_lb.health_check) != 2: - LOG.warning("There should be two HCs associated to the Load " - "Balancer %s as it has a FIP associated to it", - ovn_lb.uuid) - create_fip_lbhc = True - + # If initially the lbhc was created with no port info, vip field + # will be empty, so get it from lbhc external_ids + vip = lbhc.external_ids.get(ovn_const.LB_EXT_IDS_HM_VIP, '') + if vip: + vip = vip + ':' + str(vip_port) commands = [] commands.append( self.ovn_nbdb_api.db_set( - 'Load_Balancer_Health_Check', ovn_lb.health_check[0].uuid, + 'Load_Balancer_Health_Check', lbhc.uuid, ('vip', vip))) - if fip: - if create_fip_lbhc: - # For upgrades purposes we need to recover from this situation - # and create the health_check for the FIP - kwargs = { - 'vip': fip, - 'options': vip_lbhc[0].options, - 'external_ids': vip_lbhc[0].external_ids} - with self.ovn_nbdb_api.transaction(check_error=True) as txn: - fip_lbhc = txn.add( - self.ovn_nbdb_api.db_create( - 'Load_Balancer_Health_Check', **kwargs)) - txn.add(self.ovn_nbdb_api.db_add( - 'Load_Balancer', ovn_lb.uuid, - 'health_check', fip_lbhc)) - else: - commands.append( - self.ovn_nbdb_api.db_set( - 'Load_Balancer_Health_Check', - ovn_lb.health_check[1].uuid, ('vip', fip))) self._execute_commands(commands) return True diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py index f5976f0a..d97f7b65 100644 --- a/ovn_octavia_provider/tests/unit/test_helper.py +++ b/ovn_octavia_provider/tests/unit/test_helper.py @@ -108,7 +108,9 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.ovn_hm = mock.MagicMock() self.ovn_hm.uuid = self.healthmonitor_id self.ovn_hm.external_ids = { - ovn_const.LB_EXT_IDS_HM_KEY: self.ovn_hm.uuid} + ovn_const.LB_EXT_IDS_HM_KEY: self.ovn_hm.uuid, + ovn_const.LB_EXT_IDS_HM_POOL_KEY: self.pool_id, + ovn_const.LB_EXT_IDS_HM_VIP: '10.22.33.99'} self.ovn_hm_lb.health_check = [self.ovn_hm.uuid] self.member_line = ( 'member_%s_%s:%s_%s' % @@ -537,6 +539,23 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): found = f(self.pool_id) self.assertEqual(found, (lb.vip_subnet_id, '10.22.33.0/24')) + def test__check_lbhc_vip_format(self): + vip = "192.168.0.1:8080" + result = self.helper._check_lbhc_vip_format(vip) + self.assertTrue(result) + vip = "192.168.0.1" + result = self.helper._check_lbhc_vip_format(vip) + self.assertFalse(result) + vip = "[2001:db8:3333:4444:5555:6666:7777:8888]:8080" + result = self.helper._check_lbhc_vip_format(vip) + self.assertTrue(result) + vip = "[2001:db8:3333:4444:5555:6666:7777:8888]" + result = self.helper._check_lbhc_vip_format(vip) + self.assertFalse(result) + vip = "" + result = self.helper._check_lbhc_vip_format(vip) + self.assertFalse(result) + def test__get_subnet_from_pool_lb_no_vip_subnet_id(self): f = self.helper._get_subnet_from_pool @@ -3209,14 +3228,6 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.helper.handle_vip_fip(fip_info) - kwargs = { - 'vip': fip_info['vip_fip'], - 'options': lb.health_check[0].options, - 'external_ids': lb.health_check[0].external_ids} - self.helper.ovn_nbdb_api.db_create.assert_called_once_with( - 'Load_Balancer_Health_Check', **kwargs) - self.helper.ovn_nbdb_api.db_add.assert_called_once_with( - 'Load_Balancer', lb.uuid, 'health_check', mock.ANY) expected_db_set_calls = [ mock.call('Load_Balancer', lb.uuid, ('external_ids', {'neutron:vip_fip': '10.0.0.123'})), @@ -3669,14 +3680,21 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): 'timeout': '7', 'failure_count': '5', 'success_count': '3'} - external_ids = {ovn_const.LB_EXT_IDS_HM_KEY: self.healthmonitor_id} + external_ids = { + ovn_const.LB_EXT_IDS_HM_KEY: self.healthmonitor_id, + ovn_const.LB_EXT_IDS_HM_VIP: + self.ovn_hm_lb.external_ids[ovn_const.LB_EXT_IDS_VIP_KEY], + ovn_const.LB_EXT_IDS_HM_POOL_KEY: self.pool_id} kwargs = {'vip': vip, 'options': options, 'external_ids': external_ids} if fip: + external_ids_fips = copy.deepcopy(external_ids) + external_ids_fips[ovn_const.LB_EXT_IDS_HM_VIP] = ( + self.ovn_hm_lb.external_ids[ovn_const.LB_EXT_IDS_VIP_FIP_KEY]) fip_kwargs = {'vip': fip, 'options': options, - 'external_ids': external_ids} + 'external_ids': external_ids_fips} expected_lbhc_calls = [ mock.call('Load_Balancer_Health_Check', **kwargs)] @@ -3877,28 +3895,29 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): constants.ERROR) @mock.patch.object(ovn_helper.OvnProviderHelper, '_get_or_create_ovn_lb') - def test_hm_create_then_listener_create(self, get_ovn_lb): + def test_hm_existing_lbhc_update_on_listener_create(self, get_ovn_lb): vip = (self.ovn_hm_lb.external_ids[ovn_const.LB_EXT_IDS_VIP_KEY] + ':' + str(self.listener['protocol_port'])) fip = (self.ovn_hm_lb.external_ids[ovn_const.LB_EXT_IDS_VIP_FIP_KEY] + ':' + str(self.listener['protocol_port'])) self.ovn_hm.vip = [] - self.ovn_hm_lb.health_check = [self.ovn_hm] + self.ovn_hm_fip = copy.deepcopy(self.ovn_hm) + self.ovn_hm_fip.external_ids[ovn_const.LB_EXT_IDS_HM_VIP] = ( + self.ovn_hm_lb.external_ids[ovn_const.LB_EXT_IDS_VIP_FIP_KEY]) + self.ovn_hm_lb.health_check = [self.ovn_hm, self.ovn_hm_fip] get_ovn_lb.return_value = self.ovn_hm_lb self.listener['admin_state_up'] = True - kwargs = { - 'vip': fip, - 'options': self.ovn_hm.options, - 'external_ids': self.ovn_hm.external_ids} status = self.helper.listener_create(self.listener) - self.helper.ovn_nbdb_api.db_set.assert_called_with( - 'Load_Balancer_Health_Check', self.ovn_hm.uuid, ('vip', vip)) - self.helper.ovn_nbdb_api.db_create.assert_called_with( - 'Load_Balancer_Health_Check', **kwargs) - self.helper.ovn_nbdb_api.db_add.assert_called_with( - 'Load_Balancer', self.ovn_hm_lb.uuid, 'health_check', mock.ANY) + expected_set_external_ids_calls = [ + mock.call('Load_Balancer_Health_Check', self.ovn_hm.uuid, + ('vip', vip)), + mock.call('Load_Balancer_Health_Check', self.ovn_hm.uuid, + ('vip', fip))] + self.helper.ovn_nbdb_api.db_set.assert_has_calls( + expected_set_external_ids_calls) + self.assertEqual(status['listeners'][0]['provisioning_status'], constants.ACTIVE) self.assertEqual(status['listeners'][0]['operating_status'], @@ -3925,19 +3944,6 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.assertEqual(status['listeners'][0]['operating_status'], constants.ONLINE) - @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_lbhcs_by_hm_id') - @mock.patch.object(ovn_helper.OvnProviderHelper, '_get_or_create_ovn_lb') - def test_hm_create_then_listener_create_no_hm(self, get_ovn_lb, lookup_hm): - get_ovn_lb.return_value = self.ovn_hm_lb - lookup_hm.return_value = [] - self.ovn_hm_lb.health_check = [self.ovn_hm] - self.listener['admin_state_up'] = True - status = self.helper.listener_create(self.listener) - self.assertEqual(status['listeners'][0]['provisioning_status'], - constants.ACTIVE) - self.assertEqual(status['listeners'][0]['operating_status'], - constants.ERROR) - @mock.patch.object(ovn_helper.OvnProviderHelper, '_refresh_lb_vips') @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_lbhcs_by_hm_id') @mock.patch.object(ovn_helper.OvnProviderHelper, '_get_or_create_ovn_lb') @@ -3952,16 +3958,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.assertEqual(status['listeners'][0]['provisioning_status'], constants.ACTIVE) self.assertEqual(status['listeners'][0]['operating_status'], - constants.ERROR) - - @mock.patch.object(ovn_helper.OvnProviderHelper, '_update_lbhc_vip') - @mock.patch.object(ovn_helper.OvnProviderHelper, '_find_ovn_lbs') - def test_hm_create_then_listener_update(self, find_ovn_lbs, - update_lbhc_vip): - find_ovn_lbs.return_value = self.ovn_hm_lb - self.helper.listener_update(self.listener) - update_lbhc_vip.assert_called_once_with( - self.ovn_hm_lb, self.listener[constants.PROTOCOL_PORT]) + constants.ONLINE) @mock.patch.object(ovn_helper.OvnProviderHelper, '_find_ovn_lb_from_hm_id') def test_hm_update(self, folbfhi): diff --git a/releasenotes/notes/lbhc-fix-fip-05820d5a9d94a919.yaml b/releasenotes/notes/lbhc-fix-fip-05820d5a9d94a919.yaml new file mode 100644 index 00000000..88a54cc1 --- /dev/null +++ b/releasenotes/notes/lbhc-fix-fip-05820d5a9d94a919.yaml @@ -0,0 +1,16 @@ +--- +issues: + - | + Load Balancer Health Check for Floating IPs are not populated with the + protocol port. At this way, when a backend is detected on ERROR state + requests to the Floating IP are still distribute to the ERROR'ed members. + In order to fix the existing Load Balancer Health Checks it is required to + recreate the entire Octavia Health Monitor, which will recreate the + associated OVN Load Balancer Health Check(s). +fixes: + - | + [`bug 1997418 `_] + Added the protocol port to the Load Balancer Health Check associated with + the Floating IP, additional fields have been introduced to the external_ids + to provide more accuracy information about the entities affected by any + change over the Floating IP or LB VIP. \ No newline at end of file