From 0b74b863679a8f55642973eddf25f9e58183d984 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Sat, 15 Dec 2018 13:49:15 +0000 Subject: [PATCH 1/2] Fix repository cleanup with object storage on When the BFG object map file is in object storage (i.e., uploads in general are placed into object storage), we get an instance of the Gitlab::HttpIO class. This doesn't behave as expected when you try to read past EOF, so we need to explicitly check for this condition to avoid ending up in a tight loop around io.read --- lib/gitlab/gitaly_client/cleanup_service.rb | 1 + .../lib/gitlab/git/repository_cleaner_spec.rb | 62 ++++++++++++++----- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/lib/gitlab/gitaly_client/cleanup_service.rb b/lib/gitlab/gitaly_client/cleanup_service.rb index 8e412a9b3ef..3e8d6a773ca 100644 --- a/lib/gitlab/gitaly_client/cleanup_service.rb +++ b/lib/gitlab/gitaly_client/cleanup_service.rb @@ -20,6 +20,7 @@ module Gitlab while data = io.read(RepositoryService::MAX_MSG_SIZE) y.yield Gitaly::ApplyBfgObjectMapRequest.new(object_map: data) + break if io&.eof? end end diff --git a/spec/lib/gitlab/git/repository_cleaner_spec.rb b/spec/lib/gitlab/git/repository_cleaner_spec.rb index a9d9e67ef94..7f9cc2bc9ec 100644 --- a/spec/lib/gitlab/git/repository_cleaner_spec.rb +++ b/spec/lib/gitlab/git/repository_cleaner_spec.rb @@ -1,31 +1,61 @@ require 'spec_helper' describe Gitlab::Git::RepositoryCleaner do + include HttpIOHelpers + let(:project) { create(:project, :repository) } let(:repository) { project.repository } let(:head_sha) { repository.head_commit.id } - - let(:object_map) { StringIO.new("#{head_sha} #{'0' * 40}") } + let(:object_map_data) { "#{head_sha} #{'0' * 40}" } subject(:cleaner) { described_class.new(repository.raw) } describe '#apply_bfg_object_map' do - it 'removes internal references pointing at SHAs in the object map' do - # Create some refs we expect to be removed - repository.keep_around(head_sha) - repository.create_ref(head_sha, 'refs/environments/1') - repository.create_ref(head_sha, 'refs/merge-requests/1') - repository.create_ref(head_sha, 'refs/heads/_keep') - repository.create_ref(head_sha, 'refs/tags/_keep') + let(:clean_refs) { %W[refs/environments/1 refs/merge-requests/1 refs/keep-around/#{head_sha}] } + let(:keep_refs) { %w[refs/heads/_keep refs/tags/_keep] } - cleaner.apply_bfg_object_map(object_map) + before do + (clean_refs + keep_refs).each { |ref| repository.create_ref(head_sha, ref) } + end - aggregate_failures do - expect(repository.kept_around?(head_sha)).to be_falsy - expect(repository.ref_exists?('refs/environments/1')).to be_falsy - expect(repository.ref_exists?('refs/merge-requests/1')).to be_falsy - expect(repository.ref_exists?('refs/heads/_keep')).to be_truthy - expect(repository.ref_exists?('refs/tags/_keep')).to be_truthy + context 'from StringIO' do + let(:object_map) { StringIO.new(object_map_data) } + + it 'removes internal references' do + cleaner.apply_bfg_object_map(object_map) + + aggregate_failures do + clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_falsy } + keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_truthy } + end + end + end + + context 'from Gitlab::HttpIO' do + let(:url) { 'http://example.com/bfg_object_map.txt' } + let(:tempfile) { Tempfile.new } + let(:object_map) { Gitlab::HttpIO.new(url, object_map_data.size) } + + around do |example| + begin + tempfile.write(object_map_data) + tempfile.close + + example.run + ensure + tempfile.unlink + end + end + + it 'removes internal references' do + stub_remote_url_200(url, tempfile.path) + + cleaner.apply_bfg_object_map(object_map) + + aggregate_failures do + clean_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_falsy } + keep_refs.each { |ref| expect(repository.ref_exists?(ref)).to be_truthy } + end end end end From e8a675d35f02c6bca9d0e3c8cc116ccd240fa4f6 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 13 Dec 2018 17:52:38 +0000 Subject: [PATCH 2/2] Remove the project_cleanup feature flag --- .../settings/repository_controller.rb | 5 --- app/views/projects/cleanup/_show.html.haml | 2 -- .../settings/repository_controller_spec.rb | 30 +++-------------- .../settings/repository_settings_spec.rb | 32 ++++++------------- 4 files changed, 14 insertions(+), 55 deletions(-) diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 30724de7f6a..ac3004d069f 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -5,7 +5,6 @@ module Projects class RepositoryController < Projects::ApplicationController before_action :authorize_admin_project! before_action :remote_mirror, only: [:show] - before_action :check_cleanup_feature_flag!, only: :cleanup def show render_show @@ -37,10 +36,6 @@ module Projects private - def check_cleanup_feature_flag! - render_404 unless ::Feature.enabled?(:project_cleanup, project) - end - def render_show @deploy_keys = DeployKeysPresenter.new(@project, current_user: current_user) @deploy_tokens = @project.deploy_tokens.active diff --git a/app/views/projects/cleanup/_show.html.haml b/app/views/projects/cleanup/_show.html.haml index 778d27fc61d..cecc139b183 100644 --- a/app/views/projects/cleanup/_show.html.haml +++ b/app/views/projects/cleanup/_show.html.haml @@ -1,5 +1,3 @@ -- return unless Feature.enabled?(:project_cleanup, @project) - - expanded = Rails.env.test? %section.settings.no-animate#cleanup{ class: ('expanded' if expanded) } diff --git a/spec/controllers/projects/settings/repository_controller_spec.rb b/spec/controllers/projects/settings/repository_controller_spec.rb index 70f79a47e63..1c6ddfc1864 100644 --- a/spec/controllers/projects/settings/repository_controller_spec.rb +++ b/spec/controllers/projects/settings/repository_controller_spec.rb @@ -19,35 +19,15 @@ describe Projects::Settings::RepositoryController do end describe 'PUT cleanup' do - before do - allow(RepositoryCleanupWorker).to receive(:perform_async) - end + let(:object_map) { fixture_file_upload('spec/fixtures/bfg_object_map.txt') } - def do_put! - object_map = fixture_file_upload('spec/fixtures/bfg_object_map.txt') + it 'enqueues a RepositoryCleanupWorker' do + allow(RepositoryCleanupWorker).to receive(:perform_async) put :cleanup, namespace_id: project.namespace, project_id: project, project: { object_map: object_map } - end - context 'feature enabled' do - it 'enqueues a RepositoryCleanupWorker' do - stub_feature_flags(project_cleanup: true) - - do_put! - - expect(response).to redirect_to project_settings_repository_path(project) - expect(RepositoryCleanupWorker).to have_received(:perform_async).once - end - end - - context 'feature disabled' do - it 'shows a 404 error' do - stub_feature_flags(project_cleanup: false) - - do_put! - - expect(response).to have_gitlab_http_status(404) - end + expect(response).to redirect_to project_settings_repository_path(project) + expect(RepositoryCleanupWorker).to have_received(:perform_async).once end end end diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 418e22f8c35..1982136b89d 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/spec/features/projects/settings/repository_settings_spec.rb @@ -200,35 +200,21 @@ describe 'Projects > Settings > Repository settings' do context 'repository cleanup settings' do let(:object_map_file) { Rails.root.join('spec', 'fixtures', 'bfg_object_map.txt') } - context 'feature enabled' do - it 'uploads an object map file', :js do - stub_feature_flags(project_cleanup: true) + it 'uploads an object map file', :js do + visit project_settings_repository_path(project) - visit project_settings_repository_path(project) + expect(page).to have_content('Repository cleanup') - expect(page).to have_content('Repository cleanup') + page.within('#cleanup') do + attach_file('project[bfg_object_map]', object_map_file, visible: false) - page.within('#cleanup') do - attach_file('project[bfg_object_map]', object_map_file, visible: false) - - Sidekiq::Testing.fake! do - click_button 'Start cleanup' - end + Sidekiq::Testing.fake! do + click_button 'Start cleanup' end - - expect(page).to have_content('Repository cleanup has started') - expect(RepositoryCleanupWorker.jobs.count).to eq(1) end - end - context 'feature disabled' do - it 'does not show the settings' do - stub_feature_flags(project_cleanup: false) - - visit project_settings_repository_path(project) - - expect(page).not_to have_content('Repository cleanup') - end + expect(page).to have_content('Repository cleanup has started') + expect(RepositoryCleanupWorker.jobs.count).to eq(1) end end end