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

There are three reasons to revert this patch.

1. It broke RPC push API for trunks because it added port db model to
   event payload that is not serializeable.

2. It also broke the callback event payload interface, which requires
   that all entries in .states attribute belong to the same core object.

To quote from neutron-lib,

```
 # an iterable of states for the resource from the newest to the oldest
 # for example db states or api request/response
 # the actual object type for states will vary depending on event caller
 self.states = ...
```

3. There is no good justification why ml2/ovn would not allow this
   operation. The rationale for the original patch was to align the
   behavior with ml2/ovs, but we don't such parity requirements. The 409
   error that can be returned by the API endpoints is backend specific.

To quote api-ref,

```
409 The operation returns this error code for one of these reasons:
    A system configuration prevents the operation from succeeding.
```

AFAIU there is nothing that prevents ml2/ovn to create a trunk in this
situation.

This will have to be backported in all supported branches (the original
patch was backported down to Wallaby).

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

This reverts commit 833a6d8.

Closes-Bug: #2065707
Related-Bug: #2022059
Change-Id: I067c2f7286b2684b67b4389ca085d06a93f856ce
(cherry picked from commit ac15191)

Author: Ihar Hrachyshka

neutron/common/utils.py

@@ -37,12 +37,9 @@
 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.qos import constants as qos_consts
 from neutron_lib.services.trunk import constants as trunk_constants
 from neutron_lib.utils import helpers
@@ -1106,16 +1103,3 @@ def parse_permitted_ethertypes(permitted_ethertypes):
             continue
 
     return ret
-
-
-# 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,6 +17,7 @@
 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,6 +71,18 @@ 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.
@@ -1422,7 +1435,7 @@ def is_router_distributed(self, context, router_id):
 
     def get_ports_under_dvr_connected_subnet(self, context, subnet_id):
         ports = dvr_mac_db.get_ports_query_by_subnet_and_ip(context, subnet_id)
-        ports = [p for p in ports if n_utils.is_port_bound(p)]
+        ports = [p for p in ports if 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,12 +22,10 @@
 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 = (
@@ -157,10 +155,6 @@ 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_created(self, resource, event, trunk_plugin, payload):
         trunk = payload.states[0]
         # Check if parent port is handled by OVN.
@@ -176,18 +170,6 @@ def trunk_deleted(self, resource, event, trunk_plugin, payload):
         if trunk.sub_ports:
             self._unset_sub_ports(trunk.sub_ports)
 
-    def trunk_created_precommit(self, resource, event, trunk_plugin, payload):
-        # payload.desired_state below is the trunk object
-        parent_port = payload.desired_state.db_obj.port
-        if self._is_port_bound(parent_port):
-            raise trunk_exc.ParentPortInUse(port_id=parent_port.id)
-
-    def trunk_deleted_precommit(self, resource, event, trunk_plugin, payload):
-        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 subports_added(self, resource, event, trunk_plugin, payload):
         trunk = payload.states[0]
         subports = payload.metadata['subports']
@@ -226,14 +208,6 @@ def register(self, resource, event, trigger, payload=None):
             resource, event, trigger, payload=payload)
         self._handler = OVNTrunkHandler(self.plugin_driver)
 
-        registry.subscribe(
-            self._handler.trunk_created_precommit,
-            resources.TRUNK,
-            events.PRECOMMIT_CREATE)
-        registry.subscribe(
-            self._handler.trunk_deleted_precommit,
-            resources.TRUNK,
-            events.PRECOMMIT_DELETE)
         registry.subscribe(
             self._handler.trunk_created, resources.TRUNK, events.AFTER_CREATE)
         registry.subscribe(

neutron/services/trunk/plugin.py

@@ -294,7 +294,6 @@ 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
@@ -308,7 +307,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, parent_port))
+                                                states=(trunk,))
                 registry.publish(resources.TRUNK, events.PRECOMMIT_DELETE,
                                  self, payload=payload)
             else:
@@ -318,7 +317,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, parent_port)))
+                             states=(trunk,)))
 
     @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

@@ -14,16 +14,13 @@
 
 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.objects import registry as obj_reg
 from neutron_lib.plugins import utils
 from neutron_lib.services.trunk import constants as trunk_consts
 from oslo_utils import uuidutils
 
 from neutron.common.ovn import constants as ovn_const
-from neutron.objects import ports as port_obj
 from neutron.services.trunk import plugin as trunk_plugin
 from neutron.tests.functional import base
 
@@ -108,25 +105,6 @@ 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:
@@ -149,14 +127,3 @@ 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,7 +30,6 @@
 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
@@ -1511,7 +1510,7 @@ def test_is_router_distributed(self):
             self.assertTrue(
                 self.mixin.is_router_distributed(self.ctx, router_id))
 
-    @mock.patch.object(n_utils, 'is_port_bound')
+    @mock.patch.object(l3_dvr_db, "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/drivers/ovn/test_trunk_driver.py

@@ -454,10 +454,6 @@ def test_register(self):
         with mock.patch.object(registry, 'subscribe') as mock_subscribe:
             driver.register(mock.ANY, mock.ANY, mock.Mock())
             calls = [
-                mock.call.mock_subscribe(
-                    mock.ANY, resources.TRUNK, events.PRECOMMIT_CREATE),
-                mock.call.mock_subscribe(
-                    mock.ANY, resources.TRUNK, events.PRECOMMIT_DELETE),
                 mock.call.mock_subscribe(
                     mock.ANY, resources.TRUNK, events.AFTER_CREATE),
                 mock.call.mock_subscribe(

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

@@ -162,8 +162,7 @@ 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.states[0])
-            self.assertEqual(parent_port['port']['id'], payload.states[1].id)
+            self.assertEqual(trunk_obj, payload.latest_state)
             self.assertEqual(trunk['id'], payload.resource_id)
 
     def test_delete_trunk_notify_after_delete(self):