diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-01-30 18:26:40 -0600 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2017-02-06 16:12:24 -0600 |
commit | c8b63a28afa811881b617546fe94a19378585a04 (patch) | |
tree | 2b1bd2e9d52e059de0589a014f518f23e9b71ca3 | |
parent | 3aa1264dc6c0de3625bb1a2d6a0ee90140a2f519 (diff) | |
download | gitlab-ce-c8b63a28afa811881b617546fe94a19378585a04.tar.gz |
Improve performance of finding last deployed environment
-rw-r--r-- | app/controllers/projects/blob_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/commit_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/compare_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/environments_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 4 | ||||
-rw-r--r-- | app/models/ci/build.rb | 5 | ||||
-rw-r--r-- | app/models/environment.rb | 20 | ||||
-rw-r--r-- | app/models/merge_request.rb | 10 | ||||
-rw-r--r-- | app/models/project.rb | 13 | ||||
-rw-r--r-- | app/models/repository.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/database.rb | 14 | ||||
-rw-r--r-- | spec/lib/gitlab/database_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/environment_spec.rb | 17 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 24 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 52 |
15 files changed, 66 insertions, 119 deletions
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 1bdb8d45984..c88095abb3a 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -31,7 +31,7 @@ class Projects::BlobController < Projects::ApplicationController def show branch_name = @ref if @repository.branch_exists?(@ref) - @environment = @project.latest_environment_for(@commit, ref: branch_name) + @environment = @project.environments_for(commit: @commit, ref: branch_name).last @environment = nil unless can?(current_user, :read_environment, @environment) end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 08817afa1e9..5bcc545462f 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -96,7 +96,7 @@ class Projects::CommitController < Projects::ApplicationController @diffs = commit.diffs(opts) @notes_count = commit.notes.count - @environment = @project.latest_environment_for(@commit) + @environment = @project.environments_for(commit: @commit).last @environment = nil unless can?(current_user, :read_environment, @environment) end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 6c94a79f842..142df1ba4e9 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -58,7 +58,7 @@ class Projects::CompareController < Projects::ApplicationController @diffs = @compare.diffs(diff_options) branch_name = @head_ref if @repository.branch_exists?(@head_ref) - @environment = @project.latest_environment_for(@commit, ref: branch_name) + @environment = @project.environments_for(commit: @commit, ref: branch_name).last @environment = nil unless can?(current_user, :read_environment, @environment) @diff_notes_disabled = true diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb index 87cc36253f1..e2abcd45b5f 100644 --- a/app/controllers/projects/environments_controller.rb +++ b/app/controllers/projects/environments_controller.rb @@ -10,7 +10,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController def index @scope = params[:scope] - @environments = project.environments + @environments = project.environments.includes(:last_deployment) respond_to do |format| format.html diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 6a6d24db35d..a84e15de99a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -103,7 +103,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end end - @environment = @merge_request.latest_environment + @environment = @merge_request.environments.last @environment = nil unless can?(current_user, :read_environment, @environment) respond_to do |format| @@ -248,7 +248,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end @diff_notes_disabled = true - @environment = @merge_request.latest_environment + @environment = @merge_request.environments.last @environment = nil unless can?(current_user, :read_environment, @environment) render json: { html: view_to_html_string('projects/merge_requests/_new_diffs', diffs: @diffs, environment: @environment) } diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 44d4fb9d8d8..39b0f70a5ae 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -9,6 +9,7 @@ module Ci belongs_to :erased_by, class_name: 'User' has_many :deployments, as: :deployable + has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -183,10 +184,6 @@ module Ci success? && !last_deployment.try(:last?) end - def last_deployment - deployments.last - end - def depends_on_builds # Get builds of the same type latest_builds = self.pipeline.builds.latest diff --git a/app/models/environment.rb b/app/models/environment.rb index ed18e6bdea1..14787f79a36 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -6,7 +6,8 @@ class Environment < ActiveRecord::Base belongs_to :project, required: true, validate: true - has_many :deployments + has_many :deployments, dependent: :destroy + has_one :last_deployment, -> { order('deployments.id DESC') }, class_name: 'Deployment' before_validation :nullify_external_url before_validation :generate_slug, if: ->(env) { env.slug.blank? } @@ -37,6 +38,7 @@ class Environment < ActiveRecord::Base scope :available, -> { with_state(:available) } scope :stopped, -> { with_state(:stopped) } + scope :order_by_last_deployed_at, -> { order(Gitlab::Database.nulls_first_order('(SELECT MAX(id) FROM deployments WHERE deployments.environment_id = environments.id)', 'ASC')) } state_machine :state, initial: :available do event :start do @@ -51,14 +53,6 @@ class Environment < ActiveRecord::Base state :stopped end - def self.latest_for_commit(environments, commit) - environments.sort_by do |environment| - deployment = environment.first_deployment_for(commit) - - deployment.try(:created_at) || DateTime.parse('1970-01-01') - end.last - end - def predefined_variables [ { key: 'CI_ENVIRONMENT_NAME', value: name, public: true }, @@ -70,10 +64,6 @@ class Environment < ActiveRecord::Base ref.to_s == last_deployment.try(:ref) end - def last_deployment - deployments.last - end - def nullify_external_url self.external_url = nil if self.external_url.blank? end @@ -95,6 +85,10 @@ class Environment < ActiveRecord::Base last_deployment.includes_commit?(commit) end + def last_deployed_at + last_deployment.try(:created_at) + end + def update_merge_request_metrics? (environment_type || name) == "production" end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0155073a1c9..965315c42a8 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -720,21 +720,15 @@ class MergeRequest < ActiveRecord::Base @environments ||= begin target_envs = target_project.environments_for( - target_branch, commit: diff_head_commit, with_tags: true) + ref: target_branch, commit: diff_head_commit, with_tags: true) source_envs = source_project.environments_for( - source_branch, commit: diff_head_commit) if source_project + ref: source_branch, commit: diff_head_commit) if source_project (target_envs.to_a + source_envs.to_a).uniq end end - def latest_environment - return @latest_environment if defined?(@latest_environment) - - @latest_environment = Environment.latest_for_commit(environments, diff_head_commit) - end - def state_human_name if merged? "Merged" diff --git a/app/models/project.rb b/app/models/project.rb index 15c6e25e73f..ff4487b6c8c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1306,7 +1306,7 @@ class Project < ActiveRecord::Base Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) } end - def environments_for(ref, commit: nil, with_tags: false) + def environments_for(ref: nil, commit: nil, with_tags: false) deps = if ref deployments_query = with_tags ? 'ref = ? OR tag IS TRUE' : 'ref = ?' @@ -1322,22 +1322,17 @@ class Project < ActiveRecord::Base .select(:environment_id) environments_found = environments.available - .where(id: environment_ids).to_a + .where(id: environment_ids).order_by_last_deployed_at.to_a - return environments_found unless commit + return environments_found unless ref && commit environments_found.select do |environment| environment.includes_commit?(commit) end end - def latest_environment_for(commit, ref: nil) - environments = environments_for(ref, commit: commit) - Environment.latest_for_commit(environments, commit) - end - def environments_recently_updated_on_branch(branch) - environments_for(branch).select do |environment| + environments_for(ref: branch).select do |environment| environment.recently_updated_on_branch?(branch) end end diff --git a/app/models/repository.rb b/app/models/repository.rb index ba9c038b66d..c043753507e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1190,6 +1190,7 @@ class Repository def route_map_for(sha) blob = blob_at(sha, ROUTE_MAP_PATH) return unless blob + blob.load_all_data!(self) blob.data end @@ -1197,6 +1198,7 @@ class Repository def gitlab_ci_yml_for(sha) blob = blob_at(sha, GITLAB_CI_YML_PATH) return unless blob + blob.load_all_data!(self) blob.data end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 55b8f888d53..dc2537d36aa 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -35,6 +35,20 @@ module Gitlab order end + def self.nulls_first_order(field, direction = 'ASC') + order = "#{field} #{direction}" + + if Gitlab::Database.postgresql? + order << ' NULLS FIRST' + else + # `field IS NULL` will be `0` for non-NULL columns and `1` for NULL + # columns. In the (default) ascending order, `0` comes first. + order.prepend("#{field} IS NULL, ") if direction == 'DESC' + end + + order + end + def self.random Gitlab::Database.postgresql? ? "RANDOM()" : "RAND()" end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 3031559c613..b142b3a2781 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -55,6 +55,22 @@ describe Gitlab::Database, lib: true do end end + describe '.nulls_first_order' do + context 'when using PostgreSQL' do + before { expect(described_class).to receive(:postgresql?).and_return(true) } + + it { expect(described_class.nulls_first_order('column', 'ASC')).to eq 'column ASC NULLS FIRST'} + it { expect(described_class.nulls_first_order('column', 'DESC')).to eq 'column DESC NULLS FIRST'} + end + + context 'when using MySQL' do + before { expect(described_class).to receive(:postgresql?).and_return(false) } + + it { expect(described_class.nulls_first_order('column', 'ASC')).to eq 'column ASC'} + it { expect(described_class.nulls_first_order('column', 'DESC')).to eq 'column IS NULL, column DESC'} + end + end + describe '#true_value' do it 'returns correct value for PostgreSQL' do expect(described_class).to receive(:postgresql?).and_return(true) diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index e40a9bf4fbe..7b68d0756d0 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -22,22 +22,17 @@ describe Environment, models: true do it { is_expected.to validate_length_of(:external_url).is_at_most(255) } it { is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id) } - describe '.latest_for_commit' do + describe '.order_by_last_deployed_at' do + let(:project) { create(:project) } let!(:environment1) { create(:environment, project: project) } let!(:environment2) { create(:environment, project: project) } let!(:environment3) { create(:environment, project: project) } - let!(:deployment1) { create(:deployment, environment: environment1) } let!(:deployment2) { create(:deployment, environment: environment2) } - let(:commit) { RepoHelpers.sample_commit } - - before do - allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1) - allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2) - allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil) - end + let!(:deployment1) { create(:deployment, environment: environment1) } - it 'returns the environment that the commit was last deployed to' do - expect(Environment.latest_for_commit([environment1, environment2, environment3], commit)).to be(environment2) + it 'returns the environments in order of having been last deployed' do + # byebug + expect(project.environments.order_by_last_deployed_at.to_a).to eq([environment3, environment2, environment1]) end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 91a8f2d77ab..32ed1e96749 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1069,30 +1069,6 @@ describe MergeRequest, models: true do end end - describe '#latest_environment' do - let(:project) { subject.project } - let!(:environment1) { create(:environment, project: project) } - let!(:environment2) { create(:environment, project: project) } - let!(:environment3) { create(:environment, project: project) } - let!(:deployment1) { create(:deployment, environment: environment1, ref: 'master', sha: commit.id) } - let!(:deployment2) { create(:deployment, environment: environment2, ref: 'feature', sha: commit.id) } - let(:commit) { subject.diff_head_commit } - - before do - allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1) - allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2) - allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil) - end - - before do - allow(subject).to receive(:environments).and_return([environment1, environment2, environment3]) - end - - it 'returns the environment that the commit was last deployed to' do - expect(subject.latest_environment).to eq(environment2) - end - end - describe "#reload_diff" do let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 1072b324b22..2280d0f554a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1726,17 +1726,17 @@ describe Project, models: true do end it 'returns environment when with_tags is set' do - expect(project.environments_for('master', commit: project.commit, with_tags: true)) + expect(project.environments_for(ref: 'master', commit: project.commit, with_tags: true)) .to contain_exactly(environment) end it 'does not return environment when no with_tags is set' do - expect(project.environments_for('master', commit: project.commit)) + expect(project.environments_for(ref: 'master', commit: project.commit)) .to be_empty end it 'does not return environment when commit is not part of deployment' do - expect(project.environments_for('master', commit: project.commit('feature'))) + expect(project.environments_for(ref: 'master', commit: project.commit('feature'))) .to be_empty end end @@ -1747,22 +1747,22 @@ describe Project, models: true do end it 'returns environment when ref is set' do - expect(project.environments_for('master', commit: project.commit)) + expect(project.environments_for(ref: 'master', commit: project.commit)) .to contain_exactly(environment) end it 'does not environment when ref is different' do - expect(project.environments_for('feature', commit: project.commit)) + expect(project.environments_for(ref: 'feature', commit: project.commit)) .to be_empty end it 'does not return environment when commit is not part of deployment' do - expect(project.environments_for('master', commit: project.commit('feature'))) + expect(project.environments_for(ref: 'master', commit: project.commit('feature'))) .to be_empty end it 'returns environment when commit constraint is not set' do - expect(project.environments_for('master')) + expect(project.environments_for(ref: 'master')) .to contain_exactly(environment) end end @@ -1773,48 +1773,12 @@ describe Project, models: true do end it 'returns environment' do - expect(project.environments_for(nil, commit: project.commit)) + expect(project.environments_for(commit: project.commit)) .to contain_exactly(environment) end end end - describe '#latest_environment_for' do - let(:project) { create(:project) } - let!(:environment1) { create(:environment, project: project) } - let!(:environment2) { create(:environment, project: project) } - let!(:environment3) { create(:environment, project: project) } - let!(:deployment1) { create(:deployment, environment: environment1, ref: 'master', sha: commit.id) } - let!(:deployment2) { create(:deployment, environment: environment2, ref: 'feature', sha: commit.id) } - let(:commit) { project.commit } - - before do - allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1) - allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2) - allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil) - end - - context 'when specifying a ref' do - before do - allow(project).to receive(:environments_for).with('master', commit: commit).and_return([environment1]) - end - - it 'returns the environment that the commit was last deployed to from that ref' do - expect(project.latest_environment_for(commit, ref: 'master')).to eq(environment1) - end - end - - context 'when not specifying a ref' do - before do - allow(project).to receive(:environments_for).with(nil, commit: commit).and_return([environment1, environment2]) - end - - it 'returns the environment that the commit was last deployed to' do - expect(project.latest_environment_for(commit)).to eq(environment2) - end - end - end - describe '#environments_recently_updated_on_branch' do let(:project) { create(:project, :repository) } let(:environment) { create(:environment, project: project) } |