Commit 15802cc4 authored by Valery Sizov's avatar Valery Sizov

Fix: Geo file downloads can block Sidekiq threads

It sets a timeout for downloads of Geo files
parent f007fc33
---
title: 'Fix: Geo file downloads can block Sidekiq threads'
merge_request:
author:
type: fixed
...@@ -8,7 +8,12 @@ module Gitlab ...@@ -8,7 +8,12 @@ module Gitlab
attr_reader :file_type, :file_id, :filename, :expected_checksum, :request_data, :resource attr_reader :file_type, :file_id, :filename, :expected_checksum, :request_data, :resource
TEMP_PREFIX = 'tmp_'.freeze TEMP_PREFIX = 'tmp_'
DOWNLOAD_TIMEOUT = {
connect: 60,
write: 60,
read: 60
}.freeze
def initialize(resource:, file_type:, file_id:, request_data:, expected_checksum: nil, filename: nil) def initialize(resource:, file_type:, file_id:, request_data:, expected_checksum: nil, filename: nil)
@resource = resource @resource = resource
...@@ -128,7 +133,7 @@ module Gitlab ...@@ -128,7 +133,7 @@ module Gitlab
begin begin
# Make the request # Make the request
response = ::HTTP.get(url, headers: req_headers) response = ::HTTP.timeout(DOWNLOAD_TIMEOUT.dup).get(url, headers: req_headers)
# Check for failures # Check for failures
unless response.status.success? unless response.status.success?
...@@ -183,7 +188,7 @@ module Gitlab ...@@ -183,7 +188,7 @@ module Gitlab
begin begin
# Make the request # Make the request
response = ::HTTP.follow.get(url, headers: req_headers) response = ::HTTP.timeout(DOWNLOAD_TIMEOUT.dup).follow.get(url, headers: req_headers)
# Check for failures # Check for failures
unless response.status.success? unless response.status.success?
......
...@@ -23,6 +23,29 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do ...@@ -23,6 +23,29 @@ RSpec.describe Gitlab::Geo::Replication::BaseTransfer do
end end
end end
describe 'timeout' do
subject do
described_class.new(file_type: :avatar, file_id: 1, filename: Tempfile.new,
expected_checksum: nil, request_data: nil, resource: nil)
end
before do
stub_current_geo_node(secondary_node)
end
it 'sets a timeout when downbloads to local storage' do
expect(::HTTP).to receive(:timeout)
subject.download_from_primary
end
it 'sets a timeout when streaming to object storage' do
expect(::HTTP).to receive(:timeout)
subject.stream_from_primary_to_object_storage
end
end
describe '#can_transfer?' do describe '#can_transfer?' do
subject do subject do
described_class.new(file_type: :avatar, file_id: 1, filename: Tempfile.new, described_class.new(file_type: :avatar, file_id: 1, filename: Tempfile.new,
......
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