diff options
| -rw-r--r-- | app/controllers/dashboard/projects_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/explore/projects_controller.rb | 14 | ||||
| -rw-r--r-- | app/helpers/ci_status_helper.rb | 18 | ||||
| -rw-r--r-- | app/helpers/projects_helper.rb | 7 | ||||
| -rw-r--r-- | app/models/ci/pipeline.rb | 6 | ||||
| -rw-r--r-- | app/models/ci/pipeline_status.rb | 86 | ||||
| -rw-r--r-- | app/models/project.rb | 4 | ||||
| -rw-r--r-- | app/views/shared/projects/_project.html.haml | 7 | ||||
| -rw-r--r-- | changelogs/unreleased/27376-cache-default-branch-pipeline-on-project.yml | 4 | ||||
| -rw-r--r-- | spec/features/dashboard/projects_spec.rb | 26 | ||||
| -rw-r--r-- | spec/helpers/ci_status_helper_spec.rb | 7 | ||||
| -rw-r--r-- | spec/helpers/projects_helper_spec.rb | 40 | ||||
| -rw-r--r-- | spec/models/ci/pipeline_spec.rb | 13 | ||||
| -rw-r--r-- | spec/models/ci/pipeline_status_spec.rb | 173 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 11 |
15 files changed, 408 insertions, 10 deletions
diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 325ae565537..be00d765f73 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -42,7 +42,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController private def load_projects(base_scope) - projects = base_scope.sorted_by_activity.includes(:namespace) + projects = base_scope.sorted_by_activity.includes(:route, namespace: :route) filter_projects(projects) end diff --git a/app/controllers/explore/projects_controller.rb b/app/controllers/explore/projects_controller.rb index 26e17a7553e..6167f9bd335 100644 --- a/app/controllers/explore/projects_controller.rb +++ b/app/controllers/explore/projects_controller.rb @@ -2,7 +2,7 @@ class Explore::ProjectsController < Explore::ApplicationController include FilterProjects def index - @projects = ProjectsFinder.new.execute(current_user) + @projects = load_projects @tags = @projects.tags_on(:tags) @projects = @projects.tagged_with(params[:tag]) if params[:tag].present? @projects = @projects.where(visibility_level: params[:visibility_level]) if params[:visibility_level].present? @@ -21,7 +21,8 @@ class Explore::ProjectsController < Explore::ApplicationController end def trending - @projects = filter_projects(Project.trending) + @projects = load_projects(Project.trending) + @projects = filter_projects(@projects) @projects = @projects.sort(@sort = params[:sort]) @projects = @projects.page(params[:page]) @@ -36,7 +37,7 @@ class Explore::ProjectsController < Explore::ApplicationController end def starred - @projects = ProjectsFinder.new.execute(current_user) + @projects = load_projects @projects = filter_projects(@projects) @projects = @projects.reorder('star_count DESC') @projects = @projects.page(params[:page]) @@ -50,4 +51,11 @@ class Explore::ProjectsController < Explore::ApplicationController end end end + + protected + + def load_projects(base_scope = nil) + base_scope ||= ProjectsFinder.new.execute(current_user) + base_scope.includes(:route, namespace: :route) + end end diff --git a/app/helpers/ci_status_helper.rb b/app/helpers/ci_status_helper.rb index a7cdca9ba2e..2de9e0de310 100644 --- a/app/helpers/ci_status_helper.rb +++ b/app/helpers/ci_status_helper.rb @@ -59,6 +59,24 @@ module CiStatusHelper custom_icon(icon_name) end + def pipeline_status_cache_key(pipeline_status) + "pipeline-status/#{pipeline_status.sha}-#{pipeline_status.status}" + end + + def render_project_pipeline_status(pipeline_status, tooltip_placement: 'auto left') + project = pipeline_status.project + path = pipelines_namespace_project_commit_path( + project.namespace, + project, + pipeline_status.sha) + + render_status_with_link( + 'commit', + pipeline_status.status, + path, + tooltip_placement: tooltip_placement) + end + def render_commit_status(commit, ref: nil, tooltip_placement: 'auto left') project = commit.project path = pipelines_namespace_project_commit_path( diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 4befeacc135..bd0c2cd661e 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -159,6 +159,13 @@ module ProjectsHelper choose a GitLab CI Yaml template and commit your changes. #{link_to_autodeploy_doc}".html_safe end + def project_list_cache_key(project) + key = [project.namespace.cache_key, project.cache_key, controller.controller_name, controller.action_name, current_application_settings.cache_key, 'v2.3'] + key << pipeline_status_cache_key(project.pipeline_status) if project.pipeline_status.has_status? + + key + end + private def repo_children_classes(field) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 8a5a9aa4adb..65d08a22b4c 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -22,6 +22,7 @@ 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 @@ -328,6 +329,7 @@ module Ci when 'manual' then block end end + refresh_build_status_cache end def predefined_variables @@ -369,6 +371,10 @@ 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 new file mode 100644 index 00000000000..048047d0e34 --- /dev/null +++ b/app/models/ci/pipeline_status.rb @@ -0,0 +1,86 @@ +# 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 8c2dadf4659..2ffaaac93f3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1209,6 +1209,10 @@ class Project < ActiveRecord::Base end end + def pipeline_status + @pipeline_status ||= Ci::PipelineStatus.load_for_project(self) + end + def mark_import_as_failed(error_message) original_errors = errors.dup sanitized_message = Gitlab::UrlSanitizer.sanitize(error_message) diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 7e9fb7bb4d3..df21857e1ad 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -6,17 +6,16 @@ - css_class = '' unless local_assigns[:css_class] - show_last_commit_as_description = false unless local_assigns[:show_last_commit_as_description] == true && project.commit - css_class += " no-description" if project.description.blank? && !show_last_commit_as_description -- cache_key = [project.namespace, project, controller.controller_name, controller.action_name, current_application_settings, 'v2.3'] -- cache_key.push(project.commit&.sha, project.commit&.status) +- cache_key = project_list_cache_key(project) %li.project-row{ class: css_class } = cache(cache_key) do .controls - if project.archived %span.label.label-warning archived - - if project.commit.try(:status) + - if project.pipeline_status.has_status? %span - = render_commit_status(project.commit) + = render_project_pipeline_status(project.pipeline_status) - if forks %span = icon('code-fork') diff --git a/changelogs/unreleased/27376-cache-default-branch-pipeline-on-project.yml b/changelogs/unreleased/27376-cache-default-branch-pipeline-on-project.yml new file mode 100644 index 00000000000..a116c68ad87 --- /dev/null +++ b/changelogs/unreleased/27376-cache-default-branch-pipeline-on-project.yml @@ -0,0 +1,4 @@ +--- +title: Speed up project dashboard by caching pipeline status and eager loading routes +merge_request: 9903 +author: diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb index 63eb5c697c2..c4e58d14f75 100644 --- a/spec/features/dashboard/projects_spec.rb +++ b/spec/features/dashboard/projects_spec.rb @@ -1,10 +1,32 @@ require 'spec_helper' RSpec.describe 'Dashboard Projects', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project, name: "awesome stuff") } + before do - login_as(create :user) + 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 + visit dashboard_projects_path + 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) } + + before do + pipeline + 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 + it_behaves_like "an autodiscoverable RSS feed with current_user's private token" end diff --git a/spec/helpers/ci_status_helper_spec.rb b/spec/helpers/ci_status_helper_spec.rb index 637b02d9388..174cc84a97b 100644 --- a/spec/helpers/ci_status_helper_spec.rb +++ b/spec/helpers/ci_status_helper_spec.rb @@ -16,4 +16,11 @@ describe CiStatusHelper do helper.ci_icon_for_status(failed_commit.status) end end + + 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") + expect(helper.pipeline_status_cache_key(pipeline_status)).to eq("pipeline-status/123abc-success") + end + end end diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index aca0bb1d794..fc6ad6419ac 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -63,6 +63,46 @@ describe ProjectsHelper do end end + describe "#project_list_cache_key" do + let(:project) { create(:project) } + + it "includes the namespace" do + expect(helper.project_list_cache_key(project)).to include(project.namespace.cache_key) + end + + it "includes the project" do + expect(helper.project_list_cache_key(project)).to include(project.cache_key) + end + + it "includes the controller name" do + expect(helper.controller).to receive(:controller_name).and_return("testcontroller") + + expect(helper.project_list_cache_key(project)).to include("testcontroller") + end + + it "includes the controller action" do + expect(helper.controller).to receive(:action_name).and_return("testaction") + + expect(helper.project_list_cache_key(project)).to include("testaction") + end + + it "includes the application settings" do + settings = Gitlab::CurrentSettings.current_application_settings + + expect(helper.project_list_cache_key(project)).to include(settings.cache_key) + end + + it "includes a version" do + expect(helper.project_list_cache_key(project)).to include("v2.3") + end + + it "includes the pipeline status when there is a status" do + create(:ci_pipeline, :success, project: project, sha: project.commit.sha) + + expect(helper.project_list_cache_key(project)).to include("pipeline-status/#{project.commit.sha}-success") + end + end + describe 'link_to_member' do let(:group) { create(:group) } let(:project) { create(:empty_project, group: group) } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 9962c987110..4a664e4fae2 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1018,6 +1018,19 @@ 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/ci/pipeline_status_spec.rb b/spec/models/ci/pipeline_status_spec.rb new file mode 100644 index 00000000000..bc5b71666c2 --- /dev/null +++ b/spec/models/ci/pipeline_status_spec.rb @@ -0,0 +1,173 @@ +require 'spec_helper' + +describe Ci::PipelineStatus do + let(:project) { create(:project) } + let(:pipeline_status) { described_class.new(project) } + + describe '.load_for_project' do + it "loads the status" do + expect_any_instance_of(described_class).to receive(:load_status) + + described_class.load_for_project(project) + 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 + end + + it 'is true when all status information was loaded' do + fake_commit = double + allow(fake_commit).to receive(:status).and_return('failed') + allow(fake_commit).to receive(:sha).and_return('failed424d1b73bc0d3cb726eb7dc4ce17a4d48552f8c6') + allow(pipeline_status).to receive(:commit).and_return(fake_commit) + allow(pipeline_status).to receive(:has_cache?).and_return(false) + + pipeline_status.load_status + + expect(pipeline_status.has_status?).to be_truthy + end + end + + describe '#load_status' do + it 'loads the status from the cache when there is one' do + expect(pipeline_status).to receive(:has_cache?).and_return(true) + expect(pipeline_status).to receive(:load_from_cache) + + pipeline_status.load_status + end + + 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) + + 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) + + expect(pipeline_status).to receive(:store_in_cache) + + pipeline_status.load_status + end + + it 'sets the state to loaded' do + pipeline_status.load_status + + expect(pipeline_status).to be_loaded + end + + it 'only loads the status once' do + expect(pipeline_status).to receive(:has_cache?).and_return(true).exactly(1) + expect(pipeline_status).to receive(:load_from_cache).exactly(1) + + pipeline_status.load_status + pipeline_status.load_status + end + end + + describe "#load_from_commit" 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 + + expect(pipeline_status.status).to eq('success') + expect(pipeline_status.sha).to eq(project.commit.sha) + end + + it "doesn't fail for an empty project" do + status_for_empty_commit = described_class.new(create(:empty_project)) + + status_for_empty_commit.load_status + + expect(status_for_empty_commit).to be_loaded + end + end + + describe "#store_in_cache", :redis do + it "sets the object in redis" do + pipeline_status.sha = '123456' + pipeline_status.status = 'failed' + + pipeline_status.store_in_cache + read_sha, read_status = Gitlab::Redis.with { |redis| redis.hmget("projects/#{project.id}/build_status", :sha, :status) } + + expect(read_sha).to eq('123456') + expect(read_status).to eq('failed') + end + end + + describe '#store_in_cache_if_needed', :redis do + it 'stores the state in the cache when the sha is the HEAD of the project' do + create(:ci_pipeline, :success, project: project, sha: project.commit.sha) + 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) } + + expect(sha).not_to be_nil + expect(status).not_to be_nil + end + + it "doesn't store the status in redis when the sha is not the head of the project" do + other_status = described_class.new(project, sha: "123456", status: "failed") + + other_status.store_in_cache_if_needed + sha, status = Gitlab::Redis.with { |redis| redis.hmget("projects/#{project.id}/build_status", :sha, :status) } + + expect(sha).to be_nil + expect(status).to be_nil + end + + 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" }) } + 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) } + + expect(sha).to be_nil + expect(status).to be_nil + end + end + + describe "with a status in redis", :redis do + let(:status) { 'success' } + let(:sha) { '424d1b73bc0d3cb726eb7dc4ce17a4d48552f8c6' } + + before do + Gitlab::Redis.with { |redis| redis.mapped_hmset("projects/#{project.id}/build_status", { sha: sha, status: status }) } + end + + describe '#load_from_cache' do + it 'reads the status from redis' do + pipeline_status.load_from_cache + + expect(pipeline_status.sha).to eq(sha) + expect(pipeline_status.status).to eq(status) + end + end + + describe '#has_cache?' do + it 'knows the status is cached' do + expect(pipeline_status.has_cache?).to be_truthy + end + end + + describe '#delete_from_cache' do + it 'deletes values from redis' do + pipeline_status.delete_from_cache + + key_exists = Gitlab::Redis.with { |redis| redis.exists("projects/#{project.id}/build_status") } + + expect(key_exists).to be_falsy + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e120e21af06..ff1defcd32d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1916,4 +1916,15 @@ describe Project, models: true do end end end + + describe '#pipeline_status' do + let(:project) { create(:project) } + it 'builds a pipeline status' do + expect(project.pipeline_status).to be_a(Ci::PipelineStatus) + end + + it 'hase a loaded pipeline status' do + expect(project.pipeline_status).to be_loaded + end + end end |