Merge "Remove obsolete PID files before start" into stable/2023.1

Author: Zuul

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,