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.

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

[RHELC-895] Fix error message when repoquery fails to retrieve information on kmod packages #818

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

abadger
Copy link
Member

@abadger abadger commented May 5, 2023

When repoquery fails to run (for instance, when repoquery decides that there isn't enough disk space to download metadata from the repository), we were printing an error message that there were no kmod containing packages. With this change, we will print an error message that says repoquery failed.

Fixes https://issues.redhat.com/browse/RHELC-895
Fixes #610

Jira Issues: RHELC-895

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • GitHub label has been added to help with Release notes
  • PR title explains the change from the user's point of view
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending if relevant

@abadger abadger added the bug-fix A bug has been fixed label May 5, 2023
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dfbb8ee) 93.78% compared to head (bcece6f) 93.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
+ Coverage   93.78%   93.87%   +0.09%     
==========================================
  Files          47       47              
  Lines        4345     4360      +15     
  Branches      769      773       +4     
==========================================
+ Hits         4075     4093      +18     
+ Misses        192      190       -2     
+ Partials       78       77       -1     
Flag Coverage Δ
centos-linux-7 88.82% <100.00%> (+0.10%) ⬆️
centos-linux-8 89.88% <100.00%> (+0.10%) ⬆️
centos-linux-9 89.95% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rt2rhel/actions/pre_ponr_changes/kernel_modules.py 100.00% <100.00%> (+3.89%) ⬆️
convert2rhel/pkghandler.py 94.14% <100.00%> (+0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
@abadger
Copy link
Member Author

abadger commented May 9, 2023

/packit test

@abadger abadger marked this pull request as draft May 10, 2023 16:25
@abadger abadger marked this pull request as ready for review June 1, 2023 00:53
@abadger
Copy link
Member Author

abadger commented Jun 14, 2023

/packit test

@danmyway
Copy link
Member

danmyway commented Aug 8, 2023

/packit build

convert2rhel/actions/pre_ponr_changes/kernel_modules.py Outdated Show resolved Hide resolved
@@ -67,8 +76,26 @@ def _get_rhel_supported_kmods(self):
if system_info.version.major == 8:
basecmd.append("--setopt=module_platform_id=platform:el8")

active_repos = ["--disablerepo=*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky. What if the user uses the --disablerepo c2r option?

We handle it in the call_yum_cmd (https://github.com/oamg/convert2rhel/blob/v1.3.2/convert2rhel/pkghandler.py#L123).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think --disablerepo from the c2r options is safe to ignore as --disablerepo=* is a superset of anything that the user might have specified.

OTOH, prior to this change, we didn't disable any repos when running repoqery in this function. I'm not sure how that worked... It seems like we would be running repoquery using both the vendor's repos and the RHEL repos in that case, but we only want to find kernel modules that are in packages in the RHEL repos here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think --disablerepo from the c2r options is safe to ignore as --disablerepo=* is a superset of anything that the user might have specified.

Ah, right, that makes sense.

It seems like we would be running repoquery using both the vendor's repos and the RHEL repos in that case, but we only want to find kernel modules that are in packages in the RHEL repos here?

Yes, we want to query just the RHEL repos. Previously we've been using the --repoid option.

--repoid=<repo>
     Specify  which  repository  to query. Using this option disables all repositories not explicitly enabled with --repoid option (can be used multiple times). By default repoquery uses whatever repositories are enabled in YUM configuration.

So we were really querying just the RHEL repos. Now given that yum doesn't have this option and you've switched to using --enablerepo then passing --disablerepo=* makes sense.

@abadger abadger added the tests-run-tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. label Sep 13, 2023
@has-bot
Copy link
Member

has-bot commented Sep 13, 2023

/packit test --labels tier0


@oamg/conversions-qe please review results and provide ack.

@abadger
Copy link
Member Author

abadger commented Oct 2, 2023

@bocekm Does the latest set of changes address your concerns?

convert2rhel/actions/pre_ponr_changes/kernel_modules.py Outdated Show resolved Hide resolved
@@ -67,8 +76,26 @@ def _get_rhel_supported_kmods(self):
if system_info.version.major == 8:
basecmd.append("--setopt=module_platform_id=platform:el8")

active_repos = ["--disablerepo=*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think --disablerepo from the c2r options is safe to ignore as --disablerepo=* is a superset of anything that the user might have specified.

Ah, right, that makes sense.

It seems like we would be running repoquery using both the vendor's repos and the RHEL repos in that case, but we only want to find kernel modules that are in packages in the RHEL repos here?

Yes, we want to query just the RHEL repos. Previously we've been using the --repoid option.

--repoid=<repo>
     Specify  which  repository  to query. Using this option disables all repositories not explicitly enabled with --repoid option (can be used multiple times). By default repoquery uses whatever repositories are enabled in YUM configuration.

So we were really querying just the RHEL repos. Now given that yum doesn't have this option and you've switched to using --enablerepo then passing --disablerepo=* makes sense.

@abadger
Copy link
Member Author

abadger commented Oct 6, 2023

This is Dev complete. Just needs integration tests.

@r0x0d
Copy link
Member

r0x0d commented Oct 6, 2023

Probably its best to rebase this and get the newest changes? Making sure the other targets will run the tests properly as well

@abadger abadger force-pushed the fixing-repoquery branch 2 times, most recently from 4510e83 to c7e1530 Compare October 6, 2023 18:08
@abadger
Copy link
Member Author

abadger commented Oct 6, 2023

Rebased and also squashed into one commit.

@Venefilyn
Copy link
Member

@abadger Needs a rebase then we can merge

…d packages

* When repoquery fails to run (for instance, when repoquery decides that there isn't enough disk space
  to download metadata from the repository), we were printing an error message that there were no kmod
  containing packages.  With this change, we will print an error message that says repoquery failed.

* Now that we are printing a unified report for pre-check failures, we need to include the referenced
  yum output in the logger.critical message.  Otherwise, the report will say to look at the "above yum
  output", but that output won't be present in the report handed to insights.

Fixes https://issues.redhat.com/browse/RHELC-895
Fixes oamg#610
@abadger
Copy link
Member Author

abadger commented Oct 31, 2023

@SpyTec Rebased and pushed!

@bocekm bocekm merged commit 8a7b98c into oamg:main Nov 2, 2023
17 of 49 checks passed
@pr-watson pr-watson mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix A bug has been fixed tests-run-tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CentOS7 >> RHEL 7 Converting with RH Satellite 6.11.2
6 participants