diff --git a/app/models/integrations/jira.rb b/app/models/integrations/jira.rb index 37735b956fc2aadc227dffe231fa6cc1ca585981..22367ee336de42f29ae1284353809da9a7f90f3f 100644 --- a/app/models/integrations/jira.rb +++ b/app/models/integrations/jira.rb @@ -184,8 +184,8 @@ def options options end - def client - @client ||= JIRA::Client.new(options).tap do |client| + def client(additional_options = {}) + JIRA::Client.new(options.merge(additional_options)).tap do |client| # Replaces JIRA default http client with our implementation client.request_client = Gitlab::Jira::HttpClient.new(client.options) end diff --git a/app/workers/gitlab/jira_import/stage/import_issues_worker.rb b/app/workers/gitlab/jira_import/stage/import_issues_worker.rb index 7a5eb6c1e3acdb46f345b62dd6e2f484bba9fb5c..5d890ecfe1361898f361f1f8c81d20563640697c 100644 --- a/app/workers/gitlab/jira_import/stage/import_issues_worker.rb +++ b/app/workers/gitlab/jira_import/stage/import_issues_worker.rb @@ -9,7 +9,14 @@ class ImportIssuesWorker # rubocop:disable Scalability/IdempotentWorker private def import(project) - jobs_waiter = Gitlab::JiraImport::IssuesImporter.new(project).execute + jira_client = if Feature.enabled?(:increase_jira_import_issues_timeout) + project.jira_integration.client(read_timeout: 2.minutes) + end + + jobs_waiter = Gitlab::JiraImport::IssuesImporter.new( + project, + jira_client + ).execute project.latest_jira_import.refresh_jid_expiration diff --git a/config/feature_flags/development/increase_jira_import_issues_timeout.yml b/config/feature_flags/development/increase_jira_import_issues_timeout.yml new file mode 100644 index 0000000000000000000000000000000000000000..8c5b9e5566c984963acde76d6476675885694c1e --- /dev/null +++ b/config/feature_flags/development/increase_jira_import_issues_timeout.yml @@ -0,0 +1,8 @@ +--- +name: increase_jira_import_issues_timeout +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/135050 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429293 +milestone: '16.6' +type: development +group: group::project management +default_enabled: false diff --git a/lib/gitlab/jira/http_client.rb b/lib/gitlab/jira/http_client.rb index 7abfe8e38e82f9084e246c6dfa7675b6b426389f..2b8b01e202358114f0683bc365477643cf6692bc 100644 --- a/lib/gitlab/jira/http_client.rb +++ b/lib/gitlab/jira/http_client.rb @@ -34,6 +34,17 @@ def make_request(http_method, path, body = '', headers = {}) request_params[:headers][:Cookie] = get_cookies if options[:use_cookies] request_params[:base_uri] = uri.to_s request_params.merge!(auth_params) + # Setting defaults here so we can also set `timeout` which prevents setting defaults in the HTTP gem's code + request_params[:open_timeout] = options[:open_timeout] || default_timeout_for(:open_timeout) + request_params[:read_timeout] = options[:read_timeout] || default_timeout_for(:read_timeout) + request_params[:write_timeout] = options[:write_timeout] || default_timeout_for(:write_timeout) + # Global timeout. Needs to be at least as high as the maximum defined in other timeouts + request_params[:timeout] = [ + Gitlab::HTTP::DEFAULT_READ_TOTAL_TIMEOUT, + request_params[:open_timeout], + request_params[:read_timeout], + request_params[:write_timeout] + ].max result = Gitlab::HTTP.public_send(http_method, path, **request_params) # rubocop:disable GitlabSecurity/PublicSend @authenticated = result.response.is_a?(Net::HTTPOK) @@ -52,6 +63,10 @@ def make_request(http_method, path, body = '', headers = {}) private + def default_timeout_for(param) + Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS[param] + end + def auth_params return {} unless @options[:username] && @options[:password] diff --git a/lib/gitlab/jira_import/base_importer.rb b/lib/gitlab/jira_import/base_importer.rb index 2b83f0492cb040ffd5f822946e0054bf7efbe354..04ef1a0ef688102cf03b78fc23d59ce2efd61e08 100644 --- a/lib/gitlab/jira_import/base_importer.rb +++ b/lib/gitlab/jira_import/base_importer.rb @@ -5,7 +5,7 @@ module JiraImport class BaseImporter attr_reader :project, :client, :formatter, :jira_project_key, :running_import - def initialize(project) + def initialize(project, client = nil) Gitlab::JiraImport.validate_project_settings!(project) @running_import = project.latest_jira_import @@ -14,7 +14,7 @@ def initialize(project) raise Projects::ImportService::Error, _('Unable to find Jira project to import data from.') unless @jira_project_key @project = project - @client = project.jira_integration.client + @client = client || project.jira_integration.client @formatter = Gitlab::ImportFormatter.new end diff --git a/lib/gitlab/jira_import/issues_importer.rb b/lib/gitlab/jira_import/issues_importer.rb index 458f7c3f4707819c009d7f4247d38e7287f81a6a..54ececc49380e503ac1d90d2622c52cd30dba599 100644 --- a/lib/gitlab/jira_import/issues_importer.rb +++ b/lib/gitlab/jira_import/issues_importer.rb @@ -10,7 +10,7 @@ class IssuesImporter < BaseImporter attr_reader :imported_items_cache_key, :start_at, :job_waiter - def initialize(project) + def initialize(project, client = nil) super # get cached start_at value, or zero if not cached yet @start_at = Gitlab::JiraImport.get_issues_next_start_at(project.id) diff --git a/spec/models/integrations/jira_spec.rb b/spec/models/integrations/jira_spec.rb index d3ae92ea52a1246fa20b2874e1430e4bb995dedc..af021c51035ac7065423dc58d2e08f1ccf26eaa1 100644 --- a/spec/models/integrations/jira_spec.rb +++ b/spec/models/integrations/jira_spec.rb @@ -603,6 +603,17 @@ jira_integration.client.get('/foo') end + context 'when a custom read_timeout option is passed as an argument' do + it 'uses the default GitLab::HTTP timeouts plus a custom read_timeout' do + expected_timeouts = Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS.merge(read_timeout: 2.minutes, timeout: 2.minutes) + + expect(Gitlab::HTTP_V2::Client).to receive(:httparty_perform_request) + .with(Net::HTTP::Get, '/foo', hash_including(expected_timeouts)).and_call_original + + jira_integration.client(read_timeout: 2.minutes).get('/foo') + end + end + context 'with basic auth' do before do jira_integration.jira_auth_type = 0 diff --git a/spec/workers/gitlab/jira_import/stage/import_issues_worker_spec.rb b/spec/workers/gitlab/jira_import/stage/import_issues_worker_spec.rb index 594f961877024b8453478e834ffc447a8b0ae122..f9b03fc1b448b1702d1b738f55f2eb3d3ca51e43 100644 --- a/spec/workers/gitlab/jira_import/stage/import_issues_worker_spec.rb +++ b/spec/workers/gitlab/jira_import/stage/import_issues_worker_spec.rb @@ -25,7 +25,11 @@ end context 'when import started', :clean_gitlab_redis_cache do - let_it_be(:jira_integration) { create(:jira_integration, project: project) } + let(:job_waiter) { Gitlab::JobWaiter.new(2, 'some-job-key') } + + before_all do + create(:jira_integration, project: project) + end before do jira_import.start! @@ -34,6 +38,40 @@ end end + it 'uses a custom http client for the issues importer' do + jira_integration = project.jira_integration + client = instance_double(JIRA::Client) + issue_importer = instance_double(Gitlab::JiraImport::IssuesImporter) + + allow(Project).to receive(:find_by_id).with(project.id).and_return(project) + allow(issue_importer).to receive(:execute).and_return(job_waiter) + + expect(jira_integration).to receive(:client).with(read_timeout: 2.minutes).and_return(client) + expect(Gitlab::JiraImport::IssuesImporter).to receive(:new).with( + project, + client + ).and_return(issue_importer) + + described_class.new.perform(project.id) + end + + context 'when increase_jira_import_issues_timeout feature flag is disabled' do + before do + stub_feature_flags(increase_jira_import_issues_timeout: false) + end + + it 'does not provide a custom client to IssuesImporter' do + issue_importer = instance_double(Gitlab::JiraImport::IssuesImporter) + expect(Gitlab::JiraImport::IssuesImporter).to receive(:new).with( + instance_of(Project), + nil + ).and_return(issue_importer) + allow(issue_importer).to receive(:execute).and_return(job_waiter) + + described_class.new.perform(project.id) + end + end + context 'when start_at is nil' do it_behaves_like 'advance to next stage', :attachments end