Merge branch 'bvl-fix-project-ci-status-cache' into 'master'

Fix invalidating Project build status cache to often

See merge request !10313

13 files changed, 155 insertions, 123 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 37a81fa7781..445247f1b41 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -31,7 +31,6 @@ module Ci
     validate :valid_commit_sha, unless: :importing?
 
     after_create :keep_around_commits, unless: :importing?
-    after_create :refresh_build_status_cache
 
     state_machine :status, initial: :created do
       event :enqueue do
@@ -351,7 +350,6 @@ module Ci
         when 'manual' then block
         end
       end
-      refresh_build_status_cache
     end
 
     def predefined_variables
@@ -393,10 +391,6 @@ module Ci
         .fabricate!
     end
 
-    def refresh_build_status_cache
-      Ci::PipelineStatus.new(project, sha: sha, status: status).store_in_cache_if_needed
-    end
-
     private
 
     def pipeline_data
diff --git a/app/models/ci/pipeline_status.rb b/app/models/ci/pipeline_status.rb
deleted file mode 100644
index 048047d0e34..00000000000
--- a/app/models/ci/pipeline_status.rb
+++ /dev/null
@@ -1,86 +0,0 @@
-# This class is not backed by a table in the main database.
-# It loads the latest Pipeline for the HEAD of a repository, and caches that
-# in Redis.
-module Ci
-  class PipelineStatus
-    attr_accessor :sha, :status, :project, :loaded
-
-    delegate :commit, to: :project
-
-    def self.load_for_project(project)
-      new(project).tap do |status|
-        status.load_status
-      end
-    end
-
-    def initialize(project, sha: nil, status: nil)
-      @project = project
-      @sha = sha
-      @status = status
-    end
-
-    def has_status?
-      loaded? && sha.present? && status.present?
-    end
-
-    def load_status
-      return if loaded?
-
-      if has_cache?
-        load_from_cache
-      else
-        load_from_commit
-        store_in_cache
-      end
-
-      self.loaded = true
-    end
-
-    def load_from_commit
-      return unless commit
-
-      self.sha = commit.sha
-      self.status = commit.status
-    end
-
-    # We only cache the status for the HEAD commit of a project
-    # This status is rendered in project lists
-    def store_in_cache_if_needed
-      return unless sha
-      return delete_from_cache unless commit
-      store_in_cache if commit.sha == self.sha
-    end
-
-    def load_from_cache
-      Gitlab::Redis.with do |redis|
-        self.sha, self.status = redis.hmget(cache_key, :sha, :status)
-      end
-    end
-
-    def store_in_cache
-      Gitlab::Redis.with do |redis|
-        redis.mapped_hmset(cache_key, { sha: sha, status: status })
-      end
-    end
-
-    def delete_from_cache
-      Gitlab::Redis.with do |redis|
-        redis.del(cache_key)
-      end
-    end
-
-    def has_cache?
-      Gitlab::Redis.with do |redis|
-        redis.exists(cache_key)
-      end
-    end
-
-    def loaded?
-      self.loaded
-    end
-
-    def cache_key
-      "projects/#{project.id}/build_status"
-    end
-  end
-end
diff --git a/app/models/project.rb b/app/models/project.rb
index 778a9fb2af6..a160efba912 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1182,7 +1182,7 @@ class Project < ActiveRecord::Base
   end
 
   def pipeline_status
-    @pipeline_status ||= Ci::PipelineStatus.load_for_project(self)
+    @pipeline_status ||= Gitlab::Cache::Ci::ProjectPipelineStatus.load_for_project(self)
   end
 
   def mark_import_as_failed(error_message)
diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb
index c0e4a798f6a..91d9c1d2ba1 100644
--- a/app/services/ci/expire_pipeline_cache_service.rb
+++ b/app/services/ci/expire_pipeline_cache_service.rb
@@ -10,6 +10,8 @@ module Ci
       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
diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb
index 4ee879fe922..73206c3f30d 100644
--- a/features/steps/shared/project.rb
+++ b/features/steps/shared/project.rb
@@ -251,7 +251,8 @@ module SharedProject
 
   step 'project "Shop" has CI build' do
     project = Project.find_by(name: "Shop")
-    create :ci_pipeline, project: project, sha: project.commit.sha, ref: 'master', status: 'skipped'
+    pipeline = create :ci_pipeline, project: project, sha: project.commit.sha, ref: 'master'
+    pipeline.skip
   end
 
   step 'I should see last commit with CI status' do
diff --git a/lib/gitlab/cache/ci/project_pipeline_status.rb b/lib/gitlab/cache/ci/project_pipeline_status.rb
new file mode 100644
index 00000000000..b358f2efa4f
--- /dev/null
+++ b/lib/gitlab/cache/ci/project_pipeline_status.rb
@@ -0,0 +1,103 @@
+# This class is not backed by a table in the main database.
+# It loads the latest Pipeline for the HEAD of a repository, and caches that
+# in Redis.
+module Gitlab
+  module Cache
+    module Ci
+      class ProjectPipelineStatus
+        attr_accessor :sha, :status, :ref, :project, :loaded
+
+        delegate :commit, to: :project
+
+        def self.load_for_project(project)
+          new(project).tap do |status|
+            status.load_status
+          end
+        end
+
+        def self.update_for_pipeline(pipeline)
+          new(pipeline.project,
+              sha: pipeline.sha,
+              status: pipeline.status,
+              ref: pipeline.ref).store_in_cache_if_needed
+        end
+
+        def initialize(project, sha: nil, status: nil, ref: nil)
+          @project = project
+          @sha = sha
+          @ref = ref
+          @status = status
+        end
+
+        def has_status?
+          loaded? && sha.present? && status.present?
+        end
+
+        def load_status
+          return if loaded?
+
+          if has_cache?
+            load_from_cache
+          else
+            load_from_project
+            store_in_cache
+          end
+
+          self.loaded = true
+        end
+
+        def load_from_project
+          return unless commit
+
+          self.sha = commit.sha
+          self.status = commit.status
+          self.ref = project.default_branch
+        end
+
+        # We only cache the status for the HEAD commit of a project
+        # This status is rendered in project lists
+        def store_in_cache_if_needed
+          return delete_from_cache unless commit
+          return unless sha
+          return unless ref
+
+          if commit.sha == sha && project.default_branch == ref
+            store_in_cache
+          end
+        end
+
+        def load_from_cache
+          Gitlab::Redis.with do |redis|
+            self.sha, self.status, self.ref = redis.hmget(cache_key, :sha, :status, :ref)
+          end
+        end
+
+        def store_in_cache
+          Gitlab::Redis.with do |redis|
+            redis.mapped_hmset(cache_key, { sha: sha, status: status, ref: ref })
+          end
+        end
+
+        def delete_from_cache
+          Gitlab::Redis.with do |redis|
+            redis.del(cache_key)
+          end
+        end
+
+        def has_cache?
+          Gitlab::Redis.with do |redis|
+            redis.exists(cache_key)
+          end
+        end
+
+        def loaded?
+          self.loaded
+        end
+
+        def cache_key
+          "projects/#{project.id}/build_status"
+        end
+      end
+    end
+  end
+end
diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb
index c4e58d14f75..f1789fc9d43 100644
--- a/spec/features/dashboard/projects_spec.rb
+++ b/spec/features/dashboard/projects_spec.rb
@@ -7,7 +7,6 @@ RSpec.describe 'Dashboard Projects', feature: true do
   before do
     project.team << [user, :developer]
     login_as user
-    visit dashboard_projects_path
   end
 
   it 'shows the project the user in a member of in the list' do
@@ -15,15 +14,19 @@ RSpec.describe 'Dashboard Projects', feature: true do
     expect(page).to have_content('awesome stuff')
   end
 
-  describe "with a pipeline" do
-    let(:pipeline) {  create(:ci_pipeline, :success, project: project, sha: project.commit.sha) }
+  describe "with a pipeline", redis: true do
+    let!(:pipeline) {  create(:ci_pipeline, project: project, sha: project.commit.sha) }
 
     before do
