Refactor router delete processing

The discussion in [2] indicated that [1] would lead to orphaned
items during error cases.  This refactoring replaces the
optimistic approach followed by [1] with a separate delete code
path that does not execute the operations that take place within
the namespace that will be removed.  Operations that take place
outside of those namespaces are still performed to ensure that
no orphaned items result.

A comment has been added to the functional test to explain what
case is being tested.

[1] https://review.openstack.org/#/c/240971
[2] conversation starting at http://goo.gl/bZgvqW

Change-Id: I663f1264fb3963789b79a4a7c3e46d232b2f0620
Signed-off-by: Ryan Moats <rmoats@us.ibm.com>

diff --git a/neutron/agent/l3/dvr_edge_ha_router.py b/neutron/agent/l3/dvr_edge_ha_router.py
index d0844ca75facbe1ac186327837a236d5799b05ef..377d51e4c44094e3fbe6b50a5eb4238644d6d624 100644 (file)
--- a/neutron/agent/l3/dvr_edge_ha_router.py
+++ b/neutron/agent/l3/dvr_edge_ha_router.py
@@ -88,8 +88,8 @@ class DvrEdgeHaRouter(DvrEdgeRouter, HaRouter):
         self._create_snat_namespace()
         super(DvrEdgeHaRouter, self).initialize(process_monitor)
 
-    def process(self, agent, delete=False):
-        super(DvrEdgeHaRouter, self).process(agent, delete)
+    def process(self, agent):
+        super(DvrEdgeHaRouter, self).process(agent)
         if self.ha_port:
             self.enable_keepalived()
 

diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py
index 2a4336364c6d716e0a517f9ed1eb04724e665071..563c88a9220e1f96843d2a9ef757e05b88df16ab 100644 (file)
--- a/neutron/agent/l3/dvr_local_router.py
+++ b/neutron/agent/l3/dvr_local_router.py
@@ -414,11 +414,11 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
     def _handle_router_snat_rules(self, ex_gw_port, interface_name):
         pass
 
-    def process_external(self, agent, delete=False):
+    def process_external(self, agent):
         ex_gw_port = self.get_ex_gw_port()
         if ex_gw_port:
             self.create_dvr_fip_interfaces(ex_gw_port)
-        super(DvrLocalRouter, self).process_external(agent, delete)
+        super(DvrLocalRouter, self).process_external(agent)
 
     def create_dvr_fip_interfaces(self, ex_gw_port):
         floating_ips = self.get_floating_ips()
@@ -455,10 +455,10 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
                 # configured
                 self.agent.process_router_add(self)
 
-    def process(self, agent, delete=False):
+    def process(self, agent):
         ex_gw_port = self.get_ex_gw_port()
         if ex_gw_port:
             self.fip_ns = agent.get_fip_ns(ex_gw_port['network_id'])
             self.fip_ns.scan_fip_ports(self)
 
-        super(DvrLocalRouter, self).process(agent, delete)
+        super(DvrLocalRouter, self).process(agent)

diff --git a/neutron/agent/l3/dvr_router_base.py b/neutron/agent/l3/dvr_router_base.py
index f78968cdfd7ef1566625eff60166ccbb9ca959a6..781085afa8b48e83181e3488987f6156c6151b9f 100644 (file)
--- a/neutron/agent/l3/dvr_router_base.py
+++ b/neutron/agent/l3/dvr_router_base.py
@@ -26,8 +26,8 @@ class DvrRouterBase(router.RouterInfo):
         self.agent = agent
         self.host = host
 
-    def process(self, agent, delete=False):
-        super(DvrRouterBase, self).process(agent, delete)
+    def process(self, agent):
+        super(DvrRouterBase, self).process(agent)
         # NOTE:  Keep a copy of the interfaces around for when they are removed
         self.snat_ports = self.get_snat_interfaces()
 

diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py
index 6d54ddfab382832f44f3905628c56783292769ab..3e4389bf94623b3d5b246adf2edfa0dab9e8d28e 100644 (file)
--- a/neutron/agent/l3/ha_router.py
+++ b/neutron/agent/l3/ha_router.py
@@ -380,8 +380,8 @@ class HaRouter(router.RouterInfo):
         self.ha_network_removed()
         self.disable_keepalived()
 
-    def process(self, agent, delete=False):
-        super(HaRouter, self).process(agent, delete)
+    def process(self, agent):
+        super(HaRouter, self).process(agent)
 
         if self.ha_port:
             self.enable_keepalived()

diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py
index 86d6afcbeb8a655b895db33775d86dbcfc53ae51..8cc6b75145b8b0883e0013e18f00c78d5416bb7e 100644 (file)
--- a/neutron/agent/l3/router_info.py
+++ b/neutron/agent/l3/router_info.py
@@ -255,7 +255,7 @@ class RouterInfo(object):
         self.router['gw_port'] = None
         self.router[l3_constants.INTERFACE_KEY] = []
         self.router[l3_constants.FLOATINGIP_KEY] = []
-        self.process(agent, delete=True)
+        self.process_delete(agent)
         self.disable_radvd()
         self.router_namespace.delete()
 
@@ -646,35 +646,50 @@ class RouterInfo(object):
                              self.iptables_manager,
                              interface_name)
 
