diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2016-10-18 09:48:36 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2016-10-18 09:48:36 +0000 |
commit | 275bcfb6b2b1a417fbedca8b175ed812d12ba70a (patch) | |
tree | aec892c1e918d7672e4d809e301c97afd7fe013c | |
parent | 4e6af0c3fa335d138343dce3e0216303a9b1cd79 (diff) | |
parent | b63b13f930804c83682f6424b8dddf2236649d6d (diff) | |
download | gitlab-ce-275bcfb6b2b1a417fbedca8b175ed812d12ba70a.tar.gz |
Merge branch 'fix/build-erase-race-condition' into 'master'
Avoid race condition when expiring artifacts
## What does this MR do?
It may happen that job which purpose is to remove expired artifacts will be executed asynchronously when, in the meantime, project associated with given build gets removed by another async job. In that case we should not remove artifacts because such build will be eventually removed anyway along with artifacts, when project removal is complete.
## Does this MR meet the acceptance criteria?
- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
- [x] Added for this feature/bug
- [x] All builds are passing
## What are the relevant issue numbers?
Closes #22296
See merge request !6881
-rw-r--r-- | CHANGELOG.md | 1 | ||||
-rw-r--r-- | app/workers/expire_build_instance_artifacts_worker.rb | 10 | ||||
-rw-r--r-- | spec/workers/expire_build_instance_artifacts_worker_spec.rb | 44 |
3 files changed, 40 insertions, 15 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dbe1832de0..b2501bd265e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. ## 8.13.0 (2016-10-22) + - Avoid race condition when asynchronously removing expired artifacts. (!6881) - Improve Merge When Build Succeeds triggers and execute on pipeline success. (!6675) - Respond with 404 Not Found for non-existent tags (Linus Thiel) - Truncate long labels with ellipsis in labels page diff --git a/app/workers/expire_build_instance_artifacts_worker.rb b/app/workers/expire_build_instance_artifacts_worker.rb index 916c2e633c1..d9e2cc37bb3 100644 --- a/app/workers/expire_build_instance_artifacts_worker.rb +++ b/app/workers/expire_build_instance_artifacts_worker.rb @@ -2,10 +2,14 @@ class ExpireBuildInstanceArtifactsWorker include Sidekiq::Worker def perform(build_id) - build = Ci::Build.with_expired_artifacts.reorder(nil).find_by(id: build_id) - return unless build + build = Ci::Build + .with_expired_artifacts + .reorder(nil) + .find_by(id: build_id) - Rails.logger.info "Removing artifacts build #{build.id}..." + return unless build.try(:project) + + Rails.logger.info "Removing artifacts for build #{build.id}..." build.erase_artifacts! end end diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb index 2b140f2ba28..d202b3de77e 100644 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb @@ -6,28 +6,48 @@ describe ExpireBuildInstanceArtifactsWorker do let(:worker) { described_class.new } describe '#perform' do - before { build } - - subject! { worker.perform(build.id) } + before do + worker.perform(build.id) + end context 'with expired artifacts' do - let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now - 7.days) } + let(:artifacts_expiry) { { artifacts_expire_at: Time.now - 7.days } } - it 'does expire' do - expect(build.reload.artifacts_expired?).to be_truthy - end + context 'when associated project is valid' do + let(:build) do + create(:ci_build, :artifacts, artifacts_expiry) + end - it 'does remove files' do - expect(build.reload.artifacts_file.exists?).to be_falsey + it 'does expire' do + expect(build.reload.artifacts_expired?).to be_truthy + end + + it 'does remove files' do + expect(build.reload.artifacts_file.exists?).to be_falsey + end + + it 'does nullify artifacts_file column' do + expect(build.reload.artifacts_file_identifier).to be_nil + end end - it 'does nullify artifacts_file column' do - expect(build.reload.artifacts_file_identifier).to be_nil + context 'when associated project was removed' do + let(:build) do + create(:ci_build, :artifacts, artifacts_expiry) do |build| + build.project.delete + end + end + + it 'does not remove artifacts' do + expect(build.reload.artifacts_file.exists?).to be_truthy + end end end context 'with not yet expired artifacts' do - let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days) } + let(:build) do + create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days) + end it 'does not expire' do expect(build.reload.artifacts_expired?).to be_falsey |