Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
1
Merge Requests
1
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
nexedi
gitlab-ce
Commits
3d94b504
Commit
3d94b504
authored
3 years ago
by
Alex Kalderimis
Committed by
Matthias Käppler
3 years ago
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Move write operations to a separate worker
See merge request gitlab-org/gitlab!62761
parent
2f97b0fd
Changes
8
Hide whitespace changes
Inline
Side-by-side
Showing
8 changed files
with
111 additions
and
30 deletions
+111
-30
app/services/web_hook_service.rb
app/services/web_hook_service.rb
+13
-10
app/services/web_hooks/log_execution_service.rb
app/services/web_hooks/log_execution_service.rb
+35
-0
app/workers/all_queues.yml
app/workers/all_queues.yml
+9
-0
app/workers/web_hook_worker.rb
app/workers/web_hook_worker.rb
+1
-1
app/workers/web_hooks/log_execution_worker.rb
app/workers/web_hooks/log_execution_worker.rb
+24
-0
config/sidekiq_queues.yml
config/sidekiq_queues.yml
+2
-0
spec/services/web_hook_service_spec.rb
spec/services/web_hook_service_spec.rb
+26
-18
spec/workers/web_hook_worker_spec.rb
spec/workers/web_hook_worker_spec.rb
+1
-1
No files found.
app/services/web_hook_service.rb
View file @
3d94b504
...
...
@@ -29,15 +29,17 @@ class WebHookService
GITLAB_EVENT_HEADER
=
'X-Gitlab-Event'
attr_accessor
:hook
,
:data
,
:hook_name
,
:request_options
attr_reader
:uniqueness_token
def
self
.
hook_to_event
(
hook_name
)
hook_name
.
to_s
.
singularize
.
titleize
end
def
initialize
(
hook
,
data
,
hook_name
)
def
initialize
(
hook
,
data
,
hook_name
,
uniqueness_token
=
nil
)
@hook
=
hook
@data
=
data
@hook_name
=
hook_name
.
to_s
@uniqueness_token
=
uniqueness_token
@request_options
=
{
timeout:
Gitlab
.
config
.
gitlab
.
webhook_timeout
,
allow_local_requests:
hook
.
allow_local_requests?
...
...
@@ -123,10 +125,8 @@ class WebHookService
end
def
log_execution
(
trigger
:,
url
:,
request_data
:,
response
:,
execution_duration
:,
error_message:
nil
)
handle_failure
(
response
,
hook
)
WebHookLog
.
create
(
web_hook:
hook
,
category
=
response_category
(
response
)
log_data
=
{
trigger:
trigger
,
url:
url
,
execution_duration:
execution_duration
,
...
...
@@ -136,16 +136,19 @@ class WebHookService
response_body:
safe_response_body
(
response
),
response_status:
response
.
code
,
internal_error_message:
error_message
)
}
::
WebHooks
::
LogExecutionWorker
.
perform_async
(
hook
.
id
,
log_data
,
category
,
uniqueness_token
)
end
def
handle_failure
(
response
,
hook
)
def
response_category
(
response
)
if
response
.
success?
||
response
.
redirection?
hook
.
enable!
:ok
elsif
response
.
internal_server_error?
hook
.
backoff!
:error
else
hook
.
failed!
:failed
end
end
...
...
This diff is collapsed.
Click to expand it.
app/services/web_hooks/log_execution_service.rb
0 → 100644
View file @
3d94b504
# frozen_string_literal: true
module
WebHooks
class
LogExecutionService
attr_reader
:hook
,
:log_data
,
:response_category
def
initialize
(
hook
:,
log_data
:,
response_category
:)
@hook
=
hook
@log_data
=
log_data
@response_category
=
response_category
end
def
execute
update_hook_executability
log_execution
end
private
def
log_execution
WebHookLog
.
create!
(
web_hook:
hook
,
**
log_data
.
transform_keys
(
&
:to_sym
))
end
def
update_hook_executability
case
response_category
when
:ok
hook
.
enable!
when
:error
hook
.
backoff!
when
:failed
hook
.
failed!
end
end
end
end
This diff is collapsed.
Click to expand it.
app/workers/all_queues.yml
View file @
3d94b504
...
...
@@ -2828,6 +2828,15 @@
:idempotent:
true
:tags:
- :exclude_from_kubernetes
-
:name: web_hooks_log_execution
:worker_name: WebHooks::LogExecutionWorker
:feature_category: :integrations
:has_external_dependencies:
:urgency: :low
:resource_boundary: :unknown
:weight:
1
:idempotent:
true
:tags: []
-
:name: wikis_git_garbage_collect
:worker_name: Wikis::GitGarbageCollectWorker
:feature_category: :gitaly
...
...
This diff is collapsed.
Click to expand it.
app/workers/web_hook_worker.rb
View file @
3d94b504
...
...
@@ -16,7 +16,7 @@ class WebHookWorker
hook
=
WebHook
.
find
(
hook_id
)
data
=
data
.
with_indifferent_access
WebHookService
.
new
(
hook
,
data
,
hook_name
).
execute
WebHookService
.
new
(
hook
,
data
,
hook_name
,
jid
).
execute
end
end
# rubocop:enable Scalability/IdempotentWorker
This diff is collapsed.
Click to expand it.
app/workers/web_hooks/log_execution_worker.rb
0 → 100644
View file @
3d94b504
# frozen_string_literal: true
module
WebHooks
class
LogExecutionWorker
include
ApplicationWorker
idempotent!
feature_category
:integrations
urgency
:low
# This worker accepts an extra argument. This enables us to
# treat this worker as idempotent. Currently this is set to
# the Job ID (jid) of the parent worker.
def
perform
(
hook_id
,
log_data
,
response_category
,
_unique_by
)
hook
=
WebHook
.
find_by_id
(
hook_id
)
return
unless
hook
# hook has been deleted before we could run.
::
WebHooks
::
LogExecutionService
.
new
(
hook:
hook
,
log_data:
log_data
,
response_category:
response_category
.
to_sym
)
.
execute
end
end
end
This diff is collapsed.
Click to expand it.
config/sidekiq_queues.yml
View file @
3d94b504
...
...
@@ -400,6 +400,8 @@
-
1
-
-
web_hooks_destroy
-
1
-
-
web_hooks_log_execution
-
1
-
-
wikis_git_garbage_collect
-
1
-
-
x509_certificate_revoke
...
...
This diff is collapsed.
Click to expand it.
spec/services/web_hook_service_spec.rb
View file @
3d94b504
...
...
@@ -174,13 +174,19 @@ RSpec.describe WebHookService do
context
'execution logging'
do
let
(
:hook_log
)
{
project_hook
.
web_hook_logs
.
last
}
def
run_service
service_instance
.
execute
::
WebHooks
::
LogExecutionWorker
.
drain
project_hook
.
reload
end
context
'with success'
do
before
do
stub_full_request
(
project_hook
.
url
,
method: :post
).
to_return
(
status:
200
,
body:
'Success'
)
end
it
'log successful execution'
do
service_instance
.
execut
e
run_servic
e
expect
(
hook_log
.
trigger
).
to
eq
(
'push_hooks'
)
expect
(
hook_log
.
url
).
to
eq
(
project_hook
.
url
)
...
...
@@ -191,12 +197,16 @@ RSpec.describe WebHookService do
expect
(
hook_log
.
internal_error_message
).
to
be_nil
end
it
'does not log in the service itself'
do
expect
{
service_instance
.
execute
}.
not_to
change
(
::
WebHookLog
,
:count
)
end
it
'does not increment the failure count'
do
expect
{
service_instance
.
execut
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
expect
{
run_servic
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
end
it
'does not change the disabled_until attribute'
do
expect
{
service_instance
.
execut
e
}.
not_to
change
(
project_hook
,
:disabled_until
)
expect
{
run_servic
e
}.
not_to
change
(
project_hook
,
:disabled_until
)
end
context
'when the hook had previously failed'
do
...
...
@@ -205,7 +215,7 @@ RSpec.describe WebHookService do
end
it
'resets the failure count'
do
expect
{
service_instance
.
execut
e
}.
to
change
(
project_hook
,
:recent_failures
).
to
(
0
)
expect
{
run_servic
e
}.
to
change
(
project_hook
,
:recent_failures
).
to
(
0
)
end
end
end
...
...
@@ -216,7 +226,7 @@ RSpec.describe WebHookService do
end
it
'logs failed execution'
do
service_instance
.
execut
e
run_servic
e
expect
(
hook_log
).
to
have_attributes
(
trigger:
eq
(
'push_hooks'
),
...
...
@@ -230,17 +240,17 @@ RSpec.describe WebHookService do
end
it
'increments the failure count'
do
expect
{
service_instance
.
execut
e
}.
to
change
(
project_hook
,
:recent_failures
).
by
(
1
)
expect
{
run_servic
e
}.
to
change
(
project_hook
,
:recent_failures
).
by
(
1
)
end
it
'does not change the disabled_until attribute'
do
expect
{
service_instance
.
execut
e
}.
not_to
change
(
project_hook
,
:disabled_until
)
expect
{
run_servic
e
}.
not_to
change
(
project_hook
,
:disabled_until
)
end
it
'does not allow the failure count to overflow'
do
project_hook
.
update!
(
recent_failures:
32767
)
expect
{
service_instance
.
execut
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
expect
{
run_servic
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
end
context
'when the web_hooks_disable_failed FF is disabled'
do
...
...
@@ -252,7 +262,7 @@ RSpec.describe WebHookService do
it
'does not allow the failure count to overflow'
do
project_hook
.
update!
(
recent_failures:
32767
)
expect
{
service_instance
.
execut
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
expect
{
run_servic
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
end
end
end
...
...
@@ -263,7 +273,7 @@ RSpec.describe WebHookService do
end
it
'log failed execution'
do
service_instance
.
execut
e
run_servic
e
expect
(
hook_log
.
trigger
).
to
eq
(
'push_hooks'
)
expect
(
hook_log
.
url
).
to
eq
(
project_hook
.
url
)
...
...
@@ -275,17 +285,15 @@ RSpec.describe WebHookService do
end
it
'does not increment the failure count'
do
expect
{
service_instance
.
execut
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
expect
{
run_servic
e
}.
not_to
change
(
project_hook
,
:recent_failures
)
end
it
'backs off'
do
expect
(
project_hook
).
to
receive
(
:backoff!
).
and_call_original
expect
{
service_instance
.
execute
}.
to
change
(
project_hook
,
:disabled_until
)
expect
{
run_service
}.
to
change
(
project_hook
,
:disabled_until
)
end
it
'increases the backoff count'
do
expect
{
service_instance
.
execut
e
}.
to
change
(
project_hook
,
:backoff_count
).
by
(
1
)
expect
{
run_servic
e
}.
to
change
(
project_hook
,
:backoff_count
).
by
(
1
)
end
context
'when the previous cool-off was near the maximum'
do
...
...
@@ -294,7 +302,7 @@ RSpec.describe WebHookService do
end
it
'sets the disabled_until attribute'
do
expect
{
service_instance
.
execut
e
}.
to
change
(
project_hook
,
:disabled_until
).
to
(
1
.
day
.
from_now
)
expect
{
run_servic
e
}.
to
change
(
project_hook
,
:disabled_until
).
to
(
1
.
day
.
from_now
)
end
end
...
...
@@ -304,7 +312,7 @@ RSpec.describe WebHookService do
end
it
'sets the disabled_until attribute'
do
expect
{
service_instance
.
execut
e
}.
to
change
(
project_hook
,
:disabled_until
).
to
(
1
.
day
.
from_now
)
expect
{
run_servic
e
}.
to
change
(
project_hook
,
:disabled_until
).
to
(
1
.
day
.
from_now
)
end
end
end
...
...
@@ -312,7 +320,7 @@ RSpec.describe WebHookService do
context
'with unsafe response body'
do
before
do
stub_full_request
(
project_hook
.
url
,
method: :post
).
to_return
(
status:
200
,
body:
"
\xBB
"
)
service_instance
.
execut
e
run_servic
e
end
it
'log successful execution'
do
...
...
This diff is collapsed.
Click to expand it.
spec/workers/web_hook_worker_spec.rb
View file @
3d94b504
...
...
@@ -10,7 +10,7 @@ RSpec.describe WebHookWorker do
describe
'#perform'
do
it
'delegates to WebHookService'
do
expect_next
(
WebHookService
,
project_hook
,
data
.
with_indifferent_access
,
hook_name
).
to
receive
(
:execute
)
expect_next
(
WebHookService
,
project_hook
,
data
.
with_indifferent_access
,
hook_name
,
anything
).
to
receive
(
:execute
)
subject
.
perform
(
project_hook
.
id
,
data
,
hook_name
)
end
...
...
This diff is collapsed.
Click to expand it.
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment