Address review comments46940-hashed-storage-extend-enable-hashed-storage-for-all-new-projects-to-for-all-new-and-renamed-projects

MR - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19747

11 files changed, 91 insertions, 126 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 76c7369046c..f95755b8e8e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1569,7 +1569,7 @@ class Project < ActiveRecord::Base
 
     expire_caches_before_rename(full_path_was)
 
-    if storage.rename_repo
+    if rename_or_migrate_repository!
       Gitlab::AppLogger.info "Project was renamed: #{full_path_was} -> #{new_full_path}"
       rename_repo_notify!
       after_rename_repo
@@ -1957,10 +1957,6 @@ class Project < ActiveRecord::Base
     end
   end
 
-  def rename_using_hashed_storage!
-    ProjectMigrateHashedStorageWorker.new.perform(id, full_path_was)
-  end
-
   def storage_version=(value)
     super
 
@@ -2045,12 +2041,16 @@ class Project < ActiveRecord::Base
     auto_cancel_pending_pipelines == 'enabled'
   end
 
-  def latest_storage_version?
-    storage_version == LATEST_STORAGE_VERSION
-  end
-
   private
 
+  def rename_or_migrate_repository!
+    if Gitlab::CurrentSettings.hashed_storage_enabled && storage_version != LATEST_STORAGE_VERSION
+      ::Projects::HashedStorageMigrationService.new(self.clone, full_path_was).execute
+    else
+      storage.rename_repo
+    end
+  end
+
   def storage
     @storage ||=
       if hashed_storage?(:repository)
diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb
index 2567ddbacf8..8741874a6b9 100644
--- a/app/services/projects/hashed_storage/migrate_attachments_service.rb
+++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb
@@ -3,20 +3,19 @@ module Projects
     AttachmentMigrationError = Class.new(StandardError)
 
     class MigrateAttachmentsService < BaseService
-      attr_reader :logger, :old_path, :new_path
+      attr_reader :logger, :old_disk_path, :new_disk_path
 
-      def initialize(project, old_path, logger: nil)
+      def initialize(project, old_disk_path, logger: nil)
         @project = project
         @logger = logger || Rails.logger
-        @old_path = old_path
+        @old_disk_path = old_disk_path
+        @new_disk_path = project.disk_path
       end
 
       def execute
-        @new_path = project.disk_path
-
         origin = FileUploader.absolute_base_dir(project)
-        # It's possible that old_path does not match project.full_path
-        origin.sub!(/#{Regexp.escape(project.full_path)}\z/, old_path)
+        # It's possible that old_disk_path does not match project.disk_path. For example, that happens when we rename a project
+        origin.sub!(/#{Regexp.escape(project.full_path)}\z/, old_disk_path)
 
         project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments]
         target = FileUploader.absolute_base_dir(project)
@@ -33,22 +32,22 @@ module Projects
 
       private
 
-      def move_folder!(old_path, new_path)
-        unless File.directory?(old_path)
-          logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})")
+      def move_folder!(old_disk_path, new_disk_path)
+        unless File.directory?(old_disk_path)
+          logger.info("Skipped attachments migration from '#{old_disk_path}' to '#{new_disk_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})")
           return
         end
 
-        if File.exist?(new_path)
-          logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})")
-          raise AttachmentMigrationError, "Target path '#{new_path}' already exist"
+        if File.exist?(new_disk_path)
+          logger.error("Cannot migrate attachments from '#{old_disk_path}' to '#{new_disk_path}', target path already exist (PROJECT_ID=#{project.id})")
+          raise AttachmentMigrationError, "Target path '#{new_disk_path}' already exist"
         end
 
         # Create hashed storage base path folder
-        FileUtils.mkdir_p(File.dirname(new_path))
+        FileUtils.mkdir_p(File.dirname(new_disk_path))
 
-        FileUtils.mv(old_path, new_path)
-        logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})")
+        FileUtils.mv(old_disk_path, new_disk_path)
+        logger.info("Migrated project attachments from '#{old_disk_path}' to '#{new_disk_path}' (PROJECT_ID=#{project.id})")
 
         true
       end
diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb
index c10e855f749..5499907af03 100644
--- a/app/services/projects/hashed_storage/migrate_repository_service.rb
+++ b/app/services/projects/hashed_storage/migrate_repository_service.rb
@@ -3,29 +3,26 @@ module Projects
     class MigrateRepositoryService < BaseService
       include Gitlab::ShellAdapter
 
-      attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :old_storage_version,
-                  :logger
+      attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :logger, :move_wiki
 
-      def initialize(project, old_path, logger: nil)
+      def initialize(project, old_disk_path, logger: nil)
         @project = project
         @logger = logger || Rails.logger
-        @old_disk_path = old_path
-        @old_wiki_disk_path = "#{@old_disk_path}.wiki"
+        @old_disk_path = old_disk_path
+        @old_wiki_disk_path = "#{old_disk_path}.wiki"
+        @move_wiki = has_wiki?
       end
 
       def execute
-        has_wiki = gitlab_shell.exists?(project.repository_storage, "#{@old_wiki_disk_path}.git")
-
-        @old_storage_version = project.storage_version
         project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
         project.ensure_storage_path_exists
 
         @new_disk_path = project.disk_path
 
-        result = move_repository(@old_disk_path, @new_disk_path)
+        result = move_repository(old_disk_path, new_disk_path)
 
-        if has_wiki
-          result &&= move_repository("#{@old_wiki_disk_path}", "#{@new_disk_path}.wiki")
+        if move_wiki
+          result &&= move_repository("#{old_wiki_disk_path}", "#{new_disk_path}.wiki")
         end
 
         if result
@@ -47,6 +44,10 @@ module Projects
 
       private
 
+      def has_wiki?
+        gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git")
+      end
+
       def move_repository(from_name, to_name)
         from_exists = gitlab_shell.exists?(project.repository_storage, "#{from_name}.git")
         to_exists = gitlab_shell.exists?(project.repository_storage, "#{to_name}.git")
@@ -65,8 +66,8 @@ module Projects
       end
 
       def rollback_folder_move
-        move_repository(@new_disk_path, @old_disk_path)
-        move_repository("#{@new_disk_path}.wiki", "#{@old_disk_path}.wiki")
+        move_repository(new_disk_path, old_disk_path)
+        move_repository("#{new_disk_path}.wiki", old_wiki_disk_path)
       end
     end
   end
diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb
index fa26e1c8fbb..9e24e944d51 100644
--- a/app/services/projects/hashed_storage_migration_service.rb
+++ b/app/services/projects/hashed_storage_migration_service.rb
@@ -1,23 +1,25 @@
 module Projects
   class HashedStorageMigrationService < BaseService
-    attr_reader :logger, :old_path
+    attr_reader :logger, :old_disk_path
 
-    def initialize(project, old_path, logger: nil)
+    def initialize(project, old_disk_path, logger: nil)
       @project = project
-      @old_path = old_path
+      @old_disk_path = old_disk_path
       @logger = logger || Rails.logger
     end
 
     def execute
       # Migrate repository from Legacy to Hashed Storage
       unless project.hashed_storage?(:repository)
-        return unless HashedStorage::MigrateRepositoryService.new(project, old_path, logger: logger).execute
+        return unless HashedStorage::MigrateRepositoryService.new(project, old_disk_path, logger: logger).execute
       end
 
       # Migrate attachments from Legacy to Hashed Storage
       unless project.hashed_storage?(:attachments)
-        HashedStorage::MigrateAttachmentsService.new(project, old_path, logger: logger).execute
+        HashedStorage::MigrateAttachmentsService.new(project, old_disk_path, logger: logger).execute
       end
+
+      true
     end
   end
 end
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index 6a65a87f1b9..f375f2dc8be 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -2,26 +2,26 @@ module Projects
   class UpdateService < BaseService
     include UpdateVisibilityLevel
 
-    UpdateError = Class.new(StandardError)
+    ValidationError = Class.new(StandardError)
 
     def execute
-      pre_checks
+      validate!
 
       ensure_wiki_exists if enabling_wiki?
 
       yield if block_given?
 
       # If the block added errors, don't try to save the project
-      return validation_failed! if project.errors.any?
+      return update_failed! if project.errors.any?
 
       if project.update(params.except(:default_branch))
         after_update
 
         success
       else
-        validation_failed!
+        update_failed!
       end
-    rescue UpdateError => e
+    rescue ValidationError => e
       error(e.message)
     end
 
@@ -33,27 +33,23 @@ module Projects
 
     private
 
-    def pre_checks
+    def validate!
       unless valid_visibility_level_change?(project, params[:visibility_level])
-        raise UpdateError.new('New visibility level not allowed!')
+        raise ValidationError.new('New visibility level not allowed!')
       end
 
       if renaming_project_with_container_registry_tags?
-        raise UpdateError.new('Cannot rename project because it contains container registry tags!')
+        raise ValidationError.new('Cannot rename project because it contains container registry tags!')
       end
 
       if changing_default_branch?
-        raise UpdateError.new("Could not set the default branch") unless project.change_head(params[:default_branch])
+        raise ValidationError.new("Could not set the default branch") unless project.change_head(params[:default_branch])
       end
     end
 
     def after_update
       if project.previous_changes.include?('path')
-        if migrate_to_hashed_storage?
-          project.rename_using_hashed_storage!
-        else
-          project.rename_repo
-        end
+        project.rename_repo
       else
         system_hook_service.execute_hooks_for(project, :update)
       end
@@ -61,11 +57,7 @@ module Projects
       update_pages_config if changing_pages_https_only?
     end
 
-    def migrate_to_hashed_storage?
-      Gitlab::CurrentSettings.hashed_storage_enabled && !project.latest_storage_version?
-    end
-
-    def validation_failed!
+    def update_failed!
       model_errors = project.errors.full_messages.to_sentence
       error_message = model_errors.presence || 'Project could not be updated!'
 
@@ -87,7 +79,7 @@ module Projects
     end
 
     def enabling_wiki?
-      return false if @project.wiki_enabled?
+      return false if project.wiki_enabled?
 
       params.dig(:project_feature_attributes, :wiki_access_level).to_i > ProjectFeature::DISABLED
     end
diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb
index 678efd75292..ad0003e7bff 100644
--- a/app/workers/project_migrate_hashed_storage_worker.rb
+++ b/app/workers/project_migrate_hashed_storage_worker.rb
@@ -5,13 +5,13 @@ class ProjectMigrateHashedStorageWorker
 
   LEASE_TIMEOUT = 30.seconds.to_i
 
-  def perform(project_id, old_path = nil)
+  def perform(project_id, old_disk_path = nil)
     project = Project.find_by(id: project_id)
     return if project.nil? || project.pending_delete?
 
     uuid = lease_for(project_id).try_obtain
     if uuid
-      ::Projects::HashedStorageMigrationService.new(project, old_path || project.full_path, logger: logger).execute
+      ::Projects::HashedStorageMigrationService.new(project, old_disk_path || project.full_path, logger: logger).execute
     else
       false
     end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 3f775174743..b347ee4bfc8 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3091,6 +3091,19 @@ describe Project do
         allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
       end
 
+      context 'migration to hashed storage' do
+        it 'calls HashedStorageMigrationService with correct options' do
+          project = create(:project, :repository, :legacy_storage)
+          allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
+
+          expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service|
+            expect(service).to receive(:execute).and_return(true)
+          end
+
+          project.rename_repo
+        end
+      end
+
       it 'renames a repository' do
         stub_container_registry_config(enabled: false)
 
@@ -3140,6 +3153,8 @@ describe Project do
           let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
 
           it 'moves pages folder to new location' do
+            stub_application_setting(hashed_storage_enabled: false)
+
             expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
 
             project.rename_repo
@@ -3869,18 +3884,4 @@ describe Project do
       project.repository.rugged.config
     end
   end
-
-  describe '#rename_using_hashed_storage!' do
-    let(:project) { create(:project) }
-    let!(:old_path) { project.full_path }
-
-    it 'calls ProjectMigrateHashedStorageWorker with correct options' do
-      project.update!(path: 'some-new-path')
-
-      expect_any_instance_of(ProjectMigrateHashedStorageWorker)
-        .to receive(:perform).with(project.id, old_path)
-
-      project.rename_using_hashed_storage!
-    end
-  end
 end
diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
index d7ea938a99e..28d8a95fe07 100644
--- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
@@ -2,20 +2,21 @@ require 'spec_helper'
 
 describe Projects::HashedStorage::MigrateAttachmentsService do
   subject(:service) { described_class.new(project, project.full_path, logger: nil) }
+
   let(:project) { create(:project, :legacy_storage) }
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
   let!(:upload) { Upload.find_by(path: file_uploader.upload_path) }
   let(:file_uploader) { build(:file_uploader, project: project) }
-  let(:old_path) { File.join(base_path(legacy_storage), upload.path) }
-  let(:new_path) { File.join(base_path(hashed_storage), upload.path) }
+  let(:old_disk_path) { File.join(base_path(legacy_storage), upload.path) }
+  let(:new_disk_path) { File.join(base_path(hashed_storage), upload.path) }
 
   context '#execute' do
     context 'when succeeds' do
       it 'moves attachments to hashed storage layout' do
