Merge branch...

Merge branch '36952-migrate-existing-projects-to-hashed-storage-must-handle-design-repositories' into 'master'

Handle design repositories when moving existing projects to Hashed Storage

Closes #36952

See merge request gitlab-org/gitlab!20540

app/models/project.rb

@@ -1577,7 +1577,9 @@ class Project < ApplicationRecord
   end
 
   def wiki
-    @wiki ||= ProjectWiki.new(self, self.owner)
+    strong_memoize(:wiki) do
+      ProjectWiki.new(self, self.owner)
+    end
   end
 
   def jira_tracker_active?

app/services/projects/hashed_storage/base_repository_service.rb

@@ -8,13 +8,12 @@ module Projects
     class BaseRepositoryService < BaseService
       include Gitlab::ShellAdapter
 
-      attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :old_storage_version, :logger, :move_wiki
+      attr_reader :old_disk_path, :new_disk_path, :old_storage_version, :logger, :move_wiki
 
       def initialize(project:, old_disk_path:, logger: nil)
         @project = project
         @logger = logger || Gitlab::AppLogger
         @old_disk_path = old_disk_path
-        @old_wiki_disk_path = "#{old_disk_path}.wiki"
         @move_wiki = has_wiki?
       end
 
@@ -44,9 +43,21 @@ module Projects
         gitlab_shell.mv_repository(project.repository_storage, from_name, to_name)
       end
 
+      def move_repositories
+        result = move_repository(old_disk_path, new_disk_path)
+        project.reload_repository!
+
+        if move_wiki
+          result &&= move_repository(old_wiki_disk_path, new_wiki_disk_path)
+          project.clear_memoization(:wiki)
+        end
+
+        result
+      end
+
       def rollback_folder_move
         move_repository(new_disk_path, old_disk_path)
-        move_repository("#{new_disk_path}.wiki", old_wiki_disk_path)
+        move_repository(new_wiki_disk_path, old_wiki_disk_path)
       end
 
       def try_to_set_repository_read_only!
@@ -58,6 +69,20 @@ module Projects
           raise RepositoryInUseError, migration_error
         end
       end
+
+      def wiki_path_suffix
+        @wiki_path_suffix ||= Gitlab::GlRepository::WIKI.path_suffix
+      end
+
+      def old_wiki_disk_path
+        @old_wiki_disk_path ||= "#{old_disk_path}#{wiki_path_suffix}"
+      end
+
+      def new_wiki_disk_path
+        @new_wiki_disk_path ||= "#{new_disk_path}#{wiki_path_suffix}"
+      end
     end
   end
 end
+
+Projects::HashedStorage::BaseRepositoryService.prepend_if_ee('EE::Projects::HashedStorage::BaseRepositoryService')

app/services/projects/hashed_storage/migrate_repository_service.rb

@@ -11,11 +11,7 @@ module Projects
 
         @new_disk_path = project.disk_path
 
-        result = move_repository(old_disk_path, new_disk_path)
-
-        if move_wiki
-          result &&= move_repository(old_wiki_disk_path, "#{new_disk_path}.wiki")
-        end
+        result = move_repositories
 
         if result
           project.write_repository_config

app/services/projects/hashed_storage/rollback_repository_service.rb

@@ -11,11 +11,7 @@ module Projects
 
         @new_disk_path = project.disk_path
 
-        result = move_repository(old_disk_path, new_disk_path)
-
-        if move_wiki
-          result &&= move_repository(old_wiki_disk_path, "#{new_disk_path}.wiki")
-        end
+        result = move_repositories
 
         if result
           project.write_repository_config

db/migrate/20191204192726_add_design_disk_path_to_geo_hashed_storage_migrated_events.rb

+# frozen_string_literal: true
+
+class AddDesignDiskPathToGeoHashedStorageMigratedEvents < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :geo_hashed_storage_migrated_events, :old_design_disk_path, :text
+    add_column :geo_hashed_storage_migrated_events, :new_design_disk_path, :text
+  end
+end

db/schema.rb

