From 5f5906d8724fd27bc3f16f36e709990c03fb6954 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Thu, 14 Mar 2019 16:05:06 +1100 Subject: [PATCH 1/3] New pg_last_xact_replay_timestamp DB helper --- ee/lib/gitlab/geo/health_check.rb | 2 +- lib/gitlab/database.rb | 4 ++++ spec/lib/gitlab/database_spec.rb | 6 ++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/ee/lib/gitlab/geo/health_check.rb b/ee/lib/gitlab/geo/health_check.rb index 6ff59e77dfcd1a..781e74d3578909 100644 --- a/ee/lib/gitlab/geo/health_check.rb +++ b/ee/lib/gitlab/geo/health_check.rb @@ -42,7 +42,7 @@ def db_replication_lag_seconds_query WHEN #{Gitlab::Database.pg_last_wal_receive_lsn}() = #{Gitlab::Database.pg_last_wal_replay_lsn}() THEN 0 ELSE - EXTRACT (EPOCH FROM now() - pg_last_xact_replay_timestamp())::INTEGER + EXTRACT (EPOCH FROM now() - #{Gitlab::Database.pg_last_xact_replay_timestamp}())::INTEGER END AS replication_lag SQL diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index abf5105eb8f4af..8fcca5da1804ee 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -102,6 +102,10 @@ def self.pg_last_wal_replay_lsn Gitlab::Database.postgresql_9_or_less? ? 'pg_last_xlog_replay_location' : 'pg_last_wal_replay_lsn' end + def self.pg_last_xact_replay_timestamp + 'pg_last_xact_replay_timestamp' + end + def self.nulls_last_order(field, direction = 'ASC') order = "#{field} #{direction}" diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 60106ee3c0b327..891822db5a1a52 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -195,6 +195,12 @@ end end + describe '.pg_last_xact_replay_timestamp' do + it 'returns pg_last_xact_replay_timestamp' do + expect(described_class.pg_last_xact_replay_timestamp).to eq('pg_last_xact_replay_timestamp') + end + end + describe '.nulls_last_order' do context 'when using PostgreSQL' do before do -- GitLab From 30c323a3cd176dc855c368b05de57bb5cfa6a886 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Thu, 14 Mar 2019 16:09:15 +1100 Subject: [PATCH 2/3] Rename pg_stat_wal_receiver_supported? DB helper From pg_stat_wal_receiver_supported? to postgresql_minimum_supported_version? Also add test coverage --- .../controllers/admin/geo/nodes_controller.rb | 2 +- ee/lib/tasks/geo.rake | 2 +- .../admin/geo/nodes_controller_spec.rb | 4 +-- lib/gitlab/database.rb | 2 +- spec/lib/gitlab/database_spec.rb | 32 +++++++++++++++++++ 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/ee/app/controllers/admin/geo/nodes_controller.rb b/ee/app/controllers/admin/geo/nodes_controller.rb index 92c69e0662b477..e04d4cfbb36599 100644 --- a/ee/app/controllers/admin/geo/nodes_controller.rb +++ b/ee/app/controllers/admin/geo/nodes_controller.rb @@ -15,7 +15,7 @@ def index flash.now[:alert] = _('You need a different license to enable Geo replication.') end - unless Gitlab::Database.pg_stat_wal_receiver_supported? + unless Gitlab::Database.postgresql_minimum_supported_version? flash.now[:warning] = _('Please upgrade PostgreSQL to version 9.6 or greater. The status of the replication cannot be determined reliably with the current version.') end end diff --git a/ee/lib/tasks/geo.rake b/ee/lib/tasks/geo.rake index 3894b44be7bda1..9fbed099665e85 100644 --- a/ee/lib/tasks/geo.rake +++ b/ee/lib/tasks/geo.rake @@ -220,7 +220,7 @@ namespace :geo do puts GeoNode.current_node_url puts '-----------------------------------------------------'.color(:yellow) - unless Gitlab::Database.pg_stat_wal_receiver_supported? + unless Gitlab::Database.postgresql_minimum_supported_version? puts puts 'WARNING: Please upgrade PostgreSQL to version 9.6 or greater. The status of the replication cannot be determined reliably with the current version.'.color(:red) puts diff --git a/ee/spec/controllers/admin/geo/nodes_controller_spec.rb b/ee/spec/controllers/admin/geo/nodes_controller_spec.rb index 90b9c6d170cc04..486881b1136ec0 100644 --- a/ee/spec/controllers/admin/geo/nodes_controller_spec.rb +++ b/ee/spec/controllers/admin/geo/nodes_controller_spec.rb @@ -65,7 +65,7 @@ def go context 'with Postgres 9.6 or greater' do before do - allow(Gitlab::Database).to receive(:pg_stat_wal_receiver_supported?).and_return(true) + allow(Gitlab::Database).to receive(:postgresql_minimum_supported_version?).and_return(true) end it_behaves_like 'no flash message', :warning @@ -73,7 +73,7 @@ def go context 'without Postgres 9.6 or greater' do before do - allow(Gitlab::Database).to receive(:pg_stat_wal_receiver_supported?).and_return(false) + allow(Gitlab::Database).to receive(:postgresql_minimum_supported_version?).and_return(false) end it_behaves_like 'with flash message', :warning, 'Please upgrade PostgreSQL to version 9.6 or greater.' diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 8fcca5da1804ee..0a58265145822b 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -80,7 +80,7 @@ def self.replication_slots_supported? postgresql? && version.to_f >= 9.4 end - def self.pg_stat_wal_receiver_supported? + def self.postgresql_minimum_supported_version? postgresql? && version.to_f >= 9.6 end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 891822db5a1a52..ae50abd0e7a202 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -87,6 +87,38 @@ end end + describe '.postgresql_minimum_supported_version?' do + it 'returns false when not using PostgreSQL' do + allow(described_class).to receive(:postgresql?).and_return(false) + + expect(described_class.postgresql_minimum_supported_version?).to eq(false) + end + + context 'when using PostgreSQL' do + before do + allow(described_class).to receive(:postgresql?).and_return(true) + end + + it 'returns false when using PostgreSQL 9.5' do + allow(described_class).to receive(:version).and_return('9.5') + + expect(described_class.postgresql_minimum_supported_version?).to eq(false) + end + + it 'returns true when using PostgreSQL 9.6' do + allow(described_class).to receive(:version).and_return('9.6') + + expect(described_class.postgresql_minimum_supported_version?).to eq(true) + end + + it 'returns true when using PostgreSQL 10 or newer' do + allow(described_class).to receive(:version).and_return('10') + + expect(described_class.postgresql_minimum_supported_version?).to eq(true) + end + end + end + describe '.join_lateral_supported?' do it 'returns false when using MySQL' do allow(described_class).to receive(:postgresql?).and_return(false) -- GitLab From 4bef08931111c5f291b485724a0a399c402c3d3f Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Thu, 14 Mar 2019 16:17:07 +1100 Subject: [PATCH 3/3] Geo: Support archive recovery / streaming types In the context of health checks --- ...tatus-when-wal-e-streaming-is-in-use-2.yml | 5 +++ ee/lib/gitlab/geo/health_check.rb | 38 ++++++++++++++++--- ee/spec/lib/gitlab/geo/health_check_spec.rb | 34 ++++++++++++++--- 3 files changed, 66 insertions(+), 11 deletions(-) create mode 100644 ee/changelogs/unreleased/5933-geo-inconsistent-database-replication-status-when-wal-e-streaming-is-in-use-2.yml diff --git a/ee/changelogs/unreleased/5933-geo-inconsistent-database-replication-status-when-wal-e-streaming-is-in-use-2.yml b/ee/changelogs/unreleased/5933-geo-inconsistent-database-replication-status-when-wal-e-streaming-is-in-use-2.yml new file mode 100644 index 00000000000000..afaab0f819c688 --- /dev/null +++ b/ee/changelogs/unreleased/5933-geo-inconsistent-database-replication-status-when-wal-e-streaming-is-in-use-2.yml @@ -0,0 +1,5 @@ +--- +title: 'Geo: Support archive recovery or streaming replication types in health check' +merge_request: 9935 +author: +type: fixed diff --git a/ee/lib/gitlab/geo/health_check.rb b/ee/lib/gitlab/geo/health_check.rb index 781e74d3578909..e23468f2303f66 100644 --- a/ee/lib/gitlab/geo/health_check.rb +++ b/ee/lib/gitlab/geo/health_check.rb @@ -10,9 +10,9 @@ def perform_checks return '' unless Gitlab::Geo.secondary? return 'Geo database configuration file is missing.' unless Gitlab::Geo.geo_database_configured? - return 'Geo node has a database that is not configured for streaming replication with the primary node.' unless Gitlab::Database.db_read_only? - return 'Geo node does not appear to be replicating the database from the primary node.' if Gitlab::Database.pg_stat_wal_receiver_supported? && !streaming_active? - return "Current Geo database version (#{database_version}) does not match latest migration (#{migration_version}).\nYou may have to run `gitlab-rake geo:db:migrate` as root on the secondary." unless database_migration_version_match? + return 'Geo node has a database that is writable which is an indication it is not configured for replication with the primary node.' unless Gitlab::Database.db_read_only? + return 'Geo node does not appear to be replicating the database from the primary node.' if replication_enabled? && !replication_working? + return "Geo database version (#{database_version}) does not match latest migration (#{migration_version}).\nYou may have to run `gitlab-rake geo:db:migrate` as root on the secondary." unless database_migration_version_match? return 'Geo database is not configured to use Foreign Data Wrapper.' unless Gitlab::Geo::Fdw.enabled? unless Gitlab::Geo::Fdw.foreign_tables_up_to_date? @@ -105,12 +105,40 @@ def schema_tables_match? gitlab_schema_tables_count == foreign_schema_tables_count end - def streaming_active? - # Get a streaming status + def replication_enabled? + streaming_replication_enabled? || archive_recovery_replication_enabled? + end + + def archive_recovery_replication_enabled? + !streaming_replication_enabled? && some_replication_active? + end + + def streaming_replication_enabled? + !ActiveRecord::Base.connection + .execute("SELECT * FROM #{Gitlab::Database.pg_last_wal_receive_lsn}() as result") + .first['result'] + .nil? + end + + def some_replication_active? + # Is some sort of replication active? + !ActiveRecord::Base.connection + .execute("SELECT * FROM #{Gitlab::Database.pg_last_xact_replay_timestamp}() as result") + .first['result'] + .nil? + end + + def streaming_replication_active? # This only works for Postgresql 9.6 and greater ActiveRecord::Base.connection .select_values('SELECT pid FROM pg_stat_wal_receiver').first.to_i > 0 end + + def replication_working? + return streaming_replication_active? if streaming_replication_enabled? + + some_replication_active? + end end end end diff --git a/ee/spec/lib/gitlab/geo/health_check_spec.rb b/ee/spec/lib/gitlab/geo/health_check_spec.rb index 951643a49f4216..ada6204e57fddb 100644 --- a/ee/spec/lib/gitlab/geo/health_check_spec.rb +++ b/ee/spec/lib/gitlab/geo/health_check_spec.rb @@ -61,26 +61,48 @@ let(:db_read_only) { false } it 'returns an error' do - expect(subject.perform_checks).to include('Geo node has a database that is not configured for streaming replication with the primary node.') + expect(subject.perform_checks).to include('Geo node has a database that is writable which is an indication it is not configured for replication with the primary node.') end end context 'streaming replication' do + it 'returns an error when replication is not working' do + allow(Gitlab::Database).to receive(:pg_last_wal_receive_lsn).and_return('pg_last_xlog_receive_location') + allow(ActiveRecord::Base).to receive_message_chain('connection.execute').with(no_args).with('SELECT * FROM pg_last_xlog_receive_location() as result').and_return(['result' => 'fake']) + allow(ActiveRecord::Base).to receive_message_chain('connection.select_values').with(no_args).with('SELECT pid FROM pg_stat_wal_receiver').and_return([]) + + expect(subject.perform_checks).to match(/Geo node does not appear to be replicating the database from the primary node/) + end + end + + context 'archive recovery replication' do + it 'returns an error when replication is not working' do + allow(subject).to receive(:streaming_replication_enabled?).and_return(false) + allow(subject).to receive(:archive_recovery_replication_enabled?).and_return(true) + allow(Gitlab::Database).to receive(:pg_last_xact_replay_timestamp).and_return('pg_last_xact_replay_timestamp') + allow(ActiveRecord::Base).to receive_message_chain('connection.execute').with(no_args).with('SELECT * FROM pg_last_xact_replay_timestamp() as result').and_return([{ 'result' => nil }]) + + expect(subject.perform_checks).to match(/Geo node does not appear to be replicating the database from the primary node/) + end + end + + context 'some sort of replication' do before do - allow(Gitlab::Database).to receive(:pg_stat_wal_receiver_supported?).and_return(true) + allow(subject).to receive(:replication_enabled?).and_return(true) end - context 'that is supported but not working' do + context 'that is not working' do it 'returns an error' do - allow(ActiveRecord::Base).to receive_message_chain('connection.select_values').with(no_args).with('SELECT pid FROM pg_stat_wal_receiver').and_return([]) + allow(subject).to receive(:archive_recovery_replication_enabled?).and_return(false) + allow(subject).to receive(:streaming_replication_enabled?).and_return(false) expect(subject.perform_checks).to match(/Geo node does not appear to be replicating the database from the primary node/) end end - context 'that is supported and working' do + context 'that is working' do before do - allow(ActiveRecord::Base).to receive_message_chain('connection.select_values').with(no_args).with('SELECT pid FROM pg_stat_wal_receiver').and_return(['123']) + allow(subject).to receive(:replication_working?).and_return(true) allow(Gitlab::Geo::Fdw).to receive(:enabled?) { true } allow(Gitlab::Geo::Fdw).to receive(:foreign_tables_up_to_date?) { true } end -- GitLab