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 #35947 - Consume install_all param when finding hosts #10416

Merged
merged 1 commit into from Feb 9, 2023

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Jan 13, 2023

What are the changes introduced in this pull request?

When selecting all hosts on the Errata page, Bastion was sending an install_all param but the backend was ignoring it. This resulted in an error "No hosts have been specified."

In addition, after fixing that I discovered that Bastion was only sending the ids param and not the search param, resulting in the "all hosts" selection meaning "ALL hosts, not just those with this errata applicable." That should be fixed now too.

Considerations taken when implementing this change?

The bulk errata controller seems to be the only Angular page that sends the install_all param. The name is a bit misleading because it's sent when selecting hosts, not errata.

What are the testing steps for this pull request?

  1. Content > Errata
  2. Select any errata with 1 or more installable hosts, and click Apply Errata
  3. On the host list page, Click the checkbox above "Name" to select the page of results
  4. (Important) A banner appears with a link to "Select all xxx" - Click this to activate select all. The banner will turn green and say "X results are selected."
  5. Click Next and then Confirm

Before this patch: You'd get an error that no hosts have been specified.
After this patch: You'll be taken to the remote execution job.

@jeremylenz jeremylenz changed the title Fixes #35947 - Consume install_all param when bulk applying errata Fixes #35947 - Consume install_all param when finding hosts Jan 13, 2023
@ianballou
Copy link
Member

I'm not reproducing the issue because, with this particular workflow, Katello keeps using katello-agent even though I selected to use remote execution by default. What else did you do to get it to use remote execution?

I'm assuming that the issue only occurs when remote execution is in use. I'm not reproducing the error with katello-agent.

@jeremylenz
Copy link
Member Author

What else did you do to get it to use remote execution?

You shouldn't have to, but you could try disabling katello-agent via the installer with --foreman-proxy-content-enable-katello-agent=false

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a general issue with this workflow -- install_all will try installing errata on hosts that do not have that errata marked as applicable. To reproduce the issue, try having two hosts, but only one has an erratum that is applicable. The remote execution job will be run for both hosts instead of just the one applicable host.

@jeremylenz
Copy link
Member Author

install_all will try installing errata on hosts that do not have that errata marked as applicable. To reproduce the issue, try having two hosts, but only one has an erratum that is applicable. The remote execution job will be run for both hosts instead of just the one applicable host.

Finally got this set up, but I'm not seeing the issue. My REX job is scoped correctly to name ^ (rhel8b.fedora.example.com)..

@ianballou
Copy link
Member

@jeremylenz do you have a host that has no content facet? I have one host with a content facet and one without, that's the only difference I could potentially see with my setup ...

@ianballou
Copy link
Member

You can see the centos8-proxy-devel host on the bottom that has no content facet, which makes it even more confusing why errata is trying to be applied to it.

image

@ianballou
Copy link
Member

So strange, it really is applying it to all of my hosts.

@ianballou
Copy link
Member

ianballou commented Feb 3, 2023

find_bulk_hosts at least is returning my one requested host.

Edit: it looks like this method is being called multiple times, and one of the calls is wrong somehow.

@ianballou
Copy link
Member

In the remote execution controller in Katello, my bulk_host_params looks like this: {:included=>{:ids=>[]}}

In my params I see this: "install_all"=>""

It really doesn't make sense, but it seems the install_all param is failing to be passed along from the UI.

@ianballou
Copy link
Member

ianballou commented Feb 6, 2023

I reproduced the issue I found on a separate Katello development environment that is also on a different computer. I'm using Firefox (but I saw the same in Chrome).

Screencast.from.02-06-2023.05.28.56.PM.webm

@ianballou
Copy link
Member

From debugging:

[8] pry(#<Katello::Api::V2::HostsBulkActionsController>)> params[:install_all]
22:39:47 rails.1   | => nil

[9] pry(#<Katello::Api::V2::HostsBulkActionsController>)> bulk_params[:included][:ids]
22:40:02 rails.1   | => []

[10] pry(#<Katello::Api::V2::HostsBulkActionsController>)> bulk_params[:included][:search]
22:40:17 rails.1   | => "( applicable_errata = \"RHEA-2012:0055\" )"

@ianballou
Copy link
Member

ianballou commented Feb 6, 2023

What's weird is, I don't have install_all, but I do see a param called just "all":

=> #<ActionController::Parameters {"included"=>#<ActionController::Parameters {"ids"=>[], "resources"=>[], "search"=>"( applicable_errata = \"RHEA-2012:0055\" )", "params"=>{"organization_id"=>"1", "paged"=>true, "page"=>1, "per_page"=>"20", "search"=>"( applicable_errata = \"RHEA-2012:0055\" )"}} permitted: false>, "excluded"=>#<ActionController::Parameters {"ids"=>[]} permitted: false>, "all"=>true, "errata_ids"=>["RHEA-2012:0055"], "organization_id"=>"1", "api_version"=>"v2", "controller"=>"katello/api/v2/hosts_bulk_actions", "action"=>"available_incremental_updates", "hosts_bulk_action"=>{"included"=>{"ids"=>[], "resources"=>[], "search"=>"( applicable_errata = \"RHEA-2012:0055\" )", "params"=>{"organization_id"=>"1", "paged"=>true, "page"=>1, "per_page"=>"20", "search"=>"( applicable_errata = \"RHEA-2012:0055\" )"}}, "excluded"=>{"ids"=>[]}, "all"=>true, "errata_ids"=>["RHEA-2012:0055"], "organization_id"=>"1"}} permitted: false>

This :all could be related to this: https://github.com/Katello/katello/blob/master/app/controllers/katello/concerns/api/v2/bulk_extensions.rb#L14

Edit: In my debugging, when I don't have install_all, I do always have all being either true or false in the correct manner.

@jeremylenz
Copy link
Member Author

Hmm.. 🤔

Does a manual host search for "( applicable_errata = \"RHEA-2012:0055\" )" return the expected hosts, or all hosts?

@ianballou
Copy link
Member

Hmm.. thinking

Does a manual host search for "( applicable_errata = \"RHEA-2012:0055\" )" return the expected hosts, or all hosts?

That returns only the correct host.

@jeremylenz
Copy link
Member Author

@ianballou Updated; give it another whirl ♻️

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I tested with page size of 1 so we know that it works across pages. I also tested some of the bulk errata workflows to ensure nothing there was broken.

@ianballou
Copy link
Member

Looks like an angular test is failing

@jeremylenz jeremylenz merged commit 8001e4c into Katello:master Feb 9, 2023
qcjames53 pushed a commit to qcjames53/katello that referenced this pull request Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants