From 130ed10bedf4100d03d1c38507471fadfdf77e72 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Mon, 21 Nov 2022 14:23:37 +0100 Subject: [PATCH] Make clear distinction between health check and health monitor There was quite a mix between (Octavia) Health Monitors and (OVN) Load Balancer Health Checks. This patch tries to make a more clear distinction between Octavia HM and OVN LB HCs. This patch also add a reference to the Octavia Health Monitors Ids to the external_ids of the OVN NB Load_Balancer entries. Related-Bug: #1997418 Change-Id: Ib8499d7c4ea102e183ead31f063a3c0a70af6e23 (cherry picked from commit 9cb8cd5054e64ab7fd13b4b63259b639b4f26c50) --- ovn_octavia_provider/common/constants.py | 1 + ovn_octavia_provider/event.py | 4 +- ovn_octavia_provider/helper.py | 112 ++++++++++-------- .../tests/unit/test_helper.py | 25 ++-- 4 files changed, 80 insertions(+), 62 deletions(-) diff --git a/ovn_octavia_provider/common/constants.py b/ovn_octavia_provider/common/constants.py index 13f37991..03606611 100644 --- a/ovn_octavia_provider/common/constants.py +++ b/ovn_octavia_provider/common/constants.py @@ -38,6 +38,7 @@ 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_HMS_KEY = 'octavia:healthmonitors' LB_EXT_IDS_VIP_KEY = 'neutron:vip' LB_EXT_IDS_VIP_FIP_KEY = 'neutron:vip_fip' LB_EXT_IDS_VIP_PORT_ID_KEY = 'neutron:vip_port_id' diff --git a/ovn_octavia_provider/event.py b/ovn_octavia_provider/event.py index 39ec6308..ef16b9db 100644 --- a/ovn_octavia_provider/event.py +++ b/ovn_octavia_provider/event.py @@ -81,6 +81,6 @@ class ServiceMonitorUpdateEvent(row_event.RowEvent): {'event': event, 'row': row}) if event == self.ROW_DELETE: - self.driver.hm_update_event_handler(row, sm_delete_event=True) + self.driver.sm_update_event_handler(row, sm_delete_event=True) elif event == self.ROW_UPDATE: - self.driver.hm_update_event_handler(row) + self.driver.sm_update_event_handler(row) diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py index b0287956..056e2bbd 100644 --- a/ovn_octavia_provider/helper.py +++ b/ovn_octavia_provider/helper.py @@ -1300,8 +1300,8 @@ class OvnProviderHelper(): operating_status = constants.OFFLINE if (ovn_lb.health_check and - not self._update_hm_vip(ovn_lb, - listener[constants.PROTOCOL_PORT])): + not self._update_lbhc_vip(ovn_lb, + listener[constants.PROTOCOL_PORT])): operating_status = constants.ERROR status = { @@ -2210,7 +2210,7 @@ class OvnProviderHelper(): # We found particular port return port - def _add_hm(self, ovn_lb, pool_key, info): + def _add_lbhc(self, ovn_lb, pool_key, info): hm_id = info[constants.ID] status = {constants.ID: hm_id, constants.PROVISIONING_STATUS: constants.ERROR, @@ -2257,6 +2257,10 @@ class OvnProviderHelper(): operating_status = constants.ONLINE if not info['admin_state_up']: operating_status = constants.OFFLINE + + hms_key = ovn_lb.external_ids.get(ovn_const.LB_EXT_IDS_HMS_KEY, []) + if hms_key: + hms_key = jsonutils.loads(hms_key) try: with self.ovn_nbdb_api.transaction(check_error=True) as txn: health_check = txn.add( @@ -2266,6 +2270,11 @@ class OvnProviderHelper(): txn.add(self.ovn_nbdb_api.db_add( 'Load_Balancer', ovn_lb.uuid, 'health_check', health_check)) + hms_key.append(hm_id) + txn.add(self.ovn_nbdb_api.db_set( + 'Load_Balancer', ovn_lb.uuid, + ('external_ids', {ovn_const.LB_EXT_IDS_HMS_KEY: + jsonutils.dumps(hms_key)}))) status = {constants.ID: hm_id, constants.PROVISIONING_STATUS: constants.ACTIVE, constants.OPERATING_STATUS: operating_status} @@ -2274,23 +2283,23 @@ class OvnProviderHelper(): LOG.exception(ovn_const.EXCEPTION_MSG, "set of health check") return status - def _update_hm_vip(self, ovn_lb, vip_port): - hm = self._lookup_hm_by_id(ovn_lb.health_check) - if not hm: - LOG.error("Could not find HM with key: %s", ovn_lb.health_check) + def _update_lbhc_vip(self, ovn_lb, vip_port): + lbhc = self._lookup_lbhc_by_hm_id(ovn_lb.health_check) + if not lbhc: + LOG.error("Could not find HC with key: %s", ovn_lb.health_check) return False vip = ovn_lb.external_ids.get(ovn_const.LB_EXT_IDS_VIP_KEY) if not vip: - LOG.error("Could not find VIP for HM %s, LB external_ids: %s", - hm.uuid, ovn_lb.external_ids) + LOG.error("Could not find VIP for HC %s, LB external_ids: %s", + lbhc.uuid, ovn_lb.external_ids) return False vip = vip + ':' + str(vip_port) commands = [] commands.append( self.ovn_nbdb_api.db_set( - 'Load_Balancer_Health_Check', hm.uuid, + 'Load_Balancer_Health_Check', lbhc.uuid, ('vip', vip))) self._execute_commands(commands) return True @@ -2360,35 +2369,33 @@ class OvnProviderHelper(): self._execute_commands(commands) return True - def _lookup_hm_by_id(self, hm_id): - hms = self.ovn_nbdb_api.db_list_rows( + def _lookup_lbhc_by_hm_id(self, hm_id): + lbhcs = self.ovn_nbdb_api.db_list_rows( 'Load_Balancer_Health_Check').execute(check_error=True) - for hm in hms: - if (ovn_const.LB_EXT_IDS_HM_KEY in hm.external_ids and - hm.external_ids[ovn_const.LB_EXT_IDS_HM_KEY] == hm_id): - return hm + for lbhc in lbhcs: + if (ovn_const.LB_EXT_IDS_HM_KEY in lbhc.external_ids and + lbhc.external_ids[ovn_const.LB_EXT_IDS_HM_KEY] == hm_id): + return lbhc raise idlutils.RowNotFound(table='Load_Balancer_Health_Check', col='external_ids', match=hm_id) - def _lookup_lb_by_hm_id(self, hm_id): - lbs = self.ovn_nbdb_api.db_find_rows( - 'Load_Balancer', ('health_check', '=', [hm_id])).execute() - return lbs[0] if lbs else None - def _find_ovn_lb_from_hm_id(self, hm_id): - try: - hm = self._lookup_hm_by_id(hm_id) - except idlutils.RowNotFound: - LOG.debug("Loadbalancer health monitor %s not found!", hm_id) - return None, None + lbs = self.ovn_nbdb_api.db_list_rows( + 'Load_Balancer').execute(check_error=True) + ovn_lb = None + for lb in lbs: + if (ovn_const.LB_EXT_IDS_HMS_KEY in lb.external_ids.keys() and + hm_id in lb.external_ids[ovn_const.LB_EXT_IDS_HMS_KEY]): + ovn_lb = lb + break try: - ovn_lb = self._lookup_lb_by_hm_id(hm.uuid) + lbhc = self._lookup_lbhc_by_hm_id(hm_id) except idlutils.RowNotFound: - LOG.debug("Loadbalancer not found with health_check %s !", hm.uuid) - return hm, None + LOG.debug("Loadbalancer health check %s not found!", hm_id) + return None, ovn_lb - return hm, ovn_lb + return lbhc, ovn_lb def hm_create(self, info): status = { @@ -2443,7 +2450,7 @@ class OvnProviderHelper(): # ${OVN_LB_ID} health_check @hc # options here are interval, timeout, failure_count and success_count # from info object passed-in - hm_status = self._add_hm(ovn_lb, pool_key, info) + hm_status = self._add_lbhc(ovn_lb, pool_key, info) if hm_status[constants.PROVISIONING_STATUS] == constants.ACTIVE: if not self._update_hm_members(ovn_lb, pool_key): hm_status[constants.PROVISIONING_STATUS] = constants.ERROR @@ -2461,9 +2468,9 @@ class OvnProviderHelper(): hm_id = info[constants.ID] pool_id = info[constants.POOL_ID] - hm, ovn_lb = self._find_ovn_lb_from_hm_id(hm_id) - if not hm: - LOG.debug("Loadbalancer health monitor %s not found!", hm_id) + lbhc, ovn_lb = self._find_ovn_lb_from_hm_id(hm_id) + if not lbhc: + LOG.debug("Loadbalancer health check %s not found!", hm_id) return status if not ovn_lb: LOG.debug("Could not find LB with health monitor id %s", hm_id) @@ -2486,7 +2493,7 @@ class OvnProviderHelper(): commands = [] commands.append( self.ovn_nbdb_api.db_set( - 'Load_Balancer_Health_Check', hm.uuid, + 'Load_Balancer_Health_Check', lbhc.uuid, ('options', options))) self._execute_commands(commands) @@ -2514,11 +2521,12 @@ class OvnProviderHelper(): constants.OPERATING_STATUS: constants.NO_MONITOR, constants.PROVISIONING_STATUS: constants.DELETED}]} - hm, ovn_lb = self._find_ovn_lb_from_hm_id(hm_id) - if not hm or not ovn_lb: - LOG.debug("Loadbalancer Health Check %s not found in OVN " - "Northbound DB. Setting the Loadbalancer Health " - "Monitor status to DELETED in Octavia", hm_id) + lbhc, ovn_lb = self._find_ovn_lb_from_hm_id(hm_id) + if not lbhc or not ovn_lb: + LOG.debug("Loadbalancer Health Check associated to Health Monitor " + "%s not found in OVN Northbound DB. Setting the " + "Loadbalancer Health Monitor status to DELETED in " + "Octavia", hm_id) return status # Need to send pool info in status update to avoid immutable objects, @@ -2532,16 +2540,26 @@ class OvnProviderHelper(): # ovn-nbctl clear load_balancer ${OVN_LB_ID} ip_port_mappings # ovn-nbctl clear load_balancer ${OVN_LB_ID} health_check # TODO(haleyb) remove just the ip_port_mappings for this hm + hms_key = ovn_lb.external_ids.get(ovn_const.LB_EXT_IDS_HMS_KEY, []) + if hms_key: + hms_key = jsonutils.loads(hms_key) + if lbhc.uuid in hms_key: + hms_key.remove(lbhc.uuid) commands = [] commands.append( self.ovn_nbdb_api.db_clear('Load_Balancer', ovn_lb.uuid, 'ip_port_mappings')) commands.append( self.ovn_nbdb_api.db_remove('Load_Balancer', ovn_lb.uuid, - 'health_check', hm.uuid)) + 'health_check', lbhc.uuid)) + commands.append( + self.ovn_nbdb_api.db_set( + 'Load_Balancer', ovn_lb.uuid, + ('external_ids', {ovn_const.LB_EXT_IDS_HMS_KEY: + jsonutils.dumps(hms_key)}))) commands.append( self.ovn_nbdb_api.db_destroy('Load_Balancer_Health_Check', - hm.uuid)) + lbhc.uuid)) self._execute_commands(commands) status = { constants.LOADBALANCERS: [ @@ -2592,24 +2610,24 @@ class OvnProviderHelper(): ('protocol', '=', row.protocol[0])).execute() return lbs if lbs else None - def hm_update_event_handler(self, row, sm_delete_event=False): + def sm_update_event_handler(self, row, sm_delete_event=False): # NOTE(froyo): When a delete event is triggered, the Service_Monitor # deleted row will include the last valid information, e.g. when the # port is directly removed from the VM, the status will be 'online', # in order to protect from this behaviour, we will set manually the # status to 'offline' if sm_delete_event is reported as True. try: - ovn_lb = self._get_lbs_on_hm_event(row) + ovn_lbs = self._get_lbs_on_hm_event(row) except idlutils.RowNotFound: LOG.debug("Load balancer information not found") return - if not ovn_lb: + if not ovn_lbs: LOG.debug("Load balancer not found") return request_info = { - "ovn_lb": ovn_lb, + "ovn_lbs": ovn_lbs, "ip": row.ip, "port": str(row.port), "status": row.status @@ -2730,7 +2748,7 @@ class OvnProviderHelper(): return status def hm_update_event(self, info): - ovn_lbs = info['ovn_lb'] + ovn_lbs = info['ovn_lbs'] statuses = [] for ovn_lb in ovn_lbs: diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py index 84f9804e..7efd5263 100644 --- a/ovn_octavia_provider/tests/unit/test_helper.py +++ b/ovn_octavia_provider/tests/unit/test_helper.py @@ -98,11 +98,11 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.ovn_hm_lb = mock.MagicMock() self.ovn_hm_lb.protocol = ['tcp'] self.ovn_hm_lb.uuid = uuidutils.generate_uuid() - self.ovn_hm_lb.health_check = [] 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} + self.ovn_hm_lb.health_check = [self.ovn_hm.uuid] self.member_line = ( 'member_%s_%s:%s_%s' % (self.member_id, self.member_address, @@ -120,6 +120,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): ovn_const.LB_EXT_IDS_VIP_KEY: '10.22.33.99', ovn_const.LB_EXT_IDS_VIP_FIP_KEY: '123.123.123.99', ovn_const.LB_EXT_IDS_VIP_PORT_ID_KEY: 'foo_hm_port', + ovn_const.LB_EXT_IDS_HMS_KEY: '["%s"]' % (self.ovn_hm.uuid), 'enabled': True, 'pool_%s' % self.pool_id: [], 'listener_%s' % self.listener_id: '80:pool_%s' % self.pool_id, @@ -3578,7 +3579,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.assertEqual(status['healthmonitors'][0]['operating_status'], constants.ERROR) - @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_hm_by_id') + @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_lbhc_by_hm_id') @mock.patch.object(ovn_helper.OvnProviderHelper, '_get_or_create_ovn_lb') def test_hm_create_then_listener_create(self, get_ovn_lb, lookup_hm): get_ovn_lb.return_value = self.ovn_hm_lb @@ -3595,7 +3596,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.assertEqual(status['listeners'][0]['operating_status'], constants.ONLINE) - @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_hm_by_id') + @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_lbhc_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 @@ -3609,7 +3610,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): constants.ERROR) @mock.patch.object(ovn_helper.OvnProviderHelper, '_refresh_lb_vips') - @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_hm_by_id') + @mock.patch.object(ovn_helper.OvnProviderHelper, '_lookup_lbhc_by_hm_id') @mock.patch.object(ovn_helper.OvnProviderHelper, '_get_or_create_ovn_lb') def test_hm_create_then_listener_create_no_vip(self, get_ovn_lb, lookup_hm, refresh_vips): @@ -3690,9 +3691,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): def test_hm_delete(self): self.helper.ovn_nbdb_api.db_list_rows.return_value.\ - execute.return_value = [self.ovn_hm] - self.helper.ovn_nbdb_api.db_find_rows.return_value.\ - execute.return_value = [self.ovn_hm_lb] + execute.side_effect = [[self.ovn_hm_lb], [self.ovn_hm]] status = self.helper.hm_delete(self.health_monitor) self.assertEqual(status['healthmonitors'][0]['provisioning_status'], constants.DELETED) @@ -3758,7 +3757,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.hm_update_event.run('update', row, mock.ANY) expected = { 'info': - {'ovn_lb': [self.ovn_hm_lb], + {'ovn_lbs': [self.ovn_hm_lb], 'ip': self.member_address, 'port': self.member_port, 'status': ovn_const.HM_EVENT_MEMBER_PORT_OFFLINE}, @@ -3786,7 +3785,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.hm_update_event.run('delete', row, mock.ANY) expected = { 'info': - {'ovn_lb': [self.ovn_hm_lb], + {'ovn_lbs': [self.ovn_hm_lb], 'ip': self.member_address, 'port': self.member_port, 'status': ovn_const.HM_EVENT_MEMBER_PORT_OFFLINE}, @@ -3853,7 +3852,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): if bad_port: port = 'bad-port' info = { - 'ovn_lb': [self.ovn_hm_lb], + 'ovn_lbs': [self.ovn_hm_lb], 'ip': ip, 'logical_port': 'a-logical-port', 'src_ip': '10.22.33.4', @@ -3873,7 +3872,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): def _test_hm_update_status(self, ovn_lbs, member_id, ip, port, member_status): info = { - 'ovn_lb': ovn_lbs, + 'ovn_lbs': ovn_lbs, 'ip': ip, 'logical_port': 'a-logical-port', 'src_ip': '10.22.33.4', @@ -3975,7 +3974,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): ovn_hm_lb_2, fake_subnet, 8080, ip=member['address']) info = { - 'ovn_lb': [self.ovn_hm_lb, ovn_hm_lb_2], + 'ovn_lbs': [self.ovn_hm_lb, ovn_hm_lb_2], 'ip': member['address'], 'logical_port': 'a-logical-port', 'src_ip': '10.22.33.4', @@ -4096,7 +4095,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self._update_member_status(ovn_hm_lb2, member_lb2['id'], 'offline') info = { - 'ovn_lb': [self.ovn_hm_lb, ovn_hm_lb2], + 'ovn_lbs': [self.ovn_hm_lb, ovn_hm_lb2], 'ip': ip_member, 'logical_port': 'a-logical-port', 'src_ip': '10.22.33.4',