From 500007411a6009e94b56cfa1d719bf279cc0f813 Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Mon, 23 Feb 2015 20:57:02 +0000 Subject: [PATCH] Move _set_subnet_arp_info to dvr_router Further teasing DVR apart from the agent and other router types. This change illustrates the need to use plugin_rpc and context from the agent. It leaves this as a dependence of dvr_router on the instance of the agent. So, when the agent creates a dvr_router, it passes itself in as an argument. It doesn't do this with other router types. I would like to eliminate the dependence of RPC while processing a router. However, this is just a refactor and at least decouples it as much as possible and isolates the dependence to DVR only. Change-Id: Icb2154648b7aee6ddf781c48f1801ff97d35c6e6 Partially-Implements: bp/restructure-l3-agent --- neutron/agent/l3/agent.py | 4 +- neutron/agent/l3/dvr.py | 18 +-------- neutron/agent/l3/dvr_router.py | 22 ++++++++++- .../tests/unit/agent/l3/test_dvr_router.py | 3 +- neutron/tests/unit/test_l3_agent.py | 37 +++++++++++-------- 5 files changed, 50 insertions(+), 34 deletions(-) diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index b0651f1cc..8ac4b02b2 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -276,6 +276,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, } if router.get('distributed'): + kwargs['agent'] = self kwargs['host'] = self.host return dvr_router.DvrRouter(*args, **kwargs) @@ -354,7 +355,8 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, self._set_subnet_info(p) self.internal_network_added(ri, p) ri.internal_ports.append(p) - self._set_subnet_arp_info(ri, p) + if ri.router['distributed']: + ri._set_subnet_arp_info(p) if (not new_ipv6_port and netaddr.IPNetwork(p['subnet']['cidr']).version == 6): new_ipv6_port = True diff --git a/neutron/agent/l3/dvr.py b/neutron/agent/l3/dvr.py index 56460aa44..cb5e10d55 100644 --- a/neutron/agent/l3/dvr.py +++ b/neutron/agent/l3/dvr.py @@ -61,22 +61,8 @@ class AgentMixin(object): fip_ns = self.get_fip_ns(ex_net_id) fip_ns.delete() - def _set_subnet_arp_info(self, ri, port): - """Set ARP info retrieved from Plugin for existing ports.""" - if 'id' not in port['subnet'] or not ri.router['distributed']: - return - subnet_id = port['subnet']['id'] - subnet_ports = ( - self.plugin_rpc.get_ports_by_subnet(self.context, - subnet_id)) - - for p in subnet_ports: - if p['device_owner'] not in l3_constants.ROUTER_INTERFACE_OWNERS: - for fixed_ip in p['fixed_ips']: - ri._update_arp_entry(fixed_ip['ip_address'], - p['mac_address'], - subnet_id, - 'add') + def get_ports_by_subnet(self, subnet_id): + return self.plugin_rpc.get_ports_by_subnet(self.context, subnet_id) def get_snat_int_device_name(self, port_id): return (SNAT_INT_DEV_PREFIX + diff --git a/neutron/agent/l3/dvr_router.py b/neutron/agent/l3/dvr_router.py index 82371a8a5..15425652c 100644 --- a/neutron/agent/l3/dvr_router.py +++ b/neutron/agent/l3/dvr_router.py @@ -28,9 +28,10 @@ LOG = logging.getLogger(__name__) class DvrRouter(router.RouterInfo): - def __init__(self, host, *args, **kwargs): + def __init__(self, agent, host, *args, **kwargs): super(DvrRouter, self).__init__(*args, **kwargs) + self.agent = agent self.host = host self.floating_ips_dict = {} @@ -194,3 +195,22 @@ class DvrRouter(router.RouterInfo): except Exception: with excutils.save_and_reraise_exception(): LOG.exception(_LE("DVR: Failed updating arp entry")) + + def _set_subnet_arp_info(self, port): + """Set ARP info retrieved from Plugin for existing ports.""" + if 'id' not in port['subnet']: + return + + subnet_id = port['subnet']['id'] + + # TODO(Carl) Can we eliminate the need to make this RPC while + # processing a router. + subnet_ports = self.agent.get_ports_by_subnet(subnet_id) + + for p in subnet_ports: + if p['device_owner'] not in l3_constants.ROUTER_INTERFACE_OWNERS: + for fixed_ip in p['fixed_ips']: + self._update_arp_entry(fixed_ip['ip_address'], + p['mac_address'], + subnet_id, + 'add') diff --git a/neutron/tests/unit/agent/l3/test_dvr_router.py b/neutron/tests/unit/agent/l3/test_dvr_router.py index 24009d2aa..adfc50a5c 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_router.py @@ -38,7 +38,8 @@ class TestDvrRouterOperations(base.BaseTestCase): self.router_id = _uuid() if not router: router = mock.MagicMock() - return dvr_router.DvrRouter(mock.sentinel.myhost, + return dvr_router.DvrRouter(mock.sentinel.agent, + mock.sentinel.myhost, self.router_id, router, agent_conf, diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index 77bf19002..968e70211 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -424,7 +424,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): agent.host = HOSTNAME agent._create_dvr_gateway = mock.Mock() agent.get_snat_interfaces = mock.Mock(return_value=self.snat_ports) - ri = dvr_router.DvrRouter(HOSTNAME, + ri = dvr_router.DvrRouter(agent, + HOSTNAME, router['id'], router, **self.ri_kwargs) @@ -526,7 +527,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): agent_mode, expected_call_count): router = prepare_router_data(num_internal_ports=2) agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) - ri = dvr_router.DvrRouter(HOSTNAME, + ri = dvr_router.DvrRouter(agent, + HOSTNAME, router['id'], router, **self.ri_kwargs) @@ -648,7 +650,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router = prepare_router_data(num_internal_ports=2) router['distributed'] = True ri = dvr_router.DvrRouter( - HOSTNAME, router['id'], router, **self.ri_kwargs) + agent, HOSTNAME, router['id'], router, **self.ri_kwargs) ports = ri.router.get(l3_constants.INTERFACE_KEY, []) test_ports = [{'mac_address': '00:11:22:33:44:55', 'device_owner': 'network:dhcp', @@ -659,13 +661,13 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): # Test basic case ports[0]['subnet']['id'] = _get_subnet_id(ports[0]) - agent._set_subnet_arp_info(ri, ports[0]) + ri._set_subnet_arp_info(ports[0]) self.mock_ip_dev.neigh.add.assert_called_once_with( 4, '1.2.3.4', '00:11:22:33:44:55') # Test negative case router['distributed'] = False - agent._set_subnet_arp_info(ri, ports[0]) + ri._set_subnet_arp_info(ports[0]) self.mock_ip_dev.neigh.add.never_called() def test_add_arp_entry(self): @@ -697,6 +699,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def test__update_arp_entry_with_no_subnet(self): ri = dvr_router.DvrRouter( + mock.sentinel.agent, HOSTNAME, 'foo_router_id', {'distributed': True, 'gw_port_host': HOSTNAME}, @@ -733,7 +736,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def test_process_dist_router(self): router = prepare_router_data() agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) - ri = dvr_router.DvrRouter(HOSTNAME, + ri = dvr_router.DvrRouter(agent, + HOSTNAME, router['id'], router, **self.ri_kwargs) @@ -855,9 +859,9 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router = prepare_router_data(enable_snat=True) router[l3_constants.FLOATINGIP_AGENT_INTF_KEY] = agent_gateway_port router['distributed'] = True - ri = dvr_router.DvrRouter( - HOSTNAME, router['id'], router, **self.ri_kwargs) agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + ri = dvr_router.DvrRouter( + agent, HOSTNAME, router['id'], router, **self.ri_kwargs) self.assertEqual( agent_gateway_port[0], agent.get_floating_agent_gw_interface(ri, fake_network_id)) @@ -886,10 +890,10 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router[l3_constants.FLOATINGIP_KEY] = fake_floatingips['floatingips'] router[l3_constants.FLOATINGIP_AGENT_INTF_KEY] = agent_gateway_port router['distributed'] = True + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) ri = dvr_router.DvrRouter( - HOSTNAME, router['id'], router, **self.ri_kwargs) + agent, HOSTNAME, router['id'], router, **self.ri_kwargs) - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) ext_gw_port = ri.router.get('gw_port') ri.fip_ns = agent.get_fip_ns(ext_gw_port['network_id']) ri.dist_fip_count = 0 @@ -947,13 +951,14 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router = prepare_router_data(enable_snat=True) router[l3_constants.FLOATINGIP_KEY] = fake_floatingips['floatingips'] router['distributed'] = True - ri = dvr_router.DvrRouter(HOSTNAME, + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + ri = dvr_router.DvrRouter(agent, + HOSTNAME, router['id'], router, **self.ri_kwargs) ri.iptables_manager.ipv4['nat'] = mock.MagicMock() ri.dist_fip_count = 0 - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) fip_ns = agent.get_fip_ns(mock.sentinel.ext_net_id) fip_ns.agent_gateway_port = ( {'fixed_ips': [{'ip_address': '20.0.0.30', @@ -1270,14 +1275,15 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): {fip_id: l3_constants.FLOATINGIP_STATUS_ERROR}) def test_handle_router_snat_rules_distributed_without_snat_manager(self): + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) ri = dvr_router.DvrRouter( + agent, HOSTNAME, 'foo_router_id', {'distributed': True}, **self.ri_kwargs) ri.iptables_manager = mock.Mock() - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) with mock.patch.object(l3_agent.LOG, 'debug') as log_debug: agent._handle_router_snat_rules( ri, mock.ANY, mock.ANY, mock.ANY) @@ -1709,7 +1715,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def test_create_dvr_gateway(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) router = prepare_router_data() - ri = dvr_router.DvrRouter(HOSTNAME, + ri = dvr_router.DvrRouter(agent, + HOSTNAME, router['id'], router, **self.ri_kwargs) @@ -1781,7 +1788,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): external_net_id = router['gw_port']['network_id'] ri = dvr_router.DvrRouter( - HOSTNAME, router['id'], router, **self.ri_kwargs) + agent, HOSTNAME, router['id'], router, **self.ri_kwargs) ri.remove_floating_ip = mock.Mock() agent._fetch_external_net_id = mock.Mock(return_value=external_net_id) ri.ex_gw_port = ri.router['gw_port'] -- 2.45.2