[OVN] Warn about invalid OVN and FIP PF config during start of Neutron

In case when port_forwarding service plugin is enabled and vlan or flat
network (provider network types) is configured as one of the
tenant_network_types in the ML2 config there is an issue with
centralized and distributed traffic.
FIP port forwarding in ovn backend are implemented as OVN Load balancers
thus are always centralized but if "enable_distributed_floating_ip" is
set to True, FIPs are distributed. And in such case it won't work as
expected as either it tries to send FIP PF's traffic as distributed when
"reside-on-redirect-chassis" for LRP is set to "false" or
tries to centralized everything (even FIP which should be distributed)
when "reside-on-redirect-chassis" is set to "true".

It's not really easy to avoid that issue from the code so this patch
adds warning in the upgrade checks and also log warning about it during
start of the neutron server process to at least warn cloud admin that
such potential issue may happen in the cloud.

Related-Bug: #2028846
Change-Id: I398f3f676c59dc794cf03320fa45efc7b22fc003

neutron/cmd/upgrade_checks/checks.py

@@ -28,6 +28,8 @@ from sqlalchemy import or_
 
 from neutron._i18n import _
 from neutron.cmd.upgrade_checks import base
+from neutron.common.ovn import exceptions as ovn_exc
+from neutron.common.ovn import utils as ovn_utils
 from neutron.conf.plugins.ml2 import config as ml2_conf
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 from neutron.conf import service as conf_service
@@ -231,6 +233,8 @@ class CoreChecks(base.BaseChecks):
              self.ovn_for_bm_provisioning_over_ipv6_check),
             (_('ML2/OVS IGMP Flood check'),
              self.ml2_ovs_igmp_flood_check),
+            (_('Floating IP Port forwarding and OVN L3 plugin configuration'),
+             self.ovn_port_forwarding_configuration_check),
         ]
 
     @staticmethod
@@ -669,3 +673,28 @@ class CoreChecks(base.BaseChecks):
         return upgradecheck.Result(
             upgradecheck.Code.SUCCESS,
             _('IGMP related traffic configuration is not affected.'))
+
+    @staticmethod
+    def ovn_port_forwarding_configuration_check(checker):
+        ovn_l3_plugin_names = [
+            'ovn-router',
+            'neutron.services.ovn_l3.plugin.OVNL3RouterPlugin']
+        if not any(plugin in ovn_l3_plugin_names
+                   for plugin in cfg.CONF.service_plugins):
+            return upgradecheck.Result(
+                upgradecheck.Code.SUCCESS, _('No OVN L3 plugin enabled.'))
+
+        ml2_conf.register_ml2_plugin_opts()
+        ovn_conf.register_opts()
+        try:
+            ovn_utils.validate_port_forwarding_configuration()
+            return upgradecheck.Result(
+                upgradecheck.Code.SUCCESS,
+                _('OVN L3 plugin and Port Forwarding configuration are fine.'))
+        except ovn_exc.InvalidPortForwardingConfiguration:
+            return upgradecheck.Result(
+                upgradecheck.Code.WARNING,
+                _('Neutron configuration is invalid. Port forwardings '
+                  'can not be used with ML2/OVN backend, distributed '
+                  'floating IPs and provider network type(s) used as '
+                  'tenant networks.'))

neutron/common/ovn/exceptions.py

@@ -37,3 +37,10 @@ class HashRingIsEmpty(n_exc.NeutronException):
                 '%(node_count)d nodes were found offline. This should never '
                 'happen in a normal situation, please check the status '
                 'of your cluster')
+
+
+class InvalidPortForwardingConfiguration(n_exc.NeutronException):
+    message = _('Neutron configuration is invalid. Port forwardings '
+                'can not be used with ML2/OVN backend, distributed '
+                'floating IPs and provider network type(s) used as '
+                'tenant networks.')

neutron/common/ovn/utils.py

@@ -1246,3 +1246,20 @@ def is_nat_gateway_port_supported(idl):
 def is_ovn_provider_router(router):
     flavor_id = router.get('flavor_id')
     return flavor_id is None or flavor_id is const.ATTR_NOT_SPECIFIED
+
+
+def validate_port_forwarding_configuration():
+    if not ovn_conf.is_ovn_distributed_floating_ip():
+        return
+
+    pf_plugin_names = [
+        'port_forwarding',
+        'neutron.services.portforwarding.pf_plugin.PortForwardingPlugin']
+    if not any(plugin in pf_plugin_names
+               for plugin in cfg.CONF.service_plugins):
+        return
+
+    provider_network_types = ['vlan', 'flat']
+    if any(net_type in provider_network_types
+           for net_type in cfg.CONF.ml2.tenant_network_types):
+        raise ovn_exc.InvalidPortForwardingConfiguration()

neutron/services/portforwarding/drivers/ovn/driver.py

@@ -10,19 +10,18 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from oslo_log import log
-
-from ovsdbapp.backend.ovs_idl import idlutils
-from ovsdbapp import constants as ovsdbapp_const
-
 from neutron_lib.callbacks import events
 from neutron_lib.callbacks import registry
 from neutron_lib.callbacks import resources
 from neutron_lib import constants as const
 from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
