[OVN] Prevent Trunk creation/deletion with parent port bound

This patch imitates the ML2/OVS Trunk driver behaviour. When the
trunk parent port is bound:
* A new trunk cannot be created using this parent port.
* If the port is assigned as parent port of a trunk, this
  trunk cannot be deleted.

Conflicts:
    neutron/db/l3_dvr_db.py
    neutron/common/utils.py

Closes-Bug: #2022059
Change-Id: I8cfa7e67524a42224cbb4b3c3cec3cfa49b795fd
(cherry picked from commit 833a6d8)
(cherry picked from commit 2f48c24)
(cherry picked from commit 0d49980)

Author: ralonsoh

neutron/common/utils.py

@@ -35,9 +35,12 @@
 from eventlet.green import subprocess
 import netaddr
 from neutron_lib.api.definitions import availability_zone as az_def
+from neutron_lib.api.definitions import portbindings
+from neutron_lib.api.definitions import portbindings_extended
 from neutron_lib import constants as n_const
 from neutron_lib import context as n_context
 from neutron_lib.db import api as db_api
+from neutron_lib.plugins import utils as plugin_utils
 from neutron_lib.services.trunk import constants as trunk_constants
 from neutron_lib.utils import helpers
 from oslo_config import cfg
@@ -1046,3 +1049,16 @@ def is_session_active(session):
     if not (session.dirty or session.deleted or session.new):
         return False
     return True
+
+
+# TODO(slaweq): this should be moved to neutron_lib.plugins.utils module
+def is_port_bound(port, log_message=True):
+    active_binding = plugin_utils.get_port_binding_by_status_and_host(
+        port.get('port_bindings', []), n_const.ACTIVE)
+    if not active_binding:
+        if log_message:
+            LOG.warning('Binding for port %s was not found.', port)
+        return False
+    return active_binding[portbindings_extended.VIF_TYPE] not in (
+        portbindings.VIF_TYPE_UNBOUND,
+        portbindings.VIF_TYPE_BINDING_FAILED)

neutron/db/l3_dvr_db.py

@@ -17,7 +17,6 @@
 from neutron_lib.api.definitions import external_net as extnet_apidef
 from neutron_lib.api.definitions import l3 as l3_apidef
 from neutron_lib.api.definitions import portbindings
-from neutron_lib.api.definitions import portbindings_extended
 from neutron_lib.api.definitions import router_admin_state_down_before_update
 from neutron_lib.api import validators
 from neutron_lib.callbacks import events
@@ -70,18 +69,6 @@ def is_admin_state_down_necessary():
     return _IS_ADMIN_STATE_DOWN_NECESSARY
 
 
