diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2018-03-06 12:11:00 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2018-03-06 12:11:00 +0000 |
commit | 2a7b3d074338541d9152610fcce0d768ce4c8170 (patch) | |
tree | bede4f0707206c2b7f70d7031f967cec4ce09cf2 | |
parent | ce12b60e97a9a4518dd99b990e20e55ec8da22bc (diff) | |
parent | c428aaac6613b9fcfecd479f7bb510a6e74b761c (diff) | |
download | gitlab-ce-2a7b3d074338541d9152610fcce0d768ce4c8170.tar.gz |
Merge branch 'fix/sm/fix_pages_worker' into 'master'
Reload stable object to prevent erase artifacts with old state (Ver 2)
Closes #43641
See merge request gitlab-org/gitlab-ce!17522
-rw-r--r-- | app/services/projects/update_pages_service.rb | 35 | ||||
-rw-r--r-- | app/workers/pages_worker.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/fix-sm-fix_pages_worker.yml | 5 | ||||
-rw-r--r-- | spec/features/projects/pages_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/projects/update_pages_service_spec.rb | 16 |
5 files changed, 45 insertions, 15 deletions
diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index c760bd3b626..00fdd047208 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -1,5 +1,8 @@ module Projects class UpdatePagesService < BaseService + InvaildStateError = Class.new(StandardError) + FailedToExtractError = Class.new(StandardError) + BLOCK_SIZE = 32.kilobytes MAX_SIZE = 1.terabyte SITE_PATH = 'public/'.freeze @@ -11,13 +14,15 @@ module Projects end def execute + register_attempt + # Create status notifying the deployment of pages @status = create_status @status.enqueue! @status.run! - raise 'missing pages artifacts' unless build.artifacts? - raise 'pages are outdated' unless latest? + raise InvaildStateError, 'missing pages artifacts' unless build.artifacts? + raise InvaildStateError, 'pages are outdated' unless latest? # Create temporary directory in which we will extract the artifacts FileUtils.mkdir_p(tmp_path) @@ -26,24 +31,22 @@ module Projects # Check if we did extract public directory archive_public_path = File.join(archive_path, 'public') - raise 'pages miss the public folder' unless Dir.exist?(archive_public_path) - raise 'pages are outdated' unless latest? + raise FailedToExtractError, 'pages miss the public folder' unless Dir.exist?(archive_public_path) + raise InvaildStateError, 'pages are outdated' unless latest? deploy_page!(archive_public_path) success end - rescue => e + rescue InvaildStateError, FailedToExtractError => e register_failure error(e.message) - ensure - register_attempt - build.erase_artifacts! unless build.has_expiring_artifacts? end private def success @status.success + delete_artifact! super end @@ -52,6 +55,7 @@ module Projects @status.allow_failure = !latest? @status.description = message @status.drop(:script_failure) + delete_artifact! super end @@ -72,7 +76,7 @@ module Projects elsif artifacts.ends_with?('.zip') extract_zip_archive!(temp_path) else - raise 'unsupported artifacts format' + raise FailedToExtractError, 'unsupported artifacts format' end end @@ -81,17 +85,17 @@ module Projects %W(dd bs=#{BLOCK_SIZE} count=#{blocks}), %W(tar -x -C #{temp_path} #{SITE_PATH}), err: '/dev/null') - raise 'pages failed to extract' unless results.compact.all?(&:success?) + raise FailedToExtractError, 'pages failed to extract' unless results.compact.all?(&:success?) end def extract_zip_archive!(temp_path) - raise 'missing artifacts metadata' unless build.artifacts_metadata? + raise FailedToExtractError, 'missing artifacts metadata' unless build.artifacts_metadata? # Calculate page size after extract public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true) if public_entry.total_size > max_size - raise "artifacts for pages are too large: #{public_entry.total_size}" + raise FailedToExtractError, "artifacts for pages are too large: #{public_entry.total_size}" end # Requires UnZip at least 6.00 Info-ZIP. @@ -100,7 +104,7 @@ module Projects # We add * to end of SITE_PATH, because we want to extract SITE_PATH and all subdirectories site_path = File.join(SITE_PATH, '*') unless system(*%W(unzip -qq -n #{artifacts} #{site_path} -d #{temp_path})) - raise 'pages failed to extract' + raise FailedToExtractError, 'pages failed to extract' end end @@ -163,6 +167,11 @@ module Projects build.artifacts_file.path end + def delete_artifact! + build.reload # Reload stable object to prevent erase artifacts with old state + build.erase_artifacts! unless build.has_expiring_artifacts? + end + def latest_sha project.commit(build.ref).try(:sha).to_s end diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index d3b95009364..66a0ff83bef 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -1,7 +1,7 @@ class PagesWorker include ApplicationWorker - sidekiq_options retry: false + sidekiq_options retry: 3 def perform(action, *arg) send(action, *arg) # rubocop:disable GitlabSecurity/PublicSend diff --git a/changelogs/unreleased/fix-sm-fix_pages_worker.yml b/changelogs/unreleased/fix-sm-fix_pages_worker.yml new file mode 100644 index 00000000000..190c7d3e83e --- /dev/null +++ b/changelogs/unreleased/fix-sm-fix_pages_worker.yml @@ -0,0 +1,5 @@ +--- +title: Fix pages flaky failure by reloading stale object +merge_request: 17522 +author: +type: fixed diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb index 2a0d235ef04..233d2e67b9d 100644 --- a/spec/features/projects/pages_spec.rb +++ b/spec/features/projects/pages_spec.rb @@ -258,7 +258,7 @@ feature 'Pages' do end let(:ci_build) do - build( + create( :ci_build, project: project, pipeline: pipeline, diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index bfb86284d86..934106627a9 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -34,6 +34,7 @@ describe Projects::UpdatePagesService do context 'with expiry date' do before do build.artifacts_expire_in = "2 days" + build.save! end it "doesn't delete artifacts" do @@ -105,6 +106,7 @@ describe Projects::UpdatePagesService do context 'with expiry date' do before do build.artifacts_expire_in = "2 days" + build.save! end it "doesn't delete artifacts" do @@ -159,6 +161,20 @@ describe Projects::UpdatePagesService do expect(execute).not_to eq(:success) end + + context 'when timeout happens by DNS error' do + before do + allow_any_instance_of(described_class) + .to receive(:extract_zip_archive!).and_raise(SocketError) + end + + it 'raises an error' do + expect { execute }.to raise_error(SocketError) + + build.reload + expect(build.artifacts?).to eq(true) + end + end end end |