From 13f8296d5c1bf0e2bc90ee689a4825ec9acf8881 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Thu, 18 Jun 2020 23:18:07 +0200 Subject: [PATCH] Make _ensure_default_security_group method atomic Method _ensure_default_security_group wasn't atomic as it first tries to get default SG and if that not exists in DB, it tries to create it. It may happend, like e.g. in Calico plugin that between get_default_sg_id method and create_security_group method, this default SG will be created by other neutron worker. And in such case there will be Duplicate entry exception raised. So this patch is adding handling of such exception. Conflicts: neutron/db/securitygroups_db.py Change-Id: I515c310f221e7d9ae3be59a26260538d1bc591c2 Closes-Bug: #1883730 (cherry picked from commit 7019c5cf50bc4ad97b302220664a26efaf81a7fd) --- neutron/db/securitygroups_db.py | 13 +++-- .../tests/unit/db/test_securitygroups_db.py | 47 +++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 5b47ccfabda..7167a749f9a 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -26,6 +26,7 @@ from neutron_lib.db import model_query from neutron_lib.db import resource_extend from neutron_lib.db import utils as db_utils from neutron_lib import exceptions as n_exc +from neutron_lib.objects import exceptions as obj_exc from neutron_lib.utils import helpers from neutron_lib.utils import net from oslo_utils import uuidutils @@ -45,6 +46,9 @@ from neutron.objects import securitygroup as sg_obj from neutron import quota +DEFAULT_SG_DESCRIPTION = _('Default security group') + + @resource_extend.has_resource_extenders @registry.has_registry_receivers class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, @@ -821,10 +825,13 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, 'security_group': {'name': 'default', 'tenant_id': tenant_id, - 'description': _('Default security group')} + 'description': DEFAULT_SG_DESCRIPTION} } - return self.create_security_group(context, security_group, - default_sg=True)['id'] + try: + return self.create_security_group(context, security_group, + default_sg=True)['id'] + except obj_exc.NeutronDbObjectDuplicateEntry: + return self._get_default_sg_id(context, tenant_id) def _get_security_groups_on_port(self, context, port): """Check that all security groups on port belong to tenant. diff --git a/neutron/tests/unit/db/test_securitygroups_db.py b/neutron/tests/unit/db/test_securitygroups_db.py index 5084f984df0..ba73d360777 100644 --- a/neutron/tests/unit/db/test_securitygroups_db.py +++ b/neutron/tests/unit/db/test_securitygroups_db.py @@ -20,6 +20,7 @@ from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources from neutron_lib import constants from neutron_lib import context +from neutron_lib.objects import exceptions as obj_exc import sqlalchemy import testtools @@ -539,3 +540,49 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): for rule in (rule for rule in rules_after if rule not in rules_before): self.assertEqual('tenant_1', rule['tenant_id']) self.assertEqual(self.sg_user['id'], rule['security_group_id']) + + def test__ensure_default_security_group(self): + with mock.patch.object( + self.mixin, '_get_default_sg_id') as get_default_sg_id,\ + mock.patch.object( + self.mixin, 'create_security_group') as create_sg: + get_default_sg_id.return_value = None + self.mixin._ensure_default_security_group(self.ctx, 'tenant_1') + create_sg.assert_called_once_with( + self.ctx, + {'security_group': { + 'name': 'default', + 'tenant_id': 'tenant_1', + 'description': securitygroups_db.DEFAULT_SG_DESCRIPTION}}, + default_sg=True) + get_default_sg_id.assert_called_once_with(self.ctx, 'tenant_1') + + def test__ensure_default_security_group_already_exists(self): + with mock.patch.object( + self.mixin, '_get_default_sg_id') as get_default_sg_id,\ + mock.patch.object( + self.mixin, 'create_security_group') as create_sg: + get_default_sg_id.return_value = 'default_sg_id' + self.mixin._ensure_default_security_group(self.ctx, 'tenant_1') + create_sg.assert_not_called() + get_default_sg_id.assert_called_once_with(self.ctx, 'tenant_1') + + def test__ensure_default_security_group_created_in_parallel(self): + with mock.patch.object( + self.mixin, '_get_default_sg_id') as get_default_sg_id,\ + mock.patch.object( + self.mixin, 'create_security_group') as create_sg: + get_default_sg_id.side_effect = [None, 'default_sg_id'] + create_sg.side_effect = obj_exc.NeutronDbObjectDuplicateEntry( + mock.Mock(), mock.Mock()) + self.mixin._ensure_default_security_group(self.ctx, 'tenant_1') + create_sg.assert_called_once_with( + self.ctx, + {'security_group': { + 'name': 'default', + 'tenant_id': 'tenant_1', + 'description': securitygroups_db.DEFAULT_SG_DESCRIPTION}}, + default_sg=True) + get_default_sg_id.assert_has_calls([ + mock.call(self.ctx, 'tenant_1'), + mock.call(self.ctx, 'tenant_1')])