diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 8f349eb42db..98b7eea4b99 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -633,15 +633,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, raise n_exc.BadRequest(resource='router', msg=msg) return port - def _add_interface_by_port(self, context, router, port_id, owner): - # Update owner before actual process in order to avoid the - # case where a port might get attached to a router without the - # owner successfully updating due to an unavailable backend. - self._check_router_port(context, port_id, '') - self._core_plugin.update_port( - context, port_id, {'port': {'device_id': router.id, - 'device_owner': owner}}) - + def _validate_router_port_info(self, context, router, port_id): with context.session.begin(subtransactions=True): # check again within transaction to mitigate race port = self._check_router_port(context, port_id, router.id) @@ -678,6 +670,23 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, raise n_exc.BadRequest(resource='router', msg=msg) return port, subnets + def _add_interface_by_port(self, context, router, port_id, owner): + # Update owner before actual process in order to avoid the + # case where a port might get attached to a router without the + # owner successfully updating due to an unavailable backend. + port = self._check_router_port(context, port_id, '') + prev_owner = port['device_owner'] + self._core_plugin.update_port( + context, port_id, {'port': {'device_id': router.id, + 'device_owner': owner}}) + try: + return self._validate_router_port_info(context, router, port_id) + except Exception: + with excutils.save_and_reraise_exception(): + self._core_plugin.update_port( + context, port_id, {'port': {'device_id': '', + 'device_owner': prev_owner}}) + def _port_has_ipv6_address(self, port): for fixed_ip in port['fixed_ips']: if netaddr.IPNetwork(fixed_ip['ip_address']).version == 6: diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index e7f8382cf5e..b7617384b6b 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -1241,6 +1241,30 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): 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) + expected_port_update_before_update = { + 'device_owner': l3_constants.DEVICE_OWNER_ROUTER_INTF, + 'device_id': device_id} + expected_port_update_after_fail = { + 'device_owner': '', + 'device_id': ''} + mock_update_port.assert_has_calls( + [mock.call( + mock.ANY, + port_id, + {'port': expected_port_update_before_update}), + mock.call( + mock.ANY, + port_id, + {'port': expected_port_update_after_fail})], + any_order=False) + # fetch port and confirm device_id and device_owner + body = self._show('ports', port_id) + self.assertEqual('', body['port']['device_owner']) + self.assertEqual('', body['port']['device_id']) + def test_router_add_interface_multiple_ipv4_subnet_port_returns_400(self): """Test adding router port with multiple IPv4 subnets fails. @@ -1252,13 +1276,17 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): self.subnet(network=n, cidr='10.0.1.0/24')) as s2: fixed_ips = [{'subnet_id': s1['subnet']['id']}, {'subnet_id': s2['subnet']['id']}] - with self.port(subnet=s1, fixed_ips=fixed_ips) as p: + orig_update_port = self.plugin.update_port + with self.port(subnet=s1, fixed_ips=fixed_ips) as p, ( + mock.patch.object(self.plugin, + 'update_port')) as update_port: + update_port.side_effect = orig_update_port exp_code = exc.HTTPBadRequest.code - self._router_interface_action('add', - r['router']['id'], - None, - p['port']['id'], - expected_code=exp_code) + body = self._router_interface_action( + 'add', r['router']['id'], None, p['port']['id'], + expected_code=exp_code) + self._assert_body_port_id_and_update_port( + body, update_port, p['port']['id'], r['router']['id']) def test_router_add_interface_ipv6_port_existing_network_returns_400(self): """Ensure unique IPv6 router ports per network id. @@ -1273,17 +1301,21 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): ip_version=6) as s1, ( self.subnet(network=n, cidr='fd01::/64', ip_version=6)) as s2: - with self.port(subnet=s1) as p: + orig_update_port = self.plugin.update_port + with self.port(subnet=s1) as p, ( + mock.patch.object(self.plugin, + 'update_port')) as update_port: + update_port.side_effect = orig_update_port self._router_interface_action('add', r['router']['id'], s2['subnet']['id'], None) exp_code = exc.HTTPBadRequest.code - self._router_interface_action('add', - r['router']['id'], - None, - p['port']['id'], - expected_code=exp_code) + body = self._router_interface_action( + 'add', r['router']['id'], None, p['port']['id'], + expected_code=exp_code) + self._assert_body_port_id_and_update_port( + body, update_port, p['port']['id'], r['router']['id']) self._router_interface_action('remove', r['router']['id'], s2['subnet']['id'], @@ -1378,18 +1410,22 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): with self.router() as r: with self.subnet() as s1, self.subnet(cidr='1.0.0.0/24') as s2: with self.port(subnet=s1) as p1, self.port(subnet=s2) as p2: - with self.port(subnet=s1) as p3: + orig_update_port = self.plugin.update_port + with self.port(subnet=s1) as p3, ( + mock.patch.object(self.plugin, + 'update_port')) as update_port: + update_port.side_effect = orig_update_port for p in [p1, p2]: self._router_interface_action('add', r['router']['id'], None, p['port']['id']) - self._router_interface_action('add', - r['router']['id'], - None, - p3['port']['id'], - expected_code=exc. - HTTPBadRequest.code) + body = self._router_interface_action( + 'add', r['router']['id'], None, p3['port']['id'], + expected_code=exc.HTTPBadRequest.code) + self._assert_body_port_id_and_update_port( + body, update_port, p3['port']['id'], + r['router']['id']) def test_router_add_interface_overlapped_cidr_returns_400(self): with self.router() as r: