Commit fb010c05 authored by Bala Kumar's avatar Bala Kumar

Refactor to remove cross join with ci_builds

parent 24b51f40
...@@ -46,7 +46,6 @@ class Deployment < ApplicationRecord ...@@ -46,7 +46,6 @@ class Deployment < ApplicationRecord
scope :stoppable, -> { where.not(on_stop: nil).where.not(deployable_id: nil).success } scope :stoppable, -> { where.not(on_stop: nil).where.not(deployable_id: nil).success }
scope :active, -> { where(status: %i[created running]) } scope :active, -> { where(status: %i[created running]) }
scope :older_than, -> (deployment) { where('deployments.id < ?', deployment.id) } scope :older_than, -> (deployment) { where('deployments.id < ?', deployment.id) }
scope :with_deployable, -> { joins('INNER JOIN ci_builds ON ci_builds.id = deployments.deployable_id').preload(:deployable) }
scope :with_api_entity_associations, -> { preload({ deployable: { runner: [], tags: [], user: [], job_artifacts_archive: [] } }) } scope :with_api_entity_associations, -> { preload({ deployable: { runner: [], tags: [], user: [], job_artifacts_archive: [] } }) }
scope :finished_after, ->(date) { where('finished_at >= ?', date) } scope :finished_after, ->(date) { where('finished_at >= ?', date) }
...@@ -148,6 +147,16 @@ class Deployment < ApplicationRecord ...@@ -148,6 +147,16 @@ class Deployment < ApplicationRecord
success.find_by!(iid: iid) success.find_by!(iid: iid)
end end
# It should be used with caution especially on chaining.
# Fetching any unbounded or large intermediate dataset could lead to loading too many IDs into memory.
# See: https://docs.gitlab.com/ee/development/database/multiple_databases.html#use-disable_joins-for-has_one-or-has_many-through-relations
# For safety we default limit to fetch not more than 1000 records.
def self.builds(limit = 1000)
deployable_ids = where.not(deployable_id: nil).limit(limit).pluck(:deployable_id)
Ci::Build.where(id: deployable_ids)
end
class << self class << self
## ##
# FastDestroyAll concerns # FastDestroyAll concerns
......
...@@ -260,10 +260,9 @@ class Environment < ApplicationRecord ...@@ -260,10 +260,9 @@ class Environment < ApplicationRecord
end end
def cancel_deployment_jobs! def cancel_deployment_jobs!
jobs = active_deployments.with_deployable active_deployments.builds.each do |build|
jobs.each do |deployment| Gitlab::OptimisticLocking.retry_lock(build, name: 'environment_cancel_deployment_jobs') do |build|
Gitlab::OptimisticLocking.retry_lock(deployment.deployable, name: 'environment_cancel_deployment_jobs') do |deployable| build.cancel! if build&.cancelable?
deployable.cancel! if deployable&.cancelable?
end end
rescue StandardError => e rescue StandardError => e
Gitlab::ErrorTracking.track_exception(e, environment_id: id, deployment_id: deployment.id) Gitlab::ErrorTracking.track_exception(e, environment_id: id, deployment_id: deployment.id)
......
...@@ -11,23 +11,23 @@ module Deployments ...@@ -11,23 +11,23 @@ module Deployments
def execute def execute
return unless @deployment&.running? return unless @deployment&.running?
older_deployments.find_each do |older_deployment| older_deployments_builds.each do |build|
Gitlab::OptimisticLocking.retry_lock(older_deployment.deployable, name: 'older_deployments_drop') do |deployable| Gitlab::OptimisticLocking.retry_lock(build, name: 'older_deployments_drop') do |build|
deployable.drop(:forward_deployment_failure) build.drop(:forward_deployment_failure)
end end
rescue StandardError => e rescue StandardError => e
Gitlab::ErrorTracking.track_exception(e, subject_id: @deployment.id, deployment_id: older_deployment.id) Gitlab::ErrorTracking.track_exception(e, subject_id: @deployment.id, build_id: build.id)
end end
end end
private private
def older_deployments def older_deployments_builds
@deployment @deployment
.environment .environment
.active_deployments .active_deployments
.older_than(@deployment) .older_than(@deployment)
.with_deployable .builds
end end
end end
end end
...@@ -39,10 +39,10 @@ module Deployments ...@@ -39,10 +39,10 @@ module Deployments
return unless previous_commit_ids return unless previous_commit_ids
rollback_target = environment.successful_deployments rollback_target = environment.successful_deployments
.with_deployable
.latest_for_sha(previous_commit_ids) .latest_for_sha(previous_commit_ids)
return unless rollback_target && rollback_target.deployable.retryable? # Safely navigate deployable because of https://gitlab.com/gitlab-org/gitlab/-/issues/218659
return unless rollback_target && rollback_target.deployable&.retryable?
rollback_target rollback_target
end end
......
...@@ -97,6 +97,16 @@ RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_rate_limiti ...@@ -97,6 +97,16 @@ RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_rate_limiti
end end
end end
context "when rollback target's deployable is not available" do
before do
environment.all_deployments.first.deployable.destroy!
end
it_behaves_like 'rollback failure' do
let(:message) { 'Failed to find a rollback target.' }
end
end
context "when rollback target's deployable is not retryable" do context "when rollback target's deployable is not retryable" do
before do before do
environment.all_deployments.first.deployable.degenerate! environment.all_deployments.first.deployable.degenerate!
......
...@@ -456,18 +456,6 @@ RSpec.describe Deployment do ...@@ -456,18 +456,6 @@ RSpec.describe Deployment do
end end
end end
describe 'with_deployable' do
subject { described_class.with_deployable }
it 'retrieves deployments with deployable builds' do
with_deployable = create(:deployment)
create(:deployment, deployable: nil)
create(:deployment, deployable_type: 'CommitStatus', deployable_id: non_existing_record_id)
is_expected.to contain_exactly(with_deployable)
end
end
describe 'visible' do describe 'visible' do
subject { described_class.visible } subject { described_class.visible }
...@@ -613,6 +601,26 @@ RSpec.describe Deployment do ...@@ -613,6 +601,26 @@ RSpec.describe Deployment do
end end
end end
describe '.builds' do
let!(:deployment1) { create(:deployment) }
let!(:deployment2) { create(:deployment) }
let!(:deployment3) { create(:deployment) }
subject { described_class.builds }
it 'retrieves builds for the deployments' do
is_expected.to match_array(
[deployment1.deployable, deployment2.deployable, deployment3.deployable])
end
it 'does not fetch the null deployable_ids' do
deployment3.update!(deployable_id: nil, deployable_type: nil)
is_expected.to match_array(
[deployment1.deployable, deployment2.deployable])
end
end
describe '#previous_deployment' do describe '#previous_deployment' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
......
...@@ -84,7 +84,7 @@ RSpec.describe Deployments::OlderDeploymentsDropService do ...@@ -84,7 +84,7 @@ RSpec.describe Deployments::OlderDeploymentsDropService do
it 'does not drop an older deployment and tracks the exception' do it 'does not drop an older deployment and tracks the exception' do
expect(Gitlab::ErrorTracking).to receive(:track_exception) expect(Gitlab::ErrorTracking).to receive(:track_exception)
.with(kind_of(RuntimeError), subject_id: deployment.id, deployment_id: older_deployment.id) .with(kind_of(RuntimeError), subject_id: deployment.id, build_id: older_deployment.deployable_id)
expect { subject }.not_to change { Ci::Build.failed.count } expect { subject }.not_to change { Ci::Build.failed.count }
end end
......
...@@ -70,8 +70,6 @@ ...@@ -70,8 +70,6 @@
- "./spec/models/ci/job_artifact_spec.rb" - "./spec/models/ci/job_artifact_spec.rb"
- "./spec/models/ci/pipeline_spec.rb" - "./spec/models/ci/pipeline_spec.rb"
- "./spec/models/ci/runner_spec.rb" - "./spec/models/ci/runner_spec.rb"
- "./spec/models/deployment_spec.rb"
- "./spec/models/environment_spec.rb"
- "./spec/models/merge_request_spec.rb" - "./spec/models/merge_request_spec.rb"
- "./spec/models/project_spec.rb" - "./spec/models/project_spec.rb"
- "./spec/models/user_spec.rb" - "./spec/models/user_spec.rb"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment