Commit d863326e authored by Rémy Coutable's avatar Rémy Coutable

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
parents 6374530d b3566a01
...@@ -4,8 +4,6 @@ class Key < ActiveRecord::Base ...@@ -4,8 +4,6 @@ class Key < ActiveRecord::Base
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
include Sortable include Sortable
LAST_USED_AT_REFRESH_TIME = 1.day.to_i
belongs_to :user belongs_to :user
before_validation :generate_fingerprint before_validation :generate_fingerprint
...@@ -54,10 +52,7 @@ class Key < ActiveRecord::Base ...@@ -54,10 +52,7 @@ class Key < ActiveRecord::Base
end end
def update_last_used_at def update_last_used_at
lease = Gitlab::ExclusiveLease.new("key_update_last_used_at:#{id}", timeout: LAST_USED_AT_REFRESH_TIME) Keys::LastUsedService.new(self).execute
return unless lease.try_obtain
UseKeyWorker.perform_async(id)
end end
def add_to_shell def add_to_shell
......
module Keys
class LastUsedService
TIMEOUT = 1.day.to_i
attr_reader :key
# key - The Key for which to update the last used timestamp.
def initialize(key)
@key = key
end
def execute
# We _only_ want to update last_used_at and not also updated_at (which
# would be updated when using #touch).
key.update_column(:last_used_at, Time.zone.now) if update?
end
def update?
last_used = key.last_used_at
return false if last_used && (Time.zone.now - last_used) <= TIMEOUT
!!redis_lease.try_obtain
end
private
def redis_lease
Gitlab::ExclusiveLease
.new("key_update_last_used_at:#{key.id}", timeout: TIMEOUT)
end
end
end
class UseKeyWorker
include Sidekiq::Worker
include DedicatedSidekiqQueue
def perform(key_id)
key = Key.find(key_id)
key.touch(:last_used_at)
rescue ActiveRecord::RecordNotFound
Rails.logger.error("UseKeyWorker: couldn't find key with ID=#{key_id}, skipping job")
false
end
end
---
title: Stop using Sidekiq for updating Key#last_used_at
merge_request:
author:
type: changed
...@@ -38,7 +38,6 @@ ...@@ -38,7 +38,6 @@
- [invalid_gpg_signature_update, 2] - [invalid_gpg_signature_update, 2]
- [create_gpg_signature, 2] - [create_gpg_signature, 2]
- [upload_checksum, 1] - [upload_checksum, 1]
- [use_key, 1]
- [repository_fork, 1] - [repository_fork, 1]
- [repository_import, 1] - [repository_import, 1]
- [project_service, 1] - [project_service, 1]
......
...@@ -37,30 +37,17 @@ describe Key, :mailer do ...@@ -37,30 +37,17 @@ describe Key, :mailer do
end end
describe "#update_last_used_at" do describe "#update_last_used_at" do
let(:key) { create(:key) } it 'updates the last used timestamp' do
key = build(:key)
context 'when key was not updated during the last day' do service = double(:service)
before do
allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) expect(Keys::LastUsedService).to receive(:new)
.and_return('000000') .with(key)
end .and_return(service)
it 'enqueues a UseKeyWorker job' do
expect(UseKeyWorker).to receive(:perform_async).with(key.id)
key.update_last_used_at
end
end
context 'when key was updated during the last day' do expect(service).to receive(:execute)
before do
allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
.and_return(false)
end
it 'does not enqueue a UseKeyWorker job' do key.update_last_used_at
expect(UseKeyWorker).not_to receive(:perform_async)
key.update_last_used_at
end
end end
end end
end end
......
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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment