From de1967452e935e8d78255c448715a588d19ae838 Mon Sep 17 00:00:00 2001 From: Akihiro MOTOKI Date: Tue, 16 Jul 2013 17:05:19 +0900 Subject: [PATCH] Fix argument name mismatch in L3-RPC sync_routers In sync_routers L3-RPC method l3-agent sends router_ids but the server side expected router_id. This commit fixes the server side to accept router_ids, and drops "fullsync" arg from the agent side (fullsync is not used anywhere and it does not affect RPC signature). This change allows l3-agent to sync only the specified routers instead of all routers. Fixes bug #1201553 As a result of the above change, auto_schedule_routers() and list_active_sync_routers_on_active_l3_agent() in L3 scheduler needs to handle a list of router IDs. This commit changes L3 scheduler to accept a list of router IDs in the above two methods. Also fixes the argument order of fullsync and router_ids in get_routers in L3PluginApi. L3-agent main code expects router_ids as the second arg. Change-Id: I22e8d11b9676cbcfe9e72449031bb63071be8314 --- neutron/agent/l3_agent.py | 3 +- neutron/db/agentschedulers_db.py | 15 ++-- neutron/db/l3_rpc_base.py | 10 +-- neutron/scheduler/l3_agent_scheduler.py | 52 +++++++------ .../unit/openvswitch/test_agent_scheduler.py | 76 ++++++++++++++++++- 5 files changed, 117 insertions(+), 39 deletions(-) diff --git a/neutron/agent/l3_agent.py b/neutron/agent/l3_agent.py index 1f5105d1a..402c1cc18 100644 --- a/neutron/agent/l3_agent.py +++ b/neutron/agent/l3_agent.py @@ -67,11 +67,10 @@ class L3PluginApi(proxy.RpcProxy): topic=topic, default_version=self.BASE_RPC_API_VERSION) self.host = host - def get_routers(self, context, fullsync=True, router_ids=None): + def get_routers(self, context, router_ids=None): """Make a remote process call to retrieve the sync data for routers.""" return self.call(context, self.make_msg('sync_routers', host=self.host, - fullsync=fullsync, router_ids=router_ids), topic=self.topic) diff --git a/neutron/db/agentschedulers_db.py b/neutron/db/agentschedulers_db.py index c9ea7b118..b030b1316 100644 --- a/neutron/db/agentschedulers_db.py +++ b/neutron/db/agentschedulers_db.py @@ -123,7 +123,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, result = self.auto_schedule_routers(context, agent_db.host, - router_id) + [router_id]) if not result: raise l3agentscheduler.RouterSchedulingFailed( router_id=router_id, agent_id=id) @@ -166,7 +166,7 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, return {'routers': []} def list_active_sync_routers_on_active_l3_agent( - self, context, host, router_id): + self, context, host, router_ids): agent = self._get_agent_by_type_and_host( context, constants.AGENT_TYPE_L3, host) if not agent.admin_state_up: @@ -174,9 +174,12 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, query = context.session.query(RouterL3AgentBinding.router_id) query = query.filter( RouterL3AgentBinding.l3_agent_id == agent.id) - if router_id: - query = query.filter(RouterL3AgentBinding.router_id == router_id) + if not router_ids: + pass + else: + query = query.filter( + RouterL3AgentBinding.router_id.in_(router_ids)) router_ids = [item[0] for item in query] if router_ids: return self.get_sync_data(context, router_ids=router_ids, @@ -272,10 +275,10 @@ class L3AgentSchedulerDbMixin(l3agentscheduler.L3AgentSchedulerPluginBase, candidates.append(l3_agent) return candidates - def auto_schedule_routers(self, context, host, router_id): + def auto_schedule_routers(self, context, host, router_ids): if self.router_scheduler: return self.router_scheduler.auto_schedule_routers( - self, context, host, router_id) + self, context, host, router_ids) def schedule_router(self, context, router): if self.router_scheduler: diff --git a/neutron/db/l3_rpc_base.py b/neutron/db/l3_rpc_base.py index 4b1387187..1cad6c94c 100644 --- a/neutron/db/l3_rpc_base.py +++ b/neutron/db/l3_rpc_base.py @@ -33,22 +33,22 @@ class L3RpcCallbackMixin(object): """Sync routers according to filters to a specific agent. @param context: contain user information - @param kwargs: host, or router_id + @param kwargs: host, router_ids @return: a list of routers with their interfaces and floating_ips """ - router_id = kwargs.get('router_id') + router_ids = kwargs.get('router_ids') host = kwargs.get('host') context = neutron_context.get_admin_context() plugin = manager.NeutronManager.get_plugin() if utils.is_extension_supported( plugin, constants.L3_AGENT_SCHEDULER_EXT_ALIAS): if cfg.CONF.router_auto_schedule: - plugin.auto_schedule_routers(context, host, router_id) + plugin.auto_schedule_routers(context, host, router_ids) routers = plugin.list_active_sync_routers_on_active_l3_agent( - context, host, router_id) + context, host, router_ids) else: - routers = plugin.get_sync_data(context, router_id) + routers = plugin.get_sync_data(context, router_ids) LOG.debug(_("Routers returned to l3 agent:\n %s"), jsonutils.dumps(routers, indent=5)) return routers diff --git a/neutron/scheduler/l3_agent_scheduler.py b/neutron/scheduler/l3_agent_scheduler.py index e90dfa268..22dd97734 100644 --- a/neutron/scheduler/l3_agent_scheduler.py +++ b/neutron/scheduler/l3_agent_scheduler.py @@ -36,10 +36,11 @@ class ChanceScheduler(object): can be introduced later. """ - def auto_schedule_routers(self, plugin, context, host, router_id): + def auto_schedule_routers(self, plugin, context, host, router_ids): """Schedule non-hosted routers to L3 Agent running on host. - If router_id is given, only this router is scheduled - if it is not hosted yet. + If router_ids is given, each router in router_ids is scheduled + if it is not scheduled yet. Otherwise all unscheduled routers + are scheduled. Don't schedule the routers which are hosted already by active l3 agents. """ @@ -59,42 +60,45 @@ class ChanceScheduler(object): if agents_db.AgentDbMixin.is_agent_down( l3_agent.heartbeat_timestamp): LOG.warn(_('L3 agent %s is not active'), l3_agent.id) - # check if the specified router is hosted - if router_id: - l3_agents = plugin.get_l3_agents_hosting_routers( - context, [router_id], admin_state_up=True) - if l3_agents: - LOG.debug(_('Router %(router_id)s has already been hosted' - ' by L3 agent %(agent_id)s'), - {'router_id': router_id, - 'agent_id': l3_agents[0]['id']}) + # check if each of the specified routers is hosted + if router_ids: + unscheduled_router_ids = [] + for router_id in router_ids: + l3_agents = plugin.get_l3_agents_hosting_routers( + context, [router_id], admin_state_up=True) + if l3_agents: + LOG.debug(_('Router %(router_id)s has already been' + ' hosted by L3 agent %(agent_id)s'), + {'router_id': router_id, + 'agent_id': l3_agents[0]['id']}) + else: + unscheduled_router_ids.append(router_id) + if not unscheduled_router_ids: + # all (specified) routers are already scheduled return False - - # get the router ids - if router_id: - router_ids = [(router_id,)] else: # get all routers that are not hosted #TODO(gongysh) consider the disabled agent's router stmt = ~exists().where( l3_db.Router.id == agentschedulers_db.RouterL3AgentBinding.router_id) - router_ids = context.session.query( - l3_db.Router.id).filter(stmt).all() - if not router_ids: - LOG.debug(_('No non-hosted routers')) - return False + unscheduled_router_ids = [router_id_[0] for router_id_ in + context.session.query( + l3_db.Router.id).filter(stmt)] + if not unscheduled_router_ids: + LOG.debug(_('No non-hosted routers')) + return False # check if the configuration of l3 agent is compatible # with the router - router_ids = [router_id_[0] for router_id_ in router_ids] - routers = plugin.get_routers(context, filters={'id': router_ids}) + routers = plugin.get_routers( + context, filters={'id': unscheduled_router_ids}) to_removed_ids = [] for router in routers: candidates = plugin.get_l3_agent_candidates(router, [l3_agent]) if not candidates: to_removed_ids.append(router['id']) - router_ids = list(set(router_ids) - set(to_removed_ids)) + router_ids = set(unscheduled_router_ids) - set(to_removed_ids) if not router_ids: LOG.warn(_('No routers compatible with L3 agent configuration' ' on host %s'), host) diff --git a/neutron/tests/unit/openvswitch/test_agent_scheduler.py b/neutron/tests/unit/openvswitch/test_agent_scheduler.py index 804e2e7d8..77221ceed 100644 --- a/neutron/tests/unit/openvswitch/test_agent_scheduler.py +++ b/neutron/tests/unit/openvswitch/test_agent_scheduler.py @@ -568,10 +568,13 @@ class OvsAgentSchedulerTestCase(test_l3_plugin.L3NatTestCaseMixin, with self.router() as router: l3_rpc = l3_rpc_base.L3RpcCallbackMixin() self._register_agent_states() - l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA) - l3_rpc.sync_routers(self.adminContext, host=L3_HOSTB) + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA) + ret_b = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTB) l3_agents = self._list_l3_agents_hosting_router( router['router']['id']) + self.assertEqual(1, len(ret_a)) + self.assertIn(router['router']['id'], [r['id'] for r in ret_a]) + self.assertFalse(len(ret_b)) self.assertEqual(1, len(l3_agents['agents'])) self.assertEqual(L3_HOSTA, l3_agents['agents'][0]['host']) @@ -682,6 +685,75 @@ class OvsAgentSchedulerTestCase(test_l3_plugin.L3NatTestCaseMixin, self.assertEqual(1, len(l3_agents_1['agents'])) self.assertEqual(0, len(l3_agents_2['agents'])) + def test_rpc_sync_routers(self): + l3_rpc = l3_rpc_base.L3RpcCallbackMixin() + self._register_agent_states() + + # No routers + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA) + self.assertEqual(0, len(ret_a)) + + with contextlib.nested(self.router(), + self.router(), + self.router()) as routers: + router_ids = [r['router']['id'] for r in routers] + + # Get all routers + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA) + self.assertEqual(3, len(ret_a)) + self.assertEqual(set(router_ids), set([r['id'] for r in ret_a])) + + # Get all routers (router_ids=None) + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA, + router_ids=None) + self.assertEqual(3, len(ret_a)) + self.assertEqual(set(router_ids), set([r['id'] for r in ret_a])) + + # Get router2 only + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA, + router_ids=[router_ids[1]]) + self.assertEqual(1, len(ret_a)) + self.assertIn(router_ids[1], [r['id'] for r in ret_a]) + + # Get router1 and router3 + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA, + router_ids=[router_ids[0], + router_ids[2]]) + self.assertEqual(2, len(ret_a)) + self.assertIn(router_ids[0], [r['id'] for r in ret_a]) + self.assertIn(router_ids[2], [r['id'] for r in ret_a]) + + def test_router_auto_schedule_for_specified_routers(self): + + def _sync_router_with_ids(router_ids, exp_synced, exp_hosted, host_id): + ret_a = l3_rpc.sync_routers(self.adminContext, host=L3_HOSTA, + router_ids=router_ids) + self.assertEqual(exp_synced, len(ret_a)) + for r in router_ids: + self.assertIn(r, [r['id'] for r in ret_a]) + host_routers = self._list_routers_hosted_by_l3_agent(host_id) + num_host_routers = len(host_routers['routers']) + self.assertEqual(exp_hosted, num_host_routers) + + l3_rpc = l3_rpc_base.L3RpcCallbackMixin() + self._register_agent_states() + hosta_id = self._get_agent_id(constants.AGENT_TYPE_L3, L3_HOSTA) + + with contextlib.nested(self.router(), self.router(), + self.router(), self.router()) as routers: + router_ids = [r['router']['id'] for r in routers] + # Sync router1 (router1 is scheduled) + _sync_router_with_ids([router_ids[0]], 1, 1, hosta_id) + # Sync router1 only (no router is scheduled) + _sync_router_with_ids([router_ids[0]], 1, 1, hosta_id) + # Schedule router2 + _sync_router_with_ids([router_ids[1]], 1, 2, hosta_id) + # Sync router2 and router4 (router4 is scheduled) + _sync_router_with_ids([router_ids[1], router_ids[3]], + 2, 3, hosta_id) + # Sync all routers (router3 is scheduled) + _sync_router_with_ids(router_ids, 4, 4, hosta_id) + def test_router_schedule_with_candidates(self): l3_hosta = { 'binary': 'neutron-l3-agent',