ovn: revert to stateful dnat_and_snat

This is an effective revert of:
I312a950131d62d93fb4bc121bc5e60febb8d35ee
"ovn: use stateless NAT rules for FIPs".

The performance benefits promised by the "reverted" patch never
materialized. On the contrary, the discussion in [1] revealed that the
switch to stateless=true made it impossible to fully hw offload nat
rules, while it's possible with stateless=false.  Specifically, see this
comment [2].

Since at this point it's unclear if keeping stateless=true as an option
is beneficial for any case, even when w/o hw offload, and to avoid
complexity of introducing a config option for unclear benefit, this
patch reverts the effects of the original patch, switching all
dnat_and_snat objects to implicit stateless=false state.

This patch cannot be a clean revert because of the need for db
migration.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2004995
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2004995#c18

Change-Id: I9e6e05b7a4f36383a44bd80f07d25052b17bdfa0

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

@@ -290,10 +290,10 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
             raise RuntimeError(_("Currently only supports "
                                  "delete by lport-name"))
 
-    def get_all_stateful_fip_nats(self):
+    def get_all_stateless_fip_nats(self):
         cmd = self.db_find('NAT',
             ('external_ids', '!=', {ovn_const.OVN_FIP_EXT_ID_KEY: ''}),
-            ('options', '!=', {'stateless': ''}),
+            ('options', '=', {'stateless': 'true'}),
             ('type', '=', 'dnat_and_snat')
         )
         return cmd.execute(check_error=True)

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

@@ -276,11 +276,11 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
     # The migration will run just once per neutron-server instance. If the lock
     # is held by some other neutron-server instance in the cloud, we'll attempt
     # to perform the migration every 10 seconds until completed.
-    # TODO(ihrachys): Remove the migration to stateless fips at some point.
+    # TODO(ihrachys): Remove the migration to stateful fips in Z+1.
     @periodics.periodic(spacing=10, run_immediately=True)
     @rerun_on_schema_updates
-    def migrate_to_stateless_fips(self):
+    def migrate_to_stateful_fips(self):
-        """Perform the migration from stateful to stateless Floating IPs. """
+        """Perform the migration from stateless to stateful Floating IPs. """
         # Only the worker holding a valid lock within OVSDB will perform the
         # migration.
         if not self.has_lock:
@@ -290,7 +290,7 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
         nb_sync = ovn_db_sync.OvnNbSynchronizer(
             self._ovn_client._plugin, self._nb_idl, self._ovn_client._sb_idl,
             None, None)
-        nb_sync.migrate_to_stateless_fips(admin_context)
+        nb_sync.migrate_to_stateful_fips(admin_context)
         raise periodics.NeverAgain()
 
     # The migration will run just once per neutron-server instance. If the lock

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

@@ -804,8 +804,7 @@ class OVNClient(object):
                    'logical_ip': floatingip['fixed_ip_address'],
                    'external_ip': floatingip['floating_ip_address'],
                    'logical_port': floatingip['port_id'],
-                   'external_ids': ext_ids,
+                   'external_ids': ext_ids}
-                   'options': {'stateless': 'true'}}
 
         if ovn_conf.is_ovn_distributed_floating_ip():
             if self._nb_idl.lsp_get_up(floatingip['port_id']).execute():

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

@@ -106,7 +106,7 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
         self.sync_port_dns_records(ctx)
         self.sync_acls(ctx)
         self.sync_routers_and_rports(ctx)
-        self.migrate_to_stateless_fips(ctx)
+        self.migrate_to_stateful_fips(ctx)
         self.sync_port_qos_policies(ctx)
         self.sync_fip_qos_policies(ctx)
 
@@ -1190,14 +1190,13 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
                     txn.add(self.ovn_api.pg_add_ports(
                         utils.ovn_port_group_name(sg), port['id']))
 
-    def migrate_to_stateless_fips(self, ctx):
+    def migrate_to_stateful_fips(self, ctx):
-        # This routine will set options:stateless=true for all dnat_and_snats
+        # This routine will clear options:stateless=true for all dnat_and_snats
-        # that belong to neutron fips.
+        # that belong to neutron fips. Since we don't set any other options,
+        # just clear the whole column.
         with self.ovn_api.transaction(check_error=True) as txn:
-            for nat in self.ovn_api.get_all_stateful_fip_nats():
+            for nat in self.ovn_api.get_all_stateless_fip_nats():
-                txn.add(self.ovn_api.db_set(
+                txn.add(self.ovn_api.db_clear('NAT', nat['_uuid'], 'options'))
-                    'NAT', nat['_uuid'],
-                    ('options', {'stateless': 'true'})))
 
     def migrate_to_port_groups(self, ctx):
         # This routine is responsible for migrating the current Security

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py

