From 97f4a3fdbbe1a2b9dd9b09cc042094a4cd5177ca Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Tue, 11 Oct 2016 09:23:32 -0400 Subject: [PATCH] Move sysctl out of IPDevice class Not all callers use a device with sysctl calls, so move it out of the IPDevice class to make it more generic. This will help facilitate a future change to set accept_ra to fix another bug. Change-Id: Iaf85981a227234466863f00ee4e2209405a2b083 Related-bug: #1627902 --- neutron/agent/l3/dvr_fip_ns.py | 5 +- neutron/agent/l3/dvr_local_router.py | 12 +-- neutron/agent/linux/interface.py | 4 +- neutron/agent/linux/ip_lib.py | 90 ++++++++++--------- .../functional/agent/linux/test_ip_lib.py | 6 +- .../tests/unit/agent/linux/test_bridge_lib.py | 6 +- neutron/tests/unit/agent/linux/test_ip_lib.py | 3 +- 7 files changed, 64 insertions(+), 62 deletions(-) diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 86ea3b19cd5..350ad945666 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -138,10 +138,9 @@ 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. - try: - ip_lib.set_ip_nonlocal_bind( + failed = ip_lib.set_ip_nonlocal_bind( value=1, namespace=self.name, log_fail_as_error=False) - except RuntimeError: + if failed: LOG.debug('DVR: fip namespace (%s) does not support setting ' 'net.ipv4.ip_nonlocal_bind, trying in root namespace', self.name) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index f1b5eac2f32..b9f26f51f5a 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -320,11 +320,10 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): def _snat_redirect_modify(self, gateway, sn_port, sn_int, is_add): """Adds or removes rules and routes for SNAT redirection.""" + cmd = ['net.ipv4.conf.%s.send_redirects=0' % sn_int] try: ns_ipr = ip_lib.IPRule(namespace=self.ns_name) ns_ipd = ip_lib.IPDevice(sn_int, namespace=self.ns_name) - if is_add: - ns_ipwrapr = ip_lib.IPWrapper(namespace=self.ns_name) for port_fixed_ip in sn_port['fixed_ips']: # Iterate and find the gateway IP address matching # the IP version @@ -342,9 +341,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): ns_ipr.rule.add(ip=sn_port_cidr, table=snat_idx, priority=snat_idx) - ns_ipwrapr.netns.execute( - ['sysctl', '-w', - 'net.ipv4.conf.%s.send_redirects=0' % sn_int]) + ip_lib.sysctl(cmd, namespace=self.ns_name) else: self._delete_gateway_device_if_exists(ns_ipd, gw_ip_addr, @@ -426,9 +423,8 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): # TODO(Carl) Refactor external_gateway_added/updated/removed to use # super class implementation where possible. Looks like preserve_ips, # and ns_name are the key differences. - ip_wrapr = ip_lib.IPWrapper(namespace=self.ns_name) - ip_wrapr.netns.execute(['sysctl', '-w', - 'net.ipv4.conf.all.send_redirects=0']) + cmd = ['net.ipv4.conf.all.send_redirects=0'] + ip_lib.sysctl(cmd, namespace=self.ns_name) for p in self.internal_ports: gateway = self.get_snat_port_for_internal_port(p) id_name = self.get_internal_device_name(p['id']) diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py index ce4adb67355..41e8e0d17a6 100644 --- a/neutron/agent/linux/interface.py +++ b/neutron/agent/linux/interface.py @@ -216,8 +216,8 @@ class LinuxInterfaceDriver(object): def configure_ipv6_ra(namespace, dev_name): """Configure acceptance of IPv6 route advertisements on an intf.""" # Learn the default router's IP address via RAs - ip_lib.IPWrapper(namespace=namespace).netns.execute( - ['sysctl', '-w', 'net.ipv6.conf.%s.accept_ra=2' % dev_name]) + cmd = ['net.ipv6.conf.%s.accept_ra=2' % dev_name] + ip_lib.sysctl(cmd, namespace=namespace) @abc.abstractmethod def plug_new(self, network_id, port_id, device_name, mac_address, diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 2a091edadd0..e2d20e221bf 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -329,36 +329,10 @@ class IPDevice(SubProcessBase): LOG.exception(_LE("Failed deleting egress connection state of" " floatingip %s"), ip_str) - def _sysctl(self, cmd): - """execute() doesn't return the exit status of the command it runs, - it returns stdout and stderr. Setting check_exit_code=True will cause - it to raise a RuntimeError if the exit status of the command is - non-zero, which in sysctl's case is an error. So we're normalizing - that into zero (success) and one (failure) here to mimic what - "echo $?" in a shell would be. - - This is all because sysctl is too verbose and prints the value you - just set on success, unlike most other utilities that print nothing. - - execute() will have dumped a message to the logs with the actual - output on failure, so it's not lost, and we don't need to print it - here. - """ - cmd = ['sysctl', '-w'] + cmd - ip_wrapper = IPWrapper(self.namespace) - try: - ip_wrapper.netns.execute(cmd, run_as_root=True, - check_exit_code=True) - except RuntimeError: - LOG.exception(_LE("Failed running %s"), cmd) - return 1 - - return 0 - def disable_ipv6(self): sysctl_name = re.sub(r'\.', '/', self.name) - cmd = 'net.ipv6.conf.%s.disable_ipv6=1' % sysctl_name - return self._sysctl([cmd]) + cmd = ['net.ipv6.conf.%s.disable_ipv6=1' % sysctl_name] + return sysctl(cmd, namespace=self.namespace) @property def name(self): @@ -1066,6 +1040,43 @@ def send_ip_addr_adv_notif( eventlet.spawn_n(arping) +def sysctl(cmd, namespace=None, log_fail_as_error=True): + """Run sysctl command 'cmd' + + @param cmd: a list containing the sysctl command to run + @param namespace: network namespace to run command in + @param log_fail_as_error: failure logged as LOG.error + + execute() doesn't return the exit status of the command it runs, + it returns stdout and stderr. Setting check_exit_code=True will cause + it to raise a RuntimeError if the exit status of the command is + non-zero, which in sysctl's case is an error. So we're normalizing + that into zero (success) and one (failure) here to mimic what + "echo $?" in a shell would be. + + This is all because sysctl is too verbose and prints the value you + just set on success, unlike most other utilities that print nothing. + + execute() will have dumped a message to the logs with the actual + output on failure, so it's not lost, and we don't need to print it + here. + """ + cmd = ['sysctl', '-w'] + cmd + ip_wrapper = IPWrapper(namespace=namespace) + try: + ip_wrapper.netns.execute(cmd, run_as_root=True, + log_fail_as_error=log_fail_as_error) + except RuntimeError as rte: + LOG.warning( + _LW("Setting %(cmd)s in namespace %(ns)s failed: %(err)s."), + {'cmd': cmd, + 'ns': namespace, + 'err': rte}) + return 1 + + return 0 + + def add_namespace_to_cmd(cmd, namespace=None): """Add an optional namespace to the command.""" @@ -1089,25 +1100,20 @@ def get_ip_nonlocal_bind(namespace=None): 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) + cmd = ['%s=%d' % (IP_NONLOCAL_BIND, value)] + return sysctl(cmd, namespace=namespace, + 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: + failed = set_ip_nonlocal_bind(value=0, namespace=namespace, + log_fail_as_error=False) + if failed: 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 " + _LW("%s 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}) + IP_NONLOCAL_BIND) diff --git a/neutron/tests/functional/agent/linux/test_ip_lib.py b/neutron/tests/functional/agent/linux/test_ip_lib.py index 5bdee9db725..9c11b5fbee4 100644 --- a/neutron/tests/functional/agent/linux/test_ip_lib.py +++ b/neutron/tests/functional/agent/linux/test_ip_lib.py @@ -221,8 +221,9 @@ class TestSetIpNonlocalBind(functional_base.BaseSudoTestCase): def test_assigned_value(self): namespace = self.useFixture(net_helpers.NamespaceFixture()) for expected in (0, 1): + failed = ip_lib.set_ip_nonlocal_bind(expected, namespace.name) try: - ip_lib.set_ip_nonlocal_bind(expected, namespace.name) + observed = ip_lib.get_ip_nonlocal_bind(namespace.name) except RuntimeError as rte: stat_message = ( 'cannot stat /proc/sys/net/ipv4/ip_nonlocal_bind') @@ -231,5 +232,6 @@ class TestSetIpNonlocalBind(functional_base.BaseSudoTestCase): "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.assertFalse(failed) self.assertEqual(expected, observed) diff --git a/neutron/tests/unit/agent/linux/test_bridge_lib.py b/neutron/tests/unit/agent/linux/test_bridge_lib.py index 10b8d2cfe69..357abeb1c77 100644 --- a/neutron/tests/unit/agent/linux/test_bridge_lib.py +++ b/neutron/tests/unit/agent/linux/test_bridge_lib.py @@ -36,9 +36,9 @@ class BridgeLibTest(base.BaseTestCase): self.execute.assert_called_once_with(cmd, run_as_root=True) self.execute.reset_mock() - def _verify_bridge_mock_check_exit_code(self, cmd): + def _verify_bridge_sysctl_mock(self, cmd): self.execute.assert_called_once_with(cmd, run_as_root=True, - check_exit_code=True) + log_fail_as_error=True) self.execute.reset_mock() def test_is_bridged_interface(self): @@ -69,7 +69,7 @@ class BridgeLibTest(base.BaseTestCase): br.disable_ipv6() cmd = 'net.ipv6.conf.%s.disable_ipv6=1' % self._BR_NAME - self._verify_bridge_mock_check_exit_code(['sysctl', '-w', cmd]) + self._verify_bridge_sysctl_mock(['sysctl', '-w', cmd]) br.addif(self._IF_NAME) self._verify_bridge_mock( diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index 76b08cf4764..0ee6c18ec32 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -1424,6 +1424,5 @@ class TestAddNamespaceToCmd(base.BaseTestCase): 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): + with mock.patch.object(ip_lib, 'set_ip_nonlocal_bind', return_value=1): ip_lib.set_ip_nonlocal_bind_for_namespace('foo')