From 25103df197c1f366eac8dd3069fabc01d3bd18e9 Mon Sep 17 00:00:00 2001 From: berlin Date: Mon, 31 Mar 2014 14:46:56 +0800 Subject: [PATCH] Check NVP router's status before deploying a service With NVP advanced service plugin, router creation is asynchronous while all service call is synchronous, so it is possible that advanced service request is called before edge deployment completed. The solution is to check the router status before deploying an advanced service. If the router is not in ACTIVE status, the service deployment request would return "Router not in 'ACTIVE' status" error. Change-Id: Idde71c37f5d5c113ac04376ed607e0c156b07477 Closes-Bug: #1298865 --- neutron/plugins/vmware/common/exceptions.py | 5 ++ neutron/plugins/vmware/plugins/service.py | 37 ++++++--------- .../unit/vmware/vshield/test_edge_router.py | 25 ++++++++-- .../unit/vmware/vshield/test_fwaas_plugin.py | 36 ++++++++++---- .../unit/vmware/vshield/test_lbaas_plugin.py | 33 +++++++++---- .../unit/vmware/vshield/test_vpnaas_plugin.py | 47 ++++++++++++++----- 6 files changed, 128 insertions(+), 55 deletions(-) diff --git a/neutron/plugins/vmware/common/exceptions.py b/neutron/plugins/vmware/common/exceptions.py index 3f435bd53..83cc05bf4 100644 --- a/neutron/plugins/vmware/common/exceptions.py +++ b/neutron/plugins/vmware/common/exceptions.py @@ -90,6 +90,11 @@ class VcnsDriverException(NsxPluginException): message = _("Error happened in NSX VCNS Driver: %(err_msg)s") +class AdvRouterServiceUnavailable(n_exc.ServiceUnavailable): + message = _("Router %(router_id)s is not in 'ACTIVE' " + "status, thus unable to provide advanced service") + + class ServiceClusterUnavailable(NsxPluginException): message = _("Service cluster: '%(cluster_id)s' is unavailable. Please, " "check NSX setup and/or configuration") diff --git a/neutron/plugins/vmware/plugins/service.py b/neutron/plugins/vmware/plugins/service.py index ca1dca82e..4a5b8e957 100644 --- a/neutron/plugins/vmware/plugins/service.py +++ b/neutron/plugins/vmware/plugins/service.py @@ -520,6 +520,17 @@ class NsxAdvancedPlugin(sr_db.ServiceRouter_mixin, router_id=router_id, firewall_id=firewalls[0]['id']) + def check_router(self, context, router_id): + if not router_id: + msg = _("router_id is not provided!") + raise n_exc.BadRequest(resource='router', msg=msg) + router = self._get_router(context, router_id) + if not self._is_advanced_service_router(context, router=router): + msg = _("router_id:%s is not an advanced router!") % router['id'] + raise n_exc.BadRequest(resource='router', msg=msg) + if router['status'] != service_constants.ACTIVE: + raise nsx_exc.AdvRouterServiceUnavailable(router_id=router['id']) + def _delete_lrouter(self, context, router_id, nsx_router_id): binding = vcns_db.get_vcns_router_binding(context.session, router_id) if not binding: @@ -903,14 +914,7 @@ class NsxAdvancedPlugin(sr_db.ServiceRouter_mixin, def create_firewall(self, context, firewall): LOG.debug(_("create_firewall() called")) router_id = firewall['firewall'].get(vcns_const.ROUTER_ID) - if not router_id: - msg = _("router_id is not provided!") - LOG.error(msg) - raise n_exc.BadRequest(resource='router', msg=msg) - if not self._is_advanced_service_router(context, router_id): - msg = _("router_id:%s is not an advanced router!") % router_id - LOG.error(msg) - raise n_exc.BadRequest(resource='router', msg=msg) + self.check_router(context, router_id) if self._get_resource_router_id_binding( context, firewall_db.Firewall, router_id=router_id): msg = _("A firewall is already associated with the router") @@ -1219,16 +1223,7 @@ class NsxAdvancedPlugin(sr_db.ServiceRouter_mixin, def create_vip(self, context, vip): LOG.debug(_("create_vip() called")) router_id = vip['vip'].get(vcns_const.ROUTER_ID) - if not router_id: - msg = _("router_id is not provided!") - LOG.error(msg) - raise n_exc.BadRequest(resource='router', msg=msg) - - if not self._is_advanced_service_router(context, router_id): - msg = _("router_id: %s is not an advanced router!") % router_id - LOG.error(msg) - raise nsx_exc.NsxPluginException(err_msg=msg) - + self.check_router(context, router_id) #Check whether the vip port is an external port subnet_id = vip['vip']['subnet_id'] network_id = self.get_subnet(context, subnet_id)['network_id'] @@ -1607,11 +1602,7 @@ class NsxAdvancedPlugin(sr_db.ServiceRouter_mixin, def create_vpnservice(self, context, vpnservice): LOG.debug(_("create_vpnservice() called")) router_id = vpnservice['vpnservice'].get('router_id') - if not self._is_advanced_service_router(context, router_id): - msg = _("router_id:%s is not an advanced router!") % router_id - LOG.warning(msg) - raise exceptions.VcnsBadRequest(resource='router', msg=msg) - + self.check_router(context, router_id) if self.get_vpnservices(context, filters={'router_id': [router_id]}): msg = _("a vpnservice is already associated with the router: %s" ) % router_id diff --git a/neutron/tests/unit/vmware/vshield/test_edge_router.py b/neutron/tests/unit/vmware/vshield/test_edge_router.py index c43e9b34b..e19de2502 100644 --- a/neutron/tests/unit/vmware/vshield/test_edge_router.py +++ b/neutron/tests/unit/vmware/vshield/test_edge_router.py @@ -21,9 +21,11 @@ from oslo.config import cfg from neutron.api.v2 import attributes from neutron import context +from neutron.db import l3_db from neutron.extensions import l3 from neutron import manager as n_manager from neutron.openstack.common import uuidutils +from neutron.plugins.common import constants as service_constants from neutron.plugins.vmware.common import utils from neutron.plugins.vmware.plugins import service as nsp from neutron.tests import base @@ -140,10 +142,11 @@ class ServiceRouterTest(test_nsx_plugin.L3NatTest, if admin_state_up: data['router']['admin_state_up'] = admin_state_up for arg in (('admin_state_up', 'tenant_id') + (arg_list or ())): - # Arg must be present and not empty - if arg in kwargs and kwargs[arg]: + # Arg must be present + if arg in kwargs: data['router'][arg] = kwargs[arg] - data['router']['service_router'] = True + if data['router'].get('service_router') is None: + data['router']['service_router'] = True router_req = self.new_create_request('routers', data, fmt) if set_context and tenant_id: # create a specific auth context for this request @@ -152,6 +155,22 @@ class ServiceRouterTest(test_nsx_plugin.L3NatTest, return router_req.get_response(self.ext_api) + def _create_and_get_router(self, active_set=True, **kwargs): + """Create advanced service router for services.""" + req = self._create_router(self.fmt, self._tenant_id, **kwargs) + res = self.deserialize(self.fmt, req) + router_id = res['router']['id'] + # manually set router status ACTIVE to pass through the router check, + # else mimic router creation ERROR condition. + status = (service_constants.ACTIVE if active_set + else service_constants.ERROR) + self.plugin._resource_set_status( + context.get_admin_context(), + l3_db.Router, + router_id, + status) + return router_id + class ServiceRouterTestCase(ServiceRouterTest, test_nsx_plugin.TestL3NatTestCase): diff --git a/neutron/tests/unit/vmware/vshield/test_fwaas_plugin.py b/neutron/tests/unit/vmware/vshield/test_fwaas_plugin.py index acd8e7da7..19bf692f2 100644 --- a/neutron/tests/unit/vmware/vshield/test_fwaas_plugin.py +++ b/neutron/tests/unit/vmware/vshield/test_fwaas_plugin.py @@ -100,11 +100,6 @@ class FirewallPluginTestCase(test_db_firewall.FirewallPluginDbTestCase, self.ext_api = None self.plugin = None - def _create_and_get_router(self): - req = self._create_router(self.fmt, self._tenant_id) - res = self.deserialize(self.fmt, req) - return res['router']['id'] - def _create_firewall(self, fmt, name, description, firewall_policy_id, admin_state_up=True, expected_res_status=None, **kwargs): @@ -141,21 +136,46 @@ class FirewallPluginTestCase(test_db_firewall.FirewallPluginDbTestCase, for k, v in attrs.iteritems(): self.assertEqual(fw['firewall'][k], v) - def test_create_firewall_without_policy(self): + def test_create_firewall_without_policy(self, **kwargs): name = "new_fw" attrs = self._get_test_firewall_attrs(name) - attrs['router_id'] = self._create_and_get_router() + if 'router_id' in kwargs: + attrs['router_id'] = kwargs.pop('router_id') + else: + attrs['router_id'] = self._create_and_get_router() with self.firewall(name=name, router_id=attrs['router_id'], admin_state_up= test_db_firewall.ADMIN_STATE_UP, - expected_res_status=201) as fw: + **kwargs) as fw: attrs = self._replace_firewall_status( attrs, const.PENDING_CREATE, const.ACTIVE) for k, v in attrs.iteritems(): self.assertEqual(fw['firewall'][k], v) + def test_create_firewall_with_invalid_router(self): + name = "new_fw" + attrs = self._get_test_firewall_attrs(name) + attrs['router_id'] = self._create_and_get_router() + self.assertRaises(webob.exc.HTTPClientError, + self.test_create_firewall_without_policy, + router_id=None) + self.assertRaises(webob.exc.HTTPClientError, + self.test_create_firewall_without_policy, + router_id='invalid_id') + + router_id = self._create_and_get_router( + arg_list=('service_router',), service_router=False) + self.assertRaises(webob.exc.HTTPClientError, + self.test_create_firewall_without_policy, + router_id=router_id) + + router_id = self._create_and_get_router(active_set=False) + self.assertRaises(webob.exc.HTTPClientError, + self.test_create_firewall_without_policy, + router_id=router_id) + def test_update_firewall(self): name = "new_fw" attrs = self._get_test_firewall_attrs(name) diff --git a/neutron/tests/unit/vmware/vshield/test_lbaas_plugin.py b/neutron/tests/unit/vmware/vshield/test_lbaas_plugin.py index a6737cfb8..fdb0be434 100644 --- a/neutron/tests/unit/vmware/vshield/test_lbaas_plugin.py +++ b/neutron/tests/unit/vmware/vshield/test_lbaas_plugin.py @@ -108,11 +108,6 @@ class TestLoadbalancerPlugin( self.ext_api = None self.plugin = None - def _create_and_get_router(self): - req = self._create_router(self.fmt, self._tenant_id) - res = self.deserialize(self.fmt, req) - return res['router']['id'] - def _get_vip_optional_args(self): args = super(TestLoadbalancerPlugin, self)._get_vip_optional_args() return args + ('router_id',) @@ -152,7 +147,7 @@ class TestLoadbalancerPlugin( for k, v in keys: self.assertEqual(res['health_monitor'][k], v) - def test_create_vip(self, **extras): + def test_create_vip(self, **kwargs): expected = { 'name': 'vip1', 'description': '', @@ -161,11 +156,14 @@ class TestLoadbalancerPlugin( 'connection_limit': -1, 'admin_state_up': True, 'status': 'ACTIVE', - 'router_id': self._create_and_get_router(), 'tenant_id': self._tenant_id, } + if 'router_id' in kwargs: + expected['router_id'] = kwargs.pop('router_id') + else: + expected['router_id'] = self._create_and_get_router() - expected.update(extras) + expected.update(kwargs) name = expected['name'] @@ -183,7 +181,7 @@ class TestLoadbalancerPlugin( ) with self.vip( router_id=expected['router_id'], name=name, - pool=pool, subnet=subnet, **extras) as vip: + pool=pool, subnet=subnet, **kwargs) as vip: for k in ('id', 'address', 'port_id', 'pool_id'): self.assertTrue(vip['vip'].get(k, None)) self.assertEqual( @@ -192,6 +190,23 @@ class TestLoadbalancerPlugin( expected ) + def test_create_vip_with_invalid_router(self): + self.assertRaises( + web_exc.HTTPClientError, + self.test_create_vip, router_id=None) + self.assertRaises( + web_exc.HTTPClientError, + self.test_create_vip, router_id='invalid_router_id') + router_id = self._create_and_get_router( + arg_list=('service_router',), service_router=False) + self.assertRaises( + web_exc.HTTPClientError, + self.test_create_vip, router_id=router_id) + router_id = self._create_and_get_router(active_set=False) + self.assertRaises( + web_exc.HTTPClientError, + self.test_create_vip, router_id=router_id) + def test_create_vip_with_session_persistence(self): self.test_create_vip(session_persistence={'type': 'HTTP_COOKIE'}) diff --git a/neutron/tests/unit/vmware/vshield/test_vpnaas_plugin.py b/neutron/tests/unit/vmware/vshield/test_vpnaas_plugin.py index 8d8f115ec..7ef1a5445 100644 --- a/neutron/tests/unit/vmware/vshield/test_vpnaas_plugin.py +++ b/neutron/tests/unit/vmware/vshield/test_vpnaas_plugin.py @@ -79,22 +79,18 @@ class TestVpnPlugin(test_db_vpnaas.VPNTestMixin, self.plugin = None @contextlib.contextmanager - def router(self, vlan_id=None): + def router(self, vlan_id=None, do_delete=True, **kwargs): with self._create_l3_ext_network(vlan_id) as net: with self.subnet(cidr='100.0.0.0/24', network=net) as s: - data = {'router': {'tenant_id': self._tenant_id}} - data['router']['service_router'] = True - router_req = self.new_create_request('routers', data, self.fmt) - - res = router_req.get_response(self.ext_api) - router = self.deserialize(self.fmt, res) + router_id = self._create_and_get_router(**kwargs) self._add_external_gateway_to_router( - router['router']['id'], - s['subnet']['network_id']) - router = self._show('routers', router['router']['id']) + router_id, s['subnet']['network_id']) + router = self._show('routers', router_id) yield router - - self._delete('routers', router['router']['id']) + if do_delete: + self._remove_external_gateway_from_router( + router_id, s['subnet']['network_id']) + self._delete('routers', router_id) def test_create_vpnservice(self, **extras): """Test case to create a vpnservice.""" @@ -135,6 +131,33 @@ class TestVpnPlugin(test_db_vpnaas.VPNTestMixin, self.assertEqual( res.status_int, webob.exc.HTTPConflict.code) + def test_create_vpnservice_with_invalid_router(self): + """Test case to create vpnservices with invalid router.""" + with self.subnet(cidr='10.2.0.0/24') as subnet: + with contextlib.nested( + self.router(arg_list=('service_router',), + service_router=False), + self.router(active_set=False)) as (r1, r2): + res = self._create_vpnservice( + 'json', 'vpnservice', True, + router_id='invalid_id', + subnet_id=(subnet['subnet']['id'])) + self.assertEqual( + res.status_int, webob.exc.HTTPBadRequest.code) + res = self._create_vpnservice( + 'json', 'vpnservice', True, + router_id=r1['router']['id'], + subnet_id=(subnet['subnet']['id'])) + self.assertEqual( + res.status_int, webob.exc.HTTPBadRequest.code) + res = self._create_vpnservice( + 'json', 'vpnservice', True, + router_id=r2['router']['id'], + subnet_id=(subnet['subnet']['id'])) + self.assertEqual( + res.status_int, + webob.exc.HTTPServiceUnavailable.code) + def test_update_vpnservice(self): """Test case to update a vpnservice.""" name = 'new_vpnservice1' -- 2.45.2