@@ -1663,6 +1663,8 @@ ActiveRecord::Schema.define(version: 2019_12_06_122926) do
     t.text "new_wiki_disk_path", null: false
     t.integer "old_storage_version", limit: 2
     t.integer "new_storage_version", limit: 2, null: false
+    t.text "old_design_disk_path"
+    t.text "new_design_disk_path"
     t.index ["project_id"], name: "index_geo_hashed_storage_migrated_events_on_project_id"
   end
 

ee/app/models/ee/project.rb

@@ -672,7 +672,20 @@ module EE
     end
 
     def design_repository
-      @design_repository ||= DesignManagement::Repository.new(self)
+      strong_memoize(:design_repository) do
+        DesignManagement::Repository.new(self)
+      end
+    end
+
+    override(:expire_caches_before_rename)
+    def expire_caches_before_rename(old_path)
+      super
+
+      design = ::Repository.new("#{old_path}#{EE::Gitlab::GlRepository::DESIGN.path_suffix}", self)
+
+      if design.exists?
+        design.before_delete
+      end
     end
 
     def alerts_service_available?

ee/app/services/ee/projects/hashed_storage/base_repository_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Projects
+    module HashedStorage
+      module BaseRepositoryService
+        extend ::Gitlab::Utils::Override
+
+        attr_reader :move_design
+
+        override :initialize
+        def initialize(project:, old_disk_path:, logger: nil)
+          super
+          @move_design = has_design?
+        end
+
+        protected
+
+        def has_design?
+          gitlab_shell.repository_exists?(project.repository_storage, "#{old_design_disk_path}.git")
+        end
+
+        override :move_repositories
+        def move_repositories
+          result = super
+
+          if move_design
+            result &&= move_repository(old_design_disk_path, new_design_disk_path)
+            project.clear_memoization(:design_repository)
+          end
+
+          result
+        end
+
+        override :rollback_folder_move
+        def rollback_folder_move
+          super
+
+          if move_design
+            move_repository(new_design_disk_path, old_design_disk_path)
+          end
+        end
+
+        def design_path_suffix
+          @design_path_suffix ||= EE::Gitlab::GlRepository::DESIGN.path_suffix
+        end
+
+        def old_design_disk_path
+          @old_design_disk_path ||= "#{old_disk_path}#{design_path_suffix}"
+        end
+
+        def new_design_disk_path
+          @new_design_disk_path ||= "#{new_disk_path}#{design_path_suffix}"
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/projects/hashed_storage/migrate_repository_service.rb

@@ -13,7 +13,8 @@ module EE
               project,
               old_storage_version: old_storage_version,
               old_disk_path: old_disk_path,
-              old_wiki_disk_path: old_wiki_disk_path
+              old_wiki_disk_path: old_wiki_disk_path,
+              old_design_disk_path: old_design_disk_path
             ).create!
           end
         end

ee/app/services/geo/hashed_storage_migrated_event_store.rb

@@ -15,7 +15,9 @@ module Geo
         old_disk_path: old_disk_path,
         new_disk_path: project.disk_path,
         old_wiki_disk_path: old_wiki_disk_path,
-        new_wiki_disk_path: project.wiki.disk_path
+        new_wiki_disk_path: project.wiki.disk_path,
+        old_design_disk_path: old_design_disk_path,
+        new_design_disk_path: project.design_repository.disk_path
       )
     end
 
@@ -30,5 +32,9 @@ module Geo
     def old_wiki_disk_path
       params.fetch(:old_wiki_disk_path)
     end
+
+    def old_design_disk_path
+      params.fetch(:old_design_disk_path, nil)
+    end
   end
 end

ee/app/services/geo/move_repository_service.rb

@@ -45,6 +45,11 @@ module Geo
         return false
       end
 
+      if project.design_repository.exists? && !move_design_repository
+        log_error('Design repository cannot be moved')
+        return false
+      end
+
       true
     rescue => ex
       log_error('Repository cannot be moved', error: ex)
@@ -56,7 +61,35 @@ module Geo
     end
 
     def move_wiki_repository
