Check for ports correctness

This patch is updating networking-ovn to check for correctness when
creating, updating or deleting ports.

It does:

* Create the deltas using the information stored in OVNNB instead of
original_port parameter

* Save the revision number from Neutron in the ovn_revision_number table
and in OVNDB

* Expand the maintenance periodic task to detect and fix inconsistencies
for ports

Note that DNS records still can get out of sync but should be fixed by
https://bugs.launchpad.net/networking-ovn/+bug/1739257

Partial-Bug: #1605089
Change-Id: I1b2366743d76e93c8a2b19c06bcba10a3a29c7f6

networking_ovn/common/acl.py

@@ -422,16 +422,6 @@ def acl_port_ips(port):
     if not is_sg_enabled():
         return {'ip4': [], 'ip6': []}
 
-    ip_addresses = {4: [], 6: []}
-    for fixed_ip in port['fixed_ips']:
-        ip_version = netaddr.IPNetwork(fixed_ip['ip_address']).version
-        ip_addresses[ip_version].append(fixed_ip['ip_address'])
-
-    for allowed_ip in port.get('allowed_address_pairs', []):
-        if allowed_ip.get('ip_address'):
-            ip_version = \
-                netaddr.IPNetwork(allowed_ip['ip_address']).version
-            ip_addresses[ip_version].append(allowed_ip['ip_address'])
-
-    return {'ip4': ip_addresses[4],
-            'ip6': ip_addresses[6]}
+    ip_list = [x['ip_address'] for x in port.get('fixed_ips', [])]
+    ip_list.extend(utils.get_allowed_address_pairs_ip_addresses(port))
+    return utils.sort_ips_by_version(ip_list)

networking_ovn/common/constants.py

@@ -31,6 +31,8 @@ OVN_FIP_EXT_ID_KEY = 'neutron:fip_id'
 OVN_FIP_PORT_EXT_ID_KEY = 'neutron:fip_port_id'
 OVN_REV_NUM_EXT_ID_KEY = 'neutron:revision_number'
 OVN_QOS_POLICY_EXT_ID_KEY = 'neutron:qos_policy_id'