-        expect(File.file?(old_path)).to be_truthy
-        expect(File.file?(new_path)).to be_falsey
+        expect(File.file?(old_disk_path)).to be_truthy
+        expect(File.file?(new_disk_path)).to be_falsey
         expect(File.exist?(base_path(legacy_storage))).to be_truthy
         expect(File.exist?(base_path(hashed_storage))).to be_falsey
         expect(FileUtils).to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)).and_call_original
@@ -24,8 +25,8 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
 
         expect(File.exist?(base_path(hashed_storage))).to be_truthy
         expect(File.exist?(base_path(legacy_storage))).to be_falsey
-        expect(File.file?(old_path)).to be_falsey
-        expect(File.file?(new_path)).to be_truthy
+        expect(File.file?(old_disk_path)).to be_falsey
+        expect(File.file?(new_disk_path)).to be_truthy
       end
     end
 
@@ -40,7 +41,7 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
         service.execute
 
         expect(File.exist?(base_path(hashed_storage))).to be_falsey
-        expect(File.file?(new_path)).to be_falsey
+        expect(File.file?(new_disk_path)).to be_falsey
       end
     end
 
@@ -55,20 +56,6 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
         expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentMigrationError)
       end
     end
-
-    context 'when old_path does not match full_path' do
-      let(:old_path) { 'old-path' }
-      let(:logger) { double }
-      subject(:service) { described_class.new(project, old_path, logger: logger) }
-
-      it 'uses old_path parameter' do
-        expect(logger).to receive(:info).with(/source path doesn\'t exist or is not a directory/)
-
-        service.execute
-
-        expect(service.old_path).to eq old_path
-      end
-    end
   end
 
   def base_path(storage)
diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
index a624b2f22c7..5f67c325223 100644
--- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
@@ -3,10 +3,11 @@ require 'spec_helper'
 describe Projects::HashedStorage::MigrateRepositoryService do
   let(:gitlab_shell) { Gitlab::Shell.new }
   let(:project) { create(:project, :legacy_storage, :repository, :wiki_repo) }
-  let(:service) { described_class.new(project, project.full_path) }
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
 
+  subject(:service) { described_class.new(project, project.full_path) }
+
   describe '#execute' do
     before do
       allow(service).to receive(:gitlab_shell) { gitlab_shell }
@@ -79,23 +80,6 @@ describe Projects::HashedStorage::MigrateRepositoryService do
       end
     end
 
-    context 'when old_path does not match full_path' do
-      let(:old_path) { 'old-path' }
-      let(:logger) { double }
-      let(:service) { described_class.new(project, old_path, logger: logger) }
-
-      it 'uses passed old_path parameter' do
-        expect(service).to receive(:move_repository).with(old_path, /\@hashed/)
-
-        allow(service).to receive(:rollback_folder_move).and_return(nil)
-
-        service.execute
-
-        expect(service.old_disk_path).to eq old_path
-        expect(service.old_wiki_disk_path).to eq "#{old_path}.wiki"
-      end
-    end
-
     def expect_move_repository(from_name, to_name)
       expect(gitlab_shell).to receive(:mv_repository).with(project.repository_storage, from_name, to_name).and_call_original
     end
diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb
index 234669dd45d..5368c3828dd 100644
--- a/spec/services/projects/hashed_storage_migration_service_spec.rb
+++ b/spec/services/projects/hashed_storage_migration_service_spec.rb
@@ -2,7 +2,8 @@ require 'spec_helper'
 
 describe Projects::HashedStorageMigrationService do
   let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) }
-  let(:logger) { Rails.logger }
+  let(:logger) { double }
+
   subject(:service) { described_class.new(project, project.full_path, logger: logger) }
 
   describe '#execute' do
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index 4c5832b1832..40a1da9dcdb 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -219,14 +219,12 @@ describe Projects::UpdateService do
           end
 
           it 'migrates project to a hashed storage instead of renaming the repo to another legacy name' do
-            Sidekiq::Testing.inline! do
-              result = update_project(project, admin, path: 'new-path')
+            result = update_project(project, admin, path: 'new-path')
 
-              expect(result).not_to include(status: :error)
-              expect(project).to be_valid
-              expect(project.errors).to be_empty
-              expect(project.reload.hashed_storage?(:repository)).to be_truthy
-            end
+            expect(result).not_to include(status: :error)
+            expect(project).to be_valid
+            expect(project.errors).to be_empty
+            expect(project.reload.hashed_storage?(:repository)).to be_truthy
           end
         end
       end