From ac70c48be48cccd4011c7dd88e6d8118d36b1f61 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Thu, 10 Sep 2020 21:37:42 +0200 Subject: [PATCH] FIX: Prevent "uploads are missing in S3" alerts after restoring a backup After restoring a backup it takes up to 48 hours for uploads stored on S3 to appear in the S3 inventory. This change prevents alerts about missing uploads by preventing the EnsureS3UploadsExistence job from running in the first 48 hours after a restore. During the restore it deletes the count of missing uploads from the PluginStore, so that an alert isn't triggered by an old number. --- .../scheduled/ensure_s3_uploads_existence.rb | 14 ++++++- app/models/backup_metadata.rb | 10 +++++ lib/backup_restore/database_restorer.rb | 17 ++------ lib/backup_restore/restorer.rb | 5 +++ spec/jobs/ensure_s3_uploads_existence_spec.rb | 42 +++++++++++++++++++ .../backup_restore/database_restorer_spec.rb | 10 +---- 6 files changed, 75 insertions(+), 23 deletions(-) create mode 100644 spec/jobs/ensure_s3_uploads_existence_spec.rb diff --git a/app/jobs/scheduled/ensure_s3_uploads_existence.rb b/app/jobs/scheduled/ensure_s3_uploads_existence.rb index 99a99ed18f8..8a8e90643b2 100644 --- a/app/jobs/scheduled/ensure_s3_uploads_existence.rb +++ b/app/jobs/scheduled/ensure_s3_uploads_existence.rb @@ -5,6 +5,8 @@ module Jobs class EnsureS3UploadsExistence < ::Jobs::Scheduled every 1.day + WAIT_AFTER_RESTORE_HOURS = 48 + def perform(*args) super ensure @@ -24,7 +26,7 @@ module Jobs end def execute(args) - return unless SiteSetting.enable_s3_inventory + return if !executable? require 's3_inventory' if !@db_inventories && Rails.configuration.multisite && GlobalSetting.use_s3? @@ -42,5 +44,15 @@ module Jobs S3Inventory.new(s3_helper, :upload).backfill_etags_and_list_missing end end + + def executable? + return false if !SiteSetting.enable_s3_inventory + + if last_restore_date = BackupMetadata.last_restore_date + return false if last_restore_date > WAIT_AFTER_RESTORE_HOURS.hours.ago + end + + true + end end end diff --git a/app/models/backup_metadata.rb b/app/models/backup_metadata.rb index b7f3a1c4c62..3fdcd071d31 100644 --- a/app/models/backup_metadata.rb +++ b/app/models/backup_metadata.rb @@ -6,6 +6,16 @@ class BackupMetadata < ActiveRecord::Base def self.value_for(name) where(name: name).pluck_first(:value).presence end + + def self.last_restore_date + value = value_for(LAST_RESTORE_DATE) + value.present? ? Time.zone.parse(value) : nil + end + + def self.update_last_restore_date(time = Time.zone.now) + BackupMetadata.where(name: LAST_RESTORE_DATE).delete_all + BackupMetadata.create!(name: LAST_RESTORE_DATE, value: time.iso8601) + end end # == Schema Information diff --git a/lib/backup_restore/database_restorer.rb b/lib/backup_restore/database_restorer.rb index 40c1459f9dd..2ffe09e97ec 100644 --- a/lib/backup_restore/database_restorer.rb +++ b/lib/backup_restore/database_restorer.rb @@ -27,7 +27,7 @@ module BackupRestore migrate_database reconnect_database - self.class.update_last_restore_date + BackupMetadata.update_last_restore_date end def rollback @@ -51,14 +51,6 @@ module BackupRestore end end - def self.update_last_restore_date - BackupMetadata.where(name: BackupMetadata::LAST_RESTORE_DATE).delete_all - BackupMetadata.create!( - name: BackupMetadata::LAST_RESTORE_DATE, - value: Time.zone.now.iso8601 - ) - end - protected def restore_dump @@ -208,14 +200,11 @@ module BackupRestore def self.backup_schema_dropable? return false unless ActiveRecord::Base.connection.schema_exists?(BACKUP_SCHEMA) - last_restore_date = BackupMetadata.value_for(BackupMetadata::LAST_RESTORE_DATE) - - if last_restore_date.present? - last_restore_date = Time.zone.parse(last_restore_date) + if last_restore_date = BackupMetadata.last_restore_date return last_restore_date + DROP_BACKUP_SCHEMA_AFTER_DAYS.days < Time.zone.now end - update_last_restore_date + BackupMetadata.update_last_restore_date false end private_class_method :backup_schema_dropable? diff --git a/lib/backup_restore/restorer.rb b/lib/backup_restore/restorer.rb index 30f4c093ff2..a6aba0bef4d 100644 --- a/lib/backup_restore/restorer.rb +++ b/lib/backup_restore/restorer.rb @@ -55,6 +55,7 @@ module BackupRestore clear_category_cache clear_emoji_cache clear_theme_cache + clear_stats reload_translations @uploads_restorer.restore(@tmp_directory) @@ -170,6 +171,10 @@ module BackupRestore Stylesheet::Manager.cache.clear end + def clear_stats + Discourse.stats.remove("missing_s3_uploads") + end + def after_restore_hook log "Executing the after_restore_hook..." DiscourseEvent.trigger(:restore_complete) diff --git a/spec/jobs/ensure_s3_uploads_existence_spec.rb b/spec/jobs/ensure_s3_uploads_existence_spec.rb new file mode 100644 index 00000000000..baaf1a461e7 --- /dev/null +++ b/spec/jobs/ensure_s3_uploads_existence_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Jobs::EnsureS3UploadsExistence do + context "S3 inventory enabled" do + before do + SiteSetting.enable_s3_uploads = true + SiteSetting.s3_access_key_id = "abc" + SiteSetting.s3_secret_access_key = "def" + SiteSetting.enable_s3_inventory = true + end + + it "works" do + S3Inventory.any_instance.expects(:backfill_etags_and_list_missing).once + subject.execute({}) + end + + it "doesn't execute when the site was restored within the last 48 hours" do + S3Inventory.any_instance.expects(:backfill_etags_and_list_missing).never + BackupMetadata.update_last_restore_date(47.hours.ago) + + subject.execute({}) + end + + it "executes when the site was restored more than 48 hours ago" do + S3Inventory.any_instance.expects(:backfill_etags_and_list_missing).once + BackupMetadata.update_last_restore_date(49.hours.ago) + + subject.execute({}) + end + end + + context "S3 inventory disabled" do + before { SiteSetting.enable_s3_inventory = false } + + it "doesn't execute" do + S3Inventory.any_instance.expects(:backfill_etags_and_list_missing).never + subject.execute({}) + end + end +end diff --git a/spec/lib/backup_restore/database_restorer_spec.rb b/spec/lib/backup_restore/database_restorer_spec.rb index 09c203878b6..a11c8cc34f3 100644 --- a/spec/lib/backup_restore/database_restorer_spec.rb +++ b/spec/lib/backup_restore/database_restorer_spec.rb @@ -249,20 +249,14 @@ describe BackupRestore::DatabaseRestorer do it "drops the schema when the last restore was long ago" do ActiveRecord::Base.connection.expects(:drop_schema).with("backup") - - freeze_time(8.days.ago) do - subject.update_last_restore_date - end + BackupMetadata.update_last_restore_date(8.days.ago) subject.drop_backup_schema end it "doesn't drop the schema when the last restore was recently" do ActiveRecord::Base.connection.expects(:drop_schema).with("backup").never - - freeze_time(6.days.ago) do - subject.update_last_restore_date - end + BackupMetadata.update_last_restore_date(6.days.ago) subject.drop_backup_schema end