Merge "Fix dynamic segment allocation race condition"
This commit is contained in:
commit
6a534dbe2b
|
@ -20,12 +20,14 @@ from oslo_config import cfg
|
|||
from oslo_serialization import jsonutils
|
||||
from oslo_upgradecheck import common_checks
|
||||
from oslo_upgradecheck import upgradecheck
|
||||
from sqlalchemy import func
|
||||
from sqlalchemy import or_
|
||||
|
||||
from neutron._i18n import _
|
||||
from neutron.cmd.upgrade_checks import base
|
||||
from neutron.db.models import agent as agent_model
|
||||
from neutron.db.models.plugins.ml2 import vlanallocation
|
||||
from neutron.db.models import segment
|
||||
from neutron.db import models_v2
|
||||
|
||||
|
||||
|
@ -90,6 +92,20 @@ def port_mac_addresses():
|
|||
ctx.session.query(models_v2.Port.mac_address).all()]
|
||||
|
||||
|
||||
def get_duplicate_network_segment_count():
|
||||
ctx = context.get_admin_context()
|
||||
query = ctx.session.query(segment.NetworkSegment.network_id)
|
||||
# for a unique constraint it's always NULL != NULL --> we filter them out
|
||||
query = query.filter(segment.NetworkSegment.physical_network.isnot(None))
|
||||
query = query.group_by(
|
||||
segment.NetworkSegment.network_id,
|
||||
segment.NetworkSegment.network_type,
|
||||
segment.NetworkSegment.physical_network
|
||||
)
|
||||
query = query.having(func.count() > 1)
|
||||
return query.count()
|
||||
|
||||
|
||||
class CoreChecks(base.BaseChecks):
|
||||
|
||||
def get_checks(self):
|
||||
|
@ -109,6 +125,8 @@ class CoreChecks(base.BaseChecks):
|
|||
(common_checks.check_policy_json, {'conf': cfg.CONF})),
|
||||
(_('Port MAC address sanity check'),
|
||||
self.port_mac_address_sanity),
|
||||
(_('NetworkSegments unique constraint check'),
|
||||
self.networksegments_unique_constraint_check),
|
||||
]
|
||||
|
||||
@staticmethod
|
||||
|
@ -319,3 +337,31 @@ class CoreChecks(base.BaseChecks):
|
|||
upgradecheck.Code.SUCCESS,
|
||||
_("All port MAC addresses are correctly formated in the "
|
||||
"database."))
|
||||
|
||||
@staticmethod
|
||||
def networksegments_unique_constraint_check(checker):
|
||||
"""Checks that there are no duplicate networksegments
|
||||
|
||||
No two networksegments should never share the same network_id,
|
||||
network_type and physical_network. Two NULL values are not regarded
|
||||
as equal for a unique constraint, so networksegments with NULL as
|
||||
physical_network are ignored by this check.
|
||||
"""
|
||||
if not cfg.CONF.database.connection:
|
||||
return upgradecheck.Result(
|
||||
upgradecheck.Code.WARNING,
|
||||
_("Database connection string is not set. Check for port MAC "
|
||||
"sanity can't be done."))
|
||||
|
||||
count = get_duplicate_network_segment_count()
|
||||
if count:
|
||||
return upgradecheck.Result(
|
||||
upgradecheck.Code.WARNING,
|
||||
_("There are %d instances of networksegments sharing the same "
|
||||
"combination of network_id, network_type and "
|
||||
"physical_network.") % count)
|
||||
|
||||
return upgradecheck.Result(
|
||||
upgradecheck.Code.SUCCESS,
|
||||
_("No networksegments sharing the same network_id, network_type "
|
||||
"and physical_network found."))
|
||||
|
|
|
@ -1 +1 @@
|
|||
ba859d649675
|
||||
e981acd076d3
|
||||
|
|
|
@ -0,0 +1,37 @@
|
|||
# Copyright 2021 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.
|
||||
#
|
||||
|
||||
from alembic import op
|
||||
|
||||
|
||||
"""Add NetworkSegments database constraint
|
||||
|
||||
Revision ID: e981acd076d3
|
||||
Revises: ba859d649675
|
||||
Create Date: 2021-04-23 16:01:01.320910
|
||||
|
||||
"""
|
||||
|
||||
# revision identifiers, used by Alembic.
|
||||
revision = 'e981acd076d3'
|
||||
down_revision = 'ba859d649675'
|
||||
|
||||
|
||||
def upgrade():
|
||||
op.create_unique_constraint(
|
||||
'uniq_networksegment0network_id0network_type0physical_network',
|
||||
'networksegments',
|
||||
['network_id', 'network_type', 'physical_network']
|
||||
)
|
|
@ -53,6 +53,16 @@ class NetworkSegment(standard_attr.HasStandardAttributes,
|
|||
cascade='delete'))
|
||||
api_collections = [segment.SEGMENTS]
|
||||
|
||||
__table_args__ = (
|
||||
sa.UniqueConstraint(
|
||||
network_id,
|
||||
network_type,
|
||||
physical_network,
|
||||
name='uniq_networksegment0network_id0'
|
||||
'network_type0physical_network'),
|
||||
model_base.BASEV2.__table_args__
|
||||
)
|
||||
|
||||
|
||||
class SegmentHostMapping(model_base.BASEV2):
|
||||
|
||||
|
|
|
@ -340,6 +340,7 @@ class TypeManager(stevedore.named.NamedExtensionManager):
|
|||
LOG.error("Failed to release segment '%s' because "
|
||||
"network type is not supported.", segment)
|
||||
|
||||
@db_api.retry_if_session_inactive()
|
||||
def allocate_dynamic_segment(self, context, network_id, segment):
|
||||
"""Allocate a dynamic segment using a partial or full segment dict."""
|
||||
dynamic_segment = segments_db.get_dynamic_segment(
|
||||
|
@ -349,6 +350,7 @@ class TypeManager(stevedore.named.NamedExtensionManager):
|
|||
if dynamic_segment:
|
||||
return dynamic_segment
|
||||
|
||||
with db_api.CONTEXT_WRITER.using(context):
|
||||
driver = self.drivers.get(segment.get(api.NETWORK_TYPE))
|
||||
if isinstance(driver.obj, api.TypeDriver):
|
||||
dynamic_segment = driver.obj.reserve_provider_segment(
|
||||
|
@ -356,14 +358,17 @@ class TypeManager(stevedore.named.NamedExtensionManager):
|
|||
else:
|
||||
dynamic_segment = driver.obj.reserve_provider_segment(
|
||||
context, segment)
|
||||
segments_db.add_network_segment(context, network_id, dynamic_segment,
|
||||
segments_db.add_network_segment(context, network_id,
|
||||
dynamic_segment,
|
||||
is_dynamic=True)
|
||||
return dynamic_segment
|
||||
|
||||
@db_api.retry_if_session_inactive()
|
||||
def release_dynamic_segment(self, context, segment_id):
|
||||
"""Delete a dynamic segment."""
|
||||
segment = segments_db.get_segment_by_id(context, segment_id)
|
||||
if segment:
|
||||
with db_api.CONTEXT_WRITER.using(context):
|
||||
driver = self.drivers.get(segment.get(api.NETWORK_TYPE))
|
||||
if driver:
|
||||
if isinstance(driver.obj, api.TypeDriver):
|
||||
|
|
|
@ -201,3 +201,13 @@ class TestChecks(base.BaseTestCase):
|
|||
mock_port_mac_addresses.return_value = mac_addresses
|
||||
result = checks.CoreChecks.port_mac_address_sanity(mock.ANY)
|
||||
self.assertEqual(returned_code, result.code)
|
||||
|
||||
def test_networksegments_unique_constraint_check(self):
|
||||
cases = ([0, Code.SUCCESS], [1, Code.WARNING])
|
||||
with mock.patch.object(
|
||||
checks, 'get_duplicate_network_segment_count') as mock_count:
|
||||
for count, returned_code in cases:
|
||||
mock_count.return_value = count
|
||||
result = checks.CoreChecks.\
|
||||
networksegments_unique_constraint_check(mock.ANY)
|
||||
self.assertEqual(returned_code, result.code)
|
||||
|
|
|
@ -636,7 +636,7 @@ class TestSegmentSubnetAssociation(SegmentTestCase):
|
|||
subnet = subnet['subnet']
|
||||
|
||||
segment2 = self._test_create_segment(network_id=net['id'],
|
||||
physical_network='phys_net2',
|
||||
physical_network='phys_net3',
|
||||
segmentation_id=202)['segment']
|
||||
|
||||
data = {'subnet': {'segment_id': segment2['id']}}
|
||||
|
|
|
@ -113,7 +113,7 @@ class Ml2DBTestCase(testlib_api.SqlTestCase):
|
|||
api.PHYSICAL_NETWORK: 'physnet1',
|
||||
api.SEGMENTATION_ID: 1},
|
||||
{api.NETWORK_TYPE: 'vlan',
|
||||
api.PHYSICAL_NETWORK: 'physnet1',
|
||||
api.PHYSICAL_NETWORK: 'physnet2',
|
||||
api.SEGMENTATION_ID: 2}]
|
||||
self._create_segments(segments)
|
||||
|
||||
|
@ -124,13 +124,13 @@ class Ml2DBTestCase(testlib_api.SqlTestCase):
|
|||
api.PHYSICAL_NETWORK: 'physnet1',
|
||||
api.SEGMENTATION_ID: 1},
|
||||
{api.NETWORK_TYPE: 'vlan',
|
||||
api.PHYSICAL_NETWORK: 'physnet1',
|
||||
api.PHYSICAL_NETWORK: 'physnet2',
|
||||
api.SEGMENTATION_ID: 2}]
|
||||
segments2 = [{api.NETWORK_TYPE: 'vlan',
|
||||
api.PHYSICAL_NETWORK: 'physnet1',
|
||||
api.SEGMENTATION_ID: 3},
|
||||
{api.NETWORK_TYPE: 'vlan',
|
||||
api.PHYSICAL_NETWORK: 'physnet1',
|
||||
api.PHYSICAL_NETWORK: 'physnet2',
|
||||
api.SEGMENTATION_ID: 4}]
|
||||
net1segs = self._create_segments(segments1, network_id=net_id1)
|
||||
net2segs = self._create_segments(segments2, network_id=net_id2)
|
||||
|
|
|
@ -150,13 +150,17 @@ class Ml2PluginV2TestCase(test_plugin.NeutronDbPluginV2TestCase):
|
|||
self._mechanism_drivers,
|
||||
group='ml2')
|
||||
self.physnet = 'physnet1'
|
||||
self.physnet2 = 'physnet2'
|
||||
self.physnet3 = 'physnet3'
|
||||
self.vlan_range = '1:100'
|
||||
self.vlan_range2 = '200:300'
|
||||
self.physnet2 = 'physnet2'
|
||||
self.vlan_range3 = '400:500'
|
||||
self.phys_vrange = ':'.join([self.physnet, self.vlan_range])
|
||||
self.phys2_vrange = ':'.join([self.physnet2, self.vlan_range2])
|
||||
self.phys3_vrange = ':'.join([self.physnet3, self.vlan_range3])
|
||||
cfg.CONF.set_override('network_vlan_ranges',
|
||||
[self.phys_vrange, self.phys2_vrange],
|
||||
[self.phys_vrange, self.phys2_vrange,
|
||||
self.phys3_vrange],
|
||||
group='ml2_type_vlan')
|
||||
self.setup_parent()
|
||||
self.driver = directory.get_plugin()
|
||||
|
@ -223,7 +227,7 @@ class TestMl2NetworksV2(test_plugin.TestNetworksV2,
|
|||
self.mp_nets = [{'name': 'net4',
|
||||
mpnet_apidef.SEGMENTS:
|
||||
[{pnet.NETWORK_TYPE: 'vlan',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet2',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet3',
|
||||
pnet.SEGMENTATION_ID: 1},
|
||||
{pnet.NETWORK_TYPE: 'vlan',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet2',
|
||||
|
@ -481,7 +485,7 @@ class TestMl2NetworksV2(test_plugin.TestNetworksV2,
|
|||
pnet.PHYSICAL_NETWORK: 'physnet1',
|
||||
pnet.SEGMENTATION_ID: 1},
|
||||
{pnet.NETWORK_TYPE: 'vlan',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet1',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet2',
|
||||
pnet.SEGMENTATION_ID: 2}]
|
||||
with self.network(**{'arg_list': (mpnet_apidef.SEGMENTS, ),
|
||||
mpnet_apidef.SEGMENTS: segments}) as net:
|
||||
|
@ -2677,6 +2681,61 @@ class TestMultiSegmentNetworks(Ml2PluginV2TestCase):
|
|||
dynamic_segmentation2_id = dynamic_segment2[driver_api.SEGMENTATION_ID]
|
||||
self.assertNotEqual(dynamic_segmentation_id, dynamic_segmentation2_id)
|
||||
|
||||
def test_allocate_dynamic_segments_race_condition(self):
|
||||
# create same segment two times, make sure the same segment comes back
|
||||
physnet_name = 'physnet1'
|
||||
segment = {driver_api.NETWORK_TYPE: 'vlan',
|
||||
driver_api.PHYSICAL_NETWORK: physnet_name}
|
||||
|
||||
data = {'network': {'name': 'net1',
|
||||
'tenant_id': 'tenant_one'}}
|
||||
network_req = self.new_create_request('networks', data)
|
||||
network = self.deserialize(self.fmt,
|
||||
network_req.get_response(self.api))
|
||||
segment = {driver_api.NETWORK_TYPE: 'vlan',
|
||||
driver_api.PHYSICAL_NETWORK: physnet_name}
|
||||
network_id = network['network']['id']
|
||||
|
||||
# create first segment
|
||||
seg1 = self.driver.type_manager.allocate_dynamic_segment(
|
||||
self.context, network_id, segment)
|
||||
|
||||
# create same segment but make sure it is not found on first try
|
||||
# thus simulating a race condition between two creates
|
||||
orig_get_dynamic_segment = segments_db.get_dynamic_segment
|
||||
|
||||
def return_none_on_first_call(*args, **kwargs):
|
||||
if not return_none_on_first_call.called:
|
||||
return_none_on_first_call.called = True
|
||||
return
|
||||
return orig_get_dynamic_segment(*args, **kwargs)
|
||||
return_none_on_first_call.called = False
|
||||
|
||||
with mock.patch('neutron.db.segments_db.get_dynamic_segment',
|
||||
wraps=return_none_on_first_call):
|
||||
seg2 = self.driver.type_manager.allocate_dynamic_segment(
|
||||
self.context, network_id, segment)
|
||||
|
||||
self.assertEqual('vlan', seg1[driver_api.NETWORK_TYPE])
|
||||
self.assertEqual(physnet_name,
|
||||
seg1[driver_api.PHYSICAL_NETWORK])
|
||||
|
||||
# make sure we got the same segment
|
||||
self.assertEqual(seg1[driver_api.PHYSICAL_NETWORK],
|
||||
seg2[driver_api.PHYSICAL_NETWORK])
|
||||
self.assertEqual(seg1[driver_api.ID],
|
||||
seg2[driver_api.ID])
|
||||
self.assertEqual(seg1[driver_api.SEGMENTATION_ID],
|
||||
seg2[driver_api.SEGMENTATION_ID])
|
||||
|
||||
# make sure that there is only one vlan allocated to our segment
|
||||
vlan_typedriver = self.driver.type_manager.drivers['vlan']
|
||||
seg_obj = vlan_typedriver.obj.segmentation_obj
|
||||
all_allocs = seg_obj.get_objects(self.context)
|
||||
allocs = [obj for obj in all_allocs
|
||||
if obj.allocated and obj.physical_network == physnet_name]
|
||||
self.assertEqual(1, len(allocs))
|
||||
|
||||
def test_allocate_release_dynamic_segment(self):
|
||||
data = {'network': {'name': 'net1',
|
||||
'tenant_id': 'tenant_one'}}
|
||||
|
@ -2787,7 +2846,7 @@ class TestMultiSegmentNetworks(Ml2PluginV2TestCase):
|
|||
pnet.PHYSICAL_NETWORK: 'physnet1',
|
||||
pnet.SEGMENTATION_ID: 1},
|
||||
{pnet.NETWORK_TYPE: 'vlan',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet1',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet2',
|
||||
pnet.SEGMENTATION_ID: 2}],
|
||||
'tenant_id': 'tenant_one'}}
|
||||
network_req = self.new_create_request('networks', data)
|
||||
|
@ -2841,7 +2900,7 @@ class TestMultiSegmentNetworks(Ml2PluginV2TestCase):
|
|||
res = network_req.get_response(self.api)
|
||||
self.assertEqual(400, res.status_int)
|
||||
|
||||
def test_create_network_duplicate_partial_segments(self):
|
||||
def test_create_network_duplicate_partial_segments_fail(self):
|
||||
data = {'network': {'name': 'net1',
|
||||
mpnet_apidef.SEGMENTS:
|
||||
[{pnet.NETWORK_TYPE: 'vlan',
|
||||
|
@ -2849,9 +2908,11 @@ class TestMultiSegmentNetworks(Ml2PluginV2TestCase):
|
|||
{pnet.NETWORK_TYPE: 'vlan',
|
||||
pnet.PHYSICAL_NETWORK: 'physnet1'}],
|
||||
'tenant_id': 'tenant_one'}}
|
||||
retry_fixture = fixture.DBRetryErrorsFixture(max_retries=2)
|
||||
retry_fixture.setUp()
|
||||
network_req = self.new_create_request('networks', data)
|
||||
res = network_req.get_response(self.api)
|
||||
self.assertEqual(201, res.status_int)
|
||||
self.assertEqual(409, res.status_int)
|
||||
|
||||
def test_release_network_segments(self):
|
||||
data = {'network': {'name': 'net1',
|
||||
|
@ -3488,10 +3549,10 @@ class TestML2Segments(Ml2PluginV2TestCase):
|
|||
driver_api.PHYSICAL_NETWORK: self.physnet,
|
||||
driver_api.SEGMENTATION_ID: 1000}
|
||||
seg2 = {driver_api.NETWORK_TYPE: 'vlan',
|
||||
driver_api.PHYSICAL_NETWORK: self.physnet,
|
||||
driver_api.PHYSICAL_NETWORK: self.physnet2,
|
||||
driver_api.SEGMENTATION_ID: 1001}
|
||||
seg3 = {driver_api.NETWORK_TYPE: 'vlan',
|
||||
driver_api.PHYSICAL_NETWORK: self.physnet,
|
||||
driver_api.PHYSICAL_NETWORK: self.physnet3,
|
||||
driver_api.SEGMENTATION_ID: 1002}
|
||||
with self.network() as network:
|
||||
network = network['network']
|
||||
|
|
|
@ -307,12 +307,12 @@ class TestAutoScheduleSegments(test_plugin.Ml2PluginV2TestCase,
|
|||
'shared': True}})
|
||||
return net['id']
|
||||
|
||||
def _create_segment(self, network_id):
|
||||
def _create_segment(self, network_id, physical_network='physnet1'):
|
||||
seg = self.segments_plugin.create_segment(
|
||||
self.ctx,
|
||||
{'segment': {'network_id': network_id,
|
||||
'name': None, 'description': None,
|
||||
'physical_network': 'physnet1',
|
||||
'physical_network': physical_network,
|
||||
'network_type': 'vlan',
|
||||
'segmentation_id': constants.ATTR_NOT_SPECIFIED}})
|
||||
return seg['id']
|
||||
|
@ -381,7 +381,7 @@ class TestAutoScheduleSegments(test_plugin.Ml2PluginV2TestCase,
|
|||
self.ctx, [net_id])
|
||||
self.assertEqual(1, len(agents))
|
||||
|
||||
seg2_id = self._create_segment(net_id)
|
||||
seg2_id = self._create_segment(net_id, 'physnet2')
|
||||
self._create_subnet(seg2_id, net_id, '192.168.11.0/24')
|
||||
helpers.register_dhcp_agent(HOST_C)
|
||||
segments_service_db.update_segment_host_mapping(
|
||||
|
|
|
@ -0,0 +1,8 @@
|
|||
---
|
||||
upgrade:
|
||||
- |
|
||||
A unique constraint for (network_id, network_type, physical_network) is
|
||||
added to the networksegments table. This was done to prevent race
|
||||
conditions on dynamic segment allocation. Operators having networks with
|
||||
multiple segments (e.g. when using hierarchical portbinding) should check
|
||||
that this constraint is not violated with the included upgrade-check.
|
Loading…
Reference in New Issue