[RHELC-297] Clean up kmods functionality #469

r0x0d · 2022-03-23T14:54:13Z

These changes are cleanup of most of the things that were left behind from
PR #193.

Some of the functions were changed to make it easier to read and debug if
necessary, some of them were moved around to the files that made sense to place
they and some of them just needed a docstring to tell what the function was all
about.

This PR it is introduced a unit test for the function get_enabled_rhel_repos
since that was introduced in the past and never had a unit test tied to it.

Jira reference (If any): https://issues.redhat.com/browse/OAMG-4668
Bugzilla reference (If any):

Signed-off-by: Rodolfo Olivieri rolivier@redhat.com

r0x0d · 2022-03-23T15:00:07Z

convert2rhel/checks.py

 def _repos_version_key(pkg_name):
+    """Identify the version key in a given package name.
+
+    Consider the following pkg_name that will be passed to this function::
+
+        pkg_name = 'kernel-core-0:4.18.0-240.10.1.el8_3.x86_64'
+
+    The output of this will be a tuple containing the package version in a tuple::
+
+        result = _repos_version_key(pkg_name=pkg_name)
+        print(result)
+        # (4, 15, 0, 240, 10, 1)
+
+    The function will ignore the package name as it is not an important information here
+    and will only care about the version that is tied to it's name.
+
+    :param pkg_name: The package to extract the version
+    :type pkg_name: str
+    :return: A tuple containing the package version.
+    :rtype: tuple[int]
+    :raises:
+        SystemExit: Raises SytemExit if it can't find the version in the pkg_name.
+    """
+    # TODO(r0x0d): This should be moved to repo.py or utils.py?
    try:
        rpm_version = KERNEL_REPO_RE.search(pkg_name).group("version")
    except AttributeError:
        logger.critical(
-            "Unexpected package:\n%s\n is a source of kernel modules.",
+            "Unexpected package: %s\n Couldn't find the version of the given package.",
            pkg_name,
        )
    else:
        return tuple(
            map(
-                _convert_to_int_or_zero,
+                convert_to_int_or_zero,
                KERNEL_REPO_VER_SPLIT_RE.split(rpm_version),
            )
        )


I was unsure about this... Maybe we should move this function to convert2rhel/repos.py file? Since it's dealing with "repos" even it is only extracting the version from a string.

Not sure if the convert2rhel/repos.py or convert2rhel/utils.py is a better location for this function.

It feels like maybe a function for pkghandler? (Unless we want pkghandler to only deal with packages-in-repositories). It's parsing a version from a single package's NEVR. One possible reason not to move this (or to change it when we do move it), though: right now, this function is very specifically targeted at the NEVR of kernel package's that we think we are going to encounter. All of the following NEVRs would output the same value from this function despite being different packages:

kernel-2.4.0-1.el8_1

kernel-2.4-0.1.el8_1

kernel-2.4.0-1.el9_0

r0x0d · 2022-03-23T15:00:43Z

convert2rhel/systeminfo.py

        # TODO:
        # if not self.submgr_enabled_repos:


This TODO here needs any new information? It's not telling much.

If it doesn't make sense, can we remove it?

I can see the potential problem with repos not being enabled yet but I'm not sure whether that is ever a problem in actuality (especially since customers aren't reporting failures because of it). @zhukovgreen Do you happen to know about this TODO and whether we can either enhance what it says or remove it?

I don't have any input on this. Seems like some artifact which I forgot to remove

codecov · 2022-03-23T17:10:14Z

Codecov Report

Base: 93.30% // Head: 93.23% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (c9e9636) compared to base (700088a).
Patch coverage: 91.48% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
- Coverage   93.30%   93.23%   -0.07%     
==========================================
  Files          22       22              
  Lines        3107     3135      +28     
  Branches      552      557       +5     
==========================================
+ Hits         2899     2923      +24     
- Misses        144      148       +4     
  Partials       64       64

Flag	Coverage Δ
centos-linux-6	`89.72% <91.48%> (-0.04%)`	⬇️
centos-linux-7	`89.19% <61.70%> (-0.49%)`	⬇️
centos-linux-8	`89.38% <61.70%> (-0.49%)`	⬇️

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

Impacted Files	Coverage Δ
convert2rhel/systeminfo.py	`96.61% <ø> (ø)`
convert2rhel/checks.py	`94.82% <88.23%> (-1.16%)`	⬇️
convert2rhel/main.py	`92.35% <100.00%> (ø)`
convert2rhel/pkghandler.py	`92.93% <100.00%> (+0.19%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

convert2rhel/checks.py

abadger · 2022-04-07T17:20:17Z

I've reviewed this. Other than the questions about get_most_recent_unique_kernel_pkgs() and its helper functions, this looks good.

abadger · 2022-05-16T17:12:45Z

convert2rhel/pkghandler.py

@@ -1075,3 +1082,45 @@ def _get_packages_to_update_dnf():
        packages.append(package.name)

    return packages
+
+
+def package_version_cmp(pkg_1, pkg_2):


Hmmm... A few higher level thoughts on this function:

Sometimes yum and dnf return NEVR like this: NetworkManager-1:1.18.8-2.0.1.el7_9.x86_64. Other times they return it like this (ENVR form): 1:NetworkManager-1.18.8-2.0.1.el7_9.x86_64. Should we handle both of these cases? In one function or in two?

There's already a pkghandler.compare_package_versions function. That one takes its input as EVR strings: 1:1.18.8-2.0.1.el7_9.x86_64 which removes the ambiguity around whether the epoch is before or after name because there is no name. However, we can't use that directly as a key function (since the key will be a NEVR or an ENVR string).

Should we verify that the name is the same for both strings passed into this function? If I call the function with `package_version_cmp("test-1.0-1", "foobar-2.0-1") I would expect it to error rather than say foobar is later than test.

Should we parse arch off the end of the NEVR (arch is x86_64, i686, s390x, etc) and error if arches do not match?

Here's my strawman to address all of those:

Separate version parsing and version comparison

The parsing function will take a version string in either ENVR or NEVR format.

If epoch or arch does not exist in the input string, it will be set to None in the tuple.

Parsing arch will be slightly tricky as I don't think there's a fool-proof way to separate an optional arch from release. Maybe looking for a list of allowed arch patterns is the best we can do. Something like: .*(x86_64|s390x|i.86)?$

The version comparison function will take two version strings in either ENVR or NEVR format.

The strings will be passed to the parsing functions to return the information in a normalized form.

The names and arches will be compared.

An error will be emitted if names do not match.

An error will be emitted if arches do not match and both arches are not None.

rpm.labelCompare() will then be used to compare the EVR for both packages and the results returned.

Port existing use of pkghandler.compare_package_versions() to use the new comparison function.

Remove pkghandler.compare_package_versions() and utils.string_to_version().

Discuss?

@abadger, I think we can follow up on this in our 1:1 sessions. Probably next week, we can discuss it a bit more on which direction we want to move with this function.

Definitely, there is a lot of room to improve the current function.

We talked about this today and decided that this can be done in a followup PR. For now, we will:

mark this function as private so we don't have to verify a whole lot of callers work once we refactor this

Add a comment to the function which list the changes that we want to make.

I will create a Jira issue as well for specifically this one change.

Issues created: https://issues.redhat.com/browse/RHELC-667

convert2rhel/pkghandler.py

kokesak · 2022-06-22T11:36:25Z

@r0x0d once you will have a free time, please rebase the PR

r0x0d · 2022-07-11T12:38:12Z

@kokesak rebased!

r0x0d · 2022-07-15T14:33:48Z

I will rebase this to resolve the conflict with the checks.py

danmyway · 2022-08-03T08:29:39Z

/packit retest-failed

danmyway · 2022-08-03T13:22:34Z

/packit build

danmyway · 2022-08-04T07:05:53Z

/packit build

convert2rhel/checks.py

lgtm-com · 2022-08-05T19:01:03Z

This pull request fixes 1 alert when merging 5f52da8 into 121fa76 - view on LGTM.com

fixed alerts:

1 for Unused named argument in formatting call

convert2rhel/utils.py

kokesak

@danmyway since we want to improve the tests description, could you please update the summary in tests/integration/tier0/inhibit-if-kmods-is-not-supported/main.fmf and maybe even in the pytest file as well if you think it's not good enough

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

As discussed with developers, force loaded kmod should inhibit the c2r run, as it is denoted with (FE) for F = module was force loaded E = unsigned module was loaded. Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

fix typos Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

lgtm-com · 2022-10-28T19:28:57Z

This pull request fixes 3 alerts when merging 32faec7 into 700088a - view on LGTM.com

fixed alerts:

2 for Unused import
1 for Unused named argument in formatting call

kokesak

The integration test looks nice.

Co-authored-by: Michal Bocek <mbocek@redhat.com>

lgtm-com · 2022-10-31T17:06:31Z

This pull request fixes 3 alerts when merging c9e9636 into e1dfbdd - view on LGTM.com

fixed alerts:

2 for Unused import
1 for Unused named argument in formatting call

abadger

Code looks fine but there were two failing integration tests. It looks like those are due to a problem with the yum repository. I've kicked off new runs for both of those.

I also see that Eric had a crequest to change something but I wasn't able to find it in the converaation. Can you check if that was addressed before merging?

Venefilyn

I also see that Eric had a crequest to change something but I wasn't able to find it in the converaation. Can you check if that was addressed before merging?

It was related to py2.6 and been addressed, LGTM as well

