From 574bbc1acd56e8096e931248df33c96c0e50e069 Mon Sep 17 00:00:00 2001 From: Hongbin Lu Date: Sun, 27 May 2018 21:45:15 +0000 Subject: [PATCH] Prevent race condition on creating network We leverage the unique constraint on network table to gurantee at most one docker network can be created for one neutron network. In particular, this patch moves the creation of network object from API layer to the kuryr network driver, in which the docker network is created. This basically ensures that each call to create docker network is after a creation of network object and pass the unique constraint imposed by the DB layer. In addition, this patch also adds a destroy_network implementation in objects and DB layer. We will clean up the DB record if the network creation failed in docker. Change-Id: I9507e90452c5af4cf1a64a045a07a9e63200b2b4 Closes-Bug: #1743498 --- zun/api/controllers/v1/networks.py | 11 ++----- zun/compute/api.py | 4 +-- zun/compute/manager.py | 6 ++-- zun/compute/rpcapi.py | 5 +-- zun/container/docker/driver.py | 9 +++-- zun/db/api.py | 10 ++++++ zun/db/sqlalchemy/api.py | 9 +++++ zun/network/kuryr_network.py | 33 +++++++++++++++---- zun/objects/network.py | 8 ++++- .../unit/api/controllers/v1/test_networks.py | 4 ++- .../unit/compute/test_compute_manager.py | 4 +-- zun/tests/unit/network/test_kuryr_network.py | 27 ++++++++------- zun/tests/unit/objects/test_objects.py | 2 +- 13 files changed, 86 insertions(+), 46 deletions(-) diff --git a/zun/api/controllers/v1/networks.py b/zun/api/controllers/v1/networks.py index 0d012a15e..515ba1d0b 100644 --- a/zun/api/controllers/v1/networks.py +++ b/zun/api/controllers/v1/networks.py @@ -21,7 +21,6 @@ from zun.api import utils as api_utils from zun.api import validation from zun.common import exception from zun.common import policy -from zun import objects LOG = logging.getLogger(__name__) @@ -61,11 +60,7 @@ class NetworkController(base.Controller): :param network_dict: a network within the request body. """ context = pecan.request.context - policy.enforce(context, "network:create", - action="network:create") - network_dict['project_id'] = context.project_id - network_dict['user_id'] = context.user_id - new_network = objects.Network(context, **network_dict) - new_network.create(context) - pecan.request.compute_api.network_create(context, new_network) + policy.enforce(context, "network:create", action="network:create") + new_network = pecan.request.compute_api.network_create( + context, network_dict['neutron_net_id']) return view.format_network(pecan.request.host_url, new_network) diff --git a/zun/compute/api.py b/zun/compute/api.py index ee4ea3a9a..e80decc22 100644 --- a/zun/compute/api.py +++ b/zun/compute/api.py @@ -234,8 +234,8 @@ class API(object): container_actions.NETWORK_ATTACH) return self.rpcapi.network_attach(context, container, *args) - def network_create(self, context, network): - return self.rpcapi.network_create(context, network) + def network_create(self, context, *args): + return self.rpcapi.network_create(context, *args) def resize_container(self, context, container, *args): return self.rpcapi.resize_container(context, container, *args) diff --git a/zun/compute/manager.py b/zun/compute/manager.py index 4d5f506d1..e57086a53 100644 --- a/zun/compute/manager.py +++ b/zun/compute/manager.py @@ -1256,11 +1256,9 @@ class Manager(periodic_task.PeriodicTasks): self.driver.network_attach(context, container, requested_network) self._update_task_state(context, container, None) - def network_create(self, context, network): + def network_create(self, context, neutron_net_id): LOG.debug('Create network') - docker_network = self.driver.create_network(context, network) - network.network_id = docker_network['Id'] - network.save() + return self.driver.create_network(context, neutron_net_id) def resize_container(self, context, container, patch): @utils.synchronized(container.uuid) diff --git a/zun/compute/rpcapi.py b/zun/compute/rpcapi.py index 9e3fc83f1..03e749051 100644 --- a/zun/compute/rpcapi.py +++ b/zun/compute/rpcapi.py @@ -203,6 +203,7 @@ class API(rpc_service.API): container=container, requested_network=requested_network) - def network_create(self, context, new_network): + def network_create(self, context, neutron_net_id): host = None - return self._call(host, 'network_create', network=new_network) + return self._call(host, 'network_create', + neutron_net_id=neutron_net_id) diff --git a/zun/container/docker/driver.py b/zun/container/docker/driver.py index 357c76e67..e43a3cd58 100644 --- a/zun/container/docker/driver.py +++ b/zun/container/docker/driver.py @@ -1186,13 +1186,12 @@ class DockerDriver(driver.ContainerDriver): container.addresses = addresses container.save(context) - def create_network(self, context, network): + def create_network(self, context, neutron_net_id): with docker_utils.docker_client() as docker: network_api = zun_network.api(context, docker_api=docker) docker_net_name = self._get_docker_network_name( - context, network.neutron_net_id) - docker_network = network_api.create_network( - neutron_net_id=network.neutron_net_id, + context, neutron_net_id) + return network_api.create_network( + neutron_net_id=neutron_net_id, name=docker_net_name) - return docker_network diff --git a/zun/db/api.py b/zun/db/api.py index 3ebac03b1..5078319cf 100644 --- a/zun/db/api.py +++ b/zun/db/api.py @@ -1032,6 +1032,16 @@ def update_network(context, uuid, values): context, uuid, values) +@profiler.trace("db") +def destroy_network(context, network_uuid): + """Destroy a network. + + :param context: Request context + :param network_uuid: The uuid of a network. + """ + return _get_dbdriver_instance().destroy_network(context, network_uuid) + + @profiler.trace("db") def create_exec_instance(context, values): """Create a new exec instance. diff --git a/zun/db/sqlalchemy/api.py b/zun/db/sqlalchemy/api.py index 7d238436f..2295ad7c0 100644 --- a/zun/db/sqlalchemy/api.py +++ b/zun/db/sqlalchemy/api.py @@ -1228,6 +1228,15 @@ class Connection(object): except NoResultFound: raise exception.NetworkNotFound(network=network_uuid) + def destroy_network(self, context, network_uuid): + session = get_session() + with session.begin(): + query = model_query(models.Network, session=session) + query = add_identity_filter(query, network_uuid) + count = query.delete() + if count != 1: + raise exception.NetworkNotFound(network=network_uuid) + def list_exec_instances(self, context, filters=None, limit=None, marker=None, sort_key=None, sort_dir=None): query = model_query(models.ExecInstance) diff --git a/zun/network/kuryr_network.py b/zun/network/kuryr_network.py index e119d77c7..1e28677ac 100644 --- a/zun/network/kuryr_network.py +++ b/zun/network/kuryr_network.py @@ -25,6 +25,7 @@ from zun.common.i18n import _ import zun.conf from zun.network import network from zun.network import neutron +from zun import objects from zun.objects import fields as obj_fields from zun.pci import manager as pci_manager from zun.pci import utils as pci_utils @@ -113,16 +114,34 @@ class KuryrNetwork(network.Network): options['neutron.pool.v6.uuid'] = v6_subnet.get('subnetpool_id') options['neutron.subnet.v6.uuid'] = v6_subnet.get('id') + network_dict = {} + network_dict['project_id'] = self.context.project_id + network_dict['user_id'] = self.context.user_id + network_dict['name'] = name + network_dict['neutron_net_id'] = neutron_net_id + network = objects.Network(self.context, **network_dict) + # The DB model has unique constraint on 'neutron_net_id' field + # which will guarantee only one request can create the network in here + # (and call docker.create_network later) if there are concurrent + # requests on creating networks for the same neutron net. + network.create(self.context) + LOG.debug("Calling docker.create_network to create network %s, " "ipam_options %s, options %s", name, ipam_options, options) - docker_network = self.docker.create_network( - name=name, - driver=CONF.network.driver_name, - enable_ipv6=True if v6_subnet else False, - options=options, - ipam=ipam_options) + try: + docker_network = self.docker.create_network( + name=name, + driver=CONF.network.driver_name, + enable_ipv6=True if v6_subnet else False, + options=options, + ipam=ipam_options) + except Exception: + with excutils.save_and_reraise_exception(): + network.destroy() - return docker_network + network.network_id = docker_network['Id'] + network.save() + return network def _check_valid_subnetpool(self, neutron_api, subnetpool_id, subnet_cidr): diff --git a/zun/objects/network.py b/zun/objects/network.py index 86b06b258..ffa8b15d5 100644 --- a/zun/objects/network.py +++ b/zun/objects/network.py @@ -19,7 +19,8 @@ from zun.objects import base @base.ZunObjectRegistry.register class Network(base.ZunPersistentObject, base.ZunObject): # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Add destroy method + VERSION = '1.1' fields = { 'id': fields.IntegerField(), @@ -92,3 +93,8 @@ class Network(base.ZunPersistentObject, base.ZunObject): dbapi.update_network(context, self.uuid, updates) self.obj_reset_changes() + + @base.remotable + def destroy(self, context=None): + dbapi.destroy_network(context, self.uuid) + self.obj_reset_changes() diff --git a/zun/tests/unit/api/controllers/v1/test_networks.py b/zun/tests/unit/api/controllers/v1/test_networks.py index 98e5d1cbd..677caa22f 100644 --- a/zun/tests/unit/api/controllers/v1/test_networks.py +++ b/zun/tests/unit/api/controllers/v1/test_networks.py @@ -12,6 +12,7 @@ from mock import patch from zun.tests.unit.api import base as api_base +from zun.tests.unit.db import utils class TestNetworkController(api_base.FunctionalTest): @@ -19,7 +20,8 @@ class TestNetworkController(api_base.FunctionalTest): @patch('zun.compute.api.API.network_create') def test_network_create(self, mock_network_create, mock_policy): mock_policy.return_value = True - mock_network_create.side_effect = lambda x, y: y + test_object = utils.create_test_network(context=self.context) + mock_network_create.return_value = test_object params = ('{"name": "network-test", "neutron_net_id": "test-id"}') response = self.post('/v1/networks/', params=params, diff --git a/zun/tests/unit/compute/test_compute_manager.py b/zun/tests/unit/compute/test_compute_manager.py index e34892d76..d78e53def 100644 --- a/zun/tests/unit/compute/test_compute_manager.py +++ b/zun/tests/unit/compute/test_compute_manager.py @@ -1349,12 +1349,10 @@ class TestManager(base.TestCase): self.assertTrue(mock_fail.called) self.assertTrue(mock_delete_volume.called) - @mock.patch.object(Network, 'save') @mock.patch.object(fake_driver, 'create_network') - def test_network_create(self, mock_create, mock_save): + def test_network_create(self, mock_create): network = Network(self.context, **utils.get_test_network()) ret = ({'Id': '0eeftestnetwork'}) mock_create.return_value = ret self.compute_manager.network_create(self.context, network) mock_create.assert_any_call(self.context, network) - mock_save.assert_called_once() diff --git a/zun/tests/unit/network/test_kuryr_network.py b/zun/tests/unit/network/test_kuryr_network.py index a6ed8544b..7136b8631 100644 --- a/zun/tests/unit/network/test_kuryr_network.py +++ b/zun/tests/unit/network/test_kuryr_network.py @@ -20,6 +20,7 @@ from neutronclient.common import exceptions as n_exc from zun.common import exception from zun.network import kuryr_network from zun.objects.container import Container +from zun.objects.network import Network from zun.tests import base from zun.tests.unit.db import utils @@ -142,19 +143,20 @@ class KuryrNetworkTestCase(base.TestCase): self.network_api.init(self.context, self.docker_api) self.network_api.neutron_api = FakeNeutronClient() + @mock.patch.object(Network, 'create') + @mock.patch.object(Network, 'save') @mock.patch('zun.network.neutron.NeutronAPI') - def test_create_network_without_subnetpool(self, - mock_neutron_api_cls): + def test_create_network_without_subnetpool( + self, mock_neutron_api_cls, mock_save, mock_create): self.network_api.neutron_api.subnets[0].pop('subnetpool_id') mock_neutron_api_cls.return_value = self.network_api.neutron_api name = 'test_kuryr_network' neutron_net_id = 'fake-net-id' with mock.patch.object(self.network_api.docker, 'create_network', - return_value='docker-net' + return_value={'Id': 'docker-net'} ) as mock_create_network: - docker_network = self.network_api.create_network(name, - neutron_net_id) - self.assertEqual('docker-net', docker_network) + network = self.network_api.create_network(name, neutron_net_id) + self.assertEqual('docker-net', network.network_id) mock_create_network.assert_called_once_with( name=name, driver='kuryr', @@ -169,18 +171,19 @@ class KuryrNetworkTestCase(base.TestCase): 'neutron.subnet.uuid': 'fake-subnet-id', 'neutron.pool.uuid': None}) + @mock.patch.object(Network, 'create') + @mock.patch.object(Network, 'save') @mock.patch('zun.network.neutron.NeutronAPI') - def test_create_network_with_subnetpool(self, - mock_neutron_api_cls): + def test_create_network_with_subnetpool( + self, mock_neutron_api_cls, mock_save, mock_create): mock_neutron_api_cls.return_value = self.network_api.neutron_api name = 'test_kuryr_network' neutron_net_id = 'fake-net-id' with mock.patch.object(self.network_api.docker, 'create_network', - return_value='docker-net' + return_value={'Id': 'docker-net'} ) as mock_create_network: - docker_network = self.network_api.create_network(name, - neutron_net_id) - self.assertEqual('docker-net', docker_network) + network = self.network_api.create_network(name, neutron_net_id) + self.assertEqual('docker-net', network.network_id) mock_create_network.assert_called_once_with( name=name, driver='kuryr', diff --git a/zun/tests/unit/objects/test_objects.py b/zun/tests/unit/objects/test_objects.py index 0ffb91032..0938e3af4 100644 --- a/zun/tests/unit/objects/test_objects.py +++ b/zun/tests/unit/objects/test_objects.py @@ -364,7 +364,7 @@ object_data = { 'ContainerPCIRequests': '1.0-7b8f7f044661fe4e24e6949c035af2c4', 'ContainerAction': '1.1-b0c721f9e10c6c0d1e41e512c49eb877', 'ContainerActionEvent': '1.0-2974d0a6f5d4821fd4e223a88c10181a', - 'Network': '1.0-235ba13359282107f27c251af9aaffcd', + 'Network': '1.1-f57547d39a95cf36f2b026aa4a863879', 'ExecInstance': '1.0-59464e7b96db847c0abb1e96d3cec30a', }