+from oslo_log import log
+from ovsdbapp.backend.ovs_idl import idlutils
+from ovsdbapp import constants as ovsdbapp_const
 
 from neutron.common.ovn import constants as ovn_const
+from neutron.common.ovn import exceptions as ovn_exc
 from neutron.common.ovn import utils as ovn_utils
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron import manager
@@ -192,10 +191,27 @@ class OVNPortForwardingHandler(object):
 class OVNPortForwarding(object):
 
     def __init__(self, l3_plugin):
+        self._validate_configuration()
         self._l3_plugin = l3_plugin
         self._pf_plugin_property = None
         self._handler = OVNPortForwardingHandler()
 
+    def _validate_configuration(self):
+        """This method checks if Neutron config is compatible with OVN and PFs.
+
+        It stops process in case when provider network types (vlan/flat)
+        are enabled as tenant networks AND distributed floating IPs are enabled
+        as this configuration is not working fine with FIP PFs in ML2/OVN case.
+        """
+        try:
+            ovn_utils.validate_port_forwarding_configuration()
+        except ovn_exc.InvalidPortForwardingConfiguration:
+            LOG.warning("Neutron configuration is invalid for port "
+                        "forwardings and ML2/OVN backend. "
+                        "It is not valid to use together provider network "
+                        "types (vlan/flat) as tenant networks, distributed "
+                        "floating IPs and port forwardings.")
+
     @property
     def _pf_plugin(self):
         if self._pf_plugin_property is None:

neutron/tests/unit/cmd/upgrade_checks/test_checks.py

@@ -18,6 +18,8 @@ from oslo_config import cfg
 from oslo_upgradecheck.upgradecheck import Code
 
 from neutron.cmd.upgrade_checks import checks
+from neutron.common.ovn import exceptions as ovn_exc
+from neutron.common.ovn import utils as ovn_utils
 from neutron.tests import base
 
 
@@ -350,3 +352,35 @@ class TestChecks(base.BaseTestCase):
             mock.ANY)
         self.assertEqual(Code.WARNING, result.code)
         mock_get_ovn_client.assert_called_once_with()
+
+    def test_ovn_port_forwarding_configuration_check_no_ovn_l3_router(self):
+        cfg.CONF.set_override("service_plugins", 'router,some-other-plugin')
+        with mock.patch.object(
+                ovn_utils,
+                'validate_port_forwarding_configuration') as validate_mock:
+            result = checks.CoreChecks.ovn_port_forwarding_configuration_check(
+                mock.ANY)
+            self.assertEqual(Code.SUCCESS, result.code)
+            validate_mock.assert_not_called()
+
+    def test_ovn_port_forwarding_configuration_check_ovn_l3_success(self):
+        cfg.CONF.set_override("service_plugins", 'ovn-router')
+        with mock.patch.object(
+                ovn_utils,
+                'validate_port_forwarding_configuration') as validate_mock:
+            result = checks.CoreChecks.ovn_port_forwarding_configuration_check(
+                mock.ANY)
+            self.assertEqual(Code.SUCCESS, result.code)
+            validate_mock.assert_called_once_with()
+
+    def test_ovn_port_forwarding_configuration_check_ovn_l3_failure(self):
+        cfg.CONF.set_override("service_plugins", 'ovn-router')
+        with mock.patch.object(
+                ovn_utils,
+                'validate_port_forwarding_configuration',
+                side_effect=ovn_exc.InvalidPortForwardingConfiguration
+        ) as validate_mock:
+            result = checks.CoreChecks.ovn_port_forwarding_configuration_check(
+                mock.ANY)
+            self.assertEqual(Code.WARNING, result.code)
+            validate_mock.assert_called_once_with()

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

@@ -29,6 +29,7 @@ from oslo_config import cfg
 import testtools
 
 from neutron.common.ovn import constants
+from neutron.common.ovn import exceptions as ovn_exc
 from neutron.common.ovn import utils
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 from neutron.tests import base
@@ -1217,3 +1218,44 @@ class DetermineBindHostTestCase(base.BaseTestCase):
             self.fake_smartnic_hostname,
             utils.determine_bind_host(self.mock_sb_idl, {},
                                       port_context=context))
