From eaed05e02b0e96c588579a9fc5803ac9068bff17 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Thu, 6 Mar 2025 16:07:53 -0800 Subject: [PATCH] upgrades: Use bind mounts to edit sources file only upon completion - Save changes to /etc/apt/sources.list as a different file. - When launching the dist upgrade process via systemd-run, use bind mounting to ensure that the newly created sources file is treated as the original /etc/apt/sources.list. - If the process completes successfully, rename the new file to the original sources.list. If the process terminates abruptly or machine reboots, sources.list will remain unchanged. This will also the dist upgrade process to be restarted (and hopefully continued). Tests: - On a fresh stable container, running dist-upgrade succeeds. - While dist-upgrade is running, /etc/apt/sources.list is unmodified. After the operation is successfully completed, /etc/apt/sources.list has been updates successfully. If the operation fails, /etc/apt/sources.list remains unmodified. - During the run the following are run: - apt update - package holds - debconf selections - full-upgrade - autoremove - unattended-upgrades - restarting freedombox service - waiting 10 minutes - apt update Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/modules/upgrades/distupgrade.py | 50 +++++++++++++++---- plinth/modules/upgrades/privileged.py | 6 +++ .../upgrades/tests/test_distupgrade.py | 46 +++++++++++++---- 3 files changed, 83 insertions(+), 19 deletions(-) diff --git a/plinth/modules/upgrades/distupgrade.py b/plinth/modules/upgrades/distupgrade.py index ea5f65c5a..5d4f346d0 100644 --- a/plinth/modules/upgrades/distupgrade.py +++ b/plinth/modules/upgrades/distupgrade.py @@ -17,8 +17,6 @@ from . import utils logger = logging.getLogger(__name__) -SOURCES_LIST = '/etc/apt/sources.list' - OBSOLETE_PACKAGES: list[str] = [] PACKAGES_WITH_PROMPTS = ['firewalld', 'minidlna', 'radicale'] @@ -28,6 +26,9 @@ PRE_DEBCONF_SELECTIONS: list[str] = [ 'grub-pc grub-pc/install_devices_empty boolean true' ] +sources_list = pathlib.Path('/etc/apt/sources.list') +temp_sources_list = pathlib.Path('/etc/apt/sources.list.fbx-dist-upgrade') + def _apt_run(arguments): """Run an apt command and ensure that output is written to stdout.""" @@ -39,8 +40,9 @@ def _sources_list_update(old_codename: str, new_codename: str): logger.info('Upgrading from %s to %s...', old_codename, new_codename) aug = augeas.Augeas(flags=augeas.Augeas.NO_LOAD + augeas.Augeas.NO_MODL_AUTOLOAD) - aug.transform('aptsources', SOURCES_LIST) - aug.set('/augeas/context', '/files' + SOURCES_LIST) + aug.transform('aptsources', str(sources_list)) + aug.set('/augeas/context', '/files' + str(sources_list)) + aug.set('/augeas/save', 'newfile') # Save to a new file aug.load() for match_ in aug.match('*'): @@ -55,6 +57,9 @@ def _sources_list_update(old_codename: str, new_codename: str): aug.save() + aug_path = sources_list.with_suffix('.list.augnew') + aug_path.rename(temp_sources_list) + def _get_new_codename(test_upgrade: bool) -> str | None: """Return the codename for the next release.""" @@ -258,6 +263,21 @@ def _wait(): time.sleep(10 * 60) +def _trigger_on_complete(): + """Trigger the on complete step in a separate service.""" + # The dist-upgrade process will be run /etc/apt/sources.list file bind + # mounted on with a modified file. So, moving modified file to the original + # file will not be possible. For that, we need to launch a new process with + # a different systemd service (which does not have the bind mounts). + logger.info('Triggering on-complete to commit sources.lists') + subprocess.run([ + 'systemd-run', '--unit=freedombox-dist-upgrade-on-complete', + '--description=Finish up upgrade to new stable Debian release', + '/usr/share/plinth/actions/actions', 'upgrades', + 'dist_upgrade_on_complete', '--no-args' + ], check=True) + + def _logging_setup(): """Log to journal via console logging. @@ -287,27 +307,37 @@ def perform(): _freedombox_restart() _wait() _apt_update() + _trigger_on_complete() def start_service(test_upgrade: bool): """Create dist upgrade service and start it.""" - old_codename, new_codename = _check(test_upgrade) - - _sources_list_update(old_codename, new_codename) - + # Cleanup old service old_service_path = pathlib.Path( '/run/systemd/system/freedombox-dist-upgrade.service') if old_service_path.exists(): old_service_path.unlink(missing_ok=True) action_utils.service_daemon_reload() + old_codename, new_codename = _check(test_upgrade) + + _sources_list_update(old_codename, new_codename) + args = [ '--unit=freedombox-dist-upgrade', '--description=Upgrade to new stable Debian release', - '--property=KillMode=process', - '--property=TimeoutSec=12hr', + '--property=KillMode=process', '--property=TimeoutSec=12hr', + f'--property=BindPaths={temp_sources_list}:{sources_list}' ] subprocess.run(['systemd-run'] + args + [ 'systemd-inhibit', '/usr/share/plinth/actions/actions', 'upgrades', 'dist_upgrade', '--no-args' ], check=True) + + +def on_complete(): + """Perform cleanup operations.""" + _logging_setup() + logger.info('Dist upgrade complete.') + logger.info('Committing changes to /etc/apt/sources.list') + temp_sources_list.rename(sources_list) diff --git a/plinth/modules/upgrades/privileged.py b/plinth/modules/upgrades/privileged.py index 1b9de0016..63737c4f0 100644 --- a/plinth/modules/upgrades/privileged.py +++ b/plinth/modules/upgrades/privileged.py @@ -251,3 +251,9 @@ def start_dist_upgrade(test: bool = False): def dist_upgrade(): """Perform major distribution upgrade.""" distupgrade.perform() + + +@privileged +def dist_upgrade_on_complete(): + """Perform cleanup operations after distribution upgrade.""" + distupgrade.on_complete() diff --git a/plinth/modules/upgrades/tests/test_distupgrade.py b/plinth/modules/upgrades/tests/test_distupgrade.py index ca4c9b9aa..b1e2dedae 100644 --- a/plinth/modules/upgrades/tests/test_distupgrade.py +++ b/plinth/modules/upgrades/tests/test_distupgrade.py @@ -57,20 +57,21 @@ deb https://deb.debian.org/debian bookwormish main ''' # noqa: E501 sources_list = tmp_path / 'sources.list' - sources_list.write_text(original) - with patch('plinth.modules.upgrades.distupgrade.SOURCES_LIST', - str(sources_list)): + temp_sources_list = tmp_path / 'sources.list.fbx-dist-upgrade' + + module = 'plinth.modules.upgrades.distupgrade' + with patch(f'{module}.sources_list', sources_list), \ + patch(f'{module}.temp_sources_list', temp_sources_list): + sources_list.write_text(original) distupgrade._sources_list_update('bookworm', 'trixie') - assert sources_list.read_text() == modified + assert temp_sources_list.read_text() == modified - original = re.sub(r'bookworm([ -])', r'stable\1', original) - sources_list.write_text(original) - with patch('plinth.modules.upgrades.distupgrade.SOURCES_LIST', - str(sources_list)): + original = re.sub(r'bookworm([ -])', r'stable\1', original) + sources_list.write_text(original) distupgrade._sources_list_update('bookworm', 'trixie') - assert sources_list.read_text() == modified + assert temp_sources_list.read_text() == modified @patch('plinth.modules.upgrades.utils.get_http_protocol') @@ -320,3 +321,30 @@ def test_wait(sleep): """Test that sleeping works.""" distupgrade._wait() sleep.assert_called_with(600) + + +@patch('subprocess.run') +def test_trigger_on_complete(run): + """Test triggering post completion process.""" + distupgrade._trigger_on_complete() + run.assert_called_with([ + 'systemd-run', '--unit=freedombox-dist-upgrade-on-complete', + '--description=Finish up upgrade to new stable Debian release', + '/usr/share/plinth/actions/actions', 'upgrades', + 'dist_upgrade_on_complete', '--no-args' + ], check=True) + + +def test_on_complete(tmp_path): + """Test that /etc/apt/sources.list is committed.""" + sources_list = tmp_path / 'sources.list' + sources_list.write_text('before') + temp_sources_list = tmp_path / 'sources.list.fbx-dist-upgrade' + temp_sources_list.write_text('after') + + module = 'plinth.modules.upgrades.distupgrade' + with patch(f'{module}.sources_list', sources_list), \ + patch(f'{module}.temp_sources_list', temp_sources_list): + distupgrade.on_complete() + assert sources_list.read_text() == 'after' + assert not temp_sources_list.exists()