From ccb9beeed0e418ef4dea201b3507bd2f4a14b4a2 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 18 Apr 2017 21:26:56 +0200 Subject: [PATCH 1/4] Properly expire cache for **all** MRs of a pipeline Turn ExpirePipelineCacheService into Worker so it can fetch all the merge requests for which the pipeline runs or did run against. --- app/models/ci/pipeline.rb | 10 +++- .../ci/expire_pipeline_cache_service.rb | 51 ------------------- app/workers/expire_pipeline_cache_worker.rb | 51 +++++++++++++++++++ spec/models/ci/pipeline_spec.rb | 4 +- .../expire_pipeline_cache_worker_spec.rb} | 10 ++-- 5 files changed, 66 insertions(+), 60 deletions(-) delete mode 100644 app/services/ci/expire_pipeline_cache_service.rb create mode 100644 app/workers/expire_pipeline_cache_worker.rb rename spec/{services/ci/expire_pipeline_cache_service_spec.rb => workers/expire_pipeline_cache_worker_spec.rb} (80%) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 445247f1b41..bd5b0ed3cff 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -96,8 +96,7 @@ module Ci pipeline.run_after_commit do PipelineHooksWorker.perform_async(id) - Ci::ExpirePipelineCacheService.new(project, nil) - .execute(pipeline) + ExpirePipelineCacheWorker.perform_async(pipeline.id) end end @@ -385,6 +384,13 @@ module Ci .select { |merge_request| merge_request.head_pipeline.try(:id) == self.id } end + # All the merge requests for which the current pipeline runs/ran against + def all_merge_requests + @all_merge_requests ||= project.merge_requests + .where(source_branch: ref) + .select { |merge_request| merge_request.all_pipelines.includes(self) } + end + def detailed_status(current_user) Gitlab::Ci::Status::Pipeline::Factory .new(self, current_user) diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb deleted file mode 100644 index 91d9c1d2ba1..00000000000 --- a/app/services/ci/expire_pipeline_cache_service.rb +++ /dev/null @@ -1,51 +0,0 @@ -module Ci - class ExpirePipelineCacheService < BaseService - attr_reader :pipeline - - def execute(pipeline) - @pipeline = pipeline - store = Gitlab::EtagCaching::Store.new - - store.touch(project_pipelines_path) - store.touch(commit_pipelines_path) if pipeline.commit - store.touch(new_merge_request_pipelines_path) - merge_requests_pipelines_paths.each { |path| store.touch(path) } - - Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(@pipeline) - end - - private - - def project_pipelines_path - Gitlab::Routing.url_helpers.namespace_project_pipelines_path( - project.namespace, - project, - format: :json) - end - - def commit_pipelines_path - Gitlab::Routing.url_helpers.pipelines_namespace_project_commit_path( - project.namespace, - project, - pipeline.commit.id, - format: :json) - end - - def new_merge_request_pipelines_path - Gitlab::Routing.url_helpers.new_namespace_project_merge_request_path( - project.namespace, - project, - format: :json) - end - - def merge_requests_pipelines_paths - pipeline.merge_requests.collect do |merge_request| - Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path( - project.namespace, - project, - merge_request, - format: :json) - end - end - end -end diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb new file mode 100644 index 00000000000..65e7b091506 --- /dev/null +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -0,0 +1,51 @@ +class ExpirePipelineCacheWorker + include Sidekiq::Worker + include PipelineQueue + + def perform(id) + pipeline = Ci::Pipeline.find(id) + project = pipeline.project + store = Gitlab::EtagCaching::Store.new + + store.touch(project_pipelines_path(project)) + store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit + store.touch(new_merge_request_pipelines_path(project)) + merge_requests_pipelines_paths(project, pipeline).each { |path| store.touch(path) } + + Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline) + end + + private + + def project_pipelines_path(project) + Gitlab::Routing.url_helpers.namespace_project_pipelines_path( + project.namespace, + project, + format: :json) + end + + def commit_pipelines_path(project, commit) + Gitlab::Routing.url_helpers.pipelines_namespace_project_commit_path( + project.namespace, + project, + commit.id, + format: :json) + end + + def new_merge_request_pipelines_path(project) + Gitlab::Routing.url_helpers.new_namespace_project_merge_request_path( + project.namespace, + project, + format: :json) + end + + def merge_requests_pipelines_paths(project, pipeline) + pipeline.all_merge_requests.collect do |merge_request| + Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path( + project.namespace, + project, + merge_request, + format: :json) + end + end +end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index d7d6a75d38d..42c0791fba1 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -376,8 +376,8 @@ describe Ci::Pipeline, models: true do end describe 'pipeline caching' do - it 'executes ExpirePipelinesCacheService' do - expect_any_instance_of(Ci::ExpirePipelineCacheService).to receive(:execute).with(pipeline) + it 'performs ExpirePipelinesCacheWorker' do + expect(ExpirePipelineCacheWorker).to receive(:perform_async).with(pipeline.id) pipeline.cancel end diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb similarity index 80% rename from spec/services/ci/expire_pipeline_cache_service_spec.rb rename to spec/workers/expire_pipeline_cache_worker_spec.rb index 166c6dfc93e..0138bfa4359 100644 --- a/spec/services/ci/expire_pipeline_cache_service_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -1,12 +1,12 @@ require 'spec_helper' -describe Ci::ExpirePipelineCacheService, services: true do +describe ExpirePipelineCacheWorker do let(:user) { create(:user) } let(:project) { create(:empty_project) } let(:pipeline) { create(:ci_pipeline, project: project) } - subject { described_class.new(project, user) } + subject { described_class.new } - describe '#execute' do + describe '#perform' do it 'invalidate Etag caching for project pipelines path' do pipelines_path = "/#{project.full_path}/pipelines.json" new_mr_pipelines_path = "/#{project.full_path}/merge_requests/new.json" @@ -14,14 +14,14 @@ describe Ci::ExpirePipelineCacheService, services: true do expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipelines_path) expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(new_mr_pipelines_path) - subject.execute(pipeline) + subject.perform(pipeline.id) end it 'updates the cached status for a project' do expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline). with(pipeline) - subject.execute(pipeline) + subject.perform(pipeline.id) end end end From c623c41c2f917e0773a6e3f0b6c78b027ca846f7 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 20 Apr 2017 08:44:01 +0200 Subject: [PATCH 2/4] Use a better performing query to find all MRs for pipeline And add some specs. --- app/models/ci/pipeline.rb | 12 ++++------ app/workers/expire_pipeline_cache_worker.rb | 4 ++-- .../tc-realtime-every-pipeline-on-mr.yml | 4 ++++ spec/models/ci/pipeline_spec.rb | 24 +++++++++++++++++++ .../expire_pipeline_cache_worker_spec.rb | 14 ++++++++++- 5 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index bd5b0ed3cff..b5fe589706f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -80,22 +80,22 @@ module Ci end after_transition [:created, :pending] => :running do |pipeline| - pipeline.run_after_commit { PipelineMetricsWorker.perform_async(id) } + pipeline.run_after_commit { PipelineMetricsWorker.perform_async(pipeline.id) } end after_transition any => [:success] do |pipeline| - pipeline.run_after_commit { PipelineMetricsWorker.perform_async(id) } + pipeline.run_after_commit { PipelineMetricsWorker.perform_async(pipeline.id) } end after_transition [:created, :pending, :running] => :success do |pipeline| - pipeline.run_after_commit { PipelineSuccessWorker.perform_async(id) } + pipeline.run_after_commit { PipelineSuccessWorker.perform_async(pipeline.id) } end after_transition do |pipeline, transition| next if transition.loopback? pipeline.run_after_commit do - PipelineHooksWorker.perform_async(id) + PipelineHooksWorker.perform_async(pipeline.id) ExpirePipelineCacheWorker.perform_async(pipeline.id) end end @@ -386,9 +386,7 @@ module Ci # All the merge requests for which the current pipeline runs/ran against def all_merge_requests - @all_merge_requests ||= project.merge_requests - .where(source_branch: ref) - .select { |merge_request| merge_request.all_pipelines.includes(self) } + @all_merge_requests ||= project.merge_requests.where(source_branch: ref) end def detailed_status(current_user) diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 65e7b091506..0210d459048 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -2,8 +2,8 @@ class ExpirePipelineCacheWorker include Sidekiq::Worker include PipelineQueue - def perform(id) - pipeline = Ci::Pipeline.find(id) + def perform(pipeline_id) + pipeline = Ci::Pipeline.find(pipeline_id) project = pipeline.project store = Gitlab::EtagCaching::Store.new diff --git a/changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml b/changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml new file mode 100644 index 00000000000..944baae257c --- /dev/null +++ b/changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml @@ -0,0 +1,4 @@ +--- +title: Properly expire cache for all MRs of a pipeline +merge_request: 10770 +author: diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 42c0791fba1..be14fdca81c 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1037,6 +1037,30 @@ describe Ci::Pipeline, models: true do end end + describe "#all_merge_requests" do + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) } + + it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do + merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + + expect(pipeline.all_merge_requests).to eq([merge_request]) + end + + it "returns merge requests whose source branch matches the pipeline's source branch" do + pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id) + merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + + expect(pipeline.all_merge_requests).to eq([merge_request]) + end + + it "doesn't return merge requests whose source branch doesn't match the pipeline's ref" do + create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') + + expect(pipeline.all_merge_requests).to be_empty + end + end + describe '#stuck?' do before do create(:ci_build, :pending, pipeline: pipeline) diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb index 0138bfa4359..67f01539ada 100644 --- a/spec/workers/expire_pipeline_cache_worker_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -7,7 +7,7 @@ describe ExpirePipelineCacheWorker do subject { described_class.new } describe '#perform' do - it 'invalidate Etag caching for project pipelines path' do + it 'invalidates Etag caching for project pipelines path' do pipelines_path = "/#{project.full_path}/pipelines.json" new_mr_pipelines_path = "/#{project.full_path}/merge_requests/new.json" @@ -17,6 +17,18 @@ describe ExpirePipelineCacheWorker do subject.perform(pipeline.id) end + it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do + project = create(:project, :repository) + pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id) + merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + merge_request_pipelines_path = "/#{project.full_path}/merge_requests/#{merge_request.iid}/pipelines.json" + + allow_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch) + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_pipelines_path) + + subject.perform(pipeline.id) + end + it 'updates the cached status for a project' do expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline). with(pipeline) From 14642e3c28ae40d2ecf409c327e29e6c3add63fa Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 21 Apr 2017 17:39:19 +0200 Subject: [PATCH 3/4] Refactor ExpirePipelineCacheWorker#perform Make it gracefully handle unexisting pipelines and refactor iterating all the merge request paths. --- app/workers/expire_pipeline_cache_worker.rb | 16 +++++++++++----- spec/models/ci/pipeline_spec.rb | 18 ++++++++++-------- .../expire_pipeline_cache_worker_spec.rb | 6 ++++++ 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 0210d459048..603e2f1aaea 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -3,14 +3,18 @@ class ExpirePipelineCacheWorker include PipelineQueue def perform(pipeline_id) - pipeline = Ci::Pipeline.find(pipeline_id) + pipeline = Ci::Pipeline.find_by(id: pipeline_id) + return unless pipeline + project = pipeline.project store = Gitlab::EtagCaching::Store.new store.touch(project_pipelines_path(project)) store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit store.touch(new_merge_request_pipelines_path(project)) - merge_requests_pipelines_paths(project, pipeline).each { |path| store.touch(path) } + each_pipelines_merge_request_path(project, pipeline) do |path| + store.touch(path) + end Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline) end @@ -39,13 +43,15 @@ class ExpirePipelineCacheWorker format: :json) end - def merge_requests_pipelines_paths(project, pipeline) - pipeline.all_merge_requests.collect do |merge_request| - Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path( + def each_pipelines_merge_request_path(project, pipeline) + pipeline.all_merge_requests.each do |merge_request| + path = Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path( project.namespace, project, merge_request, format: :json) + + yield(path) end end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index be14fdca81c..8b9b51bcf2f 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1014,11 +1014,12 @@ describe Ci::Pipeline, models: true do end describe "#merge_requests" do - let(:project) { create(:project, :repository) } - let(:pipeline) { FactoryGirl.create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) } + let(:project) { create(:empty_project) } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') } it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' } expect(pipeline.merge_requests).to eq([merge_request]) end @@ -1038,23 +1039,24 @@ describe Ci::Pipeline, models: true do end describe "#all_merge_requests" do - let(:project) { create(:project, :repository) } - let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) } + let(:project) { create(:empty_project) } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') } - it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do + it "returns merge request if pipeline runs on `diff_head_sha`" do merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' } expect(pipeline.all_merge_requests).to eq([merge_request]) end - it "returns merge requests whose source branch matches the pipeline's source branch" do - pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id) + it "returns merge request if pipeline runs any commit of the `source_branch`" do merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { '97de212e80737a608d939f648d959671fb0a0142b' } expect(pipeline.all_merge_requests).to eq([merge_request]) end - it "doesn't return merge requests whose source branch doesn't match the pipeline's ref" do + it "doesn't return merge request if pipeline runs on a different `source_branch`" do create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') expect(pipeline.all_merge_requests).to be_empty diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb index 67f01539ada..58f587f6d11 100644 --- a/spec/workers/expire_pipeline_cache_worker_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -29,6 +29,12 @@ describe ExpirePipelineCacheWorker do subject.perform(pipeline.id) end + it "doesn't do anything if the pipeline not exist" do + expect_any_instance_of(Gitlab::EtagCaching::Store).not_to receive(:touch) + + subject.perform(617748) + end + it 'updates the cached status for a project' do expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline). with(pipeline) From 956624688dec0d63024a424accc6a52b7bf04927 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 24 Apr 2017 10:18:37 +0200 Subject: [PATCH 4/4] Simplify specs and remove unnecessary attributes --- spec/models/ci/pipeline_spec.rb | 14 +++----------- spec/workers/expire_pipeline_cache_worker_spec.rb | 3 +-- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 8b9b51bcf2f..8e1695e0c70 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1040,23 +1040,15 @@ describe Ci::Pipeline, models: true do describe "#all_merge_requests" do let(:project) { create(:empty_project) } - let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') } - it "returns merge request if pipeline runs on `diff_head_sha`" do + it "returns all merge requests having the same source branch" do merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) - allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' } expect(pipeline.all_merge_requests).to eq([merge_request]) end - it "returns merge request if pipeline runs any commit of the `source_branch`" do - merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) - allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { '97de212e80737a608d939f648d959671fb0a0142b' } - - expect(pipeline.all_merge_requests).to eq([merge_request]) - end - - it "doesn't return merge request if pipeline runs on a different `source_branch`" do + it "doesn't return merge requests having a different source branch" do create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') expect(pipeline.all_merge_requests).to be_empty diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb index 58f587f6d11..ceba604dea2 100644 --- a/spec/workers/expire_pipeline_cache_worker_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -18,8 +18,7 @@ describe ExpirePipelineCacheWorker do end it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do - project = create(:project, :repository) - pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id) + pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) merge_request_pipelines_path = "/#{project.full_path}/merge_requests/#{merge_request.iid}/pipelines.json"