+
+
+class ValidatePortForwardingConfigurationTestCase(base.BaseTestCase):
+
+    def setUp(self):
+        super().setUp()
+        ovn_conf.register_opts()
+
+    def test_validation_when_distributed_fip_disabled(self):
+        cfg.CONF.set_override(
+            'enable_distributed_floating_ip', False, group='ovn')
+        cfg.CONF.set_override('service_plugins', 'some_plugin,port_forwarding')
+        cfg.CONF.set_override('tenant_network_types', 'geneve,vlan',
+                              group='ml2')
+        utils.validate_port_forwarding_configuration()
+
+    def test_validation_when_no_pf_plugin_enabled(self):
+        cfg.CONF.set_override(
+            'enable_distributed_floating_ip', True, group='ovn')
+        cfg.CONF.set_override('service_plugins', 'some_plugin')
+        cfg.CONF.set_override('tenant_network_types', 'geneve,vlan',
+                              group='ml2')
+        utils.validate_port_forwarding_configuration()
+
+    def test_validation_when_no_provider_net_configured(self):
+        cfg.CONF.set_override(
+            'enable_distributed_floating_ip', True, group='ovn')
+        cfg.CONF.set_override('service_plugins', 'some_plugin,port_forwarding')
+        cfg.CONF.set_override('tenant_network_types', 'geneve,vxlan',
+                              group='ml2')
+        utils.validate_port_forwarding_configuration()
+
+    def test_validation_when_pf_and_provider_net_enabled(self):
+        cfg.CONF.set_override(
+            'enable_distributed_floating_ip', True, group='ovn')
+        cfg.CONF.set_override('service_plugins', 'some_plugin,port_forwarding')
+        cfg.CONF.set_override('tenant_network_types', 'geneve,vlan',
+                              group='ml2')
+        self.assertRaises(
+            ovn_exc.InvalidPortForwardingConfiguration,
+            utils.validate_port_forwarding_configuration)

neutron/tests/unit/services/portforwarding/drivers/ovn/test_driver.py

@@ -14,14 +14,6 @@
 
 from unittest import mock
 
-from neutron.common.ovn import constants as ovn_const
-from neutron.objects import port_forwarding as port_forwarding_obj
-from neutron.services.portforwarding.constants import PORT_FORWARDING
-from neutron.services.portforwarding.constants import PORT_FORWARDING_PLUGIN
-from neutron.services.portforwarding.drivers.ovn import driver \
-    as port_forwarding
-from neutron.tests import base
-from neutron.tests.unit import fake_resources
 from neutron_lib.callbacks import events
 from neutron_lib.callbacks import registry
 from neutron_lib.callbacks import resources
@@ -30,6 +22,18 @@ from neutron_lib.plugins import constants as plugin_constants
 from oslo_utils import uuidutils
 from ovsdbapp import constants as ovsdbapp_const
 
+from neutron.common.ovn import constants as ovn_const
+from neutron.common.ovn import exceptions as ovn_exc
+from neutron.common.ovn import utils as ovn_utils
+from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
+from neutron.objects import port_forwarding as port_forwarding_obj
+from neutron.services.portforwarding.constants import PORT_FORWARDING
+from neutron.services.portforwarding.constants import PORT_FORWARDING_PLUGIN
+from neutron.services.portforwarding.drivers.ovn import driver \
+    as port_forwarding
+from neutron.tests import base
+from neutron.tests.unit import fake_resources
+
 
 class TestOVNPortForwardingBase(base.BaseTestCase):
     def setUp(self):
@@ -450,6 +454,7 @@ class TestOVNPortForwardingHandler(TestOVNPortForwardingBase):
 class TestOVNPortForwarding(TestOVNPortForwardingBase):
     def setUp(self):
         super(TestOVNPortForwarding, self).setUp()
+        ovn_conf.register_opts()
         self.pf_plugin = mock.Mock()
         self.handler = mock.Mock()
         get_mock_pf_plugin = lambda alias: self.pf_plugin if (
@@ -475,6 +480,25 @@ class TestOVNPortForwarding(TestOVNPortForwardingBase):
         self.assertEqual(self._ovn_pf._handler, self.handler)
         self.assertEqual(self._ovn_pf._pf_plugin, self.pf_plugin)
 
+    def test__validate_configuration_ok(self):
+        with mock.patch.object(
+                port_forwarding.LOG, "warning") as mock_warning, \
+                mock.patch.object(ovn_utils,
+                                  "validate_port_forwarding_configuration"):
+
+            self._ovn_pf._validate_configuration()
+            mock_warning.assert_not_called()
+
+    def test__validate_configuration_wrong(self):
+        with mock.patch.object(
+                port_forwarding.LOG, "warning") as mock_warning, \
+                mock.patch.object(
+                    ovn_utils,
+                    "validate_port_forwarding_configuration",
+                    side_effect=ovn_exc.InvalidPortForwardingConfiguration):
+            self._ovn_pf._validate_configuration()
+            mock_warning.assert_called_once_with(mock.ANY)
+
     def test_register(self):
         with mock.patch.object(registry, 'subscribe') as mock_subscribe:
             self._ovn_pf.register(mock.ANY, mock.ANY, mock.Mock())

releasenotes/notes/OVN-L3-with-FIP-PF-require-centralized-FIPs-65864dfeb3edc9b1.yaml

@@ -0,0 +1,17 @@
+---
+other:
+  - |
+    When the following configuration is enabled at the same time:
+
+    * OVN L3 service plugin (``ovn-router``)
+    * Port forwarding service plugin (``port_forwarding``)
+    * "vlan" or "flat" network types configured in the ML2 configuration
+      variable ``tenant_network_types``
+    * The OVN floating IP traffic is distributed
+      (``enable_distributed_floating_ip`` = ``True``)
+
+    the Neutron server will report a warning during plugin initialization
+    because this is an invalid configuration matrix. Floating IPs need to
+    always be centralized in such a case.
+    For more details see `bug report
+    <https://bugs.launchpad.net/neutron/+bug/2028846>`_.