Merge branch 'remove-use-key-worker' into 'master'

Stop using Sidekiq for updating Key#last_used_at

Closes #36663

See merge request gitlab-org/gitlab-ce!14391

3 files changed, 77 insertions, 45 deletions

diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index dbc4aba8547..8eabc4ca72f 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -37,30 +37,17 @@ describe Key, :mailer do
     end
 
     describe "#update_last_used_at" do
-      let(:key) { create(:key) }
-
-      context 'when key was not updated during the last day' do
-        before do
-          allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-            .and_return('000000')
-        end
-
-        it 'enqueues a UseKeyWorker job' do
-          expect(UseKeyWorker).to receive(:perform_async).with(key.id)
-          key.update_last_used_at
-        end
-      end
+      it 'updates the last used timestamp' do
+        key = build(:key)
+        service = double(:service)
+
+        expect(Keys::LastUsedService).to receive(:new)
+          .with(key)
+          .and_return(service)
 
-      context 'when key was updated during the last day' do
-        before do
-          allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-            .and_return(false)
-        end
+        expect(service).to receive(:execute)
 
-        it 'does not enqueue a UseKeyWorker job' do
-          expect(UseKeyWorker).not_to receive(:perform_async)
-          key.update_last_used_at
-        end
+        key.update_last_used_at
       end
     end
   end
diff --git a/spec/services/keys/last_used_service_spec.rb b/spec/services/keys/last_used_service_spec.rb
new file mode 100644
index 00000000000..edaace293ae
--- /dev/null
+++ b/spec/services/keys/last_used_service_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe Keys::LastUsedService do
+  describe '#execute', :clean_gitlab_redis_shared_state do
+    it 'updates the key when it has not been used recently' do
+      key = create(:key, last_used_at: 1.year.ago)
+      time = Time.zone.now
+
+      Timecop.freeze(time) { described_class.new(key).execute }
+
+      expect(key.last_used_at).to eq(time)
+    end
+
+    it 'does not update the key when it has been used recently' do
+      time = 1.minute.ago
+      key = create(:key, last_used_at: time)
+
+      described_class.new(key).execute
+
+      expect(key.last_used_at).to eq(time)
+    end
+
+    it 'does not update the updated_at field' do
+      # Since a lot of these updates could happen in parallel for different keys
+      # we want these updates to be as lightweight as possible, hence we want to
+      # make sure we _only_ update last_used_at and not always updated_at.
+      key = create(:key, last_used_at: 1.year.ago)
+
+      recorder = ActiveRecord::QueryRecorder.new do
+        described_class.new(key).execute
+      end
+
+      expect(recorder.count).to eq(1)
+      expect(recorder.log[0]).not_to include('updated_at')
+    end
+  end
+
+  describe '#update?', :clean_gitlab_redis_shared_state do
+    it 'returns true when no last used timestamp is present' do
+      key = build(:key, last_used_at: nil)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(true)
+    end
+
+    it 'returns true when the key needs to be updated' do
+      key = build(:key, last_used_at: 1.year.ago)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(true)
+    end
+
+    it 'returns false when a lease has already been obtained' do
+      key = build(:key, last_used_at: 1.year.ago)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(true)
+      expect(service.update?).to eq(false)
+    end
+
+    it 'returns false when the key does not yet need to be updated' do
+      key = build(:key, last_used_at: 1.minute.ago)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(false)
+    end
+  end
+end
diff --git a/spec/workers/use_key_worker_spec.rb b/spec/workers/use_key_worker_spec.rb
deleted file mode 100644
index e50c788b82a..00000000000
--- a/spec/workers/use_key_worker_spec.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-require 'spec_helper'
-
-describe UseKeyWorker do
-  describe "#perform" do
-    it "updates the key's last_used_at attribute to the current time when it exists" do
-      worker = described_class.new
-      key = create(:key)
-      current_time = Time.zone.now
-
-      Timecop.freeze(current_time) do
-        expect { worker.perform(key.id) }
-          .to change { key.reload.last_used_at }.from(nil).to be_like_time(current_time)
-      end
-    end
-
-    it "returns false and skips the job when the key doesn't exist" do
-      worker = described_class.new
-      key = create(:key)
-
-      expect(worker.perform(key.id + 1)).to eq false
-    end
-  end
-end