From 300f73d489ce0131035bac59a1f12787e20bf3ab Mon Sep 17 00:00:00 2001 From: Ryan Moats Date: Mon, 30 Nov 2015 12:36:54 -0600 Subject: [PATCH] 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 --- neutron/agent/l3/dvr_edge_ha_router.py | 4 +- neutron/agent/l3/dvr_local_router.py | 8 +- neutron/agent/l3/dvr_router_base.py | 4 +- neutron/agent/l3/ha_router.py | 4 +- neutron/agent/l3/router_info.py | 90 ++++++++++++------- .../functional/agent/l3/test_dvr_router.py | 7 ++ 6 files changed, 77 insertions(+), 40 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_ha_router.py b/neutron/agent/l3/dvr_edge_ha_router.py index d0844ca75fa..377d51e4c44 100644 --- 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 2a4336364c6..563c88a9220 100644 --- 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 f78968cdfd7..781085afa8b 100644 --- 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 6d54ddfab38..3e4389bf946 100644 --- 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 86d6afcbeb8..8cc6b75145b 100644 --- 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 482c3f2847f..e7c5beb59bb 100644 --- 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)