Verify supported target OS version in actors #1328

tomasfratrik · 2025-01-14T11:32:58Z

Originally when user specified the target system release using --target CLI option the verification has been performed immediately as only supported releases have been listed as possible choices for this option. The benefit of this solution was that users did not have to wait for all other checks to realize they execute leapp probably incorrectly. Unfortunately,

number of users do not understand why only some versions are supported
users upgrading with via various webUIs presenting only leapp reports
could not see the error message available in terminal

To resolve this problem the checks are moved into actors so in case of specified unsupported target version the information is present in generated leapp reports.

Current behaviour is like this:

in case of invalid input (incorrect format of input data) the hard error is raised as before immediately. Malformed input data will not be processed anyhow by any actors
report error when the specified target major version is not direct successor of the current system version. I.e. specify 10.0 when upgrading from RHEL 8 (only RHEL 9 is acceptable).
- this prevents number of cryptic errors as actors are not prepared for this situation
report standard inhibitor if the target release is not in the defined upgrade path, unless LEAPP_UNSUPPORTED=1
running leapp in unsupported (devel) mode skips the inhibitor and entire report

Additional changes:

Update error message when format of target version is incorrect to clarify the expected version format

NOTE

Note this solution introduces also IPUPaths msg which is by design marked already as deprecated. Read the commit msg for detail information.

jira: RHEL-51072

github-actions · 2025-01-14T11:33:09Z

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.

tomasfratrik · 2025-01-14T18:23:29Z

I put his actor into Facts phase because when I upgraded for example with --target 10.0, the wouldn't be inhibitor, but 3 errors instead. Those 3:

Following errors occurred and the upgrade cannot continue:
    1. Actor: repository_mapping
       Message: The repository mapping file is invalid: the JSON does not match required schema (wrong field type/value): The value of "mapping" field is None, but this is not allowed
    2. Actor: repositories_blacklist
       Message: Actor didn't receive required messages: RepositoriesMapping
    3. Actor: pes_events_scanner
       Message: Cannot parse RepositoriesMapping data properly

When I added this before actor checkphase, now there are still those errors, but Inhibitor is triggered successfully.

tomasfratrik · 2025-01-15T08:16:29Z

Latest push rebased against main

tomasfratrik · 2025-01-15T12:13:06Z

/packit retest-failed

tomasfratrik · 2025-01-15T12:25:43Z

Latest pushes fixed test's misspells

pirat89

I tried to explain all wanted changes in the comments, with added suggestions for updated reports. Also, you are right about the misleading errors when incorrect major version is specified. It really bad from my pov as we are aware that this is happening from time to time to people and they could be very confused about that. Let's raise the StopActorExecutionError in such a case to protect sanity of readers. Add function like this into the ipuworkflowconfig:

def check_target_major_version(curr_version, target_version):
    required_major_version = int(curr_version.split('.')[0]) + 1
    specified_major_version = int(target_version.split('.')[0])
    if specified_major_version != required_major_version:
        raise StopActorExecutionError(
            message='Specified invalid major version of the target system',
            details={
                'Specified targed major version': str(specified_major_version),
                'Required target major version': str(required_major_version),
                'hint': (
                    'The in-place upgrade is possible only to the next system'
                    ' major version: {ver}. Current solution depends heavily on this'
                    ' value and violation of this rule leads to various errors which'
                    ' could be very confusing.'
                    .format(ver=required_major_version)
                )
            }
        )

and call it before producing the IPUConfig:

    check_target_major_version(os_release.version_id, target_version)

repos/system_upgrade/common/actors/verifytargetversion/libraries/verifytargetversion.py

commands/command_utils.py

tomasfratrik · 2025-01-27T15:30:07Z

In the check_target_major_version(...) function I don't really get this sentence

Current solution depends heavily on this'
' value and violation of this rule leads to various errors which'
' could be very confusing.'

pirat89 · 2025-01-28T12:48:52Z

In the check_target_major_version(...) function I don't really get this sentence
Current solution depends heavily on this'
' value and violation of this rule leads to various errors which'
' could be very confusing.'

Feel free to reword it. The msg is that this value is crucial for the functionality of actors and incorrect value leads to undefined behaviour with number of errors, so we must stop it here.

pirat89 · 2025-01-28T15:11:55Z

repos/system_upgrade/common/actors/verifytargetversion/tests/test_verifytargetversion.py

+
+
+def _get_env_mocked(key, default=None):
+    return os.environ.get(key, default)


do not work with envars in unit tests inside os.environ

pirat89 · 2025-01-28T15:12:22Z

repos/system_upgrade/common/actors/verifytargetversion/tests/test_verifytargetversion.py

+        monkeypatch.setattr(verifytargetversion, 'get_env', lambda key, default=None: _get_env_mocked(key, default))
+        monkeypatch.setattr(verifytargetversion, 'get_target_version', lambda: target_version)
+        monkeypatch.setenv('LEAPP_UPGRADE_PATH_FLAVOUR', 'default')
+        monkeypatch.setenv('LEAPP_UPGRADE_PATH_TARGET_RELEASE', target_version)


no need to set this envar based on the code

similar for other mocking in this function. mock the current _actor using the CurrentActorMocked class from testutils.py. it's more elegant solution, resolving number of mocks in this function. also, do not use setenv when the actor is not working with os.environ directly - which usually should not.

I will send updated version in the commit.

pirat89 · 2025-01-28T15:13:09Z

repos/system_upgrade/common/actors/verifytargetversion/actor.py

+    name = 'verify_target_version'
+    consumes = ()
+    produces = (Report,)
+    tags = (FactsPhaseTag, IPUWorkflowTag)


Check actors should be executed during ChecksPhaseTag

pirat89 · 2025-01-28T15:14:50Z

repos/system_upgrade/common/actors/verifytargetversion/libraries/verifytargetversion.py

+    if unsupported and not leapp_unsupported:
+        hint = (
+                'Choose a supported version of the target OS for the upgrade.'
+                ' Alternatively, if you require to upgrade using an unsupported upgrade path,'
+                ' set the `LEAPP_UNSUPPORTED=1` environment variable to confirm you'
+                ' want to upgrade on your own risk.'
+        )
+
+        reporting.create_report([
+            reporting.Title('Specified version of the target system is not supported'),
+            reporting.Summary(
+                'The in-place upgrade to the specified version ({tgt_ver}) of the target system'
+                ' is not supported from the current system version. Follow the official'
+                ' documentation for up to date information about supported upgrade'
+                ' paths and future plans (see the attached link).'
+                ' The in-place upgrade is enabled to the following versions of the target system:{sep}{ver_list}'
+                .format(
+                    sep=FMT_LIST_SEPARATOR,
+                    ver_list=FMT_LIST_SEPARATOR.join(supported_target_versions),
+                    tgt_ver=target_version
+                )
+            ),
+            reporting.Groups([reporting.Groups.INHIBITOR]),
+            reporting.Severity(reporting.Severity.HIGH),
+            reporting.Remediation(hint=hint),
+            reporting.ExternalLink(
+                url='https://access.redhat.com/articles/4263361',
+                title='Supported in-place upgrade paths for Red Hat Enterprise Linux'
+            )
+        ])
+        raise StopActorExecution()


seems my long comment about this has been dropped by GH :/ but this can be written in more simple way if initial conditions are negated.

I will push updated commit in ~15min with all changes

pirat89 · 2025-01-28T15:26:01Z

repos/system_upgrade/common/actors/verifytargetversion/libraries/verifytargetversion.py

@@ -0,0 +1,54 @@
+from leapp import reporting
+from leapp.cli.commands import command_utils


actors should not import anything from command utils. The thing is that the content inside the leapp repository becomes dependent on stuff for the CLI which is outside of the repository. Also the function is interacting with the system storage to load the data file which is something we do not want actually in the ChecksPhase.

repos/system_upgrade/common/actors/ipuworkflowconfig/libraries/ipuworkflowconfig.py

repos/system_upgrade/common/actors/verifytargetversion/libraries/verifytargetversion.py

pirat89 · 2025-01-28T15:29:57Z

repos/system_upgrade/common/actors/verifytargetversion/actor.py

+from leapp.tags import FactsPhaseTag, IPUWorkflowTag
+
+
+class VerifyTargetVersion(Actor):


