Skip to content

Fill/validate new sharding-related fields in ci_runner_machines

Pedro Pombeiro requested to merge pedropombeiro/497526/fill-ci_runner_machines-sharding-columns into master

What does this MR do and why?

This MR adds logic to fill the runner_type and sharding_key_id fields in new ci_runner_machines records, when e.g. they are retrieved as part of runner pings (POST /api/v4/runners/verify).

This logic was first introduced in !168069 (merged) and reverted in !168575 (merged) due to a fault in the model lookup logic (example) which caused an S2 incident. The problem was that RunnerManager.safe_find_or_create_by! was taking the runner_type and sharding_key_id values for creating a new record. However, since the table wasn't backfilled, the lookup would not find a record with the runner_type and sharding_key_id values, even though a potential match with nil values existed in the table. This resulted in a unique key constraint violation, which unfortunately didn't manifest itself in staging as it did in production (with the 404 errors).

This MR fixes the lookup and adds specs that reproduce the scenario of non-backfilled rows.

The previous code now fails with:

Failures:

  1) API::Ci::Helpers::Runner#current_runner_manager when runner manager already exists when a runner manager with nil runner_type and sharding_key_id already exists reuses and updates existing runner manager
     Got 1 failure and 1 other error:

     1.1) Failure/Error: expect { current_runner_manager }.not_to raise_error

            expected no Exception, got #<ActiveRecord::RecordNotFound: ActiveRecord::RecordNotFound> with backtrace:
              # ./app/models/application_record.rb:61:in `block in safe_find_or_create_by!'
              # ./app/models/application_record.rb:60:in `safe_find_or_create_by!'
              # ./app/models/ci/runner.rb:520:in `ensure_manager'
              # ./lib/api/ci/helpers/runner.rb:58:in `block in current_runner_manager'
              # ./gems/gitlab-utils/lib/gitlab/utils/strong_memoize.rb:34:in `strong_memoize'
              # ./lib/api/ci/helpers/runner.rb:56:in `current_runner_manager'
              # ./spec/lib/api/ci/helpers/runner_spec.rb:93:in `block (3 levels) in <main>'
              # ./spec/lib/api/ci/helpers/runner_spec.rb:116:in `block (6 levels) in <main>'
              # ./spec/lib/api/ci/helpers/runner_spec.rb:116:in `block (5 levels) in <main>'
              # ./spec/spec_helper.rb:475:in `block (3 levels) in <main>'

Part of #497526 (closed)

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Reintroduce the lookup code from the previous MR and run the tests.

Edited by Pedro Pombeiro

Merge request reports