r0x0d · 2022-11-01T12:07:35Z

Thanks, @abadger for re-running the tests. Now the only thing that failed is the codecov patch/project checks, but they are fine.

@kokesak could you approve it once again?

kokesak

Looks nice, merge it whenever you want.

In oamg#469, one of the cleanups ended up calling `"\n".join()` on a list of kernel modules twice. The first time changes the list into a string with each entry separated by a newline but the second time changes the string into another string with each chatacter separated by a newline. Fixed that by removing the second `str.join()`. This also refactors away a call to `map()` and use of `lambda`. In modern python, any uses of `map()` or `filter()` can be replaced with either a generator expression or list comprehension. Related to oamg#469

…s. (#651) * Fix the format of message about unavailable kernel modules. In #469, one of the cleanups ended up calling `"\n".join()` on a list of kernel modules twice. The first time changes the list into a string with each entry separated by a newline but the second time changes the string into another string with each chatacter separated by a newline. Fixed that by removing the second `str.join()`. This also refactors away a call to `map()` and use of `lambda`. In modern python, any uses of `map()` or `filter()` can be replaced with either a generator expression or list comprehension. Related to #469 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

…s. (oamg#651) * Fix the format of message about unavailable kernel modules. In oamg#469, one of the cleanups ended up calling `"\n".join()` on a list of kernel modules twice. The first time changes the list into a string with each entry separated by a newline but the second time changes the string into another string with each chatacter separated by a newline. Fixed that by removing the second `str.join()`. This also refactors away a call to `map()` and use of `lambda`. In modern python, any uses of `map()` or `filter()` can be replaced with either a generator expression or list comprehension. Related to oamg#469 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Clean up kmods functionality These changes are cleanup of most of the things that were left behind from PR#193. Some of the functions were changed to make it easier to read and debug if necessary, some of them were moved around to the files that made sense to place they and some of them just needed a docstring to tell what the function was all about. This PR it is introduced a unit test for the function `get_enabled_rhel_repos` since that was introduced in the past and never had a unit test tied to it. Jira reference (If any): https://issues.redhat.com/browse/OAMG-4668 Bugzilla reference (If any): Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Add unit tests for the code refactored Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Apply integration tests from a59c4bd Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Apply patch from 3b61461 Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Apply suggestions from Michal's review Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Edit force loaded kmod test As discussed with developers, force loaded kmod should inhibit the c2r run, as it is denoted with (FE) for F = module was force loaded E = unsigned module was loaded. Signed-off-by: Daniel Diblik <ddiblik@redhat.com> * Incorporate review suggestions fix typos Signed-off-by: Daniel Diblik <ddiblik@redhat.com> * Fix formatting issue Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Fix unit_tests Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Add cmp_to_key for python 2.6 compatibility Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> * Update convert2rhel/checks.py Co-authored-by: Michal Bocek <mbocek@redhat.com> Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com> Signed-off-by: Daniel Diblik <ddiblik@redhat.com> Co-authored-by: Daniel Diblik <ddiblik@redhat.com> Co-authored-by: Michal Bocek <mbocek@redhat.com>

…s. (#651) * Fix the format of message about unavailable kernel modules. In #469, one of the cleanups ended up calling `"\n".join()` on a list of kernel modules twice. The first time changes the list into a string with each entry separated by a newline but the second time changes the string into another string with each chatacter separated by a newline. Fixed that by removing the second `str.join()`. This also refactors away a call to `map()` and use of `lambda`. In modern python, any uses of `map()` or `filter()` can be replaced with either a generator expression or list comprehension. Related to #469 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

r0x0d commented Mar 23, 2022

View reviewed changes

r0x0d self-assigned this Mar 23, 2022

abadger reviewed Apr 7, 2022

View reviewed changes

convert2rhel/checks.py Outdated Show resolved Hide resolved

r0x0d force-pushed the kmods-code-cleanup branch 2 times, most recently from 74b12c5 to 882b47b Compare April 26, 2022 18:45

r0x0d force-pushed the kmods-code-cleanup branch from 882b47b to d511dd2 Compare May 12, 2022 19:26

abadger reviewed May 16, 2022

View reviewed changes

r0x0d force-pushed the kmods-code-cleanup branch from 88effe9 to acc88c6 Compare June 3, 2022 15:02

r0x0d force-pushed the kmods-code-cleanup branch from 6fa7abc to 5017f8d Compare July 11, 2022 12:37

danmyway added the need-integration-tests label Jul 14, 2022

r0x0d force-pushed the kmods-code-cleanup branch from 5017f8d to a2b8f15 Compare August 2, 2022 14:47

r0x0d changed the title ~~Clean up kmods functionality~~ [RHELC-297] Clean up kmods functionality Aug 3, 2022

bocekm requested changes Aug 4, 2022

View reviewed changes

convert2rhel/checks.py Outdated Show resolved Hide resolved

danmyway requested a review from mkluson August 5, 2022 10:48

r0x0d force-pushed the kmods-code-cleanup branch from a59c4bd to a376984 Compare August 5, 2022 13:53

danmyway removed the need-integration-tests label Aug 5, 2022

bocekm requested changes Aug 5, 2022

View reviewed changes

convert2rhel/checks.py Show resolved Hide resolved

bocekm approved these changes Aug 5, 2022

View reviewed changes

danmyway requested a review from kokesak August 8, 2022 10:01

bocekm requested changes Aug 9, 2022

View reviewed changes

convert2rhel/utils.py Outdated Show resolved Hide resolved

kokesak requested changes Aug 10, 2022

View reviewed changes

r0x0d and others added 7 commits October 28, 2022 16:14

Apply integration tests from a59c4bd

9305ca0

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Apply patch from 3b61461

7af9a7c

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Apply suggestions from Michal's review

0dc0aef

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Edit force loaded kmod test

674c7fe

As discussed with developers, force loaded kmod should inhibit the c2r run, as it is denoted with (FE) for F = module was force loaded E = unsigned module was loaded. Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

Incorporate review suggestions

90466f7

fix typos Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

Fix formatting issue

b327eb9

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Fix unit_tests

9f640b3

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d force-pushed the kmods-code-cleanup branch from 7b41411 to 82c646c Compare October 28, 2022 19:20

Add cmp_to_key for python 2.6 compatibility

32faec7

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d force-pushed the kmods-code-cleanup branch from 9194cfe to 32faec7 Compare October 28, 2022 19:21

danmyway requested a review from kokesak October 31, 2022 14:09

kokesak previously approved these changes Oct 31, 2022

View reviewed changes

Update convert2rhel/checks.py

c9e9636

Co-authored-by: Michal Bocek <mbocek@redhat.com>

r0x0d dismissed kokesak’s stale review via c9e9636 October 31, 2022 17:01

abadger approved these changes Nov 1, 2022

View reviewed changes

Venefilyn approved these changes Nov 1, 2022

View reviewed changes

kokesak approved these changes Nov 3, 2022

View reviewed changes

r0x0d merged commit 1e52449 into oamg:main Nov 3, 2022

r0x0d deleted the kmods-code-cleanup branch November 3, 2022 12:49

abadger mentioned this pull request Nov 4, 2022

[RHELC-297] Fix the format of message about unavailable kernel modules. #651

Merged

7 tasks

Venefilyn mentioned this pull request Nov 30, 2022

Bump version to 1.1 #675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-297] Clean up kmods functionality #469

[RHELC-297] Clean up kmods functionality #469

r0x0d commented Mar 23, 2022 •

edited by Venefilyn

Loading

r0x0d Mar 23, 2022

abadger Apr 4, 2022

r0x0d Mar 23, 2022

abadger Apr 7, 2022

zhukovgreen Apr 8, 2022

codecov bot commented Mar 23, 2022 •

edited

Loading

abadger commented Apr 7, 2022

abadger May 16, 2022

r0x0d May 16, 2022

abadger Jul 7, 2022

r0x0d Jul 11, 2022

r0x0d Jul 15, 2022

kokesak commented Jun 22, 2022

r0x0d commented Jul 11, 2022

r0x0d commented Jul 15, 2022

danmyway commented Aug 3, 2022

danmyway commented Aug 3, 2022

danmyway commented Aug 4, 2022

lgtm-com bot commented Aug 5, 2022

kokesak left a comment

lgtm-com bot commented Oct 28, 2022

kokesak left a comment

lgtm-com bot commented Oct 31, 2022

abadger left a comment

Venefilyn left a comment

r0x0d commented Nov 1, 2022

kokesak left a comment

[RHELC-297] Clean up kmods functionality #469

[RHELC-297] Clean up kmods functionality #469

Conversation

r0x0d commented Mar 23, 2022 • edited by Venefilyn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2022 • edited Loading

Codecov Report

abadger commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokesak commented Jun 22, 2022

r0x0d commented Jul 11, 2022

r0x0d commented Jul 15, 2022

danmyway commented Aug 3, 2022

danmyway commented Aug 3, 2022

danmyway commented Aug 4, 2022

lgtm-com bot commented Aug 5, 2022

kokesak left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 28, 2022

kokesak left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 31, 2022

abadger left a comment

Choose a reason for hiding this comment

Venefilyn left a comment

Choose a reason for hiding this comment

r0x0d commented Nov 1, 2022

kokesak left a comment

Choose a reason for hiding this comment

r0x0d commented Mar 23, 2022 •

edited by Venefilyn

Loading

codecov bot commented Mar 23, 2022 •

edited

Loading