Commit 16687ae7 authored by Jacob Vosmaer's avatar Jacob Vosmaer Committed by Nick Thomas

Ensure JobWaiter keys always expire

parent 3df1baec
---
title: Ensure JobWaiter keys always expire
merge_request: 43320
author:
type: fixed
...@@ -23,7 +23,15 @@ module Gitlab ...@@ -23,7 +23,15 @@ module Gitlab
TIMEOUTS_METRIC = :gitlab_job_waiter_timeouts_total TIMEOUTS_METRIC = :gitlab_job_waiter_timeouts_total
def self.notify(key, jid) def self.notify(key, jid)
Gitlab::Redis::SharedState.with { |redis| redis.lpush(key, jid) } Gitlab::Redis::SharedState.with do |redis|
# Use a Redis MULTI transaction to ensure we always set an expiry
redis.multi do |multi|
multi.lpush(key, jid)
# This TTL needs to be long enough to allow whichever Sidekiq job calls
# JobWaiter#wait to reach BLPOP.
multi.expire(key, 6.hours.to_i)
end
end
end end
def self.key?(key) def self.key?(key)
...@@ -52,10 +60,6 @@ module Gitlab ...@@ -52,10 +60,6 @@ module Gitlab
increment_counter(STARTED_METRIC) increment_counter(STARTED_METRIC)
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
# Fallback key expiry: allow a long grace period to reduce the chance of
# a job pushing to an expired key and recreating it
redis.expire(key, [timeout * 2, 10.minutes.to_i].max)
while jobs_remaining > 0 while jobs_remaining > 0
# Redis will not take fractional seconds. Prefer waiting too long over # Redis will not take fractional seconds. Prefer waiting too long over
# not waiting long enough # not waiting long enough
...@@ -75,9 +79,6 @@ module Gitlab ...@@ -75,9 +79,6 @@ module Gitlab
@finished << jid @finished << jid
@jobs_remaining -= 1 @jobs_remaining -= 1
end end
# All jobs have finished, so expire the key immediately
redis.expire(key, 0) if jobs_remaining == 0
end end
finished finished
......
...@@ -2,17 +2,16 @@ ...@@ -2,17 +2,16 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::JobWaiter do RSpec.describe Gitlab::JobWaiter, :redis do
describe '.notify' do describe '.notify' do
it 'pushes the jid to the named queue' do it 'pushes the jid to the named queue' do
key = 'gitlab:job_waiter:foo' key = described_class.new.key
jid = 1
redis = double('redis') described_class.notify(key, 123)
expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
expect(redis).to receive(:lpush).with(key, jid)
described_class.notify(key, jid) Gitlab::Redis::SharedState.with do |redis|
expect(redis.ttl(key)).to be > 0
end
end 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