let's rather use CheckTargetVersion instead of Verify to correspond with usual naming used for actors in these repositories.

This is hackish precursor to move checking of the specified target system versions into actors to be able to create report when unsupported target version is specified. The problem is that currently there is no information about the supported upgrade paths in messages. Also, the information about supported source and target versions are stored in two different places (in system upgrade common repo): * shared configs.version library * files/upgrade_paths.json As a temporary solution let's introduce IPUPaths message which will contain filtered data from the json file based on: * the upgrade flavour (default, saphana) * and source major version There is no value to print information to users about different upgrade paths for other flavours and OS major versions. The model is marked as deprecated so we can remove it in the next release when we redesign this solution to unify how actors get this data (and define them just in one place). jira: RHEL-51072

MichalHe

I have a single minor comment that needs to be addressed (now, or in a separate PR). Once I test this, I can will approve, since there are no blockers code-wise

repos/system_upgrade/common/actors/checktargetversion/tests/test_checktargetversion.py

Originally when user specified the target system release using `--target` CLI option the verification has been performed immediately as only supported releases have been listed as possible choices for this option. The benefit of this solution was that users did not have to wait for all other checks to realize they execute leapp probably incorrectly. Unfortunately, * number of users do not understand why only some versions are supported * users upgrading with via various webUIs presenting only leapp reports could not see the error message available in terminal To resolve this problem the checks are moved into actors so in case of specified unsupported target version the information is present in generated leapp reports. Current behaviour is like this: * in case of invalid input (incorrect format of input data) the hard error is raised as before immediately. Malformed input data will not be processed anyhow by any actors * report error when the specified target major version is not direct successor of the current system version. I.e. specify 10.0 when upgrading from RHEL 8 (only RHEL 9 is acceptable). * this prevents number of cryptic errors as actors are not prepared for this situation * report standard inhibitor if the target release is not in the defined upgrade path, unless LEAPP_UNSUPPORTED=1 * running leapp in unsupported (devel) mode skips the inhibitor and entire report Additional changes: * Update error message when format of target version is incorrect to clarify the expected version format jira: RHEL-51072 Co-authored-by: Petr Stodulk <[email protected]>

pirat89

lgtm, but as significant co-author I will wait for second green tick :)

pirat89 · 2025-01-29T10:28:20Z

/packit copr-build

MichalHe · 2025-01-29T12:38:27Z

I have tested upgrading from 8.10 to various target versions with the following results:

Target version	Result
10.7	Inhibit ⚔️
9.7	Inhibit ⚔️
9.5	No inhibit ✔️
9.2	Inhibit ⚔️

Using LEAPP_UNSUPPORTE=1, I can attempt upgrading to 9.2 without any inhibitors.

MichalHe

lgtm

tomasfratrik force-pushed the unsupported-target-inhibitor branch 4 times, most recently from ca83f6f to 46d9e31 Compare January 14, 2025 18:19

tomasfratrik changed the title ~~Add unsupported target check to the 'ipuworkflowconfig' actor~~ Add actor to verify if target version is supported, if not, invoke inhibitor Jan 15, 2025

tomasfratrik marked this pull request as ready for review January 15, 2025 07:57

tomasfratrik force-pushed the unsupported-target-inhibitor branch from 46d9e31 to 1d04457 Compare January 15, 2025 08:15

tomasfratrik self-assigned this Jan 15, 2025

tomasfratrik requested a review from pirat89 January 15, 2025 11:39

tomasfratrik force-pushed the unsupported-target-inhibitor branch from 1d04457 to d100c69 Compare January 15, 2025 11:56

tomasfratrik force-pushed the unsupported-target-inhibitor branch 2 times, most recently from 35da727 to 87c7da0 Compare January 15, 2025 12:25

tomasfratrik force-pushed the unsupported-target-inhibitor branch from 87c7da0 to 4cbe76c Compare January 15, 2025 12:40

pirat89 added the enhancement New feature or request label Jan 15, 2025

pirat89 added this to the 8.10/9.6 milestone Jan 15, 2025

pirat89 assigned pirat89 and unassigned tomasfratrik Jan 17, 2025

