From 16259796539f19f7b04ad078a28aa38c5b50912f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 12 Apr 2019 12:29:47 -0700 Subject: Properly expire all pipeline caches when pipeline is deleted When deleting a pipeline, only some of the cache structures were being expired, but not the full pipeline list. We have to synchronously schedule a pipeline cache expiration because the pipeline will be deleted if the Sidekiq expiration job picks it up. To do this, properly extract all the logic buried in the Sidekiq worker into a service, and then call the service. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60469 --- app/services/ci/destroy_pipeline_service.rb | 4 +- app/services/ci/expire_pipeline_cache_service.rb | 62 ++++++++++++++++++++++ app/workers/expire_pipeline_cache_worker.rb | 51 +----------------- .../unreleased/sh-fix-pipeline-delete-caching.yml | 5 ++ .../ci/expire_pipeline_cache_service_spec.rb | 61 +++++++++++++++++++++ spec/workers/expire_pipeline_cache_worker_spec.rb | 29 ++-------- 6 files changed, 134 insertions(+), 78 deletions(-) create mode 100644 app/services/ci/expire_pipeline_cache_service.rb create mode 100644 changelogs/unreleased/sh-fix-pipeline-delete-caching.yml create mode 100644 spec/services/ci/expire_pipeline_cache_service_spec.rb diff --git a/app/services/ci/destroy_pipeline_service.rb b/app/services/ci/destroy_pipeline_service.rb index 2292ec42b16..9aea20c45f7 100644 --- a/app/services/ci/destroy_pipeline_service.rb +++ b/app/services/ci/destroy_pipeline_service.rb @@ -5,9 +5,9 @@ module Ci def execute(pipeline) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :destroy_pipeline, pipeline) - pipeline.destroy! + Ci::ExpirePipelineCacheService.new.execute(pipeline, delete: true) - Gitlab::Cache::Ci::ProjectPipelineStatus.new(pipeline.project).delete_from_cache + pipeline.destroy! end end end diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb new file mode 100644 index 00000000000..d8d38128af6 --- /dev/null +++ b/app/services/ci/expire_pipeline_cache_service.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module Ci + class ExpirePipelineCacheService + def execute(pipeline, delete: false) + store = Gitlab::EtagCaching::Store.new + + update_etag_cache(pipeline, store) + + if delete + Gitlab::Cache::Ci::ProjectPipelineStatus.new(pipeline.project).delete_from_cache + else + Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline) + end + end + + private + + def project_pipelines_path(project) + Gitlab::Routing.url_helpers.project_pipelines_path(project, format: :json) + end + + def project_pipeline_path(project, pipeline) + Gitlab::Routing.url_helpers.project_pipeline_path(project, pipeline, format: :json) + end + + def commit_pipelines_path(project, commit) + Gitlab::Routing.url_helpers.pipelines_project_commit_path(project, commit.id, format: :json) + end + + def new_merge_request_pipelines_path(project) + Gitlab::Routing.url_helpers.project_new_merge_request_path(project, format: :json) + end + + def each_pipelines_merge_request_path(pipeline) + pipeline.all_merge_requests.each do |merge_request| + path = Gitlab::Routing.url_helpers.pipelines_project_merge_request_path(merge_request.target_project, merge_request, format: :json) + + yield(path) + end + end + + # Updates ETag caches of a pipeline. + # + # This logic resides in a separate method so that EE can more easily extend + # it. + # + # @param [Ci::Pipeline] pipeline + # @param [Gitlab::EtagCaching::Store] store + def update_etag_cache(pipeline, store) + project = pipeline.project + + store.touch(project_pipelines_path(project)) + store.touch(project_pipeline_path(project, pipeline)) + store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil? + store.touch(new_merge_request_pipelines_path(project)) + each_pipelines_merge_request_path(pipeline) do |path| + store.touch(path) + end + end + end +end diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 2c070d482a1..78e68d7bf46 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -11,56 +11,7 @@ class ExpirePipelineCacheWorker pipeline = Ci::Pipeline.find_by(id: pipeline_id) return unless pipeline - store = Gitlab::EtagCaching::Store.new - - update_etag_cache(pipeline, store) - - Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline) + Ci::ExpirePipelineCacheService.new.execute(pipeline) end # rubocop: enable CodeReuse/ActiveRecord - - private - - def project_pipelines_path(project) - Gitlab::Routing.url_helpers.project_pipelines_path(project, format: :json) - end - - def project_pipeline_path(project, pipeline) - Gitlab::Routing.url_helpers.project_pipeline_path(project, pipeline, format: :json) - end - - def commit_pipelines_path(project, commit) - Gitlab::Routing.url_helpers.pipelines_project_commit_path(project, commit.id, format: :json) - end - - def new_merge_request_pipelines_path(project) - Gitlab::Routing.url_helpers.project_new_merge_request_path(project, format: :json) - end - - def each_pipelines_merge_request_path(pipeline) - pipeline.all_merge_requests.each do |merge_request| - path = Gitlab::Routing.url_helpers.pipelines_project_merge_request_path(merge_request.target_project, merge_request, format: :json) - - yield(path) - end - end - - # Updates ETag caches of a pipeline. - # - # This logic resides in a separate method so that EE can more easily extend - # it. - # - # @param [Ci::Pipeline] pipeline - # @param [Gitlab::EtagCaching::Store] store - def update_etag_cache(pipeline, store) - project = pipeline.project - - store.touch(project_pipelines_path(project)) - store.touch(project_pipeline_path(project, pipeline)) - store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil? - store.touch(new_merge_request_pipelines_path(project)) - each_pipelines_merge_request_path(pipeline) do |path| - store.touch(path) - end - end end diff --git a/changelogs/unreleased/sh-fix-pipeline-delete-caching.yml b/changelogs/unreleased/sh-fix-pipeline-delete-caching.yml new file mode 100644 index 00000000000..98846ea9825 --- /dev/null +++ b/changelogs/unreleased/sh-fix-pipeline-delete-caching.yml @@ -0,0 +1,5 @@ +--- +title: Properly expire all pipeline caches when pipeline is deleted +merge_request: 27334 +author: +type: fixed diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/services/ci/expire_pipeline_cache_service_spec.rb new file mode 100644 index 00000000000..ff2d286465a --- /dev/null +++ b/spec/services/ci/expire_pipeline_cache_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::ExpirePipelineCacheService do + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:pipeline) { create(:ci_pipeline, project: project) } + subject { described_class.new } + + describe '#execute' 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" + pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json" + + 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) + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipeline_path) + + subject.execute(pipeline) + end + + it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do + 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" + + 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.execute(pipeline) + 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) + end + + context 'destroyed pipeline' do + let(:project_with_repo) { create(:project, :repository) } + let!(:pipeline_with_commit) { create(:ci_pipeline, :success, project: project_with_repo, sha: project_with_repo.commit.id) } + + it 'clears the cache', :use_clean_rails_memory_store_caching do + create(:commit_status, :success, pipeline: pipeline_with_commit, ref: pipeline_with_commit.ref) + + # Sanity check + expect(project_with_repo.pipeline_status.has_status?).to be_truthy + + subject.execute(pipeline_with_commit, delete: true) + + pipeline_with_commit.destroy! + + # Need to use find to avoid memoization + expect(Project.find(project_with_repo.id).pipeline_status.has_status?).to be_falsey + end + end + end +end diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb index c98b69ef76c..5652f5e8685 100644 --- a/spec/workers/expire_pipeline_cache_worker_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -9,40 +9,17 @@ describe ExpirePipelineCacheWorker do subject { described_class.new } describe '#perform' 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" - pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json" - - 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) - expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipeline_path) - - subject.perform(pipeline.id) - end - - it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do - 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" - - 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) + it 'executes the service' do + expect_any_instance_of(Ci::ExpirePipelineCacheService).to receive(:execute).with(pipeline).and_call_original subject.perform(pipeline.id) end it "doesn't do anything if the pipeline not exist" do + expect_any_instance_of(Ci::ExpirePipelineCacheService).not_to receive(:execute) 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) - - subject.perform(pipeline.id) - end end end -- cgit v1.2.1