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

7 files changed, 108 insertions, 84 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 16d63639141..02e956911a9 100644
--- a/app/models/project.rb
+++ b/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)
diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb
index 649c916a593..a1f0302aeb7 100644
--- a/app/services/projects/hashed_storage/migrate_attachments_service.rb
+++ b/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
diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb
index 70f00b7fdeb..641d46e6591 100644
--- a/app/services/projects/hashed_storage/migrate_repository_service.rb
+++ b/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
diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb
index 1828c99a65e..a0e734005f8 100644
--- a/app/services/projects/hashed_storage_migration_service.rb
+++ b/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
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index 31ab4fbe49e..97f181ccea8 100644
--- a/app/services/projects/update_service.rb
+++ b/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
diff --git a/app/views/admin/application_settings/_repository_storage.html.haml b/app/views/admin/application_settings/_repository_storage.html.haml
index 685de99917e..596385abcb1 100644
--- a/app/views/admin/application_settings/_repository_storage.html.haml
+++ b/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.
diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb
index 9e4d66250a4..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)
+  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