@@ -1749,6 +1749,62 @@ class TestOvnNbSync(base.TestOVNFunctionalBase):
         nb_synchronizer.sync_fip_qos_policies(ctx)
         self._validate_qos_records()
 
+    def test_fip_nat_revert_to_stateful(self):
+        res = self._create_network(self.fmt, 'n1_ext', True,
+                                   arg_list=('router:external', ),
+                                   **{'router:external': True})
+        net_ext = self.deserialize(self.fmt, res)['network']
+        res = self._create_subnet(self.fmt, net_ext['id'], '10.0.0.0/24')
+        subnet_ext = self.deserialize(self.fmt, res)['subnet']
+
+        res = self._create_network(self.fmt, 'n1_int', True)
+        net_int = self.deserialize(self.fmt, res)['network']
+        self._create_subnet(self.fmt, net_int['id'], '10.10.0.0/24')
+
+        port = self._make_port(self.fmt, net_int['id'],
+                               name='test-port')['port']
+
+        data = {'name': 'r1', 'admin_state_up': True,
+                'tenant_id': self._tenant_id,
+                'external_gateway_info': {
+                    'enable_snat': True,
+                    'network_id': net_ext['id'],
+                    'external_fixed_ips': [{'ip_address': '10.0.0.5',
+                                            'subnet_id': subnet_ext['id']}]}
+                }
+        router = self.l3_plugin.create_router(self.context, {'router': data})
+        self.l3_plugin.add_router_interface(
+            self.context, router['id'], {'port_id': port['id']})
+
+        body = {'tenant_id': self._tenant_id,
+                'floating_network_id': net_ext['id'],
+                'port_id': port['id']}
+        self.l3_plugin.create_floatingip(self.context, {'floatingip': body})
+
+        self.assertEqual(0, len(self.nb_api.get_all_stateless_fip_nats()))
+
+        def get_all_stateful_fip_nats():
+            cmd = self.nb_api.db_find('NAT',
+                ('external_ids', '!=', {ovn_const.OVN_FIP_EXT_ID_KEY: ''}),
+                ('options', '=', {}),
+                ('type', '=', 'dnat_and_snat'))
+            return cmd.execute(check_error=True)
+
+        with self.nb_api.transaction(check_error=True) as txn:
+            for nat in get_all_stateful_fip_nats():
+                txn.add(self.nb_api.db_set(
+                    'NAT', nat['_uuid'],
+                    ('options', {'stateless': 'true'})))
+
+        self.assertEqual(1, len(self.nb_api.get_all_stateless_fip_nats()))
+
+        nb_synchronizer = ovn_db_sync.OvnNbSynchronizer(
+            self.plugin, self.mech_driver.nb_ovn, self.mech_driver.sb_ovn,
+            'repair', self.mech_driver)
+        nb_synchronizer.migrate_to_stateful_fips(self.context)
+
+        self.assertEqual(0, len(self.nb_api.get_all_stateless_fip_nats()))
+
 
 class TestOvnSbSync(base.TestOVNFunctionalBase):
 

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

@@ -141,34 +141,34 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
                                                      migration_expected=False,
                                                      never_again=False)
 
-    def _test_migrate_to_stateless_fips_helper(
+    def _test_migrate_to_stateful_fips_helper(
             self, migration_expected, never_again):
         with mock.patch.object(ovn_db_sync.OvnNbSynchronizer,
-                               'migrate_to_stateless_fips') as mtsf:
+                               'migrate_to_stateful_fips') as mtsf:
             if never_again:
                 self.assertRaises(periodics.NeverAgain,
-                                  self.periodic.migrate_to_stateless_fips)
+                                  self.periodic.migrate_to_stateful_fips)
             else:
-                self.periodic.migrate_to_stateless_fips()
+                self.periodic.migrate_to_stateful_fips()
 
             if migration_expected:
                 mtsf.assert_called_once_with(mock.ANY)
             else:
                 mtsf.assert_not_called()
 
-    def test_migrate_to_stateless_fips(self):
+    def test_migrate_to_stateful_fips(self):
         # Check normal migration path: if the migration has to be done, it will
         # take place and won't be attempted in the future.
-        self._test_migrate_to_stateless_fips_helper(migration_expected=True,
+        self._test_migrate_to_stateful_fips_helper(migration_expected=True,
-                                                    never_again=True)
+                                                   never_again=True)
 
-    def test_migrate_to_stateless_fips_no_lock(self):
+    def test_migrate_to_stateful_fips_no_lock(self):
         with mock.patch.object(maintenance.DBInconsistenciesPeriodics,
                                'has_lock', mock.PropertyMock(
                                    return_value=False)):
             # Check that if this worker doesn't have the lock, it won't
             # perform the migration and it will try again later.
