[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.

Conflicts:
    neutron/cmd/upgrade_checks/checks.py
    neutron/common/ovn/utils.py
    neutron/tests/unit/cmd/upgrade_checks/test_checks.py

Related-Bug: #2028846
Change-Id: I398f3f676c59dc794cf03320fa45efc7b22fc003
(cherry picked from commit ce53fb5)
(cherry picked from commit 1c6d3d448d419d6176647dc0e50760c19d3fefcb)

Author: slawqo

neutron/cmd/upgrade_checks/checks.py

@@ -27,6 +27,10 @@
 
 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
 from neutron.db.extra_dhcp_opt import models as extra_dhcp_opt_models
 from neutron.db.models import agent as agent_model
@@ -179,6 +183,8 @@ def get_checks(self):
              self.floatingip_inherit_qos_from_network),
             (_('Port extra DHCP options check'),
              self.extra_dhcp_options_check),
+            (_('Floating IP Port forwarding and OVN L3 plugin configuration'),
+             self.ovn_port_forwarding_configuration_check),
         ]
 
     @staticmethod
@@ -515,3 +521,28 @@ def extra_dhcp_options_check(checker):
             upgradecheck.Code.SUCCESS,
             _('There are no extra_dhcp_opts with the newline character '
               'in the option name or option value.'))
+
+    @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

@@ -1211,3 +1211,20 @@ def get_requested_chassis(requested_chassis):
 # becomes the norm and older versions of OVN are no longer supported
 def is_additional_chassis_supported(idl):
     return idl.is_col_present('Port_Binding', 'additional_chassis')
+
+
+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 @@ def port_forwarding_deleted(self, ovn_txn, nb_ovn, pf_obj):
 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_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
 
 
@@ -278,3 +280,25 @@ def test_extra_dhcp_options_check_bad_value(self):
                           opt_value='bar\nbar')]
             result = checks.CoreChecks.extra_dhcp_options_check(mock.ANY)
             self.assertEqual(Code.WARNING, result.code)
+
+    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 @@
 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
@@ -1172,3 +1173,44 @@ def test_vnic_remote_managed_port_context(self):
             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,21 +14,25 @@
 
 from unittest import mock
 
+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 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
-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 oslo_utils import uuidutils
-from ovsdbapp import constants as ovsdbapp_const
 
 
 class TestOVNPortForwardingBase(base.BaseTestCase):
@@ -450,6 +454,7 @@ def test_port_forwarding_deleted(self, m_info):
 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 @@ def test_init(self):
         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>`_.