kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Properly expire all pipeline caches when pipeline is deleted

Browse source 
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
This commit is contained in:
Stan Hu 2019-04-12 12:29:47 -07:00
parent 0a99e0220d
commit 1625979653
6 changed files with 134 additions and 78 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/services/ci/destroy_pipeline_service.rb
View file

@ -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

62
app/services/ci/expire_pipeline_cache_service.rb Normal file
View file

@ -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

51
app/workers/expire_pipeline_cache_worker.rb
View file

@ -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

5
changelogs/unreleased/sh-fix-pipeline-delete-caching.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Properly expire all pipeline caches when pipeline is deleted
merge_request: 27334
author:
type: fixed

61
spec/services/ci/expire_pipeline_cache_service_spec.rb Normal file
View file

@ -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

29
spec/workers/expire_pipeline_cache_worker_spec.rb
View file

@ -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