Generate single-host reports without shell commands or rake task

[jeremylenz]

What are the changes introduced in this pull request?

As part of the IoP Vulnerability service, package profile upload triggers a single-host inventory report upload. Previously this was accomplished the same way as other inventory reports, meaning it was run in a shell process which ran a rake task, and report generation output was captured for display in the web UI. However, with hundreds or thousands of hosts it seems this caused large performance bottlenecks.

This change removes some of that overhead by extracting the report generation to a new Dynflow task and running it directly. This means no shell process and no rake task, hopefully eliminating that bloat and improving performance.

Considerations taken when implementing this change?

Both this and BulkGenerateApplicability are triggered by package profile upload and can run concurrently. But since inventory reports only require an updated package profile and not updated applicability calculation, I think this is fine.

I had to fix a line in slice.rb since my host didn't have a content source

What are the testing steps for this pull request?

yum -y upgrade jq or yum -y downgrade jq
Watch Foreman logs and verify that ForemanInventoryUpload::Async::SingleHostReportJob completes with success
Inspect Dynflow console inputs and outputs and make sure everything looks ok
Extract the report tar and make sure it looks ok.

Summary by Sourcery

Support generating single-host inventory reports via a new async job pipeline, replace shell/rake-based approach, and improve yum repo URL handling.

New Features:

• Introduce SingleHostReportJob to orchestrate asynchronous single-host report generation and upload
• Add GenerateSingleHostReport action to render archived report for a specific host
• Update host report endpoint to invoke SingleHostReportJob instead of the generic GenerateReportJob

Enhancements:

• Provide fallback to SmartProxy primary pulp_content_url for yum repository reporting when content_source is unavailable

[sourcery-ai]

Reviewer's Guide

Refactor host-specific report workflow to eliminate shell commands and rake tasks by introducing a dedicated asynchronous pipeline (SingleHostReportJob and GenerateSingleHostReport) for single-host reports, plus add a fallback in the slice generator for missing pulp URLs.

File-Level Changes

Change	Details	Files
Switch host report generation to use SingleHostReportJob instead of bulk report job	Replace GenerateReportJob with SingleHostReportJob Remove false flag and inline host ID argument	app/controllers/concerns/insights_cloud/package_profile_upload_extensions.rb
Add fallback for missing pulp content URL in slice generator	Use safe navigation operator on content_source Fallback to SmartProxy primary pulp_content_url if nil	lib/foreman_inventory_upload/generators/slice.rb
Introduce SingleHostReportJob to orchestrate single-host report generation and upload	Define plan steps for GenerateSingleHostReport and QueueForUploadJob Implement host lookup, humanized name, organization and filter helpers	lib/foreman_inventory_upload/async/single_host_report_job.rb
Implement GenerateSingleHostReport action for actual report rendering	Plan method to initialize and invoke ArchivedReport generator Finalize outputs result path and organization info	lib/foreman_inventory_upload/async/generate_single_host_report.rb

Possibly linked issues

• Improve test coverage - Report Generate #31: The PR refactors the single-host report generation mechanism, which is failing in the issue, potentially resolving the NoMethodError during report generation.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `lib/foreman_inventory_upload/async/generate_single_host_report.rb:10-12` </location>
<code_context>
+          filter: filter,
+        )
+        input[:target] = File.join(base_folder, ForemanInventoryUpload.facts_archive_name(input[:organization_id], input[:filter]))
+        archived_report_generator = ForemanInventoryUpload::Generators::ArchivedReport.new(input[:target])
+        archived_report_generator.render(organization: input[:organization_id], filter: input[:filter])
+      end
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Consider error handling for file creation and report generation.

Currently, exceptions from file creation or report generation are not handled. Please add error handling or logging to address possible failures.