-      pipeline
+      # Since the cache isn't updated when a new pipeline is created
+      # we need the pipeline to advance in the pipeline since the cache was created
+      # by visiting the login page.
+      pipeline.succeed
     end
 
     it 'shows that the last pipeline passed' do
       visit dashboard_projects_path
+
       expect(page).to have_xpath("//a[@href='#{pipelines_namespace_project_commit_path(project.namespace, project, project.commit)}']")
     end
   end
diff --git a/spec/helpers/ci_status_helper_spec.rb b/spec/helpers/ci_status_helper_spec.rb
index 174cc84a97b..c795fe5a2a3 100644
--- a/spec/helpers/ci_status_helper_spec.rb
+++ b/spec/helpers/ci_status_helper_spec.rb
@@ -19,7 +19,11 @@ describe CiStatusHelper do
 
   describe "#pipeline_status_cache_key" do
     it "builds a cache key for pipeline status" do
-      pipeline_status = Ci::PipelineStatus.new(build(:project), sha: "123abc", status: "success")
+      pipeline_status = Gitlab::Cache::Ci::ProjectPipelineStatus.new(
+        build(:project),
+        sha: "123abc",
+        status: "success"
+      )
       expect(helper.pipeline_status_cache_key(pipeline_status)).to eq("pipeline-status/123abc-success")
     end
   end
diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index 44312ada438..40efab6e4f7 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -63,7 +63,7 @@ describe ProjectsHelper do
     end
   end
 
-  describe "#project_list_cache_key" do
+  describe "#project_list_cache_key", redis: true do
     let(:project) { create(:project) }
 
     it "includes the namespace" do
diff --git a/spec/models/ci/pipeline_status_spec.rb b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb
index bc5b71666c2..fced253dd01 100644
--- a/spec/models/ci/pipeline_status_spec.rb
+++ b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Ci::PipelineStatus do
+describe Gitlab::Cache::Ci::ProjectPipelineStatus do
   let(:project) { create(:project) }
   let(:pipeline_status) { described_class.new(project) }
 
@@ -12,6 +12,20 @@ describe Ci::PipelineStatus do
     end
   end
 
+  describe '.update_for_pipeline' do
+    it 'refreshes the cache if nescessary' do
+      pipeline = build_stubbed(:ci_pipeline, sha: '123456', status: 'success')
+      fake_status = double
+      expect(described_class).to receive(:new).
+                                   with(pipeline.project, sha: '123456', status: 'success', ref: 'master').
+                                   and_return(fake_status)
+
+      expect(fake_status).to receive(:store_in_cache_if_needed)
+
+      described_class.update_for_pipeline(pipeline)
+    end
+  end
+
   describe '#has_status?' do
     it "is false when the status wasn't loaded yet" do
       expect(pipeline_status.has_status?).to be_falsy
@@ -41,14 +55,14 @@ describe Ci::PipelineStatus do
     it 'loads the status from the project commit when there is no cache' do
       allow(pipeline_status).to receive(:has_cache?).and_return(false)
 
-      expect(pipeline_status).to receive(:load_from_commit)
+      expect(pipeline_status).to receive(:load_from_project)
 
       pipeline_status.load_status
     end
 
     it 'stores the status in the cache when it loading it from the project' do
       allow(pipeline_status).to receive(:has_cache?).and_return(false)
-      allow(pipeline_status).to receive(:load_from_commit)
+      allow(pipeline_status).to receive(:load_from_project)
 
       expect(pipeline_status).to receive(:store_in_cache)
 
@@ -70,14 +84,15 @@ describe Ci::PipelineStatus do
     end
   end
 
-  describe "#load_from_commit" do
+  describe "#load_from_project" do
     let!(:pipeline) { create(:ci_pipeline, :success, project: project, sha: project.commit.sha) }
 
     it 'reads the status from the pipeline for the commit' do
