From a7f5f6cbe50aee0a30ec5e046a9d7a20d4be3f96 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 18 Feb 2017 10:30:05 -0800
Subject: [PATCH] Fix GeoBulkNotifyWorker from firing too often

There were two problems:

1. The crontab entry was not actually correct

2. Multiple Sidekiq workers could fire the same Sidekiq-cron worker, causing
   redundant updates.

Closes gitlab-org/gitlab-ee#1758
---
 app/workers/geo_bulk_notify_worker.rb       | 13 +++++++++++++
 config/initializers/1_settings.rb           |  2 +-
 spec/workers/geo_bulk_notify_worker_spec.rb | 21 +++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 spec/workers/geo_bulk_notify_worker_spec.rb

diff --git a/app/workers/geo_bulk_notify_worker.rb b/app/workers/geo_bulk_notify_worker.rb
index a58ffc1a826f9e..0ba67af964882f 100644
--- a/app/workers/geo_bulk_notify_worker.rb
+++ b/app/workers/geo_bulk_notify_worker.rb
@@ -2,7 +2,20 @@ class GeoBulkNotifyWorker
   include Sidekiq::Worker
   include CronjobQueue
 
+  # This crontab entry fires every 10 minutes, so choose a time less than that
+  # to handle any time differences between Sidekiq workers
+  LEASE_TIMEOUT = 2.minutes
+
   def perform
+    return unless try_obtain_lease
+
     Geo::NotifyNodesService.new.execute
   end
+
+  private
+
+  def try_obtain_lease
+    lease = ::Gitlab::ExclusiveLease.new("geo_bulk_notify_worker", timeout: LEASE_TIMEOUT)
+    lease.try_obtain
+  end
 end
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 3df5794ddc0254..16cb830f1cfa68 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -376,7 +376,7 @@ def cron_random_weekly_time
 Settings.cron_jobs['ldap_group_sync_worker']['cron'] ||= '0 * * * *'
 Settings.cron_jobs['ldap_group_sync_worker']['job_class'] = 'LdapGroupSyncWorker'
 Settings.cron_jobs['geo_bulk_notify_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['geo_bulk_notify_worker']['cron'] ||= '*/10 * * * * *'
+Settings.cron_jobs['geo_bulk_notify_worker']['cron'] ||= '*/10 * * * *'
 Settings.cron_jobs['geo_bulk_notify_worker']['job_class'] ||= 'GeoBulkNotifyWorker'
 Settings.cron_jobs['gitlab_usage_ping_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['gitlab_usage_ping_worker']['cron'] ||= Settings.send(:cron_random_weekly_time)
diff --git a/spec/workers/geo_bulk_notify_worker_spec.rb b/spec/workers/geo_bulk_notify_worker_spec.rb
new file mode 100644
index 00000000000000..73867fadbd10db
--- /dev/null
+++ b/spec/workers/geo_bulk_notify_worker_spec.rb
@@ -0,0 +1,21 @@
+require 'rails_helper'
+
+describe GeoBulkNotifyWorker do
+  describe '#perform' do
+    it 'executes Geo::NotifyNodesService if can obtain a lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(true)
+      expect_any_instance_of(Geo::NotifyNodesService).to receive(:execute)
+
+      described_class.new.perform
+    end
+
+    it 'just returns if cannot obtain a lease' do
+      allow_any_instance_of(Gitlab::ExclusiveLease)
+        .to receive(:try_obtain).and_return(false)
+      expect_any_instance_of(Geo::NotifyNodesService).not_to receive(:execute)
+
+      described_class.new.perform
+    end
+  end
+end
-- 
GitLab