tomasfratrik force-pushed the unsupported-target-inhibitor branch from 4cbe76c to 3e77258 Compare January 24, 2025 12:42

pirat89 requested changes Jan 24, 2025

View reviewed changes

pirat89 reviewed Jan 28, 2025

View reviewed changes

MichalHe reviewed Jan 28, 2025

View reviewed changes

pirat89 reviewed Jan 28, 2025

View reviewed changes

pirat89 force-pushed the unsupported-target-inhibitor branch from 706fbea to fed32f3 Compare January 29, 2025 04:21

pirat89 changed the title ~~Add actor to verify if target version is supported, if not, invoke inhibitor~~ Verify supported target OS version in actors Jan 29, 2025

pirat89 force-pushed the unsupported-target-inhibitor branch 2 times, most recently from b0506dc to 825e093 Compare January 29, 2025 04:34

pirat89 force-pushed the unsupported-target-inhibitor branch from 825e093 to 797e0d0 Compare January 29, 2025 04:51

MichalHe reviewed Jan 29, 2025

View reviewed changes

repos/system_upgrade/common/actors/checktargetversion/tests/test_checktargetversion.py Outdated Show resolved Hide resolved

pirat89 force-pushed the unsupported-target-inhibitor branch from 797e0d0 to 8d3a73e Compare January 29, 2025 10:18

pirat89 approved these changes Jan 29, 2025

View reviewed changes

MichalHe approved these changes Jan 29, 2025

View reviewed changes

MichalHe merged commit 0a5f66e into oamg:main Jan 29, 2025
21 of 23 checks passed

MichalHe added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify supported target OS version in actors #1328

Verify supported target OS version in actors #1328

tomasfratrik commented Jan 14, 2025 •

edited by pirat89

Loading

github-actions bot commented Jan 14, 2025

tomasfratrik commented Jan 14, 2025

tomasfratrik commented Jan 15, 2025

tomasfratrik commented Jan 15, 2025

tomasfratrik commented Jan 15, 2025 •

edited

Loading

pirat89 left a comment

tomasfratrik commented Jan 27, 2025

pirat89 commented Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025

pirat89 Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025

pirat89 Jan 28, 2025

pirat89 Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025

MichalHe left a comment

pirat89 left a comment

pirat89 commented Jan 29, 2025

MichalHe commented Jan 29, 2025

MichalHe left a comment



		def _get_env_mocked(key, default=None):
		return os.environ.get(key, default)

		@@ -0,0 +1,54 @@
		from leapp import reporting
		from leapp.cli.commands import command_utils

		from leapp.tags import FactsPhaseTag, IPUWorkflowTag


		class VerifyTargetVersion(Actor):

Verify supported target OS version in actors #1328

Verify supported target OS version in actors #1328

Conversation

tomasfratrik commented Jan 14, 2025 • edited by pirat89 Loading

NOTE

github-actions bot commented Jan 14, 2025

Thank you for contributing to the Leapp project!

tomasfratrik commented Jan 14, 2025

tomasfratrik commented Jan 15, 2025

tomasfratrik commented Jan 15, 2025

tomasfratrik commented Jan 15, 2025 • edited Loading

pirat89 left a comment

Choose a reason for hiding this comment

tomasfratrik commented Jan 27, 2025

pirat89 commented Jan 28, 2025 • edited Loading

pirat89 Jan 28, 2025

Choose a reason for hiding this comment

pirat89 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

pirat89 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

pirat89 Jan 28, 2025

Choose a reason for hiding this comment

pirat89 Jan 28, 2025

Choose a reason for hiding this comment

pirat89 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

pirat89 Jan 28, 2025

Choose a reason for hiding this comment

MichalHe left a comment

Choose a reason for hiding this comment

pirat89 left a comment

Choose a reason for hiding this comment

pirat89 commented Jan 29, 2025

MichalHe commented Jan 29, 2025

MichalHe left a comment

Choose a reason for hiding this comment

tomasfratrik commented Jan 14, 2025 •

edited by pirat89

Loading

tomasfratrik commented Jan 15, 2025 •

edited

Loading

pirat89 commented Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025 •

edited

Loading

pirat89 Jan 28, 2025 •

edited

Loading