Resolve "Hashed storage: extend "Enable hashed storage for all new projects"...

Resolve "Hashed storage: extend "Enable hashed storage for all new projects" to "for all new and renamed projects""

app/models/project.rb

@@ -1568,47 +1568,33 @@ class Project < ActiveRecord::Base
   end
 
   def rename_repo
-    new_full_path = build_full_path
+    path_before      = previous_changes['path'].first
+    full_path_before = full_path_was
+    full_path_after  = build_full_path
 
-    Rails.logger.error "Attempting to rename #{full_path_was} -> #{new_full_path}"
+    Gitlab::AppLogger.info("Attempting to rename #{full_path_was} -> #{full_path_after}")
 
     if has_container_registry_tags?
-      Rails.logger.error "Project #{full_path_was} cannot be renamed because container registry tags are present!"
+      Gitlab::AppLogger.info("Project #{full_path_was} cannot be renamed because container registry tags are present!")
 
-      # we currently doesn't support renaming repository if it contains images in container registry
+      # we currently don't support renaming repository if it contains images in container registry
       raise StandardError.new('Project cannot be renamed, because images are present in its container registry')
     end
 
-    expire_caches_before_rename(full_path_was)
+    expire_caches_before_rename(full_path_before)
 
-    if storage.rename_repo
-      Gitlab::AppLogger.info "Project was renamed: #{full_path_was} -> #{new_full_path}"
-      rename_repo_notify!
-      after_rename_repo
+    if rename_or_migrate_repository!
+      Gitlab::AppLogger.info("Project was renamed: #{full_path_before} -> #{full_path_after}")
+      after_rename_repository(full_path_before, path_before)
     else
-      Rails.logger.error "Repository could not be renamed: #{full_path_was} -> #{new_full_path}"
+      Gitlab::AppLogger.info("Repository could not be renamed: #{full_path_before} -> #{full_path_after}")
 
       # if we cannot move namespace directory we should rollback
       # db changes in order to prevent out of sync between db and fs
-      raise StandardError.new('repository cannot be renamed')
+      raise StandardError.new('Repository cannot be renamed')
     end
   end
 
-  def after_rename_repo
-    write_repository_config
-
-    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 write_repository_config(gl_full_path: full_path)
     # We'd need to keep track of project full path otherwise directory tree
     # created with hashed storage enabled cannot be usefully imported using
@@ -1619,17 +1605,6 @@ class Project < ActiveRecord::Base
     nil
   end
 
-  def rename_repo_notify!
-    # When we import a project overwriting the original project, there
-    # is a move operation. In that case we don't want to send the instructions.
-    send_move_instructions(full_path_was) unless import_started?
-
-    self.old_path_with_namespace = full_path_was
-    SystemHooksService.new.execute_hooks_for(self, :rename)
-
-    reload_repository!
-  end
-
   def after_import
     repository.after_import
     wiki.repository.after_import
@@ -2054,6 +2029,39 @@ class Project < ActiveRecord::Base
 
   private
 
+  def rename_or_migrate_repository!
+    if Gitlab::CurrentSettings.hashed_storage_enabled? && storage_version != LATEST_STORAGE_VERSION
+      ::Projects::HashedStorageMigrationService.new(self, full_path_was).execute
+    else
+      storage.rename_repo
+    end
+  end
+
+  def after_rename_repository(full_path_before, path_before)
+    execute_rename_repository_hooks!(full_path_before)
+
+    write_repository_config
+
+    # 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, self.path, namespace.full_path)
+    end
+
+    Gitlab::PagesTransfer.new.rename_project(path_before, self.path, namespace.full_path)
+  end
+
+  def execute_rename_repository_hooks!(full_path_before)
+    # When we import a project overwriting the original project, there
+    # is a move operation. In that case we don't want to send the instructions.
+    send_move_instructions(full_path_before) unless import_started?
+
+    self.old_path_with_namespace = full_path_before
+    SystemHooksService.new.execute_hooks_for(self, :rename)
+
+    reload_repository!
+  end
+
   def storage
     @storage ||=
       if hashed_storage?(:repository)

app/services/projects/hashed_storage/migrate_attachments_service.rb

@@ -5,18 +5,20 @@ 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, logger = nil)
+      def initialize(project, old_disk_path, logger: nil)
         @project = project
         @logger = logger || Rails.logger
