redhat-cop / infra.leapp

Collection of Ansible roles for automating RHEL in-place upgrades using Leapp.
MIT License
48 stars 40 forks source link

enablerepo not a valid option for package module #182

Closed k3mic closed 5 months ago

k3mic commented 5 months ago

roles/analysis/analysis-leapp.yml

Seems like it might still work, but it's not mentioned at:

https://docs.ansible.com/ansible/latest/collections/ansible/builtin/package_module.html

Maybe consider ensuring the repo is enabled with community.general.rhsm_repository module before the package install task?

djdanielsson commented 5 months ago

or we can just use the yum/dnf modules which do support those options and why this hasn't been an issue before because package ends up calling those modules anyways.

k3mic commented 5 months ago

or we can just use the yum/dnf modules which do support those options and why this hasn't been an issue before because package ends up calling those modules anyways.

Only concern is if the repo is enabled for a one time install of leapp-upgrade and it's deps. The newly installed package(s) may go unpatched in the future. Unless the packages are cleaned up after upgrade (haven't dug that deep yet, on mobile)...

jeffmcutter commented 5 months ago

ansible.builtin.package as per the documentation "acts as a proxy to the underlying package manager module. While all arguments will be passed to the underlying module, not all modules support the same arguments. This documentation only covers the minimum intersection of module arguments that all packaging modules support."

This means you need to refer to the underlying module to know what options are valid. They are not documented in the package module.

https://docs.ansible.com/ansible/latest/collections/ansible/builtin/package_module.html#synopsis

I have changed my activation key for RHEL 7 leapp upgrades to only enable the 1 main repo:

[root@satrhel7-1 ~]# subscription-manager repos --list-enabled
+----------------------------------------------------------+
    Available Repositories in /etc/yum.repos.d/redhat.repo
+----------------------------------------------------------+
Repo ID:   rhel-7-server-rpms
Repo Name: Red Hat Enterprise Linux 7 Server (RPMs)
Repo URL:  https://satcap1.localdomain.local/pulp/content/my_org/Dev/rhel7/content/dist/rhel/server/7/7Server/$basearch/os
Enabled:   1
[root@satrhel7-1 ~]# 

I uninstalled all leapp packages, and re-ran analysis using the EE from AAP 2.4, 2.3, and 2.1, uninstalling leapp after each time. All 3 test OK. The leapp packages get installed. I verified during that only the one repo was still enabled.

TASK [infra.leapp.analysis : analysis-leapp | Install packages for preupgrade analysis on RHEL 7] ***
changed: [satrhel7-1.localdomain.local]
Friday 26 April 2024  01:42:10 +0000 (0:00:05.680)       0:00:23.296 **********

With regards to the concern of the packages not getting updated in the future, they are removed post upgrade:

https://github.com/redhat-cop/infra.leapp/blob/main/roles/upgrade/tasks/leapp-post-upgrade.yml#L47

Also, once at RHEL 8, there is no extras repository, base and appstream are there by default, and leapp is in the appstream.

With all that said, infra.leapp is obviously only for yum/dnf (RHEL) based systems and ansible.builtin.apt doesn't support enablerepo, so this couldn't be cross platform even if it were applicable. However, ansible.builtin.package is used several other places in infra.leapp, including with enablerepo for analysis-preupg.yml. I think unless we have evidence that there is a problem with using package that we should stay with package universally. At the very least it saves us having to decide whether we should use yum or dnf. If we have evidence that package doesn't work, then I'd like to see it and we should get a bug opened up on it.

k3mic commented 5 months ago

Interesting, I didn't realize the package module worked like that... This issue can probably be closed. We found some other problems related to the upgrade activation key we were using. The specific error we were hitting was on this package task. I had assumed it could be related to that enablerepo bit...