Refactor of floating ip processing in L3 Agent

Breaks down the processing of floating ip processing in 3 methods. A key goal
is to make the handling of ip tables application deferal more logically cohesive
in the process_router method

Partially-Implements: bp restructure-l3-agent

Change-Id: I7594c16a3ee3b9c988c7db856f9f401e4742f4c2

diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py
index b06752824417d29110851adc8bccdaadcf1e77aa..566cc1c61bf4ace20673c228ab36638b9d77a1ac 100644 (file)
--- a/neutron/agent/l3/agent.py
+++ b/neutron/agent/l3/agent.py
@@ -494,25 +494,30 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
             ri.perform_snat_action(self._handle_router_snat_rules,
                                    interface_name)
 
-    def _process_snat_dnat_for_fip(self, ri):
-        ex_gw_port = self._get_ex_gw_port(ri)
-        if not ex_gw_port:
-            return
+    def _put_fips_in_error_state(self, ri):
         fip_statuses = {}
+        for fip in ri.router.get(l3_constants.FLOATINGIP_KEY, []):
+            fip_statuses[fip['id']] = l3_constants.FLOATINGIP_STATUS_ERROR
+        return fip_statuses
+
+    def _process_snat_dnat_for_fip(self, ri):
         try:
-            existing_floating_ips = ri.floating_ips
             self.process_router_floating_ip_nat_rules(ri)
