Always create a "router_extra_attributes" register per router

The table "router_extra_attributes" is a child of "router" table.
Each register contains extra information that completes the router
description. When using ML2/OVS mechanism driver, the methods that
create and populate the "router_extra_attributes" register are always
called from the L3 DVR, L3 HA and availability zones extensions.

When using ML2/OVN, those extensions are not loaded and therefore the
"router_extra_attributes" register is not created.

Despite this register is currently not used in ML2/OVN (it will be in
future features), there are some project expecting the
"router_extra_attributes" register to be always created (for example,
neutron-dynamic-routing [1]).

This patch enforces the child register creating always when a router is
created. This register is populated with the default values. This new
register does not affect any current operation related to ML2/OVN nor
ML2/OVS.

There is a 1:1 relationship between "routers" and
"router_extra_attributes". The child register is deleted by the database
engine when the "routers" register is deleted (ondelete="CASCADE").

[1]https://review.opendev.org/c/openstack/neutron-dynamic-routing/+/863713

Conflicts:
    neutron/objects/router.py
    neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
    neutron/tests/unit/objects/test_router.py

Closes-Bug: #1995974
Change-Id: Ic546e40513402fa101c9687acce382cd6b84356c
(cherry picked from commit 2081910)

Author: ralonsoh

neutron/db/availability_zone/router.py

@@ -49,5 +49,4 @@ def _process_az_request(self, resource, event, trigger, payload):
         if az_hints:
             self.validate_availability_zones(context, 'router',
                                              az_hints)
-            self.set_extra_attr_value(context, router_db, az_def.AZ_HINTS,
-                                      az_hints)
+            self.set_extra_attr_value(router_db, az_def.AZ_HINTS, az_hints)

neutron/db/l3_attrs_db.py

@@ -45,20 +45,20 @@ def _extend_extra_router_dict(router_res, router_db):
             from_db = info.get('transform_from_db', lambda x: x)
             router_res[name] = from_db(extra_attrs.get(name, info['default']))
 
-    def _ensure_extra_attr_model(self, context, router_db):
-        if not router_db['extra_attributes']:
-            kwargs = {k: v['default'] for k, v in get_attr_info().items()}
-            kwargs['router_id'] = router_db['id']
-            new = l3_attrs.RouterExtraAttributes(**kwargs)
-            context.session.add(new)
-            router_db['extra_attributes'] = new
+    @staticmethod
+    def add_extra_attr(context, router_db):
+        kwargs = {k: v['default'] for k, v in get_attr_info().items()}
+        kwargs['router_id'] = router_db['id']
+        new = l3_attrs.RouterExtraAttributes(**kwargs)
+        context.session.add(new)
+        router_db['extra_attributes'] = new
 
-    def set_extra_attr_value(self, context, router_db, key, value):
+    @staticmethod
+    def set_extra_attr_value(router_db, key, value):
         # set a single value explicitly
         if key in get_attr_info():
             info = get_attr_info()[key]
             to_db = info.get('transform_to_db', lambda x: x)
-            self._ensure_extra_attr_model(context, router_db)
             router_db['extra_attributes'].update({key: to_db(value)})
             return
         raise RuntimeError(_("Tried to set a key '%s' that doesn't exist "

neutron/db/l3_db.py

@@ -47,6 +47,7 @@
 from neutron.common import ipv6_utils
 from neutron.common import utils
 from neutron.db import _utils as db_utils
+from neutron.db import l3_attrs_db
 from neutron.db.models import l3 as l3_models
 from neutron.db.models import l3_attrs as l3_attrs_models
 from neutron.db import models_v2
@@ -247,6 +248,7 @@ def _create_router_db(self, context, router, tenant_id):
                 status=constants.ACTIVE,
                 description=router.get('description'))
             context.session.add(router_db)
+            l3_attrs_db.ExtraAttributesMixin.add_extra_attr(context, router_db)
 
             registry.publish(resources.ROUTER, events.PRECOMMIT_CREATE, self,
                              payload=events.DBEventPayload(
@@ -282,6 +284,8 @@ def _get_stripped_router(self, router_body):
     def create_router(self, context, router):
         r = router['router']
         gw_info = r.get(EXTERNAL_GW_INFO, None)
+        # TODO(ralonsoh): migrate "tenant_id" to "project_id"
+        # https://blueprints.launchpad.net/neutron/+spec/keystone-v3
         create = functools.partial(self._create_router_db, context, r,
                                    r['tenant_id'])
         delete = functools.partial(self.delete_router, context)

neutron/db/l3_dvr_db.py

@@ -103,13 +103,11 @@ def l3plugin(self):
                        priority_group.PRIORITY_ROUTER_EXTENDED_ATTRIBUTE)
     def _set_distributed_flag(self, resource, event, trigger, payload):
         """Event handler to set distributed flag on creation."""
-        context = payload.context
         router = payload.latest_state
         router_db = payload.metadata['router_db']
         dist = is_distributed_router(router)
         router['distributed'] = dist
-        self.l3plugin.set_extra_attr_value(context, router_db, 'distributed',
-                                           dist)
+        self.l3plugin.set_extra_attr_value(router_db, 'distributed', dist)
 
     def _validate_router_migration(self, context, router_db, router_res,
                                    old_router=None):
@@ -203,8 +201,7 @@ def _handle_distributed_migration(self, resource, event,
                 payload.context, payload.resource_id,
                 agent['id'])
         self.l3plugin.set_extra_attr_value(
-            payload.context, payload.desired_state,
-            'distributed', migrating_to_distributed)
+            payload.desired_state, 'distributed', migrating_to_distributed)
 
     @registry.receives(resources.ROUTER, [events.AFTER_UPDATE],
                        priority_group.PRIORITY_ROUTER_EXTENDED_ATTRIBUTE)

neutron/db/l3_hamode_db.py

@@ -379,7 +379,7 @@ def _precommit_router_create(self, resource, event, trigger, payload):
         router_db = payload.metadata['router_db']
         is_ha = self._is_ha(router)
         router['ha'] = is_ha
-        self.set_extra_attr_value(context, router_db, 'ha', is_ha)
+        self.set_extra_attr_value(router_db, 'ha', is_ha)
         if not is_ha:
             return
         # This will throw an exception if there aren't enough agents to
@@ -453,14 +453,14 @@ def _validate_migration(self, resource, event, trigger, payload=None):
             payload.desired_state.extra_attributes.ha_vr_id = None
         if (payload.request_body.get('distributed') or
                 payload.states[0]['distributed']):
-            self.set_extra_attr_value(payload.context, payload.desired_state,
+            self.set_extra_attr_value(payload.desired_state,
                                       'ha', requested_ha_state)
             return
         self._migrate_router_ports(
              payload.context, payload.desired_state,
              old_owner=old_owner, new_owner=new_owner)
         self.set_extra_attr_value(
-            payload.context, payload.desired_state, 'ha', requested_ha_state)
+            payload.desired_state, 'ha', requested_ha_state)
 
     @registry.receives(resources.ROUTER, [events.AFTER_UPDATE],
                        priority_group.PRIORITY_ROUTER_EXTENDED_ATTRIBUTE)

neutron/objects/router.py

@@ -17,11 +17,13 @@
 from neutron_lib.api.definitions import availability_zone as az_def
 from neutron_lib.api.validators import availability_zone as az_validator
 from neutron_lib import constants as n_const
+from neutron_lib.db import api as db_api
 from neutron_lib.objects import common_types
 from neutron_lib.utils import net as net_utils
 from oslo_utils import versionutils
 from oslo_versionedobjects import fields as obj_fields
 from sqlalchemy import func
+from sqlalchemy import sql
 
 from neutron.db.models import dvr as dvr_models
 from neutron.db.models import l3
@@ -230,6 +232,16 @@ def check_routers_not_owned_by_projects(cls, context, gw_ports, projects):
 
         return bool(query.count())
 
+    @staticmethod
+    @db_api.CONTEXT_READER
+    def get_router_ids_without_router_std_attrs(context):
+        r_attrs = l3_attrs.RouterExtraAttributes
+        query = context.session.query(l3.Router)
+        query = query.join(r_attrs, r_attrs.router_id == l3.Router.id,
+                           isouter=True)
+        query = query.filter(r_attrs.router_id == sql.null())
+        return [r.id for r in query.all()]
+
 
 @base.NeutronObjectRegistry.register
 class FloatingIP(base.NeutronDbObject):

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

@@ -25,6 +25,7 @@
 from neutron_lib.api.definitions import segment as segment_def
 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 import exceptions as n_exc
 from oslo_config import cfg
 from oslo_log import log
@@ -34,9 +35,11 @@
 from neutron.common.ovn import constants as ovn_const
 from neutron.common.ovn import utils
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
+from neutron.db import l3_attrs_db
 from neutron.db import ovn_hash_ring_db as hash_ring_db
 from neutron.db import ovn_revision_numbers_db as revision_numbers_db
 from neutron.db import segments_db
+from neutron.objects import router as router_obj
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_db_sync
 
 
@@ -840,6 +843,30 @@ def update_port_virtual_type(self):
                     txn.add(cmd)
         raise periodics.NeverAgain()
 
+    # TODO(ralonsoh): Remove this in the Antelope+4 cycle
+    @periodics.periodic(spacing=600, run_immediately=True)
+    def create_router_extra_attributes_registers(self):
+        """Create missing ``RouterExtraAttributes`` registers.
+
+        ML2/OVN L3 plugin does not inherit the ``ExtraAttributesMixin`` class.
+        Before LP#1995974, the L3 plugin was not creating a
+        ``RouterExtraAttributes`` register per ``Routers`` register. This one
+        only execution method finds those ``Routers`` registers without the
+        child one and creates one with the default values.
+        """
+        if not self.has_lock:
+            return
+
+        context = n_context.get_admin_context()
+        for router_id in router_obj.Router.\
+                get_router_ids_without_router_std_attrs(context):
+            with db_api.CONTEXT_WRITER.using(context):
+                router_db = {'id': router_id}
+                l3_attrs_db.ExtraAttributesMixin.add_extra_attr(context,
+                                                                router_db)
+
+        raise periodics.NeverAgain()
+
 
 class HashRingHealthCheckPeriodics(object):
 

neutron/tests/unit/db/test_l3_db.py

@@ -37,6 +37,7 @@
 from neutron.db import extraroute_db
 from neutron.db import l3_db
 from neutron.db.models import l3 as l3_models
+from neutron.db.models import l3_attrs
 from neutron.db import models_v2
 from neutron.extensions import segment as segment_ext
 from neutron.objects import base as base_obj
@@ -732,6 +733,20 @@ def test_create_router_notify(self):
             ]
             mock_publish.assert_has_calls(expected_calls)
 
