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. + """ 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..4fb4d5736a 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 @@ -82,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: @@ -101,32 +115,42 @@ 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, - those that do not exist on the source system and those in masked-runtime state. + 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 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 +209,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) @@ -232,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 6964a65ba4..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 @@ -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,40 @@ 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_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", @@ -178,6 +216,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", ( @@ -263,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 == [] diff --git a/repos/system_upgrade/common/libraries/systemd.py b/repos/system_upgrade/common/libraries/systemd.py index c709f23328..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] @@ -237,6 +240,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..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', @@ -261,3 +268,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', {'template@.timer': 'disable'}), + ( + '.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 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