From 4fdd89e94f778a6aa96151679de2ddcd0165717a Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Fri, 4 Nov 2016 10:54:51 -0400 Subject: [PATCH] l3-ha: Send gratuitous ARP when new floating IP is added We rely on keepalived to send gratuitous ARPs when floating IP is added. Older versions of keepalived up to 1.2.20 (exclusive) contain bug [1] where keepalived does not send GARP on receiving SIGHUP. Unfortunately, newer versions containing the fix are not packaged yet for some distributions like RHEL or CentOS or Ubuntu Xenial, so this patch adds a workaround for such distributions until new packages are available. The patch also sets net.ipv4.ip_nonlocal_bind kernel parameter to 0 for Snat and HA router namespaces in order to avoid sending gratuitous ARPs for IP addresses that are not bound to the interface anymore - possibly because of failover or removal. Note that kernel < 3.19 contain a bug where this knob is missing. In case it attempts to set the parameter and it's missing on the system, it doesn't set the knob in root namespace like it's done for fip namespaces, but only issues a warning message. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1391553 Change-Id: Ieab53624dc34dc687a0e8eebd84778f7fc95dd77 Closes-bug: 1639315 --- neutron/agent/l3/dvr_fip_ns.py | 13 +-- neutron/agent/l3/dvr_snat_ns.py | 6 ++ neutron/agent/l3/ha_router.py | 19 +++++ neutron/agent/l3/keepalived_state_change.py | 29 ++++++- neutron/agent/l3/router_info.py | 7 +- neutron/agent/linux/ip_lib.py | 60 +++++++++++-- .../functional/agent/l3/test_dvr_router.py | 17 ++++ .../functional/agent/l3/test_ha_router.py | 15 ++++ .../agent/l3/test_keepalived_state_change.py | 85 +++++++++++++++++++ .../functional/agent/linux/test_ip_lib.py | 18 ++++ neutron/tests/unit/agent/linux/test_ip_lib.py | 8 ++ ...nding-garp-for-l3-ha-c118871833ad8743.yaml | 18 ++++ 12 files changed, 271 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/sending-garp-for-l3-ha-c118871833ad8743.yaml diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 51aaa97ebcf..86ea3b19cd5 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -138,21 +138,14 @@ class FipNamespace(namespaces.Namespace): # Somewhere in the 3.19 kernel timeframe ip_nonlocal_bind was # changed to be a per-namespace attribute. To be backwards # compatible we need to try both if at first we fail. - ip_wrapper = ip_lib.IPWrapper(namespace=self.name) try: - ip_wrapper.netns.execute(['sysctl', - '-w', - 'net.ipv4.ip_nonlocal_bind=1'], - log_fail_as_error=False, - run_as_root=True) + ip_lib.set_ip_nonlocal_bind( + value=1, namespace=self.name, log_fail_as_error=False) except RuntimeError: LOG.debug('DVR: fip namespace (%s) does not support setting ' 'net.ipv4.ip_nonlocal_bind, trying in root namespace', self.name) - self.ip_wrapper_root.netns.execute(['sysctl', - '-w', - 'net.ipv4.ip_nonlocal_bind=1'], - run_as_root=True) + ip_lib.set_ip_nonlocal_bind(value=1) # no connection tracking needed in fip namespace self._iptables_manager.ipv4['raw'].add_rule('PREROUTING', diff --git a/neutron/agent/l3/dvr_snat_ns.py b/neutron/agent/l3/dvr_snat_ns.py index 9639add690a..08a5903793b 100644 --- a/neutron/agent/l3/dvr_snat_ns.py +++ b/neutron/agent/l3/dvr_snat_ns.py @@ -29,6 +29,12 @@ class SnatNamespace(namespaces.Namespace): super(SnatNamespace, self).__init__( name, agent_conf, driver, use_ipv6) + def create(self): + super(SnatNamespace, self).create() + # This might be an HA router namespaces and it should not have + # ip_nonlocal_bind enabled + ip_lib.set_ip_nonlocal_bind_for_namespace(self.name) + @classmethod def get_snat_ns_name(cls, router_id): return namespaces.build_ns_name(SNAT_NS_PREFIX, router_id) diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index 5eab948fdb6..31659dcc584 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -20,6 +20,7 @@ from neutron_lib import constants as n_consts from oslo_log import log as logging from neutron._i18n import _LE +from neutron.agent.l3 import namespaces from neutron.agent.l3 import router_info as router from neutron.agent.linux import external_process from neutron.agent.linux import ip_lib @@ -32,6 +33,19 @@ HA_DEV_PREFIX = 'ha-' IP_MONITOR_PROCESS_SERVICE = 'ip_monitor' +class HaRouterNamespace(namespaces.RouterNamespace): + """Namespace for HA router. + + This namespace sets the ip_nonlocal_bind to 0 for HA router namespaces. + It does so to prevent sending gratuitous ARPs for interfaces that got VIP + removed in the middle of processing. + """ + def create(self): + super(HaRouterNamespace, self).create() + # HA router namespaces should not have ip_nonlocal_bind enabled + ip_lib.set_ip_nonlocal_bind_for_namespace(self.name) + + class HaRouter(router.RouterInfo): def __init__(self, state_change_callback, *args, **kwargs): super(HaRouter, self).__init__(*args, **kwargs) @@ -40,6 +54,11 @@ class HaRouter(router.RouterInfo): self.keepalived_manager = None self.state_change_callback = state_change_callback + def create_router_namespace_object( + self, router_id, agent_conf, iface_driver, use_ipv6): + return HaRouterNamespace( + router_id, agent_conf, iface_driver, use_ipv6) + @property def ha_priority(self): return self.router.get('priority', keepalived.HA_DEFAULT_PRIORITY) diff --git a/neutron/agent/l3/keepalived_state_change.py b/neutron/agent/l3/keepalived_state_change.py index 2d1500bdfab..d6a5eee83b3 100644 --- a/neutron/agent/l3/keepalived_state_change.py +++ b/neutron/agent/l3/keepalived_state_change.py @@ -16,6 +16,7 @@ import os import sys import httplib2 +import netaddr from oslo_config import cfg from oslo_log import log as logging import requests @@ -23,6 +24,7 @@ import requests from neutron._i18n import _, _LE from neutron.agent.l3 import ha from neutron.agent.linux import daemon +from neutron.agent.linux import ip_lib from neutron.agent.linux import ip_monitor from neutron.agent.linux import utils as agent_utils from neutron.common import config @@ -50,20 +52,21 @@ class MonitorDaemon(daemon.Daemon): self.conf_dir = conf_dir self.interface = interface self.cidr = cidr + self.monitor = None super(MonitorDaemon, self).__init__(pidfile, uuid=router_id, user=user, group=group) def run(self, run_as_root=False): - monitor = ip_monitor.IPMonitor(namespace=self.namespace, - run_as_root=run_as_root) - monitor.start() + self.monitor = ip_monitor.IPMonitor(namespace=self.namespace, + run_as_root=run_as_root) + self.monitor.start() # Only drop privileges if the process is currently running as root # (The run_as_root variable name here is unfortunate - It means to # use a root helper when the running process is NOT already running # as root if not run_as_root: super(MonitorDaemon, self).run() - for iterable in monitor: + for iterable in self.monitor: self.parse_and_handle_event(iterable) def parse_and_handle_event(self, iterable): @@ -73,6 +76,15 @@ class MonitorDaemon(daemon.Daemon): new_state = 'master' if event.added else 'backup' self.write_state_change(new_state) self.notify_agent(new_state) + elif event.interface != self.interface and event.added: + # Send GARPs for all new router interfaces. + # REVISIT(jlibosva): keepalived versions 1.2.19 and below + # contain bug where gratuitous ARPs are not sent on receiving + # SIGHUP signal. This is a workaround to this bug. keepalived + # has this issue fixed since 1.2.20 but the version is not + # packaged in some distributions (RHEL/CentOS/Ubuntu Xenial). + # Remove this code once new keepalived versions are available. + self.send_garp(event) except Exception: LOG.exception(_LE( 'Failed to process or handle event for line %s'), iterable) @@ -97,6 +109,15 @@ class MonitorDaemon(daemon.Daemon): LOG.debug('Notified agent router %s, state %s', self.router_id, state) + def send_garp(self, event): + """Send gratuitous ARP for given event.""" + ip_lib.send_ip_addr_adv_notif( + self.namespace, + event.interface, + str(netaddr.IPNetwork(event.cidr).ip), + log_exception=False + ) + def configure(conf): config.init(sys.argv[1:]) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index f432d5c2f45..7cc84e75d7a 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -57,7 +57,7 @@ class RouterInfo(object): # Invoke the setter for establishing initial SNAT action self.router = router self.use_ipv6 = use_ipv6 - ns = namespaces.RouterNamespace( + ns = self.create_router_namespace_object( router_id, agent_conf, interface_driver, use_ipv6) self.router_namespace = ns self.ns_name = ns.name @@ -94,6 +94,11 @@ class RouterInfo(object): self.router_namespace.create() + def create_router_namespace_object( + self, router_id, agent_conf, iface_driver, use_ipv6): + return namespaces.RouterNamespace( + router_id, agent_conf, iface_driver, use_ipv6) + @property def router(self): return self._router diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index dc61448fcb5..e4e311bba91 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -25,7 +25,7 @@ from oslo_log import log as logging from oslo_utils import excutils import six -from neutron._i18n import _, _LE +from neutron._i18n import _, _LE, _LW from neutron.agent.common import utils from neutron.common import exceptions as n_exc from neutron.common import utils as common_utils @@ -38,6 +38,7 @@ OPTS = [ help=_('Force ip_lib calls to use the root helper')), ] +IP_NONLOCAL_BIND = 'net.ipv4.ip_nonlocal_bind' LOOPBACK_DEVNAME = 'lo' GRE_TUNNEL_DEVICE_NAMES = ['gre0', 'gretap0'] @@ -1007,22 +1008,27 @@ def iproute_arg_supported(command, arg): return any(arg in line for line in stderr.split('\n')) -def _arping(ns_name, iface_name, address, count): +def _arping(ns_name, iface_name, address, count, log_exception): # Pass -w to set timeout to ensure exit if interface removed while running arping_cmd = ['arping', '-A', '-I', iface_name, '-c', count, '-w', 1.5 * count, address] try: ip_wrapper = IPWrapper(namespace=ns_name) ip_wrapper.netns.execute(arping_cmd, check_exit_code=True) - except Exception: - msg = _LE("Failed sending gratuitous ARP " - "to %(addr)s on %(iface)s in namespace %(ns)s") - LOG.exception(msg, {'addr': address, + except Exception as exc: + msg = _("Failed sending gratuitous ARP " + "to %(addr)s on %(iface)s in namespace %(ns)s: %(err)s") + logger_method = LOG.exception + if not log_exception: + logger_method = LOG.warning + logger_method(msg, {'addr': address, 'iface': iface_name, - 'ns': ns_name}) + 'ns': ns_name, + 'err': exc}) -def send_ip_addr_adv_notif(ns_name, iface_name, address, count=3): +def send_ip_addr_adv_notif( + ns_name, iface_name, address, count=3, log_exception=True): """Send advance notification of an IP address assignment. If the address is in the IPv4 family, send gratuitous ARP. @@ -1037,9 +1043,12 @@ def send_ip_addr_adv_notif(ns_name, iface_name, address, count=3): :param iface_name: Name of interface which GARPs are gonna be sent from. :param address: Advertised IP address. :param count: (Optional) How many GARPs are gonna be sent. Default is 3. + :param log_exception: (Optional) True if possible failures should be logged + on exception level. Otherwise they are logged on + WARNING level. Default is True. """ def arping(): - _arping(ns_name, iface_name, address, count) + _arping(ns_name, iface_name, address, count, log_exception) if count > 0 and netaddr.IPAddress(address).version == 4: eventlet.spawn_n(arping) @@ -1057,3 +1066,36 @@ def get_ip_version(ip_or_cidr): def get_ipv6_lladdr(mac_addr): return '%s/64' % netaddr.EUI(mac_addr).ipv6_link_local() + + +def get_ip_nonlocal_bind(namespace=None): + """Get kernel option value of ip_nonlocal_bind in given namespace.""" + cmd = ['sysctl', '-bn', IP_NONLOCAL_BIND] + ip_wrapper = IPWrapper(namespace) + return int(ip_wrapper.netns.execute(cmd, run_as_root=True)) + + +def set_ip_nonlocal_bind(value, namespace=None, log_fail_as_error=True): + """Set sysctl knob of ip_nonlocal_bind to given value.""" + cmd = ['sysctl', '-w', '%s=%d' % (IP_NONLOCAL_BIND, value)] + ip_wrapper = IPWrapper(namespace) + ip_wrapper.netns.execute( + cmd, run_as_root=True, log_fail_as_error=log_fail_as_error) + + +def set_ip_nonlocal_bind_for_namespace(namespace): + """Set ip_nonlocal_bind but don't raise exception on failure.""" + try: + set_ip_nonlocal_bind( + value=0, namespace=namespace, log_fail_as_error=False) + except RuntimeError as rte: + LOG.warning( + _LW("Setting %(knob)s=0 in namespace %(ns)s failed: %(err)s. It " + "will not be set to 0 in the root namespace in order to not " + "break DVR, which requires this value be set to 1. This " + "may introduce a race between moving a floating IP to a " + "different network node, and the peer side getting a " + "populated ARP cache for a given floating IP address."), + {'knob': IP_NONLOCAL_BIND, + 'ns': namespace, + 'err': rte}) diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index c2539c113ca..eb9c16c95e2 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -1078,3 +1078,20 @@ class TestDvrRouter(framework.L3AgentTestFramework): # external networks. SNAT will be used. Direct route will not work # here. src_machine.assert_no_ping(machine_diff_scope.ip) + + def test_dvr_snat_namespace_has_ip_nonlocal_bind_disabled(self): + self.agent.conf.agent_mode = 'dvr_snat' + router_info = self.generate_dvr_router_info( + enable_ha=True, enable_snat=True) + router = self.manage_router(self.agent, router_info) + try: + ip_nonlocal_bind_value = ip_lib.get_ip_nonlocal_bind( + router.snat_namespace.name) + except RuntimeError as rte: + stat_message = 'cannot stat /proc/sys/net/ipv4/ip_nonlocal_bind' + if stat_message in str(rte): + raise self.skipException( + "This kernel doesn't support %s in network namespaces." % ( + ip_lib.IP_NONLOCAL_BIND)) + raise + self.assertEqual(0, ip_nonlocal_bind_value) diff --git a/neutron/tests/functional/agent/l3/test_ha_router.py b/neutron/tests/functional/agent/l3/test_ha_router.py index 086e93f06a4..bca4c5fd913 100644 --- a/neutron/tests/functional/agent/l3/test_ha_router.py +++ b/neutron/tests/functional/agent/l3/test_ha_router.py @@ -286,6 +286,21 @@ class L3HATestCase(framework.L3AgentTestFramework): self.agent._process_updated_router(router1.router) common_utils.wait_until_true(lambda: router1.ha_state == 'master') + def test_ha_router_namespace_has_ip_nonlocal_bind_disabled(self): + router_info = self.generate_router_info(enable_ha=True) + router = self.manage_router(self.agent, router_info) + try: + ip_nonlocal_bind_value = ip_lib.get_ip_nonlocal_bind( + router.router_namespace.name) + except RuntimeError as rte: + stat_message = 'cannot stat /proc/sys/net/ipv4/ip_nonlocal_bind' + if stat_message in str(rte): + raise self.skipException( + "This kernel doesn't support %s in network namespaces." % ( + ip_lib.IP_NONLOCAL_BIND)) + raise + self.assertEqual(0, ip_nonlocal_bind_value) + class L3HATestFailover(framework.L3AgentTestFramework): diff --git a/neutron/tests/functional/agent/l3/test_keepalived_state_change.py b/neutron/tests/functional/agent/l3/test_keepalived_state_change.py index bc8a729dadc..62755bd3327 100644 --- a/neutron/tests/functional/agent/l3/test_keepalived_state_change.py +++ b/neutron/tests/functional/agent/l3/test_keepalived_state_change.py @@ -12,16 +12,46 @@ # License for the specific language governing permissions and limitations # under the License. +import functools import os +import re +import eventlet import mock +import netaddr from oslo_config import fixture as fixture_config from oslo_utils import uuidutils from neutron.agent.l3 import keepalived_state_change +from neutron.agent.linux import ip_lib +from neutron.common import utils +from neutron.conf.agent.l3 import config from neutron.conf.agent.l3 import keepalived as kd +from neutron.tests.common import machine_fixtures as mf +from neutron.tests.common import net_helpers from neutron.tests.functional import base +IPV4_NEIGH_REGEXP = re.compile( + r'(?P(\d{1,3}\.){3}\d{1,3}) ' + '.*(?P([0-9A-Fa-f]{2}:){5}([0-9A-Fa-f]){2}).*') + + +def get_arp_ip_mac_pairs(device_name, namespace): + """Generate (ip, mac) pairs from device's ip neigh. + + Each neigh entry has following format: + 192.168.0.1 lladdr fa:16:3e:01:ba:d3 STALE + """ + device = ip_lib.IPDevice(device_name, namespace) + for entry in device.neigh.show(ip_version=4).splitlines(): + match = IPV4_NEIGH_REGEXP.match(entry) + if match: + yield match.group('ip'), match.group('mac') + + +def has_expected_arp_entry(device_name, namespace, ip, mac): + return (ip, mac) in get_arp_ip_mac_pairs(device_name, namespace) + class TestKeepalivedStateChange(base.BaseSudoTestCase): def setUp(self): @@ -68,3 +98,58 @@ class TestKeepalivedStateChange(base.BaseSudoTestCase): with mock.patch.object( self.monitor, 'notify_agent', side_effect=Exception): self.monitor.parse_and_handle_event(self.line) + + +class TestMonitorDaemon(base.BaseSudoTestCase): + def setUp(self): + super(TestMonitorDaemon, self).setUp() + bridge = self.useFixture(net_helpers.OVSBridgeFixture()).bridge + self.machines = self.useFixture(mf.PeerMachines(bridge)) + self.router, self.peer = self.machines.machines[:2] + config.register_l3_agent_config_opts(config.OPTS) + + conf_dir = self.get_default_temp_dir().path + monitor = keepalived_state_change.MonitorDaemon( + self.get_temp_file_path('monitor.pid'), + uuidutils.generate_uuid(), + 1, + 2, + self.router.namespace, + conf_dir, + 'foo-iface', + self.machines.ip_cidr + ) + eventlet.spawn_n(monitor.run, run_as_root=True) + monitor_started = functools.partial( + lambda mon: mon.monitor is not None, monitor) + utils.wait_until_true(monitor_started) + self.addCleanup(monitor.monitor.stop) + + def test_new_fip_sends_garp(self): + next_ip_cidr = net_helpers.increment_ip_cidr(self.machines.ip_cidr, 2) + expected_ip = str(netaddr.IPNetwork(next_ip_cidr).ip) + # Create incomplete ARP entry + self.peer.assert_no_ping(expected_ip) + has_entry = has_expected_arp_entry( + self.peer.port.name, + self.peer.namespace, + expected_ip, + self.router.port.link.address) + self.assertFalse(has_entry) + self.router.port.addr.add(next_ip_cidr) + has_arp_entry_predicate = functools.partial( + has_expected_arp_entry, + self.peer.port.name, + self.peer.namespace, + expected_ip, + self.router.port.link.address, + ) + exc = RuntimeError( + "No ARP entry in %s namespace containing IP address %s and MAC " + "address %s" % ( + self.peer.namespace, + expected_ip, + self.router.port.link.address)) + utils.wait_until_true( + has_arp_entry_predicate, + exception=exc) diff --git a/neutron/tests/functional/agent/linux/test_ip_lib.py b/neutron/tests/functional/agent/linux/test_ip_lib.py index 11ad5761c28..5bdee9db725 100644 --- a/neutron/tests/functional/agent/linux/test_ip_lib.py +++ b/neutron/tests/functional/agent/linux/test_ip_lib.py @@ -215,3 +215,21 @@ class IpLibTestCase(IpLibTestFramework): self._check_for_device_name(namespace.ip_wrapper, dev_name, True) device.link.delete() self._check_for_device_name(namespace.ip_wrapper, dev_name, False) + + +class TestSetIpNonlocalBind(functional_base.BaseSudoTestCase): + def test_assigned_value(self): + namespace = self.useFixture(net_helpers.NamespaceFixture()) + for expected in (0, 1): + try: + ip_lib.set_ip_nonlocal_bind(expected, namespace.name) + except RuntimeError as rte: + stat_message = ( + 'cannot stat /proc/sys/net/ipv4/ip_nonlocal_bind') + if stat_message in str(rte): + raise self.skipException( + "This kernel doesn't support %s in network " + "namespaces." % ip_lib.IP_NONLOCAL_BIND) + raise + observed = ip_lib.get_ip_nonlocal_bind(namespace.name) + self.assertEqual(expected, observed) diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index ac96964351e..bfdadc72122 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -1378,3 +1378,11 @@ class TestAddNamespaceToCmd(base.BaseTestCase): def test_add_namespace_to_cmd_without_namespace(self): cmd = ['ping', '8.8.8.8'] self.assertEqual(cmd, ip_lib.add_namespace_to_cmd(cmd, None)) + + +class TestSetIpNonlocalBindForHaNamespace(base.BaseTestCase): + def test_setting_failure(self): + """Make sure message is formatted correctly.""" + with mock.patch.object( + ip_lib, 'set_ip_nonlocal_bind', side_effect=RuntimeError): + ip_lib.set_ip_nonlocal_bind_for_namespace('foo') diff --git a/releasenotes/notes/sending-garp-for-l3-ha-c118871833ad8743.yaml b/releasenotes/notes/sending-garp-for-l3-ha-c118871833ad8743.yaml new file mode 100644 index 00000000000..afcd67053b2 --- /dev/null +++ b/releasenotes/notes/sending-garp-for-l3-ha-c118871833ad8743.yaml @@ -0,0 +1,18 @@ +--- +issues: + - In kernels < 3.19 net.ipv4.ip_nonlocal_bind was not + a per-namespace kernel option. L3 HA sets this option + to zero to avoid sending gratuitous ARPs for IP addresses + that were removed while processing. If this happens then + gratuitous ARPs are going to be sent which might populate + ARP caches of peer machines with the wrong MAC address. +fixes: + - Versions of keepalived < 1.2.20 don't send gratuitous ARPs + when keepalived process receives SIGHUP signal. These + versions are not packaged in some Linux distributions like + RHEL, CentOS or Ubuntu Xenial. Not sending gratuitous ARPs + may lead to peer ARP caches containing wrong information + about floating IP addresses until the entry is invalidated. + Neutron now sends gratuitous ARPs for all new IP addresses + that appear on non-HA interfaces in router namespace which + simulates behavior of new versions of keepalived.