From 8ccce94ee8929ccc28ea6d15568e1bc979b18291 Mon Sep 17 00:00:00 2001 From: Simon Knox Date: Thu, 23 Nov 2017 16:04:22 +1100 Subject: fix some icon colors. move text-color styles to common.scss --- app/assets/stylesheets/framework/buttons.scss | 11 ------- app/assets/stylesheets/framework/common.scss | 27 +++++++++++++++-- app/assets/stylesheets/framework/icons.scss | 35 +++++++++++++++++----- app/assets/stylesheets/framework/tw_bootstrap.scss | 27 ----------------- app/assets/stylesheets/pages/projects.scss | 4 --- app/views/shared/projects/_project.html.haml | 2 +- 6 files changed, 54 insertions(+), 52 deletions(-) diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index b2f26cf7159..bc4c4f8b7e3 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -88,17 +88,6 @@ border-color: $border-dark; color: $color; } - - svg { - - path { - fill: $color; - } - - use { - stroke: $color; - } - } } @mixin btn-green { diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 5e4ddf366ef..6746cd35950 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -2,14 +2,37 @@ .cgray { color: $common-gray; } .clgray { color: $common-gray-light; } .cred { color: $common-red; } -svg.cred { fill: $common-red; } .cgreen { color: $common-green; } -svg.cgreen { fill: $common-green; } .cdark { color: $common-gray-dark; } + +.text-plain, +.text-plain:hover { + color: $gl-text-color; +} .text-secondary { color: $gl-text-color-secondary; } +.text-success, +.text-success:hover { + color: $brand-success; +} + +.text-danger, +.text-danger:hover { + color: $brand-danger; +} + +.text-warning, +.text-warning:hover { + color: $brand-warning; +} + +.text-info, +.text-info:hover { + color: $brand-info; +} + .underlined-link { text-decoration: underline; } .hint { font-style: italic; color: $hint-color; } .light { color: $common-gray; } diff --git a/app/assets/stylesheets/framework/icons.scss b/app/assets/stylesheets/framework/icons.scss index 1ab5e6a93f9..2fc8a3508d5 100644 --- a/app/assets/stylesheets/framework/icons.scss +++ b/app/assets/stylesheets/framework/icons.scss @@ -1,35 +1,56 @@ .ci-status-icon-success, .ci-status-icon-passed { - color: $green-500; + &, + &:hover { + color: $green-500; + } } .ci-status-icon-failed { - color: $gl-danger; + &, + &:hover { + color: $gl-danger; + } } .ci-status-icon-pending, .ci-status-icon-failed_with_warnings, .ci-status-icon-success_with_warnings { - color: $orange-500; + &, + &:hover { + color: $orange-500; + } } .ci-status-icon-running { - color: $blue-400; + &, + &:hover { + color: $blue-400; + } } .ci-status-icon-canceled, .ci-status-icon-disabled, .ci-status-icon-not-found { - color: $gl-text-color; + &, + &:hover { + color: $gl-text-color; + } } .ci-status-icon-created, .ci-status-icon-skipped { - color: $gray-darkest; + &, + &:hover { + color: $gray-darkest; + } } .ci-status-icon-manual { - color: $gl-text-color; + &, + &:hover { + color: $gl-text-color; + } } .icon-link { diff --git a/app/assets/stylesheets/framework/tw_bootstrap.scss b/app/assets/stylesheets/framework/tw_bootstrap.scss index d5c6ddbb4a5..1c6e2bf3074 100644 --- a/app/assets/stylesheets/framework/tw_bootstrap.scss +++ b/app/assets/stylesheets/framework/tw_bootstrap.scss @@ -195,33 +195,6 @@ summary { } } -// Typography ================================================================= - -.text-primary, -.text-primary:hover { - color: $brand-primary; -} - -.text-success, -.text-success:hover { - color: $brand-success; -} - -.text-danger, -.text-danger:hover { - color: $brand-danger; -} - -.text-warning, -.text-warning:hover { - color: $brand-warning; -} - -.text-info, -.text-info:hover { - color: $brand-info; -} - // Prevent datetimes on tooltips to break into two lines .local-timeago { white-space: nowrap; diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index aaad6dbba8e..f639f391b89 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -886,10 +886,6 @@ pre.light-well { font-size: $gl-font-size; } - a { - color: $gl-text-color; - } - .avatar-container, .controls { flex: 0 0 auto; diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 52a8fe8bb67..98bfc7c4d36 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -20,7 +20,7 @@ = project_icon(project, alt: '', class: 'avatar project-avatar s40') .project-details %h3.prepend-top-0.append-bottom-0 - = link_to project_path(project), class: dom_class(project) do + = link_to project_path(project), class: 'text-plain' do %span.project-full-name %span.namespace-name - if project.namespace && !skip_namespace -- cgit v1.2.1 From f19a5e6dc27935c4fed5e736152105a363f5a8f8 Mon Sep 17 00:00:00 2001 From: Simon Knox Date: Thu, 23 Nov 2017 16:07:13 +1100 Subject: readd missing text-primary. remove unused svg styles --- app/assets/stylesheets/framework/common.scss | 5 +++++ app/assets/stylesheets/pages/notes.scss | 6 ------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 6746cd35950..6baff110187 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -13,6 +13,11 @@ color: $gl-text-color-secondary; } +.text-primary, +.text-primary:hover { + color: $brand-primary; +} + .text-success, .text-success:hover { color: $brand-success; diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 2461b818219..9d5930e3966 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -780,12 +780,6 @@ ul.notes { } } - svg { - fill: currentColor; - height: 16px; - width: 16px; - } - .loading { margin: 0; height: auto; -- cgit v1.2.1 From 1d6f37861ed04201f90b0248fc23e51b529addac Mon Sep 17 00:00:00 2001 From: Simon Knox Date: Thu, 23 Nov 2017 21:13:57 +1100 Subject: override a:focus styles. fix Commit page pipeline icon --- app/assets/stylesheets/framework/common.scss | 1 + app/assets/stylesheets/framework/icons.scss | 21 ++++++++++++++------- app/views/projects/commit/_commit_box.html.haml | 4 ++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 6baff110187..cb1aad90a9c 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -9,6 +9,7 @@ .text-plain:hover { color: $gl-text-color; } + .text-secondary { color: $gl-text-color-secondary; } diff --git a/app/assets/stylesheets/framework/icons.scss b/app/assets/stylesheets/framework/icons.scss index 2fc8a3508d5..9e45ed52163 100644 --- a/app/assets/stylesheets/framework/icons.scss +++ b/app/assets/stylesheets/framework/icons.scss @@ -1,14 +1,16 @@ .ci-status-icon-success, .ci-status-icon-passed { &, - &:hover { + &:hover, + &:focus { color: $green-500; } } .ci-status-icon-failed { &, - &:hover { + &:hover, + &:focus { color: $gl-danger; } } @@ -17,14 +19,16 @@ .ci-status-icon-failed_with_warnings, .ci-status-icon-success_with_warnings { &, - &:hover { + &:hover, + &:focus { color: $orange-500; } } .ci-status-icon-running { &, - &:hover { + &:hover, + &:focus { color: $blue-400; } } @@ -33,7 +37,8 @@ .ci-status-icon-disabled, .ci-status-icon-not-found { &, - &:hover { + &:hover, + &:focus { color: $gl-text-color; } } @@ -41,14 +46,16 @@ .ci-status-icon-created, .ci-status-icon-skipped { &, - &:hover { + &:hover, + &:focus { color: $gray-darkest; } } .ci-status-icon-manual { &, - &:hover { + &:hover, + &:focus { color: $gl-text-color; } } diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 8b9c1bbb602..5f607c2ab25 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -67,8 +67,8 @@ - if @commit.last_pipeline - last_pipeline = @commit.last_pipeline .well-segment.pipeline-info - .status-icon-container{ class: "ci-status-icon-#{last_pipeline.status}" } - = link_to project_pipeline_path(@project, last_pipeline.id) do + .status-icon-container + = link_to project_pipeline_path(@project, last_pipeline.id), class: "ci-status-icon-#{last_pipeline.status}" do = ci_icon_for_status(last_pipeline.status) #{ _('Pipeline') } = link_to "##{last_pipeline.id}", project_pipeline_path(@project, last_pipeline.id) -- cgit v1.2.1 From 0a4d55a1c9f58f383f23b8f60bbe1acba1b8511c Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Mon, 13 Nov 2017 15:15:42 +0100 Subject: Refactor Hashed Storage migration to add additional migration steps --- app/models/storage/hashed_project.rb | 1 - .../hashed_storage/migrate_repository_service.rb | 66 +++++++++++++++++++ .../projects/hashed_storage_migration_service.rb | 63 ++---------------- .../migrate_repository_service_spec.rb | 74 ++++++++++++++++++++++ .../hashed_storage_migration_service_spec.rb | 74 ---------------------- 5 files changed, 146 insertions(+), 132 deletions(-) create mode 100644 app/services/projects/hashed_storage/migrate_repository_service.rb create mode 100644 spec/services/projects/hashed_storage/migrate_repository_service_spec.rb delete mode 100644 spec/services/projects/hashed_storage_migration_service_spec.rb diff --git a/app/models/storage/hashed_project.rb b/app/models/storage/hashed_project.rb index f025f40994e..fae1b64961a 100644 --- a/app/models/storage/hashed_project.rb +++ b/app/models/storage/hashed_project.rb @@ -4,7 +4,6 @@ module Storage delegate :gitlab_shell, :repository_storage_path, to: :project ROOT_PATH_PREFIX = '@hashed'.freeze - STORAGE_VERSION = 1 def initialize(project) @project = project diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb new file mode 100644 index 00000000000..c03db5cc1b9 --- /dev/null +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -0,0 +1,66 @@ +module Projects + module HashedStorage + class MigrateRepositoryService < BaseService + include Gitlab::ShellAdapter + + attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :old_storage_version, :logger + + def initialize(project, logger = nil) + @project = project + @logger = logger || Rails.logger + 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) + + if has_wiki + @old_wiki_disk_path = "#{@old_disk_path}.wiki" + result &&= move_repository("#{@old_wiki_disk_path}", "#{@new_disk_path}.wiki") + end + + unless result + rollback_folder_move + return result + end + + project.repository_read_only = false + project.save! + + result + end + + private + + def move_repository(from_name, to_name) + from_exists = gitlab_shell.exists?(project.repository_storage_path, "#{from_name}.git") + to_exists = gitlab_shell.exists?(project.repository_storage_path, "#{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}) ..." + return false + elsif !from_exists + # Repository have been moved already. + return true + end + + gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) + end + + def rollback_folder_move + move_repository(@new_disk_path, @old_disk_path) + move_repository("#{@new_disk_path}.wiki", "#{@old_disk_path}.wiki") + end + end + end +end diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb index f5945f3b87f..b61f71cf9a0 100644 --- a/app/services/projects/hashed_storage_migration_service.rb +++ b/app/services/projects/hashed_storage_migration_service.rb @@ -1,68 +1,17 @@ module Projects class HashedStorageMigrationService < BaseService - include Gitlab::ShellAdapter - - attr_reader :old_disk_path, :new_disk_path - + attr_reader :logger + def initialize(project, logger = nil) @project = project - @logger ||= Rails.logger + @logger = logger || Rails.logger end def execute - return if project.hashed_storage?(:repository) - - @old_disk_path = project.disk_path - has_wiki = project.wiki.repository_exists? - - project.storage_version = Storage::HashedProject::STORAGE_VERSION - project.ensure_storage_path_exists - - @new_disk_path = project.disk_path - - result = move_repository(@old_disk_path, @new_disk_path) - - if has_wiki - result &&= move_repository("#{@old_disk_path}.wiki", "#{@new_disk_path}.wiki") - end - - unless result - rollback_folder_move - return + # Migrate repository from Legacy to Hashed Storage + unless project.hashed_storage?(:repository) + return unless HashedStorage::MigrateRepositoryService.new(project, logger).execute end - - project.repository_read_only = false - project.save! - - block_given? ? yield : result - end - - private - - def move_repository(from_name, to_name) - from_exists = gitlab_shell.exists?(project.repository_storage_path, "#{from_name}.git") - to_exists = gitlab_shell.exists?(project.repository_storage_path, "#{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}) ..." - return false - elsif !from_exists - # Repository have been moved already. - return true - end - - gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) - end - - def rollback_folder_move - move_repository(@new_disk_path, @old_disk_path) - move_repository("#{@new_disk_path}.wiki", "#{@old_disk_path}.wiki") - end - - def logger - @logger end end end diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb new file mode 100644 index 00000000000..ef91d21cf20 --- /dev/null +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' + +describe Projects::HashedStorage::MigrateRepositoryService do + let(:gitlab_shell) { Gitlab::Shell.new } + let(:project) { create(:project, :empty_repo, :wiki_repo) } + let(:service) { described_class.new(project) } + let(:legacy_storage) { Storage::LegacyProject.new(project) } + let(:hashed_storage) { Storage::HashedProject.new(project) } + + describe '#execute' do + before do + allow(service).to receive(:gitlab_shell) { gitlab_shell } + end + + context 'when succeeds' do + it 'renames project and wiki repositories' do + service.execute + + expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy + end + + it 'updates project to be hashed and not read-only' do + service.execute + + expect(project.hashed_storage?(:repository)).to be_truthy + expect(project.repository_read_only).to be_falsey + end + + it 'move operation is called for both repositories' do + expect_move_repository(project.disk_path, hashed_storage.disk_path) + expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") + + service.execute + end + end + + context 'when one move fails' do + it 'rollsback repositories to original name' do + from_name = project.disk_path + to_name = hashed_storage.disk_path + 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(service).to receive(:rollback_folder_move).and_call_original + + service.execute + + expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey + end + + context 'when rollback fails' do + before do + from_name = legacy_storage.disk_path + to_name = hashed_storage.disk_path + + hashed_storage.ensure_storage_path_exists + gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) + end + + it 'does not try to move nil repository over hashed' do + expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") + + service.execute + end + end + end + + def expect_move_repository(from_name, to_name) + expect(gitlab_shell).to receive(:mv_repository).with(project.repository_storage_path, from_name, to_name).and_call_original + end + end +end diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb deleted file mode 100644 index b71b47c59b6..00000000000 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ /dev/null @@ -1,74 +0,0 @@ -require 'spec_helper' - -describe Projects::HashedStorageMigrationService do - let(:gitlab_shell) { Gitlab::Shell.new } - let(:project) { create(:project, :empty_repo, :wiki_repo) } - let(:service) { described_class.new(project) } - let(:legacy_storage) { Storage::LegacyProject.new(project) } - let(:hashed_storage) { Storage::HashedProject.new(project) } - - describe '#execute' do - before do - allow(service).to receive(:gitlab_shell) { gitlab_shell } - end - - context 'when succeeds' do - it 'renames project and wiki repositories' do - service.execute - - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_truthy - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy - end - - it 'updates project to be hashed and not read-only' do - service.execute - - expect(project.hashed_storage?(:repository)).to be_truthy - expect(project.repository_read_only).to be_falsey - end - - it 'move operation is called for both repositories' do - expect_move_repository(project.disk_path, hashed_storage.disk_path) - expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") - - service.execute - end - end - - context 'when one move fails' do - it 'rollsback repositories to original name' do - from_name = project.disk_path - to_name = hashed_storage.disk_path - 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(service).to receive(:rollback_folder_move).and_call_original - - service.execute - - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_falsey - expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey - end - - context 'when rollback fails' do - before do - from_name = legacy_storage.disk_path - to_name = hashed_storage.disk_path - - hashed_storage.ensure_storage_path_exists - gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) - end - - it 'does not try to move nil repository over hashed' do - expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") - - service.execute - end - end - end - - def expect_move_repository(from_name, to_name) - expect(gitlab_shell).to receive(:mv_repository).with(project.repository_storage_path, from_name, to_name).and_call_original - end - end -end -- cgit v1.2.1 From 4af26c1c65e4cc1d18a37acbc2af6ae29e91b336 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 15 Nov 2017 14:42:38 +0100 Subject: WIP Attachments migration --- .../hashed_storage/migrate_attachments_service.rb | 54 ++++++++++++++++ .../projects/hashed_storage_migration_service.rb | 7 ++- .../migrate_attachments_service_spec.rb | 72 ++++++++++++++++++++++ .../hashed_storage_migration_service_spec.rb | 40 ++++++++++++ 4 files changed, 172 insertions(+), 1 deletion(-) create mode 100644 app/services/projects/hashed_storage/migrate_attachments_service.rb create mode 100644 spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb create mode 100644 spec/services/projects/hashed_storage_migration_service_spec.rb diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb new file mode 100644 index 00000000000..58a47da2fcb --- /dev/null +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -0,0 +1,54 @@ +module Projects + module HashedStorage + class MigrateAttachmentsService < BaseService + attr_reader :logger + + BATCH_SIZE = 500 + + def initialize(project, logger = nil) + @project = project + @logger = logger || Rails.logger + end + + def execute + project_before_migration = project.dup + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] + + project.uploads.find_each(batch_size: BATCH_SIZE) do |upload| + old_path = attachments_path(project_before_migration, upload) + new_path = attachments_path(project, upload) + move_attachment(old_path, new_path) + end + + project.save! + end + + private + + def attachments_path(project, upload) + File.join( + FileUploader.dynamic_path_segment(project), + upload.path + ) + end + + def move_attachment(old_path, new_path) + unless File.file?(old_path) + logger.error("Failed to migrate attachment from '#{old_path}' to '#{new_path}', source file doesn't exist (PROJECT_ID=#{project.id})") + return + end + + # Create attachments folder if doesn't exist yet + FileUtils.mkdir_p(File.dirname(new_path)) unless Dir.exist?(File.dirname(new_path)) + + if File.file?(new_path) + logger.info("Skipped attachment migration from '#{old_path}' to '#{new_path}', target file already exist (PROJECT_ID=#{project.id})") + return + end + + FileUtils.mv(old_path, new_path) + logger.info("Migrated project attachment from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + end + end + end +end diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb index b61f71cf9a0..662702c1db5 100644 --- a/app/services/projects/hashed_storage_migration_service.rb +++ b/app/services/projects/hashed_storage_migration_service.rb @@ -1,7 +1,7 @@ module Projects class HashedStorageMigrationService < BaseService attr_reader :logger - + def initialize(project, logger = nil) @project = project @logger = logger || Rails.logger @@ -12,6 +12,11 @@ module Projects unless project.hashed_storage?(:repository) return unless HashedStorage::MigrateRepositoryService.new(project, logger).execute end + + # Migrate attachments from Legacy to Hashed Storage + unless project.hashed_storage?(:attachments) + HashedStorage::MigrateAttachmentsService.new(project, logger).execute + end end end end diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb new file mode 100644 index 00000000000..81f05074261 --- /dev/null +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' + +describe Projects::HashedStorage::MigrateAttachmentsService do + subject(:service) { described_class.new(project) } + let(:project) { create(:project) } + let(:legacy_storage) { Storage::LegacyProject.new(project) } + let(:hashed_storage) { Storage::HashedProject.new(project) } + + let!(:upload) { Upload.find_by(path: file_uploader.relative_path) } + let(:file_uploader) { build(:file_uploader, project: project) } + let(:old_path) { attachments_path(legacy_storage, upload) } + let(:new_path) { attachments_path(hashed_storage, upload) } + + let(:other_file_uploader) { build(:file_uploader, project: project) } + let(:other_old_path) { attachments_path(legacy_storage, other_upload) } + let(:other_new_path) { attachments_path(hashed_storage, other_upload) } + + context '#execute' do + context 'when succeeds' do + it 'moves attachments to hashed storage layout' do + expect(File.file?(old_path)).to be_truthy + expect(File.file?(new_path)).to be_falsey + + service.execute + + expect(File.file?(old_path)).to be_falsey + expect(File.file?(new_path)).to be_truthy + end + end + + context 'when original file does not exist anymore' do + let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } + + before do + File.unlink(old_path) + end + + it 'skips moving the file and goes to next' do + expect(FileUtils).not_to receive(:mv).with(old_path, new_path) + expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original + + service.execute + + expect(File.file?(new_path)).to be_falsey + expect(File.file?(other_new_path)).to be_truthy + end + end + + context 'when target file already exists' do + let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } + + before do + FileUtils.mkdir_p(File.dirname(new_path)) + FileUtils.touch(new_path) + end + + it 'skips moving the file and goes to next' do + expect(FileUtils).not_to receive(:mv).with(old_path, new_path) + expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original + expect(File.file?(new_path)).to be_truthy + + service.execute + + expect(File.file?(old_path)).to be_truthy + end + end + end + + def attachments_path(storage, upload) + File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path, upload.path) + end +end diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb new file mode 100644 index 00000000000..28b6daf217e --- /dev/null +++ b/spec/services/projects/hashed_storage_migration_service_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Projects::HashedStorageMigrationService do + let(:project) { create(:project, :empty_repo, :wiki_repo) } + subject(:service) { described_class.new(project) } + + describe '#execute' do + context 'repository migration' do + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do + expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_call_original + expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).to receive(:execute) + + service.execute + end + + it 'does not delegate migration if repository is already migrated' do + project.storage_version = ::Project::LATEST_STORAGE_VERSION + expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:execute) + + service.execute + end + end + + context 'attachments migration' do + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do + expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_call_original + expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).to receive(:execute) + + service.execute + end + + it 'does not delegate migration if attachments are already migrated' do + project.storage_version = ::Project::LATEST_STORAGE_VERSION + expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:execute) + + service.execute + end + end + end +end -- cgit v1.2.1 From d0a08ab888db33437c7df4eb37b5805757a6dce4 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 18 Nov 2017 07:26:07 +0100 Subject: Improve storage migration rake task --- app/models/project.rb | 5 ++- lib/tasks/gitlab/storage.rake | 85 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 894ded2a9f6..f20c0688bc2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -272,8 +272,9 @@ class Project < ActiveRecord::Base scope :pending_delete, -> { where(pending_delete: true) } scope :without_deleted, -> { where(pending_delete: false) } - scope :with_hashed_storage, -> { where('storage_version >= 1') } - scope :with_legacy_storage, -> { where(storage_version: [nil, 0]) } + scope :with_storage_feature, ->(feature) { where('storage_version >= :version', version: HASHED_STORAGE_FEATURES[feature]) } + scope :without_storage_feature, ->(feature) { where('storage_version < :version OR storage_version IS NULL', version: HASHED_STORAGE_FEATURES[feature]) } + scope :with_unmigrated_storage, -> { where('storage_version < :version OR storage_version IS NULL', version: LATEST_STORAGE_VERSION) } scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) } scope :sorted_by_stars, -> { reorder('projects.star_count DESC') } diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index e05be4a3405..8ac73bc8ff2 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/lib/tasks/gitlab/storage.rake @@ -2,10 +2,10 @@ namespace :gitlab do namespace :storage do desc 'GitLab | Storage | Migrate existing projects to Hashed Storage' task migrate_to_hashed: :environment do - legacy_projects_count = Project.with_legacy_storage.count + legacy_projects_count = Project.with_unmigrated_storage.count if legacy_projects_count == 0 - puts 'There are no projects using legacy storage. Nothing to do!' + puts 'There are no projects requiring storage migration. Nothing to do!' next end @@ -23,22 +23,42 @@ namespace :gitlab do desc 'Gitlab | Storage | Summary of existing projects using Legacy Storage' task legacy_projects: :environment do - projects_summary(Project.with_legacy_storage) + relation_summary('projects', Project.without_storage_feature(:repository)) end desc 'Gitlab | Storage | List existing projects using Legacy Storage' task list_legacy_projects: :environment do - projects_list(Project.with_legacy_storage) + projects_list('projects using Legacy Storage', Project.without_storage_feature(:repository)) end desc 'Gitlab | Storage | Summary of existing projects using Hashed Storage' task hashed_projects: :environment do - projects_summary(Project.with_hashed_storage) + relation_summary('projects using Hashed Storage', Project.with_storage_feature(:repository)) end desc 'Gitlab | Storage | List existing projects using Hashed Storage' task list_hashed_projects: :environment do - projects_list(Project.with_hashed_storage) + projects_list('projects using Hashed Storage', Project.with_storage_feature(:repository)) + end + + desc 'Gitlab | Storage | Summary of project attachments using Legacy Storage' + task legacy_attachments: :environment do + relation_summary('attachments using Legacy Storage', legacy_attachments_relation) + end + + desc 'Gitlab | Storage | List existing project attachments using Legacy Storage' + task list_legacy_attachments: :environment do + attachments_list('attachments using Legacy Storage', legacy_attachments_relation) + end + + desc 'Gitlab | Storage | Summary of project attachments using Hashed Storage' + task hashed_attachments: :environment do + relation_summary('attachments using Hashed Storage', hashed_attachments_relation) + end + + desc 'Gitlab | Storage | List existing project attachments using Hashed Storage' + task list_hashed_attachments: :environment do + attachments_list('attachments using Hashed Storage', hashed_attachments_relation) end def batch_size @@ -46,29 +66,43 @@ namespace :gitlab do end def project_id_batches(&block) - Project.with_legacy_storage.in_batches(of: batch_size, start: ENV['ID_FROM'], finish: ENV['ID_TO']) do |relation| # rubocop: disable Cop/InBatches + Project.with_unmigrated_storage.in_batches(of: batch_size, start: ENV['ID_FROM'], finish: ENV['ID_TO']) do |relation| # rubocop: disable Cop/InBatches ids = relation.pluck(:id) yield ids.min, ids.max end end - def projects_summary(relation) - projects_count = relation.count - puts "* Found #{projects_count} projects".color(:green) + def legacy_attachments_relation + Upload.joins(<<~SQL).where('projects.storage_version < :version OR projects.storage_version IS NULL', version: Project::HASHED_STORAGE_FEATURES[:attachments]) + JOIN projects + ON (uploads.model_type='Project' AND uploads.model_id=projects.id) + SQL + end + + def hashed_attachments_relation + Upload.joins(<<~SQL).where('projects.storage_version >= :version', version: Project::HASHED_STORAGE_FEATURES[:attachments]) + JOIN projects + ON (uploads.model_type='Project' AND uploads.model_id=projects.id) + SQL + end + + def relation_summary(relation_name, relation) + relation_count = relation.count + puts "* Found #{relation_count} #{relation_name}".color(:green) - projects_count + relation_count end - def projects_list(relation) - projects_count = projects_summary(relation) + def projects_list(relation_name, relation) + relation_count = relation_summary(relation_name, relation) projects = relation.with_route limit = ENV.fetch('LIMIT', 500).to_i - return unless projects_count > 0 + return unless relation_count > 0 - puts " ! Displaying first #{limit} projects..." if projects_count > limit + puts " ! Displaying first #{limit} #{relation_name}..." if relation_count > limit counter = 0 projects.find_in_batches(batch_size: batch_size) do |batch| @@ -81,5 +115,26 @@ namespace :gitlab do end end end + + def attachments_list(relation_name, relation) + relation_count = relation_summary(relation_name, relation) + + limit = ENV.fetch('LIMIT', 500).to_i + + return unless relation_count > 0 + + puts " ! Displaying first #{limit} #{relation_name}..." if relation_count > limit + + counter = 0 + relation.find_in_batches(batch_size: batch_size) do |batch| + batch.each do |upload| + counter += 1 + + puts " - #{upload.path} (id: #{upload.id})".color(:red) + + return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator + end + end + end end end -- cgit v1.2.1 From f0f6a237d7e95fcc5d52e85aef151f0327bf2fdc Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Mon, 20 Nov 2017 23:35:17 +0100 Subject: attachments migration should move only the base folder --- .../hashed_storage/migrate_attachments_service.rb | 37 +++++++------------ .../migrate_attachments_service_spec.rb | 42 ++++++++++------------ 2 files changed, 30 insertions(+), 49 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 58a47da2fcb..68b9a72661c 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -3,51 +3,38 @@ module Projects class MigrateAttachmentsService < BaseService attr_reader :logger - BATCH_SIZE = 500 - def initialize(project, logger = nil) @project = project @logger = logger || Rails.logger end def execute - project_before_migration = project.dup + old_path = FileUploader.dynamic_path_segment(project) project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] + new_path = FileUploader.dynamic_path_segment(project) - project.uploads.find_each(batch_size: BATCH_SIZE) do |upload| - old_path = attachments_path(project_before_migration, upload) - new_path = attachments_path(project, upload) - move_attachment(old_path, new_path) - end - + move_folder!(old_path, new_path) project.save! end private - def attachments_path(project, upload) - File.join( - FileUploader.dynamic_path_segment(project), - upload.path - ) - end - - def move_attachment(old_path, new_path) - unless File.file?(old_path) - logger.error("Failed to migrate attachment from '#{old_path}' to '#{new_path}', source file doesn't exist (PROJECT_ID=#{project.id})") + def move_folder!(old_path, new_path) + unless File.exist?(old_path) + logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist (PROJECT_ID=#{project.id})") return end - # Create attachments folder if doesn't exist yet - FileUtils.mkdir_p(File.dirname(new_path)) unless Dir.exist?(File.dirname(new_path)) - - if File.file?(new_path) - logger.info("Skipped attachment migration from '#{old_path}' to '#{new_path}', target file already exist (PROJECT_ID=#{project.id})") + if File.exist?(new_path) + logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") return end + # Create hashed storage base path folder + FileUtils.mkdir_p(File.expand_path('..', new_path)) + FileUtils.mv(old_path, new_path) - logger.info("Migrated project attachment from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") end end end diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb index 81f05074261..ce43a7e4d54 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -8,65 +8,59 @@ describe Projects::HashedStorage::MigrateAttachmentsService do let!(:upload) { Upload.find_by(path: file_uploader.relative_path) } let(:file_uploader) { build(:file_uploader, project: project) } - let(:old_path) { attachments_path(legacy_storage, upload) } - let(:new_path) { attachments_path(hashed_storage, upload) } - - let(:other_file_uploader) { build(:file_uploader, project: project) } - let(:other_old_path) { attachments_path(legacy_storage, other_upload) } - let(:other_new_path) { attachments_path(hashed_storage, other_upload) } + let(:old_path) { File.join(base_path(legacy_storage), upload.path) } + let(:new_path) { File.join(base_path(hashed_storage), upload.path) } context '#execute' do context 'when succeeds' do it 'moves attachments to hashed storage layout' do expect(File.file?(old_path)).to be_truthy expect(File.file?(new_path)).to be_falsey + expect(File.exist?(base_path(legacy_storage))).to be_truthy + expect(File.exist?(base_path(hashed_storage))).to be_falsey + expect(FileUtils).to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)).and_call_original service.execute + expect(File.exist?(base_path(hashed_storage))).to be_truthy + expect(File.exist?(base_path(legacy_storage))).to be_falsey expect(File.file?(old_path)).to be_falsey expect(File.file?(new_path)).to be_truthy end end - context 'when original file does not exist anymore' do - let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } - + context 'when original folder does not exist anymore' do before do - File.unlink(old_path) + FileUtils.rm_rf(base_path(legacy_storage)) end - it 'skips moving the file and goes to next' do - expect(FileUtils).not_to receive(:mv).with(old_path, new_path) - expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original + it 'skips moving folders and go to next' do + expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) service.execute + expect(File.exist?(base_path(hashed_storage))).to be_falsey expect(File.file?(new_path)).to be_falsey - expect(File.file?(other_new_path)).to be_truthy end end - context 'when target file already exists' do - let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } - + context 'when target folder already exists' do before do - FileUtils.mkdir_p(File.dirname(new_path)) - FileUtils.touch(new_path) + FileUtils.mkdir_p(base_path(hashed_storage)) end it 'skips moving the file and goes to next' do - expect(FileUtils).not_to receive(:mv).with(old_path, new_path) - expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original - expect(File.file?(new_path)).to be_truthy + expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) service.execute + expect(File.exist?(base_path(legacy_storage))).to be_truthy expect(File.file?(old_path)).to be_truthy end end end - def attachments_path(storage, upload) - File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path, upload.path) + def base_path(storage) + File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path) end end -- cgit v1.2.1 From 9fd31eb469de5099f7a7b44840d7930bc3c42bbe Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 21 Nov 2017 00:23:43 +0100 Subject: Raises error when migration cannot happen so job is cancelled --- .../projects/hashed_storage/migrate_attachments_service.rb | 4 +++- .../projects/hashed_storage/migrate_attachments_service_spec.rb | 7 ++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 68b9a72661c..26026899ebe 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -1,5 +1,7 @@ module Projects module HashedStorage + AttachmentMigrationError = Class.new(StandardError) + class MigrateAttachmentsService < BaseService attr_reader :logger @@ -27,7 +29,7 @@ module Projects if File.exist?(new_path) logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") - return + raise AttachmentMigrationError, "Target path '#{new_path}' already exist" end # Create hashed storage base path folder diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb index ce43a7e4d54..de2abfc1985 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -49,13 +49,10 @@ describe Projects::HashedStorage::MigrateAttachmentsService do FileUtils.mkdir_p(base_path(hashed_storage)) end - it 'skips moving the file and goes to next' do + it 'raises AttachmentMigrationError' do expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) - service.execute - - expect(File.exist?(base_path(legacy_storage))).to be_truthy - expect(File.file?(old_path)).to be_truthy + expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentMigrationError) end end end -- cgit v1.2.1 From 4b87c1afaa652d72fa6aeeb4fe52fa3883e2f4c8 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 21 Nov 2017 16:38:32 +0100 Subject: when rollingback repository migration, toggle readonly mode back --- .../projects/hashed_storage/migrate_attachments_service.rb | 4 ++-- .../projects/hashed_storage/migrate_repository_service.rb | 2 +- .../projects/hashed_storage/migrate_repository_service_spec.rb | 8 +++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 26026899ebe..b58b6f57ed7 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -22,8 +22,8 @@ module Projects private def move_folder!(old_path, new_path) - unless File.exist?(old_path) - logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist (PROJECT_ID=#{project.id})") + 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})") return end diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index c03db5cc1b9..da43877a185 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -29,7 +29,7 @@ module Projects unless result rollback_folder_move - return result + project.storage_version = nil end project.repository_read_only = false diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index ef91d21cf20..3a3e47fd9c0 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -48,18 +48,20 @@ describe Projects::HashedStorage::MigrateRepositoryService do expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_falsey expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey + expect(project.repository_read_only?).to be_falsey end context 'when rollback fails' do - before do - from_name = legacy_storage.disk_path - to_name = hashed_storage.disk_path + let(:from_name) { legacy_storage.disk_path } + let(:to_name) { hashed_storage.disk_path } + before do hashed_storage.ensure_storage_path_exists gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) end it 'does not try to move nil repository over hashed' do + expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage_path, from_name, to_name) expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") service.execute -- cgit v1.2.1 From 65bd6868d014e23c21e4d5ecff468124b2c72f4c Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 22 Nov 2017 06:35:53 +0100 Subject: Codestyle changes and Added Exclusive Lease to hashed storage migration --- .../hashed_storage/migrate_attachments_service.rb | 2 +- .../project_migrate_hashed_storage_worker.rb | 26 ++++++++++++- .../project_migrate_hashed_storage_worker_spec.rb | 44 +++++++++++++++------- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index b58b6f57ed7..93f44110605 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -33,7 +33,7 @@ module Projects end # Create hashed storage base path folder - FileUtils.mkdir_p(File.expand_path('..', new_path)) + FileUtils.mkdir_p(File.dirname(new_path)) FileUtils.mv(old_path, new_path) logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb index ca276d7801c..e3ecd6bc950 100644 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ b/app/workers/project_migrate_hashed_storage_worker.rb @@ -2,10 +2,34 @@ class ProjectMigrateHashedStorageWorker include Sidekiq::Worker include DedicatedSidekiqQueue + LEASE_TIMEOUT = 30.seconds.to_i + def perform(project_id) project = Project.find_by(id: project_id) return if project.nil? || project.pending_delete? - ::Projects::HashedStorageMigrationService.new(project, logger).execute + uuid = try_obtain_lease_for(project_id) + if uuid + ::Projects::HashedStorageMigrationService.new(project, logger).execute + else + false + end + rescue => ex + cancel_lease_for(project_id, uuid) + raise ex + end + + private + + def try_obtain_lease_for(project_id) + Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT).try_obtain + end + + def lease_key(project_id) + "project_migrate_hashed_storage_worker:#{project_id}" + end + + def cancel_lease_for(project_id, uuid) + Gitlab::ExclusiveLease.cancel(lease_key(project_id), uuid) end end diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb index f5226dee0ad..8cacdfa7173 100644 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ b/spec/workers/project_migrate_hashed_storage_worker_spec.rb @@ -5,25 +5,43 @@ describe ProjectMigrateHashedStorageWorker do let(:project) { create(:project, :empty_repo) } let(:pending_delete_project) { create(:project, :empty_repo, pending_delete: true) } - it 'skips when project no longer exists' do - nonexistent_id = 999999999999 + context 'when have exclusive lease' do + before do + allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true) + end - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) - subject.perform(nonexistent_id) - end + it 'skips when project no longer exists' do + nonexistent_id = 999999999999 + + expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + subject.perform(nonexistent_id) + end + + it 'skips when project is pending delete' do + expect(::Projects::HashedStorageMigrationService).not_to receive(:new) - it 'skips when project is pending delete' do - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + subject.perform(pending_delete_project.id) + end - subject.perform(pending_delete_project.id) + it 'delegates removal to service class' do + service = double('service') + expect(::Projects::HashedStorageMigrationService).to receive(:new).with(project, subject.logger).and_return(service) + expect(service).to receive(:execute) + + subject.perform(project.id) + end end - it 'delegates removal to service class' do - service = double('service') - expect(::Projects::HashedStorageMigrationService).to receive(:new).with(project, subject.logger).and_return(service) - expect(service).to receive(:execute) + context 'when dont have exclusive lease' do + before do + allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false) + end + + it 'skips when dont have lease' do + expect(::Projects::HashedStorageMigrationService).not_to receive(:new) - subject.perform(project.id) + subject.perform(project.id) + end end end end -- cgit v1.2.1 From 10c2ba7dbbae15cde019135fe89b20dbd793dadf Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 18 Nov 2017 08:12:37 +0100 Subject: Changelog and Documentation for storage migration of project attachments --- .../hashed-storage-attachments-migration-path.yml | 5 ++ doc/administration/raketasks/storage.md | 97 +++++++++++++++++++--- 2 files changed, 89 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/hashed-storage-attachments-migration-path.yml diff --git a/changelogs/unreleased/hashed-storage-attachments-migration-path.yml b/changelogs/unreleased/hashed-storage-attachments-migration-path.yml new file mode 100644 index 00000000000..32535437046 --- /dev/null +++ b/changelogs/unreleased/hashed-storage-attachments-migration-path.yml @@ -0,0 +1,5 @@ +--- +title: Hashed Storage migration script now supports migrating project attachments +merge_request: 15352 +author: +type: added diff --git a/doc/administration/raketasks/storage.md b/doc/administration/raketasks/storage.md index bac8fa4bd9d..6ec5baeb6e3 100644 --- a/doc/administration/raketasks/storage.md +++ b/doc/administration/raketasks/storage.md @@ -1,10 +1,43 @@ # Repository Storage Rake Tasks This is a collection of rake tasks you can use to help you list and migrate -existing projects from Legacy storage to the new Hashed storage type. +existing projects and attachments associated with it from Legacy storage to +the new Hashed storage type. You can read more about the storage types [here][storage-types]. +## Migrate existing projects to Hashed storage + +Before migrating your existing projects, you should +[enable hashed storage][storage-migration] for the new projects as well. + +This task will schedule all your existing projects and attachments associated with it to be migrated to the +**Hashed** storage type: + +**Omnibus Installation** + +```bash +gitlab-rake gitlab:storage:migrate_to_hashed +``` + +**Source Installation** + +```bash +rake gitlab:storage:migrate_to_hashed + +``` + +You can monitor the progress in the _Admin > Monitoring > Background jobs_ screen. +There is a specific Queue you can watch to see how long it will take to finish: **project_migrate_hashed_storage** + +After it reaches zero, you can confirm every project has been migrated by running the commands bellow. +If you find it necessary, you can run this migration script again to schedule missing projects. + +Any error or warning will be logged in the sidekiq's log file. + +You only need the `gitlab:storage:migrate_to_hashed` rake task to migrate your repositories, but we have additional +commands below that helps you inspect projects and attachments in both legacy and hashed storage. + ## List projects on Legacy storage To have a simple summary of projects using **Legacy** storage: @@ -73,35 +106,73 @@ rake gitlab:storage:list_hashed_projects ``` -## Migrate existing projects to Hashed storage +## List attachments on Legacy storage -Before migrating your existing projects, you should -[enable hashed storage][storage-migration] for the new projects as well. +To have a simple summary of project attachments using **Legacy** storage: -This task will schedule all your existing projects to be migrated to the -**Hashed** storage type: +**Omnibus Installation** + +```bash +gitlab-rake gitlab:storage:legacy_attachments +``` + +**Source Installation** + +```bash +rake gitlab:storage:legacy_attachments + +``` + +------ + +To list project attachments using **Legacy** storage: **Omnibus Installation** ```bash -gitlab-rake gitlab:storage:migrate_to_hashed +gitlab-rake gitlab:storage:list_legacy_attachments ``` **Source Installation** ```bash -rake gitlab:storage:migrate_to_hashed +rake gitlab:storage:list_legacy_attachments ``` -You can monitor the progress in the _Admin > Monitoring > Background jobs_ screen. -There is a specific Queue you can watch to see how long it will take to finish: **project_migrate_hashed_storage** +## List attachments on Hashed storage -After it reaches zero, you can confirm every project has been migrated by running the commands above. -If you find it necessary, you can run this migration script again to schedule missing projects. +To have a simple summary of project attachments using **Hashed** storage: + +**Omnibus Installation** + +```bash +gitlab-rake gitlab:storage:hashed_attachments +``` -Any error or warning will be logged in the sidekiq log file. +**Source Installation** + +```bash +rake gitlab:storage:hashed_attachments + +``` + +------ + +To list project attachments using **Hashed** storage: + +**Omnibus Installation** +```bash +gitlab-rake gitlab:storage:list_hashed_attachments +``` + +**Source Installation** + +```bash +rake gitlab:storage:list_hashed_attachments + +``` [storage-types]: ../repository_storage_types.md [storage-migration]: ../repository_storage_types.md#how-to-migrate-to-hashed-storage -- cgit v1.2.1 From 05876a49c67fd94777801c09129e7dd77873e668 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 22 Nov 2017 16:29:03 +0100 Subject: fix exclusive lease specs fro hashed storage migration --- app/workers/project_migrate_hashed_storage_worker.rb | 12 ++++++------ spec/workers/project_migrate_hashed_storage_worker_spec.rb | 12 +++++++++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb index e3ecd6bc950..127aa6b9d7d 100644 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ b/app/workers/project_migrate_hashed_storage_worker.rb @@ -8,23 +8,23 @@ class ProjectMigrateHashedStorageWorker project = Project.find_by(id: project_id) return if project.nil? || project.pending_delete? - uuid = try_obtain_lease_for(project_id) + uuid = lease_for(project_id).try_obtain if uuid ::Projects::HashedStorageMigrationService.new(project, logger).execute else false end rescue => ex - cancel_lease_for(project_id, uuid) + cancel_lease_for(project_id, uuid) if uuid raise ex end - private - - def try_obtain_lease_for(project_id) - Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT).try_obtain + def lease_for(project_id) + Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT) end + private + def lease_key(project_id) "project_migrate_hashed_storage_worker:#{project_id}" end diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb index 8cacdfa7173..2e3951e7afc 100644 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ b/spec/workers/project_migrate_hashed_storage_worker_spec.rb @@ -1,13 +1,16 @@ require 'spec_helper' -describe ProjectMigrateHashedStorageWorker do +describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do describe '#perform' do let(:project) { create(:project, :empty_repo) } let(:pending_delete_project) { create(:project, :empty_repo, pending_delete: true) } context 'when have exclusive lease' do before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true) + lease = subject.lease_for(project.id) + + allow(Gitlab::ExclusiveLease).to receive(:new).and_return(lease) + allow(lease).to receive(:try_obtain).and_return(true) end it 'skips when project no longer exists' do @@ -34,7 +37,10 @@ describe ProjectMigrateHashedStorageWorker do context 'when dont have exclusive lease' do before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false) + lease = subject.lease_for(project.id) + + allow(Gitlab::ExclusiveLease).to receive(:new).and_return(lease) + allow(lease).to receive(:try_obtain).and_return(false) end it 'skips when dont have lease' do -- cgit v1.2.1 From dc62441ffd47d63f342c6accbd5049f6e8f99303 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 24 Nov 2017 03:31:08 +0100 Subject: Backport EE changes to make test possible when prepending modules --- .../hashed_storage/migrate_attachments_service.rb | 4 ++++ .../hashed_storage/migrate_repository_service.rb | 4 ++++ .../projects/hashed_storage_migration_service_spec.rb | 16 ++++++++++------ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 93f44110605..8cac6221a96 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -17,6 +17,10 @@ module Projects move_folder!(old_path, new_path) project.save! + + if block_given? + yield + end end private diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index da43877a185..7212e7524ab 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -35,6 +35,10 @@ module Projects project.repository_read_only = false project.save! + if result && block_given? + yield + end + result end diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb index 28b6daf217e..466f0b5d7c2 100644 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ b/spec/services/projects/hashed_storage_migration_service_spec.rb @@ -6,32 +6,36 @@ describe Projects::HashedStorageMigrationService do describe '#execute' do context 'repository migration' do + let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, subject.logger) } + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do - expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_call_original - expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).to receive(:execute) + expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_return(repository_service) + expect(repository_service).to receive(:execute) service.execute end it 'does not delegate migration if repository is already migrated' do project.storage_version = ::Project::LATEST_STORAGE_VERSION - expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:execute) + expect(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:new) service.execute end end context 'attachments migration' do + let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, subject.logger) } + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do - expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_call_original - expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).to receive(:execute) + expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_return(attachments_service) + expect(attachments_service).to receive(:execute) service.execute end it 'does not delegate migration if attachments are already migrated' do project.storage_version = ::Project::LATEST_STORAGE_VERSION - expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:execute) + expect(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:new) service.execute end -- cgit v1.2.1 From 4beacdb2ca6ea884473cb63aee603951df7f2da1 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 24 Nov 2017 14:27:38 +0100 Subject: Fix defaults for MR states and merge statuses This ensures that merge_requests.state and merge_requests.merge_status both have a proper default value and NOT NULL constraint on database level. We also make sure to update any bogus rows first, without blowing up the database. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/40534 --- .../unreleased/default-values-for-mr-states.yml | 5 +++ ...2_add_default_values_to_merge_request_states.rb | 19 ++++++++ ...5748_populate_missing_merge_request_statuses.rb | 50 ++++++++++++++++++++++ ...4132536_make_merge_request_statuses_not_null.rb | 14 ++++++ db/schema.rb | 6 +-- 5 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/default-values-for-mr-states.yml create mode 100644 db/migrate/20171124125042_add_default_values_to_merge_request_states.rb create mode 100644 db/migrate/20171124125748_populate_missing_merge_request_statuses.rb create mode 100644 db/migrate/20171124132536_make_merge_request_statuses_not_null.rb diff --git a/changelogs/unreleased/default-values-for-mr-states.yml b/changelogs/unreleased/default-values-for-mr-states.yml new file mode 100644 index 00000000000..f873a5335d0 --- /dev/null +++ b/changelogs/unreleased/default-values-for-mr-states.yml @@ -0,0 +1,5 @@ +--- +title: Fix defaults for MR states and merge statuses +merge_request: +author: +type: fixed diff --git a/db/migrate/20171124125042_add_default_values_to_merge_request_states.rb b/db/migrate/20171124125042_add_default_values_to_merge_request_states.rb new file mode 100644 index 00000000000..d08863c3b78 --- /dev/null +++ b/db/migrate/20171124125042_add_default_values_to_merge_request_states.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddDefaultValuesToMergeRequestStates < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def up + change_column_default :merge_requests, :state, :opened + change_column_default :merge_requests, :merge_status, :unchecked + end + + def down + change_column_default :merge_requests, :state, nil + change_column_default :merge_requests, :merge_status, nil + end +end diff --git a/db/migrate/20171124125748_populate_missing_merge_request_statuses.rb b/db/migrate/20171124125748_populate_missing_merge_request_statuses.rb new file mode 100644 index 00000000000..72fbab59f4c --- /dev/null +++ b/db/migrate/20171124125748_populate_missing_merge_request_statuses.rb @@ -0,0 +1,50 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PopulateMissingMergeRequestStatuses < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + end + + def up + say 'Populating missing merge_requests.state values' + + # GitLab.com has no rows where "state" is NULL, and technically this should + # never happen. However it doesn't hurt to be 100% certain. + MergeRequest.where(state: nil).each_batch do |batch| + batch.update_all(state: 'opened') + end + + say 'Populating missing merge_requests.merge_status values. ' \ + 'This will take a few minutes...' + + # GitLab.com has 66 880 rows where "merge_status" is NULL, dating back all + # the way to 2011. + MergeRequest.where(merge_status: nil).each_batch(of: 10_000) do |batch| + batch.update_all(merge_status: 'unchecked') + + # We want to give PostgreSQL some time to vacuum any dead tuples. In + # production we see it takes roughly 1 minute for a vacuuming run to clear + # out 10-20k dead tuples, so we'll wait for 90 seconds between every + # batch. + sleep(90) if sleep? + end + end + + def down + # Reverting this makes no sense. + end + + def sleep? + Rails.env.staging? || Rails.env.production? + end +end diff --git a/db/migrate/20171124132536_make_merge_request_statuses_not_null.rb b/db/migrate/20171124132536_make_merge_request_statuses_not_null.rb new file mode 100644 index 00000000000..4bb09126036 --- /dev/null +++ b/db/migrate/20171124132536_make_merge_request_statuses_not_null.rb @@ -0,0 +1,14 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MakeMergeRequestStatusesNotNull < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + change_column_null :merge_requests, :state, false + change_column_null :merge_requests, :merge_status, false + end +end diff --git a/db/schema.rb b/db/schema.rb index d10561099b7..804bc8d6e37 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171121144800) do +ActiveRecord::Schema.define(version: 20171124132536) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1049,8 +1049,8 @@ ActiveRecord::Schema.define(version: 20171121144800) do t.datetime "created_at" t.datetime "updated_at" t.integer "milestone_id" - t.string "state" - t.string "merge_status" + t.string "state", default: "opened", null: false + t.string "merge_status", default: "unchecked", null: false t.integer "target_project_id", null: false t.integer "iid" t.text "description" -- cgit v1.2.1 From a76cd8833c8e5d72decba7139c5f57a29eae6a0c Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 23 Nov 2017 17:16:01 +0000 Subject: Added IDE commit panel Closes #40041 --- .../repo/components/commit_sidebar/list.vue | 90 +++++ .../components/commit_sidebar/list_collapsed.vue | 41 +++ .../repo/components/commit_sidebar/list_item.vue | 36 ++ app/assets/javascripts/repo/components/repo.vue | 32 +- .../repo/components/repo_commit_section.vue | 159 ++++---- .../javascripts/repo/components/repo_file.vue | 3 +- .../repo/components/repo_file_buttons.vue | 10 +- .../javascripts/repo/components/repo_preview.vue | 6 +- .../javascripts/repo/components/repo_sidebar.vue | 8 +- .../javascripts/repo/components/repo_tab.vue | 25 +- .../javascripts/repo/components/repo_tabs.vue | 4 +- app/assets/javascripts/repo/stores/getters.js | 4 + app/assets/stylesheets/pages/repo.scss | 408 ++++++++++----------- app/views/projects/tree/show.html.haml | 2 +- app/views/shared/repo/_repo.html.haml | 1 + 15 files changed, 492 insertions(+), 337 deletions(-) create mode 100644 app/assets/javascripts/repo/components/commit_sidebar/list.vue create mode 100644 app/assets/javascripts/repo/components/commit_sidebar/list_collapsed.vue create mode 100644 app/assets/javascripts/repo/components/commit_sidebar/list_item.vue diff --git a/app/assets/javascripts/repo/components/commit_sidebar/list.vue b/app/assets/javascripts/repo/components/commit_sidebar/list.vue new file mode 100644 index 00000000000..348d3afc830 --- /dev/null +++ b/app/assets/javascripts/repo/components/commit_sidebar/list.vue @@ -0,0 +1,90 @@ + + + diff --git a/app/assets/javascripts/repo/components/commit_sidebar/list_collapsed.vue b/app/assets/javascripts/repo/components/commit_sidebar/list_collapsed.vue new file mode 100644 index 00000000000..b45949e3e75 --- /dev/null +++ b/app/assets/javascripts/repo/components/commit_sidebar/list_collapsed.vue @@ -0,0 +1,41 @@ + + + diff --git a/app/assets/javascripts/repo/components/commit_sidebar/list_item.vue b/app/assets/javascripts/repo/components/commit_sidebar/list_item.vue new file mode 100644 index 00000000000..c8d4a2ad307 --- /dev/null +++ b/app/assets/javascripts/repo/components/commit_sidebar/list_item.vue @@ -0,0 +1,36 @@ + + + diff --git a/app/assets/javascripts/repo/components/repo.vue b/app/assets/javascripts/repo/components/repo.vue index 98117802016..a00e1e9d809 100644 --- a/app/assets/javascripts/repo/components/repo.vue +++ b/app/assets/javascripts/repo/components/repo.vue @@ -40,20 +40,24 @@ export default { diff --git a/app/assets/javascripts/repo/components/repo_commit_section.vue b/app/assets/javascripts/repo/components/repo_commit_section.vue index 377e3d65348..e4ee4da248c 100644 --- a/app/assets/javascripts/repo/components/repo_commit_section.vue +++ b/app/assets/javascripts/repo/components/repo_commit_section.vue @@ -1,11 +1,18 @@ diff --git a/app/assets/javascripts/repo/components/repo_file.vue b/app/assets/javascripts/repo/components/repo_file.vue index 5be47d568e7..54b26537ab9 100644 --- a/app/assets/javascripts/repo/components/repo_file.vue +++ b/app/assets/javascripts/repo/components/repo_file.vue @@ -85,12 +85,11 @@