From 5047258b23d360fdeef53f7d6862257d8968294f Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 6 Mar 2019 20:39:25 +0200 Subject: [PATCH] Geo: Limit max backoff time by 1 hour, instead of 7 days This is an alternative solution for https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9553 We just limit max backoff time by 1 hour to mitigate https://gitlab.com/gitlab-org/gitlab-ee/issues/5348 --- ...ackoff-time-is-being-respected-option3.yml | 5 ++ ee/lib/delay.rb | 4 +- ee/spec/models/geo/project_registry_spec.rb | 22 ++++++++ .../geo/file_download_service_spec.rb | 55 ++++++++++++++++--- ...itory_verification_primary_service_spec.rb | 4 +- ...ory_verification_secondary_service_spec.rb | 4 +- 6 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 ee/changelogs/unreleased/5348-slower-sync-when-backoff-time-is-being-respected-option3.yml diff --git a/ee/changelogs/unreleased/5348-slower-sync-when-backoff-time-is-being-respected-option3.yml b/ee/changelogs/unreleased/5348-slower-sync-when-backoff-time-is-being-respected-option3.yml new file mode 100644 index 00000000000000..a71a0791d53076 --- /dev/null +++ b/ee/changelogs/unreleased/5348-slower-sync-when-backoff-time-is-being-respected-option3.yml @@ -0,0 +1,5 @@ +--- +title: 'Geo: Limit max backoff time by 1 hour, instead of 7 days' +merge_request: 9893 +author: +type: changed diff --git a/ee/lib/delay.rb b/ee/lib/delay.rb index c8bef613a185d2..81e143f10511d5 100644 --- a/ee/lib/delay.rb +++ b/ee/lib/delay.rb @@ -7,10 +7,10 @@ def delay(retry_count = 0) end # To prevent the retry time from storing invalid dates in the database, - # cap the max time to a week plus some random jitter value. + # cap the max time to a hour plus some random jitter value. def next_retry_time(retry_count) proposed_time = Time.now + delay(retry_count).seconds - max_future_time = Time.now + 7.days + delay(1).seconds + max_future_time = 1.hour.from_now + delay(1).seconds [proposed_time, max_future_time].min end diff --git a/ee/spec/models/geo/project_registry_spec.rb b/ee/spec/models/geo/project_registry_spec.rb index f9b36867db89a4..b277588f2c20cb 100644 --- a/ee/spec/models/geo/project_registry_spec.rb +++ b/ee/spec/models/geo/project_registry_spec.rb @@ -334,6 +334,17 @@ expect(subject.last_repository_synced_at).to be_like_time(Time.now) end + it 'ensures repository_retry_at is capped at one hour' do + subject.update(repository_retry_count: 31) + + subject.start_sync!(type) + + expect(subject).to have_attributes( + repository_retry_at: be_within(100.seconds).of(1.hour.from_now), + repository_retry_count: 32 + ) + end + shared_examples_for 'sets repository_retry_at to a future time' do it 'sets repository_retry_at to a future time' do subject.start_sync!(type) @@ -390,6 +401,17 @@ expect(subject.last_wiki_synced_at).to be_like_time(Time.now) end + it 'ensures wiki_retry_at is capped at one hour' do + subject.update(wiki_retry_count: 31) + + subject.start_sync!(type) + + expect(subject).to have_attributes( + wiki_retry_at: be_within(100.seconds).of(1.hour.from_now), + wiki_retry_count: 32 + ) + end + shared_examples_for 'sets wiki_retry_at to a future time' do it 'sets wiki_retry_at to a future time' do subject.start_sync!(type) diff --git a/ee/spec/services/geo/file_download_service_spec.rb b/ee/spec/services/geo/file_download_service_spec.rb index 6e488c27860c43..cc447d6f86de04 100644 --- a/ee/spec/services/geo/file_download_service_spec.rb +++ b/ee/spec/services/geo/file_download_service_spec.rb @@ -11,6 +11,45 @@ stub_current_geo_node(secondary) end + context 'retry time' do + before do + stub_transfer_result(bytes_downloaded: 0, success: false) + end + + shared_examples_for 'a service that sets next retry time capped at some value' do + it 'ensures the next retry time is capped properly' do + download_service.execute + + expect(registry_entry.reload).to have_attributes( + retry_at: be_within(100.seconds).of(1.hour.from_now), + retry_count: 32 + ) + end + end + + context 'with job_artifacts' do + let!(:registry_entry) do + create(:geo_job_artifact_registry, success: false, retry_count: 31, artifact_id: file.id) + end + + let(:file) { create(:ci_job_artifact) } + let(:download_service) { described_class.new('job_artifact', file.id) } + + it_behaves_like 'a service that sets next retry time capped at some value' + end + + context 'with uploads' do + let!(:registry_entry) do + create(:geo_file_registry, :avatar, success: false, file_id: file.id, retry_count: 31) + end + + let(:file) { create(:upload) } + let(:download_service) { described_class.new('avatar', file.id) } + + it_behaves_like 'a service that sets next retry time capped at some value' + end + end + shared_examples_for 'a service that handles orphaned uploads' do |file_type| let(:download_service) { described_class.new(file_type, file.id) } let(:registry) { Geo::FileRegistry } @@ -377,14 +416,14 @@ expect { described_class.new(:bad, 1).execute }.to raise_error(NameError) end end + end - def stub_transfer_result(bytes_downloaded:, success: false, primary_missing_file: false) - result = double(:transfer_result, - bytes_downloaded: bytes_downloaded, - success: success, - primary_missing_file: primary_missing_file) - instance = double("(instance of Gitlab::Geo::Transfer)", download_from_primary: result) - allow(Gitlab::Geo::Transfer).to receive(:new).and_return(instance) - end + def stub_transfer_result(bytes_downloaded:, success: false, primary_missing_file: false) + result = double(:transfer_result, + bytes_downloaded: bytes_downloaded, + success: success, + primary_missing_file: primary_missing_file) + instance = double("(instance of Gitlab::Geo::Transfer)", download_from_primary: result) + allow(Gitlab::Geo::Transfer).to receive(:new).and_return(instance) end end diff --git a/ee/spec/services/geo/repository_verification_primary_service_spec.rb b/ee/spec/services/geo/repository_verification_primary_service_spec.rb index d79d91edf50b29..2370ccb1f75842 100644 --- a/ee/spec/services/geo/repository_verification_primary_service_spec.rb +++ b/ee/spec/services/geo/repository_verification_primary_service_spec.rb @@ -223,9 +223,9 @@ wiki_verification_checksum: nil, last_wiki_verification_ran_at: be_present, last_wiki_verification_failure: 'Something went wrong with wiki', - repository_retry_at: be_within(100.seconds).of(Time.now + 7.days), + repository_retry_at: be_within(100.seconds).of(1.hour.from_now), repository_retry_count: 31, - wiki_retry_at: be_within(100.seconds).of(Time.now + 7.days), + wiki_retry_at: be_within(100.seconds).of(1.hour.from_now), wiki_retry_count: 31 ) end diff --git a/ee/spec/services/geo/repository_verification_secondary_service_spec.rb b/ee/spec/services/geo/repository_verification_secondary_service_spec.rb index 83ab5921914cda..c213ea83e96840 100644 --- a/ee/spec/services/geo/repository_verification_secondary_service_spec.rb +++ b/ee/spec/services/geo/repository_verification_secondary_service_spec.rb @@ -104,7 +104,7 @@ expect(registry).to have_attributes( "resync_#{type}" => true, - "#{type}_retry_at" => be_within(100.seconds).of(Time.now + 7.days), + "#{type}_retry_at" => be_within(100.seconds).of(1.hour.from_now), "#{type}_retry_count" => 31 ) end @@ -137,7 +137,7 @@ expect(registry).to have_attributes( "resync_#{type}" => true, - "#{type}_retry_at" => be_within(100.seconds).of(Time.now + 7.days), + "#{type}_retry_at" => be_within(100.seconds).of(1.hour.from_now), "#{type}_retry_count" => 31 ) end -- GitLab