+OVN_SG_IDS_EXT_ID_KEY = 'neutron:security_group_ids'
+OVN_DEVICE_OWNER_EXT_ID_KEY = 'neutron:device_owner'
 OVN_PORT_BINDING_PROFILE = portbindings.PROFILE
 OVN_PORT_BINDING_PROFILE_PARAMS = [{'parent_name': six.string_types,
                                     'tag': six.integer_types},
@@ -90,3 +92,4 @@ INITIAL_REV_NUM = -1
 
 # Resource types
 TYPE_NETWORKS = 'networks'
+TYPE_PORTS = 'ports'

networking_ovn/common/maintenance.py

@@ -127,6 +127,39 @@ class DBInconsistenciesPeriodics(object):
         else:
             self._ovn_client.delete_network(row.resource_uuid)
 
+    def _fix_create_update_port(self, row):
+        # Get the latest version of the resource in Neutron DB
+        admin_context = n_context.get_admin_context()
+        p_db_obj = self._ovn_client._plugin.get_port(
+            admin_context, row.resource_uuid)
+        ovn_port = self._nb_idl.get_lswitch_port(
+            utils.ovn_name(row.resource_uuid))
+
+        if not ovn_port:
+            # If the resource doesn't exist in the OVN DB, create it.
+            self._ovn_client.create_port(p_db_obj)
+        else:
+            ext_ids = getattr(ovn_port, 'external_ids', {})
+            ovn_revision = int(ext_ids.get(
+                ovn_const.OVN_REV_NUM_EXT_ID_KEY, -1))
+            # If the resource exist in the OVN DB but the revision
+            # number is different from Neutron DB, updated it.
+            if ovn_revision != p_db_obj['revision_number']:
+                self._ovn_client.update_port(p_db_obj)
+            else:
+                # If the resource exist and the revision number
+                # is equal on both databases just bump the revision on
+                # the cache table.
+                db_rev.bump_revision(p_db_obj, ovn_const.TYPE_PORTS)
+
+    def _fix_delete_port(self, row):
+        ovn_port = self._nb_idl.get_lswitch_port(
+            utils.ovn_name(row.resource_uuid))
+        if not ovn_port:
+            db_rev.delete_revision(row.resource_uuid)
+        else:
+            self._ovn_client.delete_port(row.resource_uuid)
+
     @periodics.periodic(spacing=DB_CONSISTENCY_CHECK_INTERVAL,
                         run_immediately=True)
     def check_for_inconsistencies(self):
@@ -146,6 +179,8 @@ class DBInconsistenciesPeriodics(object):
             try:
                 if row.resource_type == ovn_const.TYPE_NETWORKS:
                     self._fix_create_update_network(row)
+                elif row.resource_type == ovn_const.TYPE_PORTS:
+                    self._fix_create_update_port(row)
             except Exception:
                 LOG.exception('Failed to fix resource %(res_uuid)s '
                               '(type: %(res_type)s)',
@@ -157,6 +192,8 @@ class DBInconsistenciesPeriodics(object):
             try:
                 if row.resource_type == ovn_const.TYPE_NETWORKS:
                     self._fix_delete_network(row)
+                elif row.resource_type == ovn_const.TYPE_PORTS:
+                    self._fix_delete_port(row)
             except Exception:
                 LOG.exception('Failed to fix deleted resource %(res_uuid)s '
                               '(type: %(res_type)s)',

networking_ovn/common/ovn_client.py

@@ -31,6 +31,7 @@ from neutron_lib.utils import net as n_net
 from oslo_config import cfg
 from oslo_log import log
 from oslo_utils import excutils
+from ovsdbapp.backend.ovs_idl import idlutils
 
 from networking_ovn.agent.metadata import agent as metadata_agent
 from networking_ovn.common import acl as ovn_acl
@@ -44,13 +45,10 @@ from networking_ovn.ml2 import qos_driver
 LOG = log.getLogger(__name__)
 
 
-OvnPortInfo = collections.namedtuple('OvnPortInfo', ['type', 'options',
-                                                     'addresses',
-                                                     'port_security',
-                                                     'parent_name', 'tag',
-                                                     'dhcpv4_options',
-                                                     'dhcpv6_options',
-                                                     'cidrs'])
+OvnPortInfo = collections.namedtuple(
+    'OvnPortInfo', ['type', 'options', 'addresses', 'port_security',
+                    'parent_name', 'tag', 'dhcpv4_options', 'dhcpv6_options',
+                    'cidrs', 'device_owner', 'security_group_ids'])
 
 
 class OVNClient(object):
@@ -226,10 +224,11 @@ class OVNClient(object):
 
         options.update({'requested-chassis':
                         port.get(portbindings.HOST_ID, '')})
-
+        device_owner = port.get('device_owner', '')
+        sg_ids = ' '.join(utils.get_lsp_security_groups(port))
         return OvnPortInfo(port_type, options, addresses, port_security,
                            parent_name, tag, dhcpv4_options, dhcpv6_options,
-                           cidrs.strip())
+                           cidrs.strip(), device_owner, sg_ids)
 
     def create_port(self, port):
         if utils.is_lsp_ignored(port):
@@ -239,7 +238,16 @@ class OVNClient(object):
         external_ids = {ovn_const.OVN_PORT_NAME_EXT_ID_KEY: port['name'],
                         ovn_const.OVN_DEVID_EXT_ID_KEY: port['device_id'],
                         ovn_const.OVN_PROJID_EXT_ID_KEY: port['project_id'],
-                        ovn_const.OVN_CIDRS_EXT_ID_KEY: port_info.cidrs}
+                        ovn_const.OVN_CIDRS_EXT_ID_KEY: port_info.cidrs,
+                        ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
+                            port_info.device_owner,
+                        ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
+                            utils.ovn_name(port['network_id']),
+                        ovn_const.OVN_SG_IDS_EXT_ID_KEY:
+                            port_info.security_group_ids,
+                        ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(
+                            utils.get_revision_number(
+                                port, ovn_const.TYPE_PORTS))}
         lswitch_name = utils.ovn_name(port['network_id'])
         admin_context = n_context.get_admin_context()
         sg_cache = {}
@@ -309,7 +317,23 @@ class OVNClient(object):
             if self.is_dns_required_for_port(port):
                 self.add_txns_to_sync_port_dns_records(txn, port)
 
-    def update_port(self, port, original_port, qos_options=None):
+        if not utils.is_lsp_router_port(port):
+            db_rev.bump_revision(port, ovn_const.TYPE_PORTS)
+
+    # TODO(lucasagomes): Remove this helper method in the Rocky release
+    def _get_lsp_backward_compat_sgs(self, ovn_port, port_object=None,
+                                     skip_trusted_port=True):
+        if ovn_const.OVN_SG_IDS_EXT_ID_KEY in ovn_port.external_ids:
+            return utils.get_ovn_port_security_groups(
+                ovn_port, skip_trusted_port=skip_trusted_port)
+        elif port_object is not None:
+            return utils.get_lsp_security_groups(
+                port_object, skip_trusted_port=skip_trusted_port)
+        return []
+
+    # TODO(lucasagomes): The ``port_object`` parameter was added to
+    # keep things backward compatible. Remove it in the Rocky release.
+    def update_port(self, port, qos_options=None, port_object=None):
         if utils.is_lsp_ignored(port):
             return
 
@@ -317,15 +341,26 @@ class OVNClient(object):
         external_ids = {ovn_const.OVN_PORT_NAME_EXT_ID_KEY: port['name'],
                         ovn_const.OVN_DEVID_EXT_ID_KEY: port['device_id'],
                         ovn_const.OVN_PROJID_EXT_ID_KEY: port['project_id'],
-                        ovn_const.OVN_CIDRS_EXT_ID_KEY: port_info.cidrs}
+                        ovn_const.OVN_CIDRS_EXT_ID_KEY: port_info.cidrs,
+                        ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
+                            port_info.device_owner,
+                        ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
+                            utils.ovn_name(port['network_id']),
+                        ovn_const.OVN_SG_IDS_EXT_ID_KEY:
+                            port_info.security_group_ids,
+                        ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(
+                            utils.get_revision_number(
+                                port, ovn_const.TYPE_PORTS))}
         admin_context = n_context.get_admin_context()
         sg_cache = {}
         subnet_cache = {}
 
+        check_rev_cmd = self._nb_idl.check_revision_number(
+            port['id'], port, ovn_const.TYPE_PORTS)
         with self._nb_idl.transaction(check_error=True) as txn:
+            txn.add(check_rev_cmd)
             columns_dict = {}
-            if port.get('device_owner') in [const.DEVICE_OWNER_ROUTER_INTF,
-                                            const.DEVICE_OWNER_ROUTER_GW]:
+            if utils.is_lsp_router_port(port):
                 port_info.options.update(
                     self._nb_idl.get_router_port_options(port['id']))
             else:
@@ -361,16 +396,26 @@ class OVNClient(object):
                     if_exists=False,
                     **columns_dict))
 
+            ovn_port = self._nb_idl.lookup('Logical_Switch_Port', port['id'])
             # Determine if security groups or fixed IPs are updated.
-            old_sg_ids = set(utils.get_lsp_security_groups(original_port))
+            old_sg_ids = set(self._get_lsp_backward_compat_sgs(
+                ovn_port, port_object=port_object))
             new_sg_ids = set(utils.get_lsp_security_groups(port))
             detached_sg_ids = old_sg_ids - new_sg_ids
             attached_sg_ids = new_sg_ids - old_sg_ids
-            is_fixed_ips_updated = \
-                original_port.get('fixed_ips') != port.get('fixed_ips')
-            is_allowed_ips_updated = \
-                original_port.get('allowed_address_pairs') != \
-                port.get('allowed_address_pairs')
+            old_fixed_ips = utils.remove_macs_from_lsp_addresses(
+                ovn_port.addresses)
+            new_fixed_ips = [x['ip_address'] for x in
+                             port.get('fixed_ips', [])]
+            old_allowed_address_pairs = (
+                utils.get_allowed_address_pairs_ip_addresses_from_ovn_port(
+                    ovn_port))
+            new_allowed_address_pairs = (
+                utils.get_allowed_address_pairs_ip_addresses(port))
+            is_fixed_ips_updated = (
+                sorted(old_fixed_ips) != sorted(new_fixed_ips))
+            is_allowed_ips_updated = (sorted(old_allowed_address_pairs) !=
+                                      sorted(new_allowed_address_pairs))
 
             # Refresh ACLs for changed security groups or fixed IPs.
             if detached_sg_ids or attached_sg_ids or is_fixed_ips_updated:
@@ -389,10 +434,10 @@ class OVNClient(object):
                                                  need_compare=True))
 
             # Refresh address sets for changed security groups or fixed IPs.
-            if (len(port.get('fixed_ips')) != 0 or
-                    len(original_port.get('fixed_ips')) != 0):
+            if len(old_fixed_ips) != 0 or len(new_fixed_ips) != 0:
                 addresses = ovn_acl.acl_port_ips(port)
-                addresses_old = ovn_acl.acl_port_ips(original_port)
+                addresses_old = utils.sort_ips_by_version(
+                    utils.get_ovn_port_addresses(ovn_port))
                 # Add current addresses to attached security groups.
                 for sg_id in attached_sg_ids:
                     for ip_version in addresses:
@@ -431,31 +476,54 @@ class OVNClient(object):
 
             if self.is_dns_required_for_port(port):
                 self.add_txns_to_sync_port_dns_records(
-                    txn, port, original_port=original_port)
-            elif self.is_dns_required_for_port(original_port):
+                    txn, port, original_port=port_object)
+            elif port_object and self.is_dns_required_for_port(port_object):
                 # We need to remove the old entries
-                self.add_txns_to_remove_port_dns_records(txn, original_port)
+                self.add_txns_to_remove_port_dns_records(txn, port_object)
 
-    def delete_port(self, port):
-        with self._nb_idl.transaction(check_error=True) as txn:
-            txn.add(self._nb_idl.delete_lswitch_port(port['id'],
-                    utils.ovn_name(port['network_id'])))
-            txn.add(self._nb_idl.delete_acl(
-                    utils.ovn_name(port['network_id']), port['id']))
+        if check_rev_cmd.result == ovn_const.TXN_COMMITTED:
+            if not utils.is_lsp_router_port(port):
+                db_rev.bump_revision(port, ovn_const.TYPE_PORTS)
 
-            if port.get('fixed_ips'):
-                addresses = ovn_acl.acl_port_ips(port)
-                # Set skip_trusted_port False for deleting port
-                for sg_id in utils.get_lsp_security_groups(port, False):
-                    for ip_version in addresses:
-                        if addresses[ip_version]:
-                            txn.add(self._nb_idl.update_address_set(
-                                name=utils.ovn_addrset_name(sg_id, ip_version),
-                                addrs_add=None,
-                                addrs_remove=addresses[ip_version]))
+    def _delete_port(self, port_id, port_object=None):
+        ovn_port = self._nb_idl.lookup('Logical_Switch_Port', port_id)
+        network_id = ovn_port.external_ids.get(
+            ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY)
 
-            if self.is_dns_required_for_port(port):
-                self.add_txns_to_remove_port_dns_records(txn, port)
+        # TODO(lucasagomes): For backward compatibility, if network_id
+        # is not in the OVNDB, look at the port_object
+        if not network_id and port_object:
+            network_id = port_object['network_id']
+
+        with self._nb_idl.transaction(check_error=True) as txn:
+            txn.add(self._nb_idl.delete_lswitch_port(
+                port_id, network_id))
+            txn.add(self._nb_idl.delete_acl(network_id, port_id))
+
+            addresses = utils.sort_ips_by_version(
+                utils.get_ovn_port_addresses(ovn_port))
+            sec_groups = self._get_lsp_backward_compat_sgs(
+                ovn_port, port_object=port_object, skip_trusted_port=False)
+            for sg_id in sec_groups:
+                for ip_version, addr_list in addresses.items():
+                    if not addr_list:
+                        continue
+                    txn.add(self._nb_idl.update_address_set(
+                        name=utils.ovn_addrset_name(sg_id, ip_version),
+                        addrs_add=None,
+                        addrs_remove=addr_list))
+
+            if port_object and self.is_dns_required_for_port(port_object):
+                self.add_txns_to_remove_port_dns_records(txn, port_object)
+
+    # TODO(lucasagomes): The ``port_object`` parameter was added to
+    # keep things backward compatible. Remove it in the Rocky release.
+    def delete_port(self, port_id, port_object=None):
+        try:
+            self._delete_port(port_id, port_object=port_object)
+        except idlutils.RowNotFound:
+            pass
+        db_rev.delete_revision(port_id)
 
     def _create_or_update_floatingip(self, floatingip, txn=None):
         router_id = floatingip.get('router_id')

networking_ovn/common/utils.py

@@ -12,6 +12,7 @@
 
 import os
 
+import netaddr
 from neutron_lib.api.definitions import extra_dhcp_opt as edo_ext
 from neutron_lib.api.definitions import l3
 from neutron_lib.api import validators
@@ -20,6 +21,7 @@ from neutron_lib import context as n_context
 from neutron_lib import exceptions as n_exc
 from neutron_lib.plugins import directory
 from neutron_lib.utils import net as n_utils
+from oslo_utils import netutils
 
 from networking_ovn._i18n import _
 from networking_ovn.common import constants
@@ -200,7 +202,74 @@ def get_ovn_ipv6_address_mode(address_mode):
 
 def get_revision_number(resource, resource_type):
     """Get the resource's revision number based on its type."""
-    if resource_type in (constants.TYPE_NETWORKS,):
+    if resource_type in (constants.TYPE_NETWORKS, constants.TYPE_PORTS):
         return resource['revision_number']
     else:
         raise ovn_exc.UnknownResourceType(resource_type=resource_type)
