Set "type=virtual" for OVN LSP with parent ports

This is a follow-up of [1]. Before this patch, any virtual logical
switch port that was updated and the "device_owner" was not and empty
string, had its type set to '' (empty string).

This maintenance task, that is executed only once, lists all logical
switch ports, checks the presence or not of virtual parents and
sets the type to "virtual" if needed.

Related-Bug: #1973276

[1]https://review.opendev.org/c/openstack/neutron/+/841711

Change-Id: I6cf1167d556f0c2c2aa2013f05c809648020b377

neutron/common/ovn/utils.py

@@ -422,6 +422,13 @@ def get_ovn_port_addresses(ovn_port):
     return list(set(addresses + port_security))
 
 
+def get_virtual_port_parents(nb_idl, virtual_ip, network_id, port_id):
+    ls = nb_idl.ls_get(ovn_name(network_id)).execute(check_error=True)
+    return [lsp.name for lsp in ls.ports
+            if lsp.name != port_id and
+            virtual_ip in get_ovn_port_addresses(lsp)]
+
+
 def sort_ips_by_version(addresses):
     ip_map = {'ip4': [], 'ip6': []}
     for addr in addresses:

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

@@ -783,6 +783,39 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
 
         raise periodics.NeverAgain()
 
+    # TODO(ralonsoh): Remove this in the Z+4 cycle
+    @periodics.periodic(spacing=600, run_immediately=True)
+    def update_port_virtual_type(self):
+        """Set type=virtual to those ports with parents
+        Before LP#1973276, any virtual port with "device_owner" defined, lost
+        its type=virtual. This task restores the type for those ports updated
+        before the fix https://review.opendev.org/c/openstack/neutron/+/841711.
+        """
+        if not self.has_lock:
+            return
+
+        context = n_context.get_admin_context()
+        cmds = []
+        for lsp in self._nb_idl.lsp_list().execute(check_error=True):
+            if lsp.type != '':
+                continue
+
+            port = self._ovn_client._plugin.get_port(context, lsp.name)
+            for ip in port.get('fixed_ips', []):
+                if utils.get_virtual_port_parents(
+                        self._nb_idl, ip['ip_address'], port['network_id'],
+                        port['id']):
+                    cmds.append(self._nb_idl.db_set(
+                        'Logical_Switch_Port', lsp.uuid,
+                        ('type', ovn_const.LSP_TYPE_VIRTUAL)))
+                    break
+
+        if cmds:
+            with self._nb_idl.transaction(check_error=True) as txn:
+                for cmd in cmds:
+                    txn.add(cmd)
+        raise periodics.NeverAgain()
+
 
 class HashRingHealthCheckPeriodics(object):
 

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

@@ -215,13 +215,6 @@ class OVNClient(object):
             external_ids=subnet_dhcp_options['external_ids'])
         return {'cmd': add_dhcp_opts_cmd}
 
