-
Notifications
You must be signed in to change notification settings - Fork 163
Add target distro option and initial conversion enablement #1438
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the leapp-repository contribution and development guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp 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,
See other labels for particular jobs defined in the 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. |
During a containerized tests run, the commands/ directory is not used when running the commands tests. Instead, the commands are imported from tut/lib/python3.X/site-packages/leapp/cli/commands/ where python3.X is the python used in the tut/ virtualenv. When there are changes breaking the commands tests, the test are still passing because the files are not being updated in the directory mentioned above. This can be currently fixed by rebuilding the container which takes a lot of time. This patch adds copying of the commands/ dir to the tut/lib/python3.X/site-packages/leapp/cli/commands/. This helped while working on oamg#1438 because I caught the failing tests only after opening a the PR. The CI containerized tests are always created from scratch so it works there.
During a containerized tests run, the commands/ directory is not used when running the commands tests. Instead, the commands are imported from tut/lib/python3.X/site-packages/leapp/cli/commands/ where python3.X is the python used in the tut/ virtualenv. When there are changes breaking the commands tests, the test are still passing because the files are not being updated in the directory mentioned above. This can be currently fixed by rebuilding the container which takes a lot of time. This patch adds copying of the commands/ dir to the tut/lib/python3.X/site-packages/leapp/cli/commands/. This helped while working on oamg#1438 because I caught the failing tests only after opening a the PR. The CI containerized tests are always created from scratch so it works there.
During a containerized tests run, the commands/ directory is not used when running the commands tests. Instead, the commands are imported from tut/lib/python3.X/site-packages/leapp/cli/commands/ where python3.X is the python used in the tut/ virtualenv. When there are changes breaking the commands tests, the test are still passing because the files are not being updated in the directory mentioned above. This can be currently fixed by rebuilding the container which takes a lot of time. This patch adds copying of the commands/ dir to the tut/lib/python3.X/site-packages/leapp/cli/commands/. This helped while working on #1438 because I caught the failing tests only after opening a the PR. The CI containerized tests are always created from scratch so it works there.
122101f to
7d78e54
Compare
|
Okay I've split out the |
01cc2e6 to
cc6183a
Compare
|
I think this is now ready for some initial review. |
cc6183a to
e24e3f0
Compare
MichalHe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A substantive piece of work that imprints admiration, good job.
commands/command_utils.py
Outdated
| 'Unexpected format of {} version: {}. The required format is \'{}\'.' | ||
| ) | ||
| raise CommandError(error_str.format(version_str, desired_format.human_readable)) | ||
| raise CommandError(error_str.format(version_kind, version_str, desired_format.human_readable)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message will look something like this:
'Unexpected format of _VersionKind.SOURCE version: ...
I believe that the intention was different 'source'/'target' instead of '_VersionKind.SOURCE. If this is the case, version_kind.value needs to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
commands/command_utils.py
Outdated
|
|
||
| target_ver = env_version_override or args.target | ||
| target_ver = env_version_override or args.target_version | ||
| distro_id = os.getenv('LEAPP_TARGET_OS') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like distro_id is ambiguous (we can have different source/target distro IDs).
| distro_id = os.getenv('LEAPP_TARGET_OS') | |
| target_distro_id = os.getenv('LEAPP_TARGET_OS') |
commands/command_utils.py
Outdated
| db_config.store() | ||
|
|
||
|
|
||
| def get_target_os_options(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit confused whether, based on the function name, a list of leapp's CLI options related to target distro will be returned, or something similar.
| def get_target_os_options(): | |
| def get_available_target_distros(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with get_available_target_distro_ids.
| ' provide any virtual version for version {}' | ||
| ).format(CENTOS_VIRTUAL_VERSIONS_KEY, version) | ||
| raise StopActorExecutionError('Failed to identify virtual minor version number for the system.', | ||
| details=details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| details=details) | |
| details={'details': details|) |
| virtual_versions = centos_upgrade_paths.get(CENTOS_VIRTUAL_VERSIONS_KEY, {}) | ||
| if not virtual_versions: # Unlikely, only if using old upgrade_paths.json, but the user should not touch the file | ||
| details = {'details': 'The file does not contain any information about virtual versions of CentOS'} | ||
| raise StopActorExecutionError('The internal upgrade_paths.json file is invalid.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise StopActorExecutionError('The internal upgrade_paths.json file is invalid.') | |
| raise StopActorExecutionError('The internal upgrade_paths.json file is invalid.', details=details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
| assert ( | ||
| len(exposed_supported_paths) == 1 | ||
| ), "Expected only 1 IPUSourceToPossibleTargets on CS->RHEL upgrade" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use StopActorExecutionError. When I started in upgrades, I tried to use assert as well, and I have received a list of reasons why I should not use them in production code. The primary reason I remember is that you can disable all asserts when executing the python interpreter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for the explanation.
| """ | ||
|
|
||
| def __init__(self, repo_map, distro='', cloud_provider='', default_channels=None): | ||
| def __init__(self, repo_map, src_distro='', dst_distro='', cloud_provider='', default_channels=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that elsewhere in the PR we are using target_distro whereas here it is dst_distro. I would prefer having a single, consistent terminology.
| :type distro: str | ||
| :param src_distro: The distribution to map repos from, default to current | ||
| :type src_distro: str | ||
| :param dst_distro: The distribution to map repos to, default to current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| :param dst_distro: The distribution to map repos to, default to current | |
| :param dst_distro: The distribution to map repos to, default to current target distro |
| # It seems obvious we should check it, but it's not clear why we | ||
| # don't and investigation might be required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, it is not strictly necessary here since later when we are actually picking out repoids (final stages of the mapping process), we filter the candidates by architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of context: the comments that were there before my changes were both added in the same PR due to misunderstanding. I just modified the comments to make a bit more sense.
| return CONSUMED_DATA_STREAM_ID | ||
|
|
||
|
|
||
| def get_distro_id(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a public library, shouldn't we first deprecate it?
e24e3f0 to
6a36b15
Compare
|
Rebased, resolved a conflict and got the new linter rule changes. |
|
Everything should be addressed by the fixup commits. |
Allow setting the target OS for development purposes. Internally (cannot be set by the user) the LEAPP_TARGET_OS envar is set, similarly to how this works other arguments. Note that this patch only adds the variable and stores it into LEAPP_TARGET_OS, otherwise it is unhandled.
The distros are now stored similarly to how versions are stored. CurrentActorMocked is modified to take src_distro and dst_distro as arguments. Although there is already release_id parameter which is kept for compatibility.
This is required for conversions. The existing error message was improved to contain the key that is actually missing in the virtual versions map.
Notable changes: - removeobsoletegpgkeys - actor needs to handle RPM GPG key removal differently during converting, for now it just returns early.
…lers This should be squashed into the src/dst distro split, breaks on it's own. RepoMapDataHandler is modified to accept source and target distro independently and therefore perform mapping across different distros.
Currently the version "autocorrection" is performed only when the source system is centos, however we might get a major only version even when the source doesn't use them and the target does. For example when matching whether a version is between source and target version as is done in peseventsscanner.
The option specifies the target OS (distribution) to upgrade to. This sets the value of LEAPP_TARGET_OS, however LEAPP_DEVEL_TARGET_OS has precedence. If none of the envars are defined, default to the source distro i.e. only upgrade, no conversion, will be performed. The available options are 'rhel', 'centos' and 'almalinux'. Note that the "ID" value from /etc/os-release is used and therefore 'centos' really refers to Centos Stream.
With the introduction of --target-os option, the --target option is a bit ambiguous, therefore --target-version is added as an alias.
On CS to RHEL upgrades, particularly on 9->10, there is only one upgrade path defined, CS 9 -> latest RHEL 10 (10.X). However a situation may occur, in which the latest RHEL version has not yet been publicly released, e.g. in pre-release builds. This is problematic because the upgrade fails if the content is not yet available. If this happens the user is informed via a hint in the error message to specify the latest available RHEL version (the previous minor version) manually using the --target-version CLI option. To make leapp consider the previous minor version as supported without adding it to upgrade_paths.json (which would affect RHEL->RHEL upgrades), it is added to the IPUConfig message after processing the json. --- Also, in the error message, move the existing proxy hints from 'details' to 'hint', otherwise the original exeception message, which is in 'details' is overwritten.
197aef7 to
8f13093
Compare
|
Rebased to resolve a conflict after the merge of #1426. |
This series of patches is the initial work required for upgrading and converting in one leapp run. This is just an initial enablement, there are still several issues.
A
--target-osCLI option is added for specifying the distro to upgrade (and convert) to. Currently, all the distros that can be upgraded (RHEL, CS, AL) can be converted to and from.A
LEAPP_DEVEL_TARGET_OSenvironment variable is added for development.Included changes overview:
distro.get_distro_id()withdistro.get_{source,target}_distro().Extendremoveobsoletegpgkeysactor to remove RPM GPG keys from the source distro.Add some missing obsolete RPM GPG keys to remove for RHEL and CS - this could be a separate PR.--target-versionCLI option as an to--target- this makes is more ambiguous next to--target-os.Jira: RHEL-111672,
RHEL-111163TODO