Commit b3566a01 authored by Yorick Peterse's avatar Yorick Peterse

Stop using Sidekiq for updating Key#last_used_at

This makes things simpler as no scheduling is involved. Further we
remove the need for running a SELECT + UPDATE just to get the key and
update it, whereas we only need an UPDATE when setting last_used_at
directly in a request.

The added service class takes care of updating Key#last_used_at without
using Sidekiq. Further it makes sure we only try to obtain a Redis lease
if we're confident that we actually need to do so, instead of always
obtaining it. We also make sure to _only_ update last_used_at instead of
also updating updated_at.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36663
parent a09d032b
......@@ -4,8 +4,6 @@ class Key < ActiveRecord::Base
include Gitlab::CurrentSettings
include Sortable
LAST_USED_AT_REFRESH_TIME = 1.day.to_i
belongs_to :user
before_validation :generate_fingerprint
......@@ -54,10 +52,7 @@ class Key < ActiveRecord::Base
end
def update_last_used_at
lease = Gitlab::ExclusiveLease.new("key_update_last_used_at:#{id}", timeout: LAST_USED_AT_REFRESH_TIME)
return unless lease.try_obtain
UseKeyWorker.perform_async(id)
Keys::LastUsedService.new(self).execute
end
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 @@
- [invalid_gpg_signature_update, 2]
- [create_gpg_signature, 2]
- [upload_checksum, 1]
- [use_key, 1]
- [repository_fork, 1]
- [repository_import, 1]
- [project_service, 1]
......
......@@ -37,33 +37,20 @@ 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 'updates the last used timestamp' do
key = build(:key)
service = double(:service)
it 'enqueues a UseKeyWorker job' do
expect(UseKeyWorker).to receive(:perform_async).with(key.id)
key.update_last_used_at
end
end
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
end
end
end
context "validation of uniqueness (based on fingerprint uniqueness)" do
let(:user) { create(:user) }
......
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