Merge branch '3674-hashed-storage-attachments' into 'master'

Hashed Storage support for Attachments

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

app/models/project.rb

@@ -26,7 +26,15 @@ class Project < ActiveRecord::Base
 
   NUMBER_OF_PERMITTED_BOARDS = 1
   UNKNOWN_IMPORT_URL = 'http://unknown.git'.freeze
-  LATEST_STORAGE_VERSION = 1
+  # Hashed Storage versions handle rolling out new storage to project and dependents models:
+  # nil: legacy
+  # 1: repository
+  # 2: attachments
+  LATEST_STORAGE_VERSION = 2
+  HASHED_STORAGE_FEATURES = {
+    repository: 1,
+    attachments: 2
+  }.freeze
 
   cache_markdown_field :description, pipeline: :description
 
@@ -1395,6 +1403,19 @@ class Project < ActiveRecord::Base
     end
   end
 
+  def after_rename_repo
+    path_before_change = previous_changes['path'].first
+
+    # We need to check if project had been rolled out to move resource to hashed storage or not and decide
+    # if we need execute any take action or no-op.
+
+    unless hashed_storage?(:attachments)
+      Gitlab::UploadsTransfer.new.rename_project(path_before_change, self.path, namespace.full_path)
+    end
+
+    Gitlab::PagesTransfer.new.rename_project(path_before_change, self.path, namespace.full_path)
+  end
+
   def rename_repo_notify!
     send_move_instructions(full_path_was)
     expires_full_path_cache
@@ -1405,13 +1426,6 @@ class Project < ActiveRecord::Base
     reload_repository!
   end
 
-  def after_rename_repo
-    path_before_change = previous_changes['path'].first
-
-    Gitlab::UploadsTransfer.new.rename_project(path_before_change, self.path, namespace.full_path)
-    Gitlab::PagesTransfer.new.rename_project(path_before_change, self.path, namespace.full_path)
-  end
-
   def running_or_pending_build_count(force: false)
     Rails.cache.fetch(['projects', id, 'running_or_pending_build_count'], force: force) do
       builds.running_or_pending.count(:all)
@@ -1601,8 +1615,13 @@ class Project < ActiveRecord::Base
     [nil, 0].include?(self.storage_version)
   end
 
-  def hashed_storage?
-    self.storage_version && self.storage_version >= 1
+  # Check if Hashed Storage is enabled for the project with at least informed feature rolled out
+  #
+  # @param [Symbol] feature that needs to be rolled out for the project (:repository, :attachments)
+  def hashed_storage?(feature)
+    raise ArgumentError, "Invalid feature" unless HASHED_STORAGE_FEATURES.include?(feature)
+
+    self.storage_version && self.storage_version >= HASHED_STORAGE_FEATURES[feature]
   end
 
   def renamed?
@@ -1638,7 +1657,7 @@ class Project < ActiveRecord::Base
   end
 
   def migrate_to_hashed_storage!
-    return if hashed_storage?
+    return if hashed_storage?(:repository)
 
     update!(repository_read_only: true)
 
@@ -1663,7 +1682,7 @@ class Project < ActiveRecord::Base
 
   def storage
     @storage ||=
-      if hashed_storage?
+      if hashed_storage?(:repository)
         Storage::HashedProject.new(self)
       else
         Storage::LegacyProject.new(self)

app/services/projects/hashed_storage_migration_service.rb

@@ -10,7 +10,7 @@ module Projects
     end
 
     def execute
-      return if project.hashed_storage?
+      return if project.hashed_storage?(:repository)
 
       @old_disk_path = project.disk_path
       has_wiki = project.wiki.repository_exists?

app/uploaders/file_uploader.rb

@@ -30,7 +30,7 @@ class FileUploader < GitlabUploader
   #
   # Returns a String without a trailing slash
   def self.dynamic_path_segment(model)
-    File.join(CarrierWave.root, base_dir, model.full_path)
+    File.join(CarrierWave.root, base_dir, model.disk_path)
   end
 
   attr_accessor :model

changelogs/unreleased/3674-hashed-storage-attachments.yml

+---
+title: Hashed Storage support for Attachments
+merge_request: 15068
+author:
+type: added

doc/administration/repository_storage_types.md

@@ -25,7 +25,10 @@ Any change in the URL will need to be reflected on disk (when groups / users or
 of load in big installations, and can be even worst if they are using any type of network based filesystem.
 
 Last, for GitLab Geo, this storage type means we have to synchronize the disk state, replicate renames in the correct
-order or we may end-up with wrong repository or missing data temporarily.   
+order or we may end-up with wrong repository or missing data temporarily.
+
+This pattern also exists in other objects stored in GitLab, like issue Attachments, GitLab Pages artifacts, 
+Docker Containers for the integrated Registry, etc.
 
 ## Hashed Storage
 
@@ -67,3 +70,23 @@ To migrate your existing projects to the new storage type, check the specific [r
 [ce-28283]: https://gitlab.com/gitlab-org/gitlab-ce/issues/28283
 [rake tasks]: raketasks/storage.md#migrate-existing-projects-to-hashed-storage
 [storage-paths]: repository_storage_types.md
+
+### Hashed Storage coverage
+
+We are incrementally moving every storable object in GitLab to the Hashed Storage pattern. You can check the current
+coverage status below.
+
+Not that things stored in S3 compatible endpoint, will not have the downsides mentioned earlier, if they are not
+prefixed with `#{namespace}/#{project_name}`, which is true for CI Cache and LFS Objects.
+
+| Storable Object | Legacy Storage | Hashed Storage | S3 Compatible | GitLab Version | 
+| ----------------| -------------- | -------------- | ------------- | -------------- |
+| Repository      | Yes            | Yes            | -             | 10.0           |
+| Attachments     | Yes            | Yes            | -             | 10.2           |
+| Avatars         | Yes            | No             | -             | -              | 
+| Pages           | Yes            | No             | -             | -              |
+| Docker Registry | Yes            | No             | -             | -              |
+| CI Build Logs   | No             | No             | -             | -              | 
+| CI Artifacts    | No             | No             | -             | -              | 
+| CI Cache        | No             | No             | Yes           | -              |
+| LFS Objects     | Yes            | No             | Yes (EEP)     | -              | 

spec/models/project_spec.rb

@@ -2453,6 +2453,7 @@ describe Project do
   context 'legacy storage' do
     let(:project) { create(:project, :repository) }
     let(:gitlab_shell) { Gitlab::Shell.new }
+    let(:project_storage) { project.send(:storage) }
 
     before do
       allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
@@ -2494,7 +2495,7 @@ describe Project do
 
     describe '#hashed_storage?' do
       it 'returns false' do
-        expect(project.hashed_storage?).to be_falsey
+        expect(project.hashed_storage?(:repository)).to be_falsey
       end
     end
 
@@ -2547,6 +2548,30 @@ describe Project do
 
         it { expect { subject }.to raise_error(StandardError) }
       end
+
+      context 'gitlab pages' do
+        before do
+          expect(project_storage).to receive(:rename_repo) { true }
+        end
+
+        it 'moves pages folder to new location' do
+          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
+
+          project.rename_repo
+        end
+      end
+
+      context 'attachments' do
+        before do
+          expect(project_storage).to receive(:rename_repo) { true }
+        end
+
+        it 'moves uploads folder to new location' do
+          expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
+
+          project.rename_repo
+        end
+      end
     end
 
     describe '#pages_path' do
@@ -2606,8 +2631,14 @@ describe Project do
     end
 
     describe '#hashed_storage?' do
-      it 'returns true' do
-        expect(project.hashed_storage?).to be_truthy
+      it 'returns true if rolled out' do
+        expect(project.hashed_storage?(:attachments)).to be_truthy
+      end
+
+      it 'returns false when not rolled out yet' do
+        project.storage_version = 1
+
+        expect(project.hashed_storage?(:attachments)).to be_falsey
       end
     end
 
@@ -2650,10 +2681,6 @@ describe Project do
           .to receive(:execute_hooks_for)
             .with(project, :rename)
 
-        expect_any_instance_of(Gitlab::UploadsTransfer)
-          .to receive(:rename_project)
-            .with('foo', project.path, project.namespace.full_path)
-
         expect(project).to receive(:expire_caches_before_rename)
 
         expect(project).to receive(:expires_full_path_cache)
@@ -2674,6 +2701,32 @@ describe Project do
 
         it { expect { subject }.to raise_error(StandardError) }
       end
+
+      context 'gitlab pages' do
+        it 'moves pages folder to new location' do
+          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
+
+          project.rename_repo
+        end
+      end
+
+      context 'attachments' do
+        it 'keeps uploads folder location unchanged' do
+          expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
+
+          project.rename_repo
+        end
+
+        context 'when not rolled out' do
+          let(:project) { create(:project, :repository, storage_version: 1) }
+
+          it 'moves pages folder to new location' do
+            expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
+
+            project.rename_repo
+          end
+        end
+      end
     end
 
     describe '#pages_path' do

spec/services/projects/hashed_storage_migration_service_spec.rb

@@ -23,7 +23,7 @@ describe Projects::HashedStorageMigrationService do
       it 'updates project to be hashed and not read-only' do
         service.execute
 
-        expect(project.hashed_storage?).to be_truthy
+        expect(project.hashed_storage?(:repository)).to be_truthy
         expect(project.repository_read_only).to be_falsey
       end
 

spec/uploaders/file_uploader_spec.rb

@@ -3,25 +3,51 @@ require 'spec_helper'
 describe FileUploader do
   let(:uploader) { described_class.new(build_stubbed(:project)) }
 
-  describe '.absolute_path' do
-    it 'returns the correct absolute path by building it dynamically' do
-      project = build_stubbed(:project)
-      upload = double(model: project, path: 'secret/foo.jpg')
+  context 'legacy storage' do
+    let(:project) { build_stubbed(:project) }
 
-      dynamic_segment = project.path_with_namespace
+    describe '.absolute_path' do
+      it 'returns the correct absolute path by building it dynamically' do
+        upload = double(model: project, path: 'secret/foo.jpg')
 
-      expect(described_class.absolute_path(upload))
-        .to end_with("#{dynamic_segment}/secret/foo.jpg")
+        dynamic_segment = project.full_path
+
+        expect(described_class.absolute_path(upload))
+          .to end_with("#{dynamic_segment}/secret/foo.jpg")
+      end
+    end
+
+    describe "#store_dir" do
+      it "stores in the namespace path" do
+        uploader = described_class.new(project)
+
+        expect(uploader.store_dir).to include(project.full_path)
+        expect(uploader.store_dir).not_to include("system")
+      end
     end
   end
 
-  describe "#store_dir" do
-    it "stores in the namespace path" do
-      project = build_stubbed(:project)
-      uploader = described_class.new(project)
+  context 'hashed storage' do
+    let(:project) { build_stubbed(:project, :hashed) }
+
+    describe '.absolute_path' do
+      it 'returns the correct absolute path by building it dynamically' do
+        upload = double(model: project, path: 'secret/foo.jpg')
+
+        dynamic_segment = project.disk_path
+
+        expect(described_class.absolute_path(upload))
+          .to end_with("#{dynamic_segment}/secret/foo.jpg")
+      end
+    end
+
+    describe "#store_dir" do
+      it "stores in the namespace path" do
+        uploader = described_class.new(project)
 
-      expect(uploader.store_dir).to include(project.path_with_namespace)
-      expect(uploader.store_dir).not_to include("system")
+        expect(uploader.store_dir).to include(project.disk_path)
+        expect(uploader.store_dir).not_to include("system")
+      end
     end
   end