-            ri.iptables_manager.defer_apply_off()
-            # Once NAT rules for floating IPs are safely in place
-            # configure their addresses on the external gateway port
-            fip_statuses = self.process_router_floating_ip_addresses(
+        except Exception:
+            # TODO(salv-orlando): Less broad catching
+            raise n_exc.FloatingIpSetupException('L3 agent failure to setup '
+                'NAT for floating IPs')
+
+    def _configure_fip_addresses(self, ri, ex_gw_port):
+        try:
+            return self.process_router_floating_ip_addresses(
                 ri, ex_gw_port)
         except Exception:
             # TODO(salv-orlando): Less broad catching
-            # All floating IPs must be put in error state
-            for fip in ri.router.get(l3_constants.FLOATINGIP_KEY, []):
-                fip_statuses[fip['id']] = l3_constants.FLOATINGIP_STATUS_ERROR
+            raise n_exc.FloatingIpSetupException('L3 agent failure to setup '
+                'floating IPs')
 
+    def _update_fip_statuses(self, ri, existing_floating_ips, fip_statuses):
         # Identify floating IPs which were disabled
         ri.floating_ips = set(fip_statuses.keys())
         for fip_id in existing_floating_ips - ri.floating_ips:
@@ -528,22 +533,40 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
             else:
                 ri.disable_keepalived()
 
+    def _process_external(self, ri):
+        try:
+            with ri.iptables_manager.defer_apply():
+                self._process_external_gateway(ri)
+                ex_gw_port = self._get_ex_gw_port(ri)
+                # TODO(Carl) Return after setting existing_floating_ips and
+                # still call _update_fip_statuses?
+                if not ex_gw_port:
+                    return
+
+                # Process SNAT/DNAT rules and addresses for floating IPs
+                existing_floating_ips = ri.floating_ips
+                self._process_snat_dnat_for_fip(ri)
+
+            # Once NAT rules for floating IPs are safely in place
+            # configure their addresses on the external gateway port
+            fip_statuses = self._configure_fip_addresses(ri, ex_gw_port)
+
+        except (n_exc.FloatingIpSetupException, n_exc.IpTablesApplyException):
+                # All floating IPs must be put in error state
+                fip_statuses = self._put_fips_in_error_state(ri)
+
+        self._update_fip_statuses(ri, existing_floating_ips, fip_statuses)
+
     @common_utils.exception_logger()
     def process_router(self, ri):
         # TODO(mrsmith) - we shouldn't need to check here
         if 'distributed' not in ri.router:
             ri.router['distributed'] = False
-
-        ri.iptables_manager.defer_apply_on()
         self._process_internal_ports(ri)
-        self._process_external_gateway(ri)
-
+        self._process_external(ri)
         # Process static routes for router
         self.routes_updated(ri)
 
-        # Process SNAT/DNAT rules for floating IPs
-        self._process_snat_dnat_for_fip(ri)
-
         # Enable or disable keepalived for ha routers
         self._process_ha_router(ri)
 

diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index ce8dae3651d6bffb214f4fc7243b44f6726a5c69..5c04b5e64ab1760a1aebfa26f2326517513c9953 100644 (file)
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -18,6 +18,7 @@
 
 """Implements iptables rules using linux utilities."""
 
+import contextlib
 import os
 import re
 import sys
@@ -28,6 +29,7 @@ from oslo.utils import excutils
 from neutron.agent.common import config
 from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import utils as linux_utils
+from neutron.common import exceptions as n_exc
 from neutron.common import utils
 from neutron.i18n import _LE, _LW
 from neutron.openstack.common import lockutils
@@ -374,6 +376,19 @@ class IptablesManager(object):
     def is_chain_empty(self, table, chain, ip_version=4, wrap=True):
         return not self.get_chain(table, chain, ip_version, wrap)
 
+    @contextlib.contextmanager
+    def defer_apply(self):
+        """Defer apply context."""
+        self.defer_apply_on()
+        try:
+            yield
+        finally:
+            try:
+                self.defer_apply_off()
+            except Exception:
+                raise n_exc.IpTablesApplyException('Failure applying ip '
+                                                   'tables rules')
+
     def defer_apply_on(self):
         self.iptables_apply_deferred = True
 

diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py
index a629f26cd5a5e9b7086fd3cc8b7ff14c8b1f968c..a8d50c3e3c4f1b4a1f07e5d6ba87532507776d02 100644 (file)
--- a/neutron/common/exceptions.py
+++ b/neutron/common/exceptions.py
@@ -344,3 +344,15 @@ class RouterNotCompatibleWithAgent(NeutronException):
 class FailToDropPrivilegesExit(SystemExit):
     """Exit exception raised when a drop privileges action fails."""
     code = 99
+
+
+class FloatingIpSetupException(NeutronException):
+    def __init__(self, message=None):
+        self.message = message
+        super(FloatingIpSetupException, self).__init__()
+
+
+class IpTablesApplyException(NeutronException):
+    def __init__(self, message=None):
+        self.message = message
+        super(IpTablesApplyException, self).__init__()

diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py
index f5ca6e63a84b0e9cb8d54d21beec009a47553ef6..865072ca9d780776bad9a14b9e4f817f35247fb0 100644 (file)
--- a/neutron/tests/unit/test_l3_agent.py
+++ b/neutron/tests/unit/test_l3_agent.py
@@ -2139,3 +2139,40 @@ class TestBasicRouterOperations(base.BaseTestCase):
             asserter = self.assertIn if flag_set else self.assertNotIn
             asserter('AdvOtherConfigFlag on;',
                      self.utils_replace_file.call_args[0][1])
+
+    def test__put_fips_in_error_state(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        ri = mock.Mock()
+        ri.router.get.return_value = [{'id': mock.sentinel.id1},
+                                      {'id': mock.sentinel.id2}]
+
+        statuses = agent._put_fips_in_error_state(ri)
+
+        expected = [{mock.sentinel.id1: l3_constants.FLOATINGIP_STATUS_ERROR,
+                     mock.sentinel.id2: l3_constants.FLOATINGIP_STATUS_ERROR}]
+        self.assertNotEqual(expected, statuses)
+
+    def test__process_snat_dnat_for_fip(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        agent.process_router_floating_ip_nat_rules = mock.Mock(
+            side_effect=Exception)
+
+        self.assertRaises(n_exc.FloatingIpSetupException,
+                          agent._process_snat_dnat_for_fip,
+                          mock.sentinel.ri)
+
+        agent.process_router_floating_ip_nat_rules.assert_called_with(
+            mock.sentinel.ri)
+
+    def test__configure_fip_addresses(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        agent.process_router_floating_ip_addresses = mock.Mock(
+            side_effect=Exception)
+
+        self.assertRaises(n_exc.FloatingIpSetupException,
+                          agent._configure_fip_addresses,
+                          mock.sentinel.ri,
+                          mock.sentinel.ex_gw_port)
+
+        agent.process_router_floating_ip_addresses.assert_called_with(
+            mock.sentinel.ri, mock.sentinel.ex_gw_port)