From fe158ad15339577bb3bfa1ab8b388c5f27f90514 Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 4 Sep 2019 17:36:15 -0700 Subject: [PATCH] Call RenameRepository instead of RenameNamespace call RenameRepository RPC instead of RenameNamespace in gitlab_shell. This is a step towards deprecating all RenameNamespace calls. --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +-- .../concerns/storage/legacy_namespace.rb | 20 ++++-------- app/models/storage/legacy_project.rb | 2 +- .../hashed_storage/base_repository_service.rb | 12 ++----- .../migrate_repository_service.rb | 1 - .../jc-legacy-storage-to-hashed-storage.yml | 5 +++ lib/gitlab/git/repository.rb | 6 ++++ .../gitaly_client/repository_service.rb | 6 ++++ lib/gitlab/shell.rb | 16 ++++++++-- .../v1/rename_projects_spec.rb | 32 ++++++++++++++++++- spec/models/namespace_spec.rb | 6 ++-- 13 files changed, 78 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/jc-legacy-storage-to-hashed-storage.yml diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 76d05362056..af92bdd9f58 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.62.0 +1.63.0 diff --git a/Gemfile b/Gemfile index ac848cce5e8..4b7c498d39d 100644 --- a/Gemfile +++ b/Gemfile @@ -428,7 +428,7 @@ group :ed25519 do end # Gitaly GRPC protocol definitions -gem 'gitaly', '~> 1.58.0' +gem 'gitaly', '~> 1.63.0' gem 'grpc', '~> 1.19.0' diff --git a/Gemfile.lock b/Gemfile.lock index 48053e5740e..7c7e69232a2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -332,7 +332,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) git (1.5.0) - gitaly (1.58.0) + gitaly (1.63.0) grpc (~> 1.0) github-markup (1.7.0) gitlab-labkit (0.5.2) @@ -1126,7 +1126,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly (~> 1.58.0) + gitaly (~> 1.63.0) github-markup (~> 1.7.0) gitlab-labkit (~> 0.5) gitlab-markup (~> 1.7.0) diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 78544405c49..21bef0381ed 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -54,25 +54,19 @@ module Storage private def move_repositories - # Move the namespace directory in all storages used by member projects - repository_storages.each do |repository_storage| - # Ensure old directory exists before moving it - gitlab_shell.add_namespace(repository_storage, full_path_before_last_save) + all_projects.each do |project| + next unless project.legacy_storage? - # Ensure new directory exists before moving it (if there's a parent) - gitlab_shell.add_namespace(repository_storage, parent.full_path) if parent - - unless gitlab_shell.mv_namespace(repository_storage, full_path_before_last_save, full_path) - - Rails.logger.error "Exception moving path #{repository_storage} from #{full_path_before_last_save} to #{full_path}" # rubocop:disable Gitlab/RailsLogger - - # if we cannot move namespace directory we should rollback - # db changes in order to prevent out of sync between db and fs + unless gitlab_shell.mv_repository(project.repository_storage, full_repo_path_before_last_save(project), project.disk_path ) raise Gitlab::UpdatePathError.new('namespace directory cannot be moved') end end end + def full_repo_path_before_last_save(project) + project.disk_path.gsub(full_path, full_path_before_last_save) + end + def old_repository_storages @old_repository_storage_paths ||= repository_storages end diff --git a/app/models/storage/legacy_project.rb b/app/models/storage/legacy_project.rb index 928c773c307..933dee673b2 100644 --- a/app/models/storage/legacy_project.rb +++ b/app/models/storage/legacy_project.rb @@ -38,7 +38,7 @@ module Storage # However we cannot allow rollback since we moved repository # So we basically we mute exceptions in next actions begin - gitlab_shell.mv_repository(repository_storage, "#{old_full_path}.wiki", "#{new_full_path}.wiki") + gitlab_shell.mv_wiki(repository_storage, old_full_path, new_full_path) if project.wiki.exists? return true rescue => e Rails.logger.error "Exception renaming #{old_full_path} -> #{new_full_path}: #{e}" # rubocop:disable Gitlab/RailsLogger diff --git a/app/services/projects/hashed_storage/base_repository_service.rb b/app/services/projects/hashed_storage/base_repository_service.rb index f97a28b8c3b..5c561984de1 100644 --- a/app/services/projects/hashed_storage/base_repository_service.rb +++ b/app/services/projects/hashed_storage/base_repository_service.rb @@ -29,20 +29,12 @@ module Projects # rubocop: disable CodeReuse/ActiveRecord 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") # If we don't find the repository on either original or target we should log that as it could be an issue if the # project was not originally empty. - if !from_exists && !to_exists - logger.warn "Can't find a repository on either source or target paths for #{project.full_path} (ID=#{project.id}) ..." - # We return true so we still reflect the change in the database. - # Next time the repository is (re)created it will be under the new storage layout - return true - elsif !from_exists - # Repository have been moved already. - return true - end + # Repository have been moved already. + return true unless from_exists gitlab_shell.mv_repository(project.repository_storage, from_name, to_name) end diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index e8393128d58..d19946af9c6 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -8,7 +8,6 @@ module Projects @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 diff --git a/changelogs/unreleased/jc-legacy-storage-to-hashed-storage.yml b/changelogs/unreleased/jc-legacy-storage-to-hashed-storage.yml new file mode 100644 index 00000000000..81f34c5c6d6 --- /dev/null +++ b/changelogs/unreleased/jc-legacy-storage-to-hashed-storage.yml @@ -0,0 +1,5 @@ +--- +title: Call RenameRepository instead of RenameNamespace +merge_request: 32684 +author: +type: deprecated diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 4ea618f063b..45935bb8a09 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -131,6 +131,12 @@ module Gitlab end end + def rename(new_relative_path) + wrapped_gitaly_errors do + gitaly_repository_client.rename(new_relative_path) + end + end + def expire_has_local_branches_cache clear_memoization(:has_local_branches) end diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb index ca3e5b51ecc..631af3fa2f0 100644 --- a/lib/gitlab/gitaly_client/repository_service.rb +++ b/lib/gitlab/gitaly_client/repository_service.rb @@ -346,6 +346,12 @@ module Gitlab GitalyClient.call(@storage, :object_pool_service, :disconnect_git_alternates, request) end + def rename(relative_path) + request = Gitaly::RenameRepositoryRequest.new(repository: @gitaly_repo, relative_path: relative_path) + + GitalyClient.call(@storage, :repository_service, :rename_repository, request, timeout: GitalyClient.fast_timeout) + end + private def search_results_from_response(gitaly_response) diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 9e813968093..d97f11e6675 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -117,16 +117,26 @@ module Gitlab # mv_namespace. Given the underlying implementation is a move action, # indescriminate of what the folders might be. # - # storage - project's storage path + # storage - project's storage name # path - project disk path # new_path - new project disk path # # Ex. - # mv_repository("/path/to/storage", "gitlab/gitlab-ci", "randx/gitlab-ci-new") + # mv_repository("default", "gitlab/gitlab-ci", "randx/gitlab-ci-new") def mv_repository(storage, path, new_path) return false if path.empty? || new_path.empty? - !!mv_directory(storage, "#{path}.git", "#{new_path}.git") + Gitlab::Git::Repository.new(storage, "#{path}.git", nil, nil).rename("#{new_path}.git") + rescue => e + Gitlab::Sentry.track_acceptable_exception(e, extra: { path: path, new_path: new_path, storage: storage }) + + false + end + + def mv_wiki(storage, path, new_path) + return false if path.empty? || new_path.empty? + + Gitlab::Git::Repository.new(storage, "#{path}.wiki.git", nil, nil).rename("#{new_path}.wiki.git") end # Fork repository to new path diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb index 1ccdb1d9447..9474febd578 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb @@ -21,6 +21,8 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de it 'searches using nested paths' do namespace = create(:namespace, path: 'hello') project = create(:project, :legacy_storage, path: 'THE-path', namespace: namespace) + project.create_repository + project.create_wiki result_ids = described_class.new(['Hello/the-path'], migration) .projects_for_paths.map(&:id) @@ -30,6 +32,9 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de it 'includes the correct projects' do project = create(:project, :legacy_storage, path: 'THE-path') + project.create_repository + project.create_wiki + _other_project = create(:project, :legacy_storage) result_ids = subject.projects_for_paths.map(&:id) @@ -41,6 +46,13 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de describe '#rename_projects' do let!(:projects) { create_list(:project, 2, :legacy_storage, path: 'the-path') } + before do + projects.each do |p| + p.create_repository + p.create_wiki + end + end + it 'renames each project' do expect(subject).to receive(:rename_project).twice @@ -56,6 +68,10 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de end describe '#rename_project' do + before do + project.create_repository + project.create_wiki + end it 'renames path & route for the project' do expect(subject).to receive(:rename_path_for_routable) .with(project) @@ -81,6 +97,11 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de end describe '#move_project_folders' do + before do + project.create_repository + project.create_wiki + end + it 'moves the wiki & the repo' do expect(subject).to receive(:move_repository) .with(project, 'known-parent/the-path.wiki', 'known-parent/the-path0.wiki') @@ -125,6 +146,11 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de let(:known_parent) { create(:namespace, path: 'known-parent') } let(:project) { create(:project, :repository, :legacy_storage, path: 'the-path', namespace: known_parent) } + before do + project.create_repository + project.create_wiki + end + it 'moves the repository for a project' do expected_path = File.join(TestEnv.repos_path, 'known-parent', 'new-repo.git') @@ -135,6 +161,11 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de end describe '#revert_renames', :redis do + before do + project.create_repository + project.create_wiki + end + it 'renames the routes back to the previous values' do subject.rename_project(project) @@ -152,7 +183,6 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :de end it 'moves the repositories back to their original place' do - project.create_repository subject.rename_project(project) expected_path = File.join(TestEnv.repos_path, 'known-parent', 'the-path.git') diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 972f26ac745..b361b1b524d 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -362,9 +362,9 @@ describe Namespace do it 'updates project full path in .git/config' do parent.update(path: 'mygroup_new') - expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}" - expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}" - expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}" + expect(project_rugged(project_in_parent_group.reload).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}" + expect(project_rugged(hashed_project_in_subgroup.reload).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}" + expect(project_rugged(legacy_project_in_subgroup.reload).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}" end it 'updates the project storage location' do -- GitLab