Merge branch '2181-fix-secondary-node-settings-changed' into 'master'

Geo - Properly set tracking database connection and cron jobs on secondary node

Closes #2181 and #2289

See merge request !1987

app/models/geo/base_registry.rb

 class Geo::BaseRegistry < ActiveRecord::Base
   self.abstract_class = true
 
-  if Gitlab::Geo.configured? && (Gitlab::Geo.secondary? || Rails.env.test?)
+  if Gitlab::Geo.secondary_role_enabled?
     establish_connection Rails.configuration.geo_database
   end
 end

app/models/geo_node.rb

@@ -21,9 +21,7 @@ class GeoNode < ActiveRecord::Base
   validates :encrypted_secret_access_key, presence: true
 
   after_initialize :build_dependents
-  after_save :refresh_bulk_notify_worker_status
   after_save :expire_cache!
-  after_destroy :refresh_bulk_notify_worker_status
   after_destroy :expire_cache!
   before_validation :update_dependents_attributes
 
@@ -130,10 +128,6 @@ class GeoNode < ActiveRecord::Base
     { protocol: schema, host: host, port: port, script_name: relative_url }
   end
 
-  def refresh_bulk_notify_worker_status
-    Gitlab::Geo.configure_cron_jobs!
-  end
-
   def build_dependents
     unless persisted?
       self.build_geo_node_key if geo_node_key.nil?

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.configured?
+    return unless Gitlab::Geo.secondary_role_enabled?
     return unless Gitlab::Geo.secondary?
 
     @start_time = Time.now
@@ -153,7 +153,7 @@ class GeoFileDownloadDispatchWorker
 
   def node_enabled?
     # Only check every minute to avoid polling the DB excessively
-    unless @last_enabled_check.present? && (Time.now - @last_enabled_check > 1.minute)
+    unless @last_enabled_check.present? && @last_enabled_check > 1.minute.ago
       @last_enabled_check = Time.now
       @current_node_enabled = nil
     end

app/workers/geo_repository_sync_worker.rb

@@ -7,7 +7,7 @@ class GeoRepositorySyncWorker
   LAST_SYNC_INTERVAL = 24.hours
 
   def perform
-    return unless Gitlab::Geo.configured?
+    return unless Gitlab::Geo.secondary_role_enabled?
     return unless Gitlab::Geo.primary_node.present?
 
     start_time = Time.now
@@ -20,7 +20,7 @@ class GeoRepositorySyncWorker
     project_ids.each do |project_id|
       begin
         break if over_time?(start_time)
-        break unless Gitlab::Geo.current_node_enabled?
+        break unless node_enabled?
 
         # We try to obtain a lease here for the entire sync process because we
         # want to sync the repositories continuously at a controlled rate
@@ -73,6 +73,16 @@ class GeoRepositorySyncWorker
     Time.now - start_time >= RUN_TIME
   end
 
+  def node_enabled?
+    # Only check every minute to avoid polling the DB excessively
+    unless @last_enabled_check.present? && @last_enabled_check > 1.minute.ago
+      @last_enabled_check = Time.now
+      @current_node_enabled = nil
+    end
+
+    @current_node_enabled ||= Gitlab::Geo.current_node_enabled?
+  end
+
   def try_obtain_lease
     lease = Gitlab::ExclusiveLease.new(lease_key, timeout: lease_timeout).try_obtain
 

changelogs/unreleased-ee/2181-fix-secondary-node-settings-changed.yml

+---
+title: Geo - Properly set tracking database connection and cron jobs on secondary nodes
+merge_request:
+author:

config/gitlab.yml.example

@@ -622,6 +622,12 @@ production: &base
     #   host: localhost
     #   port: 3808
 
+  ## GitLab Geo settings (EE-only)
+  geo_primary_role:
+    enabled: false
+  geo_secondary_role:
+    enabled: false
+
   #
   # 5. Extra customization
   # ==========================
@@ -705,6 +711,10 @@ test:
         user_filter: ''
         group_base: 'ou=groups,dc=example,dc=com'
         admin_group: ''
+  geo_primary_role:
+    enabled: true
+  geo_secondary_role:
+    enabled: true
 
 staging:
   <<: *base

config/initializers/1_settings.rb

@@ -342,6 +342,10 @@ 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'))
+if Gitlab::Geo.secondary_role_enabled?
   Rails.application.configure do
     config.geo_database = config_for(:database_geo)
   end

lib/api/geo.rb

@@ -35,7 +35,6 @@ module API
       get 'status' do
         authenticate_by_gitlab_geo_node_token!
         require_node_to_be_secondary!
-        require_node_to_have_tracking_db!
 
         present GeoNodeStatus.new(id: Gitlab::Geo.current_node.id), with: Entities::GeoNodeStatus
       end
@@ -110,10 +109,6 @@ module API
       def require_node_to_be_secondary!
         forbidden! 'Geo node is not secondary node.' unless Gitlab::Geo.current_node&.secondary?
       end
-
-      def require_node_to_have_tracking_db!
-        not_found! 'Geo node does not have its tracking database enabled.' unless Gitlab::Geo.configured?
-      end
     end
   end
 end

lib/gitlab/geo.rb

@@ -42,8 +42,12 @@ module Gitlab
       Gitlab::Geo.current_node.reload.enabled?
     end
 
-    def self.configured?
-      Rails.configuration.respond_to?(:geo_database)
+    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.license_allows?
@@ -94,9 +98,9 @@ module Gitlab
     end
 
     def self.configure_cron_jobs!
-      if self.primary?
+      if self.primary_role_enabled?
         self.configure_primary_jobs!
-      elsif self.secondary?
+      elsif self.secondary_role_enabled?
         self.configure_secondary_jobs!
       else
         self.disable_all_jobs!

lib/gitlab/geo/health_check.rb

@@ -3,7 +3,8 @@ module Gitlab
     class HealthCheck
       def self.perform_checks
         return '' unless Gitlab::Geo.secondary?
-        return 'The Geo database configuration file is missing.' unless Gitlab::Geo.configured?
+        return 'The Geo secondary role is disabled.' unless Gitlab::Geo.secondary_role_enabled?
+        return 'The Geo database configuration file is missing.' unless self.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?
 
         database_version  = self.get_database_version.to_i
@@ -57,6 +58,10 @@ module Gitlab
           .first
           .fetch('pg_is_in_recovery') == 't'
       end
+
+      def self.geo_database_configured?
+        Rails.configuration.respond_to?(:geo_database)
+      end
     end
   end
 end

spec/lib/gitlab/geo/health_check_spec.rb

@@ -16,6 +16,13 @@ describe Gitlab::Geo::HealthCheck 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 }

spec/lib/gitlab/geo_spec.rb

@@ -127,7 +127,9 @@ describe Gitlab::Geo, lib: true do
     end
 
     it 'activates cron jobs for primary' do
-      allow(described_class).to receive(:primary?).and_return(true)
+      allow(described_class).to receive(:primary_role_enabled?).and_return(true)
+      allow(described_class).to receive(:secondary_role_enabled?).and_return(false)
+
       described_class.configure_cron_jobs!
 
       expect(described_class.bulk_notify_job).to be_enabled
@@ -136,7 +138,9 @@ describe Gitlab::Geo, lib: true do
     end
 
     it 'activates cron jobs for secondary' do
-      allow(described_class).to receive(:secondary?).and_return(true)
+      allow(described_class).to receive(:primary_role_enabled?).and_return(false)
+      allow(described_class).to receive(:secondary_role_enabled?).and_return(true)
+
       described_class.configure_cron_jobs!
 
       expect(described_class.bulk_notify_job).not_to be_enabled
@@ -145,6 +149,9 @@ 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)
+
       described_class.configure_cron_jobs!
 
       expect(described_class.bulk_notify_job).not_to be_enabled

spec/requests/api/geo_spec.rb

@@ -307,14 +307,6 @@ describe API::Geo, api: true do
         expect(response).to have_http_status(200)
         expect(response).to match_response_schema('geo_node_status')
       end
-
-      it 'responds with a 404 when the tracking database is disabled' do
-        allow(Gitlab::Geo).to receive(:configured?).and_return(false)
-
-        get api('/geo/status'), nil, request.headers
-
-        expect(response).to have_http_status(404)
-      end
     end
 
     context 'when requesting primary node with valid auth header' do

spec/workers/geo_file_download_dispatch_worker_spec.rb

@@ -13,10 +13,10 @@ describe GeoFileDownloadDispatchWorker do
   subject { described_class.new }
 
   describe '#perform' do
-    it 'does not schedule anything when tracking DB is not available' do
+    it 'does not schedule anything when secondary role is disabled' do
       create(:lfs_object, :with_file)
 
-      allow(Rails.configuration).to receive(:respond_to?).with(:geo_database) { false }
+      allow(Gitlab::Geo).to receive(:secondary_role_enabled?) { false }
 
       expect(GeoFileDownloadWorker).not_to receive(:perform_async)
 

spec/workers/geo_repository_sync_worker_spec.rb

@@ -52,8 +52,8 @@ describe GeoRepositorySyncWorker do
       subject.perform
     end
 
-    it 'does not perform Geo::RepositorySyncService when tracking DB is not available' do
-      allow(Rails.configuration).to receive(:respond_to?).with(:geo_database) { false }
+    it 'does not perform Geo::RepositorySyncService when secondary role is disabled' do
+      allow(Gitlab::Geo).to receive(:secondary_role_enabled?) { false }
 
       expect(Geo::RepositorySyncService).not_to receive(:new)