Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pes_events_scanner: fix processing of RpmTransactionTasks #1296

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

pes_events_scanner: fix processing of RpmTransactionTasks #1296

wants to merge 4 commits into from

Conversation

pirat89
Copy link
Member

@pirat89 pirat89 commented Sep 17, 2024

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

Note: just written on knees without testing. Unit tests should be updated too.

@pirat89
pes_events_scanner: fix processing of RpmTransactionTasks
24ca41f 
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
@pirat89 pirat89 added the bug Something isn't working label Sep 17, 2024
@github-actions GitHub Actions
Copy link

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
However, here are additional useful commands for packit:

  • /packit test to re-run manually the default tests
  • /packit retest-failed to re-run failed tests manually
  • /packit test oamg/leapp#42 to run tests with leapp builds for the leapp PR#42 (default is latest upstream - main - build)

Note that first time contributors cannot run tests automatically - they need to be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

  • /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
  • /packit test --labels beaker-minimal-8.10to9.4,kernel-rt-8.10to9.4 to schedule kernel-rt and beaker-minimal test sets for 8.10->9.4 upgrade path

See other labels for particular jobs defined in the .packit.yaml file.

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

@pirat89
Copy link
Member Author

pirat89 commented Sep 18, 2024

/packit copr-build

pes_event_scanner(tests) test transaction configuration
ed6e403 
add test to check whether transaction configuration is applied
correctly in presence of multiple removals of the same package
@MichalHe
Copy link
Member

@pirat89 Although this fixes the crash, I believe that transaction configuration is handled in a weird manner altogether. If user says to_remove=['tomcat'] then the output of the pes_events_scanner actor will not contain tomcat in its to_remove fields -- see the failing unit test. Not sure whether this is intended (unlikely) but we (e.g. me) can fix this problem within this PR.

@pirat89
Copy link
Member Author

pirat89 commented Nov 2, 2024 •
edited
Loading

@pirat89 Although this fixes the crash, I believe that transaction configuration is handled in a weird manner altogether. If user says to_remove=['tomcat'] then the output of the pes_events_scanner actor will not contain tomcat in its to_remove fields -- see the failing unit test. Not sure whether this is intended (unlikely) but we (e.g. me) can fix this problem within this PR.

@MichalHe you are right that this sounds weird. feel free to fix it properly in this PR - or close this one and open new one if you want. this has been really a draft to ilustrate what I found and initial possible fix of the problem. based on your finding, it seems there is something dirty still around.

@MichalHe
Copy link
Member

MichalHe commented Nov 3, 2024

@pirat89 I added a post-processing step that will add transaction configuration's fields to the scanner's output.

pes_events_scanner: ensure user's transaction cfg is reflected
ddbe786 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pirat89 @MichalHe