From: Ryan Moats Date: Mon, 30 Nov 2015 18:36:54 +0000 (-0600) Subject: Refactor router delete processing X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=300f73d489ce0131035bac59a1f12787e20bf3ab;p=openstack-build%2Fneutron-build.git 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 --- diff --git a/neutron/agent/l3/dvr_edge_ha_router.py b/neutron/agent/l3/dvr_edge_ha_router.py index d0844ca75..377d51e4c 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 2a4336364..563c88a92 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 f78968cdf..781085afa 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 6d54ddfab..3e4389bf9 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 86d6afcbe..8cc6b7514 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 482c3f284..e7c5beb59 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)