-    def get_virtual_port_parents(self, virtual_ip, port):
-        ls = self._nb_idl.ls_get(utils.ovn_name(port['network_id'])).execute(
-            check_error=True)
-        return [lsp.name for lsp in ls.ports
-                if lsp.name != port['id'] and
-                virtual_ip in utils.get_ovn_port_addresses(lsp)]
-
     def determine_bind_host(self, port, port_context=None):
         """Determine which host the port should be bound to.
 
@@ -299,7 +292,8 @@ class OVNClient(object):
                                          subnet['cidr'].split('/')[1])
 
                 # Check if the port being created is a virtual port
-                parents = self.get_virtual_port_parents(ip_addr, port)
+                parents = utils.get_virtual_port_parents(
+                    self._nb_idl, ip_addr, port['network_id'], port['id'])
                 if not parents:
                     continue
 

neutron/tests/unit/db/test_db_base_plugin_v2.py

@@ -50,6 +50,7 @@ from neutron.api import api_common
 from neutron.api import extensions
 from neutron.api.v2 import router
 from neutron.common import ipv6_utils
+from neutron.common.ovn import utils as ovn_utils
 from neutron.common import test_lib
 from neutron.common import utils
 from neutron.conf import policies
@@ -63,7 +64,6 @@ from neutron.ipam.drivers.neutrondb_ipam import driver as ipam_driver
 from neutron.ipam import exceptions as ipam_exc
 from neutron.objects import network as network_obj
 from neutron.objects import router as l3_obj
-from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
 from neutron import policy
 from neutron import quota
 from neutron.quota import resource_registry
@@ -1075,8 +1075,7 @@ class TestPortsV2(NeutronDbPluginV2TestCase):
     def setUp(self, **kwargs):
         super().setUp(**kwargs)
         self.mock_vp_parents = mock.patch.object(
-            ovn_client.OVNClient, 'get_virtual_port_parents',
+            ovn_utils, 'get_virtual_port_parents', return_value=None).start()
-            return_value=None).start()
 
     def test_create_port_json(self):
         keys = [('admin_state_up', True), ('status', self.port_create_status)]

neutron/tests/unit/extensions/test_portsecurity.py

@@ -25,11 +25,11 @@ from neutron_lib.exceptions import port_security as psec_exc
 from neutron_lib.plugins import directory
 from webob import exc
 
+from neutron.common.ovn import utils as ovn_utils
 from neutron.db import db_base_plugin_v2
 from neutron.db import portsecurity_db
 from neutron.db import securitygroups_db
 from neutron.extensions import securitygroup as ext_sg
-from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
 from neutron import quota
 from neutron.tests.unit.db import test_db_base_plugin_v2
 from neutron.tests.unit.extensions import test_securitygroup
@@ -188,8 +188,7 @@ class TestPortSecurity(PortSecurityDBTestCase):
         self.mock_quota_make_res = make_res.start()
         self.mock_quota_commit_res = commit_res.start()
         self.mock_vp_parents = mock.patch.object(
-            ovn_client.OVNClient, 'get_virtual_port_parents',
+            ovn_utils, 'get_virtual_port_parents', return_value=None).start()
-            return_value=None).start()
 
     def test_create_network_with_portsecurity_mac(self):
         res = self._create_network('json', 'net1', True)

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -686,3 +686,22 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
 
         self.fake_ovn_client._nb_idl.set_lswitch_port.assert_has_calls(
             expected_calls)
+
+    @mock.patch.object(utils, 'get_virtual_port_parents',
+                       return_value=[mock.ANY])
+    def test_update_port_virtual_type(self, *args):
+        nb_idl = self.fake_ovn_client._nb_idl
+        lsp0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp0', 'type': ''})
+        lsp1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp1', 'type': constants.LSP_TYPE_VIRTUAL})
+        port0 = {'fixed_ips': [{'ip_address': mock.ANY}],
+                 'network_id': mock.ANY, 'id': mock.ANY}
+        nb_idl.lsp_list.return_value.execute.return_value = (lsp0, lsp1)
+        self.fake_ovn_client._plugin.get_port.return_value = port0
+
+        self.assertRaises(
+            periodics.NeverAgain, self.periodic.update_port_virtual_type)
+        expected_calls = [mock.call('Logical_Switch_Port', lsp0.uuid,
+                                    ('type', constants.LSP_TYPE_VIRTUAL))]
+        nb_idl.db_set.assert_has_calls(expected_calls)

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py

@@ -93,6 +93,8 @@ class MechDriverSetupBase(abc.ABC):
         agent1 = self._add_agent('agent1')
         neutron_agent.AgentCache().get_agents = mock.Mock()
         neutron_agent.AgentCache().get_agents.return_value = [agent1]
+        self.mock_vp_parents = mock.patch.object(
+            ovn_utils, 'get_virtual_port_parents', return_value=None).start()
 
     def _add_chassis(self, nb_cfg, name=None):
         chassis_private = mock.Mock()
@@ -188,9 +190,6 @@ class TestOVNMechanismDriverBase(MechDriverSetupBase,
         p = mock.patch.object(ovn_revision_numbers_db, 'bump_revision')
         p.start()
         self.addCleanup(p.stop)
-        self.mock_vp_parents = mock.patch.object(
-            ovn_client.OVNClient, 'get_virtual_port_parents',
-            return_value=None).start()
 
     def test_delete_mac_binding_entries(self):
         self.config(group='ovn', ovn_sb_private_key=None)
@@ -218,12 +217,6 @@ class TestOVNMechanismDriverBase(MechDriverSetupBase,
 
 class TestOVNMechanismDriver(TestOVNMechanismDriverBase):
 
-    def setUp(self):
-        super().setUp()
-        self.mock_vp_parents = mock.patch.object(
-            ovn_client.OVNClient, 'get_virtual_port_parents',
-            return_value=None).start()
-
     @mock.patch.object(ovsdb_monitor.OvnInitPGNbIdl, 'from_server')
     @mock.patch.object(ovsdb_monitor, 'short_living_ovsdb_api')
     def test__create_neutron_pg_drop_non_existing(
@@ -2483,9 +2476,6 @@ class OVNMechanismDriverTestCase(MechDriverSetupBase,
         p = mock.patch.object(ovn_utils, 'get_revision_number', return_value=1)
         p.start()
         self.addCleanup(p.stop)
-        self.mock_vp_parents = mock.patch.object(
-            ovn_client.OVNClient, 'get_virtual_port_parents',
-            return_value=None).start()
 
 
 class TestOVNMechanismDriverBasicGet(test_plugin.TestMl2BasicGet,
@@ -2797,9 +2787,7 @@ class TestOVNMechanismDriverSegment(MechDriverSetupBase,
                          segment_id=self.seg_2['id']) as subnet:
             self.sub_2 = subnet
 
-    @mock.patch.object(ovn_client.OVNClient, 'get_virtual_port_parents',
+    def test_create_segments_subnet_metadata_ip_allocation(self):
-                       return_value=[])
-    def test_create_segments_subnet_metadata_ip_allocation(self, *args):
         self._test_segments_helper()
         ovn_nb_api = self.mech_driver.nb_ovn
 
@@ -3502,9 +3490,6 @@ class TestOVNMechanismDriverSecurityGroup(MechDriverSetupBase,
         super(TestOVNMechanismDriverSecurityGroup, self).setUp()
         self.ctx = context.get_admin_context()
         revision_plugin.RevisionPlugin()
-        self.mock_vp_parents = mock.patch.object(
-            ovn_client.OVNClient, 'get_virtual_port_parents',
-            return_value=None).start()
 
     def _delete_default_sg_rules(self, security_group_id):
         res = self._list(
@@ -3871,9 +3856,7 @@ class TestOVNMechanismDriverMetadataPort(MechDriverSetupBase,
             with self.network():
                 self.assertEqual(0, self.nb_ovn.create_lswitch_port.call_count)
 
-    @mock.patch.object(ovn_client.OVNClient, 'get_virtual_port_parents',
+    def test_metadata_ip_on_subnet_create(self):
-                       return_value=[])
-    def test_metadata_ip_on_subnet_create(self, *args):
         """Check metadata port update.
 
         Check that the metadata port is updated with a new IP address when a
@@ -4038,11 +4021,9 @@ class TestOVNVVirtualPort(OVNMechanismDriverTestCase):
             '10.0.0.1', '10.0.0.0/24')['subnet']
 
     @mock.patch.object(ovn_client.OVNClient, 'determine_bind_host')
-    @mock.patch.object(ovn_client.OVNClient, 'get_virtual_port_parents')
+    def test_create_port_with_virtual_type_and_options(self, *args):
-    def test_create_port_with_virtual_type_and_options(
-            self, mock_get_parents, mock_determine_bind_host):
         fake_parents = ['parent-0', 'parent-1']
-        mock_get_parents.return_value = fake_parents
+        self.mock_vp_parents.return_value = fake_parents
         for device_owner in ('', 'myVIPowner'):
             port = {'id': 'virt-port',
                     'mac_address': '00:00:00:00:00:00',