-            self._test_migrate_to_stateless_fips_helper(
+            self._test_migrate_to_stateful_fips_helper(
                 migration_expected=False, never_again=False)
 
     def _test_fix_create_update_network(self, ovn_rev, neutron_rev):

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

@@ -453,8 +453,8 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase):
         ovn_api.get_all_logical_switches_with_ports.return_value = (
             self.lswitches_with_ports)
 
-        ovn_api.get_all_stateful_fip_nats = mock.Mock()
+        ovn_api.get_all_stateless_fip_nats = mock.Mock()
-        ovn_api.get_all_stateful_fip_nats.return_value = []
+        ovn_api.get_all_stateless_fip_nats.return_value = []
         ovn_api.get_all_logical_routers_with_rports = mock.Mock()
         ovn_api.get_all_logical_routers_with_rports.return_value = (
             self.lrouters_with_rports)

neutron/tests/unit/services/ovn_l3/test_plugin.py

@@ -168,7 +168,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
                         self.fake_floating_ip['port_id'],
                     ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(
                         self.fake_floating_ip['router_id'])},
-                'options': {'stateless': 'true'}
             }))
         self.l3_inst = directory.get_plugin(plugin_constants.L3)
         self.lb_id = uuidutils.generate_uuid()
@@ -958,8 +957,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.0.0.10',
             external_ip='192.168.0.10',
             logical_port='port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     def test_create_floatingip_distributed(self):
         self.l3_inst._nb_ovn.is_col_present.return_value = True
@@ -983,8 +981,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             'neutron-router-id', type='dnat_and_snat', logical_ip='10.0.0.10',
             external_ip='192.168.0.10', external_mac='00:01:02:03:04:05',
             logical_port='port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     def test_create_floatingip_distributed_logical_port_down(self):
         # Check that when the port is down, the external_mac field is not
@@ -1012,8 +1009,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             'neutron-router-id', type='dnat_and_snat', logical_ip='10.0.0.10',
             external_ip='192.168.0.10',
             logical_port='port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     def test_create_floatingip_external_ip_present_in_nat_rule(self):
         self.l3_inst._nb_ovn.is_col_present.return_value = True
@@ -1038,8 +1034,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.0.0.10',
             external_ip='192.168.0.10',
             logical_port='port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     def test_create_floatingip_external_ip_present_type_snat(self):
         self.l3_inst._nb_ovn.is_col_present.return_value = True
@@ -1065,8 +1060,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.0.0.10',
             external_ip='192.168.0.10',
             logical_port='port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     def test_create_floatingip_lsp_external_id(self):
         foo_lport = fake_resources.FakeOvsdbRow.create_one_ovsdb_row()
@@ -1104,8 +1098,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             external_ip='192.168.0.10',
             logical_ip='10.0.0.10',
             type='dnat_and_snat',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     def test_create_floatingip_lb_vip_fip(self):
         config.cfg.CONF.set_override(
@@ -1135,8 +1128,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.0.0.10',
             logical_port='port_id',
             type='dnat_and_snat',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
         self.l3_inst._nb_ovn.db_find_rows.assert_called_with(
             'NAT', ('external_ids', '=', {ovn_const.OVN_FIP_PORT_EXT_ID_KEY:
                                           self.member_lsp.name}))
@@ -1246,8 +1238,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.10.10.10',
             external_ip='192.168.0.10',
             logical_port='new-port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
                 'update_floatingip')
@@ -1273,8 +1264,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.10.10.10',
             external_ip='192.168.0.10',
             logical_port='new-port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     @mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.get_network')
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
@@ -1308,7 +1298,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             'neutron-new-router-id', type='dnat_and_snat',
             logical_ip='10.10.10.10', external_ip='192.168.0.10',
             external_mac='00:01:02:03:04:05', logical_port='new-port_id',
-            external_ids=expected_ext_ids, options={'stateless': 'true'})
+            external_ids=expected_ext_ids)
 
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
                 'update_floatingip')
@@ -1342,8 +1332,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.10.10.10',
             external_ip='192.168.0.10',
             logical_port='foo',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
                 'update_floatingip')
@@ -1379,8 +1368,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
             logical_ip='10.10.10.10',
             external_ip='192.168.0.10',
             logical_port='port_id',
-            external_ids=expected_ext_ids,
+            external_ids=expected_ext_ids)
-            options={'stateless': 'true'})
 
     @mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin.get_floatingips')
     def test_disassociate_floatingips(self, gfs):

releasenotes/notes/revert-ovn-stateless-nat-88076892fd6f7354.yaml

@@ -0,0 +1,8 @@
+---
+other:
+  - |
+    OVN driver reverted to using stateful NAT for floating IP implementation.
+    The previous switch to stateless didn't materialize the expected
+    performance benefits and instead introduced problems with potential
+    hardware offloading.
+