-      gitlab_shell.mv_repository(project.repository_storage, "#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
+      gitlab_shell.mv_repository(project.repository_storage, old_wiki_disk_path, new_wiki_disk_path)
+    end
+
+    def move_design_repository
+      gitlab_shell.mv_repository(project.repository_storage, old_design_disk_path, new_design_disk_path)
+    end
+
+    def wiki_path_suffix
+      Gitlab::GlRepository::WIKI.path_suffix
+    end
+
+    def old_wiki_disk_path
+      "#{old_disk_path}#{wiki_path_suffix}"
+    end
+
+    def new_wiki_disk_path
+      "#{new_disk_path}#{wiki_path_suffix}"
+    end
+
+    def design_path_suffix
+      EE::Gitlab::GlRepository::DESIGN.path_suffix
+    end
+
+    def old_design_disk_path
+      "#{old_disk_path}#{design_path_suffix}"
+    end
+
+    def new_design_disk_path
+      "#{new_disk_path}#{design_path_suffix}"
     end
   end
 end

ee/changelogs/unreleased/36952-migrate-existing-projects-to-hashed-storage-must-handle-design-r.yml

+---
+title: Handle design repositories when moving existing projects to Hashed Storage
+merge_request: 20540
+author:
+type: fixed

ee/spec/models/project_spec.rb

@@ -2383,4 +2383,29 @@ describe Project do
   describe '#license_compliance' do
     it { expect(subject.license_compliance).to be_instance_of(::SCA::LicenseCompliance) }
   end
+
+  describe '#expire_caches_before_rename' do
+    let(:project) { create(:project, :repository) }
+    let(:repo)    { double(:repo, exists?: true, before_delete: true) }
+    let(:wiki)    { double(:wiki, exists?: true, before_delete: true) }
+    let(:design)  { double(:design, exists?: true) }
+
+    it 'expires the caches of the design repository' do
+      allow(Repository).to receive(:new)
+        .with('foo', project)
+        .and_return(repo)
+
+      allow(Repository).to receive(:new)
+        .with('foo.wiki', project)
+        .and_return(wiki)
+
+      allow(Repository).to receive(:new)
+        .with('foo.design', project)
+        .and_return(design)
+
+      expect(design).to receive(:before_delete)
+
+      project.expire_caches_before_rename('foo')
+    end
+  end
 end

ee/spec/services/geo/hashed_storage_migrated_event_store_spec.rb

@@ -7,11 +7,20 @@ describe Geo::HashedStorageMigratedEventStore do
 
   set(:secondary_node) { create(:geo_node) }
 
-  let(:project) { create(:project, path: 'bar') }
+  let(:project) { create(:project, :design_repo, path: 'bar') }
   let(:old_disk_path) { "#{project.namespace.full_path}/foo" }
   let(:old_wiki_disk_path) { "#{old_disk_path}.wiki" }
-
-  subject { described_class.new(project, old_storage_version: nil, old_disk_path: old_disk_path, old_wiki_disk_path: old_wiki_disk_path) }
+  let(:old_design_disk_path) { "#{old_disk_path}.design" }
+
+  subject do
+    described_class.new(
+      project,
+      old_storage_version: nil,
+      old_disk_path: old_disk_path,
+      old_wiki_disk_path: old_wiki_disk_path,
+      old_design_disk_path: old_design_disk_path
+    )
+  end
 
   before do
     TestEnv.clean_test_path
@@ -35,7 +44,9 @@ describe Geo::HashedStorageMigratedEventStore do
           old_disk_path: old_disk_path,
           new_disk_path: project.disk_path,
           old_wiki_disk_path: old_wiki_disk_path,
-          new_wiki_disk_path: project.wiki.disk_path
+          new_wiki_disk_path: project.wiki.disk_path,
+          old_design_disk_path: old_design_disk_path,
+          new_design_disk_path: project.design_repository.disk_path
         )
       end
     end

ee/spec/services/geo/move_repository_service_spec.rb

@@ -59,6 +59,30 @@ describe Geo::MoveRepositoryService, :geo do
       expect(service.execute).to eq false
     end
 
+    context 'when design repository exists' do
+      before do
+        project.design_repository.create_if_not_exists
+      end
+
+      it 'returns false when design repository can not be renamed' do
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage, old_path, new_path)
+          .and_return(true)
+
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage, "#{old_path}.wiki", "#{new_path}.wiki")
+          .and_return(true)
+
+        allow_any_instance_of(Gitlab::Shell).to receive(:mv_repository)
+          .with(project.repository_storage, "#{old_path}.design", "#{new_path}.design")
+          .and_return(false)
+
+        expect(service).to receive(:log_error).with('Design repository cannot be moved')
+
+        expect(service.execute).to eq false
+      end
+    end
+
     context 'wiki disabled' do
       let(:project) { create(:project, :repository, :wiki_disabled, :legacy_storage) }
 

ee/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb

@@ -5,45 +5,109 @@ require 'spec_helper'
 describe Projects::HashedStorage::MigrateRepositoryService do
   include EE::GeoHelpers
 
-  let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) }
+  let(:gitlab_shell) { Gitlab::Shell.new }
+  let(:project) { create(:project, :empty_repo, :wiki_repo, :design_repo, :legacy_storage) }
   let(:legacy_storage) { Storage::LegacyProject.new(project) }
   let(:hashed_storage) { Storage::HashedProject.new(project) }
+  let(:old_disk_path) { legacy_storage.disk_path }
+  let(:new_disk_path) { hashed_storage.disk_path }
 
-  subject { described_class.new(project: project, old_disk_path: legacy_storage.disk_path) }
+  subject(:service) { described_class.new(project: project, old_disk_path: old_disk_path) }
 
   describe '#execute' do
-    set(:primary) { create(:geo_node, :primary) }
-    set(:secondary) { create(:geo_node) }
+    context 'when a project has a design repository' do
+      before do
+        allow(service).to receive(:gitlab_shell) { gitlab_shell }
+      end
 
-    before do
-      TestEnv.clean_test_path
-      stub_current_geo_node(primary)
-    end
+      context 'when succeeds' do
+        it 'renames project, wiki and design repositories' do
+          service.execute
+
+          expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy
+          expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy
+          expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_truthy
+        end
+
+        it 'move operation is called for each repository' do
+          expect_move_repository(old_disk_path, new_disk_path)
+          expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
+          expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design")
+
+          service.execute
+        end
+      end
+
+      context 'when one move fails' do
+        it 'rollsback repositories to original name' do
+          allow(service).to receive(:move_repository).and_call_original
+          allow(service).to receive(:move_repository).with(old_disk_path, new_disk_path).once { false } # will disable first move only
+
+          expect(service).to receive(:rollback_folder_move).and_call_original
+
+          service.execute
 
-    it 'creates a Geo::HashedStorageMigratedEvent on success' do
-      expect { subject.execute }.to change(Geo::EventLog, :count).by(1)
+          expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey
+          expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey
+          expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_falsey
+          expect(project.repository_read_only?).to be_falsey
+        end
 
-      event = Geo::EventLog.first.event
+        context 'when rollback fails' do
+          before do
+            gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path)
+          end
 
-      expect(event).to be_a(Geo::HashedStorageMigratedEvent)
-      expect(event).to have_attributes(
-        old_storage_version: nil,
-        new_storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository],
-        old_disk_path: legacy_storage.disk_path,
-        new_disk_path: hashed_storage.disk_path,
-        old_wiki_disk_path: legacy_storage.disk_path + '.wiki',
-        new_wiki_disk_path: hashed_storage.disk_path + '.wiki'
-      )
+          it 'does not try to move nil repository over existing' do
+            expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, old_disk_path, new_disk_path)
+            expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
+            expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design")
+
+            service.execute
+          end
+        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
     end
 
