From 1270fbca01b6578e0ca37f8890e330af2bd36625 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 28 Feb 2022 15:32:29 +1300 Subject: [PATCH 1/3] Conditionally raise when lease cannot be obtained When we fail to obtain a lease, only allow the error to be raised if the hook's failure state needs to be updated. This will have the effect that the worker that calls the service will only retry when it needs to. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/81770#note_856943572 --- .../web_hooks/log_execution_service.rb | 14 ++++++++ .../web_hooks/log_execution_service_spec.rb | 35 ++++++++++++++----- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/services/web_hooks/log_execution_service.rb b/app/services/web_hooks/log_execution_service.rb index 8a6ba1d23803d8..0eb6ed5d950b9a 100644 --- a/app/services/web_hooks/log_execution_service.rb +++ b/app/services/web_hooks/log_execution_service.rb @@ -42,10 +42,24 @@ def update_hook_failure_state hook.failed! end end + rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + raise if raise_lock_error? end def lock_name "web_hooks:update_hook_failure_state:#{hook.id}" end + + # Allow an error to be raised after failing to obtain a lease only if the hook + # is not already in the correct failure state. + def raise_lock_error? + hook.reset # Reload so properties are guaranteed to be current. + + if response_category == :ok + !hook.executable? + else + hook.executable? + end + end end end diff --git a/spec/services/web_hooks/log_execution_service_spec.rb b/spec/services/web_hooks/log_execution_service_spec.rb index 8dceba164326d1..774563b8f01ec0 100644 --- a/spec/services/web_hooks/log_execution_service_spec.rb +++ b/spec/services/web_hooks/log_execution_service_spec.rb @@ -34,16 +34,35 @@ expect(WebHookLog.recent.first).to have_attributes(data) end - it 'updates failure state using a lease that ensures fresh state is written' do - service = described_class.new(hook: project_hook, log_data: data, response_category: :error) - WebHook.find(project_hook.id).update!(backoff_count: 1) + context 'obtaining an exclusive lease' do + let(:lease_key) { "web_hooks:update_hook_failure_state:#{project_hook.id}" } - lease_key = "web_hooks:update_hook_failure_state:#{project_hook.id}" - lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL) + it 'updates failure state using a lease that ensures fresh state is written' do + service = described_class.new(hook: project_hook, log_data: data, response_category: :error) + WebHook.find(project_hook.id).update!(backoff_count: 1) - expect(lease).to receive(:try_obtain) - expect(lease).to receive(:cancel) - expect { service.execute }.to change { WebHook.find(project_hook.id).backoff_count }.to(2) + lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL) + + expect(lease).to receive(:try_obtain) + expect(lease).to receive(:cancel) + expect { service.execute }.to change { WebHook.find(project_hook.id).backoff_count }.to(2) + end + + context 'when a lease cannot be obtained' do + before do + stub_exclusive_lease_taken(lease_key, timeout: described_class::LOCK_TTL) + end + + it 'raises an error if the hook needs to be updated' do + WebHook.find(project_hook.id).disable! + + expect { service.execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + + it 'does not raise an error if the hook does not need to be updated' do + expect { service.execute }.not_to raise_error + end + end end context 'when response_category is :ok' do -- GitLab From a0dfa2c95520131ed0e3d75952fee6b10de9079f Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 2 Mar 2022 10:59:22 +1300 Subject: [PATCH 2/3] Add reviewer feedback --- .../web_hooks/log_execution_service.rb | 4 +-- .../web_hooks/log_execution_service_spec.rb | 33 +++++++++++++------ 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/app/services/web_hooks/log_execution_service.rb b/app/services/web_hooks/log_execution_service.rb index 0eb6ed5d950b9a..c63ff7aa18450b 100644 --- a/app/services/web_hooks/log_execution_service.rb +++ b/app/services/web_hooks/log_execution_service.rb @@ -43,6 +43,8 @@ def update_hook_failure_state end end rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + # Allow an error to be raised after failing to obtain a lease only if the hook + # is not already in the correct failure state. raise if raise_lock_error? end @@ -50,8 +52,6 @@ def lock_name "web_hooks:update_hook_failure_state:#{hook.id}" end - # Allow an error to be raised after failing to obtain a lease only if the hook - # is not already in the correct failure state. def raise_lock_error? hook.reset # Reload so properties are guaranteed to be current. diff --git a/spec/services/web_hooks/log_execution_service_spec.rb b/spec/services/web_hooks/log_execution_service_spec.rb index 774563b8f01ec0..7e9a8de2dee667 100644 --- a/spec/services/web_hooks/log_execution_service_spec.rb +++ b/spec/services/web_hooks/log_execution_service_spec.rb @@ -4,6 +4,7 @@ RSpec.describe WebHooks::LogExecutionService do include ExclusiveLeaseHelpers + using RSpec::Parameterized::TableSyntax describe '#execute' do around do |example| @@ -49,18 +50,30 @@ end context 'when a lease cannot be obtained' do - before do - stub_exclusive_lease_taken(lease_key, timeout: described_class::LOCK_TTL) - end - - it 'raises an error if the hook needs to be updated' do - WebHook.find(project_hook.id).disable! - - expect { service.execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + where(:response_category, :executable, :needs_updating) do + :ok | true | false + :ok | false | true + :failed | true | true + :failed | false | false + :error | true | true + :error | false | false end - it 'does not raise an error if the hook does not need to be updated' do - expect { service.execute }.not_to raise_error + with_them do + subject(:service) { described_class.new(hook: project_hook, log_data: data, response_category: response_category) } + + before do + stub_exclusive_lease_taken(lease_key, timeout: described_class::LOCK_TTL) + allow(project_hook).to receive(:executable?).and_return(executable) + end + + it 'raises an error if the hook needs to be updated' do + if needs_updating + expect { service.execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + else + expect { service.execute }.not_to raise_error + end + end end end end -- GitLab From 66322f613f99aaf1001e2b359677bb6681bfd6af Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 4 Mar 2022 16:40:12 +1300 Subject: [PATCH 3/3] Add reviewer feedback --- app/services/web_hooks/log_execution_service.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/services/web_hooks/log_execution_service.rb b/app/services/web_hooks/log_execution_service.rb index c63ff7aa18450b..04b9ff593825c2 100644 --- a/app/services/web_hooks/log_execution_service.rb +++ b/app/services/web_hooks/log_execution_service.rb @@ -43,8 +43,6 @@ def update_hook_failure_state end end rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError - # Allow an error to be raised after failing to obtain a lease only if the hook - # is not already in the correct failure state. raise if raise_lock_error? end @@ -52,14 +50,12 @@ def lock_name "web_hooks:update_hook_failure_state:#{hook.id}" end + # Allow an error to be raised after failing to obtain a lease only if the hook + # is not already in the correct failure state. def raise_lock_error? hook.reset # Reload so properties are guaranteed to be current. - if response_category == :ok - !hook.executable? - else - hook.executable? - end + hook.executable? != (response_category == :ok) end end end -- GitLab