Fixes #37831 - Don't filter job template install content by bound repos

[ianballou]

What are the changes introduced in this pull request?

Stops RPM, Deb, and Errata installation jobs from limiting content searches to what Katello thinks is applicable to a host. If a user made changes to reposets, the applicability data could be out of date. This change lets dnf rather than Katello be the final determination of whether or not some content is installable.

There's a bonus too -- if the dnf run completes, the package profile will get uploaded, which will in turn update applicability.

Considerations taken when implementing this change?

I couldn't think of any reason why Katello needs to block content installation attempts when dnf can do it itself.

I considered just updating the error message to tell the user that the package profile may need updating. However, I think it's a bit of a technical thing to have users worry about.

What are the testing steps for this pull request?

1. Enable a repository set on a host but don't do anything to get the package profile re-uploaded
   -> We want the package profile to be stale so that packages are installable but Katello doesn't know that
2. Try to install both packages and errata that should be installable even though Katello doesn't think so
3. Without the patch, the job will fail on Katello not being able to find the packages / errata. With the patch, the job should succeed (or at least dnf will be the one to report any issues).
4. Try other package actions, like updating them and removing them with and without stale package profiles
5. Think -- are there any other scenarios that this change might break?

[ianballou]

I'm waiting to see if I need to update any tests.

[jeremylenz]

Remove uses Katello::InstalledPackage, but that list would still be out of date if the package profile is out of date. Can we update this for remove as well?

[jeremylenz]

The line above this still searches the host's installed_packages, and then errors if an installed package is not found. Doesn't this need updating as well?

[ianballou]

Remove uses Katello::InstalledPackage, but that list would still be out of date if the package profile is out of date. Can we update this for remove as well?

The host installed package list being out of date is a different problem from the host bound repository list being out of date in my mind.
Through Katello, a user can invalidate their bound repository list by changing the repo sets. However, they cannot invalidate their installed package list.
Any dnf action should result in the installed packages being uploaded back to Katello. If that list is out of date, then something is wrong with the user's environment (or they uninstalled packages behind dnf with rpm).

I'm mentioning this because, if we want to send the raw package list for updates / removals rather than a list of found packages, we'll need to stop using the search query entirely. If somehow the host has packages installed that are not known to Katello, they may not be in our database at all. In fact, at that point, wouldn't they not show up in the UI anyway?

[ianballou]

I think the best I could do to make a removal / update error less likely would be to have the search look at all of ::Katello::InstalledPackage instead of the installed packages for that actual host. @jeremylenz let me know if that still sounds helpful based on what I mentioned above.

[jeremylenz]

What I was thinking about is this:

The new Packages wizard uses name rather than id for scoped searches. This means that in regular mode, the wizard will give you something like

name ^ (package1, package2)

And in select all mode, it will be

name !^ (package1, package2)

For the select all mode case, we can't really know what the user intent was, so we'd have to error. But in the regular mode case (which should be the majority of jobs) we could simply parse and use the literal list package1 package2 and pass it straight to dnf/yum. (Or if you hate parsing, we could maybe have the frontend send it over both ways.) It won't cover all cases but would at least reduce errors a bit. Thoughts?

cc @parthaa

[ianballou]

For the select all mode case, we can't really know what the user intent was, so we'd have to error. But in the regular mode case (which should be the majority of jobs) we could simply parse and use the literal list package1 package2 and pass it straight to dnf/yum.

Can't this search query be anything, even wildcard matching like name ~ pa ? It feels hacky to use the query string as something other than the query string, especially since users can use the job template outside of the all hosts UI. I might be missing something though?

Is the idea that if we detect the query string looks like full package names then we pass them to dnf directly, otherwise we treat it as a query string?

[jeremylenz]

Can't this search query be anything, even wildcard matching like name ~ pa ? It feels hacky to use the query string as something other than the query string, especially since users can use the job template outside of the all hosts UI. I might be missing something though?

Is the idea that if we detect the query string looks like full package names then we pass them to dnf directly, otherwise we treat it as a query string?

Yeah I mean it's not perfect for sure, but in regular (not select all) mode fetchBulkParams returns the query in a predictable format: https://github.com/theforeman/foreman/blob/92e7ae96383932808fba4a0a7420a69032a09a66/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/TableHooks.js#L274

So it relies on that mostly.

[ianballou]

Yeah I mean it's not perfect for sure, but in regular (not select all) mode fetchBulkParams returns the query in a predictable format: https://github.com/theforeman/foreman/blob/92e7ae96383932808fba4a0a7420a69032a09a66/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/TableHooks.js#L274

So it relies on that mostly.

It feels a little bit fragile, especially if the format of the query string ever changes. I think we need a third opinion, @parthaa ? :)

[jeremylenz]

It could also be done without parsing the query string; it'd just be a bit more work because you'd have to have the frontend send the explicit package names over cleanly. (But again, it would only work in normal mode and not select-all mode, and only when name is used as the id column.)

You're right that simply looking at all available packages is much cleaner. I was just hoping there was a way to avoid erroring altogether and let dnf/yum handle package availability. If you can think of any others I'm open! Otherwise I'm fine going with your approach.

[ianballou]

It could also be done without parsing the query string; it'd just be a bit more work because you'd have to have the frontend send the explicit package names over cleanly. (But again, it would only work in normal mode and not select-all mode, and only when name is used as the id column.)

You're right that simply looking at all available packages is much cleaner. I was just hoping there was a way to avoid erroring altogether and let dnf/yum handle package availability. If you can think of any others I'm open! Otherwise I'm fine going with your approach.

I thought a bit and I think continuing to use the search query as an actual search query would be less risky for the quick turnaround that this PR needs to have. I'm going to improve the chances of finding a package on update/remove by searching for all ::Katello::InstalledPackages rather than limiting the search to those packages associated with the host in question.

I agree that the best long-term solution would be to change the template to not even use a search query and to send the package info directly to DNF, maybe we could do a further redesign when there's more time.

[jeremylenz]

Let's get this in for now, it's better than what we've got. Ack pending finishing of your testing. 👍

[ianballou]

I found an issue with this strategy with how upgrades work -- the full nvra is used for updates rather than just the package name. So, if I don't filter installed packages by host, updating a package on a RHEL 8 host might use the full nvra from a RHEL 10 version of the same package.
The changes are getting more complex, but I'm going to look into not sending the full nvra if no specific upgrade version is specified.

[jeremylenz]

I think an eventual solution could look like

1. Add new job templates for bulk actions only (or maybe we could even reuse the super-old ones)
2. Disable select all mode in the frontend, so we can keep track of package names everywhere
3. Send explicit package names directly to the REX job template input
4. dnf/yum blindly uses the inputs it's given

we could also do something similar for errata.

[ianballou]

The changes are getting more complex, but I'm going to look into not sending the full nvra if no specific upgrade version is specified.

I managed to find a solution that works in my testing. I've pushed the changes.
For upgrades, we use full NVRAs for packages where a specific version was requested. For packages where no specific version was requested (update to latest), I pass only the package name to dnf. With this strategy, I was able to remove the query for packages installable to the host. Instead, I just match ::Katello::Rpms that have the same name and arch as the installed packages on the host that match the update query.

[ianballou]

Tested:

• Install
• Install with stale package profile
• Remove
• Update
• Update with stale package profile
• Update all with a couple hosts