Refs #39448 - improve performance on host index page

app/models/concerns/foreman_openscap/host_extensions.rb

@@ -129,12 +129,12 @@ def reports_for_policy(policy, limit = nil)
       report_scope
     end
 
-    def compliance_status(options = {})
-      @compliance_status ||= get_status(ForemanOpenscap::ComplianceStatus).to_status(options)
+    def compliance_status(_options = {})
+      @compliance_status ||= get_status(ForemanOpenscap::ComplianceStatus).status
     end
 
-    def compliance_status_label(options = {})
-      @compliance_status_label ||= get_status(ForemanOpenscap::ComplianceStatus).to_label(options)
+    def compliance_status_label(_options = {})
+      @compliance_status_label ||= get_status(ForemanOpenscap::ComplianceStatus).to_label
     end
 
     def openscap_proxy_in_taxonomy

app/models/foreman_openscap/asset.rb

@@ -1,7 +1,9 @@
 module ForemanOpenscap
   class Asset < ApplicationRecord
     has_many :asset_policies, :dependent => :delete_all
-    has_many :policies, :through => :asset_policies
+    has_many :policies, :through => :asset_policies,
+                        :after_add => :reset_compliance_status,
+                        :after_remove => :reset_compliance_status
     belongs_to :assetable, :polymorphic => true
 
     scope :hosts, lambda { where(:assetable_type => 'Host::Base') }
@@ -20,6 +22,24 @@ def name
 
     private
 
+    def reset_compliance_status(_policy)
+      ForemanOpenscap::ComplianceStatusResetter.to_inconclusive(host_ids_reached_by_asset)
+    end
+
+    def host_ids_reached_by_asset
+      case assetable_type
+      when 'Host::Base'
+        [assetable_id]
+      when 'Hostgroup'
+        hostgroup = Hostgroup.find_by(:id => assetable_id)
+        return [] unless hostgroup
+
+        Host.where(:hostgroup_id => hostgroup.subtree_ids).pluck(:id)
+      else
+        []
+      end
+    end
+
     def fetch_asset(type)
       assetable if assetable_type == type
     end

app/models/foreman_openscap/compliance_status.rb

@@ -16,8 +16,8 @@ def self.bit_mask(status)
       "#{ArfReport::BIT_NUM * ArfReport::METRIC.index(status)} & #{ArfReport::MAX}"
     end
 
-    def to_label(options = {})
-      case to_status
+    def to_label(_options = {})
+      case status
       when COMPLIANT
         N_('Compliant')
       when INCONCLUSIVE
@@ -29,8 +29,8 @@ def to_label(options = {})
       end
     end
 
-    def to_global(options = {})
-      case to_status
+    def to_global(_options = {})
+      case status
       when COMPLIANT
         ::HostStatus::Global::OK
       when INCONCLUSIVE
@@ -40,17 +40,29 @@ def to_global(options = {})
       end
     end
 
-    def relevant?(options = {})
+    def relevant?(_options = {})
       # May fail host status during migration
       return false unless ForemanOpenscap::Asset.table_exists?
-      host.combined_policies.present?
+      host.policies.any? || host_has_hostgroup_policies?
     end
 
-    def to_status(options = {})
+    def to_status(_options = {})
       latest_reports = host.combined_policies.flat_map { |p| host.last_report_for_policy p }
       return INCOMPLIANT if latest_reports.any?(&:failed?)
       return INCONCLUSIVE if latest_reports.any?(&:othered?)
       COMPLIANT
     end
+
+    private
+
+    def host_has_hostgroup_policies?
+      return false if host.hostgroup_id.blank?
+
+      ids = [host.hostgroup_id] + host.hostgroup.ancestor_ids
+      ForemanOpenscap::Policy.joins(:assets)
+                             .where('foreman_openscap_assets.assetable_type' => 'Hostgroup',
+                                    'foreman_openscap_assets.assetable_id' => ids)
+                             .exists?
+    end
   end
 end

app/models/foreman_openscap/policy.rb

@@ -42,6 +42,7 @@ def self.deploy_by_variants
     validate :valid_tailoring, :valid_tailoring_profile, :no_mixed_deployments
     validate :valid_cron_line, :valid_weekday, :valid_day_of_month, :if => Proc.new { |policy| policy.should_validate?('Schedule') }
     after_save :assign_policy_to_hostgroups
+    after_save :reset_changed_assignment_hosts_to_inconclusive
     # before_destroy - ensure that the policy has no hostgroups, or classes
 
     default_scope do
@@ -179,10 +180,14 @@ def unassign_hosts(hosts)
         :assetable_type => 'Host::Base',
         :assetable_id => hosts.map(&:id),
         :foreman_openscap_asset_policies => { :policy_id => id }
-      ).pluck(:id)
+      ).pluck(:id, :assetable_id)
 
-      self.asset_ids = self.asset_ids - policy_host_assets
-      ForemanOpenscap::Asset.where(:id => policy_host_assets).destroy_all
+      asset_ids = policy_host_assets.map(&:first)
+      host_ids_to_reset = policy_host_assets.map(&:second)
+
+      self.asset_ids = self.asset_ids - asset_ids
+      ForemanOpenscap::Asset.where(:id => asset_ids).destroy_all
+      ForemanOpenscap::ComplianceStatusResetter.to_inconclusive(host_ids_to_reset)
     end
 
     def to_enc
@@ -251,12 +256,47 @@ def profile_for_scan
     end
 
     def assign_ids(ids, class_name)
+      before_host_ids = host_ids_reached_by_assignments(class_name)
       new_assets = ids.uniq.reject { |id| id.respond_to?(:empty?) && id.empty? }.reduce([]) do |memo, id|
         memo << assets.where(:assetable_type => class_name, :assetable_id => id).first_or_initialize
       end
       complimentary_class_name = class_name == 'Host::Base' ? 'Hostgroup' : 'Host::Base'
       existing_assets = self.assets.select { |assigned_asset| assigned_asset.assetable_type == complimentary_class_name }
       self.assets = existing_assets + new_assets
+      after_host_ids = host_ids_reached_by_assignments(class_name, self.assets)
+
+      return unless host_assignment_changed?(before_host_ids, after_host_ids)
+
+      remember_hosts_for_compliance_status_reset(before_host_ids + after_host_ids)
+    end
+
+    def host_ids_reached_by_assignments(class_name, assigned_assets = assets)
+      assetable_ids = assigned_assets.select { |asset| asset.assetable_type == class_name }.map { |asset| asset.assetable_id.to_i }
+
+      if class_name == 'Host::Base'
+        assetable_ids
+      else
+        hostgroup_ids = ::Hostgroup.where(:id => assetable_ids).flat_map(&:subtree_ids).uniq
+        ::Host.where(:hostgroup_id => hostgroup_ids).pluck(:id)
+      end
+    end
+
+    def remember_hosts_for_compliance_status_reset(host_ids)
+      @compliance_status_reset_host_ids ||= []
+      @compliance_status_reset_host_ids.concat(host_ids)
+    end
+
+    def host_assignment_changed?(before_host_ids, after_host_ids)
+      before_host_ids.to_set != after_host_ids.to_set
+    end
+
+    def reset_changed_assignment_hosts_to_inconclusive
+      host_ids = Array(@compliance_status_reset_host_ids).compact.uniq
+      return if host_ids.empty?
+
+      ForemanOpenscap::ComplianceStatusResetter.to_inconclusive(host_ids)
+    ensure
+      @compliance_status_reset_host_ids = nil
     end
 
     def no_mixed_deployments

app/services/foreman_openscap/compliance_status_resetter.rb

@@ -0,0 +1,20 @@
+module ForemanOpenscap
+  class ComplianceStatusResetter
+    def self.to_inconclusive(host_ids)
+      host_ids = Array(host_ids).compact.uniq
+      return if host_ids.empty?
+
+      now = Time.current
+      rows = host_ids.map do |host_id|
+        {
+          :host_id => host_id,
+          :type => ComplianceStatus.to_s,
+          :status => ComplianceStatus::INCONCLUSIVE,
+          :reported_at => now
+        }
+      end
+
+      ComplianceStatus.upsert_all(rows, :unique_by => %i[type host_id], :update_only => %i[status reported_at])
+    end
+  end
+end