+
+
+def remove_macs_from_lsp_addresses(addresses):
+    """Remove the mac addreses from the Logical_Switch_Port addresses column.
+
+    :param addresses: The list of addresses from the Logical_Switch_Port.
+        Example: ["80:fa:5b:06:72:b7 158.36.44.22",
+                  "ff:ff:ff:ff:ff:ff 10.0.0.2"]
+    :returns: A list of IP addesses (v4 and v6)
+    """
+    ip_list = []
+    for addr in addresses:
+        ip_list.extend([x for x in addr.split() if
+                       (netutils.is_valid_ipv4(x) or
+                        netutils.is_valid_ipv6(x))])
+    return ip_list
+
+
+def get_allowed_address_pairs_ip_addresses(port):
+    """Return a list of IP addresses from port's allowed_address_pairs.
+
+    :param port: A neutron port
+    :returns: A list of IP addesses (v4 and v6)
+    """
+    return [x['ip_address'] for x in port.get('allowed_address_pairs', [])
+            if 'ip_address' in x]
+
+
+def get_allowed_address_pairs_ip_addresses_from_ovn_port(ovn_port):
+    """Return a list of IP addresses from ovn port.
+
+    Return a list of IP addresses equivalent of Neutron's port
+    allowed_address_pairs column using the data in the OVN port.
+
+    :param ovn_port: A OVN port
+    :returns: A list of IP addesses (v4 and v6)
+    """
+    addresses = remove_macs_from_lsp_addresses(ovn_port.addresses)
+    port_security = remove_macs_from_lsp_addresses(ovn_port.port_security)
+    return [x for x in port_security if x not in addresses]
+
+
+def get_ovn_port_security_groups(ovn_port, skip_trusted_port=True):
+    info = {'security_groups': ovn_port.external_ids.get(
+            constants.OVN_SG_IDS_EXT_ID_KEY, '').split(),
+            'device_owner': ovn_port.external_ids.get(
+            constants.OVN_DEVICE_OWNER_EXT_ID_KEY, '')}
+    return get_lsp_security_groups(info, skip_trusted_port=skip_trusted_port)
+
+
+def get_ovn_port_addresses(ovn_port):
+    addresses = remove_macs_from_lsp_addresses(ovn_port.addresses)
+    port_security = remove_macs_from_lsp_addresses(ovn_port.port_security)
+    return list(set(addresses + port_security))
+
+
+def sort_ips_by_version(addresses):
+    ip_map = {'ip4': [], 'ip6': []}
+    for addr in addresses:
+        ip_version = netaddr.IPNetwork(addr).version
+        ip_map['ip%d' % ip_version].append(addr)
+    return ip_map
+
+
+def is_lsp_router_port(port):
+    return port.get('device_owner') in [const.DEVICE_OWNER_ROUTER_INTF,
+                                        const.DEVICE_OWNER_ROUTER_GW]

networking_ovn/ml2/mech_driver.py

@@ -342,10 +342,16 @@ class OVNMechanismDriver(api.MechanismDriver):
         of the current transaction.
         """
         port = context.current
+        if utils.is_lsp_ignored(port):
+            return
         utils.validate_and_get_data_from_binding_profile(port)
         if self._is_port_provisioning_required(port, context.host):
             self._insert_port_provisioning_block(context._plugin_context, port)
 
+        if not utils.is_lsp_router_port(port):
+            db_rev.create_initial_revision(port['id'], ovn_const.TYPE_PORTS,
+                                           context._plugin_context.session)
+
     def _is_port_provisioning_required(self, port, host, original_host=None):
         vnic_type = port.get(portbindings.VNIC_TYPE, portbindings.VNIC_NORMAL)
         if vnic_type not in self.supported_vnic_types:
@@ -467,7 +473,7 @@ class OVNMechanismDriver(api.MechanismDriver):
         """
         port = context.current
         original_port = context.original
-        self._ovn_client.update_port(port, original_port)
+        self._ovn_client.update_port(port, port_object=original_port)
         self._notify_dhcp_updated(port['id'])
 
     def delete_port_postcommit(self, context):
@@ -483,7 +489,7 @@ class OVNMechanismDriver(api.MechanismDriver):
         deleted.
         """
         port = context.current
-        self._ovn_client.delete_port(port)
+        self._ovn_client.delete_port(port['id'], port_object=port)
 
     def bind_port(self, context):
         """Attempt to bind a port.

networking_ovn/ml2/qos_driver.py

@@ -141,7 +141,7 @@ class OVNQosDriver(object):
             if utils.is_network_device_port(port):
                 continue
             # Call into OVN client to update port
-            self._driver.update_port(port, port, qos_options=options)
+            self._driver.update_port(port, qos_options=options)
 
     def update_network(self, network):
         # Is qos service enabled
@@ -166,4 +166,4 @@ class OVNQosDriver(object):
         port_bindings = policy.get_bound_ports()
         for port_id in port_bindings:
             port = self._plugin.get_port(context, port_id)
-            self._driver.update_port(port, port, qos_options=options)
+            self._driver.update_port(port, qos_options=options)

networking_ovn/ovsdb/commands.py

@@ -20,6 +20,7 @@ from networking_ovn.common import utils
 
 RESOURCE_TYPE_MAP = {
     ovn_const.TYPE_NETWORKS: 'Logical_Switch',
+    ovn_const.TYPE_PORTS: 'Logical_Switch_Port',
 }
 
 

networking_ovn/ovsdb/impl_idl_ovn.py

