Merge branch '2826-remove-geo-roles' into 'master'

Geo: Implement alternative to geo_{primary|secondary}_role in gitlab.yml

Closes #2826

See merge request !2352

app/models/geo/base_registry.rb

 class Geo::BaseRegistry < ActiveRecord::Base
   self.abstract_class = true
 
-  if Gitlab::Geo.secondary_role_enabled? && Gitlab::Geo.geo_database_configured?
+  if Gitlab::Geo.geo_database_configured?
     establish_connection Rails.configuration.geo_database
   end
+
+  def self.connection
+    raise 'Geo secondary database is not configured' unless Gitlab::Geo.geo_database_configured?
+
+    super
+  end
 end

app/workers/geo_file_download_dispatch_worker.rb

@@ -22,7 +22,7 @@ class GeoFileDownloadDispatchWorker
   #    files, excluding ones in progress.
   # 5. Quit when we have scheduled all downloads or exceeded an hour.
   def perform
-    return unless Gitlab::Geo.secondary_role_enabled?
+    return unless Gitlab::Geo.geo_database_configured?
     return unless Gitlab::Geo.secondary?
 
     @start_time = Time.now

app/workers/geo_repository_sync_worker.rb

@@ -15,7 +15,7 @@ class GeoRepositorySyncWorker
   end
 
   def perform
-    return unless Gitlab::Geo.secondary_role_enabled?
+    return unless Gitlab::Geo.geo_database_configured?
     return unless Gitlab::Geo.primary_node.present?
 
     logger.info "Started Geo repository sync scheduler"

changelogs/unreleased-ee/2826-remove-geo-roles.yml

+---
+title: 'Geo: Implement alternative to geo_{primary|secondary}_role in gitlab.yml'
+merge_request: 2352
+author:

config/gitlab.yml.example

@@ -644,12 +644,6 @@ production: &base
     ip_whitelist:
       - 127.0.0.0/8
 
-  ## GitLab Geo settings (EE-only)
-  geo_primary_role:
-    enabled: false
-  geo_secondary_role:
-    enabled: false
-
   #
   # 5. Extra customization
   # ==========================
@@ -780,10 +774,6 @@ test:
         user_filter: ''
         group_base: 'ou=groups,dc=example,dc=com'
         admin_group: ''
-  geo_primary_role:
-    enabled: true
-  geo_secondary_role:
-    enabled: false
 
 staging:
   <<: *base

config/initializers/1_settings.rb

