Merge branch '211452-rollback_to_legacy-causes-repos-to-404' into 'master'

Fix regression on storage rollback causing repositories to report as 404

Closes #211452

See merge request gitlab-org/gitlab!28496

app/services/projects/hashed_storage/base_repository_service.rb

@@ -40,7 +40,13 @@ module Projects
           return true
         end
 
-        gitlab_shell.mv_repository(project.repository_storage, from_name, to_name)
+        gitlab_shell.mv_repository(project.repository_storage, from_name, to_name).tap do |moved|
+          if moved
+            logger.info("Repository moved from '#{from_name}' to '#{to_name}' (PROJECT_ID=#{project.id})")
+          else
+            logger.error("Repository cannot be moved from '#{from_name}' to '#{to_name}' (PROJECT_ID=#{project.id})")
+          end
+        end
       end
 
       def move_repositories

app/services/projects/hashed_storage/rollback_service.rb

@@ -5,6 +5,12 @@ module Projects
     class RollbackService < BaseService
       attr_reader :logger, :old_disk_path
 
+      def initialize(project, old_disk_path, logger: nil)
+        @project = project
+        @old_disk_path = old_disk_path
+        @logger = logger || Gitlab::AppLogger
+      end
+
       def execute
         # Rollback attachments from Hashed Storage to Legacy
         if project.hashed_storage?(:attachments)

changelogs/unreleased/211452-rollback_to_legacy-causes-repos-to-404.yml

+---
+title: Fix storage rollback regression caused by previous refactor
+merge_request: 28496
+author:
+type: fixed

spec/services/projects/hashed_storage/migration_service_spec.rb

@@ -5,6 +5,11 @@ require 'spec_helper'
 describe Projects::HashedStorage::MigrationService do
   let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) }
   let(:logger) { double }
+  let!(:project_attachment) { build(:file_uploader, project: project) }
+  let(:project_hashed_path) { Storage::Hashed.new(project).disk_path }
+  let(:project_legacy_path) { Storage::LegacyProject.new(project).disk_path }
+  let(:wiki_hashed_path) { "#{project_hashed_path}.wiki" }
+  let(:wiki_legacy_path) { "#{project_legacy_path}.wiki" }
 
   subject(:service) { described_class.new(project, project.full_path, logger: logger) }
 
@@ -29,9 +34,24 @@ describe Projects::HashedStorage::MigrationService do
 
         service.execute
       end
+
+      it 'migrates legacy repositories to hashed storage' do
+        legacy_attachments_path = FileUploader.absolute_base_dir(project)
+        hashed_project = project.dup.tap { |p| p.id = project.id }
+        hashed_project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments]
+        hashed_attachments_path = FileUploader.absolute_base_dir(hashed_project)
+
+        expect(logger).to receive(:info).with(/Repository moved from '#{project_legacy_path}' to '#{project_hashed_path}'/)
+        expect(logger).to receive(:info).with(/Repository moved from '#{wiki_legacy_path}' to '#{wiki_hashed_path}'/)
+        expect(logger).to receive(:info).with(/Project attachments moved from '#{legacy_attachments_path}' to '#{hashed_attachments_path}'/)
+
+        expect { service.execute }.to change { project.storage_version }.from(nil).to(2)
+      end
     end
 
     context 'attachments migration' do
+      let(:project) { create(:project, :empty_repo, :wiki_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) }
+
       let(:attachments_service) do
         Projects::HashedStorage::MigrateAttachmentsService.new(project: project,
                                                                old_disk_path: project.full_path,
@@ -51,6 +71,17 @@ describe Projects::HashedStorage::MigrationService do
 
         service.execute
       end
+
+      it 'migrates legacy attachments to hashed storage' do
+        legacy_attachments_path = FileUploader.absolute_base_dir(project)
+        hashed_project = project.dup.tap { |p| p.id = project.id }
+        hashed_project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments]
+        hashed_attachments_path = FileUploader.absolute_base_dir(hashed_project)
+
+        expect(logger).to receive(:info).with(/Project attachments moved from '#{legacy_attachments_path}' to '#{hashed_attachments_path}'/)
+
+        expect { service.execute }.to change { project.storage_version }.from(1).to(2)
+      end
     end
   end
 end

spec/services/projects/hashed_storage/rollback_service_spec.rb

@@ -5,6 +5,11 @@ require 'spec_helper'
 describe Projects::HashedStorage::RollbackService do
   let(:project) { create(:project, :empty_repo, :wiki_repo) }
   let(:logger) { double }
+  let!(:project_attachment) { build(:file_uploader, project: project) }
+  let(:project_hashed_path) { Storage::Hashed.new(project).disk_path }
+  let(:project_legacy_path) { Storage::LegacyProject.new(project).disk_path }
+  let(:wiki_hashed_path) { "#{project_hashed_path}.wiki" }
+  let(:wiki_legacy_path) { "#{project_legacy_path}.wiki" }
 
   subject(:service) { described_class.new(project, project.disk_path, logger: logger) }
 
@@ -26,6 +31,20 @@ describe Projects::HashedStorage::RollbackService do
 
         service.execute
       end
+
+      it 'rollbacks to legacy storage' do
+        hashed_attachments_path = FileUploader.absolute_base_dir(project)
+        legacy_project = project.dup
+        legacy_project.storage_version = nil
+        legacy_attachments_path = FileUploader.absolute_base_dir(legacy_project)
+
+        expect(logger).to receive(:info).with(/Project attachments moved from '#{hashed_attachments_path}' to '#{legacy_attachments_path}'/)
+
+        expect(logger).to receive(:info).with(/Repository moved from '#{project_hashed_path}' to '#{project_legacy_path}'/)
+        expect(logger).to receive(:info).with(/Repository moved from '#{wiki_hashed_path}' to '#{wiki_legacy_path}'/)
+
+        expect { service.execute }.to change { project.storage_version }.from(2).to(nil)
+      end
     end
 
     context 'repository rollback' do
@@ -47,6 +66,13 @@ describe Projects::HashedStorage::RollbackService do
 
         service.execute
       end
+
+      it 'rollbacks to legacy storage' do
+        expect(logger).to receive(:info).with(/Repository moved from '#{project_hashed_path}' to '#{project_legacy_path}'/)
+        expect(logger).to receive(:info).with(/Repository moved from '#{wiki_hashed_path}' to '#{wiki_legacy_path}'/)
+
+        expect { service.execute }.to change { project.storage_version }.from(1).to(nil)
+      end
     end
   end
 end