-      pipeline_status.load_from_commit
+      pipeline_status.load_from_project
 
       expect(pipeline_status.status).to eq('success')
       expect(pipeline_status.sha).to eq(project.commit.sha)
+      expect(pipeline_status.ref).to eq(project.default_branch)
     end
 
     it "doesn't fail for an empty project" do
@@ -108,10 +123,11 @@ describe Ci::PipelineStatus do
       build_status = described_class.load_for_project(project)
 
       build_status.store_in_cache_if_needed
-      sha, status = Gitlab::Redis.with { |redis| redis.hmget("projects/#{project.id}/build_status", :sha, :status) }
+      sha, status, ref = Gitlab::Redis.with { |redis| redis.hmget("projects/#{project.id}/build_status", :sha, :status, :ref) }
 
       expect(sha).not_to be_nil
       expect(status).not_to be_nil
+      expect(ref).not_to be_nil
     end
 
     it "doesn't store the status in redis when the sha is not the head of the project" do
@@ -126,14 +142,15 @@ describe Ci::PipelineStatus do
 
     it "deletes the cache if the repository doesn't have a head commit" do
       empty_project = create(:empty_project)
-      Gitlab::Redis.with { |redis| redis.mapped_hmset("projects/#{empty_project.id}/build_status", { sha: "sha", status: "pending" }) }
+      Gitlab::Redis.with { |redis| redis.mapped_hmset("projects/#{empty_project.id}/build_status", { sha: "sha", status: "pending", ref: 'master' }) }
       other_status = described_class.new(empty_project, sha: "123456", status: "failed")
 
       other_status.store_in_cache_if_needed
-      sha, status = Gitlab::Redis.with { |redis| redis.hmget("projects/#{empty_project.id}/build_status", :sha, :status) }
+      sha, status, ref = Gitlab::Redis.with { |redis| redis.hmget("projects/#{empty_project.id}/build_status", :sha, :status, :ref) }
 
       expect(sha).to be_nil
       expect(status).to be_nil
+      expect(ref).to be_nil
     end
   end
 
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 32aa2f4b336..d7d6a75d38d 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -375,7 +375,7 @@ describe Ci::Pipeline, models: true do
       end
     end
 
-    describe 'pipeline ETag caching' do
+    describe 'pipeline caching' do
       it 'executes ExpirePipelinesCacheService' do
         expect_any_instance_of(Ci::ExpirePipelineCacheService).to receive(:execute).with(pipeline)
 
@@ -1079,19 +1079,6 @@ describe Ci::Pipeline, models: true do
     end
   end
 
-  describe '#update_status' do
-    let(:pipeline) { create(:ci_pipeline, sha: '123456') }
-
-    it 'updates the cached status' do
-      fake_status = double
-      # after updating the status, the status is set to `skipped` for this pipeline's builds
-      expect(Ci::PipelineStatus).to receive(:new).with(pipeline.project, sha: '123456', status: 'skipped').and_return(fake_status)
-      expect(fake_status).to receive(:store_in_cache_if_needed)
-
-      pipeline.update_status
-    end
-  end
-
   describe 'notifications when pipeline success or failed' do
     let(:project) { create(:project, :repository) }
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d485ee104fd..92d420337f9 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1874,7 +1874,7 @@ describe Project, models: true do
   describe '#pipeline_status' do
     let(:project) { create(:project) }
     it 'builds a pipeline status' do
-      expect(project.pipeline_status).to be_a(Ci::PipelineStatus)
+      expect(project.pipeline_status).to be_a(Gitlab::Cache::Ci::ProjectPipelineStatus)
     end
 
     it 'hase a loaded pipeline status' do
diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/services/ci/expire_pipeline_cache_service_spec.rb
index 3c735872c30..166c6dfc93e 100644
--- a/spec/services/ci/expire_pipeline_cache_service_spec.rb
+++ b/spec/services/ci/expire_pipeline_cache_service_spec.rb
@@ -16,5 +16,12 @@ describe Ci::ExpirePipelineCacheService, services: true do
 
       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
   end
 end