From 4d72093de571c2a12dc12c3761e3bcec11ccf20e Mon Sep 17 00:00:00 2001 From: Isaku Yamahata Date: Tue, 19 Mar 2013 06:56:15 +0900 Subject: [PATCH] netns: ip netns exec kill doesn't make sense It seems confusing netns with pidns. Although 'ip netns exec' doesn't make sense, 'ip netns exec kill ' itself success as expected. But as side effects, dentry of /proc//ns/net becomes young, which increases the possibility to fail to delete netns. That's not good. Fixes: Bug #1158589 Change-Id: I9aa717ccb86d8bf00bb1e707d39bfb65d043532b Signed-off-by: Isaku Yamahata --- quantum/agent/linux/dhcp.py | 14 ++----------- quantum/agent/linux/external_process.py | 7 +------ .../drivers/haproxy/namespace_driver.py | 9 ++++---- .../driver/haproxy/test_namespace_driver.py | 21 ++++++++++--------- quantum/tests/unit/test_linux_dhcp.py | 15 ++++++------- .../tests/unit/test_linux_external_process.py | 8 +++---- 6 files changed, 27 insertions(+), 47 deletions(-) diff --git a/quantum/agent/linux/dhcp.py b/quantum/agent/linux/dhcp.py index 709d7da32..0d87039f1 100644 --- a/quantum/agent/linux/dhcp.py +++ b/quantum/agent/linux/dhcp.py @@ -127,12 +127,7 @@ class DhcpLocalProcess(DhcpBase): if self.active: cmd = ['kill', '-9', pid] - if self.namespace: - ip_wrapper = ip_lib.IPWrapper(self.root_helper, self.namespace) - ip_wrapper.netns.execute(cmd) - else: - utils.execute(cmd, self.root_helper) - + utils.execute(cmd, self.root_helper) if not retain_port: self.device_delegate.destroy(self.network, self.interface_name) @@ -307,12 +302,7 @@ class Dnsmasq(DhcpLocalProcess): self._output_hosts_file() self._output_opts_file() cmd = ['kill', '-HUP', self.pid] - - if self.namespace: - ip_wrapper = ip_lib.IPWrapper(self.root_helper, self.namespace) - ip_wrapper.netns.execute(cmd) - else: - utils.execute(cmd, self.root_helper) + utils.execute(cmd, self.root_helper) LOG.debug(_('Reloading allocations for network: %s'), self.network.id) def _output_hosts_file(self): diff --git a/quantum/agent/linux/external_process.py b/quantum/agent/linux/external_process.py index 874aba161..12d9a038a 100644 --- a/quantum/agent/linux/external_process.py +++ b/quantum/agent/linux/external_process.py @@ -62,12 +62,7 @@ class ProcessManager(object): if self.active: cmd = ['kill', '-9', pid] - if self.namespace: - ip_wrapper = ip_lib.IPWrapper(self.root_helper, self.namespace) - ip_wrapper.netns.execute(cmd) - else: - utils.execute(cmd, self.root_helper) - + utils.execute(cmd, self.root_helper) elif pid: LOG.debug(_('Process for %(uuid)s pid %(pid)d is stale, ignoring ' 'command'), {'uuid': self.uuid, 'pid': pid}) diff --git a/quantum/plugins/services/agent_loadbalancer/drivers/haproxy/namespace_driver.py b/quantum/plugins/services/agent_loadbalancer/drivers/haproxy/namespace_driver.py index d262b2322..dc928d084 100644 --- a/quantum/plugins/services/agent_loadbalancer/drivers/haproxy/namespace_driver.py +++ b/quantum/plugins/services/agent_loadbalancer/drivers/haproxy/namespace_driver.py @@ -22,6 +22,7 @@ import socket import netaddr from quantum.agent.linux import ip_lib +from quantum.agent.linux import utils from quantum.common import exceptions from quantum.openstack.common import log as logging from quantum.plugins.services.agent_loadbalancer.drivers.haproxy import ( @@ -79,7 +80,7 @@ class HaproxyNSDriver(object): sock_path = self._get_state_file_path(pool_id, 'sock') # kill the process - kill_pids_in_file(ns, pid_path) + kill_pids_in_file(self.root_helper, pid_path) # unplug the ports if pool_id in self.pool_to_port_id: @@ -199,15 +200,13 @@ def get_ns_name(namespace_id): return NS_PREFIX + namespace_id -def kill_pids_in_file(namespace_wrapper, pid_path): +def kill_pids_in_file(root_helper, pid_path): if os.path.exists(pid_path): with open(pid_path, 'r') as pids: for pid in pids: pid = pid.strip() try: - namespace_wrapper.netns.execute( - ['kill', '-9', pid.strip()] - ) + utils.execute(['kill', '-9', pid], root_helper) except RuntimeError: LOG.exception( _('Unable to kill haproxy process: %s'), diff --git a/quantum/tests/unit/services/agent_loadbalancer/driver/haproxy/test_namespace_driver.py b/quantum/tests/unit/services/agent_loadbalancer/driver/haproxy/test_namespace_driver.py index b7cb98042..d550a0325 100644 --- a/quantum/tests/unit/services/agent_loadbalancer/driver/haproxy/test_namespace_driver.py +++ b/quantum/tests/unit/services/agent_loadbalancer/driver/haproxy/test_namespace_driver.py @@ -101,7 +101,7 @@ class TestHaproxyNSDriver(base.BaseTestCase): self.driver.destroy('pool_id') - kill.assert_called_once_with(ip_wrap(), '/pool/pid') + kill.assert_called_once_with('sudo', '/pool/pid') unplug.assert_called_once_with('qlbaas-pool_id', 'port_id') isdir.called_once_with('/pool') rmtree.assert_called_once_with('/pool') @@ -222,25 +222,26 @@ class TestHaproxyNSDriver(base.BaseTestCase): def test_kill_pids_in_file(self): with contextlib.nested( - mock.patch('os.path.exists'), - mock.patch('__builtin__.open') - ) as (path_exists, mock_open): + mock.patch('os.path.exists'), + mock.patch('__builtin__.open'), + mock.patch('quantum.agent.linux.utils.execute') + ) as (path_exists, mock_open, mock_execute): file_mock = mock.MagicMock() mock_open.return_value = file_mock file_mock.__enter__.return_value = file_mock file_mock.__iter__.return_value = iter(['123']) - ns_wrapper = mock.Mock() path_exists.return_value = False - namespace_driver.kill_pids_in_file(ns_wrapper, 'test_path') + namespace_driver.kill_pids_in_file('sudo', 'test_path') path_exists.assert_called_once_with('test_path') self.assertFalse(mock_open.called) + self.assertFalse(mock_execute.called) path_exists.return_value = True - ns_wrapper.netns.execute.side_effect = RuntimeError - namespace_driver.kill_pids_in_file(ns_wrapper, 'test_path') - ns_wrapper.netns.execute.assert_called_once_with(['kill', '-9', - '123']) + mock_execute.side_effect = RuntimeError + namespace_driver.kill_pids_in_file('sudo', 'test_path') + mock_execute.assert_called_once_with( + ['kill', '-9', '123'], 'sudo') def test_get_state_file_path(self): with mock.patch('os.makedirs') as mkdir: diff --git a/quantum/tests/unit/test_linux_dhcp.py b/quantum/tests/unit/test_linux_dhcp.py index a551c8fea..c19b96334 100644 --- a/quantum/tests/unit/test_linux_dhcp.py +++ b/quantum/tests/unit/test_linux_dhcp.py @@ -328,9 +328,8 @@ class TestDhcpLocalProcess(TestBase): lp.disable(retain_port=True) self.assertFalse(delegate.called) - exp_args = ['ip', 'netns', 'exec', 'qdhcp-ns', 'kill', '-9', 5] - self.execute.assert_called_once_with(exp_args, root_helper='sudo', - check_exit_code=True) + exp_args = ['kill', '-9', 5] + self.execute.assert_called_once_with(exp_args, 'sudo') def test_disable(self): attrs_to_mock = dict([(a, mock.DEFAULT) for a in @@ -346,9 +345,8 @@ class TestDhcpLocalProcess(TestBase): lp.disable() delegate.assert_has_calls([mock.call.destroy(network, 'tap0')]) - exp_args = ['ip', 'netns', 'exec', 'qdhcp-ns', 'kill', '-9', 5] - self.execute.assert_called_once_with(exp_args, root_helper='sudo', - check_exit_code=True) + exp_args = ['kill', '-9', 5] + self.execute.assert_called_once_with(exp_args, 'sudo') def test_pid(self): with mock.patch('__builtin__.open') as mock_open: @@ -538,7 +536,7 @@ tag:tag1,option:classless-static-route,%s,%s""".lstrip() % (fake_v6, fake_v6_cidr, fake_v6) - exp_args = ['ip', 'netns', 'exec', 'qdhcp-ns', 'kill', '-HUP', 5] + exp_args = ['kill', '-HUP', 5] with mock.patch('os.path.isdir') as isdir: isdir.return_value = True @@ -555,8 +553,7 @@ tag:tag1,option:classless-static-route,%s,%s""".lstrip() % (fake_v6, self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), mock.call(exp_opt_name, exp_opt_data)]) - self.execute.assert_called_once_with(exp_args, root_helper='sudo', - check_exit_code=True) + self.execute.assert_called_once_with(exp_args, 'sudo') def test_make_subnet_interface_ip_map(self): with mock.patch('quantum.agent.linux.ip_lib.IPDevice') as ip_dev: diff --git a/quantum/tests/unit/test_linux_external_process.py b/quantum/tests/unit/test_linux_external_process.py index 3a96f414d..4d9420268 100644 --- a/quantum/tests/unit/test_linux_external_process.py +++ b/quantum/tests/unit/test_linux_external_process.py @@ -95,12 +95,10 @@ class TestProcessManager(base.BaseTestCase): manager = ep.ProcessManager(self.conf, 'uuid', namespace='ns') - with mock.patch.object(ep, 'ip_lib') as ip_lib: + with mock.patch.object(ep, 'utils') as utils: manager.disable() - ip_lib.assert_has_calls([ - mock.call.IPWrapper('sudo', 'ns'), - mock.call.IPWrapper().netns.execute(['kill', '-9', 4])] - ) + utils.assert_has_calls( + mock.call.execute(['kill', '-9', 4], 'sudo')) def test_disable_not_active(self): with mock.patch.object(ep.ProcessManager, 'pid') as pid: -- 2.45.2