From 6b8bf55dac97e7a9c7521fd79a416e712b77ea5c Mon Sep 17 00:00:00 2001 From: Petr Stodulka Date: Wed, 18 Sep 2024 00:40:25 +0200 Subject: [PATCH 1/2] fix(pes_events_scanner): ensure output contains no duplicates RpmTransactionTasks messages have higher priority than instructions based on PES data. Previously, if multiple such messages existed with duplicate instructions, this could lead to the crash of the actor - especially in case when an existing package has been asked to be removed several times. Ensure the occurance of each instruction is unique (list -> set). jira: RHEL-50076 --- .../libraries/pes_events_scanner.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py b/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py index f5cb2613ff..a798017fcb 100644 --- a/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py +++ b/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py @@ -78,19 +78,22 @@ def get_installed_pkgs(): def get_transaction_configuration(): """ - Get pkgs to install, keep and remove from the user configuration files in /etc/leapp/transaction/. + Get pkgs to install, keep and remove from RpmTransactionTasks messages. - These configuration files have higher priority than PES data. - :return: RpmTransactionTasks model instance + Note these messages reflects inputs from various actors and configuration + files in /etc/leapp/transaction/. As these are explicit instruction, they + have higher priority than instructions from PES data. + + :return: TransactionConfiguration """ - transaction_configuration = TransactionConfiguration(to_install=[], to_remove=[], to_keep=[]) + transaction_configuration = TransactionConfiguration(to_install=set(), to_remove=set(), to_keep=set()) _Pkg = partial(Package, repository=None, modulestream=None) for tasks in api.consume(RpmTransactionTasks): - transaction_configuration.to_install.extend(_Pkg(name=pkg_name) for pkg_name in tasks.to_install) - transaction_configuration.to_remove.extend(_Pkg(name=pkg_name) for pkg_name in tasks.to_remove) - transaction_configuration.to_keep.extend(_Pkg(name=pkg_name) for pkg_name in tasks.to_keep) + transaction_configuration.to_install.update(_Pkg(name=pkg_name) for pkg_name in tasks.to_install) + transaction_configuration.to_remove.update(_Pkg(name=pkg_name) for pkg_name in tasks.to_remove) + transaction_configuration.to_keep.update(_Pkg(name=pkg_name) for pkg_name in tasks.to_keep) return transaction_configuration From 4a10ecc5689cabbee375ace7a0a03ccbf2334882 Mon Sep 17 00:00:00 2001 From: Michal Hecko Date: Thu, 31 Oct 2024 18:13:49 +0100 Subject: [PATCH 2/2] fix(pes_event_scanner): respect user's trasaction configuration Previously, pes_events_scanner used transaction configuration to only modify the way it initializes event application. As a consequence, if a user specified to_remove=['pkg'], then the information would not make it to pes_events_scanner's output. Similar situation would arise with to_install/to_keep. This patch adds a post-processing to explicitly add transaction configuration to the result of applying PES events. --- .../libraries/pes_events_scanner.py | 64 +++++++++++++-- .../tests/test_pes_event_scanner.py | 78 +++++++++++++++---- 2 files changed, 120 insertions(+), 22 deletions(-) diff --git a/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py b/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py index a798017fcb..5033615029 100644 --- a/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py +++ b/repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py @@ -470,9 +470,8 @@ def replace_pesids_with_repoids_in_packages(packages, source_pkgs_repoids): return packages_with_repoid.union(packages_without_pesid) -def apply_transaction_configuration(source_pkgs): +def apply_transaction_configuration(source_pkgs, transaction_configuration): source_pkgs_with_conf_applied = set(source_pkgs) - transaction_configuration = get_transaction_configuration() source_pkgs_with_conf_applied = source_pkgs.union(transaction_configuration.to_install) @@ -504,6 +503,50 @@ def remove_leapp_related_events(events): return res +def include_instructions_from_transaction_configuration(rpm_tasks, transaction_configuration, installed_pkgs): + """ + Extend current rpm_tasks applying data from transaction_configuration + + :param PESRpmTransactionTasks rpm_tasks: Currently calculated rpm tasks based on PES data. + :param TransactionConfiguration transaction_configuration: Tasked configured by user manually. + :param set(str) installed_pkgs: Set of distribution signed packages installed on the system. + :returns: updated tasks respecting configuration changes made by user + :rtype: PESRpmTransactionTasks + """ + to_install_from_rpm_tasks = set() if not rpm_tasks else set(rpm_tasks.to_install) + to_remove_from_rpm_tasks = set() if not rpm_tasks else set(rpm_tasks.to_remove) + to_keep_from_rpm_tasks = set() if not rpm_tasks else set(rpm_tasks.to_keep) + + # We don't want to try removing packages that are not installed - include only installed ones + installed_pkgs_requested_to_be_removed = transaction_configuration.to_remove.intersection(installed_pkgs) + pkgs_names_to_extend_to_remove_with = set(pkg.name for pkg in installed_pkgs_requested_to_be_removed) + + # Add packages to 'to_install' only if they are not already requested to be installed by rpm_tasks + pkgs_names_requested_to_be_installed = set(pkg.name for pkg in transaction_configuration.to_install) + to_install_pkgs_names_missing_from_tasks = pkgs_names_requested_to_be_installed - to_install_from_rpm_tasks + + pkg_names_user_wants_to_keep = {pkg.name for pkg in transaction_configuration.to_keep} + + # Remove packages that were requested by rpm_tasks or by user, but exclude those that should be kept + new_to_remove_set = (to_remove_from_rpm_tasks | pkgs_names_to_extend_to_remove_with) - pkg_names_user_wants_to_keep + new_to_remove_list = sorted(new_to_remove_set) + + new_to_install_list = sorted(to_install_from_rpm_tasks | to_install_pkgs_names_missing_from_tasks) + new_to_keep_list = sorted(to_keep_from_rpm_tasks | pkg_names_user_wants_to_keep) + + if not any((new_to_remove_list, new_to_keep_list, new_to_install_list)): # Are all empty? + return rpm_tasks # We do not modify the original tasks + + modules_to_enable = rpm_tasks.modules_to_enable if rpm_tasks else [] + modules_to_reset = rpm_tasks.modules_to_reset if rpm_tasks else [] + + return PESRpmTransactionTasks(to_install=new_to_install_list, + to_remove=new_to_remove_list, + to_keep=new_to_keep_list, + modules_to_enable=modules_to_enable, + modules_to_reset=modules_to_reset) + + def process(): # Retrieve data - installed_pkgs, transaction configuration, pes events events = get_pes_events('/etc/leapp/files', 'pes-events.json') @@ -511,24 +554,27 @@ def process(): return releases = get_relevant_releases(events) - source_pkgs = get_installed_pkgs() - source_pkgs = apply_transaction_configuration(source_pkgs) + installed_pkgs = get_installed_pkgs() + transaction_configuration = get_transaction_configuration() + pkgs_to_begin_computation_with = apply_transaction_configuration(installed_pkgs, transaction_configuration) # Keep track of what repoids have the source packages to be able to determine what are the PESIDs of the computed # packages of the target system, so we can distinguish what needs to be repomapped - repoids_of_source_pkgs = {pkg.repository for pkg in source_pkgs} + repoids_of_source_pkgs = {pkg.repository for pkg in pkgs_to_begin_computation_with} events = remove_leapp_related_events(events) events = remove_undesired_events(events, releases) # Apply events - compute what packages should the target system have - target_pkgs, pkgs_to_demodularize = compute_packages_on_target_system(source_pkgs, events, releases) + target_pkgs, pkgs_to_demodularize = compute_packages_on_target_system(pkgs_to_begin_computation_with, + events, releases) # Packages coming out of the events have PESID as their repository, however, we need real repoid target_pkgs = replace_pesids_with_repoids_in_packages(target_pkgs, repoids_of_source_pkgs) # Apply the desired repository blacklisting - blacklisted_repoids, target_pkgs = remove_new_packages_from_blacklisted_repos(source_pkgs, target_pkgs) + blacklisted_repoids, target_pkgs = remove_new_packages_from_blacklisted_repos(pkgs_to_begin_computation_with, + target_pkgs) # Look at the target packages and determine what repositories to enable target_repoids = sorted(set(p.repository for p in target_pkgs) - blacklisted_repoids - repoids_of_source_pkgs) @@ -536,6 +582,8 @@ def process(): api.produce(repos_to_enable) # Compare the packages on source system and the computed packages on target system and determine what to install - rpm_tasks = compute_rpm_tasks_from_pkg_set_diff(source_pkgs, target_pkgs, pkgs_to_demodularize) + rpm_tasks = compute_rpm_tasks_from_pkg_set_diff(pkgs_to_begin_computation_with, target_pkgs, pkgs_to_demodularize) + rpm_tasks = include_instructions_from_transaction_configuration(rpm_tasks, transaction_configuration, + installed_pkgs) if rpm_tasks: api.produce(rpm_tasks) diff --git a/repos/system_upgrade/common/actors/peseventsscanner/tests/test_pes_event_scanner.py b/repos/system_upgrade/common/actors/peseventsscanner/tests/test_pes_event_scanner.py index 80ece77065..9a499baaeb 100644 --- a/repos/system_upgrade/common/actors/peseventsscanner/tests/test_pes_event_scanner.py +++ b/repos/system_upgrade/common/actors/peseventsscanner/tests/test_pes_event_scanner.py @@ -9,9 +9,7 @@ api, compute_packages_on_target_system, compute_rpm_tasks_from_pkg_set_diff, - get_installed_pkgs, Package, - process, reporting, TransactionConfiguration ) @@ -27,8 +25,8 @@ RepositoriesSetupTasks, RepositoryData, RepositoryFile, - RHUIInfo, - RPM + RPM, + RpmTransactionTasks ) @@ -286,17 +284,14 @@ def test_actor_performs(monkeypatch): def test_transaction_configuration_has_effect(monkeypatch): _Pkg = partial(Package, repository=None, modulestream=None) - def mocked_transaction_conf(): - return TransactionConfiguration( - to_install=[_Pkg('pkg-a'), _Pkg('pkg-b')], - to_remove=[_Pkg('pkg-c'), _Pkg('pkg-d')], - to_keep=[] - ) - - monkeypatch.setattr(pes_events_scanner, 'get_transaction_configuration', mocked_transaction_conf) + transaction_cfg = TransactionConfiguration( + to_install=[_Pkg('pkg-a'), _Pkg('pkg-b')], + to_remove=[_Pkg('pkg-c'), _Pkg('pkg-d')], + to_keep=[] + ) packages = {_Pkg('pkg-a'), _Pkg('pkg-c')} - _result = pes_events_scanner.apply_transaction_configuration(packages) + _result = pes_events_scanner.apply_transaction_configuration(packages, transaction_cfg) result = {(p.name, p.repository, p.modulestream) for p in _result} expected = {('pkg-a', None, None), ('pkg-b', None, None)} @@ -340,7 +335,7 @@ def test_blacklisted_repoid_is_not_produced(monkeypatch): monkeypatch.setattr(pes_events_scanner, 'get_installed_pkgs', lambda: installed_pkgs) monkeypatch.setattr(pes_events_scanner, 'get_pes_events', lambda folder, filename: events) - monkeypatch.setattr(pes_events_scanner, 'apply_transaction_configuration', lambda pkgs: pkgs) + monkeypatch.setattr(pes_events_scanner, 'apply_transaction_configuration', lambda pkgs, transaction_cfg: pkgs) monkeypatch.setattr(pes_events_scanner, 'get_blacklisted_repoids', lambda: {'blacklisted-rhel8'}) monkeypatch.setattr(pes_events_scanner, 'replace_pesids_with_repoids_in_packages', lambda pkgs, src_pkgs_repoids: pkgs) @@ -475,3 +470,58 @@ def test_remove_leapp_related_events(monkeypatch): out_events = pes_events_scanner.remove_leapp_related_events(in_events) assert out_events == expected_out_events + + +def test_transaction_configuration_is_applied(monkeypatch): + installed_pkgs = { + Package(name='moved-in', repository='rhel7-base', modulestream=None), + Package(name='split-in', repository='rhel7-base', modulestream=None), + Package(name='pkg-not-in-events', repository='rhel7-base', modulestream=None), + } + monkeypatch.setattr(pes_events_scanner, 'get_installed_pkgs', lambda *args, **kwags: installed_pkgs) + + Pkg = partial(Package, modulestream=None) + events = [ + Event(1, Action.SPLIT, + {Pkg('split-in', 'rhel7-base')}, + {Pkg('split-out0', 'rhel8-BaseOS'), Pkg('split-out1', 'rhel8-BaseOS')}, + (7, 9), (8, 0), []), + Event(3, Action.MOVED, + {Pkg('moved-in', 'rhel7-base')}, {Pkg('moved-out', 'rhel8-BaseOS')}, + (7, 9), (8, 0), []), + ] + monkeypatch.setattr(pes_events_scanner, 'get_pes_events', lambda *args, **kwargs: events) + monkeypatch.setattr(pes_events_scanner, 'remove_leapp_related_events', lambda events: events) + monkeypatch.setattr(pes_events_scanner, 'remove_undesired_events', lambda events, releases: events) + monkeypatch.setattr(pes_events_scanner, '_get_enabled_modules', lambda *args: []) + monkeypatch.setattr(pes_events_scanner, 'replace_pesids_with_repoids_in_packages', + lambda target_pkgs, repoids_of_source_pkgs: target_pkgs) + monkeypatch.setattr(pes_events_scanner, + 'remove_new_packages_from_blacklisted_repos', + lambda source_pkgs, target_pkgs: (set(), target_pkgs)) + + msgs = [ + RpmTransactionTasks(to_remove=['pkg-not-in-events']), + RpmTransactionTasks(to_remove=['pkg-not-in-events', 'pkg-not-in-events']), + RpmTransactionTasks(to_install=['pkg-to-install']), + RpmTransactionTasks(to_keep=['keep-me']), + ] + mocked_actor = CurrentActorMocked(arch='x86_64', src_ver='7.9', dst_ver='8.8', msgs=msgs) + monkeypatch.setattr(api, 'current_actor', mocked_actor) + + monkeypatch.setattr(api, 'produce', produce_mocked()) + + pes_events_scanner.process() + + assert api.produce.called == 2 + + produced_rpm_transaction_tasks = [ + msg for msg in api.produce.model_instances if isinstance(msg, PESRpmTransactionTasks) + ] + + assert len(produced_rpm_transaction_tasks) == 1 + rpm_transaction_tasks = produced_rpm_transaction_tasks[0] + # It is important to see 'pkg-not-in-events' in the list - if the user says remove pkg A, we really remove it + assert sorted(rpm_transaction_tasks.to_remove) == ['moved-in', 'pkg-not-in-events', 'split-in'] + assert sorted(rpm_transaction_tasks.to_install) == ['moved-out', 'pkg-to-install', 'split-out0', 'split-out1'] + assert sorted(rpm_transaction_tasks.to_keep) == ['keep-me']