openstack/neutron
Code Issues Proposed changes

Refactor router delete processing

Browse Source 
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>
This commit is contained in:
Ryan Moats 2015-11-30 12:36:54 -06:00 committed by RYAN D. MOATS
parent 2c599814fb
commit 300f73d489
6 changed files with 77 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
neutron/agent/l3/dvr_edge_ha_router.py
View File

@ -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()

8
neutron/agent/l3/dvr_local_router.py
View File

@ -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)

4
neutron/agent/l3/dvr_router_base.py
View File

@ -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()

4
neutron/agent/l3/ha_router.py
View File

@ -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()

90
neutron/agent/l3/router_info.py
View File

@ -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']

7
neutron/tests/functional/agent/l3/test_dvr_router.py
View File

@ -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)