Merge branch 'sh-fix-geo-secondary-cron' into 'master'

Fix bug where Geo secondary Sidekiq cron jobs would not be activated if settings changed

Closes #2289

See merge request !1754

app/models/geo_node.rb

@@ -115,11 +115,7 @@ class GeoNode < ActiveRecord::Base
   end
 
   def refresh_bulk_notify_worker_status
-    if Gitlab::Geo.primary?
-      Gitlab::Geo.bulk_notify_job.try(:enable!)
-    else
-      Gitlab::Geo.bulk_notify_job.try(:disable!)
-    end
+    Gitlab::Geo.configure_cron_jobs!
   end
 
   def build_dependents

changelogs/unreleased-ee/sh-fix-geo-secondary-cron.yml

+---
+title: Fix bug where Geo secondary Sidekiq cron jobs would not be activated if settings
+  changed
+merge_request:
+author:

config/initializers/sidekiq.rb

@@ -42,14 +42,7 @@ Sidekiq.configure_server do |config|
 
   Gitlab::Mirror.configure_cron_jobs!
 
-  # Gitlab Geo: enable bulk notify job only on primary node
-  Gitlab::Geo.bulk_notify_job.disable! unless Gitlab::Geo.primary?
-
-  # GitLab Geo: enable backfill job only on secondary nodes
-  Gitlab::Geo.backfill_job.disable! unless Gitlab::Geo.secondary?
-
-  # GitLab Geo: enable backfill job only on secondary nodes
-  Gitlab::Geo.file_download_job.disable! unless Gitlab::Geo.secondary?
+  Gitlab::Geo.configure_cron_jobs!
 
   Gitlab::SidekiqThrottler.execute!
 

lib/gitlab/geo.rb

@@ -2,7 +2,7 @@ module Gitlab
   module Geo
     OauthApplicationUndefinedError = Class.new(StandardError)
 
-    CACHE_KEYS = %i[
+    CACHE_KEYS = %i(
       geo_primary_node
       geo_secondary_nodes
       geo_node_enabled
@@ -10,7 +10,10 @@ module Gitlab
       geo_node_secondary
       geo_primary_ssh_path_prefix
       geo_oauth_application
-    ].freeze
+    ).freeze
+
+    PRIMARY_JOBS = %i(bulk_notify_job).freeze
+    SECONDARY_JOBS = %i(backfill_job file_download_job).freeze
 
     def self.current_node
       self.cache_value(:geo_node_current) do
@@ -79,6 +82,31 @@ module Gitlab
       Sidekiq::Cron::Job.find('geo_download_dispatch_worker')
     end
 
+    def self.configure_primary_jobs!
+      PRIMARY_JOBS.each { |job| self.send(job).try(:enable!) }
+      SECONDARY_JOBS.each { |job| self.send(job).try(:disable!) }
+    end
+
+    def self.configure_secondary_jobs!
+      PRIMARY_JOBS.each { |job| self.send(job).try(:disable!) }
+      SECONDARY_JOBS.each { |job| self.send(job).try(:enable!) }
+    end
+
+    def self.disable_all_jobs!
+      PRIMARY_JOBS.each { |job| self.send(job).try(:disable!) }
+      SECONDARY_JOBS.each { |job| self.send(job).try(:disable!) }
+    end
+
+    def self.configure_cron_jobs!
+      if self.primary?
+        self.configure_primary_jobs!
+      elsif self.secondary?
+        self.configure_secondary_jobs!
+      else
+        self.disable_all_jobs!
+      end
+    end
+
     def self.oauth_authentication
       return false unless Gitlab::Geo.secondary?
 

spec/lib/gitlab/geo_spec.rb

@@ -111,4 +111,48 @@ describe Gitlab::Geo, lib: true do
       expect(keys[:secret_access_key].length).to eq(40)
     end
   end
+
+  describe '.configure_cron_jobs!' do
+    def init_cron_job(job_name, class_name)
+      Sidekiq::Cron::Job.create(
+        name: job_name,
+        cron: '0 * * * *',
+        class: class_name
+      )
+    end
+
+    before(:all) do
+      jobs = %w(geo_bulk_notify_worker geo_backfill_worker)
+
+      jobs.each { |job| init_cron_job(job, job.camelize) }
+      # TODO: Make this name consistent
+      init_cron_job('geo_download_dispatch_worker', 'GeoFileDownloadDispatchWorker')
+    end
+
+    it 'activates cron jobs for primary' do
+      allow(described_class).to receive(:primary?).and_return(true)
+      described_class.configure_cron_jobs!
+
+      expect(described_class.bulk_notify_job).to be_enabled
+      expect(described_class.backfill_job).not_to be_enabled
+      expect(described_class.file_download_job).not_to be_enabled
+    end
+
+    it 'activates cron jobs for secondary' do
+      allow(described_class).to receive(:secondary?).and_return(true)
+      described_class.configure_cron_jobs!
+
+      expect(described_class.bulk_notify_job).not_to be_enabled
+      expect(described_class.backfill_job).to be_enabled
+      expect(described_class.file_download_job).to be_enabled
+    end
+
+    it 'deactivates all jobs when Geo is not active' do
+      described_class.configure_cron_jobs!
+
+      expect(described_class.bulk_notify_job).not_to be_enabled
+      expect(described_class.backfill_job).not_to be_enabled
+      expect(described_class.file_download_job).not_to be_enabled
+    end
+  end
 end