-    def process_external(self, agent, delete=False):
+    def _process_external_on_delete(self, agent):
         fip_statuses = {}
-        if not delete:
-            try:
-                with self.iptables_manager.defer_apply():
-                    ex_gw_port = self.get_ex_gw_port()
-                    self._process_external_gateway(ex_gw_port, agent.pd)
-                    if not ex_gw_port:
-                        return
-
-                    # Process SNAT/DNAT rules and addresses for floating IPs
-                    self.process_snat_dnat_for_fip()
-
-                # Once NAT rules for floating IPs are safely in place
-                # configure their addresses on the external gateway port
-                interface_name = self.get_external_device_interface_name(
-                    ex_gw_port)
-                fip_statuses = self.configure_fip_addresses(interface_name)
-
-            except (n_exc.FloatingIpSetupException,
-                    n_exc.IpTablesApplyException):
-                    # All floating IPs must be put in error state
-                    LOG.exception(_LE("Failed to process floating IPs."))
-                    fip_statuses = self.put_fips_in_error_state()
-            finally:
-                self.update_fip_statuses(agent, fip_statuses)
-        else:
+        try:
             ex_gw_port = self.get_ex_gw_port()
             self._process_external_gateway(ex_gw_port, agent.pd)
+            if not ex_gw_port:
+                return
+
+            interface_name = self.get_external_device_interface_name(
+                ex_gw_port)
+            fip_statuses = self.configure_fip_addresses(interface_name)
+
+        except (n_exc.FloatingIpSetupException):
+                # All floating IPs must be put in error state
+                LOG.exception(_LE("Failed to process floating IPs."))
+                fip_statuses = self.put_fips_in_error_state()
+        finally:
+            self.update_fip_statuses(agent, fip_statuses)
+
+    def process_external(self, agent):
+        fip_statuses = {}
+        try:
+            with self.iptables_manager.defer_apply():
+                ex_gw_port = self.get_ex_gw_port()
+                self._process_external_gateway(ex_gw_port, agent.pd)
+                if not ex_gw_port:
+                    return
+
+                # Process SNAT/DNAT rules and addresses for floating IPs
+                self.process_snat_dnat_for_fip()
+
+            # Once NAT rules for floating IPs are safely in place
+            # configure their addresses on the external gateway port
+            interface_name = self.get_external_device_interface_name(
+                ex_gw_port)
+            fip_statuses = self.configure_fip_addresses(interface_name)
+
+        except (n_exc.FloatingIpSetupException,
+                n_exc.IpTablesApplyException):
+                # All floating IPs must be put in error state
+                LOG.exception(_LE("Failed to process floating IPs."))
+                fip_statuses = self.put_fips_in_error_state()
+        finally:
+            self.update_fip_statuses(agent, fip_statuses)
 
     def update_fip_statuses(self, agent, fip_statuses):
         # Identify floating IPs which were disabled
@@ -693,19 +708,34 @@ class RouterInfo(object):
             agent.context, self.router_id, fip_statuses)
 
     @common_utils.exception_logger()
-    def process(self, agent, delete=False):
+    def process_delete(self, agent):
+        """Process the delete of this router
+
+        This method is the point where the agent requests that this router
+        be deleted. This is a separate code path from process in that it
+        avoids any changes to the qrouter namespace that will be removed
+        at the end of the operation.
+
+        :param agent: Passes the agent in order to send RPC messages.
+        """
+        LOG.debug("process router delete")
+        self._process_internal_ports(agent.pd)
+        agent.pd.sync_router(self.router['id'])
+        self._process_external_on_delete(agent)
+
+    @common_utils.exception_logger()
+    def process(self, agent):
         """Process updates to this router
 
         This method is the point where the agent requests that updates be
         applied to this router.
 
         :param agent: Passes the agent in order to send RPC messages.
-        :param delete: Indicates whether this update is from a delete operation
         """
         LOG.debug("process router updates")
         self._process_internal_ports(agent.pd)
         agent.pd.sync_router(self.router['id'])
-        self.process_external(agent, delete)
+        self.process_external(agent)
         # Process static routes for router
         self.routes_updated(self.routes, self.router['routes'])
         self.routes = self.router['routes']

diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py
index 482c3f2847f688efdb10912713f5292d54e7cda2..e7c5beb59bbd6126573abd752bab00ac7c3eab08 100644 (file)
--- a/neutron/tests/functional/agent/l3/test_dvr_router.py
+++ b/neutron/tests/functional/agent/l3/test_dvr_router.py
@@ -172,6 +172,13 @@ class TestDvrRouter(framework.L3AgentTestFramework):
             self._assert_onlink_subnet_routes(
                 router, ip_versions, snat_ns_name)
             self._assert_extra_routes(router, namespace=snat_ns_name)
+
+        # During normal operation, a router-gateway-clear followed by
+        # a router delete results in two notifications to the agent.  This
+        # code flow simulates the exceptional case where the notification of
+        # the clearing of the gateway hast been missed, so we are checking
+        # that the L3 agent is robust enough to handle that case and delete
+        # the router correctly.
         self._delete_router(self.agent, router.router_id)
         self._assert_fip_namespace_deleted(ext_gateway_port)
         self._assert_router_does_not_exist(router)