From 5850a8ffc6c6275e913a5922626295f219e31cf5 Mon Sep 17 00:00:00 2001 From: Roman Prilipskii Date: Wed, 27 May 2026 14:24:26 -0400 Subject: [PATCH 1/5] transitionsystemdservicesstates: enable new preset-enabled units The systemd service-state transition only re-applied source states to units that existed on the source system; units new on the target were dropped by _filter_irrelevant_services. Such units then relied solely on the package %systemd_post scriptlet to apply their vendor preset, which only runs on a fresh install (not when an existing package gains a new unit on upgrade), so preset-enabled units could end up disabled after the upgrade. Keep target-only units in the filter and, when a unit is absent on the source, honor the target vendor preset (enable if preset is "enable"), replicating what a fresh install would do. Guard _get_newly_enabled against the now-possible missing source entry. Co-Authored-By: Claude Opus 4.7 --- .../transitionsystemdservicesstates.py | 26 +++++-- .../test_transitionsystemdservicesstates.py | 73 ++++++++++++++++++- 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py index 53f53fb570..0086807593 100644 --- a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py +++ b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py @@ -17,12 +17,23 @@ def _get_desired_service_state(state_source, preset_source, preset_target): """ Get the desired service state on the target system - :param state_source: State on the source system + :param state_source: State on the source system, or None if the unit does + not exist on the source system (it is new on the target) :param preset_source: Preset on the source system :param preset_target: Preset on the target system - :return: The desired state on the target system + :return: The desired state on the target system, or None for no-op """ + if state_source is None: + # The unit is new on the target system (no equivalent on the source). + # The package upgrade scriptlets apply vendor presets only on a fresh + # install, not when an existing package merely gains a new unit (e.g. + # logrotate.timer on EL8->EL9). Replicate what a fresh install would do + # by honoring the target preset; leave non-enabled presets untouched. + if preset_target == "enable": + return "enabled" + return None + if state_source in ("disabled", "enabled-runtime"): if preset_source == "disable": return preset_target + "d" # use the default from target @@ -111,8 +122,10 @@ def _filter_irrelevant_services(services_source, services_target): """ Filter out irrelevant services - Irrelevant services are those that cannot be enabled/disabled, - those that do not exist on the source system and those in masked-runtime state. + Irrelevant services are those that cannot be enabled/disabled and those in + masked-runtime state. Services that do not exist on the source system are + kept: they are new on the target and the desired-state logic decides whether + to enable them based on the target preset. :return: Target system services without the irrelevant ones. :rtype: list @@ -124,9 +137,6 @@ def _filter_irrelevant_services(services_source, services_target): continue state_source = services_source.get(service.name) - if not state_source: - # The service doesn't exist on the source system - continue if state_source == "masked-runtime": # TODO(mmatuska): It's not possible to get the persistent @@ -185,7 +195,7 @@ def _report_kept_enabled(tasks): def _get_newly_enabled(services_source, desired_states): newly_enabled = [] for service, state in desired_states.items(): - state_source = services_source[service] + state_source = services_source.get(service) if state_source == "disabled" and state == "enabled": newly_enabled.append(service) diff --git a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py index 6964a65ba4..47bcbe8659 100644 --- a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py +++ b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py @@ -31,6 +31,10 @@ ["masked", "enable", "disable", "masked"], ["disabled", "enable", "enable", "disabled"], ["disabled", "enable", "disable", "disabled"], + # Unit absent on the source system (new on target): apply the target + # preset, replicating what a fresh package install would do. + [None, "disable", "enable", "enabled"], + [None, "disable", "disable", None], ), ) def test_get_desired_service_state( @@ -75,15 +79,15 @@ def test_filter_irrelevant_services_services_filtered(): "test3.service": "masked", "test4.service": "indirect", "test5.service": "indirect", - "test6.service": "indirect", + "test6.service": "masked-runtime", } services_target = [ - SystemdServiceFile(name="test1.service", state="enabled"), SystemdServiceFile(name="test2.service", state="masked"), SystemdServiceFile(name="test3.service", state="indirect"), SystemdServiceFile(name="test4.service", state="static"), SystemdServiceFile(name="test5.service", state="generated"), - SystemdServiceFile(name="test6.service", state="masked-runtime"), + # Enable-able target state, but masked-runtime on the source -> filtered + SystemdServiceFile(name="test6.service", state="enabled"), ] filtered = transitionsystemdservicesstates._filter_irrelevant_services( @@ -93,6 +97,23 @@ def test_filter_irrelevant_services_services_filtered(): assert not filtered +def test_filter_irrelevant_services_keeps_new_target_units(): + # Units absent on the source but in an enable/disable-able state are new on + # the target and must be kept so the target preset can be applied to them. + services_source = {} + services_target = [ + SystemdServiceFile(name="new-enabled.service", state="enabled"), + SystemdServiceFile(name="new-disabled.timer", state="disabled"), + SystemdServiceFile(name="new-static.service", state="static"), + ] + + filtered = transitionsystemdservicesstates._filter_irrelevant_services( + services_source, services_target + ) + + assert [s.name for s in filtered] == ["new-enabled.service", "new-disabled.timer"] + + def test_filter_irrelevant_services_services_not_filtered(): services_source = { "test1.service": "enabled", @@ -178,6 +199,52 @@ def test_tasks_produced_reports_created(monkeypatch): assert api.produce.model_instances[0].to_disable == expected_tasks.to_disable +def test_new_target_unit_enabled_by_preset(monkeypatch): + """ + A unit that does not exist on the source but is shipped enabled-by-preset on + the target (e.g. logrotate.timer on EL8->EL9) must be enabled, even though it + was left disabled by the package upgrade scriptlet. + """ + service_info_source = SystemdServicesInfoSource(service_files=[ + SystemdServiceFile(name="test.service", state="enabled"), + ]) + preset_info_source = SystemdServicesPresetInfoSource(presets=[ + SystemdServicePreset(service="test.service", state="enable"), + ]) + + services_target = [ + SystemdServiceFile(name="test.service", state="enabled"), + # New unit, left disabled by the upgrade (preset not applied on upgrade) + SystemdServiceFile(name="logrotate.timer", state="disabled"), + ] + service_info_target = SystemdServicesInfoTarget(service_files=services_target) + preset_info_target = SystemdServicesPresetInfoTarget(presets=[ + SystemdServicePreset(service="test.service", state="enable"), + SystemdServicePreset(service="logrotate.timer", state="enable"), + ]) + + monkeypatch.setattr( + api, + "current_actor", + CurrentActorMocked( + msgs=[ + service_info_source, + service_info_target, + preset_info_source, + preset_info_target, + ] + ), + ) + monkeypatch.setattr(api, "produce", produce_mocked()) + monkeypatch.setattr(reporting, "create_report", create_report_mocked()) + + transitionsystemdservicesstates.process() + + assert api.produce.called + assert api.produce.model_instances[0].to_enable == ["logrotate.timer"] + assert api.produce.model_instances[0].to_disable == [] + + @pytest.mark.parametrize( "tasks, expect_extended_summary", ( From 43e12863a38b03ffc2d355939e9c87bd751851a7 Mon Sep 17 00:00:00 2001 From: Roman Prilipskii Date: Fri, 7 Aug 2026 17:10:15 -0400 Subject: [PATCH 2/5] systemd: add get_system_unit_presets() for non-service unit types get_system_service_preset_files() filters preset entries down to '.service' units and returns SystemdServicePreset models. Vendor presets matter for other unit types too - timers in particular - and there is no model for those. Add a small companion that takes a unit-file suffix and returns a plain {unit: 'enable'|'disable'} mapping. It reuses the same preset discovery and parsing, so preset-file override and first-match-wins semantics stay identical. Pure addition: no existing caller changes behavior. Co-Authored-By: Claude Opus 5 --- .../common/libraries/systemd.py | 22 +++++++++++++ .../common/libraries/tests/test_systemd.py | 32 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/repos/system_upgrade/common/libraries/systemd.py b/repos/system_upgrade/common/libraries/systemd.py index c709f23328..0fdfd3b0b1 100644 --- a/repos/system_upgrade/common/libraries/systemd.py +++ b/repos/system_upgrade/common/libraries/systemd.py @@ -237,6 +237,28 @@ def _parse_preset_files(preset_files, load_path, ignore_invalid_entries): return presets +def get_system_unit_presets(suffix, ignore_invalid_entries=True): + """ + Get vendor preset states for units of a single unit type + + Unlike :func:`get_system_service_preset_files` this is not restricted to + '.service' units and returns a plain mapping instead of models, so it can + also be used for unit types that have no dedicated model (e.g. timers). + + :param suffix: Unit file suffix to filter on, including the dot, e.g. '.timer' + :param ignore_invalid_entries: Ignore invalid entries in preset files if True, raise ValueError otherwise + :return: Dictionary mapping unit names to their preset state ('enable' or 'disable') + :rtype: dict[str, str] + :raises: CalledProcessError: In case of errors when discovering systemd preset files + :raises: OSError: When the `find` command is not available + :raises: ValueError: When a preset file has invalid content and ignore_invalid_entries is False + """ + preset_files = _get_system_preset_files() + presets = _parse_preset_files(preset_files, SYSTEMD_SYSTEM_LOAD_PATH, ignore_invalid_entries) + + return dict((unit, state) for unit, state in presets.items() if unit.endswith(suffix)) + + def get_system_service_preset_files(service_files, ignore_invalid_entries=False): """ Get system preset files for services diff --git a/repos/system_upgrade/common/libraries/tests/test_systemd.py b/repos/system_upgrade/common/libraries/tests/test_systemd.py index a91fce1135..82f3a7b48e 100644 --- a/repos/system_upgrade/common/libraries/tests/test_systemd.py +++ b/repos/system_upgrade/common/libraries/tests/test_systemd.py @@ -261,3 +261,35 @@ def get_system_preset_files_mocked(): with pytest.raises(ValueError): # doesn't matter what service_files are systemd.get_system_service_preset_files([], ignore_invalid_entries=False) + + +@pytest.mark.parametrize( + 'suffix,expected', + [ + ('.socket', {'example.socket': 'disable'}), + ('.timer', {}), + ( + '.service', + { + 'example.service': 'enable', + 'abc.service': 'disable', + 'template@.service': 'disable', + 'template@instance1.service': 'enable', + 'template@instance2.service': 'enable', + 'globbed-one.service': 'enable', + 'globbed-two.service': 'enable', + 'extra.service': 'disable', + 'template2@.service': 'disable', + }, + ), + ] +) +def test_get_system_unit_presets(monkeypatch, suffix, expected): + + def get_system_preset_files_mocked(): + return TESTING_PRESET_FILES + + monkeypatch.setattr(systemd, '_get_system_preset_files', get_system_preset_files_mocked) + monkeypatch.setattr(systemd, '_parse_preset_files', parse_preset_files_mocked()) + + assert systemd.get_system_unit_presets(suffix) == expected From 8eb0c63903455267f74da08bb258553d2386a529 Mon Sep 17 00:00:00 2001 From: Roman Prilipskii Date: Fri, 7 Aug 2026 17:10:15 -0400 Subject: [PATCH 3/5] Re-enable systemd timers left disabled by a cron-to-timer migration When a package that already exists on the source system gains a systemd timer on the target, the timer ends up disabled after an in-place upgrade even though its vendor preset says 'enable': the %systemd_post scriptlet applies presets only on a fresh install ($1 -eq 1), and leapp's systemd state transition scans '--type=service' only, so non-service units are never transitioned. Two packages migrate a cron job to a preset-enabled timer across EL8->EL9, and both fail silently: - logrotate: /etc/cron.daily/logrotate -> logrotate.timer. Nothing rotates logs; they grow until the disk fills (ZD 284537, ~9.4 GB reclaimed). - mdadm: /etc/cron.d/raid-check -> raid-check.timer. The weekly software RAID consistency check never runs, so latent sector errors accumulate undetected and surface as an unrecoverable read error during an array rebuild. Rather than curate a list of affected timers, record the source system's timer inventory during the Facts phase and, on first boot, enable only timers that are absent from it, disabled, and preset-enabled. A timer that did not exist on the source cannot have been disabled by the administrator, so applying the target preset is safe by construction - the same rule the service-state transition uses for units new on the target. Timers present on the source keep their state, so a deliberate 'systemctl disable' always survives the upgrade. Without the source inventory the actor does nothing rather than guess. Validated CL8 -> CL9.8 on a no-panel VM with mdadm installed: logrotate.timer and raid-check.timer both enabled and active afterwards (next runs scheduled), while mdcheck_start/mdcheck_continue/mdmonitor-oneshot - present but disabled on the source - were correctly left untouched. Co-Authored-By: Claude Opus 5 --- .../actors/enablemigratedtimers/actor.py | 33 +++ .../libraries/enablemigratedtimers.py | 89 ++++++++ .../tests/test_enablemigratedtimers.py | 201 ++++++++++++++++++ .../actors/scansystemdtimerssource/actor.py | 36 ++++ .../libraries/scansystemdtimerssource.py | 23 ++ .../tests/test_scansystemdtimerssource.py | 65 ++++++ .../cloudlinux/models/systemdtimers.py | 23 ++ 7 files changed, 470 insertions(+) create mode 100644 repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/actor.py create mode 100644 repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/libraries/enablemigratedtimers.py create mode 100644 repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/tests/test_enablemigratedtimers.py create mode 100644 repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/actor.py create mode 100644 repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/libraries/scansystemdtimerssource.py create mode 100644 repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/tests/test_scansystemdtimerssource.py create mode 100644 repos/system_upgrade/cloudlinux/models/systemdtimers.py diff --git a/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/actor.py b/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/actor.py new file mode 100644 index 0000000000..7ec952d992 --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/actor.py @@ -0,0 +1,33 @@ +from leapp.actors import Actor +from leapp.libraries.actor import enablemigratedtimers +from leapp.libraries.common.cllaunch import run_on_cloudlinux +from leapp.models import SystemdTimersInfoSource +from leapp.reporting import Report +from leapp.tags import FirstBootPhaseTag, IPUWorkflowTag + + +class EnableMigratedTimers(Actor): + """ + Enable systemd timers that the upgrade left disabled despite a vendor preset. + + Some packages replace a cron job with a systemd timer across major versions. + logrotate is the canonical case (EL8 ships /etc/cron.daily/logrotate, EL9 + ships logrotate.timer with preset 'enable'), and mdadm does the same with + /etc/cron.d/raid-check and raid-check.timer. Because the package is upgraded + rather than freshly installed, its %systemd_post scriptlet does not apply the + timer's preset, and leapp's systemd state transition only covers '.service' + units. The timer is therefore left disabled, silently stopping its work - no + logs are rotated, no RAID consistency check is ever run. + + Only timers absent from the source system are considered, so a timer the + administrator disabled deliberately is never re-enabled. + """ + + name = 'enable_migrated_timers' + consumes = (SystemdTimersInfoSource,) + produces = (Report,) + tags = (FirstBootPhaseTag, IPUWorkflowTag) + + @run_on_cloudlinux + def process(self): + enablemigratedtimers.process() diff --git a/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/libraries/enablemigratedtimers.py b/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/libraries/enablemigratedtimers.py new file mode 100644 index 0000000000..66b1a89eb1 --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/libraries/enablemigratedtimers.py @@ -0,0 +1,89 @@ +from leapp import reporting +from leapp.libraries.common import systemd +from leapp.libraries.stdlib import api, CalledProcessError, run +from leapp.models import SystemdTimersInfoSource + +FMT_LIST_SEPARATOR = '\n - ' + +_LIST_TIMERS_CMD = ['systemctl', 'list-unit-files', '--type=timer', '--all', '--plain', '--no-legend'] + + +def _get_target_timer_states(): + """ + Get the state of every systemd timer unit file on the upgraded system. + + :return: Dictionary mapping timer unit names to their state + :rtype: dict[str, str] + """ + states = {} + for entry in run(_LIST_TIMERS_CMD, split=True)['stdout']: + columns = entry.split() + if len(columns) >= 2: + states[columns[0]] = columns[1] + return states + + +def _timers_to_enable(source_timers, target_states, presets): + """ + Pick the timers that the upgrade left disabled against their vendor preset. + + A timer qualifies only when it does not exist on the source system. Such a + timer was never visible to the administrator, so a 'disabled' state cannot + express an explicit choice - it can only be the upgrade failing to apply the + vendor preset. Timers that existed on the source keep whatever state the + regular systemd state transition gave them. + + :return: Sorted names of the timers to enable + :rtype: list[str] + """ + return sorted( + name for name, state in target_states.items() + if name not in source_timers and state == 'disabled' and presets.get(name) == 'enable' + ) + + +def process(): + source_info = next(api.consume(SystemdTimersInfoSource), None) + if source_info is None: + # Without the source inventory a new timer cannot be told apart from one + # the administrator disabled on purpose, so nothing may be touched. + api.current_logger().warning( + 'No SystemdTimersInfoSource message found, skipping the systemd timer check.' + ) + return + + source_timers = set(source_info.timers) + target_states = _get_target_timer_states() + presets = systemd.get_system_unit_presets('.timer') + + enabled = [] + for unit in _timers_to_enable(source_timers, target_states, presets): + try: + run(['systemctl', 'enable', '--now', unit]) + except CalledProcessError as err: + api.current_logger().warning( + 'Failed to enable systemd timer {}: {}'.format(unit, err) + ) + continue + enabled.append(unit) + + if not enabled: + return + + reporting.create_report([ + reporting.Title('Enabled systemd timers left disabled by the upgrade'), + reporting.Summary( + 'The following systemd timers are new on the upgraded system and are' + ' enabled by vendor preset, but were left disabled by the upgrade:' + ' the owning package already existed on the source system, so its' + ' scriptlet did not apply the preset, and leapp does not transition' + ' non-service units. Leapp has enabled and started them to restore' + ' the behavior of a freshly installed system (e.g. logrotate.timer' + ' rotates system logs and raid-check.timer runs the weekly software' + ' RAID consistency check; left disabled they fail silently):{}{}'.format( + FMT_LIST_SEPARATOR, FMT_LIST_SEPARATOR.join(enabled) + ) + ), + reporting.Severity(reporting.Severity.INFO), + reporting.Groups([reporting.Groups.POST, reporting.Groups.SERVICES]), + ]) diff --git a/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/tests/test_enablemigratedtimers.py b/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/tests/test_enablemigratedtimers.py new file mode 100644 index 0000000000..42d10758cf --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/enablemigratedtimers/tests/test_enablemigratedtimers.py @@ -0,0 +1,201 @@ +import pytest + +from leapp import reporting +from leapp.libraries.actor import enablemigratedtimers +from leapp.libraries.common.testutils import create_report_mocked, CurrentActorMocked, logger_mocked +from leapp.libraries.stdlib import api, CalledProcessError +from leapp.models import SystemdTimersInfoSource + + +class _RunMocked(object): + """ + Fake leapp `run` that answers `systemctl list-unit-files --type=timer` with + the configured target timer states and records every command issued. + `enable --now` can be made to fail. + """ + + def __init__(self, target_states=None, enable_raises=False): + self.target_states = target_states or {} + self.enable_raises = enable_raises + self.commands = [] + + def __call__(self, cmd, split=False, checked=True): + self.commands.append(cmd) + if cmd[:3] == ['systemctl', 'list-unit-files', '--type=timer']: + return {'stdout': ['{} {}'.format(n, s) for n, s in sorted(self.target_states.items())]} + if cmd[:3] == ['systemctl', 'enable', '--now']: + if self.enable_raises: + raise CalledProcessError('boom', cmd, {}) + return {'stdout': []} + return {'stdout': []} + + +def _setup(monkeypatch, run_mock, presets, source_timers=None, no_source_msg=False): + msgs = [] if no_source_msg else [SystemdTimersInfoSource(timers=source_timers or [])] + monkeypatch.setattr(api, 'current_actor', CurrentActorMocked(msgs=msgs)) + monkeypatch.setattr(api, 'current_logger', logger_mocked()) + monkeypatch.setattr(enablemigratedtimers, 'run', run_mock) + monkeypatch.setattr( + enablemigratedtimers.systemd, 'get_system_unit_presets', lambda suffix: presets + ) + reports = create_report_mocked() + monkeypatch.setattr(reporting, 'create_report', reports) + return reports + + +def _enabled_units(run_mock): + return [c[3] for c in run_mock.commands if c[:3] == ['systemctl', 'enable', '--now']] + + +def _reported_units(reports): + """ + Pull the listed units out of the report summary. + + The summary prose names logrotate.timer and raid-check.timer as examples, so + a plain substring check would pass regardless of what was actually enabled. + Only the bulleted list reflects the units this run touched. + """ + summary = reports.report_fields['summary'] + return [ + line.strip() for line in summary.split(enablemigratedtimers.FMT_LIST_SEPARATOR)[1:] + ] + + +def test_new_preset_enabled_timer_is_enabled_and_reported(monkeypatch): + run_mock = _RunMocked(target_states={'logrotate.timer': 'disabled'}) + reports = _setup(monkeypatch, run_mock, presets={'logrotate.timer': 'enable'}, source_timers=[]) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == ['logrotate.timer'] + assert reports.called == 1 + assert _reported_units(reports) == ['logrotate.timer'] + + +def test_raid_check_timer_is_enabled_alongside_logrotate(monkeypatch): + """ + mdadm migrates /etc/cron.d/raid-check to raid-check.timer on EL8->EL9 exactly + like logrotate does, so the weekly RAID consistency scrub silently stops. Both + must be re-enabled by a single run. + """ + run_mock = _RunMocked(target_states={ + 'logrotate.timer': 'disabled', + 'raid-check.timer': 'disabled', + }) + reports = _setup( + monkeypatch, + run_mock, + presets={'logrotate.timer': 'enable', 'raid-check.timer': 'enable'}, + source_timers=['dnf-makecache.timer'], + ) + + enablemigratedtimers.process() + + assert sorted(_enabled_units(run_mock)) == ['logrotate.timer', 'raid-check.timer'] + assert reports.called == 1 + assert _reported_units(reports) == ['logrotate.timer', 'raid-check.timer'] + + +def test_timer_present_on_source_is_left_alone(monkeypatch): + """ + A timer that existed on the source system may have been disabled deliberately + by the administrator, so its state must never be overridden. + """ + run_mock = _RunMocked(target_states={'dnf-makecache.timer': 'disabled'}) + reports = _setup( + monkeypatch, + run_mock, + presets={'dnf-makecache.timer': 'enable'}, + source_timers=['dnf-makecache.timer'], + ) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == [] + assert reports.called == 0 + + +@pytest.mark.parametrize('preset', ['disable', None]) +def test_new_timer_without_enable_preset_is_left_alone(monkeypatch, preset): + run_mock = _RunMocked(target_states={'rear.timer': 'disabled'}) + presets = {} if preset is None else {'rear.timer': preset} + reports = _setup(monkeypatch, run_mock, presets=presets, source_timers=[]) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == [] + assert reports.called == 0 + + +@pytest.mark.parametrize('state', ['enabled', 'static', 'masked', 'generated']) +def test_new_timer_not_in_disabled_state_is_left_alone(monkeypatch, state): + run_mock = _RunMocked(target_states={'logrotate.timer': state}) + reports = _setup(monkeypatch, run_mock, presets={'logrotate.timer': 'enable'}, source_timers=[]) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == [] + assert reports.called == 0 + + +def test_missing_source_message_is_a_no_op(monkeypatch): + """ + Without the source inventory there is no way to tell a new timer from one the + administrator disabled, so nothing may be touched. + """ + run_mock = _RunMocked(target_states={'logrotate.timer': 'disabled'}) + reports = _setup( + monkeypatch, run_mock, presets={'logrotate.timer': 'enable'}, no_source_msg=True + ) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == [] + assert reports.called == 0 + assert api.current_logger().warnmsg + + +def test_enable_failure_is_swallowed_and_not_reported(monkeypatch): + run_mock = _RunMocked(target_states={'logrotate.timer': 'disabled'}, enable_raises=True) + reports = _setup(monkeypatch, run_mock, presets={'logrotate.timer': 'enable'}, source_timers=[]) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == ['logrotate.timer'] + # enabling failed, so nothing should be reported as enabled + assert reports.called == 0 + + +def test_one_failure_does_not_block_the_other_timer(monkeypatch): + class _PartialFailRun(_RunMocked): + def __call__(self, cmd, split=False, checked=True): + if cmd[:3] == ['systemctl', 'enable', '--now'] and cmd[3] == 'logrotate.timer': + self.commands.append(cmd) + raise CalledProcessError('boom', cmd, {}) + return super(_PartialFailRun, self).__call__(cmd, split=split, checked=checked) + + run_mock = _PartialFailRun(target_states={ + 'logrotate.timer': 'disabled', + 'raid-check.timer': 'disabled', + }) + reports = _setup( + monkeypatch, + run_mock, + presets={'logrotate.timer': 'enable', 'raid-check.timer': 'enable'}, + source_timers=[], + ) + + enablemigratedtimers.process() + + assert reports.called == 1 + assert _reported_units(reports) == ['raid-check.timer'] + + +def test_no_timers_on_target_is_a_no_op(monkeypatch): + run_mock = _RunMocked(target_states={}) + reports = _setup(monkeypatch, run_mock, presets={}, source_timers=[]) + + enablemigratedtimers.process() + + assert _enabled_units(run_mock) == [] + assert reports.called == 0 diff --git a/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/actor.py b/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/actor.py new file mode 100644 index 0000000000..3ebf8d256a --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/actor.py @@ -0,0 +1,36 @@ +from leapp.actors import Actor +from leapp.exceptions import StopActorExecutionError +from leapp.libraries.actor import scansystemdtimerssource +from leapp.libraries.common.cllaunch import run_on_cloudlinux +from leapp.libraries.stdlib import CalledProcessError +from leapp.models import SystemdTimersInfoSource +from leapp.tags import FactsPhaseTag, IPUWorkflowTag + + +class ScanSystemdTimersSource(Actor): + """ + Provide the list of systemd timer unit files present on the source system. + + Leapp's own systemd scan is restricted to '.service' units, so timers are + recorded nowhere else. :class:`EnableMigratedTimers` needs this inventory on + the target system to tell a timer that is new on the target apart from one + that already existed on the source, whose state the administrator may have + chosen deliberately. + """ + + name = 'scan_systemd_timers_source' + consumes = () + produces = (SystemdTimersInfoSource,) + tags = (FactsPhaseTag, IPUWorkflowTag) + + @run_on_cloudlinux + def process(self): + try: + timers = scansystemdtimerssource.get_source_timers() + except CalledProcessError as err: + raise StopActorExecutionError( + message='Cannot obtain the list of systemd timer unit files.', + details={'details': str(err), 'stderr': err.stderr} + ) + + self.produce(SystemdTimersInfoSource(timers=timers)) diff --git a/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/libraries/scansystemdtimerssource.py b/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/libraries/scansystemdtimerssource.py new file mode 100644 index 0000000000..2e9eb66aae --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/libraries/scansystemdtimerssource.py @@ -0,0 +1,23 @@ +from leapp.libraries.stdlib import run + +_LIST_TIMERS_CMD = ['systemctl', 'list-unit-files', '--type=timer', '--all', '--plain', '--no-legend'] + + +def get_source_timers(): + """ + Get the names of all systemd timer unit files on the source system. + + Only the unit names are read: systemd 239 (EL8) prints just the name and the + state, while newer versions add a PRESET column, and the preset of the + source system is irrelevant for this purpose anyway. + + :return: Names of the timer unit files, including the '.timer' suffix + :rtype: list[str] + :raises: CalledProcessError: if the `systemctl` command fails + """ + timers = [] + for entry in run(_LIST_TIMERS_CMD, split=True)['stdout']: + columns = entry.split() + if columns: + timers.append(columns[0]) + return timers diff --git a/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/tests/test_scansystemdtimerssource.py b/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/tests/test_scansystemdtimerssource.py new file mode 100644 index 0000000000..9dac460680 --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/scansystemdtimerssource/tests/test_scansystemdtimerssource.py @@ -0,0 +1,65 @@ +import pytest + +from leapp.libraries.actor import scansystemdtimerssource +from leapp.libraries.stdlib import CalledProcessError + + +class _RunMocked(object): + """ + Fake leapp `run` answering `systemctl list-unit-files --type=timer ...`. + """ + + def __init__(self, stdout=None, raises=False): + self.stdout = stdout or [] + self.raises = raises + self.commands = [] + + def __call__(self, cmd, split=False): + self.commands.append(cmd) + if self.raises: + raise CalledProcessError('boom', cmd, {}) + return {'stdout': self.stdout} + + +def test_timer_names_are_parsed(monkeypatch): + run_mock = _RunMocked(stdout=[ + 'logrotate.timer enabled enabled', + 'dnf-makecache.timer enabled enabled', + 'mdadm-last-resort@.timer disabled disabled', + ]) + monkeypatch.setattr(scansystemdtimerssource, 'run', run_mock) + + assert scansystemdtimerssource.get_source_timers() == [ + 'logrotate.timer', 'dnf-makecache.timer', 'mdadm-last-resort@.timer' + ] + assert run_mock.commands[0][:3] == ['systemctl', 'list-unit-files', '--type=timer'] + + +def test_two_column_output_is_parsed(monkeypatch): + """ + systemd 239 (EL8) prints no PRESET column; only the name is needed here. + """ + run_mock = _RunMocked(stdout=['logrotate.timer enabled']) + monkeypatch.setattr(scansystemdtimerssource, 'run', run_mock) + + assert scansystemdtimerssource.get_source_timers() == ['logrotate.timer'] + + +def test_blank_lines_are_ignored(monkeypatch): + run_mock = _RunMocked(stdout=['logrotate.timer enabled', '', ' ']) + monkeypatch.setattr(scansystemdtimerssource, 'run', run_mock) + + assert scansystemdtimerssource.get_source_timers() == ['logrotate.timer'] + + +def test_no_timers_gives_empty_list(monkeypatch): + monkeypatch.setattr(scansystemdtimerssource, 'run', _RunMocked(stdout=[])) + + assert scansystemdtimerssource.get_source_timers() == [] + + +def test_systemctl_failure_propagates(monkeypatch): + monkeypatch.setattr(scansystemdtimerssource, 'run', _RunMocked(raises=True)) + + with pytest.raises(CalledProcessError): + scansystemdtimerssource.get_source_timers() diff --git a/repos/system_upgrade/cloudlinux/models/systemdtimers.py b/repos/system_upgrade/cloudlinux/models/systemdtimers.py new file mode 100644 index 0000000000..fb40e641ea --- /dev/null +++ b/repos/system_upgrade/cloudlinux/models/systemdtimers.py @@ -0,0 +1,23 @@ +from leapp.models import fields, Model +from leapp.topics import SystemInfoTopic + + +class SystemdTimersInfoSource(Model): + """ + Names of the systemd timer unit files present on the source system. + + Leapp's own systemd scan covers '.service' units only, so timers are not + represented anywhere else. This information is needed on the target system + to tell a timer that is new on the target (and therefore never seen, let + alone disabled, by the administrator) apart from one that already existed + on the source system, whose state must be left alone. + """ + + topic = SystemInfoTopic + + timers = fields.List(fields.String(), default=[]) + """ + Names of all installed systemd timer unit files, including the '.timer' + suffix. Template units are included; instances of templates are not, as + they have no unit file of their own. + """ From 3dd26a4530fc360f8cf870cfddc771d8548870f9 Mon Sep 17 00:00:00 2001 From: Roman Prilipskii Date: Tue, 11 Aug 2026 10:00:56 -0400 Subject: [PATCH 4/5] transitionsystemdservicesstates: keep ignoring the libvirt services The CL8+ libvirt exclusion works by removing the libvirtd/virt*d names from the source inventory, on the reasoning that "if a service is not present on the source system it's not handled either way". That reasoning depended on the absent-on-source guard removed in the preceding commit: with target-only units now retained and resolved against the target preset, a popped name reads as a unit new on the target, and the CL9 preset "enable virtqemud.service" puts it straight into to_enable. That reinstates the invalid monolithic/modular libvirt combination the exclusion exists to prevent. Distinguish "ignored" from "absent": _filter_ignored_services() now returns the names it excludes, and _filter_irrelevant_services() drops those target units as well. Also fix the comment that documented the old, now-wrong reasoning. Found in review of PR #69. Co-Authored-By: Claude Opus 5 --- .../transitionsystemdservicesstates.py | 34 ++++++-- .../test_transitionsystemdservicesstates.py | 87 ++++++++++++++++++- 2 files changed, 111 insertions(+), 10 deletions(-) diff --git a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py index 0086807593..4fb4d5736a 100644 --- a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py +++ b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/libraries/transitionsystemdservicesstates.py @@ -93,6 +93,9 @@ def _get_service_preset(service_name, presets): def _filter_ignored_services(services_source): """ Filter out services that should be ignored i.e. not handled + + :return: Names of the services that must not be handled + :rtype: set[str] """ to_ignore = [] if int(version.get_source_major_version()) >= 8: @@ -112,26 +115,37 @@ def _filter_ignored_services(services_source): ]) for s in to_ignore: - # It's sufficient to remove just from the source system services, - # because if a service is not present on the source system it's not handled either way + # Removing these from the source inventory is not enough on its own: an + # entry missing from it is otherwise taken to mean "new on the target", + # which applies the target preset. The names are returned so target + # filtering can drop them as well. if services_source.pop(s, None): api.current_logger().debug("Ignored service {} found on the source system".format(s)) + return set(to_ignore) + -def _filter_irrelevant_services(services_source, services_target): +def _filter_irrelevant_services(services_source, services_target, ignored_services=frozenset()): """ Filter out irrelevant services - Irrelevant services are those that cannot be enabled/disabled and those in - masked-runtime state. Services that do not exist on the source system are - kept: they are new on the target and the desired-state logic decides whether - to enable them based on the target preset. + Irrelevant services are those that cannot be enabled/disabled, those in + masked-runtime state, and those explicitly ignored. Services that do not + exist on the source system are kept: they are new on the target and the + desired-state logic decides whether to enable them based on the target + preset. + :param ignored_services: Names of services that must not be handled at all :return: Target system services without the irrelevant ones. :rtype: list """ filtered = [] for service in services_target: + if service.name in ignored_services: + # Excluded from handling; it is not a unit new on the target, even + # though it is absent from the source inventory. + continue + if service.state not in ("enabled", "disabled", "enabled-runtime"): # Enabling/disabling of services is only relevant to these states continue @@ -242,8 +256,10 @@ def process(): presets_source = {p.service: p.state for p in presets_source} presets_target = {p.service: p.state for p in presets_target} - _filter_ignored_services(services_source) - services_target = _filter_irrelevant_services(services_source, services_target) + ignored_services = _filter_ignored_services(services_source) + services_target = _filter_irrelevant_services( + services_source, services_target, ignored_services + ) desired_states = _get_desired_states( services_source, presets_source, services_target, presets_target diff --git a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py index 47bcbe8659..d67b8df2b8 100644 --- a/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py +++ b/repos/system_upgrade/common/actors/systemd/transitionsystemdservicesstates/tests/test_transitionsystemdservicesstates.py @@ -114,6 +114,23 @@ def test_filter_irrelevant_services_keeps_new_target_units(): assert [s.name for s in filtered] == ["new-enabled.service", "new-disabled.timer"] +def test_filter_irrelevant_services_drops_ignored_units(): + # Ignored units are removed from the source inventory, which must not make + # them look new on the target - otherwise the target preset gets applied to + # a unit that was deliberately excluded from handling. + services_source = {} + services_target = [ + SystemdServiceFile(name="virtqemud.service", state="disabled"), + SystemdServiceFile(name="new-disabled.timer", state="disabled"), + ] + + filtered = transitionsystemdservicesstates._filter_irrelevant_services( + services_source, services_target, ignored_services={"virtqemud.service"} + ) + + assert [s.name for s in filtered] == ["new-disabled.timer"] + + def test_filter_irrelevant_services_services_not_filtered(): services_source = { "test1.service": "enabled", @@ -330,10 +347,78 @@ def test_filter_ignored_services(monkeypatch, source_major_ver, expected): 'virtlogd.service': 'disabled', 'virtproxyd.service': 'masked', } + services_before = dict(services) monkeypatch.setattr( version, "get_source_major_version", lambda: source_major_ver, ) - transitionsystemdservicesstates._filter_ignored_services(services) + ignored = transitionsystemdservicesstates._filter_ignored_services(services) assert services == expected + + # The ignored names must be reported back, so target filtering can exclude + # them too instead of mistaking them for units new on the target. The set + # covers the whole libvirt group, not only the entries that happened to be + # present in the source inventory. + if int(source_major_ver) < 8: + assert ignored == set() + else: + assert { + 'libvirtd.service', + 'virtqemud.service', + 'virtlogd.service', + 'virtproxyd.service', + 'libvirt-guests.service', + }.issubset(ignored) + # everything dropped from the inventory is accounted for + assert not set(services_before) - set(services) - ignored + + +def test_ignored_libvirt_service_is_not_enabled_by_target_preset(monkeypatch): + """ + Regression test for the CL8+ libvirt exclusion. + + virtqemud.service is excluded from handling on 8->9+ by removing it from the + source inventory. It is present but disabled on the target, and the CL9 + vendor preset enables it, so it must still be left alone - enabling it would + reinstate the invalid monolithic/modular libvirt combination the exclusion + exists to prevent. + """ + service_info_source = SystemdServicesInfoSource(service_files=[ + SystemdServiceFile(name="virtqemud.service", state="disabled"), + SystemdServiceFile(name="test.service", state="enabled"), + ]) + preset_info_source = SystemdServicesPresetInfoSource(presets=[ + SystemdServicePreset(service="virtqemud.service", state="disable"), + SystemdServicePreset(service="test.service", state="enable"), + ]) + service_info_target = SystemdServicesInfoTarget(service_files=[ + SystemdServiceFile(name="virtqemud.service", state="disabled"), + SystemdServiceFile(name="test.service", state="enabled"), + ]) + preset_info_target = SystemdServicesPresetInfoTarget(presets=[ + SystemdServicePreset(service="virtqemud.service", state="enable"), + SystemdServicePreset(service="test.service", state="enable"), + ]) + + monkeypatch.setattr(version, "get_source_major_version", lambda: '8') + monkeypatch.setattr( + api, + "current_actor", + CurrentActorMocked( + msgs=[ + service_info_source, + service_info_target, + preset_info_source, + preset_info_target, + ] + ), + ) + monkeypatch.setattr(api, "produce", produce_mocked()) + monkeypatch.setattr(reporting, "create_report", create_report_mocked()) + + transitionsystemdservicesstates.process() + + assert api.produce.called + assert api.produce.model_instances[0].to_enable == [] + assert api.produce.model_instances[0].to_disable == [] From 7323dbb38f225c3c124af65f53786f3e568bf2b2 Mon Sep 17 00:00:00 2001 From: Roman Prilipskii Date: Tue, 11 Aug 2026 10:00:56 -0400 Subject: [PATCH 5/5] systemd: keep the unit type when expanding preset template instances _parse_preset_entry() hardcoded '.service' when expanding a preset entry that names template instances, so "enable backup@.timer daily" yielded backup@daily.service. Harmless while only service presets were consumed; now that presets are read per unit type, such an entry is attributed to the wrong type and dropped by the type filter. Derive the suffix from the matched template unit file instead. No EL8->EL9 preset currently declares template instances for a non-service unit, so this is a latent fix rather than a behavior change on any supported upgrade path. Found in review of PR #69. Co-Authored-By: Claude Opus 5 --- repos/system_upgrade/common/libraries/systemd.py | 9 ++++++--- .../common/libraries/tests/test_systemd.py | 9 ++++++++- .../libraries/tests/test_systemd_files/template@.timer | 0 3 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 repos/system_upgrade/common/libraries/tests/test_systemd_files/template@.timer diff --git a/repos/system_upgrade/common/libraries/systemd.py b/repos/system_upgrade/common/libraries/systemd.py index 0fdfd3b0b1..0d5e579f52 100644 --- a/repos/system_upgrade/common/libraries/systemd.py +++ b/repos/system_upgrade/common/libraries/systemd.py @@ -200,10 +200,13 @@ def _parse_preset_entry(entry, presets, load_path): # if the entry contains instance names after template unit name # the entry only applies to the specified instances, not to the # template itself + # The instance keeps the unit type of the template it comes from, + # which is not necessarily '.service' (e.g. 'backup@.timer'). + unit_type = os.path.splitext(unit_file)[1] for instance in columns[2:]: - service_name = unit_file[:unit_file.index('@') + 1] + instance + '.service' - if service_name not in presets: # first occurrence has priority - presets[service_name] = columns[0] + unit_name = unit_file[:unit_file.index('@') + 1] + instance + unit_type + if unit_name not in presets: # first occurrence has priority + presets[unit_name] = columns[0] elif unit_file not in presets: # first occurrence has priority presets[unit_file] = columns[0] diff --git a/repos/system_upgrade/common/libraries/tests/test_systemd.py b/repos/system_upgrade/common/libraries/tests/test_systemd.py index 82f3a7b48e..0ebf57f282 100644 --- a/repos/system_upgrade/common/libraries/tests/test_systemd.py +++ b/repos/system_upgrade/common/libraries/tests/test_systemd.py @@ -132,12 +132,18 @@ def readlink_mocked(path): 'template@instance1.service': 'disable', 'template@instance2.service': 'disable' }), + # Instances must keep the template's own unit type, not become '.service' + ('enable template@.timer daily weekly', { + 'template@daily.timer': 'enable', + 'template@weekly.timer': 'enable' + }), ('enable globbed*.service', {'globbed-one.service': 'enable', 'globbed-two.service': 'enable'}), ('enable example.*', {'example.service': 'enable', 'example.socket': 'enable'}), ('disable *', { 'example.service': 'disable', 'abc.service': 'disable', 'template@.service': 'disable', + 'template@.timer': 'disable', 'template2@.service': 'disable', 'globbed-one.service': 'disable', 'globbed-two.service': 'disable', @@ -175,6 +181,7 @@ def test_parse_preset_files(monkeypatch): 'example.socket': 'disable', 'abc.service': 'disable', 'template@.service': 'disable', + 'template@.timer': 'disable', 'template@instance1.service': 'enable', 'template@instance2.service': 'enable', 'globbed-one.service': 'enable', @@ -267,7 +274,7 @@ def get_system_preset_files_mocked(): 'suffix,expected', [ ('.socket', {'example.socket': 'disable'}), - ('.timer', {}), + ('.timer', {'template@.timer': 'disable'}), ( '.service', { diff --git a/repos/system_upgrade/common/libraries/tests/test_systemd_files/template@.timer b/repos/system_upgrade/common/libraries/tests/test_systemd_files/template@.timer new file mode 100644 index 0000000000..e69de29bb2