Reduce Redis roundtrips when expiring etag cache

Adds support for multiple paths to Gitlab::EtagCaching::Store.touch
and uses this method in ExpirePipelineCacheService

Also removes N+1 queries with upstream and downstream pipelines

app/services/ci/expire_pipeline_cache_service.rb

@@ -74,20 +74,25 @@ module Ci
     def update_etag_cache(pipeline, store)
       project = pipeline.project
 
-      store.touch(project_pipelines_path(project))
-      store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil?
-      store.touch(new_merge_request_pipelines_path(project))
+      etag_paths = [
+        project_pipelines_path(project),
+        new_merge_request_pipelines_path(project),
+        graphql_project_on_demand_scan_counts_path(project)
+      ]
+
+      etag_paths << commit_pipelines_path(project, pipeline.commit) unless pipeline.commit.nil?
+
       each_pipelines_merge_request_path(pipeline) do |path|
-        store.touch(path)
+        etag_paths << path
       end
 
-      pipeline.self_with_upstreams_and_downstreams.each do |relative_pipeline|
-        store.touch(project_pipeline_path(relative_pipeline.project, relative_pipeline))
-        store.touch(graphql_pipeline_path(relative_pipeline))
-        store.touch(graphql_pipeline_sha_path(relative_pipeline.sha))
+      pipeline.self_with_upstreams_and_downstreams.includes(project: [:route, { namespace: :route }]).each do |relative_pipeline| # rubocop: disable CodeReuse/ActiveRecord
+        etag_paths << project_pipeline_path(relative_pipeline.project, relative_pipeline)
+        etag_paths << graphql_pipeline_path(relative_pipeline)
+        etag_paths << graphql_pipeline_sha_path(relative_pipeline.sha)
       end
 
-      store.touch(graphql_project_on_demand_scan_counts_path(project))
+      store.touch(*etag_paths)
     end
 
     def url_helpers

lib/gitlab/etag_caching/store.rb

@@ -12,14 +12,18 @@ module Gitlab
         Gitlab::Redis::SharedState.with { |redis| redis.get(redis_shared_state_key(key)) }
       end
 
-      def touch(key, only_if_missing: false)
-        etag = generate_etag
+      def touch(*keys, only_if_missing: false)
+        etags = keys.map { generate_etag }
 
         Gitlab::Redis::SharedState.with do |redis|
-          redis.set(redis_shared_state_key(key), etag, ex: EXPIRY_TIME, nx: only_if_missing)
+          redis.pipelined do
+            keys.each_with_index do |key, i|
+              redis.set(redis_shared_state_key(key), etags[i], ex: EXPIRY_TIME, nx: only_if_missing)
+            end
+          end
         end
 
-        etag
+        keys.size > 1 ? etags : etags.first
       end
 
       private

spec/lib/gitlab/etag_caching/store_spec.rb

@@ -80,5 +80,19 @@ RSpec.describe Gitlab::EtagCaching::Store, :clean_gitlab_redis_shared_state do
         expect(store.get(key)).to eq(etag)
       end
     end
+
+    context 'with multiple keys' do
+      let(:keys) { ['/my-group/my-project/builds/234.json', '/api/graphql:pipelines/id/5'] }
+
+      it 'stores and returns multiple values' do
+        etags = store.touch(*keys)
+
+        expect(etags.size).to eq(keys.size)
+
+        keys.each_with_index do |key, i|
+          expect(store.get(key)).to eq(etags[i])
+        end
+      end
+    end
   end
 end

spec/services/ci/expire_pipeline_cache_service_spec.rb

@@ -18,14 +18,14 @@ RSpec.describe Ci::ExpirePipelineCacheService do
       graphql_pipeline_sha_path = "/api/graphql:pipelines/sha/#{pipeline.sha}"
       graphql_project_on_demand_scan_counts_path = "/api/graphql:on_demand_scan/counts/#{project.full_path}"
 
-      expect_next_instance_of(Gitlab::EtagCaching::Store) do |store|
-        expect(store).to receive(:touch).with(pipelines_path)
-        expect(store).to receive(:touch).with(new_mr_pipelines_path)
-        expect(store).to receive(:touch).with(pipeline_path)
-        expect(store).to receive(:touch).with(graphql_pipeline_path)
-        expect(store).to receive(:touch).with(graphql_pipeline_sha_path)
-        expect(store).to receive(:touch).with(graphql_project_on_demand_scan_counts_path)
-      end
+      expect_touched_etag_caching_paths(
+        pipelines_path,
+        new_mr_pipelines_path,
+        pipeline_path,
+        graphql_pipeline_path,
+        graphql_pipeline_sha_path,
+        graphql_project_on_demand_scan_counts_path
+      )
 
       subject.execute(pipeline)
     end
@@ -37,9 +37,10 @@ RSpec.describe Ci::ExpirePipelineCacheService do
       merge_request_pipelines_path = "/#{project.full_path}/-/merge_requests/#{merge_request.iid}/pipelines.json"
       merge_request_widget_path = "/#{project.full_path}/-/merge_requests/#{merge_request.iid}/cached_widget.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)
-      expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_widget_path)
+      expect_touched_etag_caching_paths(
+        merge_request_pipelines_path,
+        merge_request_widget_path
+      )
 
       subject.execute(merge_request.all_pipelines.last)
     end
@@ -78,10 +79,7 @@ RSpec.describe Ci::ExpirePipelineCacheService do
       it 'updates the cache of dependent pipeline' do
         dependent_pipeline_path = "/#{source.source_project.full_path}/-/pipelines/#{source.source_pipeline.id}.json"
 
-        expect_next_instance_of(Gitlab::EtagCaching::Store) do |store|
-          allow(store).to receive(:touch)
-          expect(store).to receive(:touch).with(dependent_pipeline_path)
-        end
+        expect_touched_etag_caching_paths(dependent_pipeline_path)
 
         subject.execute(pipeline)
       end
@@ -94,13 +92,31 @@ RSpec.describe Ci::ExpirePipelineCacheService do
       it 'updates the cache of dependent pipeline' do
         dependent_pipeline_path = "/#{source.project.full_path}/-/pipelines/#{source.pipeline.id}.json"
 
-        expect_next_instance_of(Gitlab::EtagCaching::Store) do |store|
-          allow(store).to receive(:touch)
-          expect(store).to receive(:touch).with(dependent_pipeline_path)
-        end
+        expect_touched_etag_caching_paths(dependent_pipeline_path)
 
         subject.execute(pipeline)
       end
     end
+
+    it 'does not do N+1 queries' do
+      subject.execute(pipeline)
+
+      control = ActiveRecord::QueryRecorder.new { subject.execute(pipeline) }
+
+      create(:ci_sources_pipeline, pipeline: pipeline)
+      create(:ci_sources_pipeline, source_job: create(:ci_build, pipeline: pipeline))
+
+      expect { subject.execute(pipeline) }.not_to exceed_query_limit(control.count)
+    end
+  end
+
+  def expect_touched_etag_caching_paths(*paths)
+    expect_next_instance_of(Gitlab::EtagCaching::Store) do |store|
+      expect(store).to receive(:touch).and_wrap_original do |m, *args|
+        expect(args).to include(*paths)
+
+        m.call(*args)
+      end
+    end
   end
 end