Skip to content

[E2E] Increase retry for commit to sub-group project via the API

John McDonnell requested to merge jmd/harden-user-inherited-access-spec into master

What does this MR do and why?

[E2E] Increase retry for commit to sub-group project via the API

The current retry durations do not appear to be sufficient as we are continuing to return 403s on occasion. By increasing the number of retries, we will give sidekiq a better chance to process jobs currently in queue.

There's an element of trial an error here to determine if allowing more time to retry will help here as we don't actually know exactly what sidekiq's queues look like when these failures are occurring. However 10secs is clearly not sufficient right now, and an increase to 60sec for this spec should have minimal overhead if the spec is going to fail anyways, so I think it's worth exploring this option here.

Related to #435058 (closed)
Related to #422686 (closed)
Related to #399016 (closed)

How to set up and validate locally

  1. It's somewhat difficult to predict how to replicate the exact failures we're seeing in this test is, but to verify that the retry mechanism itself is working we can add some latency to sidekiq, I found it easiest to do this by adding middleware to simulate a sleep to make sidekiq slow (see below diff)
  2. bundle exec rspec qa/specs/features/api/9_data_stores/user_inherited_access_spec.rb
    1. Upon running specs you'll notice 403 no permission to push this this branch errors being caught but the retry mechanism will help with help in these situations.
    2. Monitoring api/v4/sidekiq/compound_metrics can be useful to assess what the queues are doing. If the default.backlog queue grows excessively large it's likely that the test will fail as the jobs to set the permissions may not process in time
Sample diff to apply delay to sidekiq
diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb
index 8df12671f265..39cdea7e73f7 100644
--- a/config/initializers/sidekiq.rb
+++ b/config/initializers/sidekiq.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 module SidekiqLogArguments
   def self.enabled?
     Gitlab::Utils.to_boolean(ENV['SIDEKIQ_LOG_ARGUMENTS'], default: true)
@@ -32,7 +33,17 @@ def enable_semi_reliable_fetch_mode?
 
 enable_json_logs = Gitlab.config.sidekiq.log_format != 'text'
 
+class DelayedProcessingMiddleware
+  def call(_worker, _job, _queue)
+    sleep(2)
+    yield
+  end
+end
+
 Sidekiq.configure_server do |config|
+  config.server_middleware do |chain|
+    chain.add DelayedProcessingMiddleware
+  end
   config[:strict] = false
   config[:queues] = Gitlab::SidekiqConfig.expand_queues(config[:queues])
 

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Sanad Liaquat

Merge request reports