@@ -529,13 +529,18 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
     def delete_nat_ip_from_lrport_peer_options(self, lport, nat_ip):
         return cmd.DeleteNatIpFromLRPortPeerOptionsCommand(self, lport, nat_ip)
 
-    def get_parent_port(self, lsp_name):
+    def get_lswitch_port(self, lsp_name):
         try:
-            lsp = idlutils.row_by_value(self.idl, 'Logical_Switch_Port',
-                                        'name', lsp_name)
-            return lsp.parent_name
+            return idlutils.row_by_value(self.idl, 'Logical_Switch_Port',
+                                         'name', lsp_name)
         except idlutils.RowNotFound:
+            return None
+
+    def get_parent_port(self, lsp_name):
+        lsp = self.get_lswitch_port(lsp_name)
+        if not lsp:
             return ''
+        return lsp.parent_name
 
     def get_lswitch(self, lswitch_name):
         try:

networking_ovn/ovsdb/ovn_api.py

@@ -555,6 +555,15 @@ class API(api.API):
                               OVN is equal or higher than the neutron object
         """
 
+    @abc.abstractmethod
+    def get_lswitch_port(self, lsp_name):
+        """Get a Logical Switch Port by its name.
+
+        :param lsp_name: The Logical Switch Port name
+        :type lsp_name: string
+        :returns: The Logical Switch Port row or None
+        """
+
 
 @six.add_metaclass(abc.ABCMeta)
 class SbAPI(api.API):

networking_ovn/tests/functional/test_revision_numbers.py

@@ -37,6 +37,26 @@ class TestRevisionNumbers(base.TestOVNFunctionalBase):
                     ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY) == name):
                 return row
 
+    def _create_port(self, name, net_id):
+        data = {'port': {'name': name,
+                         'tenant_id': self._tenant_id,
+                         'network_id': net_id}}
+        req = self.new_create_request('ports', data, self.fmt)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)['port']
+
+    def _update_port_name(self, port_id, new_name):
+        data = {'port': {'name': new_name}}
+        req = self.new_update_request('ports', data, port_id, self.fmt)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)['port']
+
+    def _find_port_row_by_name(self, name):
+        for row in self.nb_api._tables['Logical_Switch_Port'].rows.values():
+            if (row.external_ids.get(
+                    ovn_const.OVN_PORT_NAME_EXT_ID_KEY) == name):
+                return row
+
     def test_create_network(self):
         name = 'net1'
         neutron_net = self._create_network(name)
@@ -57,3 +77,26 @@ class TestRevisionNumbers(base.TestOVNFunctionalBase):
         self.assertEqual(str(3), ovn_revision)
         # Assert it also matches with the newest returned by neutron API
         self.assertEqual(str(updated_net['revision_number']), ovn_revision)
+
+    def test_create_port(self):
+        name = 'port1'
+        neutron_net = self._create_network('net1')
+        neutron_port = self._create_port(name, neutron_net['id'])
+        ovn_port = self._find_port_row_by_name(name)
+
+        ovn_revision = ovn_port.external_ids[ovn_const.OVN_REV_NUM_EXT_ID_KEY]
+        self.assertEqual(str(2), ovn_revision)
+        # Assert it also matches with the newest returned by neutron API
+        self.assertEqual(str(neutron_port['revision_number']), ovn_revision)
+
+    def test_update_port(self):
+        new_name = 'portnew1'
+        neutron_net = self._create_network('net1')
+        neutron_port = self._create_port('port1', neutron_net['id'])
+        updated_port = self._update_port_name(neutron_port['id'], new_name)
+        ovn_port = self._find_port_row_by_name(new_name)
+
+        ovn_revision = ovn_port.external_ids[ovn_const.OVN_REV_NUM_EXT_ID_KEY]
+        self.assertEqual(str(3), ovn_revision)
+        # Assert it also matches with the newest returned by neutron API
+        self.assertEqual(str(updated_port['revision_number']), ovn_revision)

networking_ovn/tests/unit/common/test_maintenance.py

@@ -34,6 +34,8 @@ class TestDBInconsistenciesPeriodics(db_base.DBTestCase,
         super(TestDBInconsistenciesPeriodics, self).setUp()
         self.net = self._make_network(
             self.fmt, name='net1', admin_state_up=True)['network']
+        self.port = self._make_port(
+            self.fmt, self.net['id'], name='port1')['port']
         self.fake_ovn_client = mock.Mock()
         self.periodic = maintenance.DBInconsistenciesPeriodics(
             self.fake_ovn_client)
@@ -87,3 +89,44 @@ class TestDBInconsistenciesPeriodics(db_base.DBTestCase,
 
     def test_fix_network_update(self):
         self._test_fix_create_update_network(ovn_rev=5, neutron_rev=7)
+
+    def _test_fix_create_update_port(self, ovn_rev, neutron_rev):
+        self.port['revision_number'] = neutron_rev
+
+        # Create an entry to the revision_numbers table and assert the
+        # initial revision_number for our test object is the expected
+        db_rev.create_initial_revision(
+            self.port['id'], constants.TYPE_PORTS, self.session,
+            revision_number=ovn_rev)
+        row = self.get_revision_row(self.port['id'])
+        self.assertEqual(ovn_rev, row.revision_number)
+
+        if ovn_rev < 0:
+            self.fake_ovn_client._nb_idl.get_lswitch_port.return_value = None
+        else:
+            fake_lsp = mock.Mock(external_ids={
+                constants.OVN_REV_NUM_EXT_ID_KEY: ovn_rev})
+            self.fake_ovn_client._nb_idl.get_lswitch_port.return_value = (
+                fake_lsp)
+
+        self.fake_ovn_client._plugin.get_port.return_value = self.port
+        self.periodic._fix_create_update_port(row)
+
+        # Since the revision number was < 0, make sure create_port()
+        # is invoked with the latest version of the object in the neutron
+        # database
+        if ovn_rev < 0:
+            self.fake_ovn_client.create_port.assert_called_once_with(
+                self.port)
+        # If the revision number is > 0 it means that the object already
+        # exist and we just need to update to match the latest in the
+        # neutron database so, update_port() should be called.
+        else:
+            self.fake_ovn_client.update_port.assert_called_once_with(
+                self.port)
+
+    def test_fix_port_create(self):
+        self._test_fix_create_update_port(ovn_rev=-1, neutron_rev=2)
+
+    def test_fix_port_update(self):
+        self._test_fix_create_update_port(ovn_rev=5, neutron_rev=7)

networking_ovn/tests/unit/fakes.py

@@ -17,6 +17,9 @@ import copy
 import mock
 from oslo_utils import uuidutils
 
+from networking_ovn.common import constants as ovn_const
+from networking_ovn.common import utils
+
 
 class FakeOvsdbNbOvnIdl(object):
 
@@ -100,6 +103,7 @@ class FakeOvsdbNbOvnIdl(object):
         self.get_floatingip = mock.Mock()
         self.get_floatingip.return_value = None
         self.check_revision_number = mock.Mock()
+        self.lookup = mock.MagicMock()
         # TODO(lucasagomes): The get_floatingip_by_ips() method is part
         # of a backwards compatibility layer for the Pike -> Queens release,
         # remove it in the Rocky release.
@@ -586,3 +590,59 @@ class FakeFloatingIp(object):
 
         return FakeResource(info=copy.deepcopy(fip_attrs),
                             loaded=True)
+
+
+class FakeOVNPort(object):
+    """Fake one or more ports."""
+
+    @staticmethod
+    def create_one_port(attrs=None):
+        """Create a fake ovn port.
+
+        :param Dictionary attrs:
+            A dictionary with all attributes
+        :return:
+            A FakeResource object faking the port
+        """
+        attrs = attrs or {}
+
+        # Set default attributes.
+        fake_uuid = uuidutils.generate_uuid()
+        port_attrs = {
+            'addresses': [],
+            'dhcpv4_options': '',
+            'dhcpv6_options': [],
+            'enabled': True,
+            'external_ids': {},
+            'name': fake_uuid,
+            'options': {},
+            'parent_name': [],
+            'port_security': [],
+            'tag': [],
+            'tag_request': [],
+            'type': '',
+            'up': False,
+        }
+
+        # Overwrite default attributes.
+        port_attrs.update(attrs)
+        return type('Logical_Switch_Port', (object, ), port_attrs)
+
+    @staticmethod
+    def from_neutron_port(port):
+        """Create a fake ovn port based on a neutron port."""
+        external_ids = {
+            ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
+                utils.ovn_name(port['network_id']),
+            ovn_const.OVN_SG_IDS_EXT_ID_KEY:
+                ' '.join(port['security_groups']),
+            ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
+                port.get('device_owner', '')}
+        addresses = [port['mac_address'], ]
+        addresses += [x['ip_address'] for x in port.get('fixed_ips', [])]
+        port_security = (
+            addresses + [x['ip_address'] for x in
+                         port.get('allowed_address_pairs', [])])
+        return FakeOVNPort.create_one_port(
+            {'external_ids': external_ids, 'addresses': addresses,
+             'port_security': port_security})

networking_ovn/tests/unit/ml2/test_mech_driver.py

@@ -537,6 +537,10 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
                 with self.port(subnet=subnet1,
                                set_context=True, tenant_id='test') as port1:
                     sg_id = port1['port']['security_groups'][0]
+                    fake_lsp = (
+                        fakes.FakeOVNPort.from_neutron_port(
+                            port1['port']))
+                    self.nb_ovn.lookup.return_value = fake_lsp
 
                     # Remove the default security group.
                     self.nb_ovn.set_lswitch_port.reset_mock()
@@ -555,6 +559,7 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
                     self.nb_ovn.set_lswitch_port.reset_mock()
                     self.nb_ovn.update_acls.reset_mock()
                     self.nb_ovn.update_address_set.reset_mock()
+                    fake_lsp.external_ids.pop(ovn_const.OVN_SG_IDS_EXT_ID_KEY)
                     data = {'port': {'security_groups': [sg_id]}}
                     self._update('ports', port1['port']['id'], data)
                     self.assertEqual(
@@ -569,6 +574,11 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
             with self.subnet(network=net1) as subnet1:
                 with self.port(subnet=subnet1,
                                set_context=True, tenant_id='test') as port1:
+                    fake_lsp = (
+                        fakes.FakeOVNPort.from_neutron_port(
+                            port1['port']))
+                    self.nb_ovn.lookup.return_value = fake_lsp
+
                     # Update the port name.
                     self.nb_ovn.set_lswitch_port.reset_mock()
                     self.nb_ovn.update_acls.reset_mock()
@@ -601,6 +611,10 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
                                arg_list=('security_groups',),
                                set_context=True, tenant_id='test',
                                **kwargs) as port1:
+                    fake_lsp = (
+                        fakes.FakeOVNPort.from_neutron_port(
+                            port1['port']))
+                    self.nb_ovn.lookup.return_value = fake_lsp
                     self.nb_ovn.delete_lswitch_port.reset_mock()
                     self.nb_ovn.delete_acl.reset_mock()
                     self.nb_ovn.update_address_set.reset_mock()
@@ -616,6 +630,10 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
             with self.subnet(network=net1) as subnet1:
                 with self.port(subnet=subnet1,
                                set_context=True, tenant_id='test') as port1:
+                    fake_lsp = (
+                        fakes.FakeOVNPort.from_neutron_port(
+                            port1['port']))
+                    self.nb_ovn.lookup.return_value = fake_lsp
                     self.nb_ovn.delete_lswitch_port.reset_mock()
                     self.nb_ovn.delete_acl.reset_mock()
                     self.nb_ovn.update_address_set.reset_mock()
@@ -1249,14 +1267,10 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
                                     mock_notify_dhcp):
         fake_port = fakes.FakePort.create_one_port(
             attrs={'status': const.PORT_STATUS_ACTIVE}).info()
-        fake_original_port = fakes.FakePort.create_one_port(
-            attrs={'status': const.PORT_STATUS_DOWN}).info()
-        fake_ctx = mock.Mock(current=fake_port, original=fake_original_port)
-
+        fake_ctx = mock.Mock(current=fake_port)
         self.mech_driver.update_port_postcommit(fake_ctx)
-
         mock_update_port.assert_called_once_with(
-            fake_port, fake_original_port)
+            fake_port, port_object=fake_ctx.original)
         mock_notify_dhcp.assert_called_once_with(fake_port['id'])
 
 

networking_ovn/tests/unit/ml2/test_qos_driver.py

@@ -238,5 +238,4 @@ class TestOVNQosDriver(base.BaseTestCase):
                 context, self.network_id, {})
             get_bound_ports.assert_called_once()
             get_port.assert_called_once_with(context, self.port_id)
-            update_port.assert_called_once_with(self.port, self.port,
-                                                qos_options={})
+            update_port.assert_called_once_with(self.port, qos_options={})

networking_ovn/tests/unit/test_ovn_parent_tag.py

@@ -13,6 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import mock
 
 from networking_ovn.common import constants as ovn_const
 from networking_ovn.tests.unit.ml2 import test_mech_driver
@@ -32,7 +33,8 @@ class TestOVNParentTagPortBinding(test_mech_driver.OVNMechanismDriverTestCase):
                     arg_list=(OVN_PROFILE,),
                     **binding)
 
-    def test_create_port_with_parent_and_tag(self):
+    @mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.get_port')
+    def test_create_port_with_parent_and_tag(self, mock_get_port):
         binding = {OVN_PROFILE: {"parent_name": '', 'tag': 1}}
         with self.network() as n:
             with self.subnet(n) as s:
@@ -44,6 +46,7 @@ class TestOVNParentTagPortBinding(test_mech_driver.OVNMechanismDriverTestCase):
                     port = self.deserialize(self.fmt, res)
                     self.assertEqual(port['port'][OVN_PROFILE],
                                      binding[OVN_PROFILE])
+                    mock_get_port.assert_called_with(mock.ANY, p['port']['id'])
 
     def test_create_port_with_invalid_tag(self):
         binding = {OVN_PROFILE: {"parent_name": '', 'tag': 'a'}}