```suggestion
        begin
          input[:target] = File.join(base_folder, ForemanInventoryUpload.facts_archive_name(input[:organization_id], input[:filter]))
          archived_report_generator = ForemanInventoryUpload::Generators::ArchivedReport.new(input[:target])
          archived_report_generator.render(organization: input[:organization_id], filter: input[:filter])
        rescue => e
          Rails.logger.error("Failed to generate archived report for organization #{input[:organization_id]}: #{e.message}\n#{e.backtrace.join("\n")}")
          output[:error] = "Report generation failed: #{e.message}"
          raise
        end
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[jeremylenz]

@adamruzicka I can't tag you for review, but have a look :)

[jeremylenz]

I didn't realize that QueueForUploadJob calls UploadReportJob which also uses ShellProcess and uploads via uploader.sh. I'll have to think about what to do about that.

[parthaa]

I do like the approach here. My only issue is we cant see the Shell output in Administer -> Inventory Upload (which is not a big deal imo).
We should use the same approach for Upload (may be in a separate PR.)

[parthaa]

where are you accounting for this logic
https://github.com/theforeman/foreman_rh_cloud/blob/develop/lib/tasks/rh_cloud_inventory.rake#L52-L66

Added by 15f9c55

[parthaa]

Also I dont like duplicating the logic for both rake and actions. Can you dry that part up. For example you can have this part of the logic common to both your action ad the rake task

foreman_rh_cloud/lib/tasks/rh_cloud_inventory.rake

Lines 43 to 65 in 6ba56e7

	organizations.each do |organization|
	target = File.join(base_folder, ForemanInventoryUpload.facts_archive_name(organization, filter))
	archived_report_generator = ForemanInventoryUpload::Generators::ArchivedReport.new(target, Logger.new(STDOUT))
	archived_report_generator.render(organization: organization, filter: filter)
	puts "Successfully generated #{target} for organization id #{organization}"
	next unless ForemanRhCloud.with_iop_smart_proxy?
	puts 'Creating missing insights facets'
	hosts_without_facets = ForemanInventoryUpload::Generators::Queries.for_org(organization, hosts_query: 'null? insights_uuid')
	hosts_without_facets.each do |batch|
	facets = batch.pluck(:id, 'katello_subscription_facets.uuid').map do |host_id, uuid|
	{
	host_id: host_id,
	uuid: uuid,
	}
	end
	# We don't need to validate the facets here as we create the necessary fields.
	# rubocop:disable Rails/SkipsModelValidations
	InsightsFacet.upsert_all(facets, unique_by: :host_id) unless facets.empty?
	# rubocop:enable Rails/SkipsModelValidations
	end
	puts 'Missing Insights facets created'

[adamruzicka]

Definitely a step in the right direction

I didn't realize that QueueForUploadJob calls UploadReportJob which also uses ShellProcess and uploads via uploader.sh. I'll have to think about what to do about that.

Ideally yes. Firing up a shell with curl is a relatively cheap thing to do, so I wouldn't expect any major performance gains, but it would be the more correct thing to do.

[jeremylenz]

where are you accounting for this logic https://github.com/theforeman/foreman_rh_cloud/blob/develop/lib/tasks/rh_cloud_inventory.rake#L52-L66

next unless ForemanRhCloud.with_iop_smart_proxy?

Seems I misread that unless as an if. Will update

[jeremylenz]

We should use the same approach for Upload (may be in a separate PR.)

Agree, let's do the upload in a followup.

[ShimShtein]

As a concept I like it. There are small changes that will make it a bit more adapted to the foreman tasks paradigm, but in general it looks good.

[jeremylenz]

@adamruzicka @parthaa @ShimShtein Updated 👍

[parthaa]

basically 'rh_cloud_inventory::generate' was never meant to upload (it was to only generate). So my suggested patch

$ git diff
diff --git a/lib/foreman_inventory_upload/async/host_inventory_report_job.rb b/lib/foreman_inventory_upload/async/host_inventory_report_job.rb
index c4cb9e9..d6bd97f 100644
--- a/lib/foreman_inventory_upload/async/host_inventory_report_job.rb
+++ b/lib/foreman_inventory_upload/async/host_inventory_report_job.rb
@@ -1,7 +1,7 @@
 module ForemanInventoryUpload
   module Async
     class HostInventoryReportJob < ::Actions::EntryAction
-      def plan(base_folder, organization_id, hosts_filter = "")
+      def plan(base_folder, organization_id, hosts_filter = "", upload = true)
         sequence do
           plan_action(
             GenerateHostReport,
@@ -10,12 +10,14 @@ module ForemanInventoryUpload
             hosts_filter
           )
 
-          plan_action(
-            QueueForUploadJob,
-            base_folder,
-            ForemanInventoryUpload.facts_archive_name(organization_id, hosts_filter),
-            organization_id
-          )
+          if upload
+            plan_action(
+              QueueForUploadJob,
+              base_folder,
+              ForemanInventoryUpload.facts_archive_name(organization_id, hosts_filter),
+              organization_id
+            )
+          end
 
           if ForemanRhCloud.with_iop_smart_proxy?
             plan_action(
diff --git a/lib/tasks/rh_cloud_inventory.rake b/lib/tasks/rh_cloud_inventory.rake
index 212d31f..b3bb10c 100644
--- a/lib/tasks/rh_cloud_inventory.rake
+++ b/lib/tasks/rh_cloud_inventory.rake
@@ -45,7 +45,8 @@ namespace :rh_cloud_inventory do
             ForemanInventoryUpload::Async::HostInventoryReportJob,
             base_folder,
             organization_id,
-            filter
+            filter,
+            false
           )
         end
         puts "Check the Uploading tab for report uploading status." if Setting[:subscription_connection_enabled]

[jeremylenz]

updated & squashed

[parthaa]

One more suggested patch. I hit some errors when testing the rake rh_cloud_inventory calls (not connected to changes in this PR but related to IoP in the same file.)

diff --git a/lib/foreman_inventory_upload/async/generate_report_job.rb b/lib/foreman_inventory_upload/async/generate_report_job.rb
index 86bf91f..c474415 100644
--- a/lib/foreman_inventory_upload/async/generate_report_job.rb
+++ b/lib/foreman_inventory_upload/async/generate_report_job.rb
@@ -5,7 +5,7 @@ module ForemanInventoryUpload
         "report_for_#{label}"
       end
 
-      def plan(base_folder, organization_id, disconnected, hosts_filter = nil)
+      def plan(base_folder, organization_id, disconnected = false, hosts_filter = nil)
         sequence do
           super(
             GenerateReportJob.output_label("#{organization_id}#{hosts_filter.empty? ? nil : "[#{hosts_filter.to_s.parameterize}]"}"),
diff --git a/lib/tasks/rh_cloud_inventory.rake b/lib/tasks/rh_cloud_inventory.rake
index 616001d..41bc1fc 100644
--- a/lib/tasks/rh_cloud_inventory.rake
+++ b/lib/tasks/rh_cloud_inventory.rake
@@ -9,14 +9,12 @@ namespace :rh_cloud_inventory do
       else
         organizations = [Organization.where(:id => ENV['organization_id']).first]
       end
-      disconnected = ForemanRhCloud.with_iop_smart_proxy?
       User.as_anonymous_admin do
         organizations.each do |organization|
           ForemanTasks.async_task(
             ForemanInventoryUpload::Async::GenerateReportJob,
             ForemanInventoryUpload.generated_reports_folder,
-            organization.id,
-            disconnected
+            organization.id
           )
           puts "Generated and uploaded inventory report for organization '#{organization.name}'"
         end
@@ -57,8 +55,7 @@ namespace :rh_cloud_inventory do
       base_folder = ENV['target'] || ForemanInventoryUpload.generated_reports_folder
       organization_id = ENV['organization_id']
       report_file = ForemanInventoryUpload.facts_archive_name(organization_id)
-      disconnected = ForemanRhCloud.with_iop_smart_proxy?
-      ForemanTasks.sync_task(ForemanInventoryUpload::Async::QueueForUploadJob, base_folder, report_file, organization_id, disconnected)
+      ForemanTasks.sync_task(ForemanInventoryUpload::Async::QueueForUploadJob, base_folder, report_file, organization_id)
       puts "Uploaded #{report_file}"
     end
   end

[jeremylenz]

updated

[parthaa]

I 'd like tests for some of these changes in a follow up PR. I am happy with the current state from my testing. I tested

• Inventory Upload => Generate report
• organization_id=1 target=/var/lib/foreman/red_hat_inventory//generated_reports brake rh_cloud_inventory:report:generate
• organization_id=1 brake rh_cloud_inventory:report:upload
• organization_id=1 brake rh_cloud_inventory:report:generate_upload

Going to approve. Please add tests in a follow up.

[chris1984]

🍏