-    it 'does not create a Geo event on failure' do
-      from_name = project.disk_path
-      to_name = hashed_storage.disk_path
+    context 'when running on a Geo primary node' do
+      let_it_be(:primary) { create(:geo_node, :primary) }
+      let_it_be(:secondary) { create(:geo_node) }
+
+      before do
+        stub_current_geo_node(primary)
+      end
+
+      it 'creates a Geo::HashedStorageMigratedEvent on success' do
+        expect { service.execute }.to change(Geo::EventLog, :count).by(1)
+
+        event = Geo::EventLog.first.event
+
+        expect(event).to be_a(Geo::HashedStorageMigratedEvent)
+        expect(event).to have_attributes(
+          old_storage_version: nil,
+          new_storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository],
+          old_disk_path: legacy_storage.disk_path,
+          new_disk_path: hashed_storage.disk_path,
+          old_wiki_disk_path: legacy_storage.disk_path + '.wiki',
+          new_wiki_disk_path: hashed_storage.disk_path + '.wiki',
+          old_design_disk_path: legacy_storage.disk_path + '.design',
+          new_design_disk_path: hashed_storage.disk_path + '.design'
+        )
+      end
+
+      it 'does not create a Geo event on failure' do
+        from_name = project.disk_path
+        to_name = hashed_storage.disk_path
 
-      allow(subject).to receive(:move_repository).and_call_original
-      allow(subject).to receive(:move_repository).with(from_name, to_name).once { false } # will disable first move only
+        allow(service).to receive(:move_repository).and_call_original
+        allow(service).to receive(:move_repository).with(from_name, to_name).once { false } # will disable first move only
 
-      expect { subject.execute }.not_to change { Geo::EventLog.count }
+        expect { service.execute }.not_to change { Geo::EventLog.count }
+      end
     end
   end
 end

ee/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis_shared_state do
+  include GitHelpers
+
+  let(:gitlab_shell) { Gitlab::Shell.new }
+  let(:project) { create(:project, :repository, :wiki_repo, :design_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) }
+  let(:legacy_storage) { Storage::LegacyProject.new(project) }
+  let(:hashed_storage) { Storage::HashedProject.new(project) }
+  let(:old_disk_path) { hashed_storage.disk_path }
+  let(:new_disk_path) { legacy_storage.disk_path }
+
+  subject(:service) { described_class.new(project: project, old_disk_path: project.disk_path) }
+
+  describe '#execute' do
+    before do
+      allow(service).to receive(:gitlab_shell) { gitlab_shell }
+    end
+
+    context 'when succeeds' do
+      it 'renames project, wiki and design repositories' do
+        service.execute
+
+        expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy
+        expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy
+        expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_truthy
+      end
+
+      it 'updates project to be legacy and not read-only' do
+        service.execute
+
+        expect(project.legacy_storage?).to be_truthy
+        expect(project.repository_read_only).to be_falsey
+      end
+
+      it 'move operation is called for each repository' do
+        expect_move_repository(old_disk_path, new_disk_path)
+        expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
+        expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design")
+
+        service.execute
+      end
+    end
+
+    context 'when one move fails' do
+      it 'rolls repositories back to original name' do
+        allow(service).to receive(:move_repository).and_call_original
+        allow(service).to receive(:move_repository).with(old_disk_path, new_disk_path).once { false } # will disable first move only
+
+        expect(service).to receive(:rollback_folder_move).and_call_original
+
+        service.execute
+
+        expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey
+        expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey
+        expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_falsey
+        expect(project.repository_read_only?).to be_falsey
+      end
+
+      context 'when rollback fails' do
+        before do
+          gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path)
+        end
+
+        it 'does not try to move nil repository over existing' do
+          expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, old_disk_path, new_disk_path)
+          expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki")
+          expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design")
+
+          service.execute
+        end
+      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
+  end
+end

spec/models/project_spec.rb

@@ -1793,6 +1793,7 @@ describe Project do
     let(:project) { create(:project, :repository) }
     let(:repo)    { double(:repo, exists?: true) }
     let(:wiki)    { double(:wiki, exists?: true) }
+    let(:design)  { double(:wiki, exists?: false) }
 
     it 'expires the caches of the repository and wiki' do
       allow(Repository).to receive(:new)
@@ -1803,6 +1804,10 @@ describe Project do
         .with('foo.wiki', project)
         .and_return(wiki)
 
+      allow(Repository).to receive(:new)
+        .with('foo.design', project)
+        .and_return(design)
+
       expect(repo).to receive(:before_delete)
       expect(wiki).to receive(:before_delete)