Merge branch 'project-cache-worker-lease' into 'master'

Restrict ProjectCacheWorker jobs to one per 15 min

## What does this MR do?

This restricts performing ProjectCacheWorker jobs to once every 15 minutes per project. The goal of this is to reduce disk load in the event of a lot of pushes happening in a short period of time. 

The impact is that cached data (e.g. commit count) may be updated less frequently. Furthermore it could mean that separate push payloads won't lead to e.g. READMEs being updated. Most of the cached data isn't updated very frequently, so this trade off should be worth it considering the alternative is a burning storage layer. 

In the future we'll have to flush caches in a smarter way. For example, refreshing the README cache could probably be done separately from the other caches with its own lease and such. See https://gitlab.com/gitlab-org/gitlab-ce/issues/23550 for more information.

See merge request !7017

3 files changed, 57 insertions, 11 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 670404e4fce..98cffab8c03 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,7 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 ## 8.14.0 (2016-11-22)
   - Adds user project membership expired event to clarify why user was removed (Callum Dryden)
-  - Simpler arguments passed to named_route on toggle_award_url helper method 
+  - Simpler arguments passed to named_route on toggle_award_url helper method
 
 ## 8.13.0 (2016-10-22)
 
@@ -36,6 +36,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - AbstractReferenceFilter caches project_refs on RequestStore when active
   - Replaced the check sign to arrow in the show build view. !6501
   - Add a /wip slash command to toggle the Work In Progress status of a merge request. !6259 (tbalthazar)
+  - ProjectCacheWorker updates caches at most once per 15 minutes per project
   - Fix Error 500 when viewing old merge requests with bad diff data
   - Create a new /templates namespace for the /licenses, /gitignores and /gitlab_ci_ymls API endpoints. !5717 (tbalthazar)
   - Speed-up group milestones show page
diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb
index ccefd0f71a0..0d524e88dc3 100644
--- a/app/workers/project_cache_worker.rb
+++ b/app/workers/project_cache_worker.rb
@@ -1,9 +1,30 @@
+# Worker for updating any project specific caches.
+#
+# This worker runs at most once every 15 minutes per project. This is to ensure
+# that multiple instances of jobs for this worker don't hammer the underlying
+# storage engine as much.
 class ProjectCacheWorker
   include Sidekiq::Worker
 
   sidekiq_options queue: :default
 
+  LEASE_TIMEOUT = 15.minutes.to_i
+
   def perform(project_id)
+    if try_obtain_lease_for(project_id)
+      Rails.logger.
+        info("Obtained ProjectCacheWorker lease for project #{project_id}")
+    else
+      Rails.logger.
+        info("Could not obtain ProjectCacheWorker lease for project #{project_id}")
+
+      return
+    end
+
+    update_caches(project_id)
+  end
+
+  def update_caches(project_id)
     project = Project.find(project_id)
 
     return unless project.repository.exists?
@@ -15,4 +36,10 @@ class ProjectCacheWorker
       project.repository.build_cache
     end
   end
+
+  def try_obtain_lease_for(project_id)
+    Gitlab::ExclusiveLease.
+      new("project_cache_worker:#{project_id}", timeout: LEASE_TIMEOUT).
+      try_obtain
+  end
 end
diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb
index 5785a6a06ff..f5b60b90d11 100644
--- a/spec/workers/project_cache_worker_spec.rb
+++ b/spec/workers/project_cache_worker_spec.rb
@@ -6,21 +6,39 @@ describe ProjectCacheWorker do
   subject { described_class.new }
 
   describe '#perform' do
-    it 'updates project cache data' do
-      expect_any_instance_of(Repository).to receive(:size)
-      expect_any_instance_of(Repository).to receive(:commit_count)
+    context 'when an exclusive lease can be obtained' do
+      before do
+        allow(subject).to receive(:try_obtain_lease_for).with(project.id).
+          and_return(true)
+      end
 
-      expect_any_instance_of(Project).to receive(:update_repository_size)
-      expect_any_instance_of(Project).to receive(:update_commit_count)
+      it 'updates project cache data' do
+        expect_any_instance_of(Repository).to receive(:size)
+        expect_any_instance_of(Repository).to receive(:commit_count)
 
-      subject.perform(project.id)
+        expect_any_instance_of(Project).to receive(:update_repository_size)
+        expect_any_instance_of(Project).to receive(:update_commit_count)
+
+        subject.perform(project.id)
+      end
+
+      it 'handles missing repository data' do
+        expect_any_instance_of(Repository).to receive(:exists?).and_return(false)
+        expect_any_instance_of(Repository).not_to receive(:size)
+
+        subject.perform(project.id)
+      end
     end
 
-    it 'handles missing repository data' do
-      expect_any_instance_of(Repository).to receive(:exists?).and_return(false)
-      expect_any_instance_of(Repository).not_to receive(:size)
+    context 'when an exclusive lease can not be obtained' do
+      it 'does nothing' do
+        allow(subject).to receive(:try_obtain_lease_for).with(project.id).
+          and_return(false)
+
+        expect(subject).not_to receive(:update_caches)
 
-      subject.perform(project.id)
+        subject.perform(project.id)
+      end
     end
   end
 end