From ae50ceaeb04839b98e453f90f0a11b794d1ab875 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Thu, 23 Apr 2026 13:19:10 -0700 Subject: [PATCH] radicale, bepasty: Fix issue with failed diagnostic test Fixes: #2571. - During backup, a service related to backup is stopped and then started back again after the backup. In case of .socket unit, the .service unit is not being stopped and it continues to listen on the socket path. When the .socket unit is started back again, it tries to listen on the socket path and fails. - To fix the issue, when running stop, restart, etc. operations on a .socket file, try to perform that operations that we actually intend. Tests: - Unit tests pass - Functional tests for bepasty and radicale work. - After taking a radicale backup uwsgi-app@radicale.socket does not become inactive (works when service is running or stopped). uwsgi-app@radicale.service stops if it is running, backup doesn't fail if service is not running. - After taking a radicale backup uwsgi-app@bepasty-freedombox.socket does not become inactive (works when service is running or stopped). uwsgi-app@bepasty-freedombox.service stops if it is running, backup doesn't fail if service is not running. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/action_utils.py | 60 ++++++++++++++---- plinth/tests/test_action_utils.py | 100 +++++++++++++++++++++++++++--- 2 files changed, 139 insertions(+), 21 deletions(-) diff --git a/plinth/action_utils.py b/plinth/action_utils.py index 8d50836dc..8d9c91196 100644 --- a/plinth/action_utils.py +++ b/plinth/action_utils.py @@ -120,14 +120,6 @@ def service_disable(service_name: str, check: bool = False): except subprocess.CalledProcessError: pass - if service_name.endswith('.socket'): - # Instead, may need to query the unit for associated .service file. - base_name = service_name.rpartition('.')[0] - try: - service_stop(f'{base_name}.service', check=check) - except subprocess.CalledProcessError: - pass - def service_mask(service_name: str, check: bool = False): """Mask a service""" @@ -143,25 +135,62 @@ def service_start(service_name: str, check: bool = False): """Start a service with systemd.""" service_action(service_name, 'start', check=check) + # When starting a .socket unit, there is not need to start the .service + # unit as it will be automatically started when a request is received on + # the socket. + + +def _get_service_unit(socket_name: str) -> str: + """Return the .service unit name for a .socket unit.""" + # Instead, may need to query the unit for associated .service file. + base_name = socket_name.rpartition('.')[0] + return f'{base_name}.service' + def service_stop(service_name: str, check: bool = False): """Stop a service with systemd.""" service_action(service_name, 'stop', check=check) + # When stopping a .socket unit, most of the time, we must also stop + # .service unit. This frees up resources when disabling the app. It also + # stops using resources that are being backed up. + if service_name.endswith('.socket'): + service_action(_get_service_unit(service_name), 'stop', check=check) + def service_restart(service_name: str, check: bool = False): """Restart a service with systemd.""" - service_action(service_name, 'restart', check=check) + if not service_name.endswith('.socket'): + service_action(service_name, 'restart', check=check) + else: + # When restarting a .socket unit, most of the time, we actually want to + # restart the corresponding .service unit. This reloads the + # configuration changes as needed. To restart, all we need to do stop + # the service. It will be automatically started again by .socket unit. + service_action(_get_service_unit(service_name), 'stop', check=check) def service_try_restart(service_name: str, check: bool = False): """Try to restart a service with systemd.""" - service_action(service_name, 'try-restart', check=check) + if not service_name.endswith('.socket'): + service_action(service_name, 'try-restart', check=check) + else: + # When try-restarting a .socket unit, most of the time, we actually + # want to restart the corresponding .service unit. This reloads the + # configuration changes as needed. To restart, all we need to do stop + # the service. It will be automatically started again by .socket unit. + service_action(_get_service_unit(service_name), 'stop', check=check) def service_reload(service_name: str, check: bool = False): """Reload a service with systemd.""" - service_action(service_name, 'reload', check=check) + if not service_name.endswith('.socket'): + service_action(service_name, 'reload', check=check) + else: + # When reloading a .socket unit, most of the time, we actually want to + # reload the corresponding .service unit. This reloads the + # configuration changes as needed. + service_action(_get_service_unit(service_name), 'reload', check=check) def service_try_reload_or_restart(service_name: str, check: bool = False): @@ -169,7 +198,14 @@ def service_try_reload_or_restart(service_name: str, check: bool = False): Do nothing if service is not running. """ - service_action(service_name, 'try-reload-or-restart', check=check) + if not service_name.endswith('.socket'): + service_action(service_name, 'try-reload-or-restart', check=check) + else: + # When reloading a .socket unit, most of the time, we actually want to + # reload the corresponding .service unit. This reloads the + # configuration changes as needed. + service_action(_get_service_unit(service_name), + 'try-reload-or-restart', check=check) def service_reset_failed(service_name: str, check: bool = False): diff --git a/plinth/tests/test_action_utils.py b/plinth/tests/test_action_utils.py index 7e6a13c9d..e190a19ba 100644 --- a/plinth/tests/test_action_utils.py +++ b/plinth/tests/test_action_utils.py @@ -20,7 +20,8 @@ from plinth.action_utils import (get_addresses, get_hostname, service_try_reload_or_restart, service_try_restart, service_unmask, umask) -UNKNOWN = 'unknowndeamon' +UNKNOWN = 'unknowndeamon.service' +UNKNOWN_SOCKET = 'unknowndeamon.socket' systemctl_path = pathlib.Path('/usr/bin/systemctl') systemd_installed = pytest.mark.skipif(not systemctl_path.exists(), @@ -76,20 +77,101 @@ def test_service_enable_and_disable(): @patch('plinth.action_utils.service_action') @systemd_installed -def test_service_actions(mock): - """Trivial white box test for trivial implementations.""" +def test_service_start(mock): + """Test staring a service.""" service_start(UNKNOWN) - mock.assert_called_with(UNKNOWN, 'start', check=False) + mock.mock_calls = [call(UNKNOWN, 'start', check=False)] + + mock.reset_mock() + service_start(UNKNOWN, check=True) + mock.mock_calls = [call(UNKNOWN, 'start', check=True)] + + +@patch('plinth.action_utils.service_action') +@systemd_installed +def test_service_stop(mock): + """Test stopping a service.""" service_stop(UNKNOWN) - mock.assert_called_with(UNKNOWN, 'stop', check=False) + assert mock.mock_calls == [call(UNKNOWN, 'stop', check=False)] + + mock.reset_mock() + service_stop(UNKNOWN, check=True) + mock.mock_calls = [call(UNKNOWN, 'stop', check=True)] + + mock.reset_mock() + service_stop(UNKNOWN_SOCKET) + assert mock.mock_calls == [ + call(UNKNOWN_SOCKET, 'stop', check=False), + call(UNKNOWN, 'stop', check=False) + ] + + +@patch('plinth.action_utils.service_action') +@systemd_installed +def test_service_restart(mock): + """Test restaring a service.""" service_restart(UNKNOWN) - mock.assert_called_with(UNKNOWN, 'restart', check=False) + assert mock.mock_calls == [call(UNKNOWN, 'restart', check=False)] + + mock.reset_mock() + service_restart(UNKNOWN, check=True) + mock.mock_calls = [call(UNKNOWN, 'restart', check=True)] + + mock.reset_mock() + service_restart(UNKNOWN_SOCKET) + assert mock.mock_calls == [call(UNKNOWN, 'stop', check=False)] + + +@patch('plinth.action_utils.service_action') +@systemd_installed +def test_service_try_restart(mock): + """Test try-restaring a service.""" service_try_restart(UNKNOWN) - mock.assert_called_with(UNKNOWN, 'try-restart', check=False) + assert mock.mock_calls == [call(UNKNOWN, 'try-restart', check=False)] + + mock.reset_mock() + service_try_restart(UNKNOWN, check=True) + mock.mock_calls = [call(UNKNOWN, 'try-restart', check=True)] + + mock.reset_mock() + service_restart(UNKNOWN_SOCKET) + assert mock.mock_calls == [call(UNKNOWN, 'stop', check=False)] + + +@patch('plinth.action_utils.service_action') +@systemd_installed +def test_service_reload(mock): + """Test reloading a service.""" service_reload(UNKNOWN) - mock.assert_called_with(UNKNOWN, 'reload', check=False) + assert mock.mock_calls == [call(UNKNOWN, 'reload', check=False)] + + mock.reset_mock() + service_reload(UNKNOWN, check=True) + mock.mock_calls = [call(UNKNOWN, 'reload', check=True)] + + mock.reset_mock() + service_reload(UNKNOWN_SOCKET) + assert mock.mock_calls == [call(UNKNOWN, 'reload', check=False)] + + +@patch('plinth.action_utils.service_action') +@systemd_installed +def test_service_try_reload_or_restart(mock): + """Test try-reload-or-restart on a service.""" service_try_reload_or_restart(UNKNOWN) - mock.assert_called_with(UNKNOWN, 'try-reload-or-restart', check=False) + assert mock.mock_calls == [ + call(UNKNOWN, 'try-reload-or-restart', check=False) + ] + + mock.reset_mock() + service_try_reload_or_restart(UNKNOWN, check=True) + mock.mock_calls = [call(UNKNOWN, 'try-reload-or-restart', check=True)] + + mock.reset_mock() + service_try_reload_or_restart(UNKNOWN_SOCKET) + assert mock.mock_calls == [ + call(UNKNOWN, 'try-reload-or-restart', check=False) + ] @pytest.mark.usefixtures('needs_root')