Skip to content

G
gitlab-ce
Commit 3e856638 authored by Thong Kuah's avatar Thong Kuah
Browse files
Options

Move LB call in overriden method for MatchingMergeRequest to Core

parent 0d55602f
Hide whitespace changes
Inline Side-by-side
Showing with 152 additions and 193 deletions
ee/lib/ee/gitlab/checks/matching_merge_request.rb deleted 100644 → 0
View file @ 0d55602f
# frozen_string_literal: true
module EE
module Gitlab
module Checks
module MatchingMergeRequest
extend ::Gitlab::Utils::Override
TOTAL_METRIC = :gitlab_merge_request_match_total
STALE_METRIC = :gitlab_merge_request_match_stale_secondary
# rubocop:disable Gitlab/ModuleWithInstanceVariables
override :match?
def match?
return super unless ::Gitlab::Database::LoadBalancing.enable?
# When a user merges a merge request, the following sequence happens:
#
# 1. Sidekiq: MergeService runs and updates the merge request in a locked state.
# 2. Gitaly: The UserMergeBranch RPC runs.
# 3. Gitaly (gitaly-ruby): This RPC calls the pre-receive hook.
# 4. Rails: This hook makes an API request to /api/v4/internal/allowed.
# 5. Rails: This API check does a SQL query for locked merge
# requests with a matching SHA.
#
# Since steps 1 and 5 will happen on different database
# sessions, replication lag could erroneously cause step 5 to
# report no matching merge requests. To avoid this, we check
# the write location to ensure the replica can make this query.
track_session_metrics do
if ::Feature.enabled?(:load_balancing_atomic_replica, @project, default_enabled: :yaml)
::Gitlab::Database::LoadBalancing::Sticking.select_valid_host(:project, @project.id)
else
::Gitlab::Database::LoadBalancing::Sticking.unstick_or_continue_sticking(:project, @project.id)
end
end
super
end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
private
def track_session_metrics
before = ::Gitlab::Database::LoadBalancing::Session.current.use_primary?
yield
after = ::Gitlab::Database::LoadBalancing::Session.current.use_primary?
increment_attempt_count
if !before && after
increment_stale_secondary_count
end
end
def increment_attempt_count
total_counter.increment
end
def increment_stale_secondary_count
stale_counter.increment
end
def total_counter
@total_counter ||= ::Gitlab::Metrics.counter(TOTAL_METRIC, 'Total number of merge request match attempts')
end
def stale_counter
@stale_counter ||= ::Gitlab::Metrics.counter(STALE_METRIC, 'Total number of merge request match attempts with lagging secondary')
end
end
end
end
end
ee/spec/lib/gitlab/checks/matching_merge_request_spec.rb deleted 100644 → 0
View file @ 0d55602f
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Checks::MatchingMergeRequest do
describe '#match?' do
let_it_be(:newrev) { '012345678' }
let_it_be(:target_branch) { 'feature' }
let_it_be(:project) { create(:project, :repository) }
let_it_be(:locked_merge_request) do
create(:merge_request,
:locked,
source_project: project,
target_project: project,
target_branch: target_branch,
in_progress_merge_commit_sha: newrev)
end
let(:total_counter) { subject.send(:total_counter) }
let(:stale_counter) { subject.send(:stale_counter) }
subject { described_class.new(newrev, target_branch, project) }
context 'with load balancing disabled', :request_store, :redis do
before do
expect(::Gitlab::Database::LoadBalancing).to receive(:enable?).at_least(:once).and_return(false)
expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:unstick_or_continue_sticking)
expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:select_valid_replicas)
end
it 'does not attempt to stick to primary' do
expect(subject.match?).to be true
end
it 'increments no counters' do
expect { subject.match? }
.to change { total_counter.get }.by(0)
.and change { stale_counter.get }.by(0)
end
end
context 'with load balancing enabled', :request_store, :redis do
let(:session) { ::Gitlab::Database::LoadBalancing::Session.current }
let(:all_caught_up) { true }
before do
expect(::Gitlab::Database::LoadBalancing).to receive(:enable?).at_least(:once).and_return(true)
allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?).and_return(all_caught_up)
end
shared_examples 'secondary that has caught up to a primary' do
it 'continues to use the secondary' do
expect(session.use_primary?).to be false
expect(subject.match?).to be true
end
it 'only increments total counter' do
expect { subject.match? }
.to change { total_counter.get }.by(1)
.and change { stale_counter.get }.by(0)
end
end
shared_examples 'secondary that is lagging primary' do
it 'sticks to the primary' do
expect(subject.match?).to be true
expect(session.use_primary?).to be true
end
it 'increments both total and stale counters' do
expect { subject.match? }
.to change { total_counter.get }.by(1)
.and change { stale_counter.get }.by(1)
end
end
context 'with load_balancing_atomic_replica feature flag enabled' do
before do
stub_feature_flags(load_balancing_atomic_replica: true)
expect(::Gitlab::Database::LoadBalancing::Sticking).to receive(:select_valid_host).with(:project, project.id).and_call_original
allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:select_caught_up_replicas).with(:project, project.id).and_return(all_caught_up)
end
it_behaves_like 'secondary that has caught up to a primary'
context 'on secondary behind primary' do
let(:all_caught_up) { false }
it_behaves_like 'secondary that is lagging primary'
end
end
context 'with load_balancing_atomic_replica feature flag disabled' do
before do
stub_feature_flags(load_balancing_atomic_replica: false)
expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:select_valid_host)
expect(::Gitlab::Database::LoadBalancing::Sticking).to receive(:unstick_or_continue_sticking).and_call_original
allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?).and_return(all_caught_up)
end
it_behaves_like 'secondary that has caught up to a primary'
context 'on secondary behind primary' do
let(:all_caught_up) { false }
it_behaves_like 'secondary that is lagging primary'
end
end
end
end
end
lib/gitlab/checks/matching_merge_request.rb
View file @ 3e856638
......@@ -3,22 +3,78 @@
module Gitlab
module Checks
class MatchingMergeRequest
TOTAL_METRIC = :gitlab_merge_request_match_total
STALE_METRIC = :gitlab_merge_request_match_stale_secondary
def initialize(newrev, branch_name, project)
@newrev = newrev
@branch_name = branch_name
@project = project
end
# rubocop: disable CodeReuse/ActiveRecord
def match?
if ::Gitlab::Database::LoadBalancing.enable?
# When a user merges a merge request, the following sequence happens:
#
# 1. Sidekiq: MergeService runs and updates the merge request in a locked state.
# 2. Gitaly: The UserMergeBranch RPC runs.
# 3. Gitaly (gitaly-ruby): This RPC calls the pre-receive hook.
# 4. Rails: This hook makes an API request to /api/v4/internal/allowed.
# 5. Rails: This API check does a SQL query for locked merge
# requests with a matching SHA.
#
# Since steps 1 and 5 will happen on different database
# sessions, replication lag could erroneously cause step 5 to
# report no matching merge requests. To avoid this, we check
# the write location to ensure the replica can make this query.
track_session_metrics do
if ::Feature.enabled?(:load_balancing_atomic_replica, @project, default_enabled: :yaml)
::Gitlab::Database::LoadBalancing::Sticking.select_valid_host(:project, @project.id)
else
::Gitlab::Database::LoadBalancing::Sticking.unstick_or_continue_sticking(:project, @project.id)
end
end
end
# rubocop: disable CodeReuse/ActiveRecord
@project.merge_requests
.with_state(:locked)
.where(in_progress_merge_commit_sha: @newrev, target_branch: @branch_name)
.exists?
# rubocop: enable CodeReuse/ActiveRecord
end
private
def track_session_metrics
before = ::Gitlab::Database::LoadBalancing::Session.current.use_primary?
yield
after = ::Gitlab::Database::LoadBalancing::Session.current.use_primary?
increment_attempt_count
if !before && after
increment_stale_secondary_count
end
end
def increment_attempt_count
total_counter.increment
end
def increment_stale_secondary_count
stale_counter.increment
end
def total_counter
@total_counter ||= ::Gitlab::Metrics.counter(TOTAL_METRIC, 'Total number of merge request match attempts')
end
def stale_counter
@stale_counter ||= ::Gitlab::Metrics.counter(STALE_METRIC, 'Total number of merge request match attempts with lagging secondary')
end
# rubocop: enable CodeReuse/ActiveRecord
end
end
end
Gitlab::Checks::MatchingMergeRequest.prepend_mod_with('Gitlab::Checks::MatchingMergeRequest')
spec/lib/gitlab/checks/matching_merge_request_spec.rb
View file @ 3e856638
......@@ -18,6 +18,9 @@ RSpec.describe Gitlab::Checks::MatchingMergeRequest do
subject { described_class.new(newrev, target_branch, project) }
let(:total_counter) { subject.send(:total_counter) }
let(:stale_counter) { subject.send(:stale_counter) }
it 'matches a merge request' do
expect(subject.match?).to be true
end
......@@ -27,5 +30,94 @@ RSpec.describe Gitlab::Checks::MatchingMergeRequest do
expect(matcher.match?).to be false
end
context 'with load balancing disabled', :request_store, :redis do
before do
expect(::Gitlab::Database::LoadBalancing).to receive(:enable?).at_least(:once).and_return(false)
expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:unstick_or_continue_sticking)
expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:select_valid_replicas)
end
it 'does not attempt to stick to primary' do
expect(subject.match?).to be true
end
it 'increments no counters' do
expect { subject.match? }
.to change { total_counter.get }.by(0)
.and change { stale_counter.get }.by(0)
end
end
context 'with load balancing enabled', :request_store, :redis do
let(:session) { ::Gitlab::Database::LoadBalancing::Session.current }
let(:all_caught_up) { true }
before do
expect(::Gitlab::Database::LoadBalancing).to receive(:enable?).at_least(:once).and_return(true)
allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?).and_return(all_caught_up)
end
shared_examples 'secondary that has caught up to a primary' do
it 'continues to use the secondary' do
expect(session.use_primary?).to be false
expect(subject.match?).to be true
end
it 'only increments total counter' do
expect { subject.match? }
.to change { total_counter.get }.by(1)
.and change { stale_counter.get }.by(0)
end
end
shared_examples 'secondary that is lagging primary' do
it 'sticks to the primary' do
expect(subject.match?).to be true
expect(session.use_primary?).to be true
end
it 'increments both total and stale counters' do
expect { subject.match? }
.to change { total_counter.get }.by(1)
.and change { stale_counter.get }.by(1)
end
end
context 'with load_balancing_atomic_replica feature flag enabled' do
before do
stub_feature_flags(load_balancing_atomic_replica: true)
expect(::Gitlab::Database::LoadBalancing::Sticking).to receive(:select_valid_host).with(:project, project.id).and_call_original
allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:select_caught_up_replicas).with(:project, project.id).and_return(all_caught_up)
end
it_behaves_like 'secondary that has caught up to a primary'
context 'on secondary behind primary' do
let(:all_caught_up) { false }
it_behaves_like 'secondary that is lagging primary'
end
end
context 'with load_balancing_atomic_replica feature flag disabled' do
before do
stub_feature_flags(load_balancing_atomic_replica: false)
expect(::Gitlab::Database::LoadBalancing::Sticking).not_to receive(:select_valid_host)
expect(::Gitlab::Database::LoadBalancing::Sticking).to receive(:unstick_or_continue_sticking).and_call_original
allow(::Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?).and_return(all_caught_up)
end
it_behaves_like 'secondary that has caught up to a primary'
context 'on secondary behind primary' do
let(:all_caught_up) { false }
it_behaves_like 'secondary that is lagging primary'
end
end
end
end
end
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7