test/test_plugin_helper.rb

@@ -59,6 +59,13 @@ def add_smart_proxy
 module ScapTestCommon
   private
 
+  def set_compliance_status(host, status)
+    compliance_status = host.get_status(ForemanOpenscap::ComplianceStatus)
+    compliance_status.status = status
+    compliance_status.reported_at = Time.current
+    compliance_status.save!
+  end
+
   def create_report_with_rules(host, rule_names, rule_results)
     raise "rule_names and rule_results should have the same length!" if rule_names.size != rule_results.size
     metrics = {

test/unit/compliance_status_test.rb

@@ -74,12 +74,48 @@ def setup
     status = ForemanOpenscap::ComplianceStatus.new
     host = FactoryBot.create(:compliance_host, :policies => [@policy_a])
 
+    host.expects(:combined_policies).never
     status.host = host
 
     assert status.relevant?
   end
 
-  test 'relevant? should be true for hostgroup policies and to_status should use them' do
+  test 'relevant? should be false for host without policies' do
+    status = ForemanOpenscap::ComplianceStatus.new
+    host = FactoryBot.create(:host)
+
+    status.host = host
+
+    refute status.relevant?
+  end
+
+  test 'relevant? should be false for host with hostgroup but no policies' do
+    status = ForemanOpenscap::ComplianceStatus.new
+    host = FactoryBot.create(:host, :with_hostgroup)
+
+    status.host = host
+
+    refute status.relevant?
+  end
+
+  test 'relevant? should be true for hostgroup policies without loading combined_policies' do
+    status = ForemanOpenscap::ComplianceStatus.new
+    host = FactoryBot.create(:host, :with_hostgroup)
+
+    assert_empty host.policies
+    @policy_a.hostgroup_ids = [host.hostgroup.id]
+    assert @policy_a.save
+
+    host.reload
+    assert_includes host.combined_policies, @policy_a
+
+    status.host = host
+    host.expects(:combined_policies).never
+
+    assert status.relevant?
+  end
+
+  test 'to_status should use hostgroup policies' do
     status = ForemanOpenscap::ComplianceStatus.new
     host = FactoryBot.create(:host, :with_hostgroup)
     othered_status = { :passed => 0, :failed => 0, :othered => 1 }.with_indifferent_access
@@ -95,12 +131,10 @@ def setup
     FactoryBot.create(:policy_arf_report, :policy_id => @policy_a.id, :arf_report_id => report.id)
 
     status.host = host
-
-    assert status.relevant?
     assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, status.to_status
   end
 
-  test 'relevant? should be true for inherited parent hostgroup policies' do
+  test 'relevant? should be true for inherited parent hostgroup policies without loading combined_policies' do
     status = ForemanOpenscap::ComplianceStatus.new
     host = FactoryBot.create(:host, :with_hostgroup)
     hostgroup = host.hostgroup
@@ -116,7 +150,22 @@ def setup
     assert_includes host.combined_policies, @policy_a
 
     status.host = host
+    host.expects(:combined_policies).never
 
     assert status.relevant?
   end
+
+  test 'to_label returns label based on stored status' do
+    status = ForemanOpenscap::ComplianceStatus.new
+    status.status = ForemanOpenscap::ComplianceStatus::COMPLIANT
+    status.expects(:to_status).never
+    assert_equal 'Compliant', status.to_label
+  end
+
+  test 'to_global returns global status based on stored status' do
+    status = ForemanOpenscap::ComplianceStatus.new
+    status.status = ForemanOpenscap::ComplianceStatus::COMPLIANT
+    status.expects(:to_status).never
+    assert_equal ::HostStatus::Global::OK, status.to_global
+  end
 end

test/unit/concerns/host_extensions_test.rb

@@ -162,6 +162,40 @@ class HostExtensionsTest < ActiveSupport::TestCase
     assert_empty ForemanOpenscap::Asset.where(:assetable_id => host.id, :assetable_type => 'Host::Base')
   end
 
+  test 'should reset compliance status when assigning policy through host policies' do
+    host = FactoryBot.create(:compliance_host)
+    set_compliance_status(host, ForemanOpenscap::ComplianceStatus::COMPLIANT)
+
+    host.policies = [@policy]
+
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, host.reload.compliance_status
+  end
+
+  test 'should reset compliance status when removing policy through host policies' do
+    host = FactoryBot.create(:compliance_host, :policies => [@policy])
+    set_compliance_status(host, ForemanOpenscap::ComplianceStatus::COMPLIANT)
+
+    host.policies = []
+
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, host.reload.compliance_status
+  end
+
+  test 'compliance_status returns stored persisted status without recalculation' do
+    set_compliance_status(@host, ForemanOpenscap::ComplianceStatus::INCONCLUSIVE)
+    status = @host.reload.get_status(ForemanOpenscap::ComplianceStatus)
+    status.expects(:to_status).never
+
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, @host.compliance_status
+  end
+
+  test 'compliance_status_label returns label from stored persisted status without recalculation' do
+    set_compliance_status(@host, ForemanOpenscap::ComplianceStatus::INCOMPLIANT)
+    status = @host.reload.get_status(ForemanOpenscap::ComplianceStatus)
+    status.expects(:to_status).never
+
+    assert_equal 'Incompliant', @host.compliance_status_label
+  end
+
   private
 
   def setup_hosts_with_policy

test/unit/concerns/hostgroup_extensions_test.rb

@@ -7,4 +7,39 @@ class HostgroupExtensionsTest < ActiveSupport::TestCase
     asset_scope = ::ForemanOpenscap::Asset.where(:assetable_id => hostgroup.id, :assetable_type => 'Hostgroup')
     assert_difference("asset_scope.count", -3) { hostgroup.destroy }
   end
+
+  test "should reset compliance status for subtree when assigning policy through hostgroup asset policies" do
+    parent = FactoryBot.create(:hostgroup)
+    child = FactoryBot.create(:hostgroup, :ancestry => parent.id.to_s)
+    parent_host = FactoryBot.create(:compliance_host, :hostgroup_id => parent.id)
+    child_host = FactoryBot.create(:compliance_host, :hostgroup_id => child.id)
+    policy = FactoryBot.create(:policy)
+    asset = FactoryBot.create(:asset, :assetable_id => parent.id, :assetable_type => 'Hostgroup')
+
+    set_compliance_status(parent_host, ForemanOpenscap::ComplianceStatus::COMPLIANT)
+    set_compliance_status(child_host, ForemanOpenscap::ComplianceStatus::COMPLIANT)
+
+    asset.policies = [policy]
+
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, parent_host.reload.compliance_status
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, child_host.reload.compliance_status
+  end
+
+  test "should reset compliance status for subtree when removing policy through hostgroup asset policies" do
+    parent = FactoryBot.create(:hostgroup)
+    child = FactoryBot.create(:hostgroup, :ancestry => parent.id.to_s)
+    parent_host = FactoryBot.create(:compliance_host, :hostgroup_id => parent.id)
+    child_host = FactoryBot.create(:compliance_host, :hostgroup_id => child.id)
+    policy = FactoryBot.create(:policy)
+    asset = FactoryBot.create(:asset, :assetable_id => parent.id, :assetable_type => 'Hostgroup')
+    asset.policies = [policy]
+
+    set_compliance_status(parent_host, ForemanOpenscap::ComplianceStatus::COMPLIANT)
+    set_compliance_status(child_host, ForemanOpenscap::ComplianceStatus::COMPLIANT)
+
+    asset.policies = []
+
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, parent_host.reload.compliance_status
+    assert_equal ForemanOpenscap::ComplianceStatus::INCONCLUSIVE, child_host.reload.compliance_status
+  end
 end