Skip to content
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

Fixes #38163 - Pass installable advisories only to install command #11290

Conversation

hairmare
Copy link
Contributor

What are the changes introduced in this pull request?

Filter Errata via check_installable_for_host before passing them to the install command.

Prevents generating a command with all the errata that will result in "Argument list too long".

Considerations taken when implementing this change?

The arg was just added in Sept 2024 in ec14209, I'm only changing it for Ansible because puppet probably works as intended.

What are the testing steps for this pull request?

  • Configure Foreman/Katello to use Ansible for all actions
  • Wait for a host to be affected by more than one Errata
  • Apply several Errata at once via Errata tab in the content UI
  • The resulting job applies all installable errata

@hairmare hairmare marked this pull request as ready for review January 26, 2025 10:19
@hairmare hairmare changed the title Fixes #38163 - Pass only installable advisories to install command Fixes #38163 - Pass installable advisories only to install command Jan 26, 2025
@chris1984 chris1984 merged commit e6f6e43 into Katello:master Jan 27, 2025
19 checks passed
@ianballou
Copy link
Member

ianballou commented Jan 29, 2025

I'm concerned that this fix here doesn't get to the root issue -- why were there so many advisories being added to the dnf call that it caused an Argument list too long error? Was the errata search not refined enough?

The search for applicable errata in the job template was specifically set to false so that dnf would be the source of truth rather than Katello for what errata can be installed. This functionality should be separate from the issue of limiting how many errata can be passed into dnf.

As is, this change likely reverts the fix for https://projects.theforeman.org/issues/37831. If a user recently changed repository sets and tries applying some errata via the UI, they're going to find that the errata they choose in the UI differ from what the job tempate will let them do, which is a confusing user experience. This is due to the fact that changing repository sets, by design, does not update the host's bound_repositories.

@ianballou
Copy link
Member

ianballou commented Jan 29, 2025

To address the issue more directly, I'd prefer that we implement running multiple dnf commands via the job template to get through all of the listed errata.

@jeremylenz
Copy link
Member

Where does the "Argument list too long" come from? I remember testing DNF with over 5000 arguments and it worked.

@hairmare
Copy link
Contributor Author

hairmare commented Jan 29, 2025

Thanks for the feedback. I think i understand the regression i've introduced.

I also checked back with the colleague that initially ran into the issue, I might be able to narrow down how selecting all errata instead of applicable errata can be triggered.

We are applying errata via the errata tab in content:

image

If we select all the errata by using the "select all" checkbox at the top right, we get the argument list too long error. Before applying them the selection looks like this:

image

Applying then generates the failing Job named "Install errata".

If we select the errata individually by checking the box on each row, the the generated Job is called "Install errata errata_id ^ (ALSA-2025:0334,ALSA-2025:0377)" and works as intended. The UI looks the same before applying the job.

I'm not sure if looping over the errata to generate multiple dnf command will work. I would assume that such a job would take very long and do a ton of no-op dnf calls.

I'd be happy to contribute a fix, but will have to dig a bit deeper to find the code paths that lead to selecting all errata from the whole system. As it is looking like frontend code could be involved, this might take me a while.

Would you like me to prepare a revert commit in the meantime?

@hairmare
Copy link
Contributor Author

Also, i quickly analyzed the failing output. When we initially encountered this, it was trying to apply 17872 erratas.

@hairmare hairmare deleted the fix/template/job_templates/install_errata_by_search_query-ansible/upgrade-all-errata branch January 29, 2025 19:38
@ianballou
Copy link
Member

Thanks for the further analysis, @hairmare !

If we select all the errata by using the "select all" checkbox at the top right, we get the argument list too long error.
...
Applying then generates the failing Job named "Install errata".

With the info you provided here, I think the bug is as follows:

  1. Selecting all errata creates a remote execution job with an empty errata search string (we often use empty search => return all items)
  2. The errata installation template uses the empty search query in @host.advisory_ids
  3. @host.advisory_ids returns a ton of errata that don't actually apply to your host.

I think we have two options. Either:

  1. Have the select-all case in the UI actually gather all errata IDs and send them into the search query, or,
  2. Keep the fix here and also apply it to app/views/foreman/job_templates/install_errata_by_search_query.erb

Would you like me to prepare a revert commit in the meantime?

The situation here is trickier than I expected, so I think we can wait on reverting until we're more clear on the fix strategy.

@jeremylenz
Copy link
Member

jeremylenz commented Jan 29, 2025

  1. Have the select-all case in the UI actually gather all errata IDs and send them into the search query

This won't work because the UI doesn't have all the errata IDs when you select all. It only has those from the current page, since our API is paginated and doesn't give you all of the thousands of results.

I think the fix should go here -->

errata_scope = check_installable_for_host ? ::Katello::Erratum.installable_for_hosts([self]) : ::Katello::Erratum

and should be based on applicable_for_hosts rather than ::Katello::Erratum.

@ianballou
Copy link
Member

Let's use https://projects.theforeman.org/issues/38175 for the new fix

@ianballou
Copy link
Member

and should be based on applicable_for_hosts rather than ::Katello::Erratum.

It looks like installable_for_hosts should actually be called applicable_for_hosts. As such, I think we can just apply the same fix here for the non-ansible version of this template.

@ianballou
Copy link
Member

I created a PR: #11295

Thanks for finding this @hairmare , it's a good bug. The PR just builds on your fix for non-ansible users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants