From adb8dbff4284689e6902ff455ef74cf84fb79404 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 23 May 2023 06:07:38 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/services/git/branch_hooks_service.rb | 30 +++++++- ...> batch_delay_jira_branch_sync_worker.yml} | 10 +-- ...eplace_vsd_index_with_nulls_first_order.rb | 25 +++++++ db/schema_migrations/20230516080816 | 1 + db/structure.sql | 2 +- lib/gitlab/database/load_balancing/host.rb | 40 +++++----- .../database/load_balancing/host_spec.rb | 15 +--- spec/lib/gitlab/database/pg_depend_spec.rb | 10 ++- spec/services/git/branch_push_service_spec.rb | 75 ++++++++++++++++--- 9 files changed, 156 insertions(+), 52 deletions(-) rename config/feature_flags/development/{optimize_scope_projects_with_feature_available.yml => batch_delay_jira_branch_sync_worker.yml} (58%) create mode 100644 db/migrate/20230516080816_replace_vsd_index_with_nulls_first_order.rb create mode 100644 db/schema_migrations/20230516080816 diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb index 2ead2e2a113..1fa8cceaf3e 100644 --- a/app/services/git/branch_hooks_service.rb +++ b/app/services/git/branch_hooks_service.rb @@ -4,6 +4,9 @@ module Git class BranchHooksService < ::Git::BaseHooksService extend ::Gitlab::Utils::Override + JIRA_SYNC_BATCH_SIZE = 20 + JIRA_SYNC_BATCH_DELAY = 10.seconds + def execute execute_branch_hooks @@ -157,13 +160,34 @@ module Git return unless project.jira_subscription_exists? branch_to_sync = branch_name if Atlassian::JiraIssueKeyExtractors::Branch.has_keys?(project, branch_name) - commits_to_sync = limited_commits.select { |commit| Atlassian::JiraIssueKeyExtractor.has_keys?(commit.safe_message) }.map(&:sha) + commits_to_sync = filtered_commit_shas - if branch_to_sync || commits_to_sync.any? - JiraConnect::SyncBranchWorker.perform_async(project.id, branch_to_sync, commits_to_sync, Atlassian::JiraConnect::Client.generate_update_sequence_id) + return if branch_to_sync.nil? && commits_to_sync.empty? + + if commits_to_sync.any? && Feature.enabled?(:batch_delay_jira_branch_sync_worker, project) + commits_to_sync.each_slice(JIRA_SYNC_BATCH_SIZE).with_index do |commits, i| + JiraConnect::SyncBranchWorker.perform_in( + JIRA_SYNC_BATCH_DELAY * i, + project.id, + branch_to_sync, + commits, + Atlassian::JiraConnect::Client.generate_update_sequence_id + ) + end + else + JiraConnect::SyncBranchWorker.perform_async( + project.id, + branch_to_sync, + commits_to_sync, + Atlassian::JiraConnect::Client.generate_update_sequence_id + ) end end + def filtered_commit_shas + limited_commits.select { |commit| Atlassian::JiraIssueKeyExtractor.has_keys?(commit.safe_message) }.map(&:sha) + end + def signature_types [ ::CommitSignatures::GpgSignature, diff --git a/config/feature_flags/development/optimize_scope_projects_with_feature_available.yml b/config/feature_flags/development/batch_delay_jira_branch_sync_worker.yml similarity index 58% rename from config/feature_flags/development/optimize_scope_projects_with_feature_available.yml rename to config/feature_flags/development/batch_delay_jira_branch_sync_worker.yml index 86811a6c534..f10403e98e9 100644 --- a/config/feature_flags/development/optimize_scope_projects_with_feature_available.yml +++ b/config/feature_flags/development/batch_delay_jira_branch_sync_worker.yml @@ -1,8 +1,8 @@ --- -name: optimize_scope_projects_with_feature_available -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/119950/ -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/410693 -milestone: '16.0' +name: batch_delay_jira_branch_sync_worker +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120866 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/411865 +milestone: '16.1' type: development -group: group::tenant scale +group: group::source code default_enabled: false diff --git a/db/migrate/20230516080816_replace_vsd_index_with_nulls_first_order.rb b/db/migrate/20230516080816_replace_vsd_index_with_nulls_first_order.rb new file mode 100644 index 00000000000..dcc8bd5972e --- /dev/null +++ b/db/migrate/20230516080816_replace_vsd_index_with_nulls_first_order.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class ReplaceVsdIndexWithNullsFirstOrder < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + OLD_INDEX = 'index_on_value_stream_dashboard_aggregations_last_run_at_id' + NEW_INDEX = 'index_on_value_stream_dashboard_aggregations_last_run_at_and_id' + + def up + add_concurrent_index :value_stream_dashboard_aggregations, + [:last_run_at, :namespace_id], + where: 'enabled IS TRUE', + name: NEW_INDEX, + order: { last_run_at: 'ASC NULLS FIRST' } + remove_concurrent_index_by_name :value_stream_dashboard_aggregations, OLD_INDEX + end + + def down + add_concurrent_index :value_stream_dashboard_aggregations, + [:last_run_at, :namespace_id], + where: 'enabled IS TRUE', + name: OLD_INDEX + remove_concurrent_index_by_name :value_stream_dashboard_aggregations, NEW_INDEX + end +end diff --git a/db/schema_migrations/20230516080816 b/db/schema_migrations/20230516080816 new file mode 100644 index 00000000000..720e9275ea2 --- /dev/null +++ b/db/schema_migrations/20230516080816 @@ -0,0 +1 @@ +d45ccbc7191760bf61396cf3b50110352149958dfe3696d5e4a172f9e96e204a \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 5767bebb6c6..c117d095de4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -31679,7 +31679,7 @@ CREATE INDEX index_on_users_lower_username ON users USING btree (lower((username CREATE INDEX index_on_users_name_lower ON users USING btree (lower((name)::text)); -CREATE INDEX index_on_value_stream_dashboard_aggregations_last_run_at_id ON value_stream_dashboard_aggregations USING btree (last_run_at, namespace_id) WHERE (enabled IS TRUE); +CREATE INDEX index_on_value_stream_dashboard_aggregations_last_run_at_and_id ON value_stream_dashboard_aggregations USING btree (last_run_at NULLS FIRST, namespace_id) WHERE (enabled IS TRUE); CREATE INDEX index_onboarding_progresses_for_create_track ON onboarding_progresses USING btree (created_at) WHERE (git_write_at IS NULL); diff --git a/lib/gitlab/database/load_balancing/host.rb b/lib/gitlab/database/load_balancing/host.rb index bdbb80d6f31..156e033d183 100644 --- a/lib/gitlab/database/load_balancing/host.rb +++ b/lib/gitlab/database/load_balancing/host.rb @@ -142,11 +142,25 @@ module Gitlab # primary. # # This method will return nil if no lag size could be calculated. - def replication_lag_size - location = connection.quote(primary_write_location) + def replication_lag_size(location = primary_write_location) + location = connection.quote(location) + + # The following is necessary to handle a mix of logical and physical replicas. We assume that if they have + # pg_replication_origin_status then they are a logical replica. In a logical replica we need to use + # `remote_lsn` rather than `pg_last_wal_replay_lsn` in order for our LSN to be comparable to the source + # cluster. This logic would be broken if we have 2 logical subscriptions or if we have a logical subscription + # in the source primary cluster. Read more at https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121208 row = query_and_release(<<-SQL.squish) - SELECT pg_wal_lsn_diff(#{location}, pg_last_wal_replay_lsn())::float - AS diff + SELECT pg_wal_lsn_diff(#{location}, ( + CASE + WHEN (SELECT TRUE FROM pg_replication_origin_status) THEN + (SELECT remote_lsn FROM pg_replication_origin_status) + WHEN pg_is_in_recovery() THEN + pg_last_wal_replay_lsn() + ELSE + pg_current_wal_insert_lsn() + END + ))::float AS diff SQL row['diff'].to_i if row.any? @@ -173,22 +187,8 @@ module Gitlab # # location - The transaction write location as reported by a primary. def caught_up?(location) - string = connection.quote(location) - - # In case the host is a primary pg_last_wal_replay_lsn/pg_last_xlog_replay_location() returns - # NULL. The recovery check ensures we treat the host as up-to-date in - # such a case. - query = <<-SQL.squish - SELECT NOT pg_is_in_recovery() - OR pg_wal_lsn_diff(pg_last_wal_replay_lsn(), #{string}) >= 0 - AS result - SQL - - row = query_and_release(query) - - ::Gitlab::Utils.to_boolean(row['result']) - rescue *CONNECTION_ERRORS - false + lag = replication_lag_size(location) + lag.present? && lag.to_i <= 0 end def query_and_release(sql) diff --git a/spec/lib/gitlab/database/load_balancing/host_spec.rb b/spec/lib/gitlab/database/load_balancing/host_spec.rb index b040c7a76bd..d277514dea9 100644 --- a/spec/lib/gitlab/database/load_balancing/host_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_spec.rb @@ -357,28 +357,21 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do it 'returns true when a host has caught up' do allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => 't' }]) + expect(connection).to receive(:select_all).and_return([{ 'diff' => -1 }]) expect(host.caught_up?('foo')).to eq(true) end - it 'returns true when a host has caught up' do + it 'returns false when diff query returns nothing' do allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => true }]) - - expect(host.caught_up?('foo')).to eq(true) - end - - it 'returns false when a host has not caught up' do - allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => 'f' }]) + expect(connection).to receive(:select_all).and_return([]) expect(host.caught_up?('foo')).to eq(false) end it 'returns false when a host has not caught up' do allow(host).to receive(:connection).and_return(connection) - expect(connection).to receive(:select_all).and_return([{ 'result' => false }]) + expect(connection).to receive(:select_all).and_return([{ 'diff' => 123 }]) expect(host.caught_up?('foo')).to eq(false) end diff --git a/spec/lib/gitlab/database/pg_depend_spec.rb b/spec/lib/gitlab/database/pg_depend_spec.rb index 547a2c84b76..ff5169ebabf 100644 --- a/spec/lib/gitlab/database/pg_depend_spec.rb +++ b/spec/lib/gitlab/database/pg_depend_spec.rb @@ -13,8 +13,14 @@ RSpec.describe Gitlab::Database::PgDepend, type: :model, feature_category: :data connection.execute('CREATE EXTENSION IF NOT EXISTS pg_stat_statements;') end - it 'returns pg_stat_statements', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/410508' do - expect(subject.pluck('relname')).to eq(['pg_stat_statements']) + it 'returns pg_stat_statements' do + expected_views = ['pg_stat_statements'] + + if Gitlab::Database::Reflection.new(described_class).version.to_f >= 14 + expected_views << 'pg_stat_statements_info' # View added by pg_stat_statements starting in postgres 14 + end + + expect(subject.pluck('relname')).to match_array(expected_views) end end end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index aa534777f3e..5e43426b9dd 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -14,15 +14,17 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: let(:branch) { 'master' } let(:ref) { "refs/heads/#{branch}" } let(:push_options) { nil } + let(:service) do + described_class + .new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }, push_options: push_options) + end before do project.add_maintainer(user) end subject(:execute_service) do - described_class - .new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }, push_options: push_options) - .execute + service.execute end describe 'Push branches' do @@ -683,14 +685,44 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: let(:commits_to_sync) { [] } shared_examples 'enqueues Jira sync worker' do - specify :aggregate_failures do - Sidekiq::Testing.fake! do - expect(JiraConnect::SyncBranchWorker) - .to receive(:perform_async) - .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) - .and_call_original + context "batch_delay_jira_branch_sync_worker feature flag is enabled" do + before do + stub_feature_flags(batch_delay_jira_branch_sync_worker: true) + end - expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + specify :aggregate_failures do + Sidekiq::Testing.fake! do + if commits_to_sync.any? + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_in) + .with(kind_of(Numeric), project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original + else + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_async) + .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original + end + + expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + end + end + end + + context "batch_delay_jira_branch_sync_worker feature flag is disabled" do + before do + stub_feature_flags(batch_delay_jira_branch_sync_worker: false) + end + + specify :aggregate_failures do + Sidekiq::Testing.fake! do + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_async) + .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original + + expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + end end end end @@ -723,6 +755,29 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: end it_behaves_like 'enqueues Jira sync worker' + + describe 'batch requests' do + let(:commits_to_sync) { [sample_commit.id, another_sample_commit.id] } + + it 'enqueues multiple jobs' do + # We have to stub this as we only have two valid commits to use + stub_const('Git::BranchHooksService::JIRA_SYNC_BATCH_SIZE', 1) + + expect_any_instance_of(Git::BranchHooksService).to receive(:filtered_commit_shas).and_return(commits_to_sync) + + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_in) + .with(0.seconds, project.id, branch_to_sync, [commits_to_sync.first], kind_of(Numeric)) + .and_call_original + + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_in) + .with(10.seconds, project.id, branch_to_sync, [commits_to_sync.last], kind_of(Numeric)) + .and_call_original + + subject + end + end end context 'branch name and commit message does not contain Jira issue key' do