netns: ip netns exec <name> kill doesn't make sense

It seems confusing netns with pidns.
Although 'ip netns exec' doesn't make sense,
'ip netns exec <netns> kill <pid>' itself success as expected.
But as side effects, dentry of /proc/<pid>/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 <yamahata@valinux.co.jp>

diff --git a/quantum/agent/linux/dhcp.py b/quantum/agent/linux/dhcp.py
index 709d7da321c912e3f12ea0e870944064b1121d21..0d87039f1d6b8dd90c28518d7b70cb7073d0d4b1 100644 (file)
--- 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 874aba16159c212852dc007f46d691623272814b..12d9a038a800c2367796c55b97e67705f0cf112d 100644 (file)
--- 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 d262b2322498a42e48cb250e4a63d7fb2239f460..dc928d0844ecb4a6ad1dfd5c81037db4d294d338 100644 (file)
--- 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 b7cb98042ced108b3c0f2790f927fbfb8fd3deeb..d550a0325232beb7e095686038318671f9a40cab 100644 (file)
--- 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 a551c8feaca754eb50ba2fcef2c7009b323f01f7..c19b96334535f48d51628d49c42527a2b6720993 100644 (file)
--- 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 3a96f414d70e878488a87f0e0c14c6d3ad527ba3..4d9420268ba39173f1c80994455df9de54471819 100644 (file)
--- 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: