Remove obsolete PID files before start

External processes, such as radvd, can refuse to start
and throw an exception such as:

  "Unable to convert value in $pidfile"

because the given pidfile has more than one PID in it.
The situation can happen when the neutron node is reset
and the obsolete PID files are not cleaned before neutron
is started.

This commit adds PID file cleanup before external
process start.

Closes-bug: #2033980
Change-Id: Id62bf18067d0b144c3e8825c7603cc1e51dca052
(cherry picked from commit c3b855a)
Signed-off-by: Sven Kieske <[email protected]>

Author: brianphaley

neutron/agent/linux/external_process.py

@@ -87,7 +87,19 @@ def enable(self, cmd_callback=None, reload_cfg=False, ensure_active=False):
         if not self.active:
             if not cmd_callback:
                 cmd_callback = self.default_cmd_callback
-            cmd = cmd_callback(self.get_pid_file_name())
+            # Always try and remove the pid file, as it's existence could
+            # stop the process from starting
+            pid_file = self.get_pid_file_name()
+            try:
+                utils.delete_if_exists(pid_file, run_as_root=self.run_as_root)
+            except Exception as e:
+                LOG.error("Could not delete file %(pid_file)s, %(service)s "
+                          "could fail to start. Exception: %(exc)s",
+                          {'pid_file': pid_file,
+                           'service': self.service,
+                           'exc': e})
+
+            cmd = cmd_callback(pid_file)
 
             ip_wrapper = ip_lib.IPWrapper(namespace=self.namespace)
             ip_wrapper.netns.execute(cmd, addl_env=self.cmd_addl_env,
@@ -99,12 +111,14 @@ def enable(self, cmd_callback=None, reload_cfg=False, ensure_active=False):
 
     def reload_cfg(self):
         if self.custom_reload_callback:
-            self.disable(get_stop_command=self.custom_reload_callback)
+            self.disable(get_stop_command=self.custom_reload_callback,
+                         delete_pid_file=False)
         else:
-            self.disable('HUP')
+            self.disable('HUP', delete_pid_file=False)
 
-    def disable(self, sig='9', get_stop_command=None):
+    def disable(self, sig='9', get_stop_command=None, delete_pid_file=True):
         pid = self.pid
+        delete_pid_file = delete_pid_file or sig == '9'
 
         if self.active:
             if get_stop_command:
@@ -118,10 +132,10 @@ def disable(self, sig='9', get_stop_command=None):
                 utils.execute(cmd, addl_env=self.cmd_addl_env,
                               run_as_root=self.run_as_root,
                               privsep_exec=True)
-                # In the case of shutting down, remove the pid file
-                if sig == '9':
-                    utils.delete_if_exists(self.get_pid_file_name(),
-                                           run_as_root=self.run_as_root)
+
+            if delete_pid_file:
+                utils.delete_if_exists(self.get_pid_file_name(),
+                                       run_as_root=self.run_as_root)
         elif pid:
             LOG.debug('%(service)s process for %(uuid)s pid %(pid)d is stale, '
                       'ignoring signal %(signal)s',

neutron/agent/linux/keepalived.py

@@ -427,15 +427,6 @@ def _output_config_file(self):
 
         return config_path
 
-    @staticmethod
-    def _safe_remove_pid_file(pid_file):
-        try:
-            os.remove(pid_file)
-        except OSError as e:
-            if e.errno != errno.ENOENT:
-                LOG.error("Could not delete file %s, keepalived can "
-                          "refuse to start.", pid_file)
-
     def get_vrrp_pid_file_name(self, base_pid_file):
         return '%s-vrrp' % base_pid_file
 
@@ -516,9 +507,6 @@ def callback(pid_file):
             if vrrp_pm.active:
                 vrrp_pm.disable()
 
-            self._safe_remove_pid_file(pid_file)
-            self._safe_remove_pid_file(self.get_vrrp_pid_file_name(pid_file))
-
             cmd = ['keepalived', '-P',
                    '-f', config_path,
                    '-p', pid_file,

neutron/agent/metadata/driver.py

@@ -288,9 +288,8 @@ def destroy_monitored_metadata_proxy(cls, monitor, uuid, conf, ns_name):
                         pm.pid, SIGTERM_TIMEOUT)
             pm.disable(sig=str(int(signal.SIGKILL)))
 
-        # Delete metadata proxy config and PID files.
+        # Delete metadata proxy config.
         HaproxyConfigurator.cleanup_config_file(uuid, cfg.CONF.state_path)
-        linux_utils.delete_if_exists(pm.get_pid_file_name(), run_as_root=True)
 
         cls.monitors.pop(uuid, None)
 

neutron/tests/unit/agent/l3/test_agent.py

@@ -3267,8 +3267,9 @@ def test_external_gateway_removed_ext_gw_port_no_fip_ns(self):
         self.assertFalse(ri.remove_floating_ip.called)
 
     @mock.patch.object(os, 'geteuid', return_value=mock.ANY)
+    @mock.patch.object(linux_utils, 'delete_if_exists')
     @mock.patch.object(pwd, 'getpwuid')
-    def test_spawn_radvd(self, mock_getpwuid, *args):
+    def test_spawn_radvd(self, mock_getpwuid, mock_delete, *args):
         router = l3_test_common.prepare_router_data(
             ip_version=lib_constants.IP_VERSION_6)
 
@@ -3304,6 +3305,8 @@ def test_spawn_radvd(self, mock_getpwuid, *args):
             self.conf.set_override('radvd_user', radvd_user)
             mock_getpwuid.return_value = FakeUser(os_user)
             radvd.enable(router['_interfaces'])
+            mock_delete.assert_called_once_with(pidfile, run_as_root=True)
+            mock_delete.reset_mock()
             cmd = execute.call_args[0][0]
             _join = lambda *args: ' '.join(args)
             cmd = _join(*cmd)

neutron/tests/unit/agent/l3/test_ha_router.py

@@ -35,6 +35,9 @@ def setUp(self):
         self.device_exists_p = mock.patch(
             'neutron.agent.linux.ip_lib.device_exists')
         self.device_exists = self.device_exists_p.start()
+        self.delete_if_exists_p = mock.patch(
+            'neutron.agent.linux.utils.delete_if_exists')
+        self.delete_if_exists = self.delete_if_exists_p.start()
 
     def _create_router(self, router=None, **kwargs):
         if not router:

neutron/tests/unit/agent/linux/test_external_process.py

@@ -120,9 +120,11 @@ def setUp(self):
         self.execute_p = mock.patch('neutron.agent.common.utils.execute')
         self.execute = self.execute_p.start()
         self.delete_if_exists = mock.patch(
-            'oslo_utils.fileutils.delete_if_exists').start()
+            'neutron.agent.linux.utils.delete_if_exists').start()
         self.ensure_dir = mock.patch.object(
             fileutils, 'ensure_tree').start()
+        self.error_log = mock.patch("neutron.agent.linux.external_process."
+                                    "LOG.error").start()
 
         self.conf = mock.Mock()
         self.conf.external_pids = '/var/path'
@@ -167,6 +169,8 @@ def test_enable_with_namespace(self):
                 with mock.patch.object(ep, 'ip_lib') as ip_lib:
                     manager.enable(callback)
                     callback.assert_called_once_with('pidfile')
+                    self.delete_if_exists.assert_called_once_with(
+                        'pidfile', run_as_root=True)
                     env = {ep.PROCESS_TAG: ep.DEFAULT_SERVICE_NAME + '-uuid'}
                     ip_lib.assert_has_calls([
                         mock.call.IPWrapper(namespace='ns'),
@@ -253,11 +257,34 @@ def _create_cmd(*args):
                           (service_name, uuid))
         self.assertEqual(expected_value, ret_value)
 
+    def test_enable_delete_pid_file_raises(self):
+        callback = mock.Mock()
+        cmd = ['the', 'cmd']
+        callback.return_value = cmd
+        self.delete_if_exists.side_effect = OSError
+
+        with mock.patch.object(ep.ProcessManager, 'get_pid_file_name') as name:
+            name.return_value = 'pidfile'
+            with mock.patch.object(ep.ProcessManager, 'active') as active:
+                active.__get__ = mock.Mock(return_value=False)
+
+                manager = ep.ProcessManager(self.conf, 'uuid')
+                manager.enable(callback)
+                callback.assert_called_once_with('pidfile')
+                cmd = ['env', DEFAULT_ENVVAR + '-uuid'] + cmd
+                self.execute.assert_called_once_with(cmd,
+                                                     check_exit_code=True,
+                                                     extra_ok_codes=None,
+                                                     run_as_root=False,
+                                                     log_fail_as_error=True,
+                                                     privsep_exec=False)
+                self.error_log.assert_called_once()
+
     def test_reload_cfg_without_custom_reload_callback(self):
         with mock.patch.object(ep.ProcessManager, 'disable') as disable:
             manager = ep.ProcessManager(self.conf, 'uuid', namespace='ns')
             manager.reload_cfg()
-            disable.assert_called_once_with('HUP')
+            disable.assert_called_once_with('HUP', delete_pid_file=False)
 
     def test_reload_cfg_with_custom_reload_callback(self):
         reload_callback = mock.sentinel.callback
@@ -266,7 +293,8 @@ def test_reload_cfg_with_custom_reload_callback(self):
                 self.conf, 'uuid', namespace='ns',
                 custom_reload_callback=reload_callback)
             manager.reload_cfg()
-            disable.assert_called_once_with(get_stop_command=reload_callback)
+            disable.assert_called_once_with(get_stop_command=reload_callback,
+                                            delete_pid_file=False)
 
     def test_disable_get_stop_command(self):
         cmd = ['the', 'cmd']

neutron/tests/unit/agent/metadata/test_driver.py

@@ -93,7 +93,7 @@ def setUp(self):
         mock.patch('neutron.agent.l3.ha.AgentMixin'
                    '._init_ha_conf_path').start()
         self.delete_if_exists = mock.patch.object(linux_utils,
-                                                  'delete_if_exists')
+                                                  'delete_if_exists').start()
         self.mock_get_process = mock.patch.object(
             metadata_driver.MetadataDriver,
             '_get_metadata_proxy_process_manager')
@@ -239,6 +239,9 @@ def _test_spawn_metadata_proxy(self, dad_failed=False):
                 router_id, metadata_driver.METADATA_SERVICE_NAME,
                 mock.ANY)
 
+            self.delete_if_exists.assert_called_once_with(
+                mock.ANY, run_as_root=True)
+
     def test_spawn_metadata_proxy(self):
         self._test_spawn_metadata_proxy()
 
@@ -270,23 +273,19 @@ def test_create_config_file_wrong_group(self):
                               config.create_config_file)
 
     def test_destroy_monitored_metadata_proxy(self):
-        delete_if_exists = self.delete_if_exists.start()
-        mproxy_process = mock.Mock(
-            active=False, get_pid_file_name=mock.Mock(return_value='pid_file'))
+        mproxy_process = mock.Mock(active=False)
         mock_get_process = self.mock_get_process.start()
         mock_get_process.return_value = mproxy_process
         driver = metadata_driver.MetadataDriver(FakeL3NATAgent())
         driver.destroy_monitored_metadata_proxy(mock.Mock(), 'uuid', 'conf',
                                                 'ns_name')
         mproxy_process.disable.assert_called_once_with(
             sig=str(int(signal.SIGTERM)))
-        delete_if_exists.assert_has_calls([
-            mock.call('pid_file', run_as_root=True)])
+        self.delete_if_exists.assert_called_once_with(
+            mock.ANY, run_as_root=True)
 
     def test_destroy_monitored_metadata_proxy_force(self):
-        delete_if_exists = self.delete_if_exists.start()
-        mproxy_process = mock.Mock(
-            active=True, get_pid_file_name=mock.Mock(return_value='pid_file'))
+        mproxy_process = mock.Mock(active=True)
         mock_get_process = self.mock_get_process.start()
         mock_get_process.return_value = mproxy_process
         driver = metadata_driver.MetadataDriver(FakeL3NATAgent())
@@ -296,5 +295,5 @@ def test_destroy_monitored_metadata_proxy_force(self):
         mproxy_process.disable.assert_has_calls([
             mock.call(sig=str(int(signal.SIGTERM))),
             mock.call(sig=str(int(signal.SIGKILL)))])
-        delete_if_exists.assert_has_calls([
-            mock.call('pid_file', run_as_root=True)])
+        self.delete_if_exists.assert_called_once_with(
+            mock.ANY, run_as_root=True)

neutron/tests/unit/agent/ovn/metadata/test_driver.py

@@ -21,6 +21,7 @@
 from oslo_utils import uuidutils
 
 from neutron.agent.linux import external_process as ep
+from neutron.agent.linux import utils as linux_utils
 from neutron.agent.ovn.metadata import agent as metadata_agent
 from neutron.agent.ovn.metadata import driver as metadata_driver
 from neutron.common import metadata as comm_meta
@@ -45,6 +46,8 @@ class TestMetadataDriverProcess(base.BaseTestCase):
     def setUp(self):
         super(TestMetadataDriverProcess, self).setUp()
         mock.patch('eventlet.spawn').start()
+        self.delete_if_exists = mock.patch.object(linux_utils,
+                                                  'delete_if_exists').start()
 
         ovn_meta_conf.register_meta_conf_opts(meta_conf.SHARED_OPTS, cfg.CONF)
         ovn_conf.register_opts()
@@ -117,6 +120,9 @@ def test_spawn_metadata_proxy(self):
                                           run_as_root=True)
             ])
 
+            self.delete_if_exists.assert_called_once_with(
+                mock.ANY, run_as_root=True)
+
     def test_create_config_file_wrong_user(self):
         with mock.patch('pwd.getpwnam', side_effect=KeyError):
             config = metadata_driver.HaproxyConfigurator(mock.ANY, mock.ANY,