Merge "Patching Octavia LB pool removal issue and adding updated unit tests"

Author: Zuul

ovn_octavia_provider/helper.py

@@ -3520,18 +3520,52 @@ def _update_lbhc_vip_port(self, lbhc, vip_port):
         self._execute_commands(commands)
         return True
 
-    def _update_ip_port_mappings(self, ovn_lb, backend_ip, port_name, src_ip,
-                                 delete=False):
-
+    def _update_ip_port_mappings(
+            self,
+            ovn_lb,
+            backend_ip,
+            port_name,
+            src_ip,
+            pool_key,
+            delete=False):
         # ip_port_mappings:${MEMBER_IP}=${LSP_NAME_MEMBER}:${HEALTH_SRC}
         # where:
         #  MEMBER_IP: IP of member_lsp
         #  LSP_NAME_MEMBER: Logical switch port
         #  HEALTH_SRC: source IP of hm_port
 
         if delete:
-            self.ovn_nbdb_api.lb_del_ip_port_mapping(ovn_lb.uuid,
-                                                     backend_ip).execute()
+            # Before removing a member from ip_port_mappings,
+            # make sure no other
+            # pool uses the same member.
+            other_members = []
+            for k, v in ovn_lb.external_ids.items():
+                if ovn_const.LB_EXT_IDS_POOL_PREFIX in k and k != pool_key:
+                    other_members.extend(self._extract_member_info(
+                        ovn_lb.external_ids[k]))
+            member_statuses = ovn_lb.external_ids.get(
+                ovn_const.OVN_MEMBER_STATUS_KEY)
+            try:
+                member_statuses = jsonutils.loads(member_statuses)
+            except TypeError:
+                LOG.debug("No member status in external_ids: %s",
+                          str(member_statuses))
+                member_statuses = {}
+            execute_delete = True
+            for member_id in [item[3] for item in other_members
+                              if item[0] == backend_ip]:
+                if member_statuses.get(member_id, '') != constants.NO_MONITOR:
+                    execute_delete = False
+                    LOG.debug(
+                        f"Backend {backend_ip} still in use by member"
+                        f" {member_id}, "
+                        f"so it won't be removed"
+                    )
+                    break
+            if execute_delete:
+                LOG.debug(f"Removing ip_port_mapping for {backend_ip}")
+                self.ovn_nbdb_api.lb_del_ip_port_mapping(
+                    ovn_lb.uuid, backend_ip).execute()
         else:
             self.ovn_nbdb_api.lb_add_ip_port_mapping(ovn_lb.uuid,
                                                      backend_ip,
@@ -3620,8 +3654,11 @@ def _update_hm_member(self, ovn_lb, pool_key, backend_ip, delete=False):
                                'member': mb_ip,
                                'pool': pool_key})
                     return None
-                self._update_ip_port_mappings(ovn_lb, backend_ip,
-                                              member_lsp.name, hm_source_ip,
+                self._update_ip_port_mappings(ovn_lb,
+                                              backend_ip,
+                                              member_lsp.name,
+                                              hm_source_ip,
+                                              pool_key,
                                               delete)
                 return constants.ONLINE
 

ovn_octavia_provider/tests/unit/test_helper.py

@@ -336,15 +336,23 @@ def test__clean_ip_port_mappings_two_hm_pools_not_sharing_members(self):
     def test__update_ip_port_mappings_del_backend_member(self):
         src_ip = '10.22.33.4'
         self.helper._update_ip_port_mappings(
-            self.ovn_lb, self.member_address, 'a-logical-port', src_ip,
+            self.ovn_lb,
+            self.member_address,
+            'a-logical-port',
+            src_ip,
+            'test_pool_key',
             delete=True)
         self.helper.ovn_nbdb_api.lb_del_ip_port_mapping.\
             assert_called_once_with(self.ovn_lb.uuid, self.member_address)
 
     def test__update_ip_port_mappings_add_backend_member(self):
         src_ip = '10.22.33.4'
         self.helper._update_ip_port_mappings(
-            self.ovn_lb, self.member_address, 'a-logical-port', src_ip)
+            self.ovn_lb,
+            self.member_address,
+            'a-logical-port',
+            src_ip,
+            'test_pool_key')
         self.helper.ovn_nbdb_api.lb_add_ip_port_mapping.\
             assert_called_once_with(self.ovn_lb.uuid, self.member_address,
                                     'a-logical-port', src_ip)
@@ -353,7 +361,11 @@ def test__update_ip_port_mappings_del_backend_member_ipv6(self):
         member_address = 'fda2:918e:5869:0:f816:3eff:feab:cdef'
         src_ip = 'fda2:918e:5869:0:f816:3eff:fecd:398a'
         self.helper._update_ip_port_mappings(
-            self.ovn_lb, member_address, 'a-logical-port', src_ip,
+            self.ovn_lb,
+            member_address,
+            'a-logical-port',
+            src_ip,
+            'test_pool_key',
             delete=True)
         self.helper.ovn_nbdb_api.lb_del_ip_port_mapping.\
             assert_called_once_with(self.ovn_lb.uuid, member_address)
@@ -362,10 +374,17 @@ def test__update_ip_port_mappings_add_backend_member_ipv6(self):
         member_address = 'fda2:918e:5869:0:f816:3eff:feab:cdef'
         src_ip = 'fda2:918e:5869:0:f816:3eff:fecd:398a'
         self.helper._update_ip_port_mappings(
-            self.ovn_lb, member_address, 'a-logical-port', src_ip)
+            self.ovn_lb,
+            member_address,
+            'a-logical-port',
+            src_ip,
+            'test_pool_key')
         self.helper.ovn_nbdb_api.lb_add_ip_port_mapping.\
             assert_called_once_with(
-                self.ovn_lb.uuid, member_address, 'a-logical-port', src_ip)
+                self.ovn_lb.uuid,
+                member_address,
+                'a-logical-port',
+                src_ip)
 
     def test__update_external_ids_member_status(self):
         self.helper._update_external_ids_member_status(
@@ -374,7 +393,10 @@ def test__update_external_ids_member_status(self):
             ovn_const.OVN_MEMBER_STATUS_KEY: '{"%s": "%s"}'
             % (self.member_id, constants.NO_MONITOR)}
         self.helper.ovn_nbdb_api.db_set.assert_called_once_with(
-            'Load_Balancer', self.ovn_lb.uuid, ('external_ids', member_status))
+            'Load_Balancer',
+            self.ovn_lb.uuid,
+            ('external_ids',
+             member_status))
 
     def test__update_external_ids_member_status_delete(self):
         self.helper._update_external_ids_member_status(
@@ -390,7 +412,10 @@ def test__update_external_ids_member_status_delete_not_found(self):
             ovn_const.OVN_MEMBER_STATUS_KEY: '{"%s": "%s"}'
             % (self.member_id, constants.NO_MONITOR)}
         self.helper.ovn_nbdb_api.db_set.assert_called_once_with(
-            'Load_Balancer', self.ovn_lb.uuid, ('external_ids', member_status))
+            'Load_Balancer',
+            self.ovn_lb.uuid,
+            ('external_ids',
+             member_status))
 
     def test__find_member_status(self):
         status = self.helper._find_member_status(self.ovn_lb, self.member_id)
@@ -6746,3 +6771,73 @@ def test_refresh_lb_vips_returns_db_operations_when_is_sync_false(
             self.ovn_lb.uuid,
             ('vips', {'vip1:port1': 'ip1:port1,ip2:port1'})
         )
+
+    def test_update_ip_port_mappings_add(self):
+        # Setup mock OVN load balancer
+        ovn_lb = mock.Mock()
+        ovn_lb.uuid = 'test-lb-uuid'
+        ovn_lb.external_ids = {}
+
+        # Call the method with delete=False
+        self.helper._update_ip_port_mappings(
+            ovn_lb, '10.0.0.1', 'port1', '192.168.0.1', 'pool1', delete=False
+        )
+
+        # Assert that lb_add_ip_port_mapping was called
+        self.helper.ovn_nbdb_api.lb_add_ip_port_mapping\
+            .assert_called_once_with(
+                'test-lb-uuid',
+                '10.0.0.1',
+                'port1',
+                '192.168.0.1',
+            )
+
+    def test_update_ip_port_mappings_delete_minimal(self):
+        ovn_lb = mock.Mock()
+        ovn_lb.uuid = 'test-lb-uuid'
+        ovn_lb.external_ids = {}
+        # Patch _extract_member_info to return no other members
+        self.helper._extract_member_info = mock.Mock(return_value=[])
+        # Also patch ovn_nbdb_api call
+        self.helper.ovn_nbdb_api.lb_del_ip_port_mapping = mock.Mock()
+        self.helper.ovn_nbdb_api.lb_add_ip_port_mapping = mock.Mock()
+        self.helper._update_ip_port_mappings(
+            ovn_lb,
+            backend_ip='10.0.0.1',
+            port_name='dummy-port',
+            src_ip='192.168.0.1',
+            pool_key='pool-test',
+            delete=True
+        )
+        self.helper.ovn_nbdb_api.\
+            lb_del_ip_port_mapping.\
+            assert_called_once_with(
+                'test-lb-uuid',
+                '10.0.0.1'
+            )
+
+    def test_update_ip_port_mappings_delete_with_other_members_present(self):
+        ovn_lb = mock.Mock()
+        ovn_lb.uuid = 'test-lb-uuid'
+        ovn_lb.external_ids = {
+            "pool_A": "member_memberA_10.0.0.1:80_subnetA",
+            "pool_B": "member_memberB_10.0.0.1:80_subnetA",
+            "neutron:member_statuses": '{"memberB": "ONLINE"}'
+        }
+
+        self.helper.ovn_nbdb_api.lb_del_ip_port_mapping = mock.Mock()
+        self.helper.ovn_nbdb_api.lb_add_ip_port_mapping = mock.Mock()
+
+        # Call the method under test
+        self.helper._update_ip_port_mappings(
+            ovn_lb,
+            backend_ip='10.0.0.1',
+            port_name='dummy-port',
+            src_ip='192.168.0.1',
+            pool_key='pool_A',
+            delete=True
+        )
+
+        # Should not call delete because memberB is ONLINE and shares the IP
+        self.helper.ovn_nbdb_api.lb_del_ip_port_mapping.assert_not_called()
+        self.helper.ovn_nbdb_api.lb_add_ip_port_mapping.assert_not_called()