+        @old_disk_path = old_disk_path
+        @new_disk_path = project.disk_path
       end
 
       def execute
-        @old_path = project.full_path
-        @new_path = project.disk_path
-
         origin = FileUploader.absolute_base_dir(project)
+        # 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)
 
@@ -32,22 +34,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

app/services/projects/hashed_storage/migrate_repository_service.rb

@@ -5,28 +5,27 @@ 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, :old_storage_version, :logger, :move_wiki
 
-      def initialize(project, logger = nil)
+      def initialize(project, old_disk_path, logger: nil)
         @project = project
         @logger = logger || Rails.logger
+        @old_disk_path = old_disk_path
+        @old_wiki_disk_path = "#{old_disk_path}.wiki"
+        @move_wiki = has_wiki?
       end
 
       def execute
-        @old_disk_path = project.disk_path
-        has_wiki = project.wiki.repository_exists?
-
         @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
-          @old_wiki_disk_path = "#{@old_disk_path}.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
@@ -48,6 +47,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")
@@ -66,8 +69,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

app/services/projects/hashed_storage_migration_service.rb

@@ -2,23 +2,26 @@
 
 module Projects
   class HashedStorageMigrationService < BaseService
-    attr_reader :logger
+    attr_reader :logger, :old_disk_path
 
-    def initialize(project, logger = nil)
+    def initialize(project, old_disk_path, logger: nil)
       @project = project
+      @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, 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, logger).execute
+        HashedStorage::MigrateAttachmentsService.new(project, old_disk_path, logger: logger).execute
       end
+
+      true
     end
   end
 end

app/services/projects/update_service.rb

@@ -4,33 +4,27 @@ module Projects
   class UpdateService < BaseService
     include UpdateVisibilityLevel
 
-    def execute
-      unless valid_visibility_level_change?(project, params[:visibility_level])
-        return error('New visibility level not allowed!')
-      end
-
-      if renaming_project_with_container_registry_tags?
-        return error('Cannot rename project because it contains container registry tags!')
-      end
+    ValidationError = Class.new(StandardError)
 
-      if changing_default_branch?
-        return error("Could not set the default branch") unless project.change_head(params[:default_branch])
-      end
+    def execute
+      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 ValidationError => e
+      error(e.message)
     end
 
     def run_auto_devops_pipeline?
@@ -41,6 +35,20 @@ module Projects
 
     private
 
+    def validate!
+      unless valid_visibility_level_change?(project, params[:visibility_level])
+        raise ValidationError.new('New visibility level not allowed!')
+      end
+
+      if renaming_project_with_container_registry_tags?
+        raise ValidationError.new('Cannot rename project because it contains container registry tags!')
+      end
+
+      if changing_default_branch?
+        raise ValidationError.new("Could not set the default branch") unless project.change_head(params[:default_branch])
+      end
+    end
+
     def after_update
       todos_features_changes = %w(
         issues_access_level
@@ -65,7 +73,7 @@ module Projects
       update_pages_config if changing_pages_https_only?
     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 +95,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

app/views/admin/application_settings/_repository_storage.html.haml

@@ -7,7 +7,7 @@
         .form-check
           = f.check_box :hashed_storage_enabled, class: 'form-check-input'
           = f.label :hashed_storage_enabled, class: 'form-check-label' do
-            Create new projects using hashed storage paths
+            Use hashed storage paths for newly created and renamed projects
           .form-text.text-muted
             Enable immutable, hash-based paths and repository names to store repositories on disk. This prevents
             repositories from having to be moved or renamed when the Project URL changes and may improve disk I/O performance.

app/workers/project_migrate_hashed_storage_worker.rb

@@ -5,13 +5,13 @@ class ProjectMigrateHashedStorageWorker
 
   LEASE_TIMEOUT = 30.seconds.to_i
 
-  def perform(project_id)
+  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, logger).execute
+      ::Projects::HashedStorageMigrationService.new(project, old_disk_path || project.full_path, logger: logger).execute
     else
       false
     end

changelogs/unreleased/46940-hashed-storage-extend-enable-hashed-storage-for-all-new-projects-to-for-all-new-and-renamed-projects.yml

+---
+title: Enable hashed storage for all newly created or renamed projects
+merge_request: 19747
+author:
+type: changed

doc/administration/repository_storage_types.md

@@ -73,7 +73,7 @@ by another folder with the next 2 characters. They are both stored in a special
 ### How to migrate to Hashed Storage
 
 In GitLab, go to **Admin > Settings**, find the **Repository Storage** section
-and select "_Create new projects using hashed storage paths_".
+and select "_Use hashed storage paths for newly created and renamed projects_".
 
 To migrate your existing projects to the new storage type, check the specific
 [rake tasks].

lib/gitlab/hashed_storage/migrator.rb

@@ -30,7 +30,7 @@ module Gitlab
         end
       end
 
-      # Flag a project to me migrated
+      # Flag a project to be migrated
       #
       # @param [Object] project that will be migrated
       def migrate(project)

qa/qa/page/admin/settings/repository_storage.rb

@@ -6,11 +6,11 @@ module QA
           view 'app/views/admin/application_settings/_repository_storage.html.haml' do
             element :submit, "submit 'Save changes'"
             element :hashed_storage,
-              'Create new projects using hashed storage paths'
+              'Use hashed storage paths for newly created and renamed projects'
           end
 
           def enable_hashed_storage
-            check 'Create new projects using hashed storage paths'
+            check 'Use hashed storage paths for newly created and renamed projects'
           end
 
           def save_settings

spec/models/project_spec.rb

@@ -3107,6 +3107,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)
 
@@ -3153,8 +3166,10 @@ describe Project do
         context 'when not rolled out' do
           let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
 
-          it 'moves pages folder to new location' do
-            expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
+          it 'moves pages folder to hashed storage' do
+            expect_next_instance_of(Projects::HashedStorage::MigrateAttachmentsService) do |service|
+              expect(service).to receive(:execute)
+            end
 
             project.rename_repo
           end

spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb

 require 'spec_helper'
 
 describe Projects::HashedStorage::MigrateAttachmentsService do
-  subject(:service) { described_class.new(project) }
+  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
 

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) }
   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 }

spec/services/projects/hashed_storage_migration_service_spec.rb

@@ -2,14 +2,19 @@ require 'spec_helper'
 
 describe Projects::HashedStorageMigrationService do
   let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) }
-  subject(:service) { described_class.new(project) }
+  let(:logger) { double }
+
+  subject(:service) { described_class.new(project, project.full_path, logger: logger) }
 
   describe '#execute' do
     context 'repository migration' do
-      let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, subject.logger) }
+      let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, project.full_path, logger: logger) }
 
       it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
-        expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_return(repository_service)
+        expect(Projects::HashedStorage::MigrateRepositoryService)
+          .to receive(:new)
+                .with(project, project.full_path, logger: logger)
+                .and_return(repository_service)
         expect(repository_service).to receive(:execute)
 
         service.execute
@@ -24,10 +29,13 @@ describe Projects::HashedStorageMigrationService do
     end
 
     context 'attachments migration' do
-      let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, subject.logger) }
+      let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, project.full_path, logger: logger) }
 
       it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
-        expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_return(attachments_service)
+        expect(Projects::HashedStorage::MigrateAttachmentsService)
+          .to receive(:new)
+                .with(project, project.full_path, logger: logger)
+                .and_return(attachments_service)
         expect(attachments_service).to receive(:execute)
 
         service.execute

spec/services/projects/update_service_spec.rb

@@ -248,6 +248,21 @@ describe Projects::UpdateService do
           expect(project.errors.messages).to have_key(:base)
           expect(project.errors.messages[:base]).to include('There is already a repository with that name on disk')
         end
+
+        context 'when hashed storage enabled' do
+          before do
+            stub_application_setting(hashed_storage_enabled: true)
+          end
+
+          it 'migrates project to a hashed storage instead of renaming the repo to another legacy name' do
+            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
+        end
       end
 
       context 'with hashed storage' do

spec/workers/project_migrate_hashed_storage_worker_spec.rb

@@ -28,7 +28,7 @@ describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do
       migration_service = spy
 
       allow(::Projects::HashedStorageMigrationService)
-        .to receive(:new).with(project, subject.logger)
+        .to receive(:new).with(project, project.full_path, logger: subject.logger)
         .and_return(migration_service)
 
       subject.perform(project.id)