From d29e2f77d3933ea0279bf1863a31baf3a73a86e7 Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Thu, 14 Apr 2016 12:49:08 -0700 Subject: [PATCH] DVR: Clear SNAT namespace when agent restarts after router move When we manually move a router from one dvr_snat node to another dvr_snat node the snat_namespace should be removed in the originating node by the agent and will be re-created in the destination node by the destination agent. But when the agent dies, the router_update message reaches the agent after the agent restarts. At this time the agent should remove the snat_namespace since it is no more hosted by the current agent. Even though we do have logic in agent to take care of cleaning up the snat namespaces if the gw_port_host does not match with the existing agent host, in this particular use case the self.snat_namespace is always set to 'None' in the dvr_edge_router init call when agent restarts. This patch fixes the above issue by initializing the snat namespace object during the router_init. Since we do have a valid snat namespace object and if the gw_port_host mismatches, the agent should clean up the namespace. Change-Id: I30524dc77b743429ef70941479c9b6cccb21c23c Closes-Bug: #1557909 (cherry picked from commit 9dc70ed77e055677a4bd3257a0e9e24239ed4cce) --- neutron/agent/l3/dvr_edge_router.py | 16 ++-- neutron/agent/l3/dvr_snat_ns.py | 17 +++-- neutron/tests/unit/agent/l3/test_agent.py | 91 ++++++++++++++++++++++- 3 files changed, 110 insertions(+), 14 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py index c1670d40063..b46e88208dd 100644 --- a/neutron/agent/l3/dvr_edge_router.py +++ b/neutron/agent/l3/dvr_edge_router.py @@ -28,7 +28,12 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): def __init__(self, agent, host, *args, **kwargs): super(DvrEdgeRouter, self).__init__(agent, host, *args, **kwargs) - self.snat_namespace = None + # NOTE: Initialize the snat_namespace object here. + # The namespace can be created later, just to align with the + # parent init. + self.snat_namespace = dvr_snat_ns.SnatNamespace( + self.router_id, self.agent_conf, + self.driver, self.use_ipv6) self.snat_iptables_manager = None def external_gateway_added(self, ex_gw_port, interface_name): @@ -145,10 +150,11 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): # TODO(mlavalle): in the near future, this method should contain the # code in the L3 agent that creates a gateway for a dvr. The first step # is to move the creation of the snat namespace here - self.snat_namespace = dvr_snat_ns.SnatNamespace(self.router['id'], - self.agent_conf, - self.driver, - self.use_ipv6) + if not self.snat_namespace: + self.snat_namespace = dvr_snat_ns.SnatNamespace(self.router['id'], + self.agent_conf, + self.driver, + self.use_ipv6) self.snat_namespace.create() return self.snat_namespace diff --git a/neutron/agent/l3/dvr_snat_ns.py b/neutron/agent/l3/dvr_snat_ns.py index 2e360cca3ae..7941e36ec83 100644 --- a/neutron/agent/l3/dvr_snat_ns.py +++ b/neutron/agent/l3/dvr_snat_ns.py @@ -35,12 +35,13 @@ class SnatNamespace(namespaces.Namespace): def delete(self): ns_ip = ip_lib.IPWrapper(namespace=self.name) - for d in ns_ip.get_devices(exclude_loopback=True): - if d.name.startswith(SNAT_INT_DEV_PREFIX): - LOG.debug('Unplugging DVR device %s', d.name) - self.driver.unplug(d.name, namespace=self.name, - prefix=SNAT_INT_DEV_PREFIX) + if ns_ip.netns.exists(self.name): + for d in ns_ip.get_devices(exclude_loopback=True): + if d.name.startswith(SNAT_INT_DEV_PREFIX): + LOG.debug('Unplugging DVR device %s', d.name) + self.driver.unplug(d.name, namespace=self.name, + prefix=SNAT_INT_DEV_PREFIX) - # TODO(mrsmith): delete ext-gw-port - LOG.debug('DVR: destroy snat ns: %s', self.name) - super(SnatNamespace, self).delete() + # TODO(mrsmith): delete ext-gw-port + LOG.debug('DVR: destroy snat ns: %s', self.name) + super(SnatNamespace, self).delete() diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index bddaa6b9314..f8cd82b02c4 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -611,6 +611,95 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): def test_external_gateway_updated_dual_stack(self): self._test_external_gateway_updated(dual_stack=True) + def test_dvr_edge_router_init_for_snat_namespace_object(self): + router = l3_test_common.prepare_router_data(num_internal_ports=2) + ri = dvr_router.DvrEdgeRouter(mock.Mock(), + HOSTNAME, + router['id'], + router, + **self.ri_kwargs) + # Makesure that ri.snat_namespace object is created when the + # router is initialized + self.assertIsNotNone(ri.snat_namespace) + + @mock.patch.object(dvr_snat_ns.SnatNamespace, 'delete') + def test_ext_gw_updated_calling_snat_ns_delete_if_gw_port_host_none( + self, mock_snat_ns): + """Function to check the impact of snat_namespace object. + + This function specifically checks the impact of the snat + namespace object value on external_gateway_removed for deleting + snat_namespace when the gw_port_host mismatches or none. + """ + router = l3_test_common.prepare_router_data(num_internal_ports=2) + ri = dvr_router.DvrEdgeRouter(mock.Mock(), + HOSTNAME, + router['id'], + router, + **self.ri_kwargs) + ri._create_snat_namespace() + interface_name, ex_gw_port = l3_test_common.prepare_ext_gw_test(self, + ri) + router['gw_port_host'] = '' + ri.external_gateway_updated(ex_gw_port, interface_name) + if router['gw_port_host'] != ri.host: + if ri.snat_namespace: + self.assertEqual(1, mock_snat_ns.call_count) + + @mock.patch.object(dvr_snat_ns.SnatNamespace, 'delete') + def test_ext_gw_updated_not_calling_snat_ns_delete_if_gw_port_host_none( + self, mock_snat_ns): + """Function to check the impact of snat_namespace object. + + This function specifically checks the impact of the snat + namespace object value on external_gateway_removed for deleting + snat_namespace when gw_port_host mismatches and when the + self.snat_namespace is None. + """ + router = l3_test_common.prepare_router_data(num_internal_ports=2) + ri = dvr_router.DvrEdgeRouter(mock.Mock(), + HOSTNAME, + router['id'], + router, + **self.ri_kwargs) + ri._create_snat_namespace() + interface_name, ex_gw_port = l3_test_common.prepare_ext_gw_test(self, + ri) + router['gw_port_host'] = '' + # Initialize the snat_namespace object to the None here to emulate + # an agent restart. + ri.snat_namespace = None + ri.external_gateway_updated(ex_gw_port, interface_name) + if router['gw_port_host'] != ri.host: + if ri.snat_namespace is None: + self.assertFalse(mock_snat_ns.called) + + @mock.patch.object(namespaces.Namespace, 'delete') + def test_snat_ns_delete_not_called_when_snat_namespace_does_not_exist( + self, mock_ns_del): + """Function to check the impact of snat_namespace object. + + This function specifically checks the impact of the snat + namespace object initialization without the actual creation + of snat_namespace. When deletes are issued to the snat + namespace based on the snat namespace object existence, it + should be checking for the valid namespace existence before + it tries to delete. + """ + router = l3_test_common.prepare_router_data(num_internal_ports=2) + ri = dvr_router.DvrEdgeRouter(mock.Mock(), + HOSTNAME, + router['id'], + router, + **self.ri_kwargs) + # Make sure we set a return value to emulate the non existence + # of the namespace. + self.mock_ip.netns.exists.return_value = False + self.assertTrue(ri.snat_namespace) + if ri.snat_namespace: + ri.snat_namespace.delete() + self.assertFalse(mock_ns_del.called) + def _test_ext_gw_updated_dvr_edge_router(self, host_match, snat_hosted_before=True): """ @@ -630,7 +719,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): ri._create_snat_namespace() snat_ns_name = ri.snat_namespace.name else: - self.assertIsNone(ri.snat_namespace) + self.assertIsNotNone(ri.snat_namespace) interface_name, ex_gw_port = l3_test_common.prepare_ext_gw_test(self, ri)