-# TODO(slaweq): this should be moved to neutron_lib.plugins.utils module
-def is_port_bound(port):
-    active_binding = plugin_utils.get_port_binding_by_status_and_host(
-        port.get("port_bindings", []), const.ACTIVE)
-    if not active_binding:
-        LOG.warning("Binding for port %s was not found.", port)
-        return False
-    return active_binding[portbindings_extended.VIF_TYPE] not in [
-        portbindings.VIF_TYPE_UNBOUND,
-        portbindings.VIF_TYPE_BINDING_FAILED]
-
-
 @registry.has_registry_receivers
 class DVRResourceOperationHandler(object):
     """Contains callbacks for DVR operations.
@@ -1426,7 +1413,7 @@ def is_router_distributed(self, context, router_id):
 
     def get_ports_under_dvr_connected_subnet(self, context, subnet_id):
         query = dvr_mac_db.get_ports_query_by_subnet_and_ip(context, subnet_id)
-        ports = [p for p in query.all() if is_port_bound(p)]
+        ports = [p for p in query.all() if n_utils.is_port_bound(p)]
         # TODO(slaweq): if there would be way to pass to neutron-lib only
         # list of extensions which actually should be processed, than setting
         # process_extensions=True below could avoid that second loop and

neutron/services/trunk/drivers/ovn/trunk_driver.py

@@ -22,10 +22,12 @@
 from oslo_log import log
 
 from neutron.common.ovn import constants as ovn_const
+from neutron.common import utils as n_utils
 from neutron.db import db_base_plugin_common
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron.objects import ports as port_obj
 from neutron.services.trunk.drivers import base as trunk_base
+from neutron.services.trunk import exceptions as trunk_exc
 
 
 SUPPORTED_INTERFACES = (
@@ -162,6 +164,10 @@ def _unset_binding_profile(self, context, subport, ovn_txn):
         LOG.debug("Done unsetting parent for subport %s", subport.port_id)
         return db_port
 
+    @staticmethod
+    def _is_port_bound(port):
+        return n_utils.is_port_bound(port, log_message=False)
+
     def trunk_updated(self, trunk):
         # Check if parent port is handled by OVN.
         if not self.plugin_driver.nb_ovn.lookup('Logical_Switch_Port',
@@ -208,6 +214,16 @@ def trunk_event(self, resource, event, trunk_plugin, payload):
             self.trunk_updated(payload.states[0])
         elif event == events.AFTER_DELETE:
             self.trunk_deleted(payload.states[0])
+        elif event == events.PRECOMMIT_CREATE:
+            trunk = payload.desired_state
+            parent_port = trunk.db_obj.port
+            if self._is_port_bound(parent_port):
+                raise trunk_exc.ParentPortInUse(port_id=parent_port.id)
+        elif event == events.PRECOMMIT_DELETE:
+            trunk = payload.states[0]
+            parent_port = payload.states[1]
+            if self._is_port_bound(parent_port):
+                raise trunk_exc.TrunkInUse(trunk_id=trunk.id)
 
     def subport_event(self, resource, event, trunk_plugin, payload):
         if event == events.AFTER_CREATE:
@@ -233,7 +249,8 @@ def register(self, resource, event, trigger, payload=None):
             resource, event, trigger, payload=payload)
         self._handler = OVNTrunkHandler(self.plugin_driver)
         for _event in (events.AFTER_CREATE, events.AFTER_UPDATE,
-                       events.AFTER_DELETE):
+                       events.AFTER_DELETE, events.PRECOMMIT_CREATE,
+                       events.PRECOMMIT_DELETE):
             registry.subscribe(self._handler.trunk_event,
                                resources.TRUNK,
                                _event)

neutron/services/trunk/plugin.py

@@ -294,6 +294,7 @@ def delete_trunk(self, context, trunk_id):
             trunk = self._get_trunk(context, trunk_id)
             rules.trunk_can_be_managed(context, trunk)
             trunk_port_validator = rules.TrunkPortValidator(trunk.port_id)
+            parent_port = trunk.db_obj.port
             if trunk_port_validator.can_be_trunked_or_untrunked(context):
                 # NOTE(status_police): when a trunk is deleted, the logical
                 # object disappears from the datastore, therefore there is no
@@ -307,7 +308,7 @@ def delete_trunk(self, context, trunk_id):
                                     'deleting trunk port %s: %s', trunk_id,
                                     str(e))
                 payload = events.DBEventPayload(context, resource_id=trunk_id,
-                                                states=(trunk,))
+                                                states=(trunk, parent_port))
                 registry.publish(resources.TRUNK, events.PRECOMMIT_DELETE,
                                  self, payload=payload)
             else:
@@ -317,7 +318,7 @@ def delete_trunk(self, context, trunk_id):
         registry.publish(resources.TRUNK, events.AFTER_DELETE, self,
                          payload=events.DBEventPayload(
                              context, resource_id=trunk_id,
-                             states=(trunk,)))
+                             states=(trunk, parent_port)))
 
     @db_base_plugin_common.convert_result_to_dict
     def add_subports(self, context, trunk_id, subports):

neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py

@@ -15,6 +15,7 @@
 import contextlib
 
 from neutron_lib.api.definitions import portbindings
+from neutron_lib.callbacks import exceptions as n_exc
 from neutron_lib import constants as n_consts
 from neutron_lib.db import api as db_api
 from neutron_lib.plugins import utils
@@ -113,6 +114,25 @@ def test_trunk_create_with_subports(self):
             with self.trunk([subport]) as trunk:
                 self._verify_trunk_info(trunk, has_items=True)
 
+    def test_trunk_create_parent_port_bound(self):
+        with self.network() as network:
+            with self.subnet(network=network) as subnet:
+                with self.port(subnet=subnet) as parent_port:
+                    pb = port_obj.PortBinding.get_objects(
+                        self.context, port_id=parent_port['port']['id'])
+                    port_obj.PortBinding.update_object(
+                        self.context, {'vif_type': portbindings.VIF_TYPE_OVS},
+                        port_id=pb[0].port_id, host=pb[0].host)
+                    tenant_id = uuidutils.generate_uuid()
+                    trunk = {'trunk': {
+                        'port_id': parent_port['port']['id'],
+                        'tenant_id': tenant_id, 'project_id': tenant_id,
+                        'admin_state_up': True,
+                        'name': 'trunk', 'sub_ports': []}}
+                    self.assertRaises(n_exc.CallbackFailure,
+                                      self.trunk_plugin.create_trunk,
+                                      self.context, trunk)
+
     def test_subport_add(self):
         with self.subport() as subport:
             with self.trunk() as trunk:
@@ -147,3 +167,14 @@ def test_trunk_delete(self):
         with self.trunk() as trunk:
             self.trunk_plugin.delete_trunk(self.context, trunk['id'])
             self._verify_trunk_info({}, has_items=False)
+
+    def test_trunk_delete_parent_port_bound(self):
+        with self.trunk() as trunk:
+            bp = port_obj.PortBinding.get_objects(
+                self.context, port_id=trunk['port_id'])
+            port_obj.PortBinding.update_object(
+                self.context, {'vif_type': portbindings.VIF_TYPE_OVS},
+                port_id=bp[0].port_id, host=bp[0].host)
+            self.assertRaises(n_exc.CallbackFailure,
+                              self.trunk_plugin.delete_trunk,
+                              self.context, trunk['id'])

neutron/tests/unit/db/test_l3_dvr_db.py

@@ -30,6 +30,7 @@
 from neutron_lib.plugins import utils as plugin_utils
 from oslo_utils import uuidutils
 
+from neutron.common import utils as n_utils
 from neutron.db import agents_db
 from neutron.db import l3_dvr_db
 from neutron.db import l3_dvrscheduler_db
@@ -1521,7 +1522,7 @@ def test_is_router_distributed(self):
             self.assertTrue(
                 self.mixin.is_router_distributed(self.ctx, router_id))
 
-    @mock.patch.object(l3_dvr_db, "is_port_bound")
+    @mock.patch.object(n_utils, 'is_port_bound')
     def test_get_ports_under_dvr_connected_subnet(self, is_port_bound_mock):
         router_dict = {'name': 'test_router', 'admin_state_up': True,
                        'distributed': True}

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

@@ -162,7 +162,8 @@ def _test_trunk_delete_notify(self, event):
                 resources.TRUNK, event, self.trunk_plugin, payload=mock.ANY)
             payload = callback.mock_calls[0][2]['payload']
             self.assertEqual(self.context, payload.context)
-            self.assertEqual(trunk_obj, payload.latest_state)
+            self.assertEqual(trunk_obj, payload.states[0])
+            self.assertEqual(parent_port['port']['id'], payload.states[1].id)
             self.assertEqual(trunk['id'], payload.resource_id)
 
     def test_delete_trunk_notify_after_delete(self):

releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Now the ML2/OVN trunk driver prevents a trunk creation if the parent port
+    is already bound. In the same way, if a parent port being used in a trunk
+    is bound, the trunk cannot be deleted.