openstack
/
neutron
Code Issues Proposed changes

Add a unique key to port_id in routerports table 

If multiple commands to add router interfaces to different routers
by the same port are executed concurrently, then all the commands
would show success.

However, there are three issues:
1. Only one router interface is actually added by the port
2. Multiple router ports records are stored in routerports table
3. The port table is updated multiple times and eventually the
last-arrived command would truly take effect

This patch adds a unique key to port_id in routerport table,
so that only the first-arrived command will insert router port
record and all later requests would raise exceptions.

Besides, port.device_id and port.device_owner in port table
needs to be updated again after routerport record is inserted.
Otherwise, in race condition the port table will store the router
information from last-arrived request. However, in routerport table,
only the first-arrived request's router information is inserted.

Change-Id: I15be35689ec59ac02ed34abe5862fa4580c8587c
Closes-Bug: #1535551
This commit is contained in:
Lujin Luo 2016-06-21 14:23:33 +09:00 committed by Lujin
parent ee42af1011
commit 6281fddbcb
6 changed files with 156 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
neutron/db/l3_db.py
View File

@ -19,6 +19,7 @@ import netaddr
from neutron_lib.api import validators
from neutron_lib import constants as l3_constants
from neutron_lib import exceptions as n_exc
from oslo_db import exception as db_exc
from oslo_log import log as logging
from oslo_utils import excutils
from oslo_utils import uuidutils
@ -73,7 +74,8 @@ class RouterPort(model_base.BASEV2):
port_id = sa.Column(
sa.String(36),
sa.ForeignKey('ports.id', ondelete="CASCADE"),
primary_key=True)
primary_key=True,
unique=True)
revises_on_change = ('router', )
# The port_type attribute is redundant as the port table already specifies
# it in DEVICE_OWNER.However, this redundancy enables more efficient
@ -775,6 +777,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
with p_utils.delete_port_on_error(self._core_plugin,
context, port['id']) as delmgr:
delmgr.delete_on_error = cleanup_port
try:
with context.session.begin(subtransactions=True):
router_port = RouterPort(
port_id=port['id'],
@ -782,6 +785,21 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
port_type=device_owner
)
context.session.add(router_port)
except db_exc.DBDuplicateEntry:
qry = context.session.query(RouterPort).filter_by(
port_id=port['id']).one()
existing_router_id = qry['router_id']
raise n_exc.PortInUse(net_id=port['network_id'],
port_id=port['id'],
device_id=existing_router_id)
# Update owner after actual process again in order to
# make sure the records in routerports table and ports
# table are consistent.
else:
self._core_plugin.update_port(
context, port['id'], {'port': {
'device_id': router.id,
'device_owner': device_owner}})
gw_ips = []
gw_network_id = None

17
neutron/db/l3_dvr_db.py
View File

@ -17,6 +17,7 @@ from neutron_lib.api import validators
from neutron_lib import constants as const
from neutron_lib import exceptions as n_exc
from oslo_config import cfg
from oslo_db import exception as db_exc
from oslo_log import helpers as log_helper
from oslo_log import log as logging
from oslo_utils import excutils
@ -329,6 +330,7 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
port['fixed_ips'][-1]['subnet_id'])
delmgr.delete_on_error = cleanup_port
try:
with context.session.begin(subtransactions=True):
router_port = l3_db.RouterPort(
port_id=port['id'],
@ -336,6 +338,21 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
port_type=device_owner
)
context.session.add(router_port)
except db_exc.DBDuplicateEntry:
qry = context.session.query(l3_db.RouterPort).filter_by(
port_id=port['id']).one()
existing_router_id = qry['router_id']
raise n_exc.PortInUse(net_id=port['network_id'],
port_id=port['id'],
device_id=existing_router_id)
# Update owner after actual process again in order to
# make sure the records in routerports table and ports
# table are consistent.
else:
self._core_plugin.update_port(
context, port['id'], {'port': {
'device_id': router.id,
'device_owner': device_owner}})
# NOTE: For IPv6 additional subnets added to the same
# network we need to update the CSNAT port with respective

2
neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD
View File

@ -1 +1 @@
3d0e74aa7d37
030a959ceafa

