Merge branch 'tc-index-uploads-file-store' into 'master'

Enhance performance of counting local Uploads

Closes gitlab-ee#6070

See merge request gitlab-org/gitlab-ce!22522

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
 

changelogs/unreleased/tc-index-uploads-file-store.yml

+---
+title: Enhance performance of counting local Uploads
+merge_request: 22522
+author:
+type: performance

db/migrate/20181005125926_add_index_to_uploads_store.rb

+# 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

db/post_migrate/20181105201455_steal_fill_store_upload.rb

+# 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

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|

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 |
 

spec/migrations/steal_fill_store_upload_spec.rb

+# 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

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
 

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