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

If a secondary starts up before it has been made a secondary, the Sidekiq initializer
would disable the backfill and file transfer cron jobs. This state persists indefinitely
until either the entire Redis database is flushed or the jobs are manually activated.
To fix this, we activate the jobs at startup if Geo is being used.

Closes #2289

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.map { |job| self.send(job).try(:enable!) }
+      SECONDARY_JOBS.map { |job| self.send(job).try(:disable!) }
+    end
+
+    def self.configure_secondary_jobs!
+      PRIMARY_JOBS.map { |job| self.send(job).try(:disable!) }
+      SECONDARY_JOBS.map { |job| self.send(job).try(:enable!) }
+    end
+
+    def self.disable_all_jobs!
+      PRIMARY_JOBS.map { |job| self.send(job).try(:disable!) }
+      SECONDARY_JOBS.map { |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.enabled?).to be_truthy
+      expect(described_class.backfill_job.enabled?).to be_falsey
+      expect(described_class.file_download_job.enabled?).to be_falsey
+    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.enabled?).to be_falsey
+      expect(described_class.backfill_job.enabled?).to be_truthy
+      expect(described_class.file_download_job.enabled?).to be_truthy
+    end
+
+    it 'deactivates all jobs when Geo is not active' do
+      described_class.configure_cron_jobs!
+
+      expect(described_class.bulk_notify_job.enabled?).to be_falsey
+      expect(described_class.backfill_job.enabled?).to be_falsey
+      expect(described_class.file_download_job.enabled?).to be_falsey
+    end
+  end
 end