Fix IoP host UUID data consistency issues (#1131)

* Fix IoP host UUID data consistency issues

In IoP mode, hosts previously registered to Red Hat cloud could display
stale insights facet UUIDs instead of their current subscription manager UUIDs.
This caused data inconsistencies when viewing host details, recommendations,
and during unregistration.

Changes:
- Add insights_uuid method that returns correct UUID based on mode
- Add ensure_iop_insights_uuid to sync mismatched UUIDs
- Update all call sites to use insights_uuid method
- Add comprehensive test coverage (14 new tests)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>

* Address code review comments

- Add comment documenting ensure_iop_insights_uuid safety
- Add subscription_facet assertions to factory traits
- Fix guard in ConnectorPlaybookExecutionReporterTask to use insights_uuid
- Add IoP mode test for advisor_engine_controller

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>

* Fix rubocop: add trailing comma

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>

* Add insights facet preloading to avoid N+1 queries

Preload the insights facet when fetching hosts in the API to prevent
N+1 database queries when rendering host lists with facet data.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>

* Address nofar comments

* remove tests no longer needed

---------

Co-authored-by: Claude Sonnet 4.5 <[email protected]>

Author: jeremylenz

app/controllers/concerns/foreman_rh_cloud/registration_manager_extensions.rb

@@ -13,13 +13,13 @@ def unregister_host(host, options = {})
       host.reload
 
       # Only delete from HBI in IoP mode (hosted mode uses async job for cleanup)
-      hbi_host_destroy(host) if ForemanRhCloud.with_iop_smart_proxy? && !organization_destroy && host.insights_facet&.uuid&.presence
+      hbi_host_destroy(host) if ForemanRhCloud.with_iop_smart_proxy? && !organization_destroy && host.insights_uuid.presence
       host.insights&.destroy!
       super(host, options)
     end
 
     def hbi_host_destroy(host)
-      uuid = host.insights_facet.uuid
+      uuid = host.insights_uuid
       logger.debug "Unregistering host #{uuid} from HBI"
       execute_cloud_request(
         organization: host.organization,

app/controllers/concerns/insights_cloud/package_profile_upload_extensions.rb

@@ -21,6 +21,9 @@ def generate_host_report
         @host.id
       )
 
+      # Ensure insights UUID matches subscription UUID (only runs in IoP mode per method guard above)
+      @host.ensure_iop_insights_uuid
+
       # in IoP case, the hosts are identified by the sub-man ID, and we can assume they already
       # exist in the local inventory. This will also handle facet creation for new hosts.
       return if @host.insights

app/models/concerns/rh_cloud_host.rb

@@ -26,5 +26,19 @@ module RhCloudHost
     def insights_facet
       insights
     end
+
+    # In IoP, read directly from the subscription facet to avoid stale data (see comment on ensure_iop_insights_uuid)
+    def insights_uuid
+      ForemanRhCloud.with_iop_smart_proxy? ? subscription_facet&.uuid : insights_facet&.uuid
+    end
+
+    # In non-IoP, insights_facet uuids are assigned by Hosted.
+    # In IoP, insights_facet uuids must match Katello subscription_facet uuids.
+    # If the host was previously registered to hosted Insights,
+    # we need to correct its uuid.
+    def ensure_iop_insights_uuid
+      return unless insights_facet.present? && subscription_facet.present? && insights_facet.uuid != subscription_facet.uuid
+      insights_facet.update!(uuid: subscription_facet.uuid)
+    end
   end
 end

app/views/api/v2/advisor_engine/host_details.json.rabl

@@ -1,9 +1,7 @@
 collection @hosts
 
 attributes :name
-node :insights_uuid do |host|
-  host.insights_facet&.uuid
-end
+node :insights_uuid, &:insights_uuid
 node :insights_hit_details do |host|
   host&.facts('insights::hit_details')&.values&.first
 end

app/views/api/v2/hosts/insights/base.rabl

@@ -1,5 +1,6 @@
-attributes :uuid
-
+node :uuid do |facet|
+  facet&.host&.insights_uuid
+end
 node :insights_hit_details do |facet|
   facet&.host&.facts('insights::hit_details')&.values&.first
 end

lib/foreman_rh_cloud/plugin.rb

@@ -118,6 +118,11 @@ def self.register
           end
         end
 
+        # Preload insights facet to avoid N+1 queries when rendering host list with facets
+        add_controller_action_scope('Api::V2::HostsController', :index) do |base_scope|
+          base_scope.preload(:insights)
+        end
+
         register_global_js_file 'global'
 
         register_custom_status InventorySync::InventoryStatus

lib/insights_cloud/async/connector_playbook_execution_reporter_task.rb

@@ -126,10 +126,10 @@ def report_url
 
       def invocation_status
         hosts_state = Hash[job_invocation.targeting.hosts.map do |host|
-          next unless host.insights&.uuid
+          next unless host.insights_uuid
           [
-            host.insights.uuid,
-            task_status(job_invocation.sub_task_for_host(host), host.insights.uuid),
+            host.insights_uuid,
+            task_status(job_invocation.sub_task_for_host(host), host.insights_uuid),
           ]
         end.compact]
 

test/controllers/insights_cloud/api/advisor_engine_controller_test.rb

@@ -13,14 +13,41 @@ class AdvisorEngineControllerTest < ActionController::TestCase
       end
 
       test 'shows hosts with uuids' do
-        uuids = [@host1.insights.uuid, @host2.insights.uuid]
+        uuids = [@host1.insights_uuid, @host2.insights_uuid]
         get :host_details, params: { organization_id: @test_org.id, host_uuids: uuids }
         assert_response :success
         assert_template 'api/v2/advisor_engine/host_details'
         assert_equal @test_org.hosts.joins(:insights).where(:insights => { :uuid => uuids }).count, assigns(:hosts).count
         refute_equal @test_org.hosts.count, assigns(:hosts).count
       end
 
+      test 'in IoP mode host_details uses subscription uuid when insights uuid is stale' do
+        ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
+
+        stale_insights_uuid = 'stale-insights-uuid-123'
+        subscription_uuid   = 'subscription-uuid-456'
+
+        # Create host with diverging facet UUIDs
+        host = FactoryBot.create(:host, :with_subscription, organization: @test_org)
+        host.subscription_facet.update!(uuid: subscription_uuid)
+        host.insights = FactoryBot.create(:insights_facet, host_id: host.id, uuid: stale_insights_uuid)
+        host.save!
+
+        # Query using the stale insights UUID
+        get :host_details, params: {
+          organization_id: @test_org.id,
+          host_uuids: [stale_insights_uuid],
+        }
+
+        assert_response :success
+        body = JSON.parse(response.body)
+
+        # Should return the subscription UUID, not the stale insights UUID
+        insights_uuids = body.map { |h| h['insights_uuid'] }
+        assert_includes insights_uuids, subscription_uuid, "Should use subscription UUID in IoP mode"
+        refute_includes insights_uuids, stale_insights_uuid, "Should not use stale insights UUID"
+      end
+
       test 'shows error when no hosts found' do
         get :host_details, params: { organization_id: @test_org.id, host_uuids: ['nonexistentuuid'] }
         assert_response :not_found

test/factories/insights_factories.rb

@@ -58,5 +58,34 @@
         host.insights = FactoryBot.create(:insights_facet, :with_hits, host_id: host.id)
       end
     end
+
+    trait :with_mismatched_insights_uuid do
+      # Simulates host previously registered to cloud with stale UUID
+      # Requires :with_subscription trait to be used together
+      after(:create) do |host, _evaluator|
+        raise "subscription_facet required for :with_mismatched_insights_uuid trait" unless host.subscription_facet&.uuid
+
+        host.insights = FactoryBot.create(
+          :insights_facet,
+          host_id: host.id,
+          uuid: 'stale-cloud-uuid' # Different from subscription_facet
+        )
+      end
+    end
+
+    trait :with_synced_insights_uuid do
+      # Ideal state: insights_facet UUID matches subscription_facet UUID
+      # Requires :with_subscription trait to be used together
+      after(:create) do |host, _evaluator|
+        raise "subscription_facet required for :with_synced_insights_uuid trait" unless host.subscription_facet&.uuid
+
+        subscription_uuid = host.subscription_facet.uuid
+        host.insights = FactoryBot.create(
+          :insights_facet,
+          host_id: host.id,
+          uuid: subscription_uuid  # Same as subscription_facet
+        )
+      end
+    end
   end
 end

test/unit/lib/foreman_rh_cloud/registration_manager_extensions_test.rb

@@ -4,8 +4,8 @@ module ForemanRhCloud
   class RegistrationManagerExtensionsTest < ActiveSupport::TestCase
     setup do
       @org = FactoryBot.create(:organization)
-      @host = FactoryBot.create(:host, :managed, organization: @org)
-      @insights_facet = ::InsightsFacet.create!(host: @host, uuid: 'test-uuid-123')
+      @host = FactoryBot.create(:host, :managed, :with_subscription, organization: @org)
+      @insights_facet = ::InsightsFacet.create!(host: @host, uuid: @host.subscription_facet.uuid)
 
       # Stub Candlepin interaction (from Katello)
       ::Katello::Resources::Candlepin::Consumer.stubs(:destroy)
@@ -15,9 +15,9 @@ class RegistrationManagerExtensionsTest < ActiveSupport::TestCase
     end
 
     context 'unregister_host' do
-      test 'should call HBI delete in IoP mode when host has insights facet with UUID' do
+      test 'should call HBI delete in IoP mode when host has insights UUID' do
         ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
-        expected_url = ForemanInventoryUpload.host_by_id_url('test-uuid-123')
+        expected_url = ForemanInventoryUpload.host_by_id_url(@host.subscription_facet.uuid)
 
         # Expect the cloud request to be made
         Katello::RegistrationManager.expects(:execute_cloud_request).with do |params|
@@ -45,44 +45,6 @@ class RegistrationManagerExtensionsTest < ActiveSupport::TestCase
         assert_nil InsightsFacet.find_by(id: @insights_facet.id)
       end
 
-      test 'should NOT call HBI delete when host has no insights_facet' do
-        host_without_facet = FactoryBot.create(:host, :managed, organization: @org)
-        ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
-
-        Katello::RegistrationManager.expects(:execute_cloud_request).never
-
-        assert_nothing_raised do
-          Katello::RegistrationManager.unregister_host(host_without_facet, unregistering: true)
-        end
-      end
-
-      test 'should NOT call HBI delete when host has insights_facet with empty UUID' do
-        host_with_empty_uuid_facet = FactoryBot.create(:host, :managed, organization: @org)
-        empty_uuid_facet = InsightsFacet.create!(host: host_with_empty_uuid_facet, uuid: '')
-        ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
-
-        Katello::RegistrationManager.expects(:execute_cloud_request).never
-
-        assert_nothing_raised do
-          Katello::RegistrationManager.unregister_host(host_with_empty_uuid_facet, unregistering: true)
-        end
-
-        assert_nil InsightsFacet.find_by(id: empty_uuid_facet.id)
-      end
-
-      test 'should NOT call HBI delete when insights_facet has no UUID' do
-        facet_id = @insights_facet.id
-        @insights_facet.update(uuid: nil)
-        ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)
-
-        Katello::RegistrationManager.expects(:execute_cloud_request).never
-
-        Katello::RegistrationManager.unregister_host(@host, unregistering: true)
-
-        # Verify facet was still destroyed
-        assert_nil InsightsFacet.find_by(id: facet_id)
-      end
-
       test 'should always destroy insights_facet regardless of IoP mode' do
         facet_id = @insights_facet.id
         ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(false)