Commit dd19e278 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre

Merge branch '56636-hashed-storage-afterrenameservice' into 'master'

Hashed Storage: `AfterRenameService` was not renaming Pages or Uploads folder on legacy storage

Closes #56636

See merge request gitlab-org/gitlab-ce!24526
parents 3de32dd2 d391dfb4
...@@ -31,7 +31,7 @@ module Storage ...@@ -31,7 +31,7 @@ module Storage
gitlab_shell.add_namespace(repository_storage, base_dir) gitlab_shell.add_namespace(repository_storage, base_dir)
end end
def rename_repo def rename_repo(old_full_path: nil, new_full_path: nil)
true true
end end
......
...@@ -29,18 +29,19 @@ module Storage ...@@ -29,18 +29,19 @@ module Storage
gitlab_shell.add_namespace(repository_storage, base_dir) gitlab_shell.add_namespace(repository_storage, base_dir)
end end
def rename_repo def rename_repo(old_full_path: nil, new_full_path: nil)
new_full_path = project.build_full_path old_full_path ||= project.full_path_was
new_full_path ||= project.build_full_path
if gitlab_shell.mv_repository(repository_storage, project.full_path_was, new_full_path) if gitlab_shell.mv_repository(repository_storage, old_full_path, new_full_path)
# If repository moved successfully we need to send update instructions to users. # If repository moved successfully we need to send update instructions to users.
# However we cannot allow rollback since we moved repository # However we cannot allow rollback since we moved repository
# So we basically we mute exceptions in next actions # So we basically we mute exceptions in next actions
begin begin
gitlab_shell.mv_repository(repository_storage, "#{project.full_path_was}.wiki", "#{new_full_path}.wiki") gitlab_shell.mv_repository(repository_storage, "#{old_full_path}.wiki", "#{new_full_path}.wiki")
return true return true
rescue => e rescue => e
Rails.logger.error "Exception renaming #{project.full_path_was} -> #{new_full_path}: #{e}" Rails.logger.error "Exception renaming #{old_full_path} -> #{new_full_path}: #{e}"
# Returning false does not rollback after_* transaction but gives # Returning false does not rollback after_* transaction but gives
# us information about failing some of tasks # us information about failing some of tasks
return false return false
......
...@@ -12,22 +12,27 @@ module Projects ...@@ -12,22 +12,27 @@ module Projects
# #
# Projects::AfterRenameService.new(project).execute # Projects::AfterRenameService.new(project).execute
class AfterRenameService class AfterRenameService
attr_reader :project, :full_path_before, :full_path_after, :path_before # @return [String] The Project being renamed.
attr_reader :project
RenameFailedError = Class.new(StandardError) # @return [String] The path slug the project was using, before the rename took place.
attr_reader :path_before
# @param [Project] project The Project of the repository to rename. # @return [String] The full path of the namespace + project, before the rename took place.
def initialize(project) attr_reader :full_path_before
@project = project
# The full path of the namespace + project, before the rename took place. # @return [String] The full path of the namespace + project, after the rename took place.
@full_path_before = project.full_path_was attr_reader :full_path_after
# The full path of the namespace + project, after the rename took place. RenameFailedError = Class.new(StandardError)
@full_path_after = project.build_full_path
# The path of just the project, before the rename took place. # @param [Project] project The Project being renamed.
@path_before = project.path_was # @param [String] path_before The path slug the project was using, before the rename took place.
def initialize(project, path_before:, full_path_before:)
@project = project
@path_before = path_before
@full_path_before = full_path_before
@full_path_after = project.full_path
end end
def execute def execute
...@@ -61,7 +66,7 @@ module Projects ...@@ -61,7 +66,7 @@ module Projects
.new(project, full_path_before) .new(project, full_path_before)
.execute .execute
else else
project.storage.rename_repo project.storage.rename_repo(old_full_path: full_path_before, new_full_path: full_path_after)
end end
rename_failed! unless success rename_failed! unless success
......
...@@ -67,7 +67,7 @@ module Projects ...@@ -67,7 +67,7 @@ module Projects
end end
if project.previous_changes.include?('path') if project.previous_changes.include?('path')
AfterRenameService.new(project).execute after_rename_service(project).execute
else else
system_hook_service.execute_hooks_for(project, :update) system_hook_service.execute_hooks_for(project, :update)
end end
...@@ -75,6 +75,13 @@ module Projects ...@@ -75,6 +75,13 @@ module Projects
update_pages_config if changing_pages_related_config? update_pages_config if changing_pages_related_config?
end end
def after_rename_service(project)
# The path slug the project was using, before the rename took place.
path_before = project.previous_changes['path'].first
AfterRenameService.new(project, path_before: path_before, full_path_before: project.full_path_was)
end
def changing_pages_related_config? def changing_pages_related_config?
changing_pages_https_only? || changing_pages_access_level? changing_pages_https_only? || changing_pages_access_level?
end end
......
---
title: 'Hashed Storage: `AfterRenameService` was receiving the wrong `old_path` under some circunstances'
merge_request: 24526
author:
type: fixed
...@@ -113,7 +113,7 @@ class RenameReservedProjectNames < ActiveRecord::Migration[4.2] ...@@ -113,7 +113,7 @@ class RenameReservedProjectNames < ActiveRecord::Migration[4.2]
# Because project path update is quite complex operation we can't safely # Because project path update is quite complex operation we can't safely
# copy-paste all code from GitLab. As exception we use Rails code here # copy-paste all code from GitLab. As exception we use Rails code here
if rename_project_row(project, path) if rename_project_row(project, path)
Projects::AfterRenameService.new(project).execute after_rename_service(project, path_was, namespace_path).execute
end end
rescue Exception => e # rubocop: disable Lint/RescueException rescue Exception => e # rubocop: disable Lint/RescueException
Rails.logger.error "Exception when renaming project #{id}: #{e.message}" Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
...@@ -126,4 +126,12 @@ class RenameReservedProjectNames < ActiveRecord::Migration[4.2] ...@@ -126,4 +126,12 @@ class RenameReservedProjectNames < ActiveRecord::Migration[4.2]
project.update(path: path) && project.update(path: path) &&
defined?(Projects::AfterRenameService) defined?(Projects::AfterRenameService)
end end
def after_rename_service(project, path_was, namespace_path)
AfterRenameService.new(
project,
path_before: path_was,
full_path_before: "#{namespace_path}/#{path_was}"
).execute
end
end end
...@@ -55,7 +55,7 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration[4.2] ...@@ -55,7 +55,7 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration[4.2]
# Because project path update is quite complex operation we can't safely # Because project path update is quite complex operation we can't safely
# copy-paste all code from GitLab. As exception we use Rails code here # copy-paste all code from GitLab. As exception we use Rails code here
if rename_project_row(project, path) if rename_project_row(project, path)
Projects::AfterRenameService.new(project).execute after_rename_service(project, path_was, namespace_path).execute
end end
rescue Exception => e # rubocop: disable Lint/RescueException rescue Exception => e # rubocop: disable Lint/RescueException
Rails.logger.error "Exception when renaming project #{id}: #{e.message}" Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
...@@ -68,4 +68,12 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration[4.2] ...@@ -68,4 +68,12 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration[4.2]
project.update(path: path) && project.update(path: path) &&
defined?(Projects::AfterRenameService) defined?(Projects::AfterRenameService)
end end
def after_rename_service(project, path_was, namespace_path)
AfterRenameService.new(
project,
path_before: path_was,
full_path_before: "#{namespace_path}/#{path_was}"
).execute
end
end end
...@@ -37,9 +37,8 @@ describe RenameMoreReservedProjectNames, :delete do ...@@ -37,9 +37,8 @@ describe RenameMoreReservedProjectNames, :delete do
.to receive(:execute) .to receive(:execute)
.and_raise(Projects::AfterRenameService::RenameFailedError) .and_raise(Projects::AfterRenameService::RenameFailedError)
allow(Projects::AfterRenameService) expect(migration)
.to receive(:new) .to receive(:after_rename_service)
.with(project)
.and_return(service) .and_return(service)
end end
......
...@@ -41,9 +41,8 @@ describe RenameReservedProjectNames, :migration, schema: :latest do ...@@ -41,9 +41,8 @@ describe RenameReservedProjectNames, :migration, schema: :latest do
.to receive(:execute) .to receive(:execute)
.and_raise(Projects::AfterRenameService::RenameFailedError) .and_raise(Projects::AfterRenameService::RenameFailedError)
allow(Projects::AfterRenameService) expect(migration)
.to receive(:new) .to receive(:after_rename_service)
.with(project)
.and_return(service) .and_return(service)
end end
......
...@@ -4,53 +4,45 @@ require 'spec_helper' ...@@ -4,53 +4,45 @@ require 'spec_helper'
describe Projects::AfterRenameService do describe Projects::AfterRenameService do
let(:rugged_config) { rugged_repo(project.repository).config } let(:rugged_config) { rugged_repo(project.repository).config }
let(:legacy_storage) { Storage::LegacyProject.new(project) }
let(:hashed_storage) { Storage::HashedProject.new(project) }
let!(:path_before_rename) { project.path }
let!(:full_path_before_rename) { project.full_path }
let!(:path_after_rename) { "#{project.path}-renamed" }
let!(:full_path_after_rename) { "#{project.full_path}-renamed" }
describe '#execute' do describe '#execute' do
context 'using legacy storage' do context 'using legacy storage' do
let(:project) { create(:project, :repository, :legacy_storage) } let(:project) { create(:project, :repository, :wiki_repo, :legacy_storage) }
let(:gitlab_shell) { Gitlab::Shell.new }
let(:project_storage) { project.send(:storage) } let(:project_storage) { project.send(:storage) }
let(:gitlab_shell) { Gitlab::Shell.new }
before do before do
# Project#gitlab_shell returns a new instance of Gitlab::Shell on every # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
# call. This makes testing a bit easier. # call. This makes testing a bit easier.
allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
allow(project)
.to receive(:previous_changes)
.and_return('path' => ['foo'])
allow(project)
.to receive(:path_was)
.and_return('foo')
stub_feature_flags(skip_hashed_storage_upgrade: false) stub_feature_flags(skip_hashed_storage_upgrade: false)
end end
it 'renames a repository' do it 'renames a repository' do
stub_container_registry_config(enabled: false) stub_container_registry_config(enabled: false)
expect(gitlab_shell).to receive(:mv_repository)
.ordered
.with(project.repository_storage, "#{project.namespace.full_path}/foo", "#{project.full_path}")
.and_return(true)
expect(gitlab_shell).to receive(:mv_repository)
.ordered
.with(project.repository_storage, "#{project.namespace.full_path}/foo.wiki", "#{project.full_path}.wiki")
.and_return(true)
expect_any_instance_of(SystemHooksService) expect_any_instance_of(SystemHooksService)
.to receive(:execute_hooks_for) .to receive(:execute_hooks_for)
.with(project, :rename) .with(project, :rename)
expect_any_instance_of(Gitlab::UploadsTransfer) expect_any_instance_of(Gitlab::UploadsTransfer)
.to receive(:rename_project) .to receive(:rename_project)
.with('foo', project.path, project.namespace.full_path) .with(path_before_rename, path_after_rename, project.namespace.full_path)
expect(project).to receive(:expire_caches_before_rename) expect_repository_exist("#{full_path_before_rename}.git")
expect_repository_exist("#{full_path_before_rename}.wiki.git")
service_execute
described_class.new(project).execute expect_repository_exist("#{full_path_after_rename}.git")
expect_repository_exist("#{full_path_after_rename}.wiki.git")
end end
context 'container registry with images' do context 'container registry with images' do
...@@ -63,8 +55,7 @@ describe Projects::AfterRenameService do ...@@ -63,8 +55,7 @@ describe Projects::AfterRenameService do
end end
it 'raises a RenameFailedError' do it 'raises a RenameFailedError' do
expect { described_class.new(project).execute } expect { service_execute }.to raise_error(described_class::RenameFailedError)
.to raise_error(described_class::RenameFailedError)
end end
end end
...@@ -76,7 +67,7 @@ describe Projects::AfterRenameService do ...@@ -76,7 +67,7 @@ describe Projects::AfterRenameService do
it 'moves pages folder to new location' do it 'moves pages folder to new location' do
expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project) expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
described_class.new(project).execute service_execute
end end
end end
...@@ -88,14 +79,12 @@ describe Projects::AfterRenameService do ...@@ -88,14 +79,12 @@ describe Projects::AfterRenameService do
it 'moves uploads folder to new location' do it 'moves uploads folder to new location' do
expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project) expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
described_class.new(project).execute service_execute
end end
end end
it 'updates project full path in .git/config' do it 'updates project full path in .git/config' do
allow(project_storage).to receive(:rename_repo).and_return(true) service_execute
described_class.new(project).execute
expect(rugged_config['gitlab.fullpath']).to eq(project.full_path) expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
end end
...@@ -103,13 +92,25 @@ describe Projects::AfterRenameService do ...@@ -103,13 +92,25 @@ describe Projects::AfterRenameService do
it 'updates storage location' do it 'updates storage location' do
allow(project_storage).to receive(:rename_repo).and_return(true) allow(project_storage).to receive(:rename_repo).and_return(true)
described_class.new(project).execute service_execute
expect(project.project_repository).to have_attributes( expect(project.project_repository).to have_attributes(
disk_path: project.disk_path, disk_path: project.disk_path,
shard_name: project.repository_storage shard_name: project.repository_storage
) )
end end
context 'with hashed storage upgrade when renaming enabled' do
it 'calls HashedStorageMigrationService with correct options' do
stub_application_setting(hashed_storage_enabled: true)
expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service|
expect(service).to receive(:execute).and_return(true)
end
service_execute
end
end
end end
context 'using hashed storage' do context 'using hashed storage' do
...@@ -123,25 +124,11 @@ describe Projects::AfterRenameService do ...@@ -123,25 +124,11 @@ describe Projects::AfterRenameService do
# Project#gitlab_shell returns a new instance of Gitlab::Shell on every # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
# call. This makes testing a bit easier. # call. This makes testing a bit easier.
allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
stub_feature_flags(skip_hashed_storage_upgrade: false) stub_feature_flags(skip_hashed_storage_upgrade: false)
stub_application_setting(hashed_storage_enabled: true) stub_application_setting(hashed_storage_enabled: true)
end 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
described_class.new(project).execute
end
end
it 'renames a repository' do it 'renames a repository' do
stub_container_registry_config(enabled: false) stub_container_registry_config(enabled: false)
...@@ -153,7 +140,7 @@ describe Projects::AfterRenameService do ...@@ -153,7 +140,7 @@ describe Projects::AfterRenameService do
expect(project).to receive(:expire_caches_before_rename) expect(project).to receive(:expire_caches_before_rename)
described_class.new(project).execute service_execute
end end
context 'container registry with images' do context 'container registry with images' do
...@@ -166,7 +153,7 @@ describe Projects::AfterRenameService do ...@@ -166,7 +153,7 @@ describe Projects::AfterRenameService do
end end
it 'raises a RenameFailedError' do it 'raises a RenameFailedError' do
expect { described_class.new(project).execute } expect { service_execute }
.to raise_error(described_class::RenameFailedError) .to raise_error(described_class::RenameFailedError)
end end
end end
...@@ -175,38 +162,46 @@ describe Projects::AfterRenameService do ...@@ -175,38 +162,46 @@ describe Projects::AfterRenameService do
it 'moves pages folder to new location' do it 'moves pages folder to new location' do
expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project) expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
described_class.new(project).execute service_execute
end end
end end
context 'attachments' do context 'attachments' do
let(:uploader) { create(:upload, :issuable_upload, :with_file, model: project) }
let(:file_uploader) { build(:file_uploader, project: project) }
let(:legacy_storage_path) { File.join(file_uploader.root, legacy_storage.disk_path) }
let(:hashed_storage_path) { File.join(file_uploader.root, hashed_storage.disk_path) }
it 'keeps uploads folder location unchanged' do it 'keeps uploads folder location unchanged' do
expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project) expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
described_class.new(project).execute service_execute
end end
context 'when not rolled out' do context 'when not rolled out' do
let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) } let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
it 'moves pages folder to hashed storage' do it 'moves attachments folder to hashed storage' do
expect_next_instance_of(Projects::HashedStorage::MigrateAttachmentsService) do |service| expect(File.directory?(legacy_storage_path)).to be_truthy
expect(service).to receive(:execute) expect(File.directory?(hashed_storage_path)).to be_falsey
end
service_execute
expect(project.reload.hashed_storage?(:attachments)).to be_truthy
described_class.new(project).execute expect(File.directory?(legacy_storage_path)).to be_falsey
expect(File.directory?(hashed_storage_path)).to be_truthy
end end
end end
end end
it 'updates project full path in .git/config' do it 'updates project full path in .git/config' do
described_class.new(project).execute service_execute
expect(rugged_config['gitlab.fullpath']).to eq(project.full_path) expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
end end
it 'updates storage location' do it 'updates storage location' do
described_class.new(project).execute service_execute
expect(project.project_repository).to have_attributes( expect(project.project_repository).to have_attributes(
disk_path: project.disk_path, disk_path: project.disk_path,
...@@ -215,4 +210,21 @@ describe Projects::AfterRenameService do ...@@ -215,4 +210,21 @@ describe Projects::AfterRenameService do
end end
end end
end end
def service_execute
# AfterRenameService is called by UpdateService after a successful model.update
# the initialization will include before and after paths values
project.update(path: path_after_rename)
described_class.new(project, path_before: path_before_rename, full_path_before: full_path_before_rename).execute
end
def expect_repository_exist(full_path_with_extension)
expect(
gitlab_shell.exists?(
project.repository_storage,
full_path_with_extension
)
).to be_truthy
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment