From 3874a1ed9a3b7262ef41fe57fdd1237e61a40d44 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Fri, 11 Nov 2022 12:57:05 +0100 Subject: [PATCH] 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 2081910d6d942d49d96297884a932ff93acb8759) --- neutron/db/availability_zone/router.py | 3 +-- neutron/db/l3_attrs_db.py | 18 ++++++------- neutron/db/l3_db.py | 4 +++ neutron/db/l3_dvr_db.py | 7 ++--- neutron/db/l3_hamode_db.py | 6 ++--- neutron/objects/router.py | 12 +++++++++ .../ovn/mech_driver/ovsdb/maintenance.py | 27 +++++++++++++++++++ neutron/tests/unit/db/test_l3_db.py | 15 +++++++++++ neutron/tests/unit/extensions/test_l3.py | 22 ++++++++------- neutron/tests/unit/objects/test_router.py | 20 ++++++++++++++ 10 files changed, 106 insertions(+), 28 deletions(-) diff --git a/neutron/db/availability_zone/router.py b/neutron/db/availability_zone/router.py index 3e0650af76e..c346ce0cd29 100644 --- a/neutron/db/availability_zone/router.py +++ b/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) diff --git a/neutron/db/l3_attrs_db.py b/neutron/db/l3_attrs_db.py index 1e71f745bb6..9c0af8660c0 100644 --- a/neutron/db/l3_attrs_db.py +++ b/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 " diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 026aa440acf..70890986c4f 100644 --- a/neutron/db/l3_db.py +++ b/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) diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 928e0397acc..feed3d96c5f 100644 --- a/neutron/db/l3_dvr_db.py +++ b/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) diff --git a/neutron/db/l3_hamode_db.py b/neutron/db/l3_hamode_db.py index 79e90d505ff..1209fc1e1bb 100644 --- a/neutron/db/l3_hamode_db.py +++ b/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) diff --git a/neutron/objects/router.py b/neutron/objects/router.py index e9ae872a39c..4078e1aa251 100644 --- a/neutron/objects/router.py +++ b/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): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py index 611e4f0a130..88d14689a0d 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/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): diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 22003c38c13..e6fc1aaeed1 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/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'], diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index 0621c40a53f..07a5aea5142 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/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'] diff --git a/neutron/tests/unit/objects/test_router.py b/neutron/tests/unit/objects/test_router.py index b73d157d815..e20ae3f1749 100644 --- a/neutron/tests/unit/objects/test_router.py +++ b/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):