feat: Generate remediation vars and playbook from the analysis role#361
feat: Generate remediation vars and playbook from the analysis role#361spetrosi merged 8 commits intoredhat-cop:mainfrom
Conversation
|
@swapdisk @djdanielsson please review, this brings some integration between the analysis role results and remediations to call. The risk is that customers would run remediation without thinking, which might break their systems. I tried to make it clear that it's user's responsibility to view what remediations do before running them. |
02cbee9 to
a38a270
Compare
|
[citest] |
a38a270 to
8f02479
Compare
|
[citest] |
8f02479 to
e3c8d6b
Compare
|
[citest] |
| ansible.builtin.slurp: | ||
| src: "{{ leapp_result_filename }}" | ||
| register: results_txt | ||
| no_log: true |
There was a problem hiding this comment.
I think these no_log should be a var in case something goes wrong the user to disable them to see the output
There was a problem hiding this comment.
Look at the example output in this log: https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/tf_infra.leapp-349_RHEL-9.7.0-Nightly-2.17_20260212-083641/artifacts/leapp-tests_upgrade_custom_9to10-ANSIBLE-2.17-test_playbooks-SUCCESS.log
I hid the output that is not readable and makes it hard to read what ansible returns.
The output of the slurp module is completely unreadable, while some set_fact outputs are kinda readable, but still too much text. In the case where you run the role against >1 host it pollutes the terminal.
Now pre-upgrade reports are copied to the controller, users can open them and view.
|
Does it make sense to copy ripu.log from managed nodes to controller too? |
e3c8d6b to
00e7bf6
Compare
|
[citest] |
00e7bf6 to
8a2bc86
Compare
| - name: copy_reports_to_controller | Copy report files to the controller | ||
| ansible.builtin.fetch: | ||
| src: "{{ item.src }}" | ||
| dest: "{{ reports_directory }}/{{ leapp_result_basename }}.{{ inventory_hostname }}.{{ item.ext }}" |
There was a problem hiding this comment.
since you have inventory_hostname in the dest, you might want to use flat: true - https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/fetch_module.html#parameter-flat
There was a problem hiding this comment.
Yeah I figured it out, preparing a fix
| state: absent | ||
| delegate_to: localhost | ||
| loop: | ||
| - "{{ playbook_dir }}/host_vars/{{ inventory_hostname }}.yml" |
There was a problem hiding this comment.
you can't remove this file if there was already one created by the user
There was a problem hiding this comment.
I won't create those files, doesn't hurt to keep them, and users might search for them even after the run the role.
2e62924 to
dc32237
Compare
| leapp_system_roles_collection: "{{ infra_leapp_upgrade_system_roles_collection | default('fedora.linux_system_roles') }}" | ||
|
|
||
| # Directory to store leapp pre-upgrade reports and remediation hostvars | ||
| leapp_workdir_controller: "{{ playbook_dir }}" |
There was a problem hiding this comment.
@djdanielsson how will this work when using AAP with an EE? Where does the "controller" store local files in this case?
|
After an upgrade tree of copied from managed nodes files looks like this: |
|
[citest] |
2 similar comments
|
[citest] |
|
[citest] |
tests/tasks/common_upgrade_tasks.yml
Outdated
| - "{{ playbook_dir }}/remediate.yml" | ||
|
|
||
| # Note: We cannot use ansible.builtin.import_playbook here because import_playbook | ||
| # can only be used at the playbook level, not in task files. Therefore, we execute |
There was a problem hiding this comment.
I suppose you could generate both the remediate playbook and a remediate_tasks tasks file - that way, the user could use the remediate playbook separately if desired, and this code could directly execute the tasks file. It's generally not good to execute ansible-playbook from within ansible-playbook, although a case could be made.
There was a problem hiding this comment.
Task file would be a single task including the role. Let's just run the role with include role here, and for the generated playbook just test that it exists.
9438023 to
6dc58b6
Compare
Analysis role generates hostvars files for each node that runs pre-upgrade. It reads pre-upgrade reports and matches found inhibitors to available remediations provided by the `remediation` role. `analysis` role creates the directory `host_vars` in the path from which you run your analysis playbook. It generates variable files that set the `leapp_remediation_todo` variable with available remediations for existing inhibitors, based on each node's pre-upgrade report. The role also generates a `remediate.yml` playbook that you can run to run remediation for all nodes.
* Add leapp_workdir_controller to analysis and upgrade to select where to store logs on controller * Add leapp_copy_reports defaulting to true to upgrade too * Get rid of RIPU wording, it's confusing, use Ansible leapp logs instead. * Move copy_reports_to_controller to common to be used by analysis and upgrade * In create_remediation_hostvars do delegate_to: localhost on a block level as Rich suggested. * Move logs to a timestamped directory on controller. All logs to a single directory per run, under which there is a separate dir for each node. * Remove handlers from common. Handlers run in a different environment and don't have access to variables, and it was impossible to save to timestamp. Instead, add task files to be called in the beginning and in the end of the run. * Remove tasks from common/tasks/main.yml. For clarity, use only tasks files in the common role. * Use include_role instead of import_role as it's more robust. * Do not remove generated hostvars and remediation playbook after role finishes. It doesn't hurt to keep them for users to be able to access even after they are not needed.
Add a cleanup task to common to be reused
59879ee to
a127570
Compare
|
[citest] |
|
@PeterMocary @swapdisk please review |
| - It reads pre-upgrade reports and matches found inhibitors to available remediations provided by the remediation role. | ||
| - analysis role creates the directory host_vars in the path from which you run your analysis playbook. | ||
| - It generates variable files that set the leapp_remediation_todo variable with available remediations for existing inhibitors, based on each node's pre-upgrade report. | ||
| - The role also generates a remediate.yml playbook that you can run to run remediation for all nodes. |
There was a problem hiding this comment.
| - The role also generates a remediate.yml playbook that you can run to run remediation for all nodes. | |
| - The role also generates a remediate.yml playbook that you can use to run remediation for all nodes. |
There was a problem hiding this comment.
Looks fine, just found some nitpicks.
Maybe a good improvement for the future would be also copying the leapp.db on demand if more debugging is needed, sometimes it is nice to have it when people ask about harder problems.
Edit: Also the leapp-*.log could be copied with the leapp.db for debugging
| owner: root | ||
| group: root | ||
|
|
||
| msg: "You must run the role with tasks_from: <task_file>.yml" |
There was a problem hiding this comment.
Nitpick: We could explain it bit more. I'd do something like this: "This role is a set of tasks used across the other roles from this collection. It is not expected to be run on its own. Please use individual task files from the role with tasks_from: <task_file>.yml"
There was a problem hiding this comment.
This is not a customer facing test, so for developers I think it's alright and clear like this.
|
|
||
| - name: Test | Ensure correct error | ||
| vars: | ||
| err_pat: You must run the role with tasks_from |
There was a problem hiding this comment.
Don't forget to change this as well, if you decide to go for the error message change.
roles/remediate/README.md
Outdated
| `analysis` role creates the directory `host_vars` in the path from which you run your analysis playbook. | ||
| It generates variable files that set the `leapp_remediation_todo` variable with available remediations for existing inhibitors, based on each node's pre-upgrade report. | ||
|
|
||
| The role also generates a `remediate.yml` playbook that you can run to run remediation for all nodes. |
There was a problem hiding this comment.
| The role also generates a `remediate.yml` playbook that you can run to run remediation for all nodes. | |
| The role also generates a `remediate.yml` playbook that you can use to run remediation for all nodes. |
|
|
||
| **Use at your own risk!** | ||
| Note that remediations that this role provides example remediations for the very common scenarios. | ||
| Some remediations can be destructive, some might not do what is needed exactly at your case. | ||
| It is your responsibility to verify that what they do is suitable for your environment. |
There was a problem hiding this comment.
| **Use at your own risk!** | |
| Note that remediations that this role provides example remediations for the very common scenarios. | |
| Some remediations can be destructive, some might not do what is needed exactly at your case. | |
| It is your responsibility to verify that what they do is suitable for your environment. | |
| > [!IMPORTANT] | |
| > Note that this role provides remediations for the most common scenarios. Some remediations can be destructive and others might not do exactly what is needed in your case. It is your responsibility to verify that they are suitable for your environment. |
There was a problem hiding this comment.
It is in our other READMEs so I wanted it to be consistent. It is even in the https://github.com/redhat-cop/infra.leapp/blob/main/README.md
Even if you decide to not use it, change the at your case to in your case.
roles/remediate/README.md
Outdated
|
|
||
| To run remediations generated by running the analysis role, complete the following steps: | ||
|
|
||
| 1. After running the analysis role, view if it has generated any variables files at the directory from where which you run the role: |
There was a problem hiding this comment.
| 1. After running the analysis role, view if it has generated any variables files at the directory from where which you run the role: | |
| 1. After running the analysis role, check if it has generated any variables files at the directory from which you run the role: |
|
|
||
| ### Running Remediations | ||
|
|
||
| To run remediations generated by running the analysis role, complete the following steps: |
There was a problem hiding this comment.
| To run remediations generated by running the analysis role, complete the following steps: | |
| To run remediations generated by the analysis role, complete the following steps: |
| 2. If yes, view the generated files and the variables that they set. | ||
| Each variable represents a task file within this role's `tasks` directory. | ||
| View which tasks are suggested to be run to fix found inhibitors, modify to your needs or write new tasks if needed. |
There was a problem hiding this comment.
| 2. If yes, view the generated files and the variables that they set. | |
| Each variable represents a task file within this role's `tasks` directory. | |
| View which tasks are suggested to be run to fix found inhibitors, modify to your needs or write new tasks if needed. | |
| 2. If there were any files generated, review the variables that they set. | |
| Each variable represents a task file within this role's `tasks` directory. | |
| Review which tasks are *suggested to be run* to fix found inhibitors, modify based on your preferences. |
roles/remediate/README.md
Outdated
| Each variable represents a task file within this role's `tasks` directory. | ||
| View which tasks are suggested to be run to fix found inhibitors, modify to your needs or write new tasks if needed. | ||
|
|
||
| 3. Run the playbook `remediate.yml` that the analysis role generated at the directory from where which you run the role: |
There was a problem hiding this comment.
| 3. Run the playbook `remediate.yml` that the analysis role generated at the directory from where which you run the role: | |
| 3. Execute the playbook `remediate.yml` that was generated by the analysis role in the directory from which you've run it: |
There was a problem hiding this comment.
Technical writing standards (IBM Style Guide) are against using passive voice, past tenses, and reductions. for clairity and simplicity of translation. I think it's fine as is now.
There was a problem hiding this comment.
the where which is still wrong I think.
roles/upgrade/tasks/main.yml
Outdated
| - name: Include tasks for redhat-upgrade-tool post upgrade | ||
| ansible.builtin.include_tasks: redhat-upgrade-tool-post-upgrade-el6.yml | ||
| when: ansible_facts.ansible_local.pre_ripu.distribution_major_version|int == 6 | ||
| when: ansible_facts.ansible_local.pre_leapp.distribution_major_version|int == 6 |
There was a problem hiding this comment.
I don't like the rename, since the collection still uses other tools that leapp for the RHEL 6 upgrades. I'd rather go for pre_ipu - it is used in context of leapp commonly. However, the pre_leapp works as well.
There was a problem hiding this comment.
I like pre_ipu, thank you! Good to know that it's common in leapp too. I'll use it.
| - It reads pre-upgrade reports and matches found inhibitors to available remediations provided by the remediation role. | ||
| - analysis role creates the directory host_vars in the path from which you run your analysis playbook. | ||
| - It generates variable files that set the leapp_remediation_todo variable with available remediations for existing inhibitors, based on each node's pre-upgrade report. | ||
| - The role also generates a remediate.yml playbook that you can run to run remediation for all nodes. |
There was a problem hiding this comment.
These entries are tightly coupled - they describe a single feature but are split into separate bullet points that rely on each other for context. In the resulting changelog, each entry should stand on its own since readers won't necessarily see them grouped together I think.
Maybe something like this could be used:
major_changes
- The analysis role now generates per-host remediation variables and a remediate.yml playbook. It reads pre-upgrade reports, matches found inhibitors to available remediations provided
by the remediation role, and creates a host_vars directory containing variable files that set leapp_remediation_todo for each host. - Refactored log handling to use timestamped directories on the controller, controlled by the new leapp_workdir_controller variable.
- Added leapp_copy_reports and leapp_create_remediation_hostvars variables to control the new analysis role behavior.
|
[citest] |
* Fix wording and typos * Rename ansible_local.pre_leapp facts to ansible_local.pre_ipu
0eda26e to
400b997
Compare
|
[citest] |

Analysis role generates hostvars files for each node that runs pre-upgrade.
It reads pre-upgrade reports and matches found inhibitors to available remediations provided by the
remediationrole.analysisrole creates the directoryhost_varsin the path from which you run your analysis playbook.It generates variable files that set the
leapp_remediation_todovariable with available remediations for existing inhibitors, based on each node's pre-upgrade report.The role also generates a
remediate.ymlplaybook that you can run to run remediation for all nodes.