@@ -349,10 +349,6 @@ Settings.pages['external_https']  ||= false unless Settings.pages['external_http
 # Geo
 #
 Settings.gitlab['geo_status_timeout'] ||= 10
-Settings['geo_primary_role'] ||= Settingslogic.new({})
-Settings.geo_primary_role['enabled'] = false if Settings.geo_primary_role['enabled'].nil?
-Settings['geo_secondary_role'] ||= Settingslogic.new({})
-Settings.geo_secondary_role['enabled'] = false if Settings.geo_secondary_role['enabled'].nil?
 
 #
 # Git LFS

config/initializers/geo.rb

-if File.exist?(Rails.root.join('config/database_geo.yml')) &&
-    Gitlab::Geo.secondary_role_enabled?
+if File.exist?(Rails.root.join('config/database_geo.yml'))
   Rails.application.configure do
     config.geo_database = config_for(:database_geo)
   end
 end
 
 begin
-  # Avoid using the database if this is run in a Rake task
-  if Gitlab::Geo.primary_role_enabled?
+  if Gitlab::Geo.primary?
     Gitlab::Geo.current_node&.update_clone_url!
   end
 rescue => e

lib/gitlab/geo.rb

@@ -31,7 +31,12 @@ module Gitlab
     end
 
     def self.enabled?
-      self.cache_value(:geo_node_enabled) { GeoNode.exists? }
+      GeoNode.connected? && self.cache_value(:geo_node_enabled) { GeoNode.exists? }
+    rescue => e
+      # We can't use the actual classes in rescue because we load only one of them based on database supported
+      raise e unless %w(PG::UndefinedTable Mysql2::Error).include? e.class.name
+
+      false
     end
 
     def self.current_node_enabled?
@@ -41,14 +46,6 @@ module Gitlab
       Gitlab::Geo.current_node.reload.enabled?
     end
 
-    def self.primary_role_enabled?
-      Gitlab.config.geo_primary_role['enabled']
-    end
-
-    def self.secondary_role_enabled?
-      Gitlab.config.geo_secondary_role['enabled']
-    end
-
     def self.geo_database_configured?
       Rails.configuration.respond_to?(:geo_database)
     end
@@ -113,9 +110,9 @@ module Gitlab
     end
 
     def self.configure_cron_jobs!
-      if self.primary_role_enabled?
+      if self.primary?
         self.configure_primary_jobs!
-      elsif self.secondary_role_enabled?
+      elsif self.secondary?
         self.configure_secondary_jobs!
       else
         self.enable_all_cron_jobs!

lib/gitlab/geo/health_check.rb

@@ -5,7 +5,6 @@ module Gitlab
         raise NotImplementedError.new('Geo is only compatible with PostgreSQL') unless Gitlab::Database.postgresql?
 
         return '' unless Gitlab::Geo.secondary?
-        return 'The Geo secondary role is disabled.' unless Gitlab::Geo.secondary_role_enabled?
         return 'The Geo database configuration file is missing.' unless Gitlab::Geo.geo_database_configured?
         return 'The Geo node has a database that is not configured for streaming replication with the primary node.' unless self.database_secondary?
 

scripts/prepare_build.sh

@@ -67,6 +67,3 @@ if [ "$SETUP_DB" != "false" ]; then
     # EE-only
     bundle exec rake geo:db:drop geo:db:create geo:db:schema:load geo:db:migrate
 fi
-
-# EE-only
-sed -i -e '/geo_secondary_role\:/ {' -e 'n; s/enabled\: false/enabled\: true/' -e '}' config/gitlab.yml

spec/lib/gitlab/geo/health_check_spec.rb

@@ -8,7 +8,6 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
   describe '.perform_checks' do
     it 'returns a string if database is not fully migrated' do
       allow(Gitlab::Geo).to receive(:secondary?) { true }
-      allow(Gitlab::Geo).to receive(:secondary_role_enabled?).and_return(true)
       allow(described_class).to receive(:geo_database_configured?).and_return(true)
       allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(described_class).to receive(:get_database_version).and_return('20170101')
@@ -26,37 +25,32 @@ describe Gitlab::Geo::HealthCheck, :postgresql do
       expect(subject.perform_checks).to be_blank
     end
 
-    it 'returns an error when secondary role is disabled' do
-      allow(Gitlab::Geo).to receive(:secondary?) { true }
-      allow(Gitlab::Geo).to receive(:secondary_role_enabled?).and_return(false)
-
-      expect(subject.perform_checks).not_to be_blank
-    end
-
     it 'returns an error when database is not configured for streaming replication' do
       allow(Gitlab::Geo).to receive(:secondary?) { true }
       allow(Gitlab::Database).to receive(:postgresql?) { true }
       allow(ActiveRecord::Base).to receive_message_chain(:connection, :execute, :first, :fetch) { 'f' }
 
-      expect(subject.perform_checks).not_to be_blank
+      expect(subject.perform_checks).to include('not configured for streaming replication')
     end
 
     it 'returns an error when configuration file is missing for tracking DB' do
       allow(Rails.configuration).to receive(:respond_to?).with(:geo_database) { false }
 
-      expect(subject.perform_checks).not_to be_blank
+      expect(subject.perform_checks).to include('database configuration file is missing')
     end
 
     it 'returns an error when Geo database version does not match the latest migration version' do
+      allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(subject).to receive(:get_database_version) { 1 }
 
-      expect(subject.perform_checks).not_to be_blank
+      expect(subject.perform_checks).to match(/Current Geo database version \([0-9]+\) does not match latest migration \([0-9]+\)/)
     end
 
     it 'returns an error when latest migration version does not match the Geo database version' do
+      allow(described_class).to receive(:database_secondary?).and_return(true)
       allow(subject).to receive(:get_migration_version) { 1 }
 
-      expect(subject.perform_checks).not_to be_blank
+      expect(subject.perform_checks).to match(/Current Geo database version \([0-9]+\) does not match latest migration \([0-9]+\)/)
     end
   end
 

spec/lib/gitlab/geo_spec.rb

@@ -25,6 +25,43 @@ describe Gitlab::Geo, lib: true do
     end
   end
 
+  describe 'primary?' do
+    context 'when current node is a primary node' do
+      before(:each) do
+        primary_node
+      end
+
+      it 'returns true' do
+        expect(described_class.primary?).to be_truthy
+      end
+
+      it 'returns false when GeoNode is disabled' do
+        allow(described_class).to receive(:enabled?) { false }
+
+        expect(described_class.primary?).to be_falsey
+      end
+    end
+  end
+
+  describe 'secondary?' do
+    context 'when current node is a secondary node' do
+      before(:each) do
+        secondary_node
+        allow(described_class).to receive(:current_node) { secondary_node }
+      end
+
+      it 'returns true' do
+        expect(described_class.secondary?).to be_truthy
+      end
+
+      it 'returns false when GeoNode is disabled' do
+        allow(described_class).to receive(:enabled?) { false }
+
+        expect(described_class.secondary?).to be_falsey
+      end
+    end
+  end
+
   describe 'enabled?' do
     context 'when any GeoNode exists' do
       before do
@@ -42,6 +79,24 @@ describe Gitlab::Geo, lib: true do
       end
     end
 
+    context 'when there is a database issue' do
+      it 'returns false when database connection is down' do
+        allow(GeoNode).to receive(:connected?) { false }
+
+        expect(described_class.enabled?).to be_falsey
+      end
+
+      it 'returns false when database schema does not contain required tables' do
+        if Gitlab::Database.mysql?
+          allow(GeoNode).to receive(:exists?).and_raise(Mysql2::Error, "Table 'gitlabhq_test.geo_nodes' doesn't exist: SHOW FULL FIELDS FROM `geo_nodes`")
+        else
+          allow(GeoNode).to receive(:exists?).and_raise(PG::UndefinedTable)
+        end
+
+        expect(described_class.enabled?).to be_falsey
+      end
+    end
+
     context 'with RequestStore enabled' do
       before do
         RequestStore.begin!
@@ -142,8 +197,8 @@ describe Gitlab::Geo, lib: true do
     end
 
     it 'activates cron jobs for primary' do
-      allow(described_class).to receive(:primary_role_enabled?).and_return(true)
-      allow(described_class).to receive(:secondary_role_enabled?).and_return(false)
+      allow(described_class).to receive(:primary?).and_return(true)
+      allow(described_class).to receive(:secondary?).and_return(false)
 
       described_class.configure_cron_jobs!
 
@@ -154,8 +209,8 @@ describe Gitlab::Geo, lib: true do
     end
 
     it 'activates cron jobs for secondary' do
-      allow(described_class).to receive(:primary_role_enabled?).and_return(false)
-      allow(described_class).to receive(:secondary_role_enabled?).and_return(true)
+      allow(described_class).to receive(:primary?).and_return(false)
+      allow(described_class).to receive(:secondary?).and_return(true)
 
       described_class.configure_cron_jobs!
 
@@ -166,8 +221,8 @@ describe Gitlab::Geo, lib: true do
     end
 
     it 'deactivates all jobs when Geo is not active' do
-      allow(described_class).to receive(:primary_role_enabled?).and_return(false)
-      allow(described_class).to receive(:secondary_role_enabled?).and_return(false)
+      allow(described_class).to receive(:primary?).and_return(false)
+      allow(described_class).to receive(:secondary?).and_return(false)
 
       described_class.configure_cron_jobs!
 
@@ -178,14 +233,14 @@ describe Gitlab::Geo, lib: true do
     end
 
     it 'reactivates cron jobs when node turns off Geo' do
-      allow(described_class).to receive(:primary_role_enabled?).and_return(false)
-      allow(described_class).to receive(:secondary_role_enabled?).and_return(true)
+      allow(described_class).to receive(:primary?).and_return(false)
+      allow(described_class).to receive(:secondary?).and_return(true)
 
       described_class.configure_cron_jobs!
       expect(Sidekiq::Cron::Job.find('ldap_test')).not_to be_enabled
 
-      allow(described_class).to receive(:primary_role_enabled?).and_return(false)
-      allow(described_class).to receive(:secondary_role_enabled?).and_return(false)
+      allow(described_class).to receive(:primary?).and_return(false)
+      allow(described_class).to receive(:secondary?).and_return(false)
 
       described_class.configure_cron_jobs!
       expect(Sidekiq::Cron::Job.find('ldap_test')).to be_enabled

spec/support/db_cleaner.rb

@@ -34,7 +34,7 @@ RSpec.configure do |config|
   end
 
   def setup_database_cleaner
-    if Gitlab::Geo.secondary_role_enabled?
+    if Gitlab::Geo.geo_database_configured?
       DatabaseCleaner[:active_record, { connection: Geo::BaseRegistry }]
     end
 

spec/workers/geo_file_download_dispatch_worker_spec.rb

@@ -16,7 +16,7 @@ describe GeoFileDownloadDispatchWorker do
     it 'does not schedule anything when secondary role is disabled' do
       create(:lfs_object, :with_file)
 
-      allow(Gitlab::Geo).to receive(:secondary_role_enabled?) { false }
+      allow(Gitlab::Geo).to receive(:geo_database_configured?) { false }
 
       expect(GeoFileDownloadWorker).not_to receive(:perform_async)
 

spec/workers/geo_repository_sync_worker_spec.rb

@@ -38,8 +38,8 @@ describe GeoRepositorySyncWorker do
       subject.perform
     end
 
-    it 'does not perform Geo::ProjectSyncWorker when secondary role is disabled' do
-      allow(Gitlab::Geo).to receive(:secondary_role_enabled?) { false }
+    it 'does not perform Geo::ProjectSyncWorker when no geo database is configured' do
+      allow(Gitlab::Geo).to receive(:geo_database_configured?) { false }
 
       expect(Geo::ProjectSyncWorker).not_to receive(:perform_in)