+    def test_create_router_extra_attr(self):
+        router_args = {'router': {'name': 'foo_router',
+                                  'admin_state_up': True,
+                                  'tenant_id': 'foo_tenant'}
+                       }
+        router_dict = self.create_router(router_args)
+        with db_api.CONTEXT_READER.using(self.ctx) as session:
+            r_extra_attrs = session.query(
+                l3_attrs.RouterExtraAttributes).filter(
+                    l3_attrs.RouterExtraAttributes.router_id ==
+                    router_dict['id']).all()
+        self.assertEqual(1, len(r_extra_attrs))
+        self.assertEqual(router_dict['id'], r_extra_attrs[0].router_id)
+
     def test_update_router_notify(self):
         with mock.patch.object(l3_db.registry, 'publish') as mock_publish:
             self.mixin.update_router(self.ctx, self.router['id'],

neutron/tests/unit/extensions/test_l3.py

@@ -614,9 +614,15 @@ def setUp(self):
         self.mixin = l3_attrs_db.ExtraAttributesMixin()
         directory.add_plugin(plugin_constants.L3, self.mixin)
         self.ctx = context.get_admin_context()
-        self.router = l3_models.Router()
-        with db_api.CONTEXT_WRITER.using(self.ctx):
-            self.ctx.session.add(self.router)
+        self.router = self._new_router(self.ctx)
+
+    @staticmethod
+    @db_api.CONTEXT_WRITER
+    def _new_router(ctx):
+        router = l3_models.Router()
+        ctx.session.add(router)
+        l3_attrs_db.ExtraAttributesMixin.add_extra_attr(ctx, router)
+        return router
 
     def _get_default_api_values(self):
         return {k: v.get('transform_from_db', lambda x: x)(v['default'])
@@ -625,8 +631,7 @@ def _get_default_api_values(self):
     def test_set_extra_attr_key_bad(self):
         with testtools.ExpectedException(RuntimeError):
             with db_api.CONTEXT_WRITER.using(self.ctx):
-                self.mixin.set_extra_attr_value(self.ctx, self.router,
-                                                'bad', 'value')
+                self.mixin.set_extra_attr_value(self.router, 'bad', 'value')
 
     def test__extend_extra_router_dict_defaults(self):
         rdict = {}
@@ -635,9 +640,8 @@ def test__extend_extra_router_dict_defaults(self):
 
     def test_set_attrs_and_extend(self):
         with db_api.CONTEXT_WRITER.using(self.ctx):
-            self.mixin.set_extra_attr_value(self.ctx, self.router,
-                                            'ha_vr_id', 99)
-            self.mixin.set_extra_attr_value(self.ctx, self.router,
+            self.mixin.set_extra_attr_value(self.router, 'ha_vr_id', 99)
+            self.mixin.set_extra_attr_value(self.router,
                                             'availability_zone_hints',
                                             ['x', 'y', 'z'])
             expected = self._get_default_api_values()
@@ -647,7 +651,7 @@ def test_set_attrs_and_extend(self):
             self.mixin._extend_extra_router_dict(rdict, self.router)
             self.assertEqual(expected, rdict)
 
-            self.mixin.set_extra_attr_value(self.ctx, self.router,
+            self.mixin.set_extra_attr_value(self.router,
                                             'availability_zone_hints',
                                             ['z', 'y', 'z'])
             expected['availability_zone_hints'] = ['z', 'y', 'z']

neutron/tests/unit/objects/test_router.py

@@ -14,8 +14,10 @@
 
 from unittest import mock
 
+from neutron_lib.db import api as db_api
 from oslo_utils import uuidutils
 
+from neutron.db import l3_attrs_db
 from neutron.objects.qos import binding as qos_binding
 from neutron.objects.qos import policy
 from neutron.objects import router
@@ -127,6 +129,24 @@ def test_check_routers_not_owned_by_projects(self):
             [project, new_project])
         self.assertFalse(router_exist)
 
+    def test_get_router_ids_without_router_std_attrs(self):
+        def create_r_attr_reg(idx):
+            with db_api.CONTEXT_WRITER.using(self.context):
+                router_db = {'id': self.objs[idx].id}
+                l3_attrs_db.ExtraAttributesMixin.add_extra_attr(self.context,
+                                                                router_db)
+
+        for idx in range(3):
+            self.objs[idx].create()
+        expected_router_ids = [r.id for r in self.objs]
+
+        for idx in range(3):
+            router_ids = router.Router.\
+                get_router_ids_without_router_std_attrs(self.context)
+            self.assertEqual(expected_router_ids, router_ids)
+            create_r_attr_reg(idx)
+            expected_router_ids = expected_router_ids[1:]
+
 
 class RouterPortIfaceObjectTestCase(obj_test_base.BaseObjectIfaceTestCase):