Improve "sync_ha_chassis_group" method

The method "sync_ha_chassis_group" now creates (or retrieves) a
HA Chassis Group register and updates the needed HA Chassis registers
in a single transaction. That is possible using the new ovsdbapp
release 2.2.1 (check the depends-on patch).

Depends-On: https://review.opendev.org/c/openstack/ovsdbapp/+/871836

Conflicts:
  neutron/common/ovn/utils.py
  neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py
  neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

Related-Bug: #1995078
Change-Id: I936855214c635de0e89d5d13a86562f5b282633c
(cherry picked from commit ac231c8)

Author: ralonsoh

neutron/common/ovn/utils.py

@@ -14,6 +14,7 @@
 import copy
 import inspect
 import os
+import random
 
 import netaddr
 from neutron_lib.api.definitions import external_net
@@ -34,6 +35,7 @@
 from oslo_serialization import jsonutils
 from oslo_utils import netutils
 from oslo_utils import strutils
+from ovsdbapp.backend.ovs_idl import rowview
 from ovsdbapp import constants as ovsdbapp_const
 import tenacity
 
@@ -860,6 +862,79 @@ def get_ovn_chassis_other_config(chassis):
         return chassis.external_ids
 
 
+def sync_ha_chassis_group(context, network_id, nb_idl, sb_idl, txn):
+    """Return the UUID of the HA Chassis Group or the HA Chassis Group cmd.
+
+    Given the Neutron Network ID, this method will return (or create
+    and then return) the appropriate HA Chassis Group the external
+    port (in that network) needs to be associated with.
+
+    :param context: Neutron API context.
+    :param network_id: The Neutron network ID.
+    :param nb_idl: OVN NB IDL
+    :param sb_idl: OVN SB IDL
+    :param txn: The ovsdbapp transaction object.
+    :returns: The HA Chassis Group UUID or the HA Chassis Group command object.
+    """
+    plugin = directory.get_plugin()
+    az_hints = common_utils.get_az_hints(
+        plugin.get_network(context, network_id))
+
+    ha_ch_grp_name = ovn_name(network_id)
+    ext_ids = {constants.OVN_AZ_HINTS_EXT_ID_KEY: ','.join(az_hints)}
+    hcg_cmd = txn.add(nb_idl.ha_chassis_group_add(
+        ha_ch_grp_name, may_exist=True, external_ids=ext_ids))
+
+    if isinstance(hcg_cmd.result, rowview.RowView):
+        # The HA chassis group existed before this transaction.
+        ha_ch_grp = hcg_cmd.result
+    else:
+        # The HA chassis group is being created in this transaction.
+        ha_ch_grp = None
+
+    # Get the chassis belonging to the AZ hints
+    ch_list = sb_idl.get_gateway_chassis_from_cms_options(name_only=False)
+    if not az_hints:
+        az_chassis = get_gateway_chassis_without_azs(ch_list)
+    else:
+        az_chassis = get_chassis_in_azs(ch_list, az_hints)
+
+    priority = constants.HA_CHASSIS_GROUP_HIGHEST_PRIORITY
+    if ha_ch_grp:
+        # Remove any chassis that no longer belongs to the AZ hints
+        all_ch = {ch.chassis_name for ch in ha_ch_grp.ha_chassis}
+        ch_to_del = all_ch - az_chassis
+        for ch in ch_to_del:
+            txn.add(nb_idl.ha_chassis_group_del_chassis(
+                ha_ch_grp_name, ch, if_exists=True))
+
+        # Find the highest priority chassis in the HA Chassis Group. If
+        # it exists and still belongs to the same AZ, keep it as the
+        # highest priority in the group to avoid ports already bond to it
+        # from moving to another chassis.
+        high_prio_ch = max(ha_ch_grp.ha_chassis, key=lambda x: x.priority,
+                           default=None)
+        priority = constants.HA_CHASSIS_GROUP_HIGHEST_PRIORITY
+        if high_prio_ch and high_prio_ch.chassis_name in az_chassis:
+            txn.add(nb_idl.ha_chassis_group_add_chassis(
+                ha_ch_grp_name, high_prio_ch.chassis_name,
+                priority=priority))
+            az_chassis.remove(high_prio_ch.chassis_name)
+            priority -= 1
+
+    # Randomize the order so that networks belonging to the same
+    # availability zones do not necessarily end up with the same
+    # Chassis as the highest priority one.
+    for ch in random.sample(list(az_chassis), len(az_chassis)):
+        txn.add(nb_idl.ha_chassis_group_add_chassis(
+            hcg_cmd, ch, priority=priority))
+        priority -= 1
+
+    # Return the existing register UUID or the HA chassis group creation
+    # command (see ovsdbapp ``HAChassisGroupAddChassisCommand`` class).
+    return ha_ch_grp.uuid if ha_ch_grp else hcg_cmd
+
+
 def get_subnets_address_scopes(context, subnets, fixed_ips, ml2_plugin):
     """Returns the IPv4 and IPv6 address scopes of several subnets.
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py

@@ -17,6 +17,7 @@
 from oslo_utils import timeutils
 from ovsdbapp.backend.ovs_idl import command
 from ovsdbapp.backend.ovs_idl import idlutils
+from ovsdbapp import utils as ovsdbapp_utils
 
 from neutron._i18n import _
 from neutron.common.ovn import constants as ovn_const
@@ -140,6 +141,14 @@ def run_idl(self, txn):
             port.dhcpv6_options = dhcpv6_options
         else:
             port.dhcpv6_options = [dhcpv6_options.result]
+
+        # NOTE(ralonsoh): HA chassis group is created by Neutron, there is no
+        # need to create it in this command.
+        ha_chassis_group = self.columns.pop('ha_chassis_group', None)
+        if ha_chassis_group:
+            hcg_uuid = ovsdbapp_utils.get_uuid(ha_chassis_group)
+            port.ha_chassis_group = hcg_uuid
+
         for col, val in self.columns.items():
             setattr(port, col, val)
         # add the newly created port to existing lswitch
@@ -205,6 +214,21 @@ def run_idl(self, txn):
             external_ids[k] = v
         port.external_ids = external_ids
 
+        # NOTE(ralonsoh): HA chassis group is created by Neutron, there is no
+        # need to create it in this command. The register is also deleted when
+        # the network to which the HA chassis group is associated is deleted.
+        ha_chassis_group = self.columns.pop('ha_chassis_group', None)
+        if ha_chassis_group:
+            hcg_uuid = ovsdbapp_utils.get_uuid(ha_chassis_group)
+            try:
+                port_hcg_uuid = port.ha_chassis_group[0].uuid
+            except IndexError:
+                port_hcg_uuid = None
+            if port_hcg_uuid != hcg_uuid:
+                port.ha_chassis_group = hcg_uuid
+        elif ha_chassis_group == []:
+            port.ha_chassis_group = []
+
         for col, val in self.columns.items():
             setattr(port, col, val)
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py

@@ -605,15 +605,10 @@ def check_for_ha_chassis_group(self):
                 network_id = port.external_ids[
                     ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY].replace(
                         ovn_const.OVN_NAME_PREFIX, '')
-                ha_ch_grp = self._ovn_client.sync_ha_chassis_group(
-                    context, network_id, txn)
-                try:
-                    port_ha_ch_uuid = port.ha_chassis_group[0].uuid
-                except IndexError:
-                    port_ha_ch_uuid = None
-                if port_ha_ch_uuid != ha_ch_grp:
-                    txn.add(self._nb_idl.set_lswitch_port(
-                        port.name, ha_chassis_group=ha_ch_grp))
+                ha_ch_grp = utils.sync_ha_chassis_group(
+                    context, network_id, self._nb_idl, self._sb_idl, txn)
+                txn.add(self._nb_idl.set_lswitch_port(
+                    port.name, ha_chassis_group=ha_ch_grp))
 
             self._delete_default_ha_chassis_group(txn)
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -16,7 +16,6 @@
 import collections
 import copy
 import datetime
-import random
 
 import netaddr
 from neutron_lib.api.definitions import l3
@@ -469,76 +468,6 @@ def _get_port_options(self, port):
                            bp_info.vnic_type, bp_info.capabilities
                            )
 
-    def sync_ha_chassis_group(self, context, network_id, txn):
-        """Return the UUID of the HA Chassis Group.
-
-        Given the Neutron Network ID, this method will return (or create
-        and then return) the appropriate HA Chassis Group the external
-        port (in that network) needs to be associated with.
-
-        :param context: Neutron API context.
-        :param network_id: The Neutron network ID.
-        :param txn: The ovsdbapp transaction object.
-        :returns: An HA Chassis Group UUID.
-        """
-        az_hints = common_utils.get_az_hints(
-            self._plugin.get_network(context, network_id))
-
-        ha_ch_grp_name = utils.ovn_name(network_id)
-        # FIXME(lucasagomes): Couldn't find a better way of doing this
-        # without a sub-transaction. This shouldn't be a problem since
-        # the HA Chassis Group associated with a network will be deleted
-        # as part of the network delete method (if present)
-        with self._nb_idl.create_transaction(check_error=True) as sub_txn:
-            sub_txn.add(self._nb_idl.ha_chassis_group_add(
-                ha_ch_grp_name, may_exist=True))
-
-        ha_ch_grp = self._nb_idl.ha_chassis_group_get(
-            ha_ch_grp_name).execute(check_error=True)
-        txn.add(self._nb_idl.db_set(
-            'HA_Chassis_Group', ha_ch_grp_name,
-            ('external_ids',
-             {ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ','.join(az_hints)})))
-
-        # Get the chassis belonging to the AZ hints
-        ch_list = self._sb_idl.get_gateway_chassis_from_cms_options(
-            name_only=False)
-        if not az_hints:
-            az_chassis = utils.get_gateway_chassis_without_azs(ch_list)
-        else:
-            az_chassis = utils.get_chassis_in_azs(ch_list, az_hints)
-
-        # Remove any chassis that no longer belongs to the AZ hints
-        all_ch = {ch.chassis_name for ch in ha_ch_grp.ha_chassis}
-        ch_to_del = all_ch - az_chassis
-        for ch in ch_to_del:
-            txn.add(self._nb_idl.ha_chassis_group_del_chassis(
-                ha_ch_grp_name, ch, if_exists=True))
-
-        # Find the highest priority chassis in the HA Chassis Group. If
-        # it exists and still belongs to the same AZ, keep it as the highest
-        # priority in the group to avoid ports already bond to it from
-        # moving to another chassis.
-        high_prio_ch = max(ha_ch_grp.ha_chassis, key=lambda x: x.priority,
-                           default=None)
-        priority = ovn_const.HA_CHASSIS_GROUP_HIGHEST_PRIORITY
-        if high_prio_ch and high_prio_ch.chassis_name in az_chassis:
-            txn.add(self._nb_idl.ha_chassis_group_add_chassis(
-                ha_ch_grp_name, high_prio_ch.chassis_name,
-                priority=priority))
-            az_chassis.remove(high_prio_ch.chassis_name)
-            priority -= 1
-
-        # Randomize the order so that networks belonging to the same
-        # availability zones do not necessarily end up with the same
-        # Chassis as the highest priority one.
-        for ch in random.sample(list(az_chassis), len(az_chassis)):
-            txn.add(self._nb_idl.ha_chassis_group_add_chassis(
-                ha_ch_grp_name, ch, priority=priority))
-            priority -= 1
-
-        return ha_ch_grp.uuid
-
     def update_port_dhcp_options(self, port_info, txn):
         dhcpv4_options = []
         dhcpv6_options = []
@@ -620,9 +549,9 @@ def create_port(self, context, port):
 
             if (self.is_external_ports_supported() and
                     port_info.type == ovn_const.LSP_TYPE_EXTERNAL):
-                kwargs['ha_chassis_group'] = (
-                    self.sync_ha_chassis_group(
-                        context, port['network_id'], txn))
+                kwargs['ha_chassis_group'] = utils.sync_ha_chassis_group(
+                    context, port['network_id'], self._nb_idl, self._sb_idl,
+                    txn)
 
             # NOTE(mjozefcz): Do not set addresses if the port is not
             # bound, has no device_owner and it is OVN LB VIP port.
@@ -743,8 +672,9 @@ def update_port(self, context, port, port_object=None):
             if self.is_external_ports_supported():
                 if port_info.type == ovn_const.LSP_TYPE_EXTERNAL:
                     columns_dict['ha_chassis_group'] = (
-                        self.sync_ha_chassis_group(
-                            context, port['network_id'], txn))
+                        utils.sync_ha_chassis_group(
+                            context, port['network_id'], self._nb_idl,
+                            self._sb_idl, txn))
                 else:
                     # Clear the ha_chassis_group field
                     columns_dict['ha_chassis_group'] = []
@@ -2105,7 +2035,9 @@ def update_network(self, context, network, original_network=None):
                     neutron_net_azs = lswitch_params['external_ids'].get(
                         ovn_const.OVN_AZ_HINTS_EXT_ID_KEY, '')
                     if ovn_ls_azs != neutron_net_azs:
-                        self.sync_ha_chassis_group(context, network['id'], txn)
+                        utils.sync_ha_chassis_group(
+                            context, network['id'], self._nb_idl,
+                            self._sb_idl, txn)
 
             # Update the segment tags, if any
             segments = segments_db.get_network_segments(context, network['id'])

neutron/tests/functional/common/ovn/test_utils.py

@@ -12,6 +12,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from unittest import mock
+
+from oslo_utils import uuidutils
+
 from neutron.common.ovn import constants as ovn_const
 from neutron.common.ovn import utils
 from neutron.tests.functional import base
@@ -58,3 +62,56 @@ def test_non_existing(self):
         self.assertEqual(directions[0], acl2.direction)
         self.assertEqual('drop', acl2.action)
         self.assertEqual(matches[0], acl2.match)
+
+
+class TestSyncHaChassisGroup(base.TestOVNFunctionalBase):
+
+    def test_sync_ha_chassis_group(self):
+        plugin = mock.Mock()
+        plugin.get_network.return_value = {}
+        network_id = uuidutils.generate_uuid()
+        hcg_name = utils.ovn_name(network_id)
+        chassis1 = self.add_fake_chassis('host1', azs=[],
+                                         enable_chassis_as_gw=True)
+        chassis2 = self.add_fake_chassis('host2', azs=[],
+                                         enable_chassis_as_gw=True)
+        self.add_fake_chassis('host3')
+
+        with self.nb_api.transaction(check_error=True) as txn:
+            utils.sync_ha_chassis_group(self.context, network_id, self.nb_api,
+                                        self.sb_api, txn)
+
+        ha_chassis = self.nb_api.db_find('HA_Chassis').execute(
+            check_error=True)
+        ha_chassis_names = [hc['chassis_name'] for hc in ha_chassis]
+        self.assertEqual(2, len(ha_chassis))
+        self.assertEqual(sorted([chassis1, chassis2]),
+                         sorted(ha_chassis_names))
+
+        hcg = self.nb_api.ha_chassis_group_get(hcg_name).execute(
+            check_error=True)
+        self.assertEqual(hcg_name, hcg.name)
+        ha_chassis_exp = sorted([str(hc['_uuid']) for hc in ha_chassis])
+        ha_chassis_ret = sorted([str(hc.uuid) for hc in hcg.ha_chassis])
+        self.assertEqual(ha_chassis_exp, ha_chassis_ret)
+
+        # Delete one GW chassis and resync the HA chassis group associated to
+        # the same network. The method will now not create again the existing
+        # HA Chassis Group register but will update the "ha_chassis" list.
+        self.del_fake_chassis(chassis2)
+        with self.nb_api.transaction(check_error=True) as txn:
+            utils.sync_ha_chassis_group(self.context, network_id, self.nb_api,
+                                        self.sb_api, txn)
+
+        ha_chassis = self.nb_api.db_find('HA_Chassis').execute(
+            check_error=True)
+        ha_chassis_names = [hc['chassis_name'] for hc in ha_chassis]
+        self.assertEqual(1, len(ha_chassis))
+        self.assertEqual([chassis1], ha_chassis_names)
+
+        hcg = self.nb_api.ha_chassis_group_get(hcg_name).execute(
+            check_error=True)
+        self.assertEqual(hcg_name, hcg.name)
+        ha_chassis_exp = str(ha_chassis[0]['_uuid'])
+        ha_chassis_ret = str(hcg.ha_chassis[0].uuid)
+        self.assertEqual(ha_chassis_exp, ha_chassis_ret)