diff options
| -rw-r--r-- | app/models/upload.rb | 18 | ||||
| -rw-r--r-- | changelogs/unreleased/tc-index-uploads-file-store.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20181005125926_add_index_to_uploads_store.rb | 17 | ||||
| -rw-r--r-- | db/post_migrate/20181105201455_steal_fill_store_upload.rb | 31 | ||||
| -rw-r--r-- | db/schema.rb | 1 | ||||
| -rw-r--r-- | doc/administration/monitoring/prometheus/gitlab_metrics.md | 1 | ||||
| -rw-r--r-- | spec/migrations/steal_fill_store_upload_spec.rb | 40 | ||||
| -rw-r--r-- | spec/models/upload_spec.rb | 62 | ||||
| -rw-r--r-- | spec/tasks/gitlab/uploads/migrate_rake_spec.rb | 8 |
9 files changed, 162 insertions, 21 deletions
diff --git a/app/models/upload.rb b/app/models/upload.rb index 23bc9ca42fc..e01e9c6a4f0 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -11,7 +11,8 @@ class Upload < ActiveRecord::Base validates :model, presence: true validates :uploader, presence: true - scope :with_files_stored_locally, -> { where(store: [nil, ObjectStorage::Store::LOCAL]) } + scope :with_files_stored_locally, -> { where(store: ObjectStorage::Store::LOCAL) } + scope :with_files_stored_remotely, -> { where(store: ObjectStorage::Store::REMOTE) } before_save :calculate_checksum!, if: :foreground_checksummable? after_commit :schedule_checksum, if: :checksummable? @@ -46,7 +47,18 @@ class Upload < ActiveRecord::Base end def exist? - File.exist?(absolute_path) + exist = File.exist?(absolute_path) + + # Help sysadmins find missing upload files + if persisted? && !exist + if Gitlab::Sentry.enabled? + Raven.capture_message("Upload file does not exist", extra: self.attributes) + end + + Gitlab::Metrics.counter(:upload_file_does_not_exist_total, 'The number of times an upload record could not find its file').increment + end + + exist end def uploader_context @@ -57,8 +69,6 @@ class Upload < ActiveRecord::Base end def local? - return true if store.nil? - store == ObjectStorage::Store::LOCAL end diff --git a/changelogs/unreleased/tc-index-uploads-file-store.yml b/changelogs/unreleased/tc-index-uploads-file-store.yml new file mode 100644 index 00000000000..fa3b3164e38 --- /dev/null +++ b/changelogs/unreleased/tc-index-uploads-file-store.yml @@ -0,0 +1,5 @@ +--- +title: Enhance performance of counting local Uploads +merge_request: 22522 +author: +type: performance diff --git a/db/migrate/20181005125926_add_index_to_uploads_store.rb b/db/migrate/20181005125926_add_index_to_uploads_store.rb new file mode 100644 index 00000000000..d32ca05e980 --- /dev/null +++ b/db/migrate/20181005125926_add_index_to_uploads_store.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToUploadsStore < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :uploads, :store + end + + def down + remove_concurrent_index :uploads, :store + end +end diff --git a/db/post_migrate/20181105201455_steal_fill_store_upload.rb b/db/post_migrate/20181105201455_steal_fill_store_upload.rb new file mode 100644 index 00000000000..982001fedbe --- /dev/null +++ b/db/post_migrate/20181105201455_steal_fill_store_upload.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class StealFillStoreUpload < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + BATCH_SIZE = 10_000 + + disable_ddl_transaction! + + class Upload < ActiveRecord::Base + include EachBatch + + self.table_name = 'uploads' + self.inheritance_column = :_type_disabled # Disable STI + end + + def up + Gitlab::BackgroundMigration.steal('FillStoreUpload') + + Upload.where(store: nil).each_batch(of: BATCH_SIZE) do |batch| + range = batch.pluck('MIN(id)', 'MAX(id)').first + + Gitlab::BackgroundMigration::FillStoreUpload.new.perform(*range) + end + end + + def down + # noop + end +end diff --git a/db/schema.rb b/db/schema.rb index cfbfd7ad375..7509941325f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2155,6 +2155,7 @@ ActiveRecord::Schema.define(version: 20181107054254) do add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree + add_index "uploads", ["store"], name: "index_uploads_on_store", using: :btree add_index "uploads", ["uploader", "path"], name: "index_uploads_on_uploader_and_path", using: :btree create_table "user_agent_details", force: :cascade do |t| diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index c6fd7ef7360..5700f640e4c 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -45,6 +45,7 @@ The following metrics are available: | redis_ping_success | Gauge | 9.4 | Whether or not the last redis ping succeeded | | redis_ping_latency_seconds | Gauge | 9.4 | Round trip time of the redis ping | | user_session_logins_total | Counter | 9.4 | Counter of how many users have logged in | +| upload_file_does_not_exist | Counter | 10.7 | Number of times an upload record could not find its file | | failed_login_captcha_total | Gauge | 11.0 | Counter of failed CAPTCHA attempts during login | | successful_login_captcha_total | Gauge | 11.0 | Counter of successful CAPTCHA attempts during login | diff --git a/spec/migrations/steal_fill_store_upload_spec.rb b/spec/migrations/steal_fill_store_upload_spec.rb new file mode 100644 index 00000000000..ed809baf2b5 --- /dev/null +++ b/spec/migrations/steal_fill_store_upload_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20181105201455_steal_fill_store_upload.rb') + +describe StealFillStoreUpload, :migration do + let(:uploads) { table(:uploads) } + + describe '#up' do + it 'steals the FillStoreUpload background migration' do + expect(Gitlab::BackgroundMigration).to receive(:steal).with('FillStoreUpload').and_call_original + + migrate! + end + + it 'does not run migration if not needed' do + uploads.create(size: 100.kilobytes, + uploader: 'AvatarUploader', + path: 'uploads/-/system/avatar.jpg', + store: 1) + + expect_any_instance_of(Gitlab::BackgroundMigration::FillStoreUpload).not_to receive(:perform) + + migrate! + end + + it 'ensures all rows are migrated' do + uploads.create(size: 100.kilobytes, + uploader: 'AvatarUploader', + path: 'uploads/-/system/avatar.jpg', + store: nil) + + expect_any_instance_of(Gitlab::BackgroundMigration::FillStoreUpload).to receive(:perform).and_call_original + + expect do + migrate! + end.to change { uploads.where(store: nil).count }.from(1).to(0) + end + end +end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 3c89e99abf0..5a0df9fbbb0 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -21,7 +21,8 @@ describe Upload do path: __FILE__, size: described_class::CHECKSUM_THRESHOLD + 1.kilobyte, model: build_stubbed(:user), - uploader: double('ExampleUploader') + uploader: double('ExampleUploader'), + store: ObjectStorage::Store::LOCAL ) expect(UploadChecksumWorker) @@ -35,7 +36,8 @@ describe Upload do path: __FILE__, size: described_class::CHECKSUM_THRESHOLD, model: build_stubbed(:user), - uploader: double('ExampleUploader') + uploader: double('ExampleUploader'), + store: ObjectStorage::Store::LOCAL ) expect { upload.save } @@ -60,7 +62,7 @@ describe Upload do describe '#absolute_path' do it 'returns the path directly when already absolute' do path = '/path/to/namespace/project/secret/file.jpg' - upload = described_class.new(path: path) + upload = described_class.new(path: path, store: ObjectStorage::Store::LOCAL) expect(upload).not_to receive(:uploader_class) @@ -69,7 +71,7 @@ describe Upload do it "delegates to the uploader's absolute_path method" do uploader = spy('FakeUploader') - upload = described_class.new(path: 'secret/file.jpg') + upload = described_class.new(path: 'secret/file.jpg', store: ObjectStorage::Store::LOCAL) expect(upload).to receive(:uploader_class).and_return(uploader) upload.absolute_path @@ -81,7 +83,8 @@ describe Upload do describe '#calculate_checksum!' do let(:upload) do described_class.new(path: __FILE__, - size: described_class::CHECKSUM_THRESHOLD - 1.megabyte) + size: described_class::CHECKSUM_THRESHOLD - 1.megabyte, + store: ObjectStorage::Store::LOCAL) end it 'sets `checksum` to SHA256 sum of the file' do @@ -104,15 +107,56 @@ describe Upload do describe '#exist?' do it 'returns true when the file exists' do - upload = described_class.new(path: __FILE__) + upload = described_class.new(path: __FILE__, store: ObjectStorage::Store::LOCAL) expect(upload).to exist end - it 'returns false when the file does not exist' do - upload = described_class.new(path: "#{__FILE__}-nope") + context 'when the file does not exist' do + it 'returns false' do + upload = described_class.new(path: "#{__FILE__}-nope", store: ObjectStorage::Store::LOCAL) - expect(upload).not_to exist + expect(upload).not_to exist + end + + context 'when the record is persisted' do + it 'sends a message to Sentry' do + upload = create(:upload, :issuable_upload) + + expect(Gitlab::Sentry).to receive(:enabled?).and_return(true) + expect(Raven).to receive(:capture_message).with("Upload file does not exist", extra: upload.attributes) + + upload.exist? + end + + it 'increments a metric counter to signal a problem' do + upload = create(:upload, :issuable_upload) + + counter = double(:counter) + expect(counter).to receive(:increment) + expect(Gitlab::Metrics).to receive(:counter).with(:upload_file_does_not_exist_total, 'The number of times an upload record could not find its file').and_return(counter) + + upload.exist? + end + end + + context 'when the record is not persisted' do + it 'does not send a message to Sentry' do + upload = described_class.new(path: "#{__FILE__}-nope", store: ObjectStorage::Store::LOCAL) + + expect(Raven).not_to receive(:capture_message) + + upload.exist? + end + + it 'does not increment a metric counter' do + upload = described_class.new(path: "#{__FILE__}-nope", store: ObjectStorage::Store::LOCAL) + + expect(Gitlab::Metrics).not_to receive(:counter) + + upload.exist? + end + end end end diff --git a/spec/tasks/gitlab/uploads/migrate_rake_spec.rb b/spec/tasks/gitlab/uploads/migrate_rake_spec.rb index 6fcfae358ec..9588e8be5dc 100644 --- a/spec/tasks/gitlab/uploads/migrate_rake_spec.rb +++ b/spec/tasks/gitlab/uploads/migrate_rake_spec.rb @@ -38,14 +38,6 @@ describe 'gitlab:uploads:migrate rake tasks' do let!(:projects) { create_list(:project, 10, :with_avatar) } it_behaves_like 'enqueue jobs in batch', batch: 4 - - context 'Upload has store = nil' do - before do - Upload.where(model: projects).update_all(store: nil) - end - - it_behaves_like 'enqueue jobs in batch', batch: 4 - end end context "for Group" do |