69
neutron/db/migration/alembic_migrations/versions/newton/expand/030a959ceafa_uniq_routerports0port_id.py Normal file
View File

@ -0,0 +1,69 @@
# Copyright 2016 OpenStack Foundation
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
#
"""uniq_routerports0port_id
Revision ID: 030a959ceafa
Revises: 3d0e74aa7d37
Create Date: 2016-06-21 11:33:13.043879
"""
# revision identifiers, used by Alembic.
revision = '030a959ceafa'
down_revision = '3d0e74aa7d37'
from alembic import op
import sqlalchemy as sa
from neutron._i18n import _
from neutron.common import exceptions
routerports = sa.Table(
'routerports', sa.MetaData(),
sa.Column('router_id', sa.String(36)),
sa.Column('port_id', sa.String(36)),
sa.Column('port_type', sa.String(255)))
class DuplicatePortRecordinRouterPortdatabase(exceptions.Conflict):
message = _("Duplicate port(s) %(port_id)s records exist in routerports "
"database. Database cannot be upgraded. Please remove all "
"duplicated records before upgrading the database.")
def upgrade():
op.create_unique_constraint(
'uniq_routerports0port_id',
'routerports',
['port_id'])
def check_sanity(connection):
res = get_duplicate_port_records_in_routerport_database(connection)
if res:
raise DuplicatePortRecordinRouterPortdatabase(port_id=",".join(res))
def get_duplicate_port_records_in_routerport_database(connection):
insp = sa.engine.reflection.Inspector.from_engine(connection)
if 'routerports' not in insp.get_table_names():
return []
session = sa.orm.Session(bind=connection.connect())
query = (session.query(routerports.c.port_id)
.group_by(routerports.c.port_id)
.having(sa.func.count() > 1)).all()
return [q[0] for q in query]

21
neutron/tests/functional/db/test_migrations.py
View File

@ -378,6 +378,27 @@ class TestSanityCheck(testlib_api.SqlTestCaseLight):
self.assertRaises(script.DuplicateL3HARouterAgentPortBinding,
script.check_sanity, conn)
def test_check_sanity_030a959ceafa(self):
routerports = sqlalchemy.Table(
'routerports', sqlalchemy.MetaData(),
sqlalchemy.Column('router_id', sqlalchemy.String(36)),
sqlalchemy.Column('port_id', sqlalchemy.String(36)),
sqlalchemy.Column('port_type', sqlalchemy.String(255)))
with self.engine.connect() as conn:
routerports.create(conn)
conn.execute(routerports.insert(), [
{'router_id': '1234', 'port_id': '12345',
'port_type': '123'},
{'router_id': '12343', 'port_id': '12345',
'port_type': '1232'}
])
script_dir = alembic_script.ScriptDirectory.from_config(
self.alembic_config)
script = script_dir.get_revision("030a959ceafa").module
self.assertRaises(script.DuplicatePortRecordinRouterPortdatabase,
script.check_sanity, conn)
class TestWalkDowngrade(oslotest_base.BaseTestCase):

29
neutron/tests/unit/extensions/test_l3.py
View File

@ -1241,6 +1241,35 @@ class L3NatTestCaseBase(L3NatTestCaseMixin):
None,
p['port']['id'])
def test_router_add_interface_dup_port(self):
'''This tests that if multiple routers add one port as their
interfaces. Only the first router's interface would be added
to this port. All the later requests would return exceptions.
'''
with self.router() as r1, self.router() as r2, self.network() as n:
with self.subnet(network=n) as s:
with self.port(subnet=s) as p:
self._router_interface_action('add',
r1['router']['id'],
None,
p['port']['id'])
# mock out the sequential check
plugin = 'neutron.db.l3_db.L3_NAT_dbonly_mixin'
with mock.patch(plugin + '._check_router_port',
port_id=p['port']['id'],
device_id=r2['router']['id']) as checkport:
checkport.return_value = p['port']
self._router_interface_action('add',
r2['router']['id'],
None,
p['port']['id'],
exc.HTTPConflict.code)
# clean-up
self._router_interface_action('remove',
r1['router']['id'],
None,
p['port']['id'])
def _assert_body_port_id_and_update_port(self, body, mock_update_port,
port_id, device_id):
self.assertNotIn('port_id', body)