feat: Add leapp_pre_upgrade_update variable to the analysis role#383
feat: Add leapp_pre_upgrade_update variable to the analysis role#383spetrosi merged 2 commits intoredhat-cop:mainfrom
Conversation
1437ec4 to
ada466f
Compare
|
[citest] |
ada466f to
96d2d89
Compare
|
[citest] |
roles/analysis/defaults/main.yml
Outdated
| # Directory to store leapp pre-upgrade reports and remediation hostvars | ||
| leapp_workdir_controller: "{{ playbook_dir }}" | ||
|
|
||
| # Whether to update all packages to latest version before running leapp analysis |
There was a problem hiding this comment.
Fixed. I am not a fun of duplicating vars description in readme and here
96d2d89 to
030d856
Compare
|
[citest] |
|
Waiting for @swapdisk and @djdanielsson opinions. This changes the default behavior of the analysis role - it now clears versionlock list, updates all packages and reboots. |
|
analysis shouldn't update/touch anything that should be remediation. the most analysis should do is install leapp and run it IMO. a lot of times we tried to push because it is just doing analysis it would be safe to run anytime and not require maintenance window. @swapdisk thoughts? |
|
I agree with @djdanielsson. Having big changes and reboots possible in the analysis role would qualify as a "breaking change" to the collection. Making "no impactful changes" during the analysis phase is a key part of the narrative when we talk about the end-to-end workflow of RHEL upgrade automation. However, another key point we talk about is not making changes between when you run the analysis and when you launch the upgrade. The upgrade role doing I think the best way to solve this would be make the analysis job fail or raise an inhibitor when a host is so far behind on updates that it will lead to this issue. Then updating such a host can be treated as a required remediation just like any other inhibitor. That way, we're keeping with our existing end-to-end approach that everybody has gotten comfortable with. |
swapdisk
left a comment
There was a problem hiding this comment.
As I mentioned in the main comment thread, I would prefer a fail or inhibitor rather than allowing changes during analysis.
But if we do decide to go with this, I would at least like to make sure we fix the "pre-upgrade" terminology and make the new var default to false.
| - The analysis role historically runs only the pre-upgrade analysis that does not do any changes to the system. | ||
| - However, in the case where the target system is not fully-updated - the analysis might pass, but the dnf update that the upgrade role does might bring some new inhibitors, hence the upgrade might fail. | ||
| - So, the proper approach would be to allow the analysis role do the required pre-upgrade steps so that the analysis is run on the same system as the upgrade. |
There was a problem hiding this comment.
We need to be careful how we use the pre-upgrade wording. Historically, the term pre-upgrade has mostly been synonymous with analysis. Like we might say "The analysis phase generates the pre-upgrade report," for example.
However, "required pre-upgrade steps" here means "changes required before generating the report" which is very different.
The same goes for the leapp_pre_upgrade var name, as I commented below in more detail.
There was a problem hiding this comment.
The concept here is "prepare the system so that the analysis is as accurate as possible, so that we find all of the inhibitors/issues during analysis, so that we don't run upgrade only to find there were inhibitors/issues missed during analysis" - is there a terse way we can describe that concept?
roles/analysis/defaults/main.yml
Outdated
| leapp_workdir_controller: "{{ playbook_dir }}" | ||
|
|
||
| # Whether to update all packages to latest version and reboot before running leapp analysis | ||
| leapp_pre_upgrade: true |
There was a problem hiding this comment.
To avoid any possible confusion with the meaning of pre-upgrade as in "generating the pre-upgrade report" as documented for the leapp preupgrade command, let's change this variable name. I think the leapp_update_all_packages name in the title of the PR would be fine.
Also, unless we want to put this forward as an API breaking change for a version 2.0.0, it should be false by default.
There was a problem hiding this comment.
I was thinking that we can add more automated pre-upgrade tasks in the future. Like e.g. doing a backup etc.
roles/analysis/README.md
Outdated
| | Name | Type | Default value | Description | | ||
| |---------------------------|--------|---------------|-------------| | ||
| | leapp_upgrade_type | String | "cdn" | Set to "cdn" for hosts registered with Red Hat CDN, "rhui" for hosts using rhui repos, "satellite" for hosts registered to Satellite, and "custom" for custom repos. | | ||
| | leapp_pre_upgrade | bool | true | Prepare for the upgrade. When true, the role will do the following - clear the versionlock list, update all packages and optionally reboot if any packages were updated. | |
There was a problem hiding this comment.
As mentioned above, change the var name and default to false.
roles/analysis/README.md
Outdated
|
|
||
|
|
||
|
|
||
| leapp_pre_upgrade: true |
The analysis role historically runs only the pre-upgrade analysis that does not do any changes to the system. However, in the case where the target system is not fully-updated - the analysis might pass, but the `dnf update` that the upgrade role does might bring some new inhibitors, hence the upgrade might fail. So, the proper approach would be to allow the analysis role do the required pre-upgrade steps so that the analysis is run on the same system as the upgrade. When `leapp_pre_upgrade: true` is set, the analysis role will do the following: 1. Clear the versionlock list 2. Update all packages 3. Reboot if any packages were updated.
030d856 to
5b505e3
Compare
81d97cd to
19de2e0
Compare
|
[citest] |
19de2e0 to
3425946
Compare
|
[citest] |
3425946 to
bef8d4a
Compare
|
[citest] |
The analysis role historically runs only the pre-upgrade analysis that does not do any changes to the system.
However, in the case where the target system is not fully-updated - the analysis might pass, but the
dnf updatethat the upgrade role does might bring some new inhibitors, hence the upgrade might fail. So, the proper approach would be to allow the analysis role do the required pre-upgrade steps so that the analysis is run on the same system as the upgrade.When
leapp_pre_upgrade_update: trueis set, the analysis role will do the following: