Fixes #1592: making report::expire faster and without errors

app/models/report.rb

@@ -117,47 +117,15 @@ def <=>(other)
   def self.expire(conditions = {})
     timerange = conditions[:timerange] || 1.week
     status = conditions[:status]
-    cond = "created_at < \'#{(Time.now.utc - timerange).to_formatted_s(:db)}\'"
-    cond += " and status = #{status}" unless status.nil?
-    # using find in batches to reduce the memory abuse
-    # trying to be smart about how to delete reports and their associated data, so it would be
-    # as fast as possible without a lot of performance penalties.
-    count = 0
-    Report.find_in_batches(:conditions => cond, :select => :id) do |reports|
-      report_ids = reports.map &:id
-      Log.delete_all({:report_id => report_ids})
-      count += Report.delete_all({:id => report_ids})
-    end
-    # try to find all non used logs, messages and sources
-
-    # first extract all information from our logs
-    all_reports, used_messages, used_sources = [],[],[]
-    Log.find_in_batches do |logs|
-      logs.each do |log|
-        all_reports << log.report_id unless log.report_id.blank?
-        used_messages << log.message_id unless log.message_id.blank?
-        used_sources << log.source_id unless log.source_id.blank?
-      end
-    end
-
-    all_reports.uniq! ; used_messages.uniq! ; used_sources.uniq!
-
-    # reports which have logs entries
-    used_reports = Report.where(:id => all_reports).pluck(:id)
-
-    orphaned_logs = all_reports - used_reports
-    Log.where(:report_id => orphaned_logs).delete_all unless orphaned_logs.empty?
-
-    all_messages = Message.pluck(:id)
-    orphaned_messages = all_messages - used_messages
-    Message.where(:id => orphaned_messages).delete_all unless orphaned_messages.empty?
-
-    all_sources = Source.pluck(:id)
-    orphaned_sources = all_sources - used_sources
-    Source.where(:id => orphaned_sources).delete_all unless orphaned_sources.empty?
+    cond = "reports.created_at < \'#{(Time.now.utc - timerange).to_formatted_s(:db)}\'"
+    cond += " and reports.status = #{status}" unless status.nil?
 
+    Log.joins(:report).where(:report_id => Report.where(cond)).delete_all
+    Message.where("id not IN (#{Log.select(:message_id).to_sql})").delete_all
+    Source.where("id not IN (#{Log.select(:source_id).to_sql})").delete_all
+    count = Report.where(cond).delete_all
     logger.info Time.now.to_s + ": Expired #{count} Reports"
-    return count
+    count
   end
 
   # represent if we have a report --> used to ensure consistency across host report state the report itself

test/factories/reports_related.rb

@@ -5,4 +5,41 @@
     status 0
     metrics YAML.load("--- \n  time: \n    schedule: 0.00083\n    service: 0.149739\n    mailalias: 0.000283\n    cron: 0.000419\n    config_retrieval: 16.3637869358063\n    package: 0.003989\n    filebucket: 0.000171\n    file: 0.007025\n    exec: 0.000299\n  resources: \n    total: 33\n  changes: {}\n  events: \n    total: 0")
   end
+
+  trait :old_report do
+    after_build do |report|
+      report.created_at = 2.weeks.ago
+    end
+  end
+
+  trait :with_logs do
+    ignore do
+      log_count 30
+    end
+    after_create do |report,evaluator|
+      evaluator.log_count.times do
+      FactoryGirl.create(:log, :report => report)
+      end
+    end
+  end
+
+  factory :log do
+    report
+    level_id 1
+    after_build do |log|
+      log.message = FactoryGirl.create(:message)
+      log.source = FactoryGirl.create(:source)
+    end
+  end
+
+  factory :message do
+    sequence(:value) { |n| "message#{n}" }
+    sequence(:digest) { |n| Digest::SHA1.hexdigest("message#{n}") }
+  end
+
+  factory :source do
+    sequence(:value) { |n| "source#{n}" }
+    sequence(:digest) { |n| Digest::SHA1.hexdigest("source#{n}") }
+  end
+
 end

test/unit/report_test.rb

@@ -64,4 +64,18 @@ def setup
     assert Report.with("pending").include?(@r)
   end
 
+  test "should expire reports created 1 week ago" do
+    report_count = 25
+    Message.delete_all
+    Source.delete_all
+    FactoryGirl.create_list(:report, report_count, :with_logs)
+    FactoryGirl.create_list(:report, report_count, :with_logs, :old_report)
+    assert Report.count > report_count*2
+    assert_difference('Report.count', -1*report_count) do
+      assert_difference(['Log.count', 'Message.count', 'Source.count'], -1*report_count*30) do
+